[2/3] fs: hide another detail of delegation logic

On Tue, Sep 05 2017, J. Bruce Fields wrote:

> On Mon, Sep 04, 2017 at 02:52:43PM +1000, NeilBrown wrote:
>> On Fri, Sep 01 2017, J. Bruce Fields wrote:
>> 
>> >> 
>> >> nfsd would need to find that delegation, prevent further delegations
>> >> being handed out, and check that there aren't already conflicting
>> >> delegations.  If there are conflicts, recall them.  Once there are no
>> >> conflicting delegations, make the vfs_ request.
>> >
>> > The way that we currently serialize setting, unsetting, and breaking
>> > delegations is by locks on the delegated inodes which aren't taken till
>> > deeper in the vfs code.
>> 
>> Do we?
>> I can see nfs4_set_delegation adding a new delegation for a new client
>> without entering the vfs at all if there is already a lease held.
>
> By "delegations", I meant locks of type FL_DELEG.  But even then I was
> wrong, apologies.
>
> There is an inode_trylock in generic_add_lease that will prevent any new
> delegations from being given while the inode's locked.
>
>> If there isn't a lease already, vfs_setlease() is called, which doesn't
>> its own internal locking of course.  Much the same applies to unsetting
>> delegations.
>> Breaking delegations involves nfsd_break_deleg_cb() which has a comment
>> that it is called with i_lock held.... that seems to be used to
>> be sure that it is safe to a reference to the delegation state id.
>> Is that the only dependency on the vfs locking, or did I miss something?
>> 
>> >
>> > I guess you're suggesting adding a second mechanism to prevent
>> > delegations being given out on the inode.  We could add an atomic
>> > counter taken by each nfsd breaker while it's in progress.  Hrm.
>> 
>> Something like that.
>> We would also need to be able to look up an nfs4_file by inode (why
>> *are* they hashed by file handle??)
>
> Grepping the logs....  That was ca9432178378 "nfsd: Use the filehandle
> to look up the struct nfs4_file instead of inode" which doesn't give a
> full justification.  Later commits suggest it might be about keeping
> nfsv4 state in many-to-one filehandle->inode cases (spec requirement, I
> believe) and preventing the nfs4_file from pinning the inode (not seeing
> immediately why that was an issue).
>
> Anyway, I can't think of a reason why hashing the filehandle's a
> problem.

Thanks for the background.  I didn't see it as a problem exactly,
though I did wonder about different filehandles mapping to the same
nfs4_file (unlikely but possible).  You say that is required and I can
see how that might be.

My perspective was more that I do want to perform a lookup by inode.
When an UNLINK arrives we lookup the dentry and so know the inode.  Then
we want to see if the client holds a delegation.  So we want to find the
nfs4_file given the dentry/inode.  We could, of course, use
export_encode_fh, but that seems a bit round-about.
We could add a second index, but would need to allow that there could be
multiple nfs4_files for a given inode.

>
>> and add some wait queue somewhere
>> so the breaker could wait for a delegation to be returned.
>
> In the nfsd case we're just returning to the client immediately, so
> that's not really necessary, though maybe it could be useful.

Ah yes, so we do.  I inverted the logic in my mind.  That makes it easier.

>
>> My big-picture point is that any complexity created by NFSD's choice to
>> merge delegations to multiple clients into a single vfs-level delegation
>> should be handled by NFSD, and not imposed on the VFS.
>> It certainly makes sense for the VFS to understand that certain
>> operations are being performed by an fl_owner_t, and to allow
>> delegations to that owner to remain.  It doesn't make as much sense for
>> the VFS to understand that there is a finer granularity of ownership
>> than the one that it already supports.
>
> Fair enough, I'll think about that.

Thanks.  Below is a patch that does compile but is probably wrong is
various ways and definitely needs cleanliness work at least.  I provide
it just to be more concrete about my thinking.

NeilBrown

[2/3] fs: hide another detail of delegation logic

Commit Message

Comments

Patch