linux-kernel - Re: [GIT PULL] iomap: new code for 5.4

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20190919034502.GJ2229799@magnolia>
Date:   Wed, 18 Sep 2019 20:45:02 -0700
From:   "Darrick J. Wong" <darrick.wong@...cle.com>
To:     Linus Torvalds <torvalds@...ux-foundation.org>
Cc:     "Darrick J. Wong" <djwong@...nel.org>,
        linux-fsdevel <linux-fsdevel@...r.kernel.org>,
        linux-xfs <linux-xfs@...r.kernel.org>,
        Dave Chinner <david@...morbit.com>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        Eric Sandeen <sandeen@...deen.net>,
        Christoph Hellwig <hch@....de>,
        Andreas Gruenbacher <agruenba@...hat.com>,
        Bob Peterson <rpeterso@...hat.com>,
        cluster-devel <cluster-devel@...hat.com>
Subject: Re: [GIT PULL] iomap: new code for 5.4

On Wed, Sep 18, 2019 at 06:31:29PM -0700, Linus Torvalds wrote:
> On Tue, Sep 17, 2019 at 8:21 AM Darrick J. Wong <djwong@...nel.org> wrote:
> >
> > Please pull this series containing all the new iomap code for 5.4.
> 
> So looking at the non-iomap parts of it, I react to the new "list_pop() code.
> 
> In particular, this:
> 
>         struct list_head *pos = READ_ONCE(list->next);
> 
> is crazy to begin with..
> 
> It seems to have come from "list_empty()", but the difference is that
> it actually makes sense to check for emptiness of a list outside
> whatever lock that protects the list. It can be one of those very
> useful optimizations where you don't even bother taking the lock if
> you can optimistically check that the list is empty.
> 
> But the same is _not_ true of an operation like "list_pop()". By
> definition, the list you pop something off has to be stable, so the
> READ_ONCE() makes no sense here.
> 
> Anyway, if that was the only issue, I wouldn't care. But looking
> closer, the whole thing is just completely wrong.
> 
> All the users seem to do some version of this:
> 
>         struct list_head tmp;
> 
>         list_replace_init(&ioend->io_list, &tmp);
>         iomap_finish_ioend(ioend, error);
>         while ((ioend = list_pop_entry(&tmp, struct iomap_ioend, io_list)))
>                 iomap_finish_ioend(ioend, error);
> 
> which is completely wrong and pointless.
> 
> Why would anybody use that odd "list_pop()" thing in a loop, when what
> it really seems to just want is that bog-standard
> "list_for_each_entry_safe()"
> 
>         struct list_head tmp;
>         struct iomap_ioend *next;
> 
>         list_replace_init(&ioend->io_list, &tmp);
>         iomap_finish_ioend(ioend, error);
>         list_for_each_entry_safe(struct iomap_ioend, next, &tmp, io_list)
>                 iomap_finish_ioend(ioend, error);
> 
> which is not only the common pattern, it's more efficient and doesn't
> pointlessly re-write the list for each entry, it just walks it (and
> the "_safe()" part is because it looks up the next entry early, so
> that the entry that it's walking can be deleted).
> 
> So I pulled it. But then after looking at it, I unpulled it again
> because I don't want to see this kind of insanity in one of THE MOST
> CORE header files we have in the whole kernel.
> 
> If xfs and iomap want to think they are "popping" a list, they can do
> so. In the privacy of your own home, you can do stupid and pointless
> things.
> 
> But no, we don't pollute core kernel code with those stupid and
> pointless things.

Ok, thanks for the feedback.  TBH I'd wondered if list_pop was really
necessary, but as it didn't seem to harm anything I let it go.

Anyway, how should I proceed now?  Christoph? :D

I propose the following (assuming Linus isn't cranky enough to refuse
the entire iomap patchpile forever):

Delete patch 1 and 9 from the series, and amend patch 2 as such:

diff --git a/fs/iomap/buffered-io.c b/fs/iomap/buffered-io.c
index 051b8ec326ba..558d09bc5024 100644
--- a/fs/iomap/buffered-io.c
+++ b/fs/iomap/buffered-io.c
@@ -1156,10 +1156,11 @@ void
 iomap_finish_ioends(struct iomap_ioend *ioend, int error)
 {
 	struct list_head tmp;
+	struct iomap_ioend *next;
 
 	list_replace_init(&ioend->io_list, &tmp);
 	iomap_finish_ioend(ioend, error);
-	while ((ioend = list_pop_entry(&tmp, struct iomap_ioend, io_list)))
+	list_for_each_entry_safe(ioend, next, &tmp, io_list)
 		iomap_finish_ioend(ioend, error);
 }
 EXPORT_SYMBOL_GPL(iomap_finish_ioends);

Does that sound ok?  It's been running through xfstests for a couple of
hours now and hasn't let any smoke out...

--D

> 
>               Linus
>