| Message ID | 20221101003412.3842572-6-david@fromorbit.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/7] xfs: write page faults in iomap are not buffered writes | expand |
On Tue, Nov 01, 2022 at 11:34:10AM +1100, Dave Chinner wrote: > + /* > + * Now we have a locked folio, before we do anything with it we need to > + * check that the iomap we have cached is not stale. The inode extent > + * mapping can change due to concurrent IO in flight (e.g. > + * IOMAP_UNWRITTEN state can change and memory reclaim could have > + * reclaimed a previously partially written page at this index after IO > + * completion before this write reaches this file offset) and hence we > + * could do the wrong thing here (zero a page range incorrectly or fail > + * to zero) and corrupt data. > + */ > + if (ops->iomap_valid) { > + bool iomap_valid = ops->iomap_valid(iter->inode, &iter->iomap); > + > + if (!iomap_valid) { > + iter->iomap.flags |= IOMAP_F_STALE; > + status = 0; > + goto out_unlock; > + } > + } So the design so far has been that everything that applies at a page (or now folio) level goes into iomap_page_ops, not iomap_ops which is just the generic iteration, and I think we should probably do it that way. I'm a little disappointed that we need two callout almost next to each other, but given that we need to validate with the folio locked, and gfs2 wants the callback with the folio unlocked I think we have to do it that.
On Wed, Nov 02, 2022 at 01:36:41AM -0700, Christoph Hellwig wrote: > On Tue, Nov 01, 2022 at 11:34:10AM +1100, Dave Chinner wrote: > > + /* > > + * Now we have a locked folio, before we do anything with it we need to > > + * check that the iomap we have cached is not stale. The inode extent > > + * mapping can change due to concurrent IO in flight (e.g. > > + * IOMAP_UNWRITTEN state can change and memory reclaim could have > > + * reclaimed a previously partially written page at this index after IO > > + * completion before this write reaches this file offset) and hence we > > + * could do the wrong thing here (zero a page range incorrectly or fail > > + * to zero) and corrupt data. > > + */ > > + if (ops->iomap_valid) { > > + bool iomap_valid = ops->iomap_valid(iter->inode, &iter->iomap); > > + > > + if (!iomap_valid) { > > + iter->iomap.flags |= IOMAP_F_STALE; > > + status = 0; > > + goto out_unlock; > > + } > > + } > > So the design so far has been that everything that applies at a page (or > now folio) level goes into iomap_page_ops, not iomap_ops which is just > the generic iteration, and I think we should probably do it that way. I disagree here -- IMHO the sequence number is an attribute of the iomapping, not the folio. > I'm a little disappointed that we need two callout almost next to each > other, but given that we need to validate with the folio locked, and > gfs2 wants the callback with the folio unlocked I think we have to do > it that. <nod> --D
On Tue, Nov 01, 2022 at 11:34:10AM +1100, Dave Chinner wrote: > From: Dave Chinner <dchinner@redhat.com> > > A recent multithreaded write data corruption has been uncovered in > the iomap write code. The core of the problem is partial folio > writes can be flushed to disk while a new racing write can map it > and fill the rest of the page: > > writeback new write > > allocate blocks > blocks are unwritten > submit IO > ..... > map blocks > iomap indicates UNWRITTEN range > loop { > lock folio > copyin data > ..... > IO completes > runs unwritten extent conv > blocks are marked written > <iomap now stale> > get next folio > } > > Now add memory pressure such that memory reclaim evicts the > partially written folio that has already been written to disk. > > When the new write finally gets to the last partial page of the new > write, it does not find it in cache, so it instantiates a new page, > sees the iomap is unwritten, and zeros the part of the page that > it does not have data from. This overwrites the data on disk that > was originally written. > > The full description of the corruption mechanism can be found here: > > https://lore.kernel.org/linux-xfs/20220817093627.GZ3600936@dread.disaster.area/ > > To solve this problem, we need to check whether the iomap is still > valid after we lock each folio during the write. We have to do it > after we lock the page so that we don't end up with state changes > occurring while we wait for the folio to be locked. > > Hence we need a mechanism to be able to check that the cached iomap > is still valid (similar to what we already do in buffered > writeback), and we need a way for ->begin_write to back out and > tell the high level iomap iterator that we need to remap the > remaining write range. > > Signed-off-by: Dave Chinner <dchinner@redhat.com> > --- > fs/iomap/buffered-io.c | 53 +++++++++++++++++++++++++++++++++--------- > fs/iomap/iter.c | 19 ++++++++++++++- > include/linux/iomap.h | 17 ++++++++++++++ > 3 files changed, 77 insertions(+), 12 deletions(-) > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > index 91ee0b308e13..d3c565aa29f8 100644 > --- a/fs/iomap/buffered-io.c > +++ b/fs/iomap/buffered-io.c > @@ -584,8 +584,9 @@ static int iomap_write_begin_inline(const struct iomap_iter *iter, > return iomap_read_inline_data(iter, folio); > } > > -static int iomap_write_begin(const struct iomap_iter *iter, loff_t pos, > - size_t len, struct folio **foliop) > +static int iomap_write_begin(struct iomap_iter *iter, > + const struct iomap_ops *ops, loff_t pos, size_t len, I wonder if iomap_iter() should simply stuff its ops pointer into struct iomap_iter so that we don't have to pass around so many arguments? Eh, whatever, minor quibble for something that (so far) only applies to the buffered io path. > + struct folio **foliop) > { > const struct iomap_page_ops *page_ops = iter->iomap.page_ops; > const struct iomap *srcmap = iomap_iter_srcmap(iter); > @@ -618,6 +619,27 @@ static int iomap_write_begin(const struct iomap_iter *iter, loff_t pos, > status = (iter->flags & IOMAP_NOWAIT) ? -EAGAIN : -ENOMEM; > goto out_no_page; > } > + > + /* > + * Now we have a locked folio, before we do anything with it we need to > + * check that the iomap we have cached is not stale. The inode extent > + * mapping can change due to concurrent IO in flight (e.g. > + * IOMAP_UNWRITTEN state can change and memory reclaim could have > + * reclaimed a previously partially written page at this index after IO > + * completion before this write reaches this file offset) and hence we > + * could do the wrong thing here (zero a page range incorrectly or fail > + * to zero) and corrupt data. > + */ > + if (ops->iomap_valid) { > + bool iomap_valid = ops->iomap_valid(iter->inode, &iter->iomap); > + > + if (!iomap_valid) { > + iter->iomap.flags |= IOMAP_F_STALE; > + status = 0; > + goto out_unlock; > + } > + } The basic premise looks sound, but I wonder if a more efficient way to do this would be to add a u32 sequence counter and a u32 pointer to struct iomap and perform the comparison directly? I'm saying that purely from a post-spectre "minimize indirect calls in hot paths" perspective. From a design perspective the way you've structured this has a (IMHO) lower coupling factor. Anyway, that's an optimization that can happen later. > + > if (pos + len > folio_pos(folio) + folio_size(folio)) > len = folio_pos(folio) + folio_size(folio) - pos; > > @@ -727,7 +749,8 @@ static size_t iomap_write_end(struct iomap_iter *iter, loff_t pos, size_t len, > return ret; > } > > -static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i) > +static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i, > + const struct iomap_ops *ops) > { > loff_t length = iomap_length(iter); > loff_t pos = iter->pos; > @@ -770,9 +793,11 @@ static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i) > break; > } > > - status = iomap_write_begin(iter, pos, bytes, &folio); > + status = iomap_write_begin(iter, ops, pos, bytes, &folio); > if (unlikely(status)) > break; > + if (iter->iomap.flags & IOMAP_F_STALE) > + break; > > page = folio_file_page(folio, pos >> PAGE_SHIFT); > if (mapping_writably_mapped(mapping)) > @@ -825,14 +850,15 @@ iomap_file_buffered_write(struct kiocb *iocb, struct iov_iter *i, > iter.flags |= IOMAP_NOWAIT; > > while ((ret = iomap_iter(&iter, ops)) > 0) > - iter.processed = iomap_write_iter(&iter, i); > + iter.processed = iomap_write_iter(&iter, i, ops); > if (iter.pos == iocb->ki_pos) > return ret; > return iter.pos - iocb->ki_pos; > } > EXPORT_SYMBOL_GPL(iomap_file_buffered_write); > > -static loff_t iomap_unshare_iter(struct iomap_iter *iter) > +static loff_t iomap_unshare_iter(struct iomap_iter *iter, > + const struct iomap_ops *ops) > { > struct iomap *iomap = &iter->iomap; > const struct iomap *srcmap = iomap_iter_srcmap(iter); > @@ -853,9 +879,11 @@ static loff_t iomap_unshare_iter(struct iomap_iter *iter) > unsigned long bytes = min_t(loff_t, PAGE_SIZE - offset, length); > struct folio *folio; > > - status = iomap_write_begin(iter, pos, bytes, &folio); > + status = iomap_write_begin(iter, ops, pos, bytes, &folio); > if (unlikely(status)) > return status; > + if (iter->iomap.flags & IOMAP_F_STALE) > + break; > > status = iomap_write_end(iter, pos, bytes, bytes, folio); > if (WARN_ON_ONCE(status == 0)) > @@ -886,12 +914,13 @@ iomap_file_unshare(struct inode *inode, loff_t pos, loff_t len, > int ret; > > while ((ret = iomap_iter(&iter, ops)) > 0) > - iter.processed = iomap_unshare_iter(&iter); > + iter.processed = iomap_unshare_iter(&iter, ops); > return ret; > } > EXPORT_SYMBOL_GPL(iomap_file_unshare); > > -static loff_t iomap_zero_iter(struct iomap_iter *iter, bool *did_zero) > +static loff_t iomap_zero_iter(struct iomap_iter *iter, > + const struct iomap_ops *ops, bool *did_zero) > { > const struct iomap *srcmap = iomap_iter_srcmap(iter); > loff_t pos = iter->pos; > @@ -908,9 +937,11 @@ static loff_t iomap_zero_iter(struct iomap_iter *iter, bool *did_zero) > size_t offset; > size_t bytes = min_t(u64, SIZE_MAX, length); > > - status = iomap_write_begin(iter, pos, bytes, &folio); > + status = iomap_write_begin(iter, ops, pos, bytes, &folio); > if (status) > return status; > + if (iter->iomap.flags & IOMAP_F_STALE) > + break; > > offset = offset_in_folio(folio, pos); > if (bytes > folio_size(folio) - offset) > @@ -946,7 +977,7 @@ iomap_zero_range(struct inode *inode, loff_t pos, loff_t len, bool *did_zero, > int ret; > > while ((ret = iomap_iter(&iter, ops)) > 0) > - iter.processed = iomap_zero_iter(&iter, did_zero); > + iter.processed = iomap_zero_iter(&iter, ops, did_zero); > return ret; > } > EXPORT_SYMBOL_GPL(iomap_zero_range); > diff --git a/fs/iomap/iter.c b/fs/iomap/iter.c > index a1c7592d2ade..79a0614eaab7 100644 > --- a/fs/iomap/iter.c > +++ b/fs/iomap/iter.c > @@ -7,12 +7,28 @@ > #include <linux/iomap.h> > #include "trace.h" > > +/* > + * Advance to the next range we need to map. > + * > + * If the iomap is marked IOMAP_F_STALE, it means the existing map was not fully > + * processed - it was aborted because the extent the iomap spanned may have been > + * changed during the operation. In this case, the iteration behaviour is to > + * remap the unprocessed range of the iter, and that means we may need to remap > + * even when we've made no progress (i.e. iter->processed = 0). Hence the > + * "finished iterating" case needs to distinguish between > + * (processed = 0) meaning we are done and (processed = 0 && stale) meaning we > + * need to remap the entire remaining range. > + */ > static inline int iomap_iter_advance(struct iomap_iter *iter) > { > + bool stale = iter->iomap.flags & IOMAP_F_STALE; > + > /* handle the previous iteration (if any) */ > if (iter->iomap.length) { > - if (iter->processed <= 0) > + if (iter->processed < 0) > return iter->processed; > + if (!iter->processed && !stale) > + return 0; > if (WARN_ON_ONCE(iter->processed > iomap_length(iter))) > return -EIO; > iter->pos += iter->processed; > @@ -33,6 +49,7 @@ static inline void iomap_iter_done(struct iomap_iter *iter) > WARN_ON_ONCE(iter->iomap.offset > iter->pos); > WARN_ON_ONCE(iter->iomap.length == 0); > WARN_ON_ONCE(iter->iomap.offset + iter->iomap.length <= iter->pos); > + WARN_ON_ONCE(iter->iomap.flags & IOMAP_F_STALE); > > trace_iomap_iter_dstmap(iter->inode, &iter->iomap); > if (iter->srcmap.type != IOMAP_HOLE) > diff --git a/include/linux/iomap.h b/include/linux/iomap.h > index 238a03087e17..308931f0840a 100644 > --- a/include/linux/iomap.h > +++ b/include/linux/iomap.h > @@ -62,8 +62,13 @@ struct vm_fault; > * > * IOMAP_F_SIZE_CHANGED indicates to the iomap_end method that the file size > * has changed as the result of this write operation. > + * > + * IOMAP_F_STALE indicates that the iomap is not valid any longer and the file > + * range it covers needs to be remapped by the high level before the operation > + * can proceed. > */ > #define IOMAP_F_SIZE_CHANGED 0x100 > +#define IOMAP_F_STALE 0x200 Nit: might it be time to convert these to "(1U << NN)" format? > /* > * Flags from 0x1000 up are for file system specific usage: > @@ -165,6 +170,18 @@ struct iomap_ops { > */ > int (*iomap_end)(struct inode *inode, loff_t pos, loff_t length, > ssize_t written, unsigned flags, struct iomap *iomap); > + > + /* > + * Check that the cached iomap still maps correctly to the filesystem's > + * internal extent map. FS internal extent maps can change while iomap > + * is iterating a cached iomap, so this hook allows iomap to detect that > + * the iomap needs to be refreshed during a long running write > + * operation. > + * > + * This is called with the folio over the specified file position > + * held locked by the iomap code. > + */ > + bool (*iomap_valid)(struct inode *inode, const struct iomap *iomap); Hmmm so now that I remembered that iomap_page_ops are passed back via struct iomap, now I'm drifting towards agreeing with hch that the function pointer should be there, not iomap_ops. (The mechanics of this patch look correct to me, fwiw.) --D > }; > > /** > -- > 2.37.2 >
On Wed, Nov 02, 2022 at 09:43:53AM -0700, Darrick J. Wong wrote: > On Wed, Nov 02, 2022 at 01:36:41AM -0700, Christoph Hellwig wrote: > > On Tue, Nov 01, 2022 at 11:34:10AM +1100, Dave Chinner wrote: > > > + /* > > > + * Now we have a locked folio, before we do anything with it we need to > > > + * check that the iomap we have cached is not stale. The inode extent > > > + * mapping can change due to concurrent IO in flight (e.g. > > > + * IOMAP_UNWRITTEN state can change and memory reclaim could have > > > + * reclaimed a previously partially written page at this index after IO > > > + * completion before this write reaches this file offset) and hence we > > > + * could do the wrong thing here (zero a page range incorrectly or fail > > > + * to zero) and corrupt data. > > > + */ > > > + if (ops->iomap_valid) { > > > + bool iomap_valid = ops->iomap_valid(iter->inode, &iter->iomap); > > > + > > > + if (!iomap_valid) { > > > + iter->iomap.flags |= IOMAP_F_STALE; > > > + status = 0; > > > + goto out_unlock; > > > + } > > > + } > > > > So the design so far has been that everything that applies at a page (or > > now folio) level goes into iomap_page_ops, not iomap_ops which is just > > the generic iteration, and I think we should probably do it that way. > > I disagree here -- IMHO the sequence number is an attribute of the > iomapping, not the folio. OFC now that I've reread iomap.h I realize that iomap_page_ops are passed back via struct iomap, so I withdraw this comment. --D > > I'm a little disappointed that we need two callout almost next to each > > other, but given that we need to validate with the folio locked, and > > gfs2 wants the callback with the folio unlocked I think we have to do > > it that. > > <nod> > > --D
On Wed, Nov 02, 2022 at 09:58:34AM -0700, Darrick J. Wong wrote: > On Wed, Nov 02, 2022 at 09:43:53AM -0700, Darrick J. Wong wrote: > > On Wed, Nov 02, 2022 at 01:36:41AM -0700, Christoph Hellwig wrote: > > > On Tue, Nov 01, 2022 at 11:34:10AM +1100, Dave Chinner wrote: > > > > + /* > > > > + * Now we have a locked folio, before we do anything with it we need to > > > > + * check that the iomap we have cached is not stale. The inode extent > > > > + * mapping can change due to concurrent IO in flight (e.g. > > > > + * IOMAP_UNWRITTEN state can change and memory reclaim could have > > > > + * reclaimed a previously partially written page at this index after IO > > > > + * completion before this write reaches this file offset) and hence we > > > > + * could do the wrong thing here (zero a page range incorrectly or fail > > > > + * to zero) and corrupt data. > > > > + */ > > > > + if (ops->iomap_valid) { > > > > + bool iomap_valid = ops->iomap_valid(iter->inode, &iter->iomap); > > > > + > > > > + if (!iomap_valid) { > > > > + iter->iomap.flags |= IOMAP_F_STALE; > > > > + status = 0; > > > > + goto out_unlock; > > > > + } > > > > + } > > > > > > So the design so far has been that everything that applies at a page (or > > > now folio) level goes into iomap_page_ops, not iomap_ops which is just > > > the generic iteration, and I think we should probably do it that way. > > > > I disagree here -- IMHO the sequence number is an attribute of the > > iomapping, not the folio. > > OFC now that I've reread iomap.h I realize that iomap_page_ops are > passed back via struct iomap, so I withdraw this comment. My first thought was to make this a page op, but I ended up deciding against that because it isn't operating on the folio at all. Perhaps I misunderstood what "page_ops" was actually intended for, because it seems that the existing hooks are to allow the filesystem to wrap per-folio operations with an external context, not to perform iomap-specific per-folio operations. I guess if I read "pageops" as "operations to perform on each folio in an operation", then validating the iomap is not stale once the folio is locked could be considered a page op. I think we could probably make that work for writeback, too, because we have the folio locked when we call ->map_blocks.... -Dave.
On Thu, Nov 03, 2022 at 11:35:15AM +1100, Dave Chinner wrote: > My first thought was to make this a page op, but I ended up deciding > against that because it isn't operating on the folio at all. > Perhaps I misunderstood what "page_ops" was actually intended for, > because it seems that the existing hooks are to allow the filesystem > to wrap per-folio operations with an external context, not to > perform iomap-specific per-folio operations. > > I guess if I read "pageops" as "operations to perform on each folio > in an operation", Yes, that was the idea behind it. iomap_ops is really just the iteration, and I've still not given up on the plan from willy to merge the two ops into a single next iterator eventually. So basic idea is iomap_ops - complete generic iterator, that just returns and finishes of an iomap. iomap_page_ops - callbacks for each folio (used to be page) > then validating the iomap is not stale once the > folio is locked could be considered a page op. I think we could > probably make that work for writeback, too, because we have the > folio locked when we call ->map_blocks.... .. and then separate ops for users of iomap, like the writeback code with iomap_writeback_ops.
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c index 91ee0b308e13..d3c565aa29f8 100644 --- a/fs/iomap/buffered-io.c +++ b/fs/iomap/buffered-io.c @@ -584,8 +584,9 @@ static int iomap_write_begin_inline(const struct iomap_iter *iter, return iomap_read_inline_data(iter, folio); } -static int iomap_write_begin(const struct iomap_iter *iter, loff_t pos, - size_t len, struct folio **foliop) +static int iomap_write_begin(struct iomap_iter *iter, + const struct iomap_ops *ops, loff_t pos, size_t len, + struct folio **foliop) { const struct iomap_page_ops *page_ops = iter->iomap.page_ops; const struct iomap *srcmap = iomap_iter_srcmap(iter); @@ -618,6 +619,27 @@ static int iomap_write_begin(const struct iomap_iter *iter, loff_t pos, status = (iter->flags & IOMAP_NOWAIT) ? -EAGAIN : -ENOMEM; goto out_no_page; } + + /* + * Now we have a locked folio, before we do anything with it we need to + * check that the iomap we have cached is not stale. The inode extent + * mapping can change due to concurrent IO in flight (e.g. + * IOMAP_UNWRITTEN state can change and memory reclaim could have + * reclaimed a previously partially written page at this index after IO + * completion before this write reaches this file offset) and hence we + * could do the wrong thing here (zero a page range incorrectly or fail + * to zero) and corrupt data. + */ + if (ops->iomap_valid) { + bool iomap_valid = ops->iomap_valid(iter->inode, &iter->iomap); + + if (!iomap_valid) { + iter->iomap.flags |= IOMAP_F_STALE; + status = 0; + goto out_unlock; + } + } + if (pos + len > folio_pos(folio) + folio_size(folio)) len = folio_pos(folio) + folio_size(folio) - pos; @@ -727,7 +749,8 @@ static size_t iomap_write_end(struct iomap_iter *iter, loff_t pos, size_t len, return ret; } -static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i) +static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i, + const struct iomap_ops *ops) { loff_t length = iomap_length(iter); loff_t pos = iter->pos; @@ -770,9 +793,11 @@ static loff_t iomap_write_iter(struct iomap_iter *iter, struct iov_iter *i) break; } - status = iomap_write_begin(iter, pos, bytes, &folio); + status = iomap_write_begin(iter, ops, pos, bytes, &folio); if (unlikely(status)) break; + if (iter->iomap.flags & IOMAP_F_STALE) + break; page = folio_file_page(folio, pos >> PAGE_SHIFT); if (mapping_writably_mapped(mapping)) @@ -825,14 +850,15 @@ iomap_file_buffered_write(struct kiocb *iocb, struct iov_iter *i, iter.flags |= IOMAP_NOWAIT; while ((ret = iomap_iter(&iter, ops)) > 0) - iter.processed = iomap_write_iter(&iter, i); + iter.processed = iomap_write_iter(&iter, i, ops); if (iter.pos == iocb->ki_pos) return ret; return iter.pos - iocb->ki_pos; } EXPORT_SYMBOL_GPL(iomap_file_buffered_write); -static loff_t iomap_unshare_iter(struct iomap_iter *iter) +static loff_t iomap_unshare_iter(struct iomap_iter *iter, + const struct iomap_ops *ops) { struct iomap *iomap = &iter->iomap; const struct iomap *srcmap = iomap_iter_srcmap(iter); @@ -853,9 +879,11 @@ static loff_t iomap_unshare_iter(struct iomap_iter *iter) unsigned long bytes = min_t(loff_t, PAGE_SIZE - offset, length); struct folio *folio; - status = iomap_write_begin(iter, pos, bytes, &folio); + status = iomap_write_begin(iter, ops, pos, bytes, &folio); if (unlikely(status)) return status; + if (iter->iomap.flags & IOMAP_F_STALE) + break; status = iomap_write_end(iter, pos, bytes, bytes, folio); if (WARN_ON_ONCE(status == 0)) @@ -886,12 +914,13 @@ iomap_file_unshare(struct inode *inode, loff_t pos, loff_t len, int ret; while ((ret = iomap_iter(&iter, ops)) > 0) - iter.processed = iomap_unshare_iter(&iter); + iter.processed = iomap_unshare_iter(&iter, ops); return ret; } EXPORT_SYMBOL_GPL(iomap_file_unshare); -static loff_t iomap_zero_iter(struct iomap_iter *iter, bool *did_zero) +static loff_t iomap_zero_iter(struct iomap_iter *iter, + const struct iomap_ops *ops, bool *did_zero) { const struct iomap *srcmap = iomap_iter_srcmap(iter); loff_t pos = iter->pos; @@ -908,9 +937,11 @@ static loff_t iomap_zero_iter(struct iomap_iter *iter, bool *did_zero) size_t offset; size_t bytes = min_t(u64, SIZE_MAX, length); - status = iomap_write_begin(iter, pos, bytes, &folio); + status = iomap_write_begin(iter, ops, pos, bytes, &folio); if (status) return status; + if (iter->iomap.flags & IOMAP_F_STALE) + break; offset = offset_in_folio(folio, pos); if (bytes > folio_size(folio) - offset) @@ -946,7 +977,7 @@ iomap_zero_range(struct inode *inode, loff_t pos, loff_t len, bool *did_zero, int ret; while ((ret = iomap_iter(&iter, ops)) > 0) - iter.processed = iomap_zero_iter(&iter, did_zero); + iter.processed = iomap_zero_iter(&iter, ops, did_zero); return ret; } EXPORT_SYMBOL_GPL(iomap_zero_range); diff --git a/fs/iomap/iter.c b/fs/iomap/iter.c index a1c7592d2ade..79a0614eaab7 100644 --- a/fs/iomap/iter.c +++ b/fs/iomap/iter.c @@ -7,12 +7,28 @@ #include <linux/iomap.h> #include "trace.h" +/* + * Advance to the next range we need to map. + * + * If the iomap is marked IOMAP_F_STALE, it means the existing map was not fully + * processed - it was aborted because the extent the iomap spanned may have been + * changed during the operation. In this case, the iteration behaviour is to + * remap the unprocessed range of the iter, and that means we may need to remap + * even when we've made no progress (i.e. iter->processed = 0). Hence the + * "finished iterating" case needs to distinguish between + * (processed = 0) meaning we are done and (processed = 0 && stale) meaning we + * need to remap the entire remaining range. + */ static inline int iomap_iter_advance(struct iomap_iter *iter) { + bool stale = iter->iomap.flags & IOMAP_F_STALE; + /* handle the previous iteration (if any) */ if (iter->iomap.length) { - if (iter->processed <= 0) + if (iter->processed < 0) return iter->processed; + if (!iter->processed && !stale) + return 0; if (WARN_ON_ONCE(iter->processed > iomap_length(iter))) return -EIO; iter->pos += iter->processed; @@ -33,6 +49,7 @@ static inline void iomap_iter_done(struct iomap_iter *iter) WARN_ON_ONCE(iter->iomap.offset > iter->pos); WARN_ON_ONCE(iter->iomap.length == 0); WARN_ON_ONCE(iter->iomap.offset + iter->iomap.length <= iter->pos); + WARN_ON_ONCE(iter->iomap.flags & IOMAP_F_STALE); trace_iomap_iter_dstmap(iter->inode, &iter->iomap); if (iter->srcmap.type != IOMAP_HOLE) diff --git a/include/linux/iomap.h b/include/linux/iomap.h index 238a03087e17..308931f0840a 100644 --- a/include/linux/iomap.h +++ b/include/linux/iomap.h @@ -62,8 +62,13 @@ struct vm_fault; * * IOMAP_F_SIZE_CHANGED indicates to the iomap_end method that the file size * has changed as the result of this write operation. + * + * IOMAP_F_STALE indicates that the iomap is not valid any longer and the file + * range it covers needs to be remapped by the high level before the operation + * can proceed. */ #define IOMAP_F_SIZE_CHANGED 0x100 +#define IOMAP_F_STALE 0x200 /* * Flags from 0x1000 up are for file system specific usage: @@ -165,6 +170,18 @@ struct iomap_ops { */ int (*iomap_end)(struct inode *inode, loff_t pos, loff_t length, ssize_t written, unsigned flags, struct iomap *iomap); + + /* + * Check that the cached iomap still maps correctly to the filesystem's + * internal extent map. FS internal extent maps can change while iomap + * is iterating a cached iomap, so this hook allows iomap to detect that + * the iomap needs to be refreshed during a long running write + * operation. + * + * This is called with the folio over the specified file position + * held locked by the iomap code. + */ + bool (*iomap_valid)(struct inode *inode, const struct iomap *iomap); }; /**