Re: [QUESTION] Why overlayfs cannot mounted as nfs_export and metacopy?

[Vivek Goyal <vgoyal@redhat.com>]

On Mon, Aug 16, 2021 at 09:44:11PM +0300, Amir Goldstein wrote:
> On Mon, Aug 16, 2021 at 9:27 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> >
> > On Wed, Aug 11, 2021 at 09:51:09AM +0300, Amir Goldstein wrote:
> > > On Tue, Aug 10, 2021 at 10:49 PM Vivek Goyal <vgoyal@redhat.com> wrote:
> > > >
> > > > On Tue, Aug 10, 2021 at 07:17:23AM +0300, Amir Goldstein wrote:
> > > > > On Tue, Aug 10, 2021 at 12:35 AM Vivek Goyal <vgoyal@redhat.com> wrote:
> > > > > >
> > > > > > On Sat, Aug 07, 2021 at 07:37:00PM +0300, Amir Goldstein wrote:
> > > > > > > On Sat, Aug 7, 2021 at 2:05 PM Amir Goldstein <amir73il@gmail.com> wrote:
> > > > > > > >
> > > > > > > > On Sat, Aug 7, 2021 at 1:17 PM Zhihao Cheng <chengzhihao1@huawei.com> wrote:
> > > > > > > > >
> > > > > > > > > Hi, all.
> > > > > > > > >
> > > > > > > > > As title said. I wonder to know the reason for overlayfs mount failure
> > > > > > > > > with '-o nfs_export=on,metacopy=on'.
> > > > > > > > >
> > > > > > > > > I modified kernel to enable these two options 'on',  it looks like that
> > > > > > > > > overlayfs can still work fine under nfs_v4.
> > > > > > > > >
> > > > > > > > > Besides, I can get no more information about the reason from source
> > > > > > > > > code, maybe I missed something.
> > > > > > > > >
> > > > > > > >
> > > > > > > > It's because ovl_obtain_alias() (decoding a disconnected non-dir file handle)
> > > > > > > > does not know how to construct a metacopy overlayfs inode.
> > > > > > > >
> > > > > > > > Maybe Vivek will be able to point you to the discussion that lead to making
> > > > > > > > the features mutually exclusive.
> > > > > > > >
> > > > > > > > I don't remember any other reason.
> > > > > > > >
> > > > > > >
> > > > > > > I remembered some more details...
> > > > > > >
> > > > > > > I think the main complication discussed w.r.t decoding a metacopy
> > > > > > > inode was for the case where ovl_inode_lowerdata() differs from
> > > > > > > ovl_inode_lower().
> > > > > > >
> > > > > > > If we had a weaker variant of metacopy (e.g. metacopy=upper) that
> > > > > > > only allows creating and following metacopy inodes in the upper layer,
> > > > > > > it would have been simpler to implement ovl_obtain_alias().
> > > > > > >
> > > > > > > Specifically, when ofs->numlayer == 2 (single lower layer), there can
> > > > > > > be no valid metacopy inodes in the lower layer, so that configuration
> > > > > > > should also be rather easy to support.
> > > > > >
> > > > > > Hi Amir,
> > > > > >
> > > > > > /me does not understand well the notion of disconnected dentries and
> > > > > >  how nfs export stuff works. So please bear with my stupid questions.
> > > > >
> > > > > No stupid questions ;-)
> > > > >
> > > > > Without getting into the hairy details of nfs export there are a few basic
> > > > > things to consider:
> > > > > - A file handle does not encode the path, only an inode identifier
> > > > > - A non-directory inode may have multiple paths (hardlinks)
> > > > > - Most filesystems do not store path information in inode on-disk for
> > > > >   non-directory inode (the ".." entry stores the path for a directory)
> > > > > - When filesystem is asked to decode a file handle and does not find the
> > > > >   inode in question in inode cache nor a dentry in dcache, the only resort
> > > > >   is to instantiate a "disconnected" dentry with unknown path
> > > > > - Later "normal" lookup() by path that resolves to the same inode, does not
> > > > >   make that "disconnected" dentry connected. Istead, lookup() instantiates
> > > > >   another connected dentry "alias" to the same inode
> > > > >
> > > > > All this has some implications when enabling nfs_export for overlayfs:
> > > > > 1. ovl_obtain_dentry() needs to be able to cope with a disconnected
> > > > >     'real' dentry
> > > > > 2. Since ovl_obtain_dentry() cannot assume to know the path of the
> > > > >     'real' dentry, it needs to know how to instantiate a disconnected
> > > > >     overlayfs dentry
> > > > > 3. Other overlayfs code needs to be able to cope with a disconnected
> > > > >     overlayfs dentry (for example, copy up only to index)
> > > >
> > > > Hi Amir,
> > > >
> > > > Thanks for giving a summary. It helps a lot.
> > > >
> > > > >
> > > > > >
> > > > > > I am wondering why a lower inode can't be metacopy inode. For the
> > > > > > normal lookup case, we can lookup in all lower layers and figure out
> > > > > > which is actual data inode and which inodes are metacopy inodes.
> > > > > >
> > > > > > For the case of disconnected dentry, we probably can't do lookup. So
> > > > > > are calling underlying filesystem to decode. (Using origin?). If yes,
> > > > > > will intermediate lower not have origin xattr which we can use
> > > > > > to follow the complete lower chain and reconstruct all real lower
> > > > > > dentries and use lower data dentry and latest lower meatacopy dentry
> > > > > > (in the same way we do as for lookup).
> > > > >
> > > > > We can do that. I did not say we cannot.
> > > > > I just said it would be simpler if we can avoid this complication
> > > > > and I listed the guidelines for the "simple" implementation.
> > > > >
> > > > > But beyond the complexity, what is the benefit?
> > > > > I was under the impression that container manager do not know how
> > > > > to build images with metacopy, so what are the chances of actually
> > > > > seeing metacopy in middle layers in the wild?
> > > >
> > > > Sure, we don't put metacopy inodes into portable images. But I thougt
> > > > this could be part of a lower directory on same system. For example,
> > > > docker devicemapper driver used to take an image and explode that
> > > > on a thin volume. Then it will take a snaphost, modify some files
> > > > and prefix that intermediate state with "-init". And then containers
> > > > will use this "-init" as base for container rootfs and take snapshot
> > > > of this.
> > > >
> > > > I am not sure if container managers are doing this for overlayfs
> > > > or not on same system. But I will not be surprised if somebody
> > > > decides to do that. That's is change some metadata in image
> > > > (which triggers metacopy) and then use upper layer as lower layer
> > > > for container rootfs.
> > > >
> > > > [ CC Dan Walsh and Nalin Dahyabhai ]
> > > >
> > > > Dan, Nalin, I think now metacopy feature is being used in podman and
> > > > container/storage. Do we ever create lower layers in such a way that
> > > > metacopy inodes can be present in lower layers?
> > > >
> > > > >
> > > > > IOW, if we implemented metacopy=upper (only allow metacopy in
> > > > > upper layer), would it be sufficient for the use cases that need to enable
> > > > > nfs_export?
> > > >
> > > > IMHO, it will be good if we don't add one more variation to metacopy
> > > > option. Even if container managers will find it hard to ensure that
> > > > there are no metacopy inodes in any of the lower layers. And will
> > > > find it difficult to use this option.
> > > >
> > > > IIRC, while adding metacopy option, you had mentioned that it is possible
> > > > that intermediate layers have metacopy inodes. So I changed implementation
> > > > to take care of it. :-) And now you are the one suggesting not allowing
> > > > metacopy inodes in lower layers with nfs_export. :-)
> > > >
> > >
> > > Maybe. It wouldn't be the first time ;-)
> > > IIRC the idea to lookup metacopy by path+redirect was Miklos' not mine.
> > > I was thinking in terms of following by origin back then.
> >
> > Yes. I think we had lot of discussion on this. You wanted to use origin.
> > And I preferred not to use it primarily because I wanted to make use
> > of metacopy even without index enabled also because did not want
> > to depend on lower layers supporting nfs_export. So lookup approach
> > seemed simpler and using origin did not seem necessary.
> >
> > >
> > > The eventual code in ovl_lookup() follows upper by origin and all layers
> > > by path+redirect and only checks for agreement of origin and redirect from
> > > upper layer.
> > >
> > > Implementing "follow metacopy in middle layers by origin" in ovl_obtain_alias()
> > > is possible and even not too complicated, but it would be inconsistent
> > > with ovl_lookup() and if somebody mangaled with lower layers (i.e. rename
> > > lower file), decode file handle can result in a different inode than the encoded
> > > one.
> > >
> > > In that case we have several options:
> > > 1. Whatever - documentation already claims that modifying lower layer
> > >     after using index/metacopy results in undefined behavior
> > > 2. Fortify ovl_lookup() - also follow origin from middle layers and check
> > >     for agreement with follow by path+redirect
> >
> > The idea of fortifying that lower we found matches origin sounds
> > reasonable to me. I thought that's what verify_lower was doing. But may
> > be in limited cases.
> >
> > We probably will have to again make it opt-in so that we don't break
> > some existing use cases (not opting in for origin created so many
> > issues on squashfs that I have lost track of all the issues and fixes :-))
> >
> > >
> > > > If given a choice, I would prefer that we support metacopy inodes
> > > > in lower layers as well with nfs_export and not create a new option
> > > > metacopy=upper.
> > > >
> > >
> > > Certainly. I agree with that POV. We just need to understand the
> > > consequences.
> > >
> > > I personally have no problem with the "Whatever" option above.
> > > And the "Fortify" option isn't that complex either.
> > >
> > > I just thought that if Zhihao Cheng wanted to start with minimal
> > > testable implementation, that limiting nfs_export+metacopy to a single
> > > lower layer, may be an easier start.
> > >
> > > Overlayfs nfs_export is quite likely being used in OpenWrt and IIUC,
> > > OpenWrt uses a single (squashfs) lower layer.
> > >
> > > Therefore, enabling metacopy for the single lower layer case may be
> > > valuable to some actual users and not only as a stepping stone before a
> > > complete solution.
> >
> > I agree that metacopy=upper as it is will be useful to some people
> > in some configurations. Do you really want to support two options.
> > More options lead to more confusion for users and more configurations can
> > be harder to support. So if it was me, I would rather target just making
> > metacopy=on/off work with nfs_export. Given I am not doing the actual
> > work, I can have only so much say.. :-)
> >
> 
> Maybe I was not clear, so let me try again.
> The most simple thing to implement would be to allow
> nfs_export=on along with metacopy=on when there is one single lower layer.

Ok, got it. So if there are more than one lower layer, nfs_export +
metacopy mount will fail.

It is still one more configuration because without nfs_export we
do not have any such restriction. But I am not opposed to the idea
if you find this intermediate configuration valuable given the
complexity of work.

Thanks
Vivek

> 
> Assuming that configuration is useful for whoever intends to implement it.
> From there we can gather some experience and move on to supporting
> multi lower layers if needed.
> 
> Thanks,
> Amir.
>