On Mon, Jan 22, 2024 at 2:50 PM Miklos Szeredi <miklos@xxxxxxxxxx> wrote:
>
> On Sun, 21 Jan 2024 at 16:05, Amir Goldstein <amir73il@xxxxxxxxx> 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@xxxxxxxxxxxxxxx> # v6.7
> > Signed-off-by: Amir Goldstein <amir73il@xxxxxxxxx>
> > ---
> >
> > 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.
>
> Concept looks good overall.
>
> overlayfs.rst needs updating with the new format.
>
Something like this looks ok?
--- a/Documentation/filesystems/overlayfs.rst
+++ b/Documentation/filesystems/overlayfs.rst
@@ -145,7 +145,9 @@ filesystem, an overlay filesystem needs to record
in the upper filesystem
that files have been removed. This is done using whiteouts and opaque
directories (non-directories are always opaque).
-A whiteout is created as a character device with 0/0 device number.
+A whiteout is created as a character device with 0/0 device number or
+as a regular file with the xattr "trusted.overlay.whiteout".
+
When a whiteout is found in the upper level of a merged directory, any
matching name in the lower level is ignored, and the whiteout itself
is also hidden.
@@ -154,6 +156,11 @@ A directory is made opaque by setting the xattr
"trusted.overlay.opaque"
to "y". Where the upper filesystem contains an opaque directory, any
directory in the lower filesystem with the same name is ignored.
+An opaque directory should not conntain any whiteouts, because they do not
+serve any purpose. A merge directory containing regular files with the xattr
+"trusted.overlay.whiteout", should be additionally marked by setting the xattr
+"trusted.overlay.opaque" to "x" on the merge directory itself.
+
readdir
-------
> BTW the nesting
> format should also be documented, but that's a separate patch.
>
Alex already did that:
https://docs.kernel.org/filesystems/overlayfs.html#nesting-overlayfs-mounts
> > @@ -292,7 +292,11 @@ static int ovl_lookup_single(struct dentry *base, struct ovl_lookup_data *d,
> > if (d->last)
> > goto out;
> >
> > - if (ovl_is_opaquedir(OVL_FS(d->sb), &path)) {
> > + /* overlay.opaque=x means xwhiteouts directory */
> > + val = ovl_get_opaquedir_val(ofs, &path);
> > + if (last_element && !is_upper && val == 'x') {
> > + d->xwhiteouts = true;
>
> Maybe I'm missing something, but can't we set the flag on the layer?
>
We do not currently have per-directory-per-layer flags in ovl_lowerstack().
Your patch was optimizing only per-layer check_xwhiteout.
My patch is optimizing only per-directory check_xwhiteout.
The important thing is that for the common case (no xwhiteouts)
xwhiteout will never be checked.
Are you concerned about optimizing check_xwhiteout in a multi lower
overlayfs nested over a composefs overlay mount for the case that
one of the merge dirs in the stack have xwhiteouts and the other do not??
I guess we can use a combination of your patch (v2) and my patch (v3)
and do something like this:
if (last_element && !is_upper && val == 'x') {
d->xwhiteouts = d->layer->xwhiteouts = true;
...
to mark the dentry as OVL_E_XWHITEOUTS
AND mark the layer as having xwhiteouts
and then in readdir we check that
BOTH dentry has xwhiteouts (in some layer)
AND the layer has xwhiteouts (in some directory).
Is that what you meant?
Would you like me to make this change?
Thanks,
Amir.
