On Sat, Nov 30, 2024 at 09:39:29PM +0800, Long Li wrote:
> On Wed, Nov 27, 2024 at 02:35:02PM +0800, Long Li wrote:
> > During concurrent append writes to XFS filesystem, zero padding data
> > may appear in the file after power failure. This happens due to imprecise
> > disk size updates when handling write completion.
> >
> > Consider this scenario with concurrent append writes same file:
> >
> > Thread 1: Thread 2:
> > ------------ -----------
> > write [A, A+B]
> > update inode size to A+B
> > submit I/O [A, A+BS]
> > write [A+B, A+B+C]
> > update inode size to A+B+C
> > <I/O completes, updates disk size to min(A+B+C, A+BS)>
> > <power failure>
> >
> > After reboot:
> > 1) with A+B+C < A+BS, the file has zero padding in range [A+B, A+B+C]
> >
> > |< Block Size (BS) >|
> > |DDDDDDDDDDDDDDDD0000000000000000|
> > ^ ^ ^
> > A A+B A+B+C
> > (EOF)
> >
> > 2) with A+B+C > A+BS, the file has zero padding in range [A+B, A+BS]
> >
> > |< Block Size (BS) >|< Block Size (BS) >|
> > |DDDDDDDDDDDDDDDD0000000000000000|00000000000000000000000000000000|
> > ^ ^ ^ ^
> > A A+B A+BS A+B+C
> > (EOF)
> >
> > D = Valid Data
> > 0 = Zero Padding
> >
> > The issue stems from disk size being set to min(io_offset + io_size,
> > inode->i_size) at I/O completion. Since io_offset+io_size is block
> > size granularity, it may exceed the actual valid file data size. In
> > the case of concurrent append writes, inode->i_size may be larger
> > than the actual range of valid file data written to disk, leading to
> > inaccurate disk size updates.
> >
> > This patch modifies the meaning of io_size to represent the size of
> > valid data within EOF in an ioend. If the ioend spans beyond i_size,
> > io_size will be trimmed to provide the file with more accurate size
> > information. This is particularly useful for on-disk size updates
> > at completion time.
> >
> > After this change, ioends that span i_size will not grow or merge with
> > other ioends in concurrent scenarios. However, these cases that need
> > growth/merging rarely occur and it seems no noticeable performance impact.
> > Although rounding up io_size could enable ioend growth/merging in these
> > scenarios, we decided to keep the code simple after discussion [1].
> >
> > Another benefit is that it makes the xfs_ioend_is_append() check more
> > accurate, which can reduce unnecessary end bio callbacks of xfs_end_bio()
> > in certain scenarios, such as repeated writes at the file tail without
> > extending the file size.
> >
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Link[1]: https://patchwork.kernel.org/project/xfs/patch/20241113091907.56937-1-leo.lilong@xxxxxxxxxx
> > Signed-off-by: Long Li <leo.lilong@xxxxxxxxxx>
> > Reviewed-by: Brian Foster <bfoster@xxxxxxxxxx>
> > ---
> > v4->v5: remove iomap_ioend_size_aligned() and don't round up io_size for
> > ioend growth/merging to keep the code simple.
> > fs/iomap/buffered-io.c | 10 ++++++++++
> > include/linux/iomap.h | 2 +-
> > 2 files changed, 11 insertions(+), 1 deletion(-)
> >
> > diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
> > index d42f01e0fc1c..dc360c8e5641 100644
> > --- a/fs/iomap/buffered-io.c
> > +++ b/fs/iomap/buffered-io.c
> > @@ -1774,6 +1774,7 @@ static int iomap_add_to_ioend(struct iomap_writepage_ctx *wpc,
> > {
> > struct iomap_folio_state *ifs = folio->private;
> > size_t poff = offset_in_folio(folio, pos);
> > + loff_t isize = i_size_read(inode);
> > int error;
> >
> > if (!wpc->ioend || !iomap_can_add_to_ioend(wpc, pos)) {
> > @@ -1789,7 +1790,16 @@ static int iomap_add_to_ioend(struct iomap_writepage_ctx *wpc,
> >
> > if (ifs)
> > atomic_add(len, &ifs->write_bytes_pending);
> > +
> > + /*
> > + * If the ioend spans i_size, trim io_size to the former to provide
> > + * the fs with more accurate size information. This is useful for
> > + * completion time on-disk size updates.
> > + */
> > wpc->ioend->io_size += len;
> > + if (wpc->ioend->io_offset + wpc->ioend->io_size > isize)
> > + wpc->ioend->io_size = isize - wpc->ioend->io_offset;
> > +
>
> When performing fsstress test with this patch set, there is a very low probability of
> encountering an issue where isize is less than ioend->io_offset in iomap_add_to_ioend.
> After investigation, this was found to be caused by concurrent with truncate operations.
> Consider a scenario with 4K block size and a file size of 12K.
>
> //write back [8K, 12K] //truncate file to 4K
> ---------------------- ----------------------
> iomap_writepage_map xfs_setattr_size
> iomap_writepage_handle_eof
> truncate_setsize
> i_size_write(inode, newsize) //update inode size to 4K
> iomap_writepage_map_blocks
> iomap_add_to_ioend
> < iszie < ioend->io_offset>
> <iszie = 4K, ioend->io_offset=8K>
>
> It appears that in extreme cases, folios beyond EOF might be written back,
> resulting in situations where isize is less than pos. In such cases,
> maybe we should not trim the io_size further.
>
Hmm.. it might be wise to characterize this further to determine whether
there are potentially larger problems to address before committing to
anything. For example, assuming truncate acquires ilock and does
xfs_itruncate_extents() and whatnot before this ioend submits/completes,
does anything in that submission or completion path detect and handle
this scenario gracefully? What if the ioend happens to be unwritten
post-eof preallocation and completion wants to convert blocks that might
no longer exist in the file..?
I don't see anything obvious on a quick look other than unwritten
conversion doesn't look like it would bump up i_size, which sounds sane,
but I could have easily missed something. If nobody else can point at
something, a way to instrument this might be to do something like:
1. add post-eof preallocation to a file
2. buffered write beyond eof
3. inject a delay in the writeback path somewhere after the writeback
eof checks
4. when writeback sits on that delay, truncate the file and try to
remove those extents before the ioend submits
Maybe something similar could be done to isolate the append ioend
completion scenario as well. Hm?
Brian
> Long Li
>
>
