On Mon, 2 Jun 2014 01:46:16 -0700 Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote: > 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. > Ok, fair enough. A later patch in the series adds a per nfs4_file lock (the fi_lock) that we can use for this purpose in lieu of the i_lock. Nesting that within the i_lock shouldn't be too painful. > 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. > This is a problem however. The del_recall_lru list is a per-namespace list, so we need a lock that is either global or per-namespace to add things onto it. The point of this patch is to get that extra locking out of the codepath where the i_lock is already held. We could do that within the rpc_call_prepare operation, but the main problem there is that the delegation could leak if the rpc task allocation fails. Would you be amenable to adding a "cb_prepare" operation or something that we could use to run things from the workqueue before the actual callback runs? -- Jeff Layton <jlayton@xxxxxxxxxxxxxxx> -- 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
