Re: [PATCH v5 1/2] iomap: fix zero padding data issue in concurrent append writes

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

On Wed, Nov 27, 2024 at 08:28:29AM -0800, Darrick J. Wong wrote:
> > @@ -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.
> 
> I think it's useful to preserve the diagram showing exactly what problem
> you're solving:
> 
> 	/*
> 	 * Clamp io_offset and io_size to the incore EOF so that ondisk
> 	 * file size updates in the ioend completion are byte-accurate.
> 	 * This avoids recovering files with zeroed tail regions when
> 	 * writeback races with appending writes:
> 	 *
> 	 *    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)    >|
> 	 *    |DDDDDDDDDDDD00000000000000|
> 	 *    ^           ^        ^
> 	 *    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)    >|
> 	 *    |DDDDDDDDDDDD00000000000000|000000000000000000000000000|
> 	 *    ^           ^              ^           ^
> 	 *    A          A+B            A+BS       A+B+C
> 	 *                              (EOF)
> 	 *
> 	 *    D = Valid Data
> 	 *    0 = Zero Padding
> 	 *
> 	 * Note that this defeats the ability to chain the ioends of
> 	 * appending writes.
> 	 */
> 
> (I reduced the blocksize a bit for wrapping purposes)

Ok, I will update it.

> 
> The logic looks ok, but I'm curious about how you landed at 2.6.12-rc
> for the fixes tag.
> 
> --D

I see that io_size was introduced in version 2.6. It's quite difficult
to determine the exact version where the issue was introduced, but I can
confirm it was before version 4.19, as I can reproduce the issue in 4.19.
It should before introduce iomap infrastructure, how about using the
following fix tag?

Fixes: ae259a9c8593 ("fs: introduce iomap infrastructure") # goes further back than this

Thanks, 
Long Li