Received-SPF: pass (google.com: domain of linux-ext4-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Sat, 16 Jul 2022 02:08:15 +0800
From:   Zorro Lang <zlang@redhat.com>
To:     Theodore Ts'o <tytso@mit.edu>, Sun Ke <sunke32@huawei.com>
Cc:     fstests@vger.kernel.org, linux-ext4@vger.kernel.org
Subject: Re: [PATCH v3 1/2] ext4: resize fs after resize_inode without e2fsck
Message-ID: <20220715180815.gegmapvruor6vin3@zlang-mailbox>
References: <20220713092859.3881376-1-sunke32@huawei.com>
 <20220713092859.3881376-2-sunke32@huawei.com>
 <20220714154607.qq6cqgvncxhsn66w@zlang-mailbox>
 <YtCSAjiMc9RElnHu@mit.edu>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YtCSAjiMc9RElnHu@mit.edu>
Precedence: bulk

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
>