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

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

[Zorro Lang]

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
@@ -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
+
+# 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

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

[Darrick J. Wong]

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...

> @@ -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

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
> 

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

[Zorro Lang]

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?

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
> > 
> 

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

[Darrick J. Wong]

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
> > > 
> > 
> 

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

[Zorro Lang]

On Tue, Nov 08, 2022 at 07:52:14AM -0800, Darrick J. Wong wrote:
> 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

Oh! You're right, I forgot the ablocks. Due to fstests always use
"-o context=system_u:object_r:root_t:s0" to mount, I thought there's not
selinux labels to take xattr space by default, so ablocks generally is 0 (I think:).

But I agree with you, "$(stat -c "%b" $f)" can be affected by ablocks, so
filefrag might be a better solution.

Thanks,
Zorro

> 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
> > > > 
> > > 
> > 
>