Re: [PATCH] overlay: add test for copy up of lower file attributes

[Eryu Guan <eguan@linux.alibaba.com>]

On Sun, Jul 25, 2021 at 09:21:03PM +0300, Amir Goldstein wrote:
> On Sun, Jul 25, 2021 at 7:27 PM Eryu Guan <guan@eryu.me> wrote:
> >
> > On Thu, Jul 22, 2021 at 07:46:34PM +0300, Amir Goldstein wrote:
> > > Overlayfs copies up a subset of lower file attributes since kernel
> > > commits:
> > > 173ff5c9ec37 ("ovl: consistent behavior for immutable/append-only inodes")
> > > 2e3f6e87c2b0 ("ovl: copy up sync/noatime fileattr flags")
> > >
> > > This test verifies this functionality works correctly and that it
> > > survives power failure and/or mount cycle.
> > >
> > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> >
> > Looks good to me overall, just one minor question below.
> >
> > > ---
> > >
> > > Eryu,
> > >
> > > This test is failing on master and passes on overlayfs-next.
> > >
> > > Thanks,
> > > Amir.
> > >
> > >  tests/overlay/078     | 145 ++++++++++++++++++++++++++++++++++++++++++
> > >  tests/overlay/078.out |   2 +
> > >  2 files changed, 147 insertions(+)
> > >  create mode 100755 tests/overlay/078
> > >  create mode 100644 tests/overlay/078.out
> > >
> > > diff --git a/tests/overlay/078 b/tests/overlay/078
> > > new file mode 100755
> > > index 00000000..b43449d1
> > > --- /dev/null
> > > +++ b/tests/overlay/078
> > > @@ -0,0 +1,145 @@
> > > +#! /bin/bash
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +# Copyright (c) 2018 Huawei.  All Rights Reserved.
> > > +# Copyright (C) 2021 CTERA Networks. All Rights Reserved.
> > > +#
> > > +# FS QA Test 078
> > > +#
> > > +# Test copy up of lower file attributes.
> > > +#
> > > +# Overlayfs copies up a subset of lower file attributes since kernel commits:
> > > +# 173ff5c9ec37 ("ovl: consistent behavior for immutable/append-only inodes")
> > > +# 2e3f6e87c2b0 ("ovl: copy up sync/noatime fileattr flags")
> > > +#
> > > +# This test is similar and was derived from generic/507, but instead
> > > +# of creating new files which are created in upper layer, prepare
> > > +# the file with attributes in lower layer and verify that attributes
> > > +# are not lost during copy up, (optional) shutdown and mount cycle.
> > > +#
> > > +. ./common/preamble
> > > +_begin_fstest auto quick perms shutdown
> >
> > I noticed that generic/507 has the same groups defined, but I'm
> > wondering if 'perms' is right group, 'attr' seems a better fit to me.
> 
> The term "attr" is now very much overloaded in filesystems.
> It may refer to info of stat() (i.e. getattr())
> it my refer to xattr and it may refer to lsattr/chattr,
> which is referred to as fileattr in latest vfs API.
> 
> In fstests, most of the tests in the 'attr' group include
> ./common/attr which refers to ACL xattrs in particular...
> so it is not a good fit IMO.
> 
> The group 'perm' is already used for overlayfs tests that deal with
> immutable files and generic tests that deal with immutable files
> don't really have a common group AFAICT.

Makes sense to me. Then I'll take 'perm' for now.

> 
> I have no objection for creating a new group for this
> purpose but that would involve marking all related tests and worse...
> ...finding a name for that group ;-)

Yeah, naming is always a problem :)

> 
> > And we could add 'copyup' group as well.
> >
> 
> Sure.
> 
> > > +
> > > +# Override the default cleanup function.
> > > +_cleanup()
> > > +{
> > > +     cd /
> > > +     $CHATTR_PROG -ai $lowertestfile &> /dev/null
> > > +     rm -f $tmp.*
> > > +}
> > > +
> > > +# Import common functions.
> > > +. ./common/filter
> > > +
> > > +# real QA test starts here
> > > +_supported_fs generic
> >
> > s/generic/overlay/
> 
> Oops. I assume you would be fix this typo on commit.

Sure, I'll pick it up in next week's update and fix supported fs and add
copyup group.

Thanks,
Eryu