| Message ID | 20190901200836.14959-4-rgoldwyn@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Btrfs iomap | expand |
On Sun, Sep 01, 2019 at 03:08:24PM -0500, Goldwyn Rodrigues wrote: > + iomap_assert(!(iomap->flags & IOMAP_F_BUFFER_HEAD)); > + iomap_assert(srcmap->type == IOMAP_HOLE || srcmap->addr > 0); 0 can be a valid address in various file systems, so I don't think we can just exclude it. Then again COWing from a hole seems pointless, doesn't it? So just check for addr != IOMAP_NULL_ADDR here? > > @@ -961,7 +966,7 @@ iomap_zero_range_actor(struct inode *inode, loff_t pos, loff_t count, > if (IS_DAX(inode)) > status = iomap_dax_zero(pos, offset, bytes, iomap); > else > - status = iomap_zero(inode, pos, offset, bytes, iomap); > + status = iomap_zero(inode, pos, offset, bytes, iomap, srcmap); This introduces an > 80 character line.
On Mon, Sep 02, 2019 at 06:31:24PM +0200, Christoph Hellwig wrote: > On Sun, Sep 01, 2019 at 03:08:24PM -0500, Goldwyn Rodrigues wrote: > > > + iomap_assert(!(iomap->flags & IOMAP_F_BUFFER_HEAD)); > > + iomap_assert(srcmap->type == IOMAP_HOLE || srcmap->addr > 0); > > 0 can be a valid address in various file systems, so I don't think we > can just exclude it. Then again COWing from a hole seems pointless, > doesn't it? XFS does that if you set a cowextsize hint and a speculative cow preallocation ends up covering a hole. Granted I don't think there's much point in reading from a COW fork extent to fill in an unaligned buffered write since it /should/ just end up zero-filling the pagecache regardless of fork... but I don't see much harm in doing that. --D > So just check for addr != IOMAP_NULL_ADDR here? > > > > > @@ -961,7 +966,7 @@ iomap_zero_range_actor(struct inode *inode, loff_t pos, loff_t count, > > if (IS_DAX(inode)) > > status = iomap_dax_zero(pos, offset, bytes, iomap); > > else > > - status = iomap_zero(inode, pos, offset, bytes, iomap); > > + status = iomap_zero(inode, pos, offset, bytes, iomap, srcmap); > > This introduces an > 80 character line.
On Mon, Sep 02, 2019 at 10:01:09AM -0700, Darrick J. Wong wrote: > On Mon, Sep 02, 2019 at 06:31:24PM +0200, Christoph Hellwig wrote: > > On Sun, Sep 01, 2019 at 03:08:24PM -0500, Goldwyn Rodrigues wrote: > > > > > + iomap_assert(!(iomap->flags & IOMAP_F_BUFFER_HEAD)); > > > + iomap_assert(srcmap->type == IOMAP_HOLE || srcmap->addr > 0); > > > > 0 can be a valid address in various file systems, so I don't think we > > can just exclude it. Then again COWing from a hole seems pointless, > > doesn't it? > > XFS does that if you set a cowextsize hint and a speculative cow > preallocation ends up covering a hole. Granted I don't think there's > much point in reading from a COW fork extent to fill in an unaligned > buffered write since it /should/ just end up zero-filling the pagecache > regardless of fork... but I don't see much harm in doing that. That assumes you'd set the iomap-level COW flag for anything that writes to the COW fork in XFS. Which doesn't sound right to me - the iomap-level indicates that we actually need to read some data, which for a hole is rather pointless as you said.
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c index f27756c0b31c..99d63ba14d1b 100644 --- a/fs/iomap/buffered-io.c +++ b/fs/iomap/buffered-io.c @@ -581,7 +581,7 @@ __iomap_write_begin(struct inode *inode, loff_t pos, unsigned len, static int iomap_write_begin(struct inode *inode, loff_t pos, unsigned len, unsigned flags, - struct page **pagep, struct iomap *iomap) + struct page **pagep, struct iomap *iomap, struct iomap *srcmap) { const struct iomap_page_ops *page_ops = iomap->page_ops; pgoff_t index = pos >> PAGE_SHIFT; @@ -605,12 +605,17 @@ iomap_write_begin(struct inode *inode, loff_t pos, unsigned len, unsigned flags, goto out_no_page; } - if (iomap->type == IOMAP_INLINE) + if (iomap->type == IOMAP_INLINE) { iomap_read_inline_data(inode, page, iomap); - else if (iomap->flags & IOMAP_F_BUFFER_HEAD) + } else if (iomap->type == IOMAP_COW) { + iomap_assert(!(iomap->flags & IOMAP_F_BUFFER_HEAD)); + iomap_assert(srcmap->type == IOMAP_HOLE || srcmap->addr > 0); + status = __iomap_write_begin(inode, pos, len, page, srcmap); + } else if (iomap->flags & IOMAP_F_BUFFER_HEAD) { status = __block_write_begin_int(page, pos, len, NULL, iomap); - else + } else { status = __iomap_write_begin(inode, pos, len, page, iomap); + } if (unlikely(status)) goto out_unlock; @@ -772,7 +777,7 @@ iomap_write_actor(struct inode *inode, loff_t pos, loff_t length, void *data, } status = iomap_write_begin(inode, pos, bytes, flags, &page, - iomap); + iomap, srcmap); if (unlikely(status)) break; @@ -871,7 +876,7 @@ iomap_dirty_actor(struct inode *inode, loff_t pos, loff_t length, void *data, return PTR_ERR(rpage); status = iomap_write_begin(inode, pos, bytes, - AOP_FLAG_NOFS, &page, iomap); + AOP_FLAG_NOFS, &page, iomap, srcmap); put_page(rpage); if (unlikely(status)) return status; @@ -917,13 +922,13 @@ iomap_file_dirty(struct inode *inode, loff_t pos, loff_t len, EXPORT_SYMBOL_GPL(iomap_file_dirty); static int iomap_zero(struct inode *inode, loff_t pos, unsigned offset, - unsigned bytes, struct iomap *iomap) + unsigned bytes, struct iomap *iomap, struct iomap *srcmap) { struct page *page; int status; status = iomap_write_begin(inode, pos, bytes, AOP_FLAG_NOFS, &page, - iomap); + iomap, srcmap); if (status) return status; @@ -961,7 +966,7 @@ iomap_zero_range_actor(struct inode *inode, loff_t pos, loff_t count, if (IS_DAX(inode)) status = iomap_dax_zero(pos, offset, bytes, iomap); else - status = iomap_zero(inode, pos, offset, bytes, iomap); + status = iomap_zero(inode, pos, offset, bytes, iomap, srcmap); if (status < 0) return status;