On Wed, 2017-07-05 at 11:10 -0700, Linus Torvalds wrote: > On Mon, Jul 3, 2017 at 6:58 AM, Jeff Layton <jlayton@xxxxxxxxxx> wrote: > > > > File locking and writeback error handling patches for v4.13 > > Yeah, I won't be pulling this. > > It's a random collection of patches, and the ones I looked at looked > actively wrong. > > For example, you add support for remote pid in one commit, and then in > the next commit you convert filesystems to use them. > > Which means that in between, locking presumably doesn't work ata ll, > because the locks_translate_pid() function presumably now does the > wrong thing. It used to be conditional of fl_nspid, which hid that > issue, but that now goes away, and you rely on the sign of the pid > instead, but the sign hasn't been *changed* yet. > (cc'ing Ben) Good point. I think the end result is where we want to be, but it might be best to squash the last two locking patches there into a single patch to ensure we don't have a regression at between them. Ben, any objections to doing that? > Now, it's entirely possible that I'm mis-reading all this, but it > means that I find even the file locking changes suspicious. > > But that's not what kills this pull request for me. > > No, what makes me go "No" is that it then mixes up the file locking > changes with completely unrelated mapping things. > > And some of those look like obvious fixes that I have nothing at all against. > > And then some just look stupid (but aren't): > > - return ret; > + err = filemap_check_errors(inode->i_mapping); > + return ret ? ret : err; > > Yeah, why is it pointlessly calling filemap_check_errors() even when > the value won't get used? It turns out that that's because > filemap_check_errors() also *clears* the pending errors, but that > isn't at all obvious from the code, so it definitely merits a comment > somewhere). > > Related to that very fact, in other parts "filemap_check_errors()" is > literally used to clear the errors, and a comment *is* added there. > Which makes me think that maybe that function should just be renamed. > Fair enough -- I'll toss in a patch to rename it to filemap_check_and_clear_errors() early in the series. > But.. Then you introduce the writeback error code which is big and new > and doesn't look wrong, but wasn't really described much in your pull > request, and just makes me go "this is all a surprise, and none of it > has anythign to do with file locking". > > So together, all of these issues spell "no, I'm not happy to pull > this" to me. Maybe the code is all fine for various reasons, maybe I'm > mis-reading the non-bisectable semantic change to pid's, maybe it's > all good. > > But even if the code is all good, I *really* want this as separate > pull requests that make sense on their own. One for file locking that > was unrelated to everything else, one for the trivial fixes, and one > for the whole new writeback error handling logic. > > Linus Ok, thanks for looking -- that's all fair criticism. I'll clean this up into separate branches and send them as 3 individual PRs. I should be able to get them sent out tomorrow or Friday. Thanks, -- Jeff Layton <jlayton@xxxxxxxxxx>
