Re: [PATCH v2] ovl: require xwhiteout feature flag on layer roots

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Jan 19, 2024 at 6:35 PM Alexander Larsson <alexl@xxxxxxxxxx> wrote:
>
> On Fri, 2024-01-19 at 13:08 +0200, Amir Goldstein wrote:
> > On Fri, Jan 19, 2024 at 12:14 PM Miklos Szeredi <mszeredi@xxxxxxxxxx>
> > wrote:
> >
> >
> > Do you want me to fix/test and send this to Linus?
> >
> > Alex, can we add your RVB to v2?
>
> I ran into an issue converting composefs to use this.
>
> Suppose we have a chroot of files containing some upper dirs and we
> want to make a composefs of this. For example, say
> /foo/lower/dir/whiteout is a traditional whiteout.
>
> Previously, what happened is that I marked the whiteout file with
> trusted.overlay.overlay.whiteout, and the /foo/lower/dir with
> trusted.overlay.overlay.whiteouts.
>
> Them when I mounted then entire chroot with overlayfs these xattrs
> would get unescaped and I would get a $mnt/foo/lower/dir/whiteout with
> a trusted.overlay.whiteout xattr, and a $mnt/foo/lower/dir with a
> trusted.overlay.whiteout. When I then mounted another overlayfs with a
> lowerdir of $mnt/foo/lower it would treat the whiteout as a xwhiteout.
>
> However, now I need the lowerdir toplevel dir to also have a
> trusted.overlay.whiteouts xattr. But when I'm converting the entire
> chroot I do not know which of the directories is going to be used as
> the toplevel lower dir, so I don't know where to put this marker.
>
> The only solution I see is to put it on *all* parent directories. Is
> there a better approach here?
>

Alex,

As you can see, I posted v3 with an alternative approach that would not
require marking all possible lower layer roots.

However, I cannot help wondering if it wouldn't be better practice, when
composing layers, to always be explicit, per-directory about whether the
composed directory is a "base" or a "diff" layer.

Isn't this information always known at composing time?

In legacy overlayfs, there is an explicit mark for "this is a base dir" -
namely, the opaque xattr, but there is no such explicit mark on
directories without an entry with the same name in layers below them.

The lack of explicit mark "merge" vs. "opaque" in all directories in all
the layers had led to problems in the past, for example, this is the
reason that this fix was needed:

  b79e05aaa166 ovl: no direct iteration for dir with origin xattr

In conclusion, since composefs is the first tool, that I know of, to
compose "non-legacy" overlayfs layers (i.e. with overlay xattrs),
I think the correct design decision would mark every directory in
every layer explicitly as at exactly one of "merge"/"opaque".

Note that non-dir are always marked explicitly as "metacopy",
so there is no ambiguity with non-dirs and we also error out
if a non-dir stack does not end with an "opaque" entry.

Additionally, when composing layers, since all the children of
a directory should be explicitly marked as "merge" vs. "opaque"
then the parent's "impure" (meaning contains "merge" children)
can also be set at composing time.

Failing to set "impure" correctly when composing layers could
result in wrong readdir d_ino results.

My proposition in v3 for an explicit mark was to
"reinterpret the opaque xattr from boolean to enum".
My proposal included only the states 'y' (opaque) and 'x' (contains
xwhiteouts), but for composefs, I would extend this to also mark
a merge dir explicitly with opaque='n' and explicitly mark all the
directories in a "base layer" with opaque='y'.

Implementation-wise, composefs could start by marking each directory
with either 'y'/'n' state based on the lowerstack, and if xwhiteout entries
are added, 'n' state could be changed to 'x' state.

What do you think?

Does it make sense from composefs POV?
Am I correct to assume that at composing time, every directory
state is known (base 'y' vs. diff 'n')?

Thanks,
Amir.





[Index of Archives]     [Linux Filesystems Devel]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux