Re: [PATCH v3] ovl: mark xwhiteouts directory with overlay.opaque='x'

[Alexander Larsson <alexl@redhat.com>]

On Mon, 2024-01-22 at 14:52 +0200, Amir Goldstein wrote:
> On Mon, Jan 22, 2024 at 1:52 PM Alexander Larsson <alexl@redhat.com>
> wrote:
> > 
> > On Mon, 2024-01-22 at 13:09 +0200, Amir Goldstein wrote:
> > > On Mon, Jan 22, 2024 at 12:14 PM Alexander Larsson
> > > <alexl@redhat.com>
> > > wrote:
> > > > 
> > > > On Sun, 2024-01-21 at 17:05 +0200, Amir Goldstein wrote:
> > > > > An opaque directory cannot have xwhiteouts, so instead of
> > > > > marking
> > > > > an
> > > > > xwhiteouts directory with a new xattr, overload
> > > > > overlay.opaque
> > > > > xattr
> > > > > for marking both opaque dir ('y') and xwhiteouts dir ('x').
> > > > > 
> > > > > This is more efficient as the overlay.opaque xattr is checked
> > > > > during
> > > > > lookup of directory anyway.
> > > > > 
> > > > > This also prevents unnecessary checking the xattr when
> > > > > reading a
> > > > > directory without xwhiteouts, i.e. most of the time.
> > > > > 
> > > > > Note that the xwhiteouts marker is not checked on the upper
> > > > > layer
> > > > > and
> > > > > on the last layer in lowerstack, where xwhiteouts are not
> > > > > expected.
> > > > > 
> > > > > Fixes: bc8df7a3dc03 ("ovl: Add an alternative type of
> > > > > whiteout")
> > > > > Cc: <stable@vger.kernel.org> # v6.7
> > > > > Signed-off-by: Amir Goldstein <amir73il@gmail.com>
> > > > > ---
> > > > > 
> > > > > Miklos,
> > > > > 
> > > > > Alex has reported a problem with your suggested approach of
> > > > > requiring
> > > > > xwhiteouts xattr on layers root dir [1].
> > > > > 
> > > > > Following counter proposal, amortizes the cost of checking
> > > > > opaque
> > > > > xattr
> > > > > on directories during lookup to also check for xwhiteouts.
> > > > > 
> > > > > This change requires the following change to test
> > > > > overlay/084:
> > > > > 
> > > > > --- a/tests/overlay/084
> > > > > +++ b/tests/overlay/084
> > > > > @@ -115,7 +115,8 @@ do_test_xwhiteout()
> > > > > 
> > > > >         mkdir -p $basedir/lower $basedir/upper $basedir/work
> > > > >         touch $basedir/lower/regular $basedir/lower/hidden
> > > > > $basedir/upper/hidden
> > > > > -       setfattr -n $prefix.overlay.whiteouts -v "y"
> > > > > $basedir/upper
> > > > > +       # overlay.opaque="x" means directory has xwhiteout
> > > > > children
> > > > > +       setfattr -n $prefix.overlay.opaque -v "x"
> > > > > $basedir/upper
> > > > >         setfattr -n $prefix.overlay.whiteout -v "y"
> > > > > $basedir/upper/hidden
> > > > > 
> > > > > 
> > > > > Alex,
> > > > > 
> > > > > Please let us know if this change is acceptable for
> > > > > composefs.
> > > > 
> > > > Yes, this looks very good to me. (Minor comments below)
> > > > I'll do some testing on this.
> > > > 
> > > 
> > > Excellent, I'll be expecting your RVB/Tested-by.
> > > > 
> > 
> > Yes
> > Reviewed-by: Alexander Larsson <alexl@redhat.com>
> > Tested-by: Alexander Larsson <alexl@redhat.com>
> > 
> > for the patch in the ovl-fixes branch.
> 
> Thanks. pushed.
> 
> > 
> > I tested it manually, and with xfstest (with change), and also
> > with this composefs change:
> > 
> > https://github.com/alexlarsson/composefs/tree/new-format-version
> > 
> > I created a lowerdir with a regular whiteout in, and after running
> > that
> > though the changed mkcomposefs I was able to mount the composefs
> > image,
> > and then mount the lowerdirs from the composefs mount, and they
> > correctly handled the whiteout both when mounted normally and with
> > userxattr.
> > 
> 
> I noticed you comment in composefs:
> 
>  * 1 - Mark xwhitouts using the new opaque=x format as needed by
> Linux 6.8
> 
> Note that this "fix" is aimed to be backported to v6.7.y, so there is
> no kernel
> version that is expected to retain support for the old format.

Yes, but the composefs format needs to be bitwise reproducible, and
this change in the image will cause a different digest for the produced
image, so we can't just change what we generate, it has to be opt in to
users and able to reproduce previous versions.

-- 
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=
 Alexander Larsson                                            Red Hat,
Inc 
       alexl@redhat.com            alexander.larsson@gmail.com 
He's an uncontrollable zombie senator gone bad. She's a cosmopolitan 
hypochondriac doctor from Mars. They fight crime!