Message-ID: <CAOQ4uxiRGcPNsad==MtLFGrrwg_Sv-6g0tNwSVtvoSH+2VR5Lw@mail.gmail.com>
Date: Sat, 17 Aug 2024 10:01:34 +0200
From: Amir Goldstein <amir73il@...il.com>
To: Yuriy Belikov <yuriybelikov1@...il.com>
Cc: Miklos Szeredi <miklos@...redi.hu>, Jonathan Corbet <corbet@....net>, linux-unionfs@...r.kernel.org, 
	linux-doc@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] Update redirect_dir and metacopy sections in overlayfs documentation

On Thu, Aug 15, 2024 at 10:30 PM Yuriy Belikov <yuriybelikov1@...il.com> wrote:
>
> This patch:
> - Provides info about trusted.overlay.metacopy extended attribute.
> - Extends the description of trusted.overlay.redirect
>   with information about possible values of this xattr
>
> Signed-off-by: Yuriy Belikov <yuriybelikov1@...il.com>

Hi Yuriy,

This version has some inaccuracies and some irrelevant
information IMO. I will try to point them out.
Please be aware that this is an overview/design document
and it is not the intention to commit to the implementation details
of overlayfs.

> ---
>  Documentation/filesystems/overlayfs.rst | 32 +++++++++++++++++++------
>  1 file changed, 25 insertions(+), 7 deletions(-)
>
> diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst
> index 165514401441..f4b68b8cd67d 100644
> --- a/Documentation/filesystems/overlayfs.rst
> +++ b/Documentation/filesystems/overlayfs.rst
> @@ -207,11 +207,23 @@ handle it in two different ways:
>     applications are usually prepared to handle this error (mv(1) for example
>     recursively copies the directory tree).  This is the default behavior.
>
> -2. If the "redirect_dir" feature is enabled, then the directory will be
> -   copied up (but not the contents).  Then the "trusted.overlay.redirect"
> -   extended attribute is set to the path of the original location from the
> -   root of the overlay.  Finally the directory is moved to the new
> -   location.
> +2. If the "redirect_dir" feature is enabled, then the contents of the
> +   directory will not be copied up after any name-modifying operations
> +   (e.g. rename(2), or mv(1)).

I cannot put my finger on it, I just don't like this rephrasing and it serves no
good purpose IMO. mv(1) is not an operation, it is a tool, so the phrasing
is just inaccurate.

> Instead of performing a copy-up operation,
> +   an empty entry will be created in the upper layer with the same name
> +   as the affected entry in the overlayfs directory.

Again I do not like this phrasing. It suggests that there is a copy up
operation that copies entire directory content and that is not true.
chown of directory will copy up directory without content and that
has nothing to do with redirect_dir.

> The 'trusted.overlay.redirect'
> +   xattr will then be set to mark the upper-layer directory, indicating that
> +   its contents weren't copied up due to the 'redirect_dir' feature.
> +   This extended attribute holds the previous name of a directory as a value.
> +   For directories that were simply renamed the attribute is just the old name
> +   of the directory without preceding path.

"simply renamed" is not a clear technical description.
Not changing a parent is what you want to say, but TBH, I just don't
think that we want to commit to this implementation detail and I am
really not sure why putting it in the document is helpful.

> For directories whose locations
> +   in the overlayfs directory were changed, the corresponding xattrs are set
> +   to the paths to the original locations from the root of the overlay.
> +   The value of the xattr in the second case starts with a UNIX path delimiter
> +   (e.g. "/$PREVIOUS_PATH"). Finally the directory is moved
> +   to the new location. The output of du "$UPPER_LAYTER_DIR/$RENAMED_DIR"

I dislike the use of these invented variable names that were not
defined earlier in the doc, please don't use that notation.

> +   should be zero.

Not something that we want or can commit to.
Some filesystems count xattr blocks in du (depending on size of
all the xattr) so it is not possible to commit to this statement and
I do not think that it adds much to the document.

> Renamed directory subentries will be copied-up only
> +   after operations that directly affect their contents.

Again, not related to redirect_dir.
This is a description of how copy up generally works in overlayfs.
If you think that this information is missing from the document,
please try to add it to the Directories section.

>
>  There are several ways to tune the "redirect_dir" feature.
>
> @@ -367,8 +379,14 @@ Metadata only copy up
>
>  When the "metacopy" feature is enabled, overlayfs will only copy
>  up metadata (as opposed to whole file), when a metadata specific operation
> -like chown/chmod is performed. Full file will be copied up later when
> -file is opened for WRITE operation.
> +like chown/chmod is performed. When file metadata are modified the
> +corresponding empty file (with the same name as the modified one)
> +appears in the upper layer, however such a file contains
> +no allocated data (a sparse file);

I am sorry, I do not like this phrasing, but also cannot offer something better
because I am not sure what missing information you are trying to add.

The only thing I would change in the doc is replace
"Full file will be copied up later..."
with
"File data will be copied up later..."

> doing du "$UPPER_LAYER/$FILENAME"
> +should yield zero.

No invented env variables please. we cannot commit to zero du.

> Such an upper-layer file is marked with
> +"trusted.overlayfs.metacopy" xattr which indicates that this file contains
> +no data and copy-up should be performed before the corresponding file
> +in the overlayfs directory is opened for write.
>
>  In other words, this is delayed data copy up operation and data is copied
>  up when there is a need to actually modify data.

Maybe just add here at the end:

An upper file in this state is marked with "trusted.overlayfs.metacopy" xattr
which indicates that the upper file contains no data.
After the lower file's data is copied up, the "trusted.overlayfs.metacopy" xattr
is removed from the upper file.

Thanks,
Amir.

Powered by blists - more mailing lists