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

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.

> 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"?

My understanding of the current behavior:

1. If a redirection target dir is removed from lower, contents which do
   not exist in any upper are removed from the redirected dir in the
   overlay.  Contents which exist in an upper are unchanged.
2. If a redirection target dir is renamed in lower, it has the same
   effect as removing the directory with the old name and creating one
   with the new name and contents.
3. Permission changes to a redirection target dir have no effect in the
   overlay.
4. If a previously removed redirection target dir is created, its
   contents are added to the redirected dir in the overlay, unless the
   redirected dir had been renamed after removal, in which case it is
   ignored (because the redirected dir becomes opaque when renamed).
5. If a file with the name of a previously removed redirection target
   dir is created, it is ignored.

The only behavior which seems a bit surprising to me is #4:  If
directory B in upper is redirected to A in lower and A is subsequently
removed, then (possibly years later) a directory named A is created, its
contents would appear in B in the overlay.  However, if B had been
renamed to C after A was removed, it becomes opaque, causing A and its
contents not to appear in the overlay.  Either of these may surprise
users.

Does that match your understanding of the current behavior?  Worth
documenting or better to just remove redirect_dir from the list of
features where offline modification causes undefined behavior?

Thanks,
Kevin

  reply	other threads:[~2021-03-08 23:50 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 [this message]
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=YEa4Jd0VE6w4T7/v@kevinlocke.name \
    --to=kevin@kevinlocke.name \
    --cc=amir73il@gmail.com \
    --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).