Re: [PATCH v2] common/log: add -l su option in _xfs_log_config

[Yang Xu <xuyang2018.jy@cn.fujitsu.com>]

on 2020/02/24 0:37, Darrick J. Wong wrote:
> On Sun, Feb 23, 2020 at 10:22:10PM +0800, Eryu Guan wrote:
>> Hi Darrick,
>>
>> On Thu, Feb 20, 2020 at 01:38:01PM +0800, Yang Xu wrote:
>>> Currently, if we don't specify -l sunit or -l su option, mkfs.xfs
>>> will get the stripe size from underlying device.
>>>
>>> It works file on most situations. But on some machine, the size of
>>> underlying device greater than logbsize of mount options, it will
>>> report error like "logbuf size must be greater than or equal to log
>>> stripe size". We can specify -l su=4096 to meet this requirement and
>>> case can still run normally.
>>>
>>> Also, from xfs manpage, version 2 also supports 16k log buf size for
>>> mount option and case passed(only generic/054,055 used this api) on
>>> my machine. So delete 32k and 64k with different sunit to be consistented
> 
> I don't understand why there's a need to be consistent, which means I
> don't understand why we'd "delete 32k and 64k with different sunit".
> ext4 tests should not be invoking _xfs_log_config()

Sorry for the confused message.
_get_log_configs{
xfs)
         _xfs_log_config
         ;;
     f2fs)
         _f2fs_log_config
         ;;
     ext4)
         _ext4_log_config

}
xfs ,f2fs, and ext4 all have test1~test10.
So for a generic case, it should be consistent.
And only generic/054 055 uses _get_log_configs api and no test case
used _xfs_log_config api. I think we should keep this(test1~test10, 
054.out only 10 times mkfs and mount output)
> 
>>> with ext4 test num(10) and we can test all logbuf size.
> 
> Hm?  ext4/010 is an inode bitmap fuzz test.
>
No. I mean _ext4_log_config has test1~test10.

Best Regards
Yang Xu
>>>
>>> Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com>
>>
>> Would you like to review this v2 patch again? I feel more confident if
>> xfs maintainer could ack it :)
>>
>> Thanks,
>> Eryu
>>
>>> ---
>>>   common/log | 12 ++++++------
>>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/common/log b/common/log
>>> index c7921f50..9b5a2f6d 100644
>>> --- a/common/log
>>> +++ b/common/log
>>> @@ -546,15 +546,15 @@ _xfs_log_config()
>>>   {
>>>       echo "# mkfs-opt             mount-opt"
>>>       echo "# ------------------------------"
>>> -    echo "  version=2            logbsize=32k"
>>> +    echo "  version=2,su=4096    logbsize=16k"
> 
> The straight substitutions look fine to me, but then...
> 
>>> +    echo "  version=2,su=16k     logbsize=16k"
> 
> ...I can't tell why we're adding this extra case here, or why this
> has to be here and not in a separate patch justifying the addition?
> 
>>>       echo "  version=2,su=4096    logbsize=32k"
>>> -    echo "  version=2,su=32768   logbsize=32k"
>>> -    echo "  version=2,su=32768   logbsize=64k"
> 
> ...I also don't think it's a good idea to reduce test coverage, and
> definitely not buried in something that sounds like a fix patch.
> 
> --D
> 
>>> -    echo "  version=2            logbsize=64k"
>>> +    echo "  version=2,su=32k     logbsize=32k"
>>> +    echo "  version=2,su=4096    logbsize=64k"
>>>       echo "  version=2,su=64k     logbsize=64k"
>>> -    echo "  version=2            logbsize=128k"
>>> +    echo "  version=2,su=4096    logbsize=128k"
>>>       echo "  version=2,su=128k    logbsize=128k"
>>> -    echo "  version=2            logbsize=256k"
>>> +    echo "  version=2,su=4096    logbsize=256k"
>>>       echo "  version=2,su=256k    logbsize=256k"
>>>   }
>>>   
>>> -- 
>>> 2.18.0
>>>
>>>
>>>
> 
>