linux-unionfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Amir Goldstein <amir73il@gmail.com>
To: Kevin Locke <kevin@kevinlocke.name>
Cc: Miklos Szeredi <miklos@szeredi.hu>,
	overlayfs <linux-unionfs@vger.kernel.org>
Subject: Re: [PATCH] ovl: add xino to "changes to underlying fs" docs
Date: Mon, 8 Mar 2021 19:41:53 +0200	[thread overview]
Message-ID: <CAOQ4uxhGSbEPPwZswXHq+k1YF=+ntDfukxnfGsJ3+RaGjgNDnQ@mail.gmail.com> (raw)
In-Reply-To: <b36a429d7c563730c28d763d4d57a6fc30508a4f.1615216996.git.kevin@kevinlocke.name>

On Mon, Mar 8, 2021 at 5:23 PM Kevin Locke <kevin@kevinlocke.name> wrote:
>
> Add "xino" to the list of features which cause undefined behavior for
> offline changes to the lower tree in the "Changes to underlying
> filesystems" section of the documentation to make users aware of
> potential issues if the lower tree is modified and xino was enabled.
>
> This omission was noticed by Amir Goldstein, who mentioned that xino is
> one of the "forbidden" features for making offline changes to the lower
> tree and that it wasn't currently documented.
>

Hi Kevin,

Thanks for following up on this.
I see my original comment did not make it to the list, because the message
was formatted incorrectly.

My full comment was:

"...
My feeling is that we need to adjust the fix and not treat the case
of xino=auto as "user opted-in for xino" thus disabling origin inode
decode for lower without uuid.

Also the documentation does not mention the xino feature as one
of the "forbidden" features for this use case.
..."

So your Documentation fix may represent the current code, but
I think we should fix the code instead.

When looking again, I actually don't see a reason to include "xino"
in this check at all (not xino=on nor xino=auto):

 if (!ofs->config.index && !ofs->config.metacopy && !ofs->config.xino &&
     uuid_is_null(uuid))
         return false;

The reason that "index" and "metacopy" are in this check is because
they *need* to follow the lower inode of a non-dir upper in order to
operate correctly. The same is not true for "xino".

Moreover, "xino" will happily be enabled also when lower fs does not
support file handles at all. It will operate sub-optimally, but it will live up
to the promise to provide a unified inode namespace and uniform st_dev.


> Signed-off-by: Kevin Locke <kevin@kevinlocke.name>
> ---
>  Documentation/filesystems/overlayfs.rst | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/filesystems/overlayfs.rst b/Documentation/filesystems/overlayfs.rst
> index 78240e29b0bb..52d47bed9ef8 100644
> --- a/Documentation/filesystems/overlayfs.rst
> +++ b/Documentation/filesystems/overlayfs.rst
> @@ -476,9 +476,9 @@ a crash or deadlock.
>
>  Offline changes, when the overlay is not mounted, are allowed to the
>  upper tree.  Offline changes to the lower tree are only allowed if the
> -"metadata only copy up", "inode index", and "redirect_dir" features
> -have not been used.  If the lower tree is modified and any of these
> -features has been used, the behavior of the overlay is undefined,
> +"metadata only copy up", "inode index", "redirect_dir", and "xino"
> +features have not been used.  If the lower tree is modified and any of
> +these features has been used, the behavior of the overlay is undefined,
>  though it will not result in a crash or deadlock.

Note that "redirect_dir" is not one of the "forbidden" features.

Thanks,
Amir.

  reply	other threads:[~2021-03-08 17:42 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <CAOQ4uxj4zNHU49Q6JeUrw4dvgRBumzhtvGXpuG4WDEi5G7uyxw@mail.gmail.com>
2021-03-08 15:23 ` [PATCH] ovl: add xino to "changes to underlying fs" docs Kevin Locke
2021-03-08 17:41   ` Amir Goldstein [this message]
2021-03-08 23:49     ` Kevin Locke
2021-03-09  7:24       ` Amir Goldstein
2021-03-09 14:29         ` Amir Goldstein
2021-03-09 17:43         ` Kevin Locke
2021-03-09 18:50         ` Vivek Goyal
2021-03-09 19:24           ` Amir Goldstein

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to='CAOQ4uxhGSbEPPwZswXHq+k1YF=+ntDfukxnfGsJ3+RaGjgNDnQ@mail.gmail.com' \
    --to=amir73il@gmail.com \
    --cc=kevin@kevinlocke.name \
    --cc=linux-unionfs@vger.kernel.org \
    --cc=miklos@szeredi.hu \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).