Re: [PATCH 07/11] exportfs: Allow filehandle lookup to cross internal mount points.

From: "NeilBrown" <neilb@suse.de>
To: "Amir Goldstein" <amir73il@gmail.com>
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>
Subject: Re: [PATCH 07/11] exportfs: Allow filehandle lookup to cross internal mount points.
Date: Thu, 29 Jul 2021 10:28:42 +1000	[thread overview]
Message-ID: <162751852209.21659.13294658501847453542@noble.neil.brown.name> (raw)
In-Reply-To: <CAOQ4uxjXcVE=4K+3uSYXLsvGgi0o7Nav=DsV=0qG_DanjXB18Q@mail.gmail.com>

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?

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

I will review the overlayfs in light of these comments.
Thanks,
NeilBrown