On Thu, Apr 30, 2015 at 07:52:25AM +1000, NeilBrown wrote: > On Wed, 29 Apr 2015 15:19:34 -0400 "J. Bruce Fields" <bfields@xxxxxxxxxxxx> > wrote: > > Maybe drop the locking from nfsd_buffered_readdir and *just* take the > > i_mutex around lookup_one_len(), if that's the only place we need it? > > I think it is the only place needed. Certainly normal path lookup > only takes i_mutex very briefly around __lookup_hash(). > > One cost would be that we would take the mutex for every name instead > of once for a whole set of names. That is generally frowned upon for > performance reasons. > > An alternative might be to stop using lookup_one_len, and use > kern_path() instead. Or kern_path_mountpoint if we don't want to > cross a mountpoint. > > They would only take the i_mutex if absolutely necessary. > > The draw back is that they don't take a 'len' argument, so we would > need to make sure the name is nul terminated (and maybe double-check > that there is no '/'??). It would be fairly easy to nul-terminate > names from readdir - just use a bit more space in the buffer in > nfsd_buffered_filldir(). They're also a lot more complicated than lookup_one_len. Is any of this extra stuff they do (audit?) going to bite us? For me understanding that would be harder than writing a variant of lookup_one_len that took the i_mutex when required. But I guess that's my problem, I should try to understand the lookup code better.... > I'm less sure about the locking needed in nfsd_lookup_dentry(). The > comments suggests that there is good reason to keep the lock for an > extended period. But that reasoning might only apply to files, and > nfsd_mountpoint should only be true for directories... I hope. I thought d_mountpoint() could be true for files, but certainly we won't be doing nfs4 opens on directories. > A different approach would be to pass NULL for the rqstp to > nfsd_cross_mnt(). This will ensure it doesn't block, but it might > fail incorrectly. If it does fail, we drop the lock, retry with the > real rqstp, retake the lock and .... no, I think that gets a bit too > messy. Yes. > I think I'm in favour of: > - not taking the lock in readdir > - maybe not taking it in lookup > - using kern_path_mountpoint or kern_path > - nul terminating then names, copying the nfsd_lookup name to > __getname() if necessary. > > Sound reasonable? I guess so, though I wish I understood kern_path_mountpoint better. And nfsd_lookup_dentry looks like work for another day. No, wait, I forgot the goal here: you're right, nfsd_lookup_dentry has the same upcall-under-i_mutex problem, so we need to fix it too. OK, agreed. --b. -- 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
