Re: [PATCH v2] generic: shutdown might leave NULL files with nonzero di_size

["Darrick J. Wong" <djwong@kernel.org>]

On Tue, Nov 08, 2022 at 02:02:44PM +0800, Zorro Lang wrote:
> On Mon, Nov 07, 2022 at 08:41:47AM -0800, Darrick J. Wong wrote:
> > On Sat, Nov 05, 2022 at 11:23:24PM +0800, Zorro Lang wrote:
> > > An old issue might cause on-disk inode sizes are logged prematurely
> > > via the free eofblocks path on file close. Then fs shutdown might
> > > leave NULL files but their di_size > 0.
> > > 
> > > Signed-off-by: Zorro Lang <zlang@kernel.org>
> > > ---
> > > 
> > > V2 replace "fiemap" with "stat" command, to check if a file has extents.
> > > That helps this case more common.
> > > 
> > > Thanks,
> > > Zorro
> > > 
> > >  tests/generic/999     | 42 ++++++++++++++++++++++++++++++++++++++++++
> > >  tests/generic/999.out |  5 +++++
> > >  2 files changed, 47 insertions(+)
> > >  create mode 100755 tests/generic/999
> > >  create mode 100644 tests/generic/999.out
> > > 
> > > diff --git a/tests/generic/999 b/tests/generic/999
> > > new file mode 100755
> > > index 00000000..8b4596e0
> > > --- /dev/null
> > > +++ b/tests/generic/999
> > 
> > Ugh sorry     ^^^^^^^ I didn't notice this part and wrote my previous
> > response thinking this was an xfs-only test...
> 
> Oh, my bad, I forgot to change the "_supported_fs xfs" to "generic".
> 
> > 
> > > @@ -0,0 +1,42 @@
> > > +#! /bin/bash
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +# Copyright (c) 2022 Red Hat, Inc.  All Rights Reserved.
> > > +#
> > > +# FS QA Test No. 999
> > > +#
> > > +# Test an issue in the truncate codepath where on-disk inode sizes are logged
> > > +# prematurely via the free eofblocks path on file close.
> > > +#
> > > +. ./common/preamble
> > > +_begin_fstest auto quick shutdown
> > > +
> > > +# real QA test starts here
> > > +_supported_fs xfs
> > > +_require_scratch
> > > +_require_scratch_shutdown
> > > +_scratch_mkfs > $seqres.full 2>&1
> > > +_scratch_mount
> > > +
> > > +echo "Create many small files with one extent at least"
> > > +for ((i=0; i<10000; i++));do
> > > +	$XFS_IO_PROG -f -c "pwrite 0 4k" $SCRATCH_MNT/file.$i >/dev/null 2>&1
> > > +done
> > > +
> > > +echo "Shutdown the fs suddently"
> > > +_scratch_shutdown
> > > +
> > > +echo "Cycle mount"
> > > +_scratch_cycle_mount
> > > +
> > > +echo "Check file's (di_size > 0) extents"
> > > +for f in $(find $SCRATCH_MNT -type f -size +0);do
> > > +	# Check if the file has any extent
> > > +	if [ "$(stat -c "%b" $f)" = "0" ];then
> > > +		echo " - $f get no extents, but its di_size > 0"
> > > +		break
> > > +	fi
> > > +done
> > 
> > ...so whereas I was trying to suggest that you could use the GETFSXATTR
> > ioctl to return the extent count:
> > 
> > $XFS_IO_PROG -c stat $f | grep fsxattr.nextents | awk '{print $3}'
> > 
> > But that won't work outside of XFS.  To make this generic, I think you
> > have to do something like:
> > 
> > $FILEFRAG_PROG -v $f | wc -l
> 
> I'm wondering if we must check extent is 0, how about check allocated block = 0?
> I tried [ "$(stat -c "%b" $f)" = "0" ], it fails on old kernel without this bug
> fix, and test passed on new kernel. Does that make sense for you?

What if an LSM (e.g. selinux) attach enough security attrs to push the
xattr data into an external block?  I don't think that happens often for
XFS, but I think it will for ext3 with 128byte inodes.

--D

> Thanks,
> Zorro
> 
> > 
> > to see if there are any extents.
> > 
> > --D
> > 
> > > +
> > > +# success, all done
> > > +status=0
> > > +exit
> > > diff --git a/tests/generic/999.out b/tests/generic/999.out
> > > new file mode 100644
> > > index 00000000..50008783
> > > --- /dev/null
> > > +++ b/tests/generic/999.out
> > > @@ -0,0 +1,5 @@
> > > +QA output created by 999
> > > +Create many small files with one extent at least
> > > +Shutdown the fs suddently
> > > +Cycle mount
> > > +Check file's (di_size > 0) extents
> > > -- 
> > > 2.31.1
> > > 
> > 
>