Re: [PATCH] common/populate: Ensure that S_IFDIR.FMT_BTREE is in btree format

[Ziyang Zhang <ZiyangZhang@linux.alibaba.com>]

On 2022/12/8 05:48, Dave Chinner wrote:
> On Wed, Dec 07, 2022 at 10:11:49AM +0800, Gao Xiang wrote:
>> On Wed, Dec 07, 2022 at 10:34:17AM +1100, Dave Chinner wrote:
>>> On Fri, Dec 02, 2022 at 10:23:07AM +0800, Gao Xiang wrote:
>>>>>> +			[ "$nexts" -gt "$(((isize - 176) / 16))" ] && break
>>>>>
>>>>> Only need to calculate this once if you declare this at the top:
>>>>>
>>>>> 	# We need enough extents to guarantee that the data fork is in
>>>>> 	# btree format.  Cycling the mount to use xfs_db is too slow, so
>>>>> 	# watch for when the extent count exceeds the space after the
>>>>> 	# inode core.
>>>>> 	local max_nextents="$(((isize - 176) / 16))"
>>>>>
>>>>> and then you can do:
>>>>>
>>>>> 			[[ $nexts -gt $max_nextents ]] && break
>>>>>
>>>>> Also not a fan of hardcoding 176 around fstests, but I don't know how
>>>>> we'd detect that at all.
>>>>>
>>>>> # Number of bytes reserved for only the inode record, excluding the
>>>>> # immediate fork areas.
>>>>> _xfs_inode_core_bytes()
>>>>> {
>>>>> 	echo 176
>>>>> }
>>>>>
>>>>> I guess?  Or extract it from tests/xfs/122.out?
>>>>
>>>> Thanks for your comments.
>>>>
>>>> I guess hard-coded 176 in _xfs_inode_core_bytes() is fine for now
>>>> (It seems a bit weird to extract a number from a test expected result..)
>>>
>>> Which is wrong when testing a v4 filesystem - in that case the inode
>>> core size is 96 bytes and so max extents may be larger on v4
>>> filesystems than v5 filesystems....
>>
>> Do we really care v4 fs for now since it's deprecated?...
> 
> Yes, there are still lots of v4 filesystems in production
> environments. There shouldn't be many new ones, but there is a long
> tail of existing storage containing v4 filesystems that we must not
> break.
> 
> We have to support v4 filesystems for another few years yet, hence
> we still need solid test coverage on them to ensure we don't
> accidentally break something that is going to bite users before they
> migrate to newer filesystems....
> 
>> Darrick once also 
>> suggested using (isize / 16) but it seems it could take unnecessary time to
>> prepare.. Or we could just use (isize - 96) / 16 to keep v4 work.
> 
> It's taken me longer to write this email than it does to write the
> code to make it work properly. e.g.:
> 
> 	xfs_info $scratch | sed -ne 's/.*crc=\([01]\).*/\1/p'
> 
> And now we have 0 = v4, 1 = v5, and it's trivial to return the
> correct inode size.
> 
> You can even do this trivially with grep:
> 
> 	xfs_info $scratch | grep -wq "crc=1"
> 	if [ $? -eq 0 ]; then
> 		echo 176
> 	else
> 		echo 96
> 	fi
> 
> and now the return value tells us if we have a v4 or v5 filesystem.
> 
> -Dave.

Hi, David

I have written new versions, please see:
https://lore.kernel.org/fstests/20221207093147.1634425-1-ZiyangZhang@linux.alibaba.com/

Regards,
Zhang