Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1223064 > unrolled thread
| Started by | Chris Mason <clm@fb.com> |
|---|---|
| First post | 2015-09-11 21:40 +0200 |
| Last post | 2015-09-12 01:20 +0200 |
| Articles | 20 on this page of 50 — 8 participants |
Back to article view | Back to linux.kernel
[PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Chris Mason <clm@fb.com> - 2015-09-11 21:40 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-11 22:10 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-11 22:40 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Josef Bacik <jbacik@fb.com> - 2015-09-11 22:50 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-11 23:10 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-12 00:10 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Chris Mason <clm@fb.com> - 2015-09-12 01:20 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-12 01:40 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-12 03:00 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Chris Mason <clm@fb.com> - 2015-09-12 04:20 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-12 04:30 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Chris Mason <clm@fb.com> - 2015-09-13 01:10 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-13 01:30 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Chris Mason <clm@fb.com> - 2015-09-13 01:50 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Chris Mason <clm@fb.com> - 2015-09-13 15:20 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Dave Chinner <david@fromorbit.com> - 2015-09-14 01:00 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Dave Chinner <david@fromorbit.com> - 2015-09-14 01:20 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-14 22:10 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Jan Kara <jack@suse.cz> - 2015-09-16 22:00 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Chris Mason <clm@fb.com> - 2015-09-16 22:10 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Dave Chinner <david@fromorbit.com> - 2015-09-17 00:20 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Dave Chinner <david@fromorbit.com> - 2015-09-17 02:40 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-17 03:20 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Dave Chinner <david@fromorbit.com> - 2015-09-17 04:20 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-17 21:40 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Chris Mason <clm@fb.com> - 2015-09-18 00:50 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-18 01:10 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Chris Mason <clm@fb.com> - 2015-09-18 02:00 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Dave Chinner <david@fromorbit.com> - 2015-09-18 02:40 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-18 04:00 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Dave Chinner <david@fromorbit.com> - 2015-09-18 07:50 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-18 08:10 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-18 08:10 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Jens Axboe <axboe@fb.com> - 2015-09-18 16:30 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Chris Mason <clm@fb.com> - 2015-09-18 15:20 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Jens Axboe <axboe@fb.com> - 2015-09-18 16:30 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-18 17:40 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Peter Zijlstra <peterz@infradead.org> - 2015-09-18 18:10 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Peter Zijlstra <peterz@infradead.org> - 2015-09-18 18:10 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-18 18:20 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Dave Chinner <david@fromorbit.com> - 2015-09-19 01:20 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Jan Kara <jack@suse.cz> - 2015-09-21 11:30 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Andrew Morton <akpm@linux-foundation.org> - 2015-09-21 22:30 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Dave Chinner <david@fromorbit.com> - 2015-09-18 01:10 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-18 01:20 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Chris Mason <clm@fb.com> - 2015-09-17 05:50 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Dave Chinner <david@fromorbit.com> - 2015-09-17 06:40 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Chris Mason <clm@fb.com> - 2015-09-17 14:20 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Chris Mason <clm@fb.com> - 2015-09-12 01:10 +0200
Re: [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() Linus Torvalds <torvalds@linux-foundation.org> - 2015-09-12 01:20 +0200
Page 1 of 3 [1] 2 3 Next page →
| From | Chris Mason <clm@fb.com> |
|---|---|
| Date | 2015-09-11 21:40 +0200 |
| Subject | [PATCH] fs-writeback: drop wb->list_lock during blk_finish_plug() |
| Message-ID | <q7CkW-2Ng-25@gated-at.bofh.it> |
Linus, this is the plugging problem I mentioned in my btrfs pull. It impacts only MD raid10 and btrfs raid5/6, and I'm not wild about the patch. But I wanted to at least send in the basic fix for rc1 so this doesn't cause bigger problems for early testers: Commit d353d7587 added a plug/finish_plug pair to writeback_sb_inodes, but writeback_sb_inodes has a horrible secret...it's called with the wb->list_lock held. Btrfs raid5/6 and MD raid10 have horrible secrets of their own...they both do allocations in their unplug callbacks. None of the options to fix it are very pretty. We don't want to kick off workers for all of these unplugs, and the lock doesn't look hot enough to justify bigger restructuring. [ 2854.025042] BUG: sleeping function called from invalid context at mm/page_alloc.c:3189 [ 2854.041366] in_atomic(): 1, irqs_disabled(): 0, pid: 145562, name: kworker/u66:15 [ 2854.056813] INFO: lockdep is turned off. [ 2854.064870] CPU: 13 PID: 145562 Comm: kworker/u66:15 Not tainted 4.2.0-mason+ #1 [ 2854.080082] Hardware name: ZTSYSTEMS Echo Ridge T4 /A9DRPF-10D, BIOS 1.07 05/10/2012 [ 2854.096211] Workqueue: writeback wb_workfn (flush-btrfs-244) [ 2854.107821] ffffffff81a2bbee ffff880ee09a7598 ffffffff813307bb ffff880ee09a7598 [ 2854.123162] ffff881010d1ca00 ffff880ee09a75c8 ffffffff81086615 0000000000000000 [ 2854.138556] 0000000000000000 0000000000000c75 ffffffff81a2bbee ffff880ee09a75f8 [ 2854.153936] Call Trace: [ 2854.181101] [<ffffffff81086722>] __might_sleep+0x52/0x90 [ 2854.192136] [<ffffffff8116d2b4>] __alloc_pages_nodemask+0x344/0xbe0 [ 2854.229682] [<ffffffff811b54aa>] alloc_pages_current+0x10a/0x1e0 [ 2854.255508] [<ffffffffa0663f19>] full_stripe_write+0x59/0xc0 [btrfs] [ 2854.268600] [<ffffffffa0663fb9>] __raid56_parity_write+0x39/0x60 [btrfs] [ 2854.282385] [<ffffffffa06640fb>] run_plug+0x11b/0x140 [btrfs] [ 2854.294259] [<ffffffffa0664143>] btrfs_raid_unplug+0x23/0x70 [btrfs] [ 2854.307334] [<ffffffff81307622>] blk_flush_plug_list+0x82/0x1f0 [ 2854.319542] [<ffffffff813077c4>] blk_finish_plug+0x34/0x50 [ 2854.330878] [<ffffffff812079c2>] writeback_sb_inodes+0x122/0x580 [ 2854.343256] [<ffffffff81208016>] wb_writeback+0x136/0x4e0 Signed-off-by: Chris Mason <clm@fb.com> Reviewed-by: Jens Axboe <axboe@fb.com> --- fs/fs-writeback.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/fs/fs-writeback.c b/fs/fs-writeback.c index ae0f438..07c9c50 100644 --- a/fs/fs-writeback.c +++ b/fs/fs-writeback.c @@ -1539,7 +1539,9 @@ static long writeback_sb_inodes(struct super_block *sb, break; } } + spin_unlock(&wb->list_lock); blk_finish_plug(&plug); + spin_lock(&wb->list_lock); return wrote; } -- 1.8.1 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2015-09-11 22:10 +0200 |
| Message-ID | <q7CNX-3AD-9@gated-at.bofh.it> |
| In reply to | #1223064 |
I hate this fix.
On Fri, Sep 11, 2015 at 12:37 PM, Chris Mason <clm@fb.com> wrote:
> Linus, this is the plugging problem I mentioned in my btrfs pull. It
> impacts only MD raid10 and btrfs raid5/6, and I'm not wild about the
> patch. But I wanted to at least send in the basic fix for rc1 so this
> doesn't cause bigger problems for early testers:
>
> Commit d353d7587 added a plug/finish_plug pair to writeback_sb_inodes,
> but writeback_sb_inodes has a horrible secret...it's called with the
> wb->list_lock held.
Quite frankly, just dropping and retaking the lock around the
blk_finish_plug() is just disgusting. The whole "drop and retake lock"
pattern in general is a bad idea, because it can so easily break the
caller (because now the lock no longer covers things over the call.
Yes, in this case we already do something similar in
writeback_single_inode(), so I guess the argument is that it doesn't
make things much worse, and that the caller already cannot depend on
the lock being held. True, but no less disgusting for that. So we
could do this, but in this case I don't think there's any good
_reason_ for doing that disgusting thing.
How about we instead:
(a) revert that commit d353d7587 as broken (because it clearly is)
(b) add a big honking comment about the fact that we hold 'list_lock'
in writeback_sb_inodes()
(c) move the plugging up to wb_writeback() and writeback_inodes_wb()
_outside_ the spinlock.
because that way we not only avoid the ugliness, it should also be
more effective at plugging things since we gather _all_ the writeback
rather than just one superblock.
Let's not paper over a completely broken commit. Let's just admit that
commit d353d7587 was prue and utter shite, and rather than try to fix
up the mistake, make it all better!
Anyway, I will start by reverting that commit, and adding the comment.
I'm more than happy to take the patch that moves the plugging, but
since that one was about performance rather than correctness, I think
it would be good to just re-verify the numbers.
Dave?
Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2015-09-11 22:40 +0200 |
| Message-ID | <q7Dh1-48K-27@gated-at.bofh.it> |
| In reply to | #1223077 |
[Multipart message — attachments visible in raw view] — view raw
On Fri, Sep 11, 2015 at 1:02 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> How about we instead:
>
> (a) revert that commit d353d7587 as broken (because it clearly is)
>
> (b) add a big honking comment about the fact that we hold 'list_lock'
> in writeback_sb_inodes()
>
> (c) move the plugging up to wb_writeback() and writeback_inodes_wb()
> _outside_ the spinlock.
Ok, I've done (a) and (b) in my tree. And attached is the totally
untested patch for (c). It looks ObviouslyCorrect(tm), but since this
is a performance issue, I'm not going to commit it without some more
ACK's from people.
I obviously think this is a *much* better approach than dropping and
retaking the lock, but there might be something silly I'm missing.
For example, maybe we want to unplug and replug around the
"inode_sleep_on_writeback()" in wb_writeback()? So while the revert
was a no-brainer, this one I really want people to think about.
Linus
[toc] | [prev] | [next] | [standalone]
| From | Josef Bacik <jbacik@fb.com> |
|---|---|
| Date | 2015-09-11 22:50 +0200 |
| Message-ID | <q7DqG-4kk-13@gated-at.bofh.it> |
| In reply to | #1223094 |
On 09/11/2015 04:37 PM, Linus Torvalds wrote: > On Fri, Sep 11, 2015 at 1:02 PM, Linus Torvalds > <torvalds@linux-foundation.org> wrote: >> >> How about we instead: >> >> (a) revert that commit d353d7587 as broken (because it clearly is) >> >> (b) add a big honking comment about the fact that we hold 'list_lock' >> in writeback_sb_inodes() >> >> (c) move the plugging up to wb_writeback() and writeback_inodes_wb() >> _outside_ the spinlock. > > Ok, I've done (a) and (b) in my tree. And attached is the totally > untested patch for (c). It looks ObviouslyCorrect(tm), but since this > is a performance issue, I'm not going to commit it without some more > ACK's from people. > > I obviously think this is a *much* better approach than dropping and > retaking the lock, but there might be something silly I'm missing. > > For example, maybe we want to unplug and replug around the > "inode_sleep_on_writeback()" in wb_writeback()? So while the revert > was a no-brainer, this one I really want people to think about. So we talked about this when we were trying to figure out a solution. The problem with this approach is now we have a plug that covers multiple super blocks (__writeback_inodes_wb loops through the sb's starts writeback), which is likely to give us crappier performance than no plug at all. Thanks, Josef -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2015-09-11 23:10 +0200 |
| Message-ID | <q7DK1-4Wl-13@gated-at.bofh.it> |
| In reply to | #1223097 |
On Fri, Sep 11, 2015 at 1:40 PM, Josef Bacik <jbacik@fb.com> wrote:
>
> So we talked about this when we were trying to figure out a solution. The
> problem with this approach is now we have a plug that covers multiple super
> blocks (__writeback_inodes_wb loops through the sb's starts writeback),
> which is likely to give us crappier performance than no plug at all.
Why would that be? Either they are on separate disks, and the IO is
all independent anyway, and at most it got started by some (small)
CPU-amount later. Actual throughput should be the same. No?
Or the filesystems are on the same disk, in which case it should
presumably be a win to submit the IO together.
Of course, actual numbers would be the deciding factor if this really
is noticeable. But "cleaner code and saner locking" is definitely an
issue at least for me.
Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2015-09-12 00:10 +0200 |
| Message-ID | <q7EG6-6jS-7@gated-at.bofh.it> |
| In reply to | #1223101 |
On Fri, Sep 11, 2015 at 2:04 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Of course, actual numbers would be the deciding factor if this really
> is noticeable. But "cleaner code and saner locking" is definitely an
> issue at least for me.
Anyway, I'll hold off pushing out the revert too for a while, in the
hope that we'll have actual numbers for or against whatever the
particular solution should be.
I do get the feeling that the whole wb->list_lock needs more loving.
For example, there's that locked_inode_to_wb_and_lock_list() thing
(which might be better off trying to just do "trylock" on it, but
that's a separate issue) showing lock inversion worries.
Maybe we should *not* get that wb->list_lock early at all, and nest it
inside the inode spinlocks, and just move the list_lock locking down a
lot (ie not even try to hold it over big functions that then are
forced to releasing it anyway).
For example, realistically, it looks like the longest we ever *really*
hold that lock is at the top of the loop of writeback_sb_inodes() -
maybe we could just explicitly have a function that does "find the
first inode that matches this sb and needs writeout activity", and
literally only take hold the lock over that function. And *not* take
the lock in the caller at all?
The callers seem to want that lock mainly because they do that
"list_empty(&wb->b_io)" test. But the way our doubly linked lists
work, "list_empty()" is actually something that can be done racily
without the lock held...
That said, I doubt anybody really wants to touch this, so at least for
now we're stuck with either the "plug outside the lock" or the "drop
and retake lock" options. It really would be loveyl to haev numbers
either way.
<insert pricess leia gif>
"Dave, you're our only hope"
Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Chris Mason <clm@fb.com> |
|---|---|
| Date | 2015-09-12 01:20 +0200 |
| Message-ID | <q7FLQ-7Un-21@gated-at.bofh.it> |
| In reply to | #1223126 |
On Fri, Sep 11, 2015 at 03:06:18PM -0700, Linus Torvalds wrote: > On Fri, Sep 11, 2015 at 2:04 PM, Linus Torvalds > <torvalds@linux-foundation.org> wrote: > > > > Of course, actual numbers would be the deciding factor if this really > > is noticeable. But "cleaner code and saner locking" is definitely an > > issue at least for me. > > Anyway, I'll hold off pushing out the revert too for a while, in the > hope that we'll have actual numbers for or against whatever the > particular solution should be. > > I do get the feeling that the whole wb->list_lock needs more loving. > For example, there's that locked_inode_to_wb_and_lock_list() thing > (which might be better off trying to just do "trylock" on it, but > that's a separate issue) showing lock inversion worries. At the very least, it's kind of sad how many of us were surprised to find the lock held when Dave's patch was unplugging. If this bug slipped through, more are going to. It's also true N-1 of those people were really surprised about scheduling unplug functions, so maybe we can't put all the blame on wb->list_lock. > > Maybe we should *not* get that wb->list_lock early at all, and nest it > inside the inode spinlocks, and just move the list_lock locking down a > lot (ie not even try to hold it over big functions that then are > forced to releasing it anyway). > > For example, realistically, it looks like the longest we ever *really* > hold that lock is at the top of the loop of writeback_sb_inodes() - > maybe we could just explicitly have a function that does "find the > first inode that matches this sb and needs writeout activity", and > literally only take hold the lock over that function. And *not* take > the lock in the caller at all? > > The callers seem to want that lock mainly because they do that > "list_empty(&wb->b_io)" test. But the way our doubly linked lists > work, "list_empty()" is actually something that can be done racily > without the lock held... > > That said, I doubt anybody really wants to touch this, so at least for > now we're stuck with either the "plug outside the lock" or the "drop > and retake lock" options. It really would be loveyl to haev numbers > either way. For 4.3 timeframes, what runs do you want to see numbers for: 1) revert 2) my hack 3) plug over multiple sbs (on different devices) 4) ? -chris -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2015-09-12 01:40 +0200 |
| Message-ID | <q7G5c-8gW-1@gated-at.bofh.it> |
| In reply to | #1223234 |
On Fri, Sep 11, 2015 at 4:16 PM, Chris Mason <clm@fb.com> wrote:
>
> For 4.3 timeframes, what runs do you want to see numbers for:
>
> 1) revert
> 2) my hack
> 3) plug over multiple sbs (on different devices)
> 4) ?
Just 2 or 3.
I don't think the plain revert is all that interesting, and I think
the "anything else" is far too late for this merge window.
So we'll go with either (2) your patch (which I obviously don't
_like_, but apart from the ugliness I don't think there's anything
technically wrong with), or with (3) the "plug across a bigger area".
So the only issue with (3) is whether that's just "revert plus the
patch I sent out", or whether we should unplug/replug over the "wait
synchronously for an inode" case (iow, the
"inode_sleep_on_writeback()"). The existing plug code (that has the
spinlock issue) already has a "wait on inode" case, and did *not*
unplug over that call, but broadening the plugging further now ends up
having two of those "wait synchronosly on inode".
Are we really ok with waiting synchronously for an inode while holding
the plug? No chance of deadlock (waiting for IO that we've plugged)?
That issue is true even of the current code, though, and I have _not_
really thought that through, it's just a worry.
Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2015-09-12 03:00 +0200 |
| Message-ID | <q7HkC-1xG-7@gated-at.bofh.it> |
| In reply to | #1223264 |
On Fri, Sep 11, 2015 at 4:36 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> Are we really ok with waiting synchronously for an inode while holding
> the plug? No chance of deadlock (waiting for IO that we've plugged)?
> That issue is true even of the current code, though, and I have _not_
> really thought that through, it's just a worry.
Never mind. We still flush the plug on explicit scheduling events. I
wonder why I thought we got rid of that. Some kind of "senior moment",
I guess.
Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Chris Mason <clm@fb.com> |
|---|---|
| Date | 2015-09-12 04:20 +0200 |
| Message-ID | <q7IA1-3A6-3@gated-at.bofh.it> |
| In reply to | #1223278 |
On Fri, Sep 11, 2015 at 05:52:27PM -0700, Linus Torvalds wrote: > On Fri, Sep 11, 2015 at 4:36 PM, Linus Torvalds > <torvalds@linux-foundation.org> wrote: > > > > Are we really ok with waiting synchronously for an inode while holding > > the plug? No chance of deadlock (waiting for IO that we've plugged)? > > That issue is true even of the current code, though, and I have _not_ > > really thought that through, it's just a worry. > > Never mind. We still flush the plug on explicit scheduling events. I > wonder why I thought we got rid of that. Some kind of "senior moment", But flushing on schedule is a little different. It ends up calling blk_schedule_flush_plug() which will hand off work to kblockd through blk_run_queue_async() Not a huge deal, but if we're scheduling to wait for that IO, we should really run the plug ourselves so that we're not waiting for kblockd too. -chris -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2015-09-12 04:30 +0200 |
| Message-ID | <q7IJI-3Ls-15@gated-at.bofh.it> |
| In reply to | #1223288 |
On Fri, Sep 11, 2015 at 7:15 PM, Chris Mason <clm@fb.com> wrote:
>
> But flushing on schedule is a little different. It ends up calling
> blk_schedule_flush_plug() which will hand off work to kblockd through
> blk_run_queue_async()
I was more worried about some actual deadlock from the changes. And
blk_schedule_flush_plug() is fine in that it doesn't actually remove
the plug, it just schedules the currently plugged pending IO, so the
IO will start from waiting on the inode, but the plug will still
remain for the rest of the writeback, and it all looks like it should
be fine.
And the reason we use kblockd is simple: stack usage. The reschedule
can happen pretty deep on the stack, we don't actually want to
necessarily then cause much more stack use through things like md/raid
allocating new requests etc.
So it all looks fine to me.
Btw, very tangentially related: grepping a bit shows that
"blk_flush_plug()" isn't actually used anywhere any more. Can we get
rid of that?
Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Chris Mason <clm@fb.com> |
|---|---|
| Date | 2015-09-13 01:10 +0200 |
| Message-ID | <q825H-6zQ-1@gated-at.bofh.it> |
| In reply to | #1223264 |
On Fri, Sep 11, 2015 at 04:36:39PM -0700, Linus Torvalds wrote:
> On Fri, Sep 11, 2015 at 4:16 PM, Chris Mason <clm@fb.com> wrote:
> >
> > For 4.3 timeframes, what runs do you want to see numbers for:
> >
> > 1) revert
> > 2) my hack
> > 3) plug over multiple sbs (on different devices)
> > 4) ?
>
> Just 2 or 3.
>
> I don't think the plain revert is all that interesting, and I think
> the "anything else" is far too late for this merge window.
I did the plain revert as well, just to have a baseline. This box is a
little different from Dave's. Bare metal two socket box (E5-2660 v2 @
2.20Ghz) with 144GB of ram. I have two pcie flash devices, one nvme and
one fusionio, and I put a one FS on each device (two mounts total).
The test created 1.6M files, 4K each. I used Dave's fs_mark command
line, spread out over 16 directories from each mounted filesystem. In
btrfs they are spread over subvolumes to cut down lock contention.
I need to change around the dirty ratios more to smooth out the IO, and
I had trouble with both XFS and btrfs getting runs that were not CPU
bound. I included the time to run sync at the end of the run because
the results were not very consistent without it.
The XFS runs generally had one CPU pegged at 100%, and I think this is
throwing off the results. On Monday I'll redo them with two (four?)
filesystems per flash device, hopefully that'll break things up.
The btrfs runs generally had all the CPUs pegged at 100%. I switched to
mount -o nodatasum and squeezed out an extra 50K files/sec at much lower
CPU utilization.
wall time fs_mark files/sec bytes written/sec
XFS:
baseline v4.2: 5m6s 118,578 272MB/s
Dave's patch: 4m46s 151,421 294MB/s
my hack: 5m5s 150,714 275MB/s
Linus plug: 5m15s 147,735 266MB/s
Btrfs (nodatasum):
baseline v4.2: 4m39s 242,643 313MB/s
Dave's patch: 3m46s 252,452 389MB/s
my hack: 3m48s 257,924 379MB/s
Linus plug: 3m58s 247,528 369MB/s
Bottom line, not as conclusive as I'd like. My hack doesn't seem to
hurt, but FS internals are consuming enough CPU that this lock just
isn't showing up.
Linus' plug patch is consistently slower, and I don't have a great
explanation. My guesses: not keeping the flash pipelines full, or the
imbalance between the different speed flash is averaging the overall
result down, or its my kblockd vs explicit unplug handwaving from
yesterday.
So, next step is either more runs on flash or grab a box with a bunch of
spindles. I'd rather do the spindle runs, I agree with Dave that his
patch should help much more on actual drives.
-chris
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2015-09-13 01:30 +0200 |
| Message-ID | <q82p5-6WW-47@gated-at.bofh.it> |
| In reply to | #1223469 |
On Sat, Sep 12, 2015 at 4:00 PM, Chris Mason <clm@fb.com> wrote:
>
> I did the plain revert as well, just to have a baseline.
Ahh, I ended up not expecting you to get this done until after rc1 was
out, so I in the meantime just merged my fix instead rather than leave
the expected scheduling-while-atomic problem.
And just as well that you did a baseline, since apparently the numbers
are all over the map. I don't see how your hack and dave's original
can _possibly_ differ that much, but they clearly did on your xfs
test. So there's probably huge variance that depends on random
details.
I'll leave things as they are until we have something that looks a bit
more believable ;)
Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Chris Mason <clm@fb.com> |
|---|---|
| Date | 2015-09-13 01:50 +0200 |
| Message-ID | <q82Ip-7jp-7@gated-at.bofh.it> |
| In reply to | #1223522 |
On Sat, Sep 12, 2015 at 04:29:06PM -0700, Linus Torvalds wrote: > On Sat, Sep 12, 2015 at 4:00 PM, Chris Mason <clm@fb.com> wrote: > > > > I did the plain revert as well, just to have a baseline. > > Ahh, I ended up not expecting you to get this done until after rc1 was > out, so I in the meantime just merged my fix instead rather than leave > the expected scheduling-while-atomic problem. Yeah, I wasn't sure I'd be able to do the runs, but it was a rainy afternoon and this was more fun than cleaning. Really glad something got in for rc1 either way. > > And just as well that you did a baseline, since apparently the numbers > are all over the map. I don't see how your hack and dave's original > can _possibly_ differ that much, but they clearly did on your xfs > test. So there's probably huge variance that depends on random > details. I don't think the XFS numbers can be trusted too much since it was basically bottlenecked behind that single pegged CPU. It was bouncing around and I couldn't quite track it down to a process name (or perf profile). The btrfs numbers were much more consistent, but your patch is still a win over plain 4.2. > > I'll leave things as they are until we have something that looks a bit > more believable ;) We can build from here, thanks Linus. -chris -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Chris Mason <clm@fb.com> |
|---|---|
| Date | 2015-09-13 15:20 +0200 |
| Message-ID | <q8fmi-ga-13@gated-at.bofh.it> |
| In reply to | #1223534 |
On Sat, Sep 12, 2015 at 07:46:32PM -0400, Chris Mason wrote:
> I don't think the XFS numbers can be trusted too much since it was
> basically bottlenecked behind that single pegged CPU. It was bouncing
> around and I couldn't quite track it down to a process name (or perf
> profile).
I'll do more runs Monday, but I was able to grab a perf profile of the
pegged XFS CPU. It was just the writeback worker thread, and it
hit btrfs differently because we defer more of this stuff to endio
workers, effectively spreading it out over more CPUs.
With 4 mount points intead of 2, XFS goes from 140K files/sec to 250K.
Here's one of the profiles, but it bounced around a lot so I wouldn't
use this to actually tune anything:
11.42% kworker/u82:61 [kernel.kallsyms] [k] _raw_spin_lock
|
---_raw_spin_lock
|
|--83.43%-- xfs_extent_busy_trim
| xfs_alloc_compute_aligned
| |
| |--99.92%-- xfs_alloc_ag_vextent_near
| | xfs_alloc_ag_vextent
| | xfs_alloc_vextent
| | xfs_bmap_btalloc
| | xfs_bmap_alloc
| | xfs_bmapi_write
| | xfs_iomap_write_allocate
| | xfs_map_blocks
| | xfs_vm_writepage
| | __writepage
| | write_cache_pages
| | generic_writepages
| | xfs_vm_writepages
| | do_writepages
| | __writeback_single_inode
| | writeback_sb_inodes
| | __writeback_inodes_wb
| | wb_writeback
| | wb_do_writeback
| | wb_workfn
| | process_one_work
| | worker_thread
| | kthread
| | ret_from_fork
| --0.08%-- [...]
|
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Dave Chinner <david@fromorbit.com> |
|---|---|
| Date | 2015-09-14 01:00 +0200 |
| Message-ID | <q8opA-4ES-9@gated-at.bofh.it> |
| In reply to | #1223670 |
On Sun, Sep 13, 2015 at 09:12:44AM -0400, Chris Mason wrote: > On Sat, Sep 12, 2015 at 07:46:32PM -0400, Chris Mason wrote: > > I don't think the XFS numbers can be trusted too much since it was > > basically bottlenecked behind that single pegged CPU. It was bouncing > > around and I couldn't quite track it down to a process name (or perf > > profile). > > I'll do more runs Monday, but I was able to grab a perf profile of the > pegged XFS CPU. It was just the writeback worker thread, and it > hit btrfs differently because we defer more of this stuff to endio > workers, effectively spreading it out over more CPUs. > > With 4 mount points intead of 2, XFS goes from 140K files/sec to 250K. > Here's one of the profiles, but it bounced around a lot so I wouldn't > use this to actually tune anything: mkfs.xfs -d agcount=64 .... Cheers, Dave. -- Dave Chinner david@fromorbit.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Dave Chinner <david@fromorbit.com> |
|---|---|
| Date | 2015-09-14 01:20 +0200 |
| Message-ID | <q8oIW-5gF-15@gated-at.bofh.it> |
| In reply to | #1223469 |
On Sat, Sep 12, 2015 at 07:00:27PM -0400, Chris Mason wrote: > On Fri, Sep 11, 2015 at 04:36:39PM -0700, Linus Torvalds wrote: > > On Fri, Sep 11, 2015 at 4:16 PM, Chris Mason <clm@fb.com> wrote: > > > > > > For 4.3 timeframes, what runs do you want to see numbers for: > > > > > > 1) revert > > > 2) my hack > > > 3) plug over multiple sbs (on different devices) > > > 4) ? > > > > Just 2 or 3. > > > > I don't think the plain revert is all that interesting, and I think > > the "anything else" is far too late for this merge window. > > I did the plain revert as well, just to have a baseline. This box is a > little different from Dave's. Bare metal two socket box (E5-2660 v2 @ > 2.20Ghz) with 144GB of ram. I have two pcie flash devices, one nvme and > one fusionio, and I put a one FS on each device (two mounts total). > > The test created 1.6M files, 4K each. I used Dave's fs_mark command > line, spread out over 16 directories from each mounted filesystem. In > btrfs they are spread over subvolumes to cut down lock contention. > > I need to change around the dirty ratios more to smooth out the IO, and > I had trouble with both XFS and btrfs getting runs that were not CPU > bound. I included the time to run sync at the end of the run because > the results were not very consistent without it. > > The XFS runs generally had one CPU pegged at 100%, and I think this is > throwing off the results. On Monday I'll redo them with two (four?) > filesystems per flash device, hopefully that'll break things up. > > The btrfs runs generally had all the CPUs pegged at 100%. I switched to > mount -o nodatasum and squeezed out an extra 50K files/sec at much lower > CPU utilization. > > wall time fs_mark files/sec bytes written/sec > > XFS: > baseline v4.2: 5m6s 118,578 272MB/s > Dave's patch: 4m46s 151,421 294MB/s > my hack: 5m5s 150,714 275MB/s > Linus plug: 5m15s 147,735 266MB/s > > > Btrfs (nodatasum): > baseline v4.2: 4m39s 242,643 313MB/s > Dave's patch: 3m46s 252,452 389MB/s > my hack: 3m48s 257,924 379MB/s > Linus plug: 3m58s 247,528 369MB/s Really need to run these numbers on slower disks where block layer merging makes a difference to performance. The high level plugging improves performance on spinning disks by a huge amount on XFS because the merging reduces the number of IOs issued to disk by 2 orders of magnitude. Plugging makes comparitively little difference on devices that can sustain extremely high IOPS and hence sink the tens to hundreds of thousand individual 4k IOs that this workload generates through writeback. i.e. while throughput increases, that's not the numbers that matters here - it's the change in write IO behaviour that needs to be categorised and measured by the different patches... (I'm on holidays, so I'm not going to get to this any time soon) Cheers, Dave. -- Dave Chinner david@fromorbit.com -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Linus Torvalds <torvalds@linux-foundation.org> |
|---|---|
| Date | 2015-09-14 22:10 +0200 |
| Message-ID | <q8IeC-89j-15@gated-at.bofh.it> |
| In reply to | #1223763 |
On Sun, Sep 13, 2015 at 4:12 PM, Dave Chinner <david@fromorbit.com> wrote:
>
> Really need to run these numbers on slower disks where block layer
> merging makes a difference to performance.
Yeah. We've seen plugging and io schedulers not make much difference
for high-performance flash (although I think the people who argued
that noop should generally be used for non-rotating media were wrong,
I think - the elevator ends up still being critical to merging, and
while merging isn't a life-or-death situation, it tends to still
help).
For rotating rust with nasty seek times, the plugging is likely to
make the biggest difference.
Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Jan Kara <jack@suse.cz> |
|---|---|
| Date | 2015-09-16 22:00 +0200 |
| Message-ID | <q9r22-5SU-3@gated-at.bofh.it> |
| In reply to | #1224434 |
On Wed 16-09-15 11:16:21, Chris Mason wrote: > On Mon, Sep 14, 2015 at 01:06:25PM -0700, Linus Torvalds wrote: > > On Sun, Sep 13, 2015 at 4:12 PM, Dave Chinner <david@fromorbit.com> wrote: > > > > > > Really need to run these numbers on slower disks where block layer > > > merging makes a difference to performance. > > > > Yeah. We've seen plugging and io schedulers not make much difference > > for high-performance flash (although I think the people who argued > > that noop should generally be used for non-rotating media were wrong, > > I think - the elevator ends up still being critical to merging, and > > while merging isn't a life-or-death situation, it tends to still > > help). > > > Yeah, my big concern was that holding the plug longer would result in > lower overall perf because we weren't keeping the flash busy. So I > started with the flash boxes to make sure we weren't regressing past 4.2 > levels at least. > > I'm still worried about that, but this probably isn't the right > benchmark to show it. And if it's really a problem, it'll happen > everywhere we plug and not just here. > > > > > For rotating rust with nasty seek times, the plugging is likely to > > make the biggest difference. > > For rotating storage, I grabbed a big box and did the fs_mark run > against 8 spindles. These are all behind a megaraid card as jbods, so I > flipped the card's cache to write-through. > > I changed around the run a bit, making enough files for fs_mark to run > for ~10 minutes, and I took out the sync. I ran only xfs to cut down on > the iterations, and after the fs_mark run, I did short 30 second run with > blktrace in the background to capture the io sizes. > > v4.2: 178K files/sec > Chinner: 192K files/sec > Mason: 192K files/sec > Linus: 193K files/sec > > I added support to iowatcher to graph IO size, and attached the graph. > > Short version, Linus' patch still gives bigger IOs and similar perf to > Dave's original. I should have done the blktrace runs for 60 seconds > instead of 30, I suspect that would even out the average sizes between > the three patches. Thanks for the data Chris. So I guess we are fine with what's currently in, right? Honza -- Jan Kara <jack@suse.com> SUSE Labs, CR -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
| From | Chris Mason <clm@fb.com> |
|---|---|
| Date | 2015-09-16 22:10 +0200 |
| Message-ID | <q9rbI-6jD-17@gated-at.bofh.it> |
| In reply to | #1226430 |
On Wed, Sep 16, 2015 at 09:58:06PM +0200, Jan Kara wrote: > On Wed 16-09-15 11:16:21, Chris Mason wrote: > > Short version, Linus' patch still gives bigger IOs and similar perf to > > Dave's original. I should have done the blktrace runs for 60 seconds > > instead of 30, I suspect that would even out the average sizes between > > the three patches. > > Thanks for the data Chris. So I guess we are fine with what's currently in, > right? Looks like it works well to me. -chris -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
[toc] | [prev] | [next] | [standalone]
Page 1 of 3 [1] 2 3 Next page →
Back to top | Article view | linux.kernel
csiph-web