On Tue, Mar 9, 2021 at 9:24 AM Amir Goldstein <amir73il@xxxxxxxxx> wrote: > > On Tue, Mar 9, 2021 at 1:50 AM Kevin Locke <kevin@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxx/ > > 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.
