On Mon, 23 Jun 2014 06:39:26 -0700 Christoph Hellwig <hch@xxxxxxxxxxxxx> wrote: > Hi Jeff, > > thanks for bringing this forward. I've started looking at the series, > but I'm a little overwhelmed as it's growing bigger and bigger with > each posting. That also makes me a little worried about putting all > of it into 3.17. I know you've started separating a few bits out and > some of those actually went into 3.16, but maybe we really should > start to some easier to split piece in first and make sure they get > into 3.17 and then see if we can get through the rest of it in time. > Thanks for taking a look. The big problem with breaking this set up is that it will likely result in at least some performance regression in the interim. We're adding more granular locking inside of the coarse-grained client_mutex, which is likely to mean at least some slowdown until the client_mutex is removed. Maybe that's worth it though. > Besides the obvious fixes for bits that were racy before and don't > just need better scalability one thing that strikes to mind are some > of the higher level logic changes, like the changes fixes to various > stateowner / stateid relations. > I'll have to think about how to break those out. Unfortunately, there are strong dependencies between some of those changes, which makes it hard to do this in pieces. > Besides that some higher level comments from glacing over the series: > > - the patches that touch locks.c and the lockdep code should go > first, and include the maintainer (at least for lockdep, locks.c is > easy :)) and relevant mailing lists. Fair enough. I meant to cc the lockdep folks on the lockdep_assert_not_held patch. I don't expect pushback from them, but you're correct that they should be Cc'ed. > - lots of patches only have a subject line, but no explanation in the > body at all. While this might be enough for trivial patches few > of them are trivial enough that this would be enough. Ok. I'll try to flesh them out for the next posting. > - some patches have a signoff from Trond, but no From: line for him > in the body. I suspect this just got lost somewhere. Yeah. Some of them were originally written by Trond, but then I ended up having to do large scale rework of them. I prob should have just dropped his SOB lines from those. I'll go over those and do that or fix the From: lines. > - there is some confusion of NFSd vs nfsd in the subsystem prefixes. > While it seems odd and against the usual naming NFSd seems to be > the common one for nfs patches. > I tend to prefer "nfsd", but ok -- "NFSd" it is. > checkpatch.pl also has various valid complains (mostly about too long > lines) and a few silly ones about printks, might be worth to address > them. > Ahh right. I need to fix those -- Steve French reported a few of those too (and sent me patches privately). > Last but not least I get a new compiler warning with the whole series > applied, as put_client is only used by the fault injection code, but > still compiled when the config option for it is not set. > Good catch. I'll fix that. > I hope I'll have some more useful detailed reviews soon. > Thanks. I should also note that a couple of races were found in the nfs4_ol_stateid handling while testing this set, so I'll be respinning it to fix those up anyway. -- 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
