On Thu, Jul 18, 2024 at 09:02:11AM -0400, Brian Foster wrote:
> iomap_zero_range() does not correctly handle unwritten mappings with
> dirty folios in pagecache. It skips unwritten mappings
> unconditionally as if they were already zeroed, and thus potentially
> exposes stale data from a previous write if affected folios are not
> written back before the zero range.
>
> Most callers already flush the target range of the zero for
> unrelated, context specific reasons, so this problem is not
> necessarily prevalent. The known outliers (in XFS) are file
> extension via buffered write and truncate. The truncate path issues
> a flush to work around this iomap problem, but the file extension
> path does not and thus can expose stale data if current EOF is
> unaligned and has a dirty folio over an unwritten block.
>
> This patch implements a mechanism for making zero range pagecache
> aware for filesystems that support mapping validation (i.e.
> folio_ops->iomap_valid()). Instead of just skipping unwritten
> mappings, scan the corresponding pagecache range for dirty or
> writeback folios. If found, explicitly zero them via buffered write.
> Clean or uncached subranges of unwritten mappings are skipped, as
> before.
>
> The quirk with a post-iomap_begin() pagecache scan is that it is
> racy with writeback and reclaim activity. Even if the higher level
> code holds the invalidate lock, nothing prevents a dirty folio from
> being written back, cleaned, and even reclaimed sometime after
> iomap_begin() returns an unwritten map but before a pagecache scan
> might find the dirty folio. To handle this situation, we can rely on
> the fact that writeback completion converts unwritten extents in the
> fs before writeback state is cleared on the folio.
>
> This means that a pagecache scan followed by a mapping revalidate of
> an unwritten mapping should either find a dirty folio if it exists,
> or detect a mapping change if a dirty folio did exist and had been
> cleaned sometime before the scan but after the unwritten mapping was
> found. If the revalidation succeeds then we can safely assume
> nothing has been written back and skip the range. If the
> revalidation fails then we must assume any offset in the range could
> have been modified by writeback. In other words, we must be
> particularly careful to make sure that any uncached range we intend
> to skip does not make it into iter.processed until the mapping is
> revalidated.
>
> Altogether, this allows zero range to handle dirty folios over
> unwritten extents without needing to flush and wait for writeback
> completion.
>
> Signed-off-by: Brian Foster <bfoster@xxxxxxxxxx>
> ---
> fs/iomap/buffered-io.c | 53 +++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 50 insertions(+), 3 deletions(-)
>
> diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> index a9425170df72..ea1d396ef445 100644
> --- a/fs/iomap/buffered-io.c
> +++ b/fs/iomap/buffered-io.c
> @@ -1385,6 +1385,23 @@ iomap_file_unshare(struct inode *inode, loff_t pos, loff_t len,
> }
> EXPORT_SYMBOL_GPL(iomap_file_unshare);
>
> +/*
> + * Scan an unwritten mapping for dirty pagecache and return the length of the
> + * clean or uncached range leading up to it. This is the range that zeroing may
> + * skip once the mapping is validated.
> + */
> +static inline loff_t
> +iomap_zero_iter_unwritten(struct iomap_iter *iter, loff_t pos, loff_t length)
> +{
> + struct address_space *mapping = iter->inode->i_mapping;
> + loff_t fpos = pos;
> +
> + if (!filemap_range_has_writeback(mapping, &fpos, length))
> + return length;
> + /* fpos can be smaller if the start folio is dirty */
> + return max(fpos, pos) - pos;
I'm not sure this is safe. filemap_range_has_writeback() doesn't do
checks for invalidation races or that the folio is actually valid.
It also treats locked folios as dirty and a locked folio isn't
necessarily dirty. IOWs, this check assumes that we'll do all these
checks during the actual writeback operation that would follow this
check and so skip anything that might have given a false positive
here.
iomap_write_begin() doesn't do those sorts of check. If there's no
folio in the cache, it will simply instantiate a new one and dirty
it. If there's an existing folio, it will simply dirty it.
Hence I don't think a "is there a folio in this range that is a
potential writeback candidate" check is correct here. I think we
need to be more robust in determining if a cached folio in the range
exists and needs zeroing. Ideas on that to follow...
> static loff_t iomap_zero_iter(struct iomap_iter *iter, bool *did_zero)
> {
> const struct iomap *srcmap = iomap_iter_srcmap(iter);
> @@ -1393,16 +1410,46 @@ static loff_t iomap_zero_iter(struct iomap_iter *iter, bool *did_zero)
> loff_t written = 0;
>
> /* already zeroed? we're done. */
> - if (srcmap->type == IOMAP_HOLE || srcmap->type == IOMAP_UNWRITTEN)
> + if (srcmap->type == IOMAP_HOLE)
> return length;
>
> do {
> struct folio *folio;
> int status;
> size_t offset;
> - size_t bytes = min_t(u64, SIZE_MAX, length);
> + size_t bytes;
> + loff_t pending = 0;
> bool ret;
>
> + /*
> + * Determine the range of the unwritten mapping that is clean in
> + * pagecache. We can skip this range, but only if the mapping is
> + * still valid after the pagecache scan. This is because
> + * writeback may have cleaned folios after the mapping lookup
> + * but before we were able to find them here. If that occurs,
> + * then the mapping must now be stale and we must reprocess the
> + * range.
> + */
> + if (srcmap->type == IOMAP_UNWRITTEN) {
> + pending = iomap_zero_iter_unwritten(iter, pos, length);
> + if (pending == length) {
> + /* no dirty cache, revalidate and bounce as we're
> + * either done or the mapping is stale */
> + if (iomap_revalidate(iter))
> + written += pending;
Ok, this isn't really how the revalidation was supposed to be used
(i.e. it's not stabilising the data and page cache state first),
but it looks like works for this situation. We can use this. :)
> + break;
> + }
> +
> + /*
> + * Found a dirty folio. Update pos/length to point at
> + * it. written is updated only after the mapping is
> + * revalidated by iomap_write_begin().
> + */
> + pos += pending;
> + length -= pending;
> + }
> +
> + bytes = min_t(u64, SIZE_MAX, length);
> status = iomap_write_begin(iter, pos, bytes, &folio);
> if (status)
> return status;
We don't hold any references to the page cache between the
iomap_zero_iter_unwritten() and then the actual folio lookup in
iomap_write_begin() where we reference and lock the folio at the
given offset. e.g. we get a "dirty" hit because of a locked folio,
and that folio is clean and contains zeroes (e.g. mmap read,
readahead, etc). We now write zeroes to that folio and dirty it
when, we should actually be skipping it and leaving the range as
unwritten.
In previous patches that fixed this zeroing issue, this wasn't a
problem because it used the existing iomap page cache lookup
mechanisms from iomap_write_begin() to get referenced, locked folios
over the zeroing range. Instead of skipping potential page cache
holes, it prevented page cache instantiation over page cache holes
from occurring when zeroing unwritten extents and that skipped page
cache holes naturally.
https://lore.kernel.org/linux-xfs/20240529095206.2568162-2-yi.zhang@xxxxxxxxxxxxxxx/
This means the iteration would skip holes but still safely
revalidate the iomap once it found and locked a folio in the given
range. It could also then check the folio is dirty to determine if
zeroing was necessary. Yes, this means it iterated holes in the
range PAGE_SIZE by PAGE_SIZE to do lookups, so it was inefficient.
However, we could still use this filemap_range_has_writeback()
optimisation to skip unwritten extents that don't have any cached
pages over them completely, but ti don't think it is really safe to
use it during the iteration to discover regions that don't need
zeroing. i.e. the fast path is this:
if (srcmap->type == IOMAP_HOLE)
return length;
if (srcmap->type == IOMAP_UNWRITTEN &&
!filemap_range_has_writeback(mapping, pos, length)) {
if (!iomap_revalidate(iter))
return 0; /* stale mapping */
return length;
}
And the slow path does something similar to the above patch.
However, I'm still not sure that filemap_range_has_writeback() is
the right set of checks to use here.
I just thought of another option - after thinking about how you've
modified filemap_range_has_writeback() to increment the iomap
iterator position to skip empty ranges, I think we could actually
drive that inwards to the page cache lookup. We could use
use filemap_get_folios() instead of __filemap_get_folio() for
unwritten extent zeroing iteration. This would allow the page cache
lookup to return the first folio in the range as a referenced folio
which we can then lock and validate similar to __filemap_get_folio().
We can then increment the iterator position based on the folio
position, the iomap_write_begin() code will do the revalidation of
the iomap, and the zeroing code can then determine if zeroing is
needed based on whether the folio is dirty/writeback or not. As this
all happens under the folio lock, it is largely safe from both page
cache data and state and extent state changes racing with the
zeroing operation.
Thoughts?
-Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
