Message-ID: <1499280806.4917.3.camel@redhat.com>
Date:   Wed, 05 Jul 2017 14:53:26 -0400
From:   Jeff Layton <jlayton@...hat.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     Kees Cook <keescook@...gle.com>,
        LKML <linux-kernel@...r.kernel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        Benjamin Coddington <bcodding@...hat.com>
Subject: Re: [GIT PULL] file locking and writeback error handling patches
 for v4.13

On Wed, 2017-07-05 at 11:10 -0700, Linus Torvalds wrote:
> On Mon, Jul 3, 2017 at 6:58 AM, Jeff Layton <jlayton@...hat.com> 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@...hat.com>

Powered by blists - more mailing lists