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@xxxxxxxxxx> 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
>
