Re: [PATCH] xfstests: xfs/191 update

Re: [PATCH] xfstests: xfs/191 update

[Eryu Guan]

On Wed, Mar 15, 2017 at 05:33:47PM +0100, Jan Tulak wrote:
> The removed 'do_mkfs_pass -l size=4096b' was against man page
> (-b section). Other entries are things that weren't covered before.
> 
> Specifically, a standalone "-l size=4096b" should fail, because:
> To  specify any options on the command line in units of filesys‐
> tem blocks, this option must be  specified  first  so  that  the
> filesystem block size is applied consistently to all options.
> 
> Signed-off-by: Jan Tulak <jtulak@redhat.com>

This looks fine to me. But I'd like some reviews from xfs developers
too. (cc'd linux-xfs@vger.kernel.org).

Thanks,
Eryu

> ---
>  tests/xfs/191-input-validation | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/xfs/191-input-validation b/tests/xfs/191-input-validation
> index 5de55170..e22f9bfc 100755
> --- a/tests/xfs/191-input-validation
> +++ b/tests/xfs/191-input-validation
> @@ -114,6 +114,9 @@ do_mkfs_fail -d sectsize=2foo $SCRATCH_DEV
>  do_mkfs_fail -l sectsize=nggh $SCRATCH_DEV
>  do_mkfs_fail -l sectsize=2nggh $SCRATCH_DEV
>  
> +# option respecification should fail
> +do_mkfs_fail -d agcount=32,agcount=32 $SCRATCH_DEV
> +
>  # conflicting sector/block sizes
>  do_mkfs_fail -s size=512 -d sectsize=1024 $SCRATCH_DEV
>  do_mkfs_fail -s size=512 -l sectsize=1024 $SCRATCH_DEV
> @@ -191,6 +194,12 @@ do_mkfs_fail -d sectsize=512s,agsize=65536s $SCRATCH_DEV
>  
>  reset_fsimg
>  
> +# some sectsize/blocksize combinations
> +do_mkfs_pass -s size=512 -b size=8s $SCRATCH_DEV
> +do_mkfs_fail -s size=512s $SCRATCH_DEV
> +
> +reset_fsimg
> +
>  # file section, should pass
>  do_mkfs_pass $fsimg
>  do_mkfs_pass -d file=0 $SCRATCH_DEV
> @@ -218,7 +227,6 @@ reset_fsimg
>  # log section, should pass
>  do_mkfs_pass -l size=$logsize -d size=$fssize $SCRATCH_DEV
>  do_mkfs_pass -l agnum=2 $SCRATCH_DEV
> -do_mkfs_pass -l size=4096b $SCRATCH_DEV
>  do_mkfs_pass -l sectsize=512 $SCRATCH_DEV
>  do_mkfs_pass -l sunit=64 $SCRATCH_DEV
>  do_mkfs_pass -l sunit=64 -d sunit=8,swidth=8 $SCRATCH_DEV
> @@ -237,6 +245,7 @@ do_mkfs_pass -l version=2 -m crc=0 $SCRATCH_DEV
>  do_mkfs_pass -l version=2 $SCRATCH_DEV
>  
>  # log section, should fail
> +do_mkfs_fail -l size=4096b $SCRATCH_DEV
>  do_mkfs_fail -l size=${fssize}b $SCRATCH_DEV
>  do_mkfs_fail -l size=${fssize}s $SCRATCH_DEV
>  do_mkfs_fail -l size=${fssize}yerk $SCRATCH_DEV
> -- 
> 2.11.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xfstests: xfs/191 update

[Brian Foster]

On Thu, Mar 16, 2017 at 03:49:41PM +0800, Eryu Guan wrote:
> On Wed, Mar 15, 2017 at 05:33:47PM +0100, Jan Tulak wrote:
> > The removed 'do_mkfs_pass -l size=4096b' was against man page
> > (-b section). Other entries are things that weren't covered before.
> > 
> > Specifically, a standalone "-l size=4096b" should fail, because:
> > To  specify any options on the command line in units of filesys‐
> > tem blocks, this option must be  specified  first  so  that  the
> > filesystem block size is applied consistently to all options.
> > 
> > Signed-off-by: Jan Tulak <jtulak@redhat.com>
> 
> This looks fine to me. But I'd like some reviews from xfs developers
> too. (cc'd linux-xfs@vger.kernel.org).
> 

So has mkfs been fixed to cause '-l size=4096b' to fail? I'm not sure we
should cause the test to fail until/unless the mkfs behavior is fixed
up.

Brian

> Thanks,
> Eryu
> 
> > ---
> >  tests/xfs/191-input-validation | 11 ++++++++++-
> >  1 file changed, 10 insertions(+), 1 deletion(-)
> > 
> > diff --git a/tests/xfs/191-input-validation b/tests/xfs/191-input-validation
> > index 5de55170..e22f9bfc 100755
> > --- a/tests/xfs/191-input-validation
> > +++ b/tests/xfs/191-input-validation
> > @@ -114,6 +114,9 @@ do_mkfs_fail -d sectsize=2foo $SCRATCH_DEV
> >  do_mkfs_fail -l sectsize=nggh $SCRATCH_DEV
> >  do_mkfs_fail -l sectsize=2nggh $SCRATCH_DEV
> >  
> > +# option respecification should fail
> > +do_mkfs_fail -d agcount=32,agcount=32 $SCRATCH_DEV
> > +
> >  # conflicting sector/block sizes
> >  do_mkfs_fail -s size=512 -d sectsize=1024 $SCRATCH_DEV
> >  do_mkfs_fail -s size=512 -l sectsize=1024 $SCRATCH_DEV
> > @@ -191,6 +194,12 @@ do_mkfs_fail -d sectsize=512s,agsize=65536s $SCRATCH_DEV
> >  
> >  reset_fsimg
> >  
> > +# some sectsize/blocksize combinations
> > +do_mkfs_pass -s size=512 -b size=8s $SCRATCH_DEV
> > +do_mkfs_fail -s size=512s $SCRATCH_DEV
> > +
> > +reset_fsimg
> > +
> >  # file section, should pass
> >  do_mkfs_pass $fsimg
> >  do_mkfs_pass -d file=0 $SCRATCH_DEV
> > @@ -218,7 +227,6 @@ reset_fsimg
> >  # log section, should pass
> >  do_mkfs_pass -l size=$logsize -d size=$fssize $SCRATCH_DEV
> >  do_mkfs_pass -l agnum=2 $SCRATCH_DEV
> > -do_mkfs_pass -l size=4096b $SCRATCH_DEV
> >  do_mkfs_pass -l sectsize=512 $SCRATCH_DEV
> >  do_mkfs_pass -l sunit=64 $SCRATCH_DEV
> >  do_mkfs_pass -l sunit=64 -d sunit=8,swidth=8 $SCRATCH_DEV
> > @@ -237,6 +245,7 @@ do_mkfs_pass -l version=2 -m crc=0 $SCRATCH_DEV
> >  do_mkfs_pass -l version=2 $SCRATCH_DEV
> >  
> >  # log section, should fail
> > +do_mkfs_fail -l size=4096b $SCRATCH_DEV
> >  do_mkfs_fail -l size=${fssize}b $SCRATCH_DEV
> >  do_mkfs_fail -l size=${fssize}s $SCRATCH_DEV
> >  do_mkfs_fail -l size=${fssize}yerk $SCRATCH_DEV
> > -- 
> > 2.11.0
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe fstests" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> --
> To unsubscribe from this list: send the line "unsubscribe fstests" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] xfstests: xfs/191 update

[Eryu Guan]

On Tue, Mar 21, 2017 at 11:22:54AM -0400, Brian Foster wrote:
> On Thu, Mar 16, 2017 at 03:49:41PM +0800, Eryu Guan wrote:
> > On Wed, Mar 15, 2017 at 05:33:47PM +0100, Jan Tulak wrote:
> > > The removed 'do_mkfs_pass -l size=4096b' was against man page
> > > (-b section). Other entries are things that weren't covered before.
> > > 
> > > Specifically, a standalone "-l size=4096b" should fail, because:
> > > To  specify any options on the command line in units of filesys‐
> > > tem blocks, this option must be  specified  first  so  that  the
> > > filesystem block size is applied consistently to all options.
> > > 
> > > Signed-off-by: Jan Tulak <jtulak@redhat.com>
> > 
> > This looks fine to me. But I'd like some reviews from xfs developers
> > too. (cc'd linux-xfs@vger.kernel.org).
> > 
> 
> So has mkfs been fixed to cause '-l size=4096b' to fail? I'm not sure we
> should cause the test to fail until/unless the mkfs behavior is fixed
> up.

I agreed, we don't want to update existing test and introduce false
regressions. This is one of the reasons I want more reviews on this
patch :)

Thanks,
Eryu

Re: [PATCH] xfstests: xfs/191 update

[Jan Tulak]

On Tue, Mar 21, 2017 at 4:55 PM, Eryu Guan <eguan@redhat.com> wrote:
> On Tue, Mar 21, 2017 at 11:22:54AM -0400, Brian Foster wrote:
>> On Thu, Mar 16, 2017 at 03:49:41PM +0800, Eryu Guan wrote:
>> > On Wed, Mar 15, 2017 at 05:33:47PM +0100, Jan Tulak wrote:
>> > > The removed 'do_mkfs_pass -l size=4096b' was against man page
>> > > (-b section). Other entries are things that weren't covered before.
>> > >
>> > > Specifically, a standalone "-l size=4096b" should fail, because:
>> > > To  specify any options on the command line in units of filesys‐
>> > > tem blocks, this option must be  specified  first  so  that  the
>> > > filesystem block size is applied consistently to all options.
>> > >
>> > > Signed-off-by: Jan Tulak <jtulak@redhat.com>
>> >
>> > This looks fine to me. But I'd like some reviews from xfs developers
>> > too. (cc'd linux-xfs@vger.kernel.org).
>> >
>>
>> So has mkfs been fixed to cause '-l size=4096b' to fail? I'm not sure we
>> should cause the test to fail until/unless the mkfs behavior is fixed
>> up.
>
> I agreed, we don't want to update existing test and introduce false
> regressions. This is one of the reasons I want more reviews on this
> patch :)
>

I forgot to mention that this patch is a complement to my patchset for
mkfs.xfs. So yes, do not merge it now, it looks like the set won't get
merged yet. Sorry for not stating it in the description.

Jan


-- 
Jan Tulak
jtulak@redhat.com / jan@tulak.me