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;
Subject: Re: [PATCH v3 1/2] ext4: resize fs after resize_inode without e2fsck
To:     Zorro Lang <zlang@redhat.com>, Theodore Ts'o <tytso@mit.edu>
CC:     <fstests@vger.kernel.org>, <linux-ext4@vger.kernel.org>
References: <20220713092859.3881376-1-sunke32@huawei.com>
 <20220713092859.3881376-2-sunke32@huawei.com>
 <20220714154607.qq6cqgvncxhsn66w@zlang-mailbox> <YtCSAjiMc9RElnHu@mit.edu>
 <20220715180815.gegmapvruor6vin3@zlang-mailbox>
From:   Sun Ke <sunke32@huawei.com>
Message-ID: <5a2415ab-e40f-da66-abc0-aca2d49d77d1@huawei.com>
Date:   Thu, 21 Jul 2022 11:24:32 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:78.0) Gecko/20100101
 Thunderbird/78.6.1
MIME-Version: 1.0
In-Reply-To: <20220715180815.gegmapvruor6vin3@zlang-mailbox>
Content-Type: text/plain; charset="gbk"; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk


?? 2022/7/16 2:08, Zorro Lang д??:
> 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.

Glad to do that, it can reproduce the bug.

Thanks,
Sun Ke
> 
> _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
>>
> 
> .
>