* Re: [GIT PULL] iomap: new code for 5.4
2019-09-19 1:31 ` Linus Torvalds
@ 2019-09-19 2:07 ` Linus Torvalds
2019-09-19 3:45 ` Darrick J. Wong
2019-09-19 17:01 ` Christoph Hellwig
2 siblings, 0 replies; 9+ messages in thread
From: Linus Torvalds @ 2019-09-19 2:07 UTC (permalink / raw)
To: Darrick J. Wong
Cc: linux-fsdevel, linux-xfs, Dave Chinner,
Linux Kernel Mailing List, Eric Sandeen, Christoph Hellwig,
Andreas Gruenbacher, Bob Peterson, cluster-devel
On Wed, Sep 18, 2019 at 6:31 PM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> 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()"
Side note: I do agree that the list_for_each_entry_safe() thing isn't
exactly beautiful, particularly since you need that extra variable for
the temporary "next" pointer.
It's one of the C++ features I'd really like to use in the kernel -
the whole "declare new variable in a for (;;) statement" thing.
In fact, it made it into C - it's there in C99 - but we still use
"-std=gnu89" because of other problems with the c99 updates.
Anyway, I *would* be interested in cleaning up
list_for_each_entry_safe() if somebody has the energy and figures out
what we could do to get the c99 behavior without the breakage from
other sources.
For some background: the reason we use "gnu89" is because we use the
GNU extension with type cast initializers quite a bit, ie things like
#define __RAW_SPIN_LOCK_UNLOCKED(lockname) \
(raw_spinlock_t) __RAW_SPIN_LOCK_INITIALIZER(lockname)
and that broke in c99 and gnu99, which considers those compound
literals and you can no longer use them as initializers.
See
https://lore.kernel.org/lkml/20141019231031.GB9319@node.dhcp.inet.fi/
for some of the historical discussion about this. It really _is_ sad,
because variable declarations inside for-loops are very useful, and
would have the potential to make some of our "for_each_xyz()" macros a
lot prettier (and easier to use too).
So our list_for_each_entry_safe() thing isn't perfect, but that's no
reason to try to then make up completely new things.
Linus
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [GIT PULL] iomap: new code for 5.4
2019-09-19 1:31 ` Linus Torvalds
2019-09-19 2:07 ` Linus Torvalds
@ 2019-09-19 3:45 ` Darrick J. Wong
2019-09-19 17:03 ` Linus Torvalds
2019-09-19 17:01 ` Christoph Hellwig
2 siblings, 1 reply; 9+ messages in thread
From: Darrick J. Wong @ 2019-09-19 3:45 UTC (permalink / raw)
To: Linus Torvalds
Cc: Darrick J. Wong, linux-fsdevel, linux-xfs, Dave Chinner,
Linux Kernel Mailing List, Eric Sandeen, Christoph Hellwig,
Andreas Gruenbacher, Bob Peterson, cluster-devel
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
>
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [GIT PULL] iomap: new code for 5.4
2019-09-19 3:45 ` Darrick J. Wong
@ 2019-09-19 17:03 ` Linus Torvalds
2019-09-19 17:04 ` Linus Torvalds
2019-09-19 17:07 ` Christoph Hellwig
0 siblings, 2 replies; 9+ messages in thread
From: Linus Torvalds @ 2019-09-19 17:03 UTC (permalink / raw)
To: Darrick J. Wong
Cc: Darrick J. Wong, linux-fsdevel, linux-xfs, Dave Chinner,
Linux Kernel Mailing List, Eric Sandeen, Christoph Hellwig,
Andreas Gruenbacher, Bob Peterson, cluster-devel
On Wed, Sep 18, 2019 at 8:45 PM Darrick J. Wong <darrick.wong@oracle.com> wrote:
>
> I propose the following (assuming Linus isn't cranky enough to refuse
> the entire iomap patchpile forever):
Since I don't think the code was _wrong_, it was just that I didn't
want to introduce a new pattern for something we already have, I'd be
ok with just a fix patch on top too.
And if the xfs people really want to use the "pop" model, that's fine
too - just make it specific to just the iomap_ioend type, which is all
you seem to really want to pop, and make the typing (and naming) be
about that particular thing.
As mentioned, I don't think that whole "while (pop)" model is _wrong_
per se. But I also don't like proliferating new random list users in
general, just because some subsystem has some internal preference.
See?
And I notice that one of the users actually does keep the list around
and modifies it, ie this one:
while ((ioend = list_pop_entry(&tmp, struct xfs_ioend, io_list))) {
xfs_ioend_try_merge(ioend, &tmp);
xfs_end_ioend(ioend);
}
actually seems to _work_ on the remaining part of the list in that
xfs_ioend_try_merge() function.
So inside of xfs, that "pop ioend from the list" model really may make
perfect sense, and you could just do
static inline struct xfs_ioend *xfs_pop_ioend(struct list_head *head)
{
struct xfs_ioend *ioend = list_first_entry_or_null(head,
struct xfs_ioend *, io_list);
if (ioend)
list_del(&ioend->io_list);
return ioend;
}
and I don't think it's wrong.
It's just that we've survived three decades without that list_pop(),
and I don't want to make our header files even bigger and more complex
unless we actually have multiple real users.
Linus
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [GIT PULL] iomap: new code for 5.4
2019-09-19 17:03 ` Linus Torvalds
@ 2019-09-19 17:04 ` Linus Torvalds
2019-09-19 17:07 ` Christoph Hellwig
1 sibling, 0 replies; 9+ messages in thread
From: Linus Torvalds @ 2019-09-19 17:04 UTC (permalink / raw)
To: Darrick J. Wong
Cc: Darrick J. Wong, linux-fsdevel, linux-xfs, Dave Chinner,
Linux Kernel Mailing List, Eric Sandeen, Christoph Hellwig,
Andreas Gruenbacher, Bob Peterson, cluster-devel
On Thu, Sep 19, 2019 at 10:03 AM Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> So inside of xfs, that "pop ioend from the list" model really may make
> perfect sense, and you could just do
>
> static inline struct xfs_ioend *xfs_pop_ioend(struct list_head *head)
Note that as usual, my code in emails is written in the MUA, entirely
untested, and may be completely broken.
So take it just as a "maybe something like this works for you".
Linus
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [GIT PULL] iomap: new code for 5.4
2019-09-19 17:03 ` Linus Torvalds
2019-09-19 17:04 ` Linus Torvalds
@ 2019-09-19 17:07 ` Christoph Hellwig
2019-09-19 17:41 ` Linus Torvalds
1 sibling, 1 reply; 9+ messages in thread
From: Christoph Hellwig @ 2019-09-19 17:07 UTC (permalink / raw)
To: Linus Torvalds
Cc: Darrick J. Wong, Darrick J. Wong, linux-fsdevel, linux-xfs,
Dave Chinner, Linux Kernel Mailing List, Eric Sandeen,
Christoph Hellwig, Andreas Gruenbacher, Bob Peterson,
cluster-devel
On Thu, Sep 19, 2019 at 10:03:30AM -0700, Linus Torvalds wrote:
> It's just that we've survived three decades without that list_pop(),
> and I don't want to make our header files even bigger and more complex
> unless we actually have multiple real users.
I personally surived with it, but really hated writing the open coded
list_for_each_entry + list_del or while list_first_entry_or_null +
┐ist_del when I could have a nice primitive for it. I finally decided
to just add that helper..
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [GIT PULL] iomap: new code for 5.4
2019-09-19 17:07 ` Christoph Hellwig
@ 2019-09-19 17:41 ` Linus Torvalds
0 siblings, 0 replies; 9+ messages in thread
From: Linus Torvalds @ 2019-09-19 17:41 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Darrick J. Wong, Darrick J. Wong, linux-fsdevel, linux-xfs,
Dave Chinner, Linux Kernel Mailing List, Eric Sandeen,
Andreas Gruenbacher, Bob Peterson, cluster-devel
On Thu, Sep 19, 2019 at 10:07 AM Christoph Hellwig <hch@lst.de> wrote:
>
> I personally surived with it, but really hated writing the open coded
> list_for_each_entry + list_del or while list_first_entry_or_null +
> ┐ist_del when I could have a nice primitive for it. I finally decided
> to just add that helper..
You realize that the list helper is even mis-named?
Generally a "pop()" should pop the last entry, not the first one. Or
course, which one is last and which one is first is entirely up to you
since you can add at the tail or the beginning. So even the name is
ambiguous.
So I really think the whole thing was badly implemented (and yes, part
of that was the whole implementation thing).
Doing list_del() by hand also means that you can actually decide if
you want list_del_init() or not. So it's
But again - keep that helper if you want it. Just don't put a badly
implemented and badly named "helper" it in a global header file that
_everybody_ has to look at.
Linus
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [GIT PULL] iomap: new code for 5.4
2019-09-19 1:31 ` Linus Torvalds
2019-09-19 2:07 ` Linus Torvalds
2019-09-19 3:45 ` Darrick J. Wong
@ 2019-09-19 17:01 ` Christoph Hellwig
2 siblings, 0 replies; 9+ messages in thread
From: Christoph Hellwig @ 2019-09-19 17:01 UTC (permalink / raw)
To: Linus Torvalds
Cc: Darrick J. Wong, linux-fsdevel, linux-xfs, Dave Chinner,
Linux Kernel Mailing List, Eric Sandeen, Christoph Hellwig,
Andreas Gruenbacher, Bob Peterson, cluster-devel
On Wed, Sep 18, 2019 at 06:31:29PM -0700, Linus Torvalds wrote:
> 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.
Indeed.
> 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).
That might be true for the current two cases that operate on a temporary
local list, but in general we have lots of cases where we operate on
lists that are not just local and where have to delete all the entries.
Sure, we could somehow let them dangle and then just do a INIT_LIST_HEAD
on the list later, but that is just asking for trouble down the road
when people actually use list_empty in the functions called in the loop.
^ permalink raw reply [flat|nested] 9+ messages in thread