Re: [PATCH 9/9] NFSd: protect delegation setup with the i_lock

[Christoph Hellwig <hch@infradead.org>]

On Fri, May 30, 2014 at 09:09:33AM -0400, Jeff Layton wrote:
> From: Trond Myklebust <trond.myklebust@primarydata.com>
> 
> state_lock is a heavily contended global lock. We don't want to grab
> that while simultaneously holding the inode->i_lock. Avoid doing that in
> the delegation break callback by ensuring that we add/remove the
> dl_perfile under the inode->i_lock.

The current code never takes the state lock (or recall lock) under
i_lock.  Non-trivial use of i_lock in nfsd is only added in this patch.

> 
> This however, can cause an ABBA deadlock between the state_lock and the
> i_lock. Work around that by doing the list manipulation in the delegation
> recall from the workqueue context prior to starting the rpc call.

I very much dislike the patch for multiple reasons.  For one thing
nfsd currently stays away from i_lock which is a VFS (and sometimes
abused by the fs) lock except for a single case in the file locking code
which really should be factored into common code.  I'd rather have
locks in nfsd than starting to use i_lock in nfsd and making auditing
it's use and it's lock hierchies more complex by introducing usage
outside of the VFS.

Second I really don't like pushing the delegation setup into the
callback workqueue.  I have a patchset refactoring the callback code
to allow adding more callbacks without lots of copy & paste, and except
for this new addition the code in the workqueue already is almost
perfectly generic for arbitrary callbacks.

Please add a lock in struct nfs4_file to protects it's own members
instead.