archive mirror
 help / color / mirror / Atom feed
From: Amir Goldstein <>
To: Kevin Locke <>,
	Amir Goldstein <>,
	Miklos Szeredi <>,
	overlayfs <>
Cc: Vivek Goyal <>
Subject: Re: [PATCH] ovl: add xino to "changes to underlying fs" docs
Date: Tue, 9 Mar 2021 09:24:22 +0200	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <YEa4Jd0VE6w4T7/>

On Tue, Mar 9, 2021 at 1:50 AM Kevin Locke <> 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 <> 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:

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:

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

[*] 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.

Seeing that distros are switching to enable xino by default, I was
contemplating to change the behavior of the code as follows:

- If user opts-out of xino by mount option (xino=off is *shown*
  in /proc/mounts) do not follow origin by file handle
- Let index and metacopy require and auto-enable xino, so e.g.:
  mount options index=on,xino=off will be a conflict
- If lower does not support file handles or has NULL UUID and
  xino is enabled by default, then auto-disable xino and do not
  follow origin (xino=off will be shown in /proc/mounts)
- If xino is disabled by default, we DO follow origin as we always
  did (xino=off is NOT shown in /proc/mounts)
- Change the documented value for Persistent st_ino (!dir) in case
  of "xino=off" and in case of "ino overflow" to N

1. This makes for simpler and more coherent documentation IMO.
2. It doesn't change behavior for legacy layers with all default
    kernel configs and default mount options.
3. It actively averts the reported issues caused by re-formatting
    lower squashfs with distro kernel configs and default mount options.

1. After kernel upgrade, existing setups with lower squashfs that did
    not opt-in for xino by mount option will lose xino
2. Existing setups that opt-out of xino by mount option (because of old
    32bit applications?) will loose persistent st_ino behavior

IMO, the Pros out weight the Cons.

I've suggested adding a way to opt-out of following origin several times,
but was not able to convince Miklos so far.
Let's see if this time is any different...

> > Note that "redirect_dir" is not one of the "forbidden" features.
> To be clear, are you saying that offline modifications to directories in
> lower layers which are the redirection target of an upper layer does not
> cause undefined behavior?  Would it make sense for me to work up a patch
> which documents the behavior, or is it better to leave as "defined but
> undocumented"?

I just mislead you. Sorry for that.
We should leave "redirect_dir" in the documented list and add "xino"
just like the patch you posted.
But I guess if I am going to post a patch to change the xino behavior,
it would be better to include your change in my patch for context.


  reply	other threads:[~2021-03-09  7:25 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <>
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 [this message]
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:

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

  git send-email \ \ \ \ \ \ \

* 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).