Re: [PATCH v2] BTRFS/NFSD: provide more unique inode number for btrfs export

From: "NeilBrown" <neilb@suse.de>
To: "Christoph Hellwig" <hch@infradead.org>
Cc: "J. Bruce Fields" <bfields@fieldses.org>,
	"Chuck Lever" <chuck.lever@oracle.com>,
	linux-nfs@vger.kernel.org, "Josef Bacik" <josef@toxicpanda.com>
Subject: Re: [PATCH v2] BTRFS/NFSD: provide more unique inode number for btrfs export
Date: Tue, 31 Aug 2021 14:59:05 +1000	[thread overview]
Message-ID: <163038594541.7591.11109978693705593957@noble.neil.brown.name> (raw)
In-Reply-To: <YSnhHl0HDOgg07U5@infradead.org>

On Sat, 28 Aug 2021, Christoph Hellwig wrote:
> On Sat, Aug 28, 2021 at 09:05:08AM +1000, NeilBrown wrote:
> > There doesn't seem to be any other option - and this is still an
> > improvement over the current behaviour.
> > 
> > Collisions should still be quite a few years away, and there is hope
> > that the btrfs developers will take action before then to provide some
> > certainty.   Not much hope, but some.
> 
> I think that is a very dangerous assumption.  Given how every inode
> allocation tends to be somewhat predictable I'm also really worried
> that this actually opens up an attach vector.  Last but not least

It doesn't 'open up' anything because it is already possible to cause inode
number collisions for NFS mounts of BTRFS.  So this patch is a net
improvement.

I agree that it isn't perfect, but it is the best I have managed to find
and it does solve real problems.  Can you suggest any way to make it
better?

> I also very much disagree with any of the impact to common code.
> Most importantly the kstat structure, which exist to support the stat
> family of system calls and not as a side channel for NFS file handles
> (nevermind that it is hidden in a nfs patch and didn't even Cc the
> fsdevel list), but also all the impact to the generic nfsd code for
> this very broken concept.  If you want to support such a scheme in
> btrfs as the lesser of evils (which I disagree with), please make
> sure it stays self-contained in the btrfs specific file handle
> encoding and decoding.

Making the change purely in btrfs is simply not possible.  There is no
way for btrfs to provide nfsd with a different inode number.  To move
the bulk of the change into btrfs code we would need - at the very least
- some way for nfsd to provide the filehandle when requesting stat
information.  We would also need to provide a reference filehandle when
requesting a dentry->filehandle conversion.  Cluttering the
export_operations like that just for btrfs doesn't seem like the right
balance.  I agree that cluttering kstat is not ideal, but it was a case
of choosing the minimum change for the maximum effect.

fsdevel *was* cc:ed on the early discussion of this patch

https://lwn.net/ml/linux-fsdevel/162969155423.9892.18322100025025288277@noble.neil.brown.name/

I felt we were at a point where I really needed to focus in on the nfsd
side of the discussion, with the nfsd developers.

As you are probably aware I have already been through several approaches
to solve this problem.  I'm not against exploring other avenues, but
only if they are genuinely likely to provide measurably better results. 
I'd be very happy to hear your suggestions.

NeilBrown