On Thu, Jul 14, 2022 at 06:00:34PM -0400, Theodore Ts'o wrote: > On Thu, Jul 14, 2022 at 11:46:07PM +0800, Zorro Lang wrote: > > On Wed, Jul 13, 2022 at 05:28:58PM +0800, Sun Ke wrote: > > > + > > > +# forget to run requested e2fsck after resize_inode > > > +$TUNE2FS_PROG -O ^resize_inode $SCRATCH_DEV | grep -w "e2fsck" > > > + > > > +_scratch_mount > > > + > > > +# resize fs will trigger NULL pointer in ext4_flex_group_add > > > +$RESIZE2FS_PROG $SCRATCH_DEV 1G >> $seqres.full 2>&1 > > > + > > > +echo "Silence is golden" > ... > > > diff --git a/tests/ext4/057.out b/tests/ext4/057.out > > > new file mode 100644 > > > index 00000000..4784ad7e > > > --- /dev/null > > > +++ b/tests/ext4/057.out > > > @@ -0,0 +1,3 @@ > > > +QA output created by 057 > > > +Please run e2fsck -f on the filesystem. > > > > If you hope to match this line, means this case isn't "Silence is golden". > > > > I don't know why you'd to have this line, it looks not suit to be golden > > image. If you'd like to make sure current ext4 supports "resize_inode" > > feature, you can use: > > _require_scratch_ext4_feature resize_inode > > That's not the problem. > > The "tune2fs -O ^resize_inode" command is printing that message as a > reminder that it would be a Really Good idea to run e2fsck on the file > system, because tune2fs doesn't completely remove the resize inode > after turning off that feature. > > The commit which this test is trying to verify is that the kernel > won't oops if the system adminsitrator ignores the rather explicit > request: > > Please run e2fsck -f on the filesystem. > > ... and blithely mounts the file system without running fsck -f on the > file system first. While it could be argued that a system > administrator which fails to follow instructions deserves everything > they get, we decided the as a quality of implementation issue, it > would be better if the kernel didn't dereference a NULL pointer in > that case. :-) > > The one thing I'll note is that it is possible that at some point in > the future, tune2fs could be improved so that it cleanly removes the > resize_inode when the resize inode feature is removed, so that running > "fsck.ext4 -f" is no longer necessary. So if you want to future-proof Good to know :) > the test so it doesn't fail once tune2fs is made more idiot-proof, it > might be better if the test did something like this: > > mke2fs -t ext4 -O ^resize_inode /dev/vdc 512m > debugfs -w -R "set_super_value s_reserved_gdt_blocks 100" /dev/vdc So make sure there're reserved GDT blocks, even if disable resize_inode feature. > mount -t ext4 /dev/vdc /vdc > resize2fs /dev/vdc 1G Thanks Ted! That's really helpful to get review points from ext4 expert. Hi Ke, would you mind re-sending this case refer to above review points? You can refer to below code, but I didn't test it, so please test and make sure it works and can reproduce the bug. Feel free to improve it if something wrong. _require_command "$DEBUGFS_PROG" debugfs ... MKFS_OPTIONS="-O ^resize_inode $MKFS_OPTIONS" _scratch_mkfs_sized $dev_size \ >>$seqres.full 2>&1 || _fail "mkfs failed" $DEBUGFS_PROG -w -R "set_super_value s_reserved_gdt_blocks 100" $SCRATCH_DEV \ >>$seqres.full 2>&1 $DEBUGFS_PROG -R "show_super_stats -h" $SCRATCH_DEV 2>/dev/null | \ grep "Reserved GDT blocks" _scratch_mount $RESIZE2FS_PROG $SCRATCH_DEV 1g >> $seqres.full 2>&1 Thanks, Zorro > > Translating the above from commands suitable for manual trial using > "kvm-xfstests shell" to a proper xfstests script is left as an > exercise for the reader. :-) > > - Ted >
