On Fri, May 30, 2014 at 09:09:33AM -0400, Jeff Layton wrote: > From: Trond Myklebust <trond.myklebust@xxxxxxxxxxxxxxx> > > 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. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
