On Sat, 2024-01-20 at 12:32 +0200, Amir Goldstein wrote:
> On Fri, Jan 19, 2024 at 10:30 PM Miklos Szeredi <miklos@xxxxxxxxxx>
> wrote:
> >
> > On Fri, 19 Jan 2024 at 20:06, Amir Goldstein <amir73il@xxxxxxxxx>
> > wrote:
> >
> > > How about checking xwhiteouts xattrs along with impure and
> > > origin xattrs in ovl_get_inode()?
> > >
>
> That was not a very good suggestion on my part.
> ovl_get_inode() only checks impure xattr on upper dir and origin
> xattr
> on non-merge non-multi lower dir.
>
> If we change the location of xwhiteout xattr check, it should be in
> ovl_lookup_single() next to checking opaque xattr, which makes
> me think - hey, why don't we overload opaque xattr, just like we
> did with metacopy xattr?
>
> An overlay.opaque xattr with empty string means "may have xwhiteouts"
> and that is backward compatible, because ovl_is_opaquedir() checks
> for xattr of length 1.
>
> The only extra getxattr() needed would be for the d->last case...
>
> > > Then there will be no overhead in readdir and no need for
> > > marking the layer root?
> > >
> > > Miklos, would that be acceptable?
> >
> > It's certainly a good idea, but doesn't really address my worry.
> > The
> > minor performance impact is not what bothers me most. It's the
> > fact
> > that in the common case the result of these calls are discarded.
> > That's just plain ugly, IMO.
>
> ...so the question boils down to, whether you find it too ugly
> to always getxattr(opaque) on lookup of the last lower layer and
> whether you find the overloading of opaque xattr too hacky?
>
> As a precedent, we *always* check metacopy xattr in last lower layer
> to check for error conditions, even if user did not opt-in to
> metacopy
> at all, while we could just as easily have ignored it.
>
> >
> > My preferred alternative would be a mount option. Amir, Alex,
> > would
> > you both be okay with that?
> >
>
> I think I had suggested that escaped private xattrs would also
> require
> an opt-in mount option, but Alex explained that the users mounting
> the
> overlay are not always aware of the fact that the layers were
> composed
> this way, but I admit that I do not remember all the exact details.
>
> Alex, do I remember correctly that the overlay instance where
> xwhiteouts
> needs to be checked does NOT necessarily have a lowerdata layers?
> The composefs instance with lowerdata layers is the one exposing the
> (escaped) xwhiteout entries as xwhiteouts. Is that correct?
>
> Is there even a use case for xwhiteouts NOT inside another lower
> overlayfs?
No. Strictly speaking regular whiteouts are always preferable to
xwhiteouts (as they work for both user and system ovl mounts and are
supported by older kernels and existing software, etc). The only place
where xwhiteouts are useful is when we need to escape them to put
inside an overlayfs mount.
The best way to think about the composefs usecase is:
Suppose you had a pre-existing system image that someone installed a
multi layer container image in. This means that somewhere inside this
image is a set of lowerdirs, and one of them may have a traditional
whiteout. Now we want to create an overlayfs mount that when used,
works just like the above image would work, including when you mount
the sub-overlayfs mounts from the container image lowerdirs.
Fallout of this:
The composefs overlay lowerdir will need to contain escaped xwhiteouts,
so that the mount will have unescaped xwhiteouts. These escaped
whiteouts can be anywhere, even in a "single layer" overlayfs. But the
unescaped xwhiteouts will never be in a lowermost lowerdir.
In the composefs case we will be using lowerdata for the outer
overlayfs, but the actual unescaped xwhiteouts in the container image
lowerdirs don't need to have a lowerdata involved at all.
The container mount will be done by pre-existing software (say docker)
that isn't aware that we converted the regular whiteouts to xwhiteouts,
so having to use a mount option is not ideal (would require docker
changes).
> If we limit the check for xwhiteouts only to nested overlayfs, then
> maybe
> Miklos will care less about an extra getxattr on lookup?
>
> Attached patch implements both xwhiteout and opaque checks during
> lookup - we can later choose only one of them to keep.
Seems like you attached the wrong patch, but I will comment on the
other patch you sent to the list.
> Note that is changes the optimization to per-dentry, not per-layer,
> so in the common case (no layers have xwhiteouts) xwhiteouts will not
> be checked, but if xwhiteouts exist in any lower layer in the stack,
> then
> xwhiteouts will be checked in all the layers of the merged dir (*).
>
> (*) still need to optimize away lookup of xwhiteouts in upperdir.
>
> Let me know what you think.
>
> Thanks,
> Amir.
--
=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
=-=-=
Alexander Larsson Red Hat,
Inc
alexl@xxxxxxxxxx alexander.larsson@xxxxxxxxx
He's a benighted bohemian waffle chef on the wrong side of the law.
She's
an enchanted junkie safe cracker from the wrong side of the tracks.
They
fight crime!
