linux-unionfs.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Amir Goldstein <amir73il@gmail.com>
To: Kevin Locke <kevin@kevinlocke.name>,
	Amir Goldstein <amir73il@gmail.com>,
	Miklos Szeredi <miklos@szeredi.hu>,
	overlayfs <linux-unionfs@vger.kernel.org>
Cc: Vivek Goyal <vgoyal@redhat.com>
Subject: Re: [PATCH] ovl: add xino to "changes to underlying fs" docs
Date: Tue, 9 Mar 2021 16:29:42 +0200	[thread overview]
Message-ID: <CAOQ4uxjvGLtiyj5upsYOjnqiGnij+ar8k6v=zv0ceS9k44UMRQ@mail.gmail.com> (raw)
In-Reply-To: <CAOQ4uxjBb_whXA5eNqkwDNj2VSS-F+0uACF7tpqFTrM8fYETQg@mail.gmail.com>

On Tue, Mar 9, 2021 at 9:24 AM Amir Goldstein <amir73il@gmail.com> wrote:
>
> On Tue, Mar 9, 2021 at 1:50 AM Kevin Locke <kevin@kevinlocke.name> wrote:
> >
> > Hi Amir,
> >
> > On Mon, 2021-03-08 at 19:41 +0200, Amir Goldstein wrote:
> > > 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.
> > >
> > > [...]
> > > 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.
> >
> > Good observation!  I think you are right.  After a bit of testing, I did
> > not notice any issues after making offline changes to lower with xino
> > enabled.
> >
>
> He, that's not what I meant.
> I wouldn't expect that you *observe* any issues, because the issues
> with following the wrong object are quite rare and you need to make
> changes to lower squashfs to notice them, see:
> https://lore.kernel.org/lkml/20191106234301.283006-1-colin.king@canonical.com/
>
> But as a matter of fact, I was wrong and I misled you. Sorry.
>
> I read the code backwards.
>
> It's not true that we can allow lower modification with "xino=on/auto" -
> quite the opposite - we may need to disallow lower modifications also
> with "xino=off".
>
> Let me explain.
> The following table documents expected behavior with different
> features and layer setups:
> https://www.kernel.org/doc/html/latest/filesystems/overlayfs.html#inode-properties
>
> As you can see, the matrix is quite complex.
> The problem lies with the documented behavior of "Persistent st_ino of !dir"
> for the case of "Layers not on same fs, xino=off".
>
> It claims that st_ino will be persistent, but in fact it is only true
> if lower fs
> supports file handles AND has a unique [*] UUID amongst the lower layers.
> The claim that st_ino is persistent for !dir in case of "ino overflow" is also
> incorrect.
>
> [*] The special case of NULL UUID (e.g. squashfs) was recently changed
>      and depends on whether the opt-in features are enabled...
>
> In any case, the documented behavior for Persistent st_ino (!dir) is
> incorrect for the case of (e.g.) lower squashfs with -no-exports.
> IWO, in this setup, st_ino of a lower file will change following copy up
> and mount cycle.
>
> I do not want to add all this story to documentation - the matrix is
> complex enough to follow as it is.
>

This came out too complicated. Let me try again -

The documentation in the section:
https://www.kernel.org/doc/html/latest/filesystems/overlayfs.html#overlay-objects
speaks about overlayfs objects having non-unique and non-persistent st_ino.
It then goes on to say that "xino" can be used to make overlayfs "compliant",
but in fact never speaks of persistent st_ino until the comparison table,
where the documented values are incorrect.

So I decided to try and promote "xino" from a feature that "makes inode
numbers unique" to a feature that "makes inode numbers unique and
if possible, also persistent" by adding the following text to the section:

"...
The "xino" feature can be enabled with the "-o xino=on" overlay mount option.
If all underlying filesystems support NFS file handles, the value of st_ino
for overlay filesystem objects is not only unique, but also persistent over
the lifetime of the filesystem.  The "-o xino=auto" overlay mount option
enables the "xino" feature only if the persistent st_ino requirement is met.
..."

And with this I pured new meaning into xino=auto, which lost its original
meaning after commit:
926e94d79baf ("ovl: enable xino automatically in more cases")

The code change is to fall back from xino=auto to xino=off in cases
where the lower layer has no file handle support or bad uuid.

I'll post the patch for review soon.

Thanks,
Amir.

  reply	other threads:[~2021-03-09 14:30 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
2021-03-08 23:49     ` Kevin Locke
2021-03-09  7:24       ` Amir Goldstein
2021-03-09 14:29         ` Amir Goldstein [this message]
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='CAOQ4uxjvGLtiyj5upsYOjnqiGnij+ar8k6v=zv0ceS9k44UMRQ@mail.gmail.com' \
    --to=amir73il@gmail.com \
    --cc=kevin@kevinlocke.name \
    --cc=linux-unionfs@vger.kernel.org \
    --cc=miklos@szeredi.hu \
    --cc=vgoyal@redhat.com \
    /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).