Re: [PATCH v3] ext4/030: Ext4 online resize with bigalloc tests.

[Eryu Guan <eguan@redhat.com>]

On Thu, Jan 04, 2018 at 10:09:08AM +0200, Amir Goldstein wrote:
> On Thu, Jan 4, 2018 at 3:18 AM, harshads <harshads@google.com> wrote:
> > Add tests to verify Ext4 online resizing feature with bigalloc feature
> > enabled. We test various resizing scenarios with different cluster
> > sizes.
> >
> > Signed-off-by: Harshad Shirwadkar <harshads@google.com>
> > ---
> >  common/rc          |  23 ++++++++
> >  tests/ext4/030     | 158 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/ext4/030.out | 148 +++++++++++++++++++++++++++++++++++++++++++++++++
> >  tests/ext4/group   |   1 +
> >  4 files changed, 330 insertions(+)
> >  create mode 100755 tests/ext4/030
> >  create mode 100644 tests/ext4/030.out
> >
> > diff --git a/common/rc b/common/rc
> > index 9216efdb..052dadae 100644
> > --- a/common/rc
> > +++ b/common/rc
> > @@ -1845,6 +1845,29 @@ _require_scratch_ext4_feature()
> >      _scratch_unmount
> >  }
> >
> > +# Check whether the specified feature whether it is supported by
> > +# mkfs.ext4 and the kernel by using a sparse file image.
> > +_require_ext4_feature()
> 
> 1. please explain why this loop variant is needed
> 2. it would be great if you could also change callers to
> _require_scratch_ext4_feature
>     to use _require_scratch_feature and plug
> _require_scratch_ext4_feature in there
> 3. probably best to post this as a separate patch from the test itself

I haven't went through the whole patch yet, just want to point out that
the _require_scratch_ext4_feature helper has been added in commit
be341e36fd02 ("common: rework _require_ext4_mkfs_feature"), but not
enabled in the more generic _require_scratch_feature helper yet.

It'd be good to plug the ext4 helper to the generic helper in a separate
patch, as Amir suggested, and perhaps converting all existing callers of
the ext4 helper to the generic helper.

Thanks,
Eryu

> 
> > +{
> > +    if [ -z "$1" ]; then
> > +        echo "Usage: _require_loop_ext4_feature feature"
> 
> So which is it, _require_loop_ext4_feature or _require_ext4_feature?
> First one sounds better to me, given that you explain why the loop
> variant is needed.
> If it is needed, will it be useful to have for other fs?
> Then better implement _require_loop_feature and make ext4
> a specific case.
> 
> Cheers,
> Amir.