| Message ID | 20240828181912.41517-3-bfoster@redhat.com (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
| Series | iomap: flush dirty cache over unwritten mappings on zero range | expand |
On Wed, Aug 28, 2024 at 02:19:11PM -0400, Brian Foster wrote: > iomap_zero_range() flushes pagecache to mitigate consistency > problems with dirty pagecache and unwritten mappings. The flush is > unconditional over the entire range because checking pagecache state > after mapping lookup is racy with writeback and reclaim. There are > ways around this using iomap's mapping revalidation mechanism, but > this is not supported by all iomap based filesystems and so is not a > generic solution. > > There is another way around this limitation that is good enough to > filter the flush for most cases in practice. If we check for dirty > pagecache over the target range (instead of unconditionally flush), > we can keep track of whether the range was dirty before lookup and > defer the flush until/unless we see a combination of dirty cache > backed by an unwritten mapping. We don't necessarily know whether > the dirty cache was backed by the unwritten maping or some other > (written) part of the range, but the impliciation of a false > positive here is a spurious flush and thus relatively harmless. > > Note that we also flush for hole mappings because iomap_zero_range() > is used for partial folio zeroing in some cases. For example, if a > folio straddles EOF on a sub-page FSB size fs, the post-eof portion > is hole-backed and dirtied/written via mapped write, and then i_size > increases before writeback can occur (which otherwise zeroes the > post-eof portion of the EOF folio), then the folio becomes > inconsistent with disk until reclaimed. A flush in this case > executes partial zeroing from writeback, and iomap knows that there > is otherwise no I/O to submit for hole backed mappings. > > Signed-off-by: Brian Foster <bfoster@redhat.com> > --- > fs/iomap/buffered-io.c | 57 +++++++++++++++++++++++++++++++++++------- > 1 file changed, 48 insertions(+), 9 deletions(-) > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > index 3e846f43ff48..a6e897e6e303 100644 > --- a/fs/iomap/buffered-io.c > +++ b/fs/iomap/buffered-io.c > @@ -1393,16 +1393,47 @@ iomap_file_unshare(struct inode *inode, loff_t pos, loff_t len, > } > EXPORT_SYMBOL_GPL(iomap_file_unshare); > > -static loff_t iomap_zero_iter(struct iomap_iter *iter, bool *did_zero) > +/* > + * Flush the remaining range of the iter and mark the current mapping stale. > + * This is used when zero range sees an unwritten mapping that may have had > + * dirty pagecache over it. > + */ > +static inline int iomap_zero_iter_flush_and_stale(struct iomap_iter *i) > +{ > + struct address_space *mapping = i->inode->i_mapping; > + loff_t end = i->pos + i->len - 1; > + > + i->iomap.flags |= IOMAP_F_STALE; > + return filemap_write_and_wait_range(mapping, i->pos, end); > +} > + > +static loff_t iomap_zero_iter(struct iomap_iter *iter, bool *did_zero, > + bool *range_dirty) > { > const struct iomap *srcmap = iomap_iter_srcmap(iter); > loff_t pos = iter->pos; > loff_t length = iomap_length(iter); > loff_t written = 0; > > - /* already zeroed? we're done. */ > - if (srcmap->type == IOMAP_HOLE || srcmap->type == IOMAP_UNWRITTEN) > + /* > + * We can skip pre-zeroed mappings so long as either the mapping was > + * clean before we started or we've flushed at least once since. > + * Otherwise we don't know whether the current mapping had dirty > + * pagecache, so flush it now, stale the current mapping, and proceed > + * from there. > + * > + * The hole case is intentionally included because this is (ab)used to > + * handle partial folio zeroing in some cases. Hole backed post-eof > + * ranges can be dirtied via mapped write and the flush triggers > + * writeback time post-eof zeroing. > + */ > + if (srcmap->type == IOMAP_HOLE || srcmap->type == IOMAP_UNWRITTEN) { > + if (*range_dirty) { > + *range_dirty = false; > + return iomap_zero_iter_flush_and_stale(iter); > + } > return length; > + } > > do { > struct folio *folio; > @@ -1450,19 +1481,27 @@ iomap_zero_range(struct inode *inode, loff_t pos, loff_t len, bool *did_zero, > .flags = IOMAP_ZERO, > }; > int ret; > + bool range_dirty; > > /* > * Zero range wants to skip pre-zeroed (i.e. unwritten) mappings, but > * pagecache must be flushed to ensure stale data from previous > - * buffered writes is not exposed. > + * buffered writes is not exposed. A flush is only required for certain > + * types of mappings, but checking pagecache after mapping lookup is > + * racy with writeback and reclaim. > + * > + * Therefore, check the entire range first and pass along whether any > + * part of it is dirty. If so and an underlying mapping warrants it, > + * flush the cache at that point. This trades off the occasional false > + * positive (and spurious flush, if the dirty data and mapping don't > + * happen to overlap) for simplicity in handling a relatively uncommon > + * situation. > */ > - ret = filemap_write_and_wait_range(inode->i_mapping, > - pos, pos + len - 1); > - if (ret) > - return ret; > + range_dirty = filemap_range_needs_writeback(inode->i_mapping, > + pos, pos + len - 1); > > while ((ret = iomap_iter(&iter, ops)) > 0) > - iter.processed = iomap_zero_iter(&iter, did_zero); > + iter.processed = iomap_zero_iter(&iter, did_zero, &range_dirty); Style nit: Could we do this flush-and-stale from the loop body instead of passing pointers around? e.g. static inline bool iomap_zero_need_flush(const struct iomap_iter *i) { const struct iomap *srcmap = iomap_iter_srcmap(iter); return srcmap->type == IOMAP_HOLE || srcmap->type == IOMAP_UNWRITTEN; } static inline int iomap_zero_iter_flush(struct iomap_iter *i) { struct address_space *mapping = i->inode->i_mapping; loff_t end = i->pos + i->len - 1; i->iomap.flags |= IOMAP_F_STALE; return filemap_write_and_wait_range(mapping, i->pos, end); } and then: range_dirty = filemap_range_needs_writeback(...); while ((ret = iomap_iter(&iter, ops)) > 0) { if (range_dirty && iomap_zero_need_flush(&iter)) { /* * Zero range wants to skip pre-zeroed (i.e. * unwritten) mappings, but... */ range_dirty = false; iter.processed = iomap_zero_iter_flush(&iter); } else { iter.processed = iomap_zero_iter(&iter, did_zero); } } The logic looks correct and sensible. :) --D > return ret; > } > EXPORT_SYMBOL_GPL(iomap_zero_range); > -- > 2.45.0 > >
On Wed, Aug 28, 2024 at 03:44:20PM -0700, Darrick J. Wong wrote: > On Wed, Aug 28, 2024 at 02:19:11PM -0400, Brian Foster wrote: > > @@ -1450,19 +1481,27 @@ iomap_zero_range(struct inode *inode, loff_t pos, loff_t len, bool *did_zero, > > .flags = IOMAP_ZERO, > > }; > > int ret; > > + bool range_dirty; > > > > /* > > * Zero range wants to skip pre-zeroed (i.e. unwritten) mappings, but > > * pagecache must be flushed to ensure stale data from previous > > - * buffered writes is not exposed. > > + * buffered writes is not exposed. A flush is only required for certain > > + * types of mappings, but checking pagecache after mapping lookup is > > + * racy with writeback and reclaim. > > + * > > + * Therefore, check the entire range first and pass along whether any > > + * part of it is dirty. If so and an underlying mapping warrants it, > > + * flush the cache at that point. This trades off the occasional false > > + * positive (and spurious flush, if the dirty data and mapping don't > > + * happen to overlap) for simplicity in handling a relatively uncommon > > + * situation. > > */ > > - ret = filemap_write_and_wait_range(inode->i_mapping, > > - pos, pos + len - 1); > > - if (ret) > > - return ret; > > + range_dirty = filemap_range_needs_writeback(inode->i_mapping, > > + pos, pos + len - 1); > > > > while ((ret = iomap_iter(&iter, ops)) > 0) > > - iter.processed = iomap_zero_iter(&iter, did_zero); > > + iter.processed = iomap_zero_iter(&iter, did_zero, &range_dirty); > > Style nit: Could we do this flush-and-stale from the loop body instead > of passing pointers around? e.g. > > static inline bool iomap_zero_need_flush(const struct iomap_iter *i) > { > const struct iomap *srcmap = iomap_iter_srcmap(iter); > > return srcmap->type == IOMAP_HOLE || > srcmap->type == IOMAP_UNWRITTEN; > } > > static inline int iomap_zero_iter_flush(struct iomap_iter *i) > { > struct address_space *mapping = i->inode->i_mapping; > loff_t end = i->pos + i->len - 1; > > i->iomap.flags |= IOMAP_F_STALE; > return filemap_write_and_wait_range(mapping, i->pos, end); > } > > and then: > > range_dirty = filemap_range_needs_writeback(...); > > while ((ret = iomap_iter(&iter, ops)) > 0) { > if (range_dirty && iomap_zero_need_flush(&iter)) { > /* > * Zero range wants to skip pre-zeroed (i.e. > * unwritten) mappings, but... > */ > range_dirty = false; > iter.processed = iomap_zero_iter_flush(&iter); > } else { > iter.processed = iomap_zero_iter(&iter, did_zero); > } > } > > The logic looks correct and sensible. :) Yeah, I think this is better. However, the one thing that both versions have in common is that they don't explain -why- the iomap needs to be marked stale. So, something like: "When we flush the dirty data over the range, the extent state for the range will change. We need to to know that new state before performing any zeroing operations on the range. Hence we mark the iomap stale so that the iterator will remap this range and the next ieration pass will see the new extent state and perform the correct zeroing operation for the range." -Dave.
On Wed, Aug 28, 2024 at 03:44:20PM -0700, Darrick J. Wong wrote: > On Wed, Aug 28, 2024 at 02:19:11PM -0400, Brian Foster wrote: > > iomap_zero_range() flushes pagecache to mitigate consistency > > problems with dirty pagecache and unwritten mappings. The flush is > > unconditional over the entire range because checking pagecache state > > after mapping lookup is racy with writeback and reclaim. There are > > ways around this using iomap's mapping revalidation mechanism, but > > this is not supported by all iomap based filesystems and so is not a > > generic solution. > > > > There is another way around this limitation that is good enough to > > filter the flush for most cases in practice. If we check for dirty > > pagecache over the target range (instead of unconditionally flush), > > we can keep track of whether the range was dirty before lookup and > > defer the flush until/unless we see a combination of dirty cache > > backed by an unwritten mapping. We don't necessarily know whether > > the dirty cache was backed by the unwritten maping or some other > > (written) part of the range, but the impliciation of a false > > positive here is a spurious flush and thus relatively harmless. > > > > Note that we also flush for hole mappings because iomap_zero_range() > > is used for partial folio zeroing in some cases. For example, if a > > folio straddles EOF on a sub-page FSB size fs, the post-eof portion > > is hole-backed and dirtied/written via mapped write, and then i_size > > increases before writeback can occur (which otherwise zeroes the > > post-eof portion of the EOF folio), then the folio becomes > > inconsistent with disk until reclaimed. A flush in this case > > executes partial zeroing from writeback, and iomap knows that there > > is otherwise no I/O to submit for hole backed mappings. > > > > Signed-off-by: Brian Foster <bfoster@redhat.com> > > --- > > fs/iomap/buffered-io.c | 57 +++++++++++++++++++++++++++++++++++------- > > 1 file changed, 48 insertions(+), 9 deletions(-) > > > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > > index 3e846f43ff48..a6e897e6e303 100644 > > --- a/fs/iomap/buffered-io.c > > +++ b/fs/iomap/buffered-io.c > > @@ -1393,16 +1393,47 @@ iomap_file_unshare(struct inode *inode, loff_t pos, loff_t len, > > } > > EXPORT_SYMBOL_GPL(iomap_file_unshare); > > > > -static loff_t iomap_zero_iter(struct iomap_iter *iter, bool *did_zero) > > +/* > > + * Flush the remaining range of the iter and mark the current mapping stale. > > + * This is used when zero range sees an unwritten mapping that may have had > > + * dirty pagecache over it. > > + */ > > +static inline int iomap_zero_iter_flush_and_stale(struct iomap_iter *i) > > +{ > > + struct address_space *mapping = i->inode->i_mapping; > > + loff_t end = i->pos + i->len - 1; > > + > > + i->iomap.flags |= IOMAP_F_STALE; > > + return filemap_write_and_wait_range(mapping, i->pos, end); > > +} > > + > > +static loff_t iomap_zero_iter(struct iomap_iter *iter, bool *did_zero, > > + bool *range_dirty) > > { > > const struct iomap *srcmap = iomap_iter_srcmap(iter); > > loff_t pos = iter->pos; > > loff_t length = iomap_length(iter); > > loff_t written = 0; > > > > - /* already zeroed? we're done. */ > > - if (srcmap->type == IOMAP_HOLE || srcmap->type == IOMAP_UNWRITTEN) > > + /* > > + * We can skip pre-zeroed mappings so long as either the mapping was > > + * clean before we started or we've flushed at least once since. > > + * Otherwise we don't know whether the current mapping had dirty > > + * pagecache, so flush it now, stale the current mapping, and proceed > > + * from there. > > + * > > + * The hole case is intentionally included because this is (ab)used to > > + * handle partial folio zeroing in some cases. Hole backed post-eof > > + * ranges can be dirtied via mapped write and the flush triggers > > + * writeback time post-eof zeroing. > > + */ > > + if (srcmap->type == IOMAP_HOLE || srcmap->type == IOMAP_UNWRITTEN) { > > + if (*range_dirty) { > > + *range_dirty = false; > > + return iomap_zero_iter_flush_and_stale(iter); > > + } > > return length; > > + } > > > > do { > > struct folio *folio; > > @@ -1450,19 +1481,27 @@ iomap_zero_range(struct inode *inode, loff_t pos, loff_t len, bool *did_zero, > > .flags = IOMAP_ZERO, > > }; > > int ret; > > + bool range_dirty; > > > > /* > > * Zero range wants to skip pre-zeroed (i.e. unwritten) mappings, but > > * pagecache must be flushed to ensure stale data from previous > > - * buffered writes is not exposed. > > + * buffered writes is not exposed. A flush is only required for certain > > + * types of mappings, but checking pagecache after mapping lookup is > > + * racy with writeback and reclaim. > > + * > > + * Therefore, check the entire range first and pass along whether any > > + * part of it is dirty. If so and an underlying mapping warrants it, > > + * flush the cache at that point. This trades off the occasional false > > + * positive (and spurious flush, if the dirty data and mapping don't > > + * happen to overlap) for simplicity in handling a relatively uncommon > > + * situation. > > */ > > - ret = filemap_write_and_wait_range(inode->i_mapping, > > - pos, pos + len - 1); > > - if (ret) > > - return ret; > > + range_dirty = filemap_range_needs_writeback(inode->i_mapping, > > + pos, pos + len - 1); > > > > while ((ret = iomap_iter(&iter, ops)) > 0) > > - iter.processed = iomap_zero_iter(&iter, did_zero); > > + iter.processed = iomap_zero_iter(&iter, did_zero, &range_dirty); > > Style nit: Could we do this flush-and-stale from the loop body instead > of passing pointers around? e.g. > So FWIW, I had multiple other variations of this that used an IOMAP_DIRTY_CACHE flag on the iomap to track dirty pagecache for arbitrary operations. The flag could be set and cleared at the appropriate points as expected (for ops that care). To me, that's how I'd prefer to avoid just passing a pointer, but I intentionally factored that out to avoid using a flag for something that (for now) could be simplified to a local variable. OTOH, it is something that might be useful for the iomap seek data/hole implementations down the road. I've played with that a bit, but also have been trying to avoid getting too much into that rabbit hole for zero range. My thought was I'd reintroduce it and replace the range_dirty thing if/when it proved useful for multiple operations. > static inline bool iomap_zero_need_flush(const struct iomap_iter *i) > { > const struct iomap *srcmap = iomap_iter_srcmap(iter); > > return srcmap->type == IOMAP_HOLE || > srcmap->type == IOMAP_UNWRITTEN; > } The factoring looks mostly reasonable, but a couple things bug me that I'd like to see if we can resolve.. One is that this doesn't really indicate whether a flush is needed, because the dirty cache state is a critical part of that logic. I suppose we could rename it (to what?), but it also seems a little odd to have a helper just for mapping type checks. > > static inline int iomap_zero_iter_flush(struct iomap_iter *i) > { > struct address_space *mapping = i->inode->i_mapping; > loff_t end = i->pos + i->len - 1; > > i->iomap.flags |= IOMAP_F_STALE; > return filemap_write_and_wait_range(mapping, i->pos, end); > } > > and then: > > range_dirty = filemap_range_needs_writeback(...); > > while ((ret = iomap_iter(&iter, ops)) > 0) { > if (range_dirty && iomap_zero_need_flush(&iter)) { > /* > * Zero range wants to skip pre-zeroed (i.e. > * unwritten) mappings, but... > */ > range_dirty = false; > iter.processed = iomap_zero_iter_flush(&iter); > } else { > iter.processed = iomap_zero_iter(&iter, did_zero); > } The other is that the optimization logic is now split across multiple functions. I.e., iomap_zero_iter() has a landmine if ever called without doing the flush_and_stale() part first (a consideration if truncate_page() were ever open coded, for example). I wonder if a compromise might be to factor out the whole optimization into a separate helper rather than just the flush part (first via a prep patch), then the higher level loop ends up looking almost the same: while ((ret = iomap_iter(&iter, ops)) > 0) { /* special handling for already zeroed mappings */ if (srcmap->type == IOMAP_HOLE || srcmap->type == IOMAP_UNWRITTEN) iter.processed = iomap_zero_mapping_iter(&iter, &range_dirty); else iter.processed = iomap_zero_iter(&iter, did_zero); } That doesn't avoid passing the range_dirty pointer, but we just end up passing that instead of did_zero. Also as noted above, it could still be made to go away if the range_dirty check gets pushed down into the iomap_iter() path for more general use. Anyways those are just my thoughts. I'm of the mind that whatever factoring we do here may have to change if Dave's batched folio lookup/iteration idea pans out for fs' with validation support, so at the end of the day I'll change this to look exactly like you wrote it if it means the zeroing problem gets fixed. Thoughts or preference? Brian > } > > The logic looks correct and sensible. :) > > --D > > > return ret; > > } > > EXPORT_SYMBOL_GPL(iomap_zero_range); > > -- > > 2.45.0 > > > > >
On Thu, Aug 29, 2024 at 10:26:06AM +1000, Dave Chinner wrote: > On Wed, Aug 28, 2024 at 03:44:20PM -0700, Darrick J. Wong wrote: > > On Wed, Aug 28, 2024 at 02:19:11PM -0400, Brian Foster wrote: > > > @@ -1450,19 +1481,27 @@ iomap_zero_range(struct inode *inode, loff_t pos, loff_t len, bool *did_zero, > > > .flags = IOMAP_ZERO, > > > }; > > > int ret; > > > + bool range_dirty; > > > > > > /* > > > * Zero range wants to skip pre-zeroed (i.e. unwritten) mappings, but > > > * pagecache must be flushed to ensure stale data from previous > > > - * buffered writes is not exposed. > > > + * buffered writes is not exposed. A flush is only required for certain > > > + * types of mappings, but checking pagecache after mapping lookup is > > > + * racy with writeback and reclaim. > > > + * > > > + * Therefore, check the entire range first and pass along whether any > > > + * part of it is dirty. If so and an underlying mapping warrants it, > > > + * flush the cache at that point. This trades off the occasional false > > > + * positive (and spurious flush, if the dirty data and mapping don't > > > + * happen to overlap) for simplicity in handling a relatively uncommon > > > + * situation. > > > */ > > > - ret = filemap_write_and_wait_range(inode->i_mapping, > > > - pos, pos + len - 1); > > > - if (ret) > > > - return ret; > > > + range_dirty = filemap_range_needs_writeback(inode->i_mapping, > > > + pos, pos + len - 1); > > > > > > while ((ret = iomap_iter(&iter, ops)) > 0) > > > - iter.processed = iomap_zero_iter(&iter, did_zero); > > > + iter.processed = iomap_zero_iter(&iter, did_zero, &range_dirty); > > > > Style nit: Could we do this flush-and-stale from the loop body instead > > of passing pointers around? e.g. > > > > static inline bool iomap_zero_need_flush(const struct iomap_iter *i) > > { > > const struct iomap *srcmap = iomap_iter_srcmap(iter); > > > > return srcmap->type == IOMAP_HOLE || > > srcmap->type == IOMAP_UNWRITTEN; > > } > > > > static inline int iomap_zero_iter_flush(struct iomap_iter *i) > > { > > struct address_space *mapping = i->inode->i_mapping; > > loff_t end = i->pos + i->len - 1; > > > > i->iomap.flags |= IOMAP_F_STALE; > > return filemap_write_and_wait_range(mapping, i->pos, end); > > } > > > > and then: > > > > range_dirty = filemap_range_needs_writeback(...); > > > > while ((ret = iomap_iter(&iter, ops)) > 0) { > > if (range_dirty && iomap_zero_need_flush(&iter)) { > > /* > > * Zero range wants to skip pre-zeroed (i.e. > > * unwritten) mappings, but... > > */ > > range_dirty = false; > > iter.processed = iomap_zero_iter_flush(&iter); > > } else { > > iter.processed = iomap_zero_iter(&iter, did_zero); > > } > > } > > > > The logic looks correct and sensible. :) > > Yeah, I think this is better. > > However, the one thing that both versions have in common is that > they don't explain -why- the iomap needs to be marked stale. > So, something like: > > "When we flush the dirty data over the range, the extent state for > the range will change. We need to to know that new state before > performing any zeroing operations on the range. Hence we mark the > iomap stale so that the iterator will remap this range and the next > ieration pass will see the new extent state and perform the correct > zeroing operation for the range." > Sure, I'll update the comments however the factoring ultimately turns out. Thanks. Brian > -Dave. > > -- > Dave Chinner > david@fromorbit.com >
On Thu, Aug 29, 2024 at 11:03:57AM -0400, Brian Foster wrote: > On Wed, Aug 28, 2024 at 03:44:20PM -0700, Darrick J. Wong wrote: > > On Wed, Aug 28, 2024 at 02:19:11PM -0400, Brian Foster wrote: > > > iomap_zero_range() flushes pagecache to mitigate consistency > > > problems with dirty pagecache and unwritten mappings. The flush is > > > unconditional over the entire range because checking pagecache state > > > after mapping lookup is racy with writeback and reclaim. There are > > > ways around this using iomap's mapping revalidation mechanism, but > > > this is not supported by all iomap based filesystems and so is not a > > > generic solution. > > > > > > There is another way around this limitation that is good enough to > > > filter the flush for most cases in practice. If we check for dirty > > > pagecache over the target range (instead of unconditionally flush), > > > we can keep track of whether the range was dirty before lookup and > > > defer the flush until/unless we see a combination of dirty cache > > > backed by an unwritten mapping. We don't necessarily know whether > > > the dirty cache was backed by the unwritten maping or some other > > > (written) part of the range, but the impliciation of a false > > > positive here is a spurious flush and thus relatively harmless. > > > > > > Note that we also flush for hole mappings because iomap_zero_range() > > > is used for partial folio zeroing in some cases. For example, if a > > > folio straddles EOF on a sub-page FSB size fs, the post-eof portion > > > is hole-backed and dirtied/written via mapped write, and then i_size > > > increases before writeback can occur (which otherwise zeroes the > > > post-eof portion of the EOF folio), then the folio becomes > > > inconsistent with disk until reclaimed. A flush in this case > > > executes partial zeroing from writeback, and iomap knows that there > > > is otherwise no I/O to submit for hole backed mappings. > > > > > > Signed-off-by: Brian Foster <bfoster@redhat.com> > > > --- > > > fs/iomap/buffered-io.c | 57 +++++++++++++++++++++++++++++++++++------- > > > 1 file changed, 48 insertions(+), 9 deletions(-) > > > > > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > > > index 3e846f43ff48..a6e897e6e303 100644 > > > --- a/fs/iomap/buffered-io.c > > > +++ b/fs/iomap/buffered-io.c > > > @@ -1393,16 +1393,47 @@ iomap_file_unshare(struct inode *inode, loff_t pos, loff_t len, > > > } > > > EXPORT_SYMBOL_GPL(iomap_file_unshare); > > > > > > -static loff_t iomap_zero_iter(struct iomap_iter *iter, bool *did_zero) > > > +/* > > > + * Flush the remaining range of the iter and mark the current mapping stale. > > > + * This is used when zero range sees an unwritten mapping that may have had > > > + * dirty pagecache over it. > > > + */ > > > +static inline int iomap_zero_iter_flush_and_stale(struct iomap_iter *i) > > > +{ > > > + struct address_space *mapping = i->inode->i_mapping; > > > + loff_t end = i->pos + i->len - 1; > > > + > > > + i->iomap.flags |= IOMAP_F_STALE; > > > + return filemap_write_and_wait_range(mapping, i->pos, end); > > > +} > > > + > > > +static loff_t iomap_zero_iter(struct iomap_iter *iter, bool *did_zero, > > > + bool *range_dirty) > > > { > > > const struct iomap *srcmap = iomap_iter_srcmap(iter); > > > loff_t pos = iter->pos; > > > loff_t length = iomap_length(iter); > > > loff_t written = 0; > > > > > > - /* already zeroed? we're done. */ > > > - if (srcmap->type == IOMAP_HOLE || srcmap->type == IOMAP_UNWRITTEN) > > > + /* > > > + * We can skip pre-zeroed mappings so long as either the mapping was > > > + * clean before we started or we've flushed at least once since. > > > + * Otherwise we don't know whether the current mapping had dirty > > > + * pagecache, so flush it now, stale the current mapping, and proceed > > > + * from there. > > > + * > > > + * The hole case is intentionally included because this is (ab)used to > > > + * handle partial folio zeroing in some cases. Hole backed post-eof > > > + * ranges can be dirtied via mapped write and the flush triggers > > > + * writeback time post-eof zeroing. > > > + */ > > > + if (srcmap->type == IOMAP_HOLE || srcmap->type == IOMAP_UNWRITTEN) { > > > + if (*range_dirty) { > > > + *range_dirty = false; > > > + return iomap_zero_iter_flush_and_stale(iter); > > > + } > > > return length; > > > + } > > > > > > do { > > > struct folio *folio; > > > @@ -1450,19 +1481,27 @@ iomap_zero_range(struct inode *inode, loff_t pos, loff_t len, bool *did_zero, > > > .flags = IOMAP_ZERO, > > > }; > > > int ret; > > > + bool range_dirty; > > > > > > /* > > > * Zero range wants to skip pre-zeroed (i.e. unwritten) mappings, but > > > * pagecache must be flushed to ensure stale data from previous > > > - * buffered writes is not exposed. > > > + * buffered writes is not exposed. A flush is only required for certain > > > + * types of mappings, but checking pagecache after mapping lookup is > > > + * racy with writeback and reclaim. > > > + * > > > + * Therefore, check the entire range first and pass along whether any > > > + * part of it is dirty. If so and an underlying mapping warrants it, > > > + * flush the cache at that point. This trades off the occasional false > > > + * positive (and spurious flush, if the dirty data and mapping don't > > > + * happen to overlap) for simplicity in handling a relatively uncommon > > > + * situation. > > > */ > > > - ret = filemap_write_and_wait_range(inode->i_mapping, > > > - pos, pos + len - 1); > > > - if (ret) > > > - return ret; > > > + range_dirty = filemap_range_needs_writeback(inode->i_mapping, > > > + pos, pos + len - 1); > > > > > > while ((ret = iomap_iter(&iter, ops)) > 0) > > > - iter.processed = iomap_zero_iter(&iter, did_zero); > > > + iter.processed = iomap_zero_iter(&iter, did_zero, &range_dirty); > > > > Style nit: Could we do this flush-and-stale from the loop body instead > > of passing pointers around? e.g. > > > > So FWIW, I had multiple other variations of this that used an > IOMAP_DIRTY_CACHE flag on the iomap to track dirty pagecache for > arbitrary operations. The flag could be set and cleared at the > appropriate points as expected (for ops that care). > > To me, that's how I'd prefer to avoid just passing a pointer, but I > intentionally factored that out to avoid using a flag for something that > (for now) could be simplified to a local variable. OTOH, it is something > that might be useful for the iomap seek data/hole implementations down > the road. > > I've played with that a bit, but also have been trying to avoid getting > too much into that rabbit hole for zero range. My thought was I'd > reintroduce it and replace the range_dirty thing if/when it proved > useful for multiple operations. > > > static inline bool iomap_zero_need_flush(const struct iomap_iter *i) > > { > > const struct iomap *srcmap = iomap_iter_srcmap(iter); > > > > return srcmap->type == IOMAP_HOLE || > > srcmap->type == IOMAP_UNWRITTEN; > > } > > The factoring looks mostly reasonable, but a couple things bug me that > I'd like to see if we can resolve.. > > One is that this doesn't really indicate whether a flush is needed, > because the dirty cache state is a critical part of that logic. I > suppose we could rename it (to what?), but it also seems a little odd to > have a helper just for mapping type checks. > > > > > static inline int iomap_zero_iter_flush(struct iomap_iter *i) > > { > > struct address_space *mapping = i->inode->i_mapping; > > loff_t end = i->pos + i->len - 1; > > > > i->iomap.flags |= IOMAP_F_STALE; > > return filemap_write_and_wait_range(mapping, i->pos, end); > > } > > > > and then: > > > > range_dirty = filemap_range_needs_writeback(...); > > > > while ((ret = iomap_iter(&iter, ops)) > 0) { > > if (range_dirty && iomap_zero_need_flush(&iter)) { > > /* > > * Zero range wants to skip pre-zeroed (i.e. > > * unwritten) mappings, but... > > */ > > range_dirty = false; > > iter.processed = iomap_zero_iter_flush(&iter); > > } else { > > iter.processed = iomap_zero_iter(&iter, did_zero); > > } > > The other is that the optimization logic is now split across multiple > functions. I.e., iomap_zero_iter() has a landmine if ever called without > doing the flush_and_stale() part first (a consideration if > truncate_page() were ever open coded, for example). > > I wonder if a compromise might be to factor out the whole optimization > into a separate helper rather than just the flush part (first via a prep > patch), then the higher level loop ends up looking almost the same: > > while ((ret = iomap_iter(&iter, ops)) > 0) { > /* special handling for already zeroed mappings */ > if (srcmap->type == IOMAP_HOLE || srcmap->type == IOMAP_UNWRITTEN) > iter.processed = iomap_zero_mapping_iter(&iter, &range_dirty); > else > iter.processed = iomap_zero_iter(&iter, did_zero); > } > > That doesn't avoid passing the range_dirty pointer, but we just end up > passing that instead of did_zero. Also as noted above, it could still be > made to go away if the range_dirty check gets pushed down into the > iomap_iter() path for more general use. > FWIW, here's another variation of iomap_zero_range() that seems a bit closer to yours: range_dirty = filemap_range_needs_writeback(inode->i_mapping, pos, pos + len - 1); while ((ret = iomap_iter(&iter, ops)) > 0) { const struct iomap *srcmap = iomap_iter_srcmap(&iter); if (srcmap->type == IOMAP_HOLE || srcmap->type == IOMAP_UNWRITTEN) { iter.processed = iomap_length(&iter); if (range_dirty) { range_dirty = false; iter.processed = iomap_zero_iter_flush_and_stale(&iter); } continue; } iter.processed = iomap_zero_iter(&iter, did_zero); } This avoids passing around range_dirty, but keeps the optimization logic together. Hm? Brian > Anyways those are just my thoughts. I'm of the mind that whatever > factoring we do here may have to change if Dave's batched folio > lookup/iteration idea pans out for fs' with validation support, so at > the end of the day I'll change this to look exactly like you wrote it if > it means the zeroing problem gets fixed. Thoughts or preference? > > Brian > > > } > > > > The logic looks correct and sensible. :) > > > > --D > > > > > return ret; > > > } > > > EXPORT_SYMBOL_GPL(iomap_zero_range); > > > -- > > > 2.45.0 > > > > > > > > > >
On Thu, Aug 29, 2024 at 11:03:57AM -0400, Brian Foster wrote: > On Wed, Aug 28, 2024 at 03:44:20PM -0700, Darrick J. Wong wrote: > > On Wed, Aug 28, 2024 at 02:19:11PM -0400, Brian Foster wrote: > > > iomap_zero_range() flushes pagecache to mitigate consistency > > > problems with dirty pagecache and unwritten mappings. The flush is > > > unconditional over the entire range because checking pagecache state > > > after mapping lookup is racy with writeback and reclaim. There are > > > ways around this using iomap's mapping revalidation mechanism, but > > > this is not supported by all iomap based filesystems and so is not a > > > generic solution. > > > > > > There is another way around this limitation that is good enough to > > > filter the flush for most cases in practice. If we check for dirty > > > pagecache over the target range (instead of unconditionally flush), > > > we can keep track of whether the range was dirty before lookup and > > > defer the flush until/unless we see a combination of dirty cache > > > backed by an unwritten mapping. We don't necessarily know whether > > > the dirty cache was backed by the unwritten maping or some other > > > (written) part of the range, but the impliciation of a false > > > positive here is a spurious flush and thus relatively harmless. > > > > > > Note that we also flush for hole mappings because iomap_zero_range() > > > is used for partial folio zeroing in some cases. For example, if a > > > folio straddles EOF on a sub-page FSB size fs, the post-eof portion > > > is hole-backed and dirtied/written via mapped write, and then i_size > > > increases before writeback can occur (which otherwise zeroes the > > > post-eof portion of the EOF folio), then the folio becomes > > > inconsistent with disk until reclaimed. A flush in this case > > > executes partial zeroing from writeback, and iomap knows that there > > > is otherwise no I/O to submit for hole backed mappings. > > > > > > Signed-off-by: Brian Foster <bfoster@redhat.com> > > > --- > > > fs/iomap/buffered-io.c | 57 +++++++++++++++++++++++++++++++++++------- > > > 1 file changed, 48 insertions(+), 9 deletions(-) > > > > > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > > > index 3e846f43ff48..a6e897e6e303 100644 > > > --- a/fs/iomap/buffered-io.c > > > +++ b/fs/iomap/buffered-io.c > > > @@ -1393,16 +1393,47 @@ iomap_file_unshare(struct inode *inode, loff_t pos, loff_t len, > > > } > > > EXPORT_SYMBOL_GPL(iomap_file_unshare); > > > > > > -static loff_t iomap_zero_iter(struct iomap_iter *iter, bool *did_zero) > > > +/* > > > + * Flush the remaining range of the iter and mark the current mapping stale. > > > + * This is used when zero range sees an unwritten mapping that may have had > > > + * dirty pagecache over it. > > > + */ > > > +static inline int iomap_zero_iter_flush_and_stale(struct iomap_iter *i) > > > +{ > > > + struct address_space *mapping = i->inode->i_mapping; > > > + loff_t end = i->pos + i->len - 1; > > > + > > > + i->iomap.flags |= IOMAP_F_STALE; > > > + return filemap_write_and_wait_range(mapping, i->pos, end); > > > +} > > > + > > > +static loff_t iomap_zero_iter(struct iomap_iter *iter, bool *did_zero, > > > + bool *range_dirty) > > > { > > > const struct iomap *srcmap = iomap_iter_srcmap(iter); > > > loff_t pos = iter->pos; > > > loff_t length = iomap_length(iter); > > > loff_t written = 0; > > > > > > - /* already zeroed? we're done. */ > > > - if (srcmap->type == IOMAP_HOLE || srcmap->type == IOMAP_UNWRITTEN) > > > + /* > > > + * We can skip pre-zeroed mappings so long as either the mapping was > > > + * clean before we started or we've flushed at least once since. > > > + * Otherwise we don't know whether the current mapping had dirty > > > + * pagecache, so flush it now, stale the current mapping, and proceed > > > + * from there. > > > + * > > > + * The hole case is intentionally included because this is (ab)used to > > > + * handle partial folio zeroing in some cases. Hole backed post-eof > > > + * ranges can be dirtied via mapped write and the flush triggers > > > + * writeback time post-eof zeroing. > > > + */ > > > + if (srcmap->type == IOMAP_HOLE || srcmap->type == IOMAP_UNWRITTEN) { > > > + if (*range_dirty) { > > > + *range_dirty = false; > > > + return iomap_zero_iter_flush_and_stale(iter); > > > + } > > > return length; > > > + } > > > > > > do { > > > struct folio *folio; > > > @@ -1450,19 +1481,27 @@ iomap_zero_range(struct inode *inode, loff_t pos, loff_t len, bool *did_zero, > > > .flags = IOMAP_ZERO, > > > }; > > > int ret; > > > + bool range_dirty; > > > > > > /* > > > * Zero range wants to skip pre-zeroed (i.e. unwritten) mappings, but > > > * pagecache must be flushed to ensure stale data from previous > > > - * buffered writes is not exposed. > > > + * buffered writes is not exposed. A flush is only required for certain > > > + * types of mappings, but checking pagecache after mapping lookup is > > > + * racy with writeback and reclaim. > > > + * > > > + * Therefore, check the entire range first and pass along whether any > > > + * part of it is dirty. If so and an underlying mapping warrants it, > > > + * flush the cache at that point. This trades off the occasional false > > > + * positive (and spurious flush, if the dirty data and mapping don't > > > + * happen to overlap) for simplicity in handling a relatively uncommon > > > + * situation. > > > */ > > > - ret = filemap_write_and_wait_range(inode->i_mapping, > > > - pos, pos + len - 1); > > > - if (ret) > > > - return ret; > > > + range_dirty = filemap_range_needs_writeback(inode->i_mapping, > > > + pos, pos + len - 1); > > > > > > while ((ret = iomap_iter(&iter, ops)) > 0) > > > - iter.processed = iomap_zero_iter(&iter, did_zero); > > > + iter.processed = iomap_zero_iter(&iter, did_zero, &range_dirty); > > > > Style nit: Could we do this flush-and-stale from the loop body instead > > of passing pointers around? e.g. > > > > So FWIW, I had multiple other variations of this that used an > IOMAP_DIRTY_CACHE flag on the iomap to track dirty pagecache for > arbitrary operations. The flag could be set and cleared at the > appropriate points as expected (for ops that care). > > To me, that's how I'd prefer to avoid just passing a pointer, but I > intentionally factored that out to avoid using a flag for something that > (for now) could be simplified to a local variable. OTOH, it is something > that might be useful for the iomap seek data/hole implementations down > the road. <nod> We can always adjust again when we get there; for now a local variable sounds fine. > I've played with that a bit, but also have been trying to avoid getting > too much into that rabbit hole for zero range. My thought was I'd > reintroduce it and replace the range_dirty thing if/when it proved > useful for multiple operations. > > > static inline bool iomap_zero_need_flush(const struct iomap_iter *i) > > { > > const struct iomap *srcmap = iomap_iter_srcmap(iter); > > > > return srcmap->type == IOMAP_HOLE || > > srcmap->type == IOMAP_UNWRITTEN; > > } > > The factoring looks mostly reasonable, but a couple things bug me that > I'd like to see if we can resolve.. > > One is that this doesn't really indicate whether a flush is needed, > because the dirty cache state is a critical part of that logic. I > suppose we could rename it (to what?), but it also seems a little odd to > have a helper just for mapping type checks. I thought about passing range_dirty into iomap_zero_need_flush since it's a static inline function, but that just seemed unnecessary. > > static inline int iomap_zero_iter_flush(struct iomap_iter *i) > > { > > struct address_space *mapping = i->inode->i_mapping; > > loff_t end = i->pos + i->len - 1; > > > > i->iomap.flags |= IOMAP_F_STALE; > > return filemap_write_and_wait_range(mapping, i->pos, end); > > } > > > > and then: > > > > range_dirty = filemap_range_needs_writeback(...); > > > > while ((ret = iomap_iter(&iter, ops)) > 0) { > > if (range_dirty && iomap_zero_need_flush(&iter)) { > > /* > > * Zero range wants to skip pre-zeroed (i.e. > > * unwritten) mappings, but... > > */ > > range_dirty = false; > > iter.processed = iomap_zero_iter_flush(&iter); > > } else { > > iter.processed = iomap_zero_iter(&iter, did_zero); > > } > > The other is that the optimization logic is now split across multiple > functions. I.e., iomap_zero_iter() has a landmine if ever called without > doing the flush_and_stale() part first (a consideration if > truncate_page() were ever open coded, for example). _zero_iter is a static function, let's hope nobody does that. Though you're right, experience tells me that someone will try this eventually. That said, I see the merit of having one complete loop body function that knows how to handle all iomap types, since the others do that. > I wonder if a compromise might be to factor out the whole optimization > into a separate helper rather than just the flush part (first via a prep > patch), then the higher level loop ends up looking almost the same: > > while ((ret = iomap_iter(&iter, ops)) > 0) { > /* special handling for already zeroed mappings */ > if (srcmap->type == IOMAP_HOLE || srcmap->type == IOMAP_UNWRITTEN) > iter.processed = iomap_zero_mapping_iter(&iter, &range_dirty); > else > iter.processed = iomap_zero_iter(&iter, did_zero); > } > > That doesn't avoid passing the range_dirty pointer, but we just end up > passing that instead of did_zero. Also as noted above, it could still be > made to go away if the range_dirty check gets pushed down into the > iomap_iter() path for more general use. > > Anyways those are just my thoughts. I'm of the mind that whatever > factoring we do here may have to change if Dave's batched folio > lookup/iteration idea pans out for fs' with validation support, so at > the end of the day I'll change this to look exactly like you wrote it if > it means the zeroing problem gets fixed. Thoughts or preference? I'm ok with your original version now. Reviewed-by: Darrick J. Wong <djwong@kernel.org> --D > Brian > > > } > > > > The logic looks correct and sensible. :) > > > > --D > > > > > return ret; > > > } > > > EXPORT_SYMBOL_GPL(iomap_zero_range); > > > -- > > > 2.45.0 > > > > > > > > > >
On Thu, Aug 29, 2024 at 02:34:02PM -0700, Darrick J. Wong wrote: > On Thu, Aug 29, 2024 at 11:03:57AM -0400, Brian Foster wrote: > > On Wed, Aug 28, 2024 at 03:44:20PM -0700, Darrick J. Wong wrote: > > > On Wed, Aug 28, 2024 at 02:19:11PM -0400, Brian Foster wrote: > > > > iomap_zero_range() flushes pagecache to mitigate consistency > > > > problems with dirty pagecache and unwritten mappings. The flush is > > > > unconditional over the entire range because checking pagecache state > > > > after mapping lookup is racy with writeback and reclaim. There are > > > > ways around this using iomap's mapping revalidation mechanism, but > > > > this is not supported by all iomap based filesystems and so is not a > > > > generic solution. > > > > > > > > There is another way around this limitation that is good enough to > > > > filter the flush for most cases in practice. If we check for dirty > > > > pagecache over the target range (instead of unconditionally flush), > > > > we can keep track of whether the range was dirty before lookup and > > > > defer the flush until/unless we see a combination of dirty cache > > > > backed by an unwritten mapping. We don't necessarily know whether > > > > the dirty cache was backed by the unwritten maping or some other > > > > (written) part of the range, but the impliciation of a false > > > > positive here is a spurious flush and thus relatively harmless. > > > > > > > > Note that we also flush for hole mappings because iomap_zero_range() > > > > is used for partial folio zeroing in some cases. For example, if a > > > > folio straddles EOF on a sub-page FSB size fs, the post-eof portion > > > > is hole-backed and dirtied/written via mapped write, and then i_size > > > > increases before writeback can occur (which otherwise zeroes the > > > > post-eof portion of the EOF folio), then the folio becomes > > > > inconsistent with disk until reclaimed. A flush in this case > > > > executes partial zeroing from writeback, and iomap knows that there > > > > is otherwise no I/O to submit for hole backed mappings. > > > > > > > > Signed-off-by: Brian Foster <bfoster@redhat.com> > > > > --- > > > > fs/iomap/buffered-io.c | 57 +++++++++++++++++++++++++++++++++++------- > > > > 1 file changed, 48 insertions(+), 9 deletions(-) > > > > > > > > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c > > > > index 3e846f43ff48..a6e897e6e303 100644 > > > > --- a/fs/iomap/buffered-io.c > > > > +++ b/fs/iomap/buffered-io.c > > > > @@ -1393,16 +1393,47 @@ iomap_file_unshare(struct inode *inode, loff_t pos, loff_t len, > > > > } > > > > EXPORT_SYMBOL_GPL(iomap_file_unshare); > > > > > > > > -static loff_t iomap_zero_iter(struct iomap_iter *iter, bool *did_zero) > > > > +/* > > > > + * Flush the remaining range of the iter and mark the current mapping stale. > > > > + * This is used when zero range sees an unwritten mapping that may have had > > > > + * dirty pagecache over it. > > > > + */ > > > > +static inline int iomap_zero_iter_flush_and_stale(struct iomap_iter *i) > > > > +{ > > > > + struct address_space *mapping = i->inode->i_mapping; > > > > + loff_t end = i->pos + i->len - 1; > > > > + > > > > + i->iomap.flags |= IOMAP_F_STALE; > > > > + return filemap_write_and_wait_range(mapping, i->pos, end); > > > > +} > > > > + > > > > +static loff_t iomap_zero_iter(struct iomap_iter *iter, bool *did_zero, > > > > + bool *range_dirty) > > > > { > > > > const struct iomap *srcmap = iomap_iter_srcmap(iter); > > > > loff_t pos = iter->pos; > > > > loff_t length = iomap_length(iter); > > > > loff_t written = 0; > > > > > > > > - /* already zeroed? we're done. */ > > > > - if (srcmap->type == IOMAP_HOLE || srcmap->type == IOMAP_UNWRITTEN) > > > > + /* > > > > + * We can skip pre-zeroed mappings so long as either the mapping was > > > > + * clean before we started or we've flushed at least once since. > > > > + * Otherwise we don't know whether the current mapping had dirty > > > > + * pagecache, so flush it now, stale the current mapping, and proceed > > > > + * from there. > > > > + * > > > > + * The hole case is intentionally included because this is (ab)used to > > > > + * handle partial folio zeroing in some cases. Hole backed post-eof > > > > + * ranges can be dirtied via mapped write and the flush triggers > > > > + * writeback time post-eof zeroing. > > > > + */ > > > > + if (srcmap->type == IOMAP_HOLE || srcmap->type == IOMAP_UNWRITTEN) { > > > > + if (*range_dirty) { > > > > + *range_dirty = false; > > > > + return iomap_zero_iter_flush_and_stale(iter); > > > > + } > > > > return length; > > > > + } > > > > > > > > do { > > > > struct folio *folio; > > > > @@ -1450,19 +1481,27 @@ iomap_zero_range(struct inode *inode, loff_t pos, loff_t len, bool *did_zero, > > > > .flags = IOMAP_ZERO, > > > > }; > > > > int ret; > > > > + bool range_dirty; > > > > > > > > /* > > > > * Zero range wants to skip pre-zeroed (i.e. unwritten) mappings, but > > > > * pagecache must be flushed to ensure stale data from previous > > > > - * buffered writes is not exposed. > > > > + * buffered writes is not exposed. A flush is only required for certain > > > > + * types of mappings, but checking pagecache after mapping lookup is > > > > + * racy with writeback and reclaim. > > > > + * > > > > + * Therefore, check the entire range first and pass along whether any > > > > + * part of it is dirty. If so and an underlying mapping warrants it, > > > > + * flush the cache at that point. This trades off the occasional false > > > > + * positive (and spurious flush, if the dirty data and mapping don't > > > > + * happen to overlap) for simplicity in handling a relatively uncommon > > > > + * situation. > > > > */ > > > > - ret = filemap_write_and_wait_range(inode->i_mapping, > > > > - pos, pos + len - 1); > > > > - if (ret) > > > > - return ret; > > > > + range_dirty = filemap_range_needs_writeback(inode->i_mapping, > > > > + pos, pos + len - 1); > > > > > > > > while ((ret = iomap_iter(&iter, ops)) > 0) > > > > - iter.processed = iomap_zero_iter(&iter, did_zero); > > > > + iter.processed = iomap_zero_iter(&iter, did_zero, &range_dirty); > > > > > > Style nit: Could we do this flush-and-stale from the loop body instead > > > of passing pointers around? e.g. > > > > > > > So FWIW, I had multiple other variations of this that used an > > IOMAP_DIRTY_CACHE flag on the iomap to track dirty pagecache for > > arbitrary operations. The flag could be set and cleared at the > > appropriate points as expected (for ops that care). > > > > To me, that's how I'd prefer to avoid just passing a pointer, but I > > intentionally factored that out to avoid using a flag for something that > > (for now) could be simplified to a local variable. OTOH, it is something > > that might be useful for the iomap seek data/hole implementations down > > the road. > > <nod> We can always adjust again when we get there; for now a local > variable sounds fine. > > > I've played with that a bit, but also have been trying to avoid getting > > too much into that rabbit hole for zero range. My thought was I'd > > reintroduce it and replace the range_dirty thing if/when it proved > > useful for multiple operations. > > > > > static inline bool iomap_zero_need_flush(const struct iomap_iter *i) > > > { > > > const struct iomap *srcmap = iomap_iter_srcmap(iter); > > > > > > return srcmap->type == IOMAP_HOLE || > > > srcmap->type == IOMAP_UNWRITTEN; > > > } > > > > The factoring looks mostly reasonable, but a couple things bug me that > > I'd like to see if we can resolve.. > > > > One is that this doesn't really indicate whether a flush is needed, > > because the dirty cache state is a critical part of that logic. I > > suppose we could rename it (to what?), but it also seems a little odd to > > have a helper just for mapping type checks. > > I thought about passing range_dirty into iomap_zero_need_flush since > it's a static inline function, but that just seemed unnecessary. > > > > static inline int iomap_zero_iter_flush(struct iomap_iter *i) > > > { > > > struct address_space *mapping = i->inode->i_mapping; > > > loff_t end = i->pos + i->len - 1; > > > > > > i->iomap.flags |= IOMAP_F_STALE; > > > return filemap_write_and_wait_range(mapping, i->pos, end); > > > } > > > > > > and then: > > > > > > range_dirty = filemap_range_needs_writeback(...); > > > > > > while ((ret = iomap_iter(&iter, ops)) > 0) { > > > if (range_dirty && iomap_zero_need_flush(&iter)) { > > > /* > > > * Zero range wants to skip pre-zeroed (i.e. > > > * unwritten) mappings, but... > > > */ > > > range_dirty = false; > > > iter.processed = iomap_zero_iter_flush(&iter); > > > } else { > > > iter.processed = iomap_zero_iter(&iter, did_zero); > > > } > > > > The other is that the optimization logic is now split across multiple > > functions. I.e., iomap_zero_iter() has a landmine if ever called without > > doing the flush_and_stale() part first (a consideration if > > truncate_page() were ever open coded, for example). > > _zero_iter is a static function, let's hope nobody does that. Though > you're right, experience tells me that someone will try this > eventually. > Yeah, it's probably unlikely, but the fact I already had the open coded iomap_truncate_page() experiment (from the v1 thread) lying around that does pretty much this is what gave me pause. > That said, I see the merit of having one complete loop body function > that knows how to handle all iomap types, since the others do that. > > > I wonder if a compromise might be to factor out the whole optimization > > into a separate helper rather than just the flush part (first via a prep > > patch), then the higher level loop ends up looking almost the same: > > > > while ((ret = iomap_iter(&iter, ops)) > 0) { > > /* special handling for already zeroed mappings */ > > if (srcmap->type == IOMAP_HOLE || srcmap->type == IOMAP_UNWRITTEN) > > iter.processed = iomap_zero_mapping_iter(&iter, &range_dirty); > > else > > iter.processed = iomap_zero_iter(&iter, did_zero); > > } > > > > That doesn't avoid passing the range_dirty pointer, but we just end up > > passing that instead of did_zero. Also as noted above, it could still be > > made to go away if the range_dirty check gets pushed down into the > > iomap_iter() path for more general use. > > > > Anyways those are just my thoughts. I'm of the mind that whatever > > factoring we do here may have to change if Dave's batched folio > > lookup/iteration idea pans out for fs' with validation support, so at > > the end of the day I'll change this to look exactly like you wrote it if > > it means the zeroing problem gets fixed. Thoughts or preference? > > I'm ok with your original version now. > > Reviewed-by: Darrick J. Wong <djwong@kernel.org> > Thanks. I'll post a v3 just with Dave's comment updates then. Brian > --D > > > > Brian > > > > > } > > > > > > The logic looks correct and sensible. :) > > > > > > --D > > > > > > > return ret; > > > > } > > > > EXPORT_SYMBOL_GPL(iomap_zero_range); > > > > -- > > > > 2.45.0 > > > > > > > > > > > > > > > >
diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c index 3e846f43ff48..a6e897e6e303 100644 --- a/fs/iomap/buffered-io.c +++ b/fs/iomap/buffered-io.c @@ -1393,16 +1393,47 @@ iomap_file_unshare(struct inode *inode, loff_t pos, loff_t len, } EXPORT_SYMBOL_GPL(iomap_file_unshare); -static loff_t iomap_zero_iter(struct iomap_iter *iter, bool *did_zero) +/* + * Flush the remaining range of the iter and mark the current mapping stale. + * This is used when zero range sees an unwritten mapping that may have had + * dirty pagecache over it. + */ +static inline int iomap_zero_iter_flush_and_stale(struct iomap_iter *i) +{ + struct address_space *mapping = i->inode->i_mapping; + loff_t end = i->pos + i->len - 1; + + i->iomap.flags |= IOMAP_F_STALE; + return filemap_write_and_wait_range(mapping, i->pos, end); +} + +static loff_t iomap_zero_iter(struct iomap_iter *iter, bool *did_zero, + bool *range_dirty) { const struct iomap *srcmap = iomap_iter_srcmap(iter); loff_t pos = iter->pos; loff_t length = iomap_length(iter); loff_t written = 0; - /* already zeroed? we're done. */ - if (srcmap->type == IOMAP_HOLE || srcmap->type == IOMAP_UNWRITTEN) + /* + * We can skip pre-zeroed mappings so long as either the mapping was + * clean before we started or we've flushed at least once since. + * Otherwise we don't know whether the current mapping had dirty + * pagecache, so flush it now, stale the current mapping, and proceed + * from there. + * + * The hole case is intentionally included because this is (ab)used to + * handle partial folio zeroing in some cases. Hole backed post-eof + * ranges can be dirtied via mapped write and the flush triggers + * writeback time post-eof zeroing. + */ + if (srcmap->type == IOMAP_HOLE || srcmap->type == IOMAP_UNWRITTEN) { + if (*range_dirty) { + *range_dirty = false; + return iomap_zero_iter_flush_and_stale(iter); + } return length; + } do { struct folio *folio; @@ -1450,19 +1481,27 @@ iomap_zero_range(struct inode *inode, loff_t pos, loff_t len, bool *did_zero, .flags = IOMAP_ZERO, }; int ret; + bool range_dirty; /* * Zero range wants to skip pre-zeroed (i.e. unwritten) mappings, but * pagecache must be flushed to ensure stale data from previous - * buffered writes is not exposed. + * buffered writes is not exposed. A flush is only required for certain + * types of mappings, but checking pagecache after mapping lookup is + * racy with writeback and reclaim. + * + * Therefore, check the entire range first and pass along whether any + * part of it is dirty. If so and an underlying mapping warrants it, + * flush the cache at that point. This trades off the occasional false + * positive (and spurious flush, if the dirty data and mapping don't + * happen to overlap) for simplicity in handling a relatively uncommon + * situation. */ - ret = filemap_write_and_wait_range(inode->i_mapping, - pos, pos + len - 1); - if (ret) - return ret; + range_dirty = filemap_range_needs_writeback(inode->i_mapping, + pos, pos + len - 1); while ((ret = iomap_iter(&iter, ops)) > 0) - iter.processed = iomap_zero_iter(&iter, did_zero); + iter.processed = iomap_zero_iter(&iter, did_zero, &range_dirty); return ret; } EXPORT_SYMBOL_GPL(iomap_zero_range);
iomap_zero_range() flushes pagecache to mitigate consistency problems with dirty pagecache and unwritten mappings. The flush is unconditional over the entire range because checking pagecache state after mapping lookup is racy with writeback and reclaim. There are ways around this using iomap's mapping revalidation mechanism, but this is not supported by all iomap based filesystems and so is not a generic solution. There is another way around this limitation that is good enough to filter the flush for most cases in practice. If we check for dirty pagecache over the target range (instead of unconditionally flush), we can keep track of whether the range was dirty before lookup and defer the flush until/unless we see a combination of dirty cache backed by an unwritten mapping. We don't necessarily know whether the dirty cache was backed by the unwritten maping or some other (written) part of the range, but the impliciation of a false positive here is a spurious flush and thus relatively harmless. Note that we also flush for hole mappings because iomap_zero_range() is used for partial folio zeroing in some cases. For example, if a folio straddles EOF on a sub-page FSB size fs, the post-eof portion is hole-backed and dirtied/written via mapped write, and then i_size increases before writeback can occur (which otherwise zeroes the post-eof portion of the EOF folio), then the folio becomes inconsistent with disk until reclaimed. A flush in this case executes partial zeroing from writeback, and iomap knows that there is otherwise no I/O to submit for hole backed mappings. Signed-off-by: Brian Foster <bfoster@redhat.com> --- fs/iomap/buffered-io.c | 57 +++++++++++++++++++++++++++++++++++------- 1 file changed, 48 insertions(+), 9 deletions(-)