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. 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. 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