| Message ID | 1582177081-6235-1-git-send-email-xuyang2018.jy@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] common/log: add -l su option in _xfs_log_config | expand |
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 > with ext4 test num(10) and we can test all logbuf size. > > 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" > + echo " version=2,su=16k logbsize=16k" > echo " version=2,su=4096 logbsize=32k" > - echo " version=2,su=32768 logbsize=32k" > - echo " version=2,su=32768 logbsize=64k" > - 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 > > >
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() > > with ext4 test num(10) and we can test all logbuf size. Hm? ext4/010 is an inode bitmap fuzz test. > > > > 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 > > > > > >
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 >>> >>> >>> > >
on 2020/02/24 9:51, Yang Xu wrote: > > > 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? For 16k logbsize, I don't know much about it. it may make no sense? So at the beginning, xfstests doesn't plan to test it. If so, I will remove this adding. >> >>>> 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. OK. how about this change , as below(only specifying su=4096 for old options without su, logbusize=32K has su=4096, so add su=16k for it): diff --git a/common/log b/common/log index c7921f50..991c8e8f 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=32k" + echo " version=2,su=16384 logbsize=32k" echo " version=2,su=32768 logbsize=32k" - echo " version=2,su=32768 logbsize=64k" - echo " version=2 logbsize=64k" + echo " version=2,su=4096 logbsize=64k" + echo " version=2,su=32768 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" Best Regards Yang Xu >> >> --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 >>>> >>>> >>>> >> >> > >
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" + echo " version=2,su=16k logbsize=16k" echo " version=2,su=4096 logbsize=32k" - echo " version=2,su=32768 logbsize=32k" - echo " version=2,su=32768 logbsize=64k" - 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" }
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 with ext4 test num(10) and we can test all logbuf size. Signed-off-by: Yang Xu <xuyang2018.jy@cn.fujitsu.com> --- common/log | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)