| Message ID | 20221105152324.2233310-1-zlang@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] generic: shutdown might leave NULL files with nonzero di_size | expand |
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 >
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 > > >
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 > > > > > >
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 > > > > > > > > > >
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
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