Received-SPF: pass (google.com: domain of linux-nfs-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <162742539595.32498.13687924366155737575.stgit@noble.brown>
 <162742546554.32498.9309110546560807513.stgit@noble.brown>
 <CAOQ4uxjXcVE=4K+3uSYXLsvGgi0o7Nav=DsV=0qG_DanjXB18Q@mail.gmail.com> <162751852209.21659.13294658501847453542@noble.neil.brown.name>
In-Reply-To: <162751852209.21659.13294658501847453542@noble.neil.brown.name>
From:   Amir Goldstein <amir73il@gmail.com>
Date:   Thu, 29 Jul 2021 08:27:15 +0300
Message-ID: <CAOQ4uxj9DW2SHqWCMXy4oRdazbODMhtWeyvNsKJm__0fuuspyQ@mail.gmail.com>
Subject: Re: [PATCH 07/11] exportfs: Allow filehandle lookup to cross internal
 mount points.
To:     NeilBrown <neilb@suse.de>
Cc:     Christoph Hellwig <hch@infradead.org>,
        Josef Bacik <josef@toxicpanda.com>,
        "J. Bruce Fields" <bfields@fieldses.org>,
        Chuck Lever <chuck.lever@oracle.com>, Chris Mason <clm@fb.com>,
        David Sterba <dsterba@suse.com>,
        Alexander Viro <viro@zeniv.linux.org.uk>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>,
        Linux NFS Mailing List <linux-nfs@vger.kernel.org>,
        Linux Btrfs <linux-btrfs@vger.kernel.org>,
        Miklos Szeredi <miklos@szeredi.hu>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Thu, Jul 29, 2021 at 3:28 AM NeilBrown <neilb@suse.de> wrote:
>
> On Wed, 28 Jul 2021, Amir Goldstein wrote:
> > On Wed, Jul 28, 2021 at 1:44 AM NeilBrown <neilb@suse.de> wrote:
> > >
> > > When a filesystem has internal mounts, it controls the filehandles
> > > across all those mounts (subvols) in the filesystem.  So it is useful to
> > > be able to look up a filehandle again one mount, and get a result which
> > > is in a different mount (part of the same overall file system).
> > >
> > > This patch makes that possible by changing export_decode_fh() and
> > > export_decode_fh_raw() to take a vfsmount pointer by reference, and
> > > possibly change the vfsmount pointed to before returning.
> > >
> > > The core of the change is in reconnect_path() which now not only checks
> > > that the dentry is fully connected, but also that the vfsmnt reported
> > > has the same 'dev' (reported by vfs_getattr) as the dentry.
> > > If it doesn't, we walk up the dparent() chain to find the highest place
> > > where the dev changes without there being a mount point, and trigger an
> > > automount there.
> > >
> > > As no filesystems yet provide local-mounts, this does not yet change any
> > > behaviour.
> > >
> > > In exportfs_decode_fh_raw() we previously tested for DCACHE_DISCONNECT
> > > before calling reconnect_path().  That test is dropped.  It was only a
> > > minor optimisation and is now inconvenient.
> > >
> > > The change in overlayfs needs more careful thought than I have yet given
> > > it.
> >
> > Just note that overlayfs does not support following auto mounts in layers.
> > See ovl_dentry_weird(). ovl_lookup() fails if it finds such a dentry.
> > So I think you need to make sure that the vfsmount was not crossed
> > when decoding an overlayfs real fh.
>
> Sounds sensible - thanks.
> Does this mean that my change would cause problems for people using
> overlayfs with a btrfs lower layer?
>

It sounds like it might :-/
I assume that enabling automount in btrfs in opt-in?
Otherwise you will be changing behavior for users of existing systems.

I am not sure, but I think it may be possible to remove the AUTOMOUNT
check from the ovl_dentry_weird() condition with an explicit overlayfs
config/module/mount option so that we won't change behavior by
default, but distro may change the default for overlayfs.

Then, when admin changes the btrfs options on the system to perform
automounts, it will also need to change the overlayfs options to not
error on automounts.

Given that today, subvolume mounts (or any mounts) on the lower layer
are not followed by overlayfs, I don't really see the difference
if mounts are created manually or automatically.
Miklos?

> >
> > Apart from that, I think that your new feature should be opt-in w.r.t
> > the exportfs_decode_fh() vfs api and that overlayfs should not opt-in
> > for the cross mount decode.
>
> I did consider making it opt-in, but it is easy enough for the caller
> to ignore the changed vfsmount, and only one (of 4) callers that it
> really makes a difference for.
>

Which reminds me. Please ignore the changed vfsmount in
do_handle_to_path() (or do not opt-in to changed vfsmount).

I have an application that uses a bind mount to filter file handles
of directories by subtree. It opens by the file handles that were
acquired from fanotify DFID info record using a mountfd in the
bind mount and readlink /proc/self/fd to determine the path
relative to that subtree bind mount.

Your change, IIUC, is going to change the semantics of
open_by_handle_at(2) and is going to break my application.

If you need this change for nfsd, please keep it as an internal api
used only by nfsd.

TBH, I think it would also be nice to have an internal api to limit
reconnect_path() walk up to mnt->mnt_root, which is what overlayfs
really wants, so here is another excuse for you to introduce
"reconnect flags" to exportfs_decode_fh_raw() ;-)

Note that I had already added support for one implicit "reconnect
flag" (i.e. "don't reconnect") in commit 8a22efa15b46
("ovl: do not try to reconnect a disconnected origin dentry").

Thanks,
Amir.