On Tue, Nov 15, 2022 at 12:30:39PM +1100, Dave Chinner wrote:
> From: Dave Chinner <dchinner@xxxxxxxxxx>
>
...
>
> Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
> ---
> fs/xfs/xfs_iomap.c | 151 ++++++++++++++++++++++++++++++++++++++++++---
> 1 file changed, 141 insertions(+), 10 deletions(-)
>
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index 7bb55dbc19d3..2d48fcc7bd6f 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -1134,6 +1134,146 @@ xfs_buffered_write_delalloc_punch(
> end_fsb - start_fsb);
> }
>
...
> +/*
> + * Punch out all the delalloc blocks in the range given except for those that
> + * have dirty data still pending in the page cache - those are going to be
> + * written and so must still retain the delalloc backing for writeback.
> + *
> + * As we are scanning the page cache for data, we don't need to reimplement the
> + * wheel - mapping_seek_hole_data() does exactly what we need to identify the
> + * start and end of data ranges correctly even for sub-folio block sizes. This
> + * byte range based iteration is especially convenient because it means we don't
> + * have to care about variable size folios, nor where the start or end of the
> + * data range lies within a folio, if they lie within the same folio or even if
> + * there are multiple discontiguous data ranges within the folio.
> + */
> +static int
> +xfs_buffered_write_delalloc_release(
> + struct inode *inode,
> + loff_t start_byte,
> + loff_t end_byte)
> +{
> + loff_t punch_start_byte = start_byte;
> + int error = 0;
> +
> + /*
> + * Lock the mapping to avoid races with page faults re-instantiating
> + * folios and dirtying them via ->page_mkwrite whilst we walk the
> + * cache and perform delalloc extent removal. Failing to do this can
> + * leave dirty pages with no space reservation in the cache.
> + */
> + filemap_invalidate_lock(inode->i_mapping);
> + while (start_byte < end_byte) {
> + loff_t data_end;
> +
> + start_byte = mapping_seek_hole_data(inode->i_mapping,
> + start_byte, end_byte, SEEK_DATA);
FWIW, the fact that mapping seek data is based on uptodate status means
that seek behavior can change based on prior reads. For example, see how
seek hole/data presents reads of unwritten ranges as data [1]. The same
thing isn't observable for holes because iomap doesn't check the mapping
in that case, but underlying iop state is the same and that is what this
code is looking at.
The filtering being done here means we essentially only care about dirty
pages backed by delalloc blocks. That means if you get here with a dirty
page and the portion of the page affected by this failed write is
uptodate, this won't punch an underlying delalloc block even though
nothing else may have written to it in the meantime. That sort of state
can be created by a prior read of the range on a sub-page block size fs,
or perhaps a racing async readahead (via read fault of a lower
offset..?), etc.
I suspect this is not a serious error because the page is dirty and
writeback will thus convert the block. The only exception to that I can
see is if the block is beyond EOF (consider a mapped read to a page that
straddles EOF, followed by a post-eof write that fails), writeback won't
actually map the block directly. It may convert if contiguous with
delalloc blocks inside EOF (and sufficiently sized physical extents
exist), or even if not, should still otherwise be cleaned up by the
various other means we already have to manage post-eof blocks.
So IOW there's a tradeoff being made here for possible spurious
allocation and I/O and a subtle dependency on writeback that should
probably be documented somewhere. The larger concern is that if
writeback eventually changes based on dirty range tracking in a way that
breaks this dependency, that introduces yet another stale delalloc block
landmine associated with this error handling code (regardless of whether
you want to call that a bug in this code, seek data, whatever), and
those problems are difficult enough to root cause as it is.
Brian
[1]
# xfs_io -fc "falloc 0 4k" -c "seek -a 0" -c "pread 0 4k" -c "seek -a 0" <file>
Whence Result
HOLE 0
read 4096/4096 bytes at offset 0
4 KiB, 4 ops; 0.0000 sec (156 MiB/sec and 160000.0000 ops/sec)
Whence Result
DATA 0
HOLE 4096
> + /*
> + * If there is no more data to scan, all that is left is to
> + * punch out the remaining range.
> + */
> + if (start_byte == -ENXIO || start_byte == end_byte)
> + break;
> + if (start_byte < 0) {
> + error = start_byte;
> + goto out_unlock;
> + }
> + ASSERT(start_byte >= punch_start_byte);
> + ASSERT(start_byte < end_byte);
> +
> + /*
> + * We find the end of this contiguous cached data range by
> + * seeking from start_byte to the beginning of the next hole.
> + */
> + data_end = mapping_seek_hole_data(inode->i_mapping, start_byte,
> + end_byte, SEEK_HOLE);
> + if (data_end < 0) {
> + error = data_end;
> + goto out_unlock;
> + }
> + ASSERT(data_end > start_byte);
> + ASSERT(data_end <= end_byte);
> +
> + error = xfs_buffered_write_delalloc_scan(inode,
> + &punch_start_byte, start_byte, data_end);
> + if (error)
> + goto out_unlock;
> +
> + /* The next data search starts at the end of this one. */
> + start_byte = data_end;
> + }
> +
> + if (punch_start_byte < end_byte)
> + error = xfs_buffered_write_delalloc_punch(inode,
> + punch_start_byte, end_byte);
> +out_unlock:
> + filemap_invalidate_unlock(inode->i_mapping);
> + return error;
> +}
> +
> static int
> xfs_buffered_write_iomap_end(
> struct inode *inode,
> @@ -1179,16 +1319,7 @@ xfs_buffered_write_iomap_end(
> if (start_byte >= end_byte)
> return 0;
>
> - /*
> - * Lock the mapping to avoid races with page faults re-instantiating
> - * folios and dirtying them via ->page_mkwrite between the page cache
> - * truncation and the delalloc extent removal. Failing to do this can
> - * leave dirty pages with no space reservation in the cache.
> - */
> - filemap_invalidate_lock(inode->i_mapping);
> - truncate_pagecache_range(inode, start_byte, end_byte - 1);
> - error = xfs_buffered_write_delalloc_punch(inode, start_byte, end_byte);
> - filemap_invalidate_unlock(inode->i_mapping);
> + error = xfs_buffered_write_delalloc_release(inode, start_byte, end_byte);
> if (error && !xfs_is_shutdown(mp)) {
> xfs_alert(mp, "%s: unable to clean up ino 0x%llx",
> __func__, XFS_I(inode)->i_ino);
> --
> 2.37.2
>
>
