[BUG REPORT] generic/561 fails when testing xfs on next-20240506 kernel

[BUG REPORT] generic/561 fails when testing xfs on next-20240506 kernel

[Chandan Babu R]

Hi,

generic/561 fails when testing XFS on a next-20240506 kernel as shown below,

# ./check generic/561
FSTYP         -- xfs (debug)
PLATFORM      -- Linux/x86_64 xfs-crc-rtdev-extsize-28k 6.9.0-rc7-next-20240506+ #1 SMP PREEMPT_DYNAMIC Mon May  6 07:53:46 GMT 2024
MKFS_OPTIONS  -- -f -rrtdev=/dev/loop14 -f -m reflink=0,rmapbt=0, -d rtinherit=1 -r extsize=28k /dev/loop5
MOUNT_OPTIONS -- -o context=system_u:object_r:root_t:s0 -ortdev=/dev/loop14 /dev/loop5 /media/scratch

generic/561       - output mismatch (see /var/lib/xfstests/results/xfs-crc-rtdev-extsize-28k/6.9.0-rc7-next-20240506+/xfs_crc_rtdev_extsize_28k/generic/561.out.bad)
    --- tests/generic/561.out   2024-05-06 08:18:09.681430366 +0000
    +++ /var/lib/xfstests/results/xfs-crc-rtdev-extsize-28k/6.9.0-rc7-next-20240506+/xfs_crc_rtdev_extsize_28k/generic/561.out.bad        2024-05-08 09:14:24.908010133 +0000
    @@ -1,2 +1,5 @@
     QA output created by 561
    +/media/scratch/dir/p0/d0XXXXXXXXXXXXXXXXXXXXXXX/d486/d4bXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d5bXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d212XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d11XXXXXXXXX/d54/de4/d158/d27f/d895/d1307XXX/d8a4/d832XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/r112fXXXXXXXXXXX: FAILED
    +/media/scratch/dir/p0/d0XXXXXXXXXXXXXXXXXXXXXXX/d486/d4bXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d5bXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d212XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d11XXXXXXXXX/d54/de4/d158/d27f/d13a3XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d13c0XXXXXXXX/d2301X/d222bXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d1240XXXXXXXXXXXXXXXXXXXXXXXX/d722XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d1380XXXXXXXXXXXXXXXX/dc62XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/r10d5: FAILED
    +md5sum: WARNING: 2 computed checksums did NOT match
     Silence is golden
    ...
    (Run 'diff -u /var/lib/xfstests/tests/generic/561.out /var/lib/xfstests/results/xfs-crc-rtdev-extsize-28k/6.9.0-rc7-next-20240506+/xfs_crc_rtdev_extsize_28k/generic/561.out.bad'  to see the entire diff)
Ran: generic/561
Failures: generic/561
Failed 1 of 1 tests

The following was the fstest configuration used for the test run,

  FSTYP=xfs
  TEST_DIR=/media/test
  SCRATCH_MNT=/media/scratch
  TEST_DEV=/dev/loop16
  TEST_LOGDEV=/dev/loop13
  SCRATCH_DEV_POOL="/dev/loop5 /dev/loop6 /dev/loop7 /dev/loop8 /dev/loop9 /dev/loop10 /dev/loop11 /dev/loop12"
  MKFS_OPTIONS='-f -m crc=1,reflink=0,rmapbt=0, -i sparse=0 -lsize=1g'
  TEST_FS_MOUNT_OPTS="-o logdev=/dev/loop13"
  MOUNT_OPTIONS='-o usrquota,grpquota,prjquota'
  TEST_FS_MOUNT_OPTS="$TEST_FS_MOUNT_OPTS -o usrquota,grpquota,prjquota"
  SCRATCH_LOGDEV=/dev/loop15
  USE_EXTERNAL=yes
  LOGWRITES_DEV=/dev/loop15

Git bisect produced the following as the first bad commit,

commit 943bc0882cebf482422640924062a7daac5a27ba
Author: Zhang Yi <yi.zhang@huawei.com>
Date:   Wed Mar 20 19:05:45 2024 +0800

    iomap: don't increase i_size if it's not a write operation

    Increase i_size in iomap_zero_range() and iomap_unshare_iter() is not
    needed, the caller should handle it. Especially, when truncate partial
    block, we should not increase i_size beyond the new EOF here. It doesn't
    affect xfs and gfs2 now because they set the new file size after zero
    out, it doesn't matter that a transient increase in i_size, but it will
    affect ext4 because it set file size before truncate. So move the i_size
    updating logic to iomap_write_iter().

    Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
    Link: https://lore.kernel.org/r/20240320110548.2200662-7-yi.zhang@huaweicloud.com
    Reviewed-by: Christoph Hellwig <hch@lst.de>
    Reviewed-by: Darrick J. Wong <djwong@kernel.org>
    Signed-off-by: Christian Brauner <brauner@kernel.org>

 fs/iomap/buffered-io.c | 50 +++++++++++++++++++++++++-------------------------
 1 file changed, 25 insertions(+), 25 deletions(-)
 
-- 
Chandan

Re: [BUG REPORT] generic/561 fails when testing xfs on next-20240506 kernel

[Zhang Yi]

On 2024/5/8 17:01, Chandan Babu R wrote:
> Hi,
> 
> generic/561 fails when testing XFS on a next-20240506 kernel as shown below,
> 
> # ./check generic/561
> FSTYP         -- xfs (debug)
> PLATFORM      -- Linux/x86_64 xfs-crc-rtdev-extsize-28k 6.9.0-rc7-next-20240506+ #1 SMP PREEMPT_DYNAMIC Mon May  6 07:53:46 GMT 2024
> MKFS_OPTIONS  -- -f -rrtdev=/dev/loop14 -f -m reflink=0,rmapbt=0, -d rtinherit=1 -r extsize=28k /dev/loop5
> MOUNT_OPTIONS -- -o context=system_u:object_r:root_t:s0 -ortdev=/dev/loop14 /dev/loop5 /media/scratch
> 
> generic/561       - output mismatch (see /var/lib/xfstests/results/xfs-crc-rtdev-extsize-28k/6.9.0-rc7-next-20240506+/xfs_crc_rtdev_extsize_28k/generic/561.out.bad)
>     --- tests/generic/561.out   2024-05-06 08:18:09.681430366 +0000
>     +++ /var/lib/xfstests/results/xfs-crc-rtdev-extsize-28k/6.9.0-rc7-next-20240506+/xfs_crc_rtdev_extsize_28k/generic/561.out.bad        2024-05-08 09:14:24.908010133 +0000
>     @@ -1,2 +1,5 @@
>      QA output created by 561
>     +/media/scratch/dir/p0/d0XXXXXXXXXXXXXXXXXXXXXXX/d486/d4bXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d5bXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d212XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d11XXXXXXXXX/d54/de4/d158/d27f/d895/d1307XXX/d8a4/d832XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/r112fXXXXXXXXXXX: FAILED
>     +/media/scratch/dir/p0/d0XXXXXXXXXXXXXXXXXXXXXXX/d486/d4bXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d5bXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d212XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d11XXXXXXXXX/d54/de4/d158/d27f/d13a3XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d13c0XXXXXXXX/d2301X/d222bXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d1240XXXXXXXXXXXXXXXXXXXXXXXX/d722XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d1380XXXXXXXXXXXXXXXX/dc62XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/r10d5: FAILED
>     +md5sum: WARNING: 2 computed checksums did NOT match
>      Silence is golden
>     ...
>     (Run 'diff -u /var/lib/xfstests/tests/generic/561.out /var/lib/xfstests/results/xfs-crc-rtdev-extsize-28k/6.9.0-rc7-next-20240506+/xfs_crc_rtdev_extsize_28k/generic/561.out.bad'  to see the entire diff)
> Ran: generic/561
> Failures: generic/561
> Failed 1 of 1 tests
> 

Sorry about this regression. After debuging and analyzing the code, I notice
that this problem could only happens on xfs realtime inode. The real problem
is about realtime extent alignment.

Please assume that if we have a file that contains a written extent [A, D).
We unaligned truncate to the file to B, in the middle of this written extent.

       A            B                  D
      +wwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwww

After the truncate, the i_size is set to B, but due to the sb_rextsize,
xfs_itruncate_extents() truncate and aligned the written extent to C, so the
data in [B, C) doesn't zeroed and becomes stale.

       A            B     C
      +wwwwwwwwwwwwwwSSSSSS
                    ^
                   EOF

The if we write [E, F) beyond this written extent, xfs_file_write_checks()->
xfs_zero_range() would zero [B, C) in page cache, but since we don't increase
i_size in iomap_zero_iter(), the writeback process doesn't write zero data
to disk. After write, the data in [B, C) is still stale so once we clear the
pagecache, this stale data is exposed.

       A            B     C        E      F
      +wwwwwwwwwwwwwwSSSSSS        wwwwwwww

The reason this problem doesn't occur on normal inode is because normal inode
doesn't have a post EOF written extent. For realtime inode, I guess it's not
enough to just zero the EOF block (xfs_setattr_size()->xfs_truncate_page()),
we should also zero the extra blocks that aligned to realtime extent size
before updating i_size. Any suggestions?

Thanks,
Yi.



> The following was the fstest configuration used for the test run,
> 
>   FSTYP=xfs
>   TEST_DIR=/media/test
>   SCRATCH_MNT=/media/scratch
>   TEST_DEV=/dev/loop16
>   TEST_LOGDEV=/dev/loop13
>   SCRATCH_DEV_POOL="/dev/loop5 /dev/loop6 /dev/loop7 /dev/loop8 /dev/loop9 /dev/loop10 /dev/loop11 /dev/loop12"
>   MKFS_OPTIONS='-f -m crc=1,reflink=0,rmapbt=0, -i sparse=0 -lsize=1g'
>   TEST_FS_MOUNT_OPTS="-o logdev=/dev/loop13"
>   MOUNT_OPTIONS='-o usrquota,grpquota,prjquota'
>   TEST_FS_MOUNT_OPTS="$TEST_FS_MOUNT_OPTS -o usrquota,grpquota,prjquota"
>   SCRATCH_LOGDEV=/dev/loop15
>   USE_EXTERNAL=yes
>   LOGWRITES_DEV=/dev/loop15
> 
> Git bisect produced the following as the first bad commit,
> 
> commit 943bc0882cebf482422640924062a7daac5a27ba
> Author: Zhang Yi <yi.zhang@huawei.com>
> Date:   Wed Mar 20 19:05:45 2024 +0800
> 
>     iomap: don't increase i_size if it's not a write operation
> 
>     Increase i_size in iomap_zero_range() and iomap_unshare_iter() is not
>     needed, the caller should handle it. Especially, when truncate partial
>     block, we should not increase i_size beyond the new EOF here. It doesn't
>     affect xfs and gfs2 now because they set the new file size after zero
>     out, it doesn't matter that a transient increase in i_size, but it will
>     affect ext4 because it set file size before truncate. So move the i_size
>     updating logic to iomap_write_iter().
> 
>     Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
>     Link: https://lore.kernel.org/r/20240320110548.2200662-7-yi.zhang@huaweicloud.com
>     Reviewed-by: Christoph Hellwig <hch@lst.de>
>     Reviewed-by: Darrick J. Wong <djwong@kernel.org>
>     Signed-off-by: Christian Brauner <brauner@kernel.org>
> 
>  fs/iomap/buffered-io.c | 50 +++++++++++++++++++++++++-------------------------
>  1 file changed, 25 insertions(+), 25 deletions(-)
>  
> 

Re: [BUG REPORT] generic/561 fails when testing xfs on next-20240506 kernel

[Dave Chinner]

On Sat, May 11, 2024 at 11:11:32AM +0800, Zhang Yi wrote:
> On 2024/5/8 17:01, Chandan Babu R wrote:
> > Hi,
> > 
> > generic/561 fails when testing XFS on a next-20240506 kernel as shown below,
> > 
> > # ./check generic/561
> > FSTYP         -- xfs (debug)
> > PLATFORM      -- Linux/x86_64 xfs-crc-rtdev-extsize-28k 6.9.0-rc7-next-20240506+ #1 SMP PREEMPT_DYNAMIC Mon May  6 07:53:46 GMT 2024
> > MKFS_OPTIONS  -- -f -rrtdev=/dev/loop14 -f -m reflink=0,rmapbt=0, -d rtinherit=1 -r extsize=28k /dev/loop5
> > MOUNT_OPTIONS -- -o context=system_u:object_r:root_t:s0 -ortdev=/dev/loop14 /dev/loop5 /media/scratch
> > 
> > generic/561       - output mismatch (see /var/lib/xfstests/results/xfs-crc-rtdev-extsize-28k/6.9.0-rc7-next-20240506+/xfs_crc_rtdev_extsize_28k/generic/561.out.bad)
> >     --- tests/generic/561.out   2024-05-06 08:18:09.681430366 +0000
> >     +++ /var/lib/xfstests/results/xfs-crc-rtdev-extsize-28k/6.9.0-rc7-next-20240506+/xfs_crc_rtdev_extsize_28k/generic/561.out.bad        2024-05-08 09:14:24.908010133 +0000
> >     @@ -1,2 +1,5 @@
> >      QA output created by 561
> >     +/media/scratch/dir/p0/d0XXXXXXXXXXXXXXXXXXXXXXX/d486/d4bXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d5bXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d212XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d11XXXXXXXXX/d54/de4/d158/d27f/d895/d1307XXX/d8a4/d832XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/r112fXXXXXXXXXXX: FAILED
> >     +/media/scratch/dir/p0/d0XXXXXXXXXXXXXXXXXXXXXXX/d486/d4bXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d5bXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d212XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d11XXXXXXXXX/d54/de4/d158/d27f/d13a3XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d13c0XXXXXXXX/d2301X/d222bXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d1240XXXXXXXXXXXXXXXXXXXXXXXX/d722XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d1380XXXXXXXXXXXXXXXX/dc62XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/r10d5: FAILED
> >     +md5sum: WARNING: 2 computed checksums did NOT match
> >      Silence is golden
> >     ...
> >     (Run 'diff -u /var/lib/xfstests/tests/generic/561.out /var/lib/xfstests/results/xfs-crc-rtdev-extsize-28k/6.9.0-rc7-next-20240506+/xfs_crc_rtdev_extsize_28k/generic/561.out.bad'  to see the entire diff)
> > Ran: generic/561
> > Failures: generic/561
> > Failed 1 of 1 tests
> > 
> 
> Sorry about this regression. After debuging and analyzing the code, I notice
> that this problem could only happens on xfs realtime inode. The real problem
> is about realtime extent alignment.
> 
> Please assume that if we have a file that contains a written extent [A, D).
> We unaligned truncate to the file to B, in the middle of this written extent.
> 
>        A            B                  D
>       +wwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwww
> 
> After the truncate, the i_size is set to B, but due to the sb_rextsize,
> xfs_itruncate_extents() truncate and aligned the written extent to C, so the
> data in [B, C) doesn't zeroed and becomes stale.
> 
>        A            B     C
>       +wwwwwwwwwwwwwwSSSSSS
>                     ^
>                    EOF

This region must be zeroed on disk before we call
xfs_itruncate_extents().  i.e completed xfs_setattr_size() via
xfs_truncate_page() and flushed to disk before we start removing
extents.

The problem is that iomap_truncate_page() only zeros the trailing
portion of the i_blocksize() value, which is wrong for realtime
devices with rtextsize != fs blocksize.

Further, xfs_setattr_size() then calls truncate_setsize(newsize)
before the zeroing has been written back to disk, which means
that the flush that occurs immediately after the truncate_setsize()
call can not write blocks beyond the new EOF regardless of whether
iomap_truncate_page() wrote zeroes to them or not.

> The if we write [E, F) beyond this written extent, xfs_file_write_checks()->
> xfs_zero_range() would zero [B, C) in page cache, but since we don't increase
> i_size in iomap_zero_iter(), the writeback process doesn't write zero data
> to disk. After write, the data in [B, C) is still stale so once we clear the
> pagecache, this stale data is exposed.
> 
>        A            B     C        E      F
>       +wwwwwwwwwwwwwwSSSSSS        wwwwwwww
> 
> The reason this problem doesn't occur on normal inode is because normal inode
> doesn't have a post EOF written extent.

That's incorrect - we can have post-eof written extents on normal
files. The reason this doesn't get exposed for normal files is that
the zeroing range used in iomap_truncate_page() covers the entire
filesystem block and writeback can write the entire EOF page that
covers that block containing the zeroes. Hence when we remove all
the written extents beyond EOF later in the truncate, we don't leave
any blocks beyond EOF that we haven't zeroed.

> For realtime inode, I guess it's not
> enough to just zero the EOF block (xfs_setattr_size()->xfs_truncate_page()),
> we should also zero the extra blocks that aligned to realtime extent size
> before updating i_size. Any suggestions?

Right. xfs_setattr_size() needs fixing to flush the entire zeroed
range *before* truncating the page cache and changing the inode size.

Of course, xfs_truncate_page() also needs fixing to zero the 
entire rtextsize range, not use iomap_truncate_page() which only
zeroes to the end of the EOF filesystem block.

I note that dax_truncate_page() has the same problem with RT device
block sizes as iomap_truncate_page(), so we need the same fix for
both dax and non-dax paths here.

It might actually be easiest to pass the block size for zeroing into
iomap_truncate_page() rather than relying on being able to extract
the zeroing range from the inode via i_blocksize(). We can't use
i_blocksize() for RT files, because inode->i_blkbits and hence
i_blocksize() only supports power of 2 block sizes. Changing that is
a *much* bigger job, so fixing xfs_truncate_page() is likely the
best thing to do right now...

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [BUG REPORT] generic/561 fails when testing xfs on next-20240506 kernel

[Zhang Yi]

On 2024/5/11 11:45, Dave Chinner wrote:
> On Sat, May 11, 2024 at 11:11:32AM +0800, Zhang Yi wrote:
>> On 2024/5/8 17:01, Chandan Babu R wrote:
>>> Hi,
>>>
>>> generic/561 fails when testing XFS on a next-20240506 kernel as shown below,
>>>
>>> # ./check generic/561
>>> FSTYP         -- xfs (debug)
>>> PLATFORM      -- Linux/x86_64 xfs-crc-rtdev-extsize-28k 6.9.0-rc7-next-20240506+ #1 SMP PREEMPT_DYNAMIC Mon May  6 07:53:46 GMT 2024
>>> MKFS_OPTIONS  -- -f -rrtdev=/dev/loop14 -f -m reflink=0,rmapbt=0, -d rtinherit=1 -r extsize=28k /dev/loop5
>>> MOUNT_OPTIONS -- -o context=system_u:object_r:root_t:s0 -ortdev=/dev/loop14 /dev/loop5 /media/scratch
>>>
>>> generic/561       - output mismatch (see /var/lib/xfstests/results/xfs-crc-rtdev-extsize-28k/6.9.0-rc7-next-20240506+/xfs_crc_rtdev_extsize_28k/generic/561.out.bad)
>>>     --- tests/generic/561.out   2024-05-06 08:18:09.681430366 +0000
>>>     +++ /var/lib/xfstests/results/xfs-crc-rtdev-extsize-28k/6.9.0-rc7-next-20240506+/xfs_crc_rtdev_extsize_28k/generic/561.out.bad        2024-05-08 09:14:24.908010133 +0000
>>>     @@ -1,2 +1,5 @@
>>>      QA output created by 561
>>>     +/media/scratch/dir/p0/d0XXXXXXXXXXXXXXXXXXXXXXX/d486/d4bXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d5bXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d212XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d11XXXXXXXXX/d54/de4/d158/d27f/d895/d1307XXX/d8a4/d832XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/r112fXXXXXXXXXXX: FAILED
>>>     +/media/scratch/dir/p0/d0XXXXXXXXXXXXXXXXXXXXXXX/d486/d4bXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d5bXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d212XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d11XXXXXXXXX/d54/de4/d158/d27f/d13a3XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d13c0XXXXXXXX/d2301X/d222bXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d1240XXXXXXXXXXXXXXXXXXXXXXXX/d722XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d1380XXXXXXXXXXXXXXXX/dc62XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/r10d5: FAILED
>>>     +md5sum: WARNING: 2 computed checksums did NOT match
>>>      Silence is golden
>>>     ...
>>>     (Run 'diff -u /var/lib/xfstests/tests/generic/561.out /var/lib/xfstests/results/xfs-crc-rtdev-extsize-28k/6.9.0-rc7-next-20240506+/xfs_crc_rtdev_extsize_28k/generic/561.out.bad'  to see the entire diff)
>>> Ran: generic/561
>>> Failures: generic/561
>>> Failed 1 of 1 tests
>>>
>>
>> Sorry about this regression. After debuging and analyzing the code, I notice
>> that this problem could only happens on xfs realtime inode. The real problem
>> is about realtime extent alignment.
>>
>> Please assume that if we have a file that contains a written extent [A, D).
>> We unaligned truncate to the file to B, in the middle of this written extent.
>>
>>        A            B                  D
>>       +wwwwwwwwwwwwwwwwwwwwwwwwwwwwwwwww
>>
>> After the truncate, the i_size is set to B, but due to the sb_rextsize,
>> xfs_itruncate_extents() truncate and aligned the written extent to C, so the
>> data in [B, C) doesn't zeroed and becomes stale.
>>
>>        A            B     C
>>       +wwwwwwwwwwwwwwSSSSSS
>>                     ^
>>                    EOF
> 
> This region must be zeroed on disk before we call
> xfs_itruncate_extents().  i.e completed xfs_setattr_size() via
> xfs_truncate_page() and flushed to disk before we start removing
> extents.
> 
> The problem is that iomap_truncate_page() only zeros the trailing
> portion of the i_blocksize() value, which is wrong for realtime
> devices with rtextsize != fs blocksize.
> 
> Further, xfs_setattr_size() then calls truncate_setsize(newsize)
> before the zeroing has been written back to disk, which means
> that the flush that occurs immediately after the truncate_setsize()
> call can not write blocks beyond the new EOF regardless of whether
> iomap_truncate_page() wrote zeroes to them or not.
> 
>> The if we write [E, F) beyond this written extent, xfs_file_write_checks()->
>> xfs_zero_range() would zero [B, C) in page cache, but since we don't increase
>> i_size in iomap_zero_iter(), the writeback process doesn't write zero data
>> to disk. After write, the data in [B, C) is still stale so once we clear the
>> pagecache, this stale data is exposed.
>>
>>        A            B     C        E      F
>>       +wwwwwwwwwwwwwwSSSSSS        wwwwwwww
>>
>> The reason this problem doesn't occur on normal inode is because normal inode
>> doesn't have a post EOF written extent.
> 
> That's incorrect - we can have post-eof written extents on normal
> files. The reason this doesn't get exposed for normal files is that
> the zeroing range used in iomap_truncate_page() covers the entire
> filesystem block and writeback can write the entire EOF page that
> covers that block containing the zeroes. Hence when we remove all
> the written extents beyond EOF later in the truncate, we don't leave
> any blocks beyond EOF that we haven't zeroed.
> 
>> For realtime inode, I guess it's not
>> enough to just zero the EOF block (xfs_setattr_size()->xfs_truncate_page()),
>> we should also zero the extra blocks that aligned to realtime extent size
>> before updating i_size. Any suggestions?
> 
> Right. xfs_setattr_size() needs fixing to flush the entire zeroed
> range *before* truncating the page cache and changing the inode size.
> 
> Of course, xfs_truncate_page() also needs fixing to zero the 
> entire rtextsize range, not use iomap_truncate_page() which only
> zeroes to the end of the EOF filesystem block.
> 
> I note that dax_truncate_page() has the same problem with RT device
> block sizes as iomap_truncate_page(), so we need the same fix for
> both dax and non-dax paths here.
> 
> It might actually be easiest to pass the block size for zeroing into
> iomap_truncate_page() rather than relying on being able to extract
> the zeroing range from the inode via i_blocksize(). We can't use
> i_blocksize() for RT files, because inode->i_blkbits and hence
> i_blocksize() only supports power of 2 block sizes. Changing that is
> a *much* bigger job, so fixing xfs_truncate_page() is likely the
> best thing to do right now...
> 

Thanks for the explanation and suggestion, I agree with you. However,
why do you recommend to pass block size for zeroing in to
iomap_truncate_page()? It's looks like we could fix xfs_truncate_page()
by using iomap_zero_range() and dax_zero_range() instead and don't use
iomap_truncate_page() and dax_truncate_page().

Thanks,
Yi.

Re: [BUG REPORT] generic/561 fails when testing xfs on next-20240506 kernel

[Dave Chinner]

On Sat, May 11, 2024 at 03:43:17PM +0800, Zhang Yi wrote:
> On 2024/5/11 11:45, Dave Chinner wrote:
> > On Sat, May 11, 2024 at 11:11:32AM +0800, Zhang Yi wrote:
> >> On 2024/5/8 17:01, Chandan Babu R wrote:
> > It might actually be easiest to pass the block size for zeroing into
> > iomap_truncate_page() rather than relying on being able to extract
> > the zeroing range from the inode via i_blocksize(). We can't use
> > i_blocksize() for RT files, because inode->i_blkbits and hence
> > i_blocksize() only supports power of 2 block sizes. Changing that is
> > a *much* bigger job, so fixing xfs_truncate_page() is likely the
> > best thing to do right now...
> > 
> 
> Thanks for the explanation and suggestion, I agree with you. However,
> why do you recommend to pass block size for zeroing in to
> iomap_truncate_page()? It's looks like we could fix xfs_truncate_page()
> by using iomap_zero_range() and dax_zero_range() instead and don't use
> iomap_truncate_page() and dax_truncate_page().

Fair question. Yes, we could just fix it in XFS.

But then any other filesystem that uses iomap might have the same
problem where the allocation block size (and hence the range that
needs zeroing) is different to the filesystem block size (e.g. ext4
with bigalloc?). At that point, those filesystem developers need to:

	a) be aware of the issue; and
	b) implement their own iomap_zero_range() wrapper for the
	same function.

If the iomap infrastructure doesn't assume block sizes are always
i_blocksize() (which is clearly a bad assumption!), then the API
itself informs the filesystem developers of the fact they really
have to care about using the correct allocation block sizes during
EOF zeroing.

At the moment, only XFS uses iomap_truncate_page(), and only ext2
and XFS use dax_truncate_page(). It seems pretty simple to change
the infrastructure and document why we don't use i_blocksize() at
this point. Especially considering we have forced alignment stuff in
XFS coming up which further decouples allocation unit (and hence
zeroing range) from the filesystem block size....

-Dave.
-- 
Dave Chinner
david@fromorbit.com

Re: [BUG REPORT] generic/561 fails when testing xfs on next-20240506 kernel

[Zhang Yi]

On 2024/5/11 16:19, Dave Chinner wrote:
> On Sat, May 11, 2024 at 03:43:17PM +0800, Zhang Yi wrote:
>> On 2024/5/11 11:45, Dave Chinner wrote:
>>> On Sat, May 11, 2024 at 11:11:32AM +0800, Zhang Yi wrote:
>>>> On 2024/5/8 17:01, Chandan Babu R wrote:
>>> It might actually be easiest to pass the block size for zeroing into
>>> iomap_truncate_page() rather than relying on being able to extract
>>> the zeroing range from the inode via i_blocksize(). We can't use
>>> i_blocksize() for RT files, because inode->i_blkbits and hence
>>> i_blocksize() only supports power of 2 block sizes. Changing that is
>>> a *much* bigger job, so fixing xfs_truncate_page() is likely the
>>> best thing to do right now...
>>>
>>
>> Thanks for the explanation and suggestion, I agree with you. However,
>> why do you recommend to pass block size for zeroing in to
>> iomap_truncate_page()? It's looks like we could fix xfs_truncate_page()
>> by using iomap_zero_range() and dax_zero_range() instead and don't use
>> iomap_truncate_page() and dax_truncate_page().
> 
> Fair question. Yes, we could just fix it in XFS.
> 
> But then any other filesystem that uses iomap might have the same
> problem where the allocation block size (and hence the range that
> needs zeroing) is different to the filesystem block size (e.g. ext4
> with bigalloc?). At that point, those filesystem developers need to:
> 
> 	a) be aware of the issue; and
> 	b) implement their own iomap_zero_range() wrapper for the
> 	same function.
> 
> If the iomap infrastructure doesn't assume block sizes are always
> i_blocksize() (which is clearly a bad assumption!), then the API
> itself informs the filesystem developers of the fact they really
> have to care about using the correct allocation block sizes during
> EOF zeroing.
> 
> At the moment, only XFS uses iomap_truncate_page(), and only ext2
> and XFS use dax_truncate_page(). It seems pretty simple to change
> the infrastructure and document why we don't use i_blocksize() at
> this point. Especially considering we have forced alignment stuff in
> XFS coming up which further decouples allocation unit (and hence
> zeroing range) from the filesystem block size....
> 

ext4 with bigalloc still use block size (not cluster size) to manage
allocation extents, so it aligned to the block size when truncate and
doesn't have this problem now. But it doesn't matter, your suggestion
make sense to me, pass block size to iomap_truncate_page() and
dax_truncate_page() could make them to have better compatibility.

Thanks,
Yi.

Re: [BUG REPORT] generic/561 fails when testing xfs on next-20240506 kernel

[Zhang Yi]

On 2024/5/8 17:01, Chandan Babu R wrote:
> Hi,
> 
> generic/561 fails when testing XFS on a next-20240506 kernel as shown below,

Hello,

I've send out the fix [1] based on the solution discussed with Dave, and
I've tested it on generic/561 several times and -g auto with below 4 types
of config, no new failures. I hope I've covered all the corner cases about
zero range on xfs, please take a look at that series.

- Default config
- MKFS_OPTIONS="-f -m reflink=0"
- MOUNT_OPTIONS='-o dax'
- USE_EXTERNAL=yes
  SCRATCH_RTDEV=/dev/nvme0n1
  MKFS_OPTIONS="-f -m reflink=0,rmapbt=0, -d rtinherit=1 -r extsize=28k"

[1] https://lore.kernel.org/linux-xfs/20240515022829.2455554-1-yi.zhang@huaweicloud.com/

Thanks,
Yi.

> 
> # ./check generic/561
> FSTYP         -- xfs (debug)
> PLATFORM      -- Linux/x86_64 xfs-crc-rtdev-extsize-28k 6.9.0-rc7-next-20240506+ #1 SMP PREEMPT_DYNAMIC Mon May  6 07:53:46 GMT 2024
> MKFS_OPTIONS  -- -f -rrtdev=/dev/loop14 -f -m reflink=0,rmapbt=0, -d rtinherit=1 -r extsize=28k /dev/loop5
> MOUNT_OPTIONS -- -o context=system_u:object_r:root_t:s0 -ortdev=/dev/loop14 /dev/loop5 /media/scratch
> 
> generic/561       - output mismatch (see /var/lib/xfstests/results/xfs-crc-rtdev-extsize-28k/6.9.0-rc7-next-20240506+/xfs_crc_rtdev_extsize_28k/generic/561.out.bad)
>     --- tests/generic/561.out   2024-05-06 08:18:09.681430366 +0000
>     +++ /var/lib/xfstests/results/xfs-crc-rtdev-extsize-28k/6.9.0-rc7-next-20240506+/xfs_crc_rtdev_extsize_28k/generic/561.out.bad        2024-05-08 09:14:24.908010133 +0000
>     @@ -1,2 +1,5 @@
>      QA output created by 561
>     +/media/scratch/dir/p0/d0XXXXXXXXXXXXXXXXXXXXXXX/d486/d4bXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d5bXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d212XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d11XXXXXXXXX/d54/de4/d158/d27f/d895/d1307XXX/d8a4/d832XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/r112fXXXXXXXXXXX: FAILED
>     +/media/scratch/dir/p0/d0XXXXXXXXXXXXXXXXXXXXXXX/d486/d4bXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d5bXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d212XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d11XXXXXXXXX/d54/de4/d158/d27f/d13a3XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d13c0XXXXXXXX/d2301X/d222bXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d1240XXXXXXXXXXXXXXXXXXXXXXXX/d722XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/d1380XXXXXXXXXXXXXXXX/dc62XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX/r10d5: FAILED
>     +md5sum: WARNING: 2 computed checksums did NOT match
>      Silence is golden
>     ...
>     (Run 'diff -u /var/lib/xfstests/tests/generic/561.out /var/lib/xfstests/results/xfs-crc-rtdev-extsize-28k/6.9.0-rc7-next-20240506+/xfs_crc_rtdev_extsize_28k/generic/561.out.bad'  to see the entire diff)
> Ran: generic/561
> Failures: generic/561
> Failed 1 of 1 tests
> 
> The following was the fstest configuration used for the test run,
> 
>   FSTYP=xfs
>   TEST_DIR=/media/test
>   SCRATCH_MNT=/media/scratch
>   TEST_DEV=/dev/loop16
>   TEST_LOGDEV=/dev/loop13
>   SCRATCH_DEV_POOL="/dev/loop5 /dev/loop6 /dev/loop7 /dev/loop8 /dev/loop9 /dev/loop10 /dev/loop11 /dev/loop12"
>   MKFS_OPTIONS='-f -m crc=1,reflink=0,rmapbt=0, -i sparse=0 -lsize=1g'
>   TEST_FS_MOUNT_OPTS="-o logdev=/dev/loop13"
>   MOUNT_OPTIONS='-o usrquota,grpquota,prjquota'
>   TEST_FS_MOUNT_OPTS="$TEST_FS_MOUNT_OPTS -o usrquota,grpquota,prjquota"
>   SCRATCH_LOGDEV=/dev/loop15
>   USE_EXTERNAL=yes
>   LOGWRITES_DEV=/dev/loop15
> 
> Git bisect produced the following as the first bad commit,
> 
> commit 943bc0882cebf482422640924062a7daac5a27ba
> Author: Zhang Yi <yi.zhang@huawei.com>
> Date:   Wed Mar 20 19:05:45 2024 +0800
> 
>     iomap: don't increase i_size if it's not a write operation
> 
>     Increase i_size in iomap_zero_range() and iomap_unshare_iter() is not
>     needed, the caller should handle it. Especially, when truncate partial
>     block, we should not increase i_size beyond the new EOF here. It doesn't
>     affect xfs and gfs2 now because they set the new file size after zero
>     out, it doesn't matter that a transient increase in i_size, but it will
>     affect ext4 because it set file size before truncate. So move the i_size
>     updating logic to iomap_write_iter().
> 
>     Signed-off-by: Zhang Yi <yi.zhang@huawei.com>
>     Link: https://lore.kernel.org/r/20240320110548.2200662-7-yi.zhang@huaweicloud.com
>     Reviewed-by: Christoph Hellwig <hch@lst.de>
>     Reviewed-by: Darrick J. Wong <djwong@kernel.org>
>     Signed-off-by: Christian Brauner <brauner@kernel.org>
> 
>  fs/iomap/buffered-io.c | 50 +++++++++++++++++++++++++-------------------------
>  1 file changed, 25 insertions(+), 25 deletions(-)
>  
>