Re: [PATCH] iomap: fix zero padding data issue in concurrent append writes

[Long Li <leo.lilong@huawei.com>]

On Fri, Nov 08, 2024 at 06:55:23AM -0800, Christoph Hellwig wrote:
> On Fri, Nov 08, 2024 at 08:27:38PM +0800, Long Li wrote:
> > After reboot, file has zero padding in range [A+B, A+B+C]:
> > 
> >   |<         Block Size (BS)      >|
> >   |DDDDDDDDDDDDDDDD0000000000000000|
> >   ^               ^        ^
> >   A              A+B      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.
> 
> Oh, interesting one.  Do you have a reproducer we could wire up
> to xfstests?
> 

Yes, I have a simple reproducer, but it would require significant
work to incorporate it into xfstestis.

> > This patch introduce ioend->io_end to trace the end position of the
> > valid data in ioend, rather than solely relying on ioend->io_size.
> > It ensures more precise disk size updates and avoids the zero padding
> > issue. 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.
> 
> Hmm.  Can we do away with two members for the size by just rounding
> up the block size for the block based operations?
> 

If we only use one size record, we can remove io_size and keep only
io_end to record the tail end of valid file data in ioend. Meanwhile,
we can add a wrapper function iomep_ioend_iosize() to get the extent
size of ioend, replacing the existing ioend->io_size. Would this work?

Thanks,
Long Li