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

[NeilBrown <neilb@suse.com>]

[-- Attachment #1: Type: text/plain, Size: 2057 bytes --]

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.
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??) and add some wait queue somewhere
so the breaker could wait for a delegation to be returned.

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.

Thanks,
NeilBrown

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]