Re: [GIT PULL] iomap: new code for 5.4

["Darrick J. Wong" <darrick.wong@oracle.com>]

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