mbox series

[v2,0/2] Btrfs: workqueue cleanups

Message ID cover.1565717247.git.osandov@fb.com (mailing list archive)
Headers show
Series Btrfs: workqueue cleanups | expand

Message

Omar Sandoval Aug. 13, 2019, 5:33 p.m. UTC
From: Omar Sandoval <osandov@fb.com>

This does some cleanups to the Btrfs workqueue code following my
previous fix [1]. Changed since v1 [2]:

- Removed errant Fixes: tag in patch 1
- Fixed a comment typo in patch 2
- Added NB: to comments in patch 2
- Added Nikolay and Filipe's reviewed-by tags

Thanks!

1: https://lore.kernel.org/linux-btrfs/0bea516a54b26e4e1c42e6fe47548cb48cc4172b.1565112813.git.osandov@fb.com/
2: https://lore.kernel.org/linux-btrfs/cover.1565680721.git.osandov@fb.com/

Omar Sandoval (2):
  Btrfs: get rid of unique workqueue helper functions
  Btrfs: get rid of pointless wtag variable in async-thread.c

 fs/btrfs/async-thread.c      | 79 ++++++++++--------------------------
 fs/btrfs/async-thread.h      | 33 +--------------
 fs/btrfs/block-group.c       |  3 +-
 fs/btrfs/delayed-inode.c     |  4 +-
 fs/btrfs/disk-io.c           | 34 +++++-----------
 fs/btrfs/inode.c             | 36 +++++-----------
 fs/btrfs/ordered-data.c      |  1 -
 fs/btrfs/qgroup.c            |  1 -
 fs/btrfs/raid56.c            |  5 +--
 fs/btrfs/reada.c             |  3 +-
 fs/btrfs/scrub.c             | 14 +++----
 fs/btrfs/volumes.c           |  3 +-
 include/trace/events/btrfs.h |  6 +--
 13 files changed, 61 insertions(+), 161 deletions(-)

Comments

David Sterba Aug. 19, 2019, 5:28 p.m. UTC | #1
On Tue, Aug 13, 2019 at 10:33:42AM -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> This does some cleanups to the Btrfs workqueue code following my
> previous fix [1]. Changed since v1 [2]:
> 
> - Removed errant Fixes: tag in patch 1
> - Fixed a comment typo in patch 2
> - Added NB: to comments in patch 2
> - Added Nikolay and Filipe's reviewed-by tags

Added to misc-next, thanks.
David Sterba Aug. 21, 2019, 1:20 p.m. UTC | #2
On Tue, Aug 13, 2019 at 10:33:42AM -0700, Omar Sandoval wrote:
> From: Omar Sandoval <osandov@fb.com>
> 
> This does some cleanups to the Btrfs workqueue code following my
> previous fix [1]. Changed since v1 [2]:
> 
> - Removed errant Fixes: tag in patch 1
> - Fixed a comment typo in patch 2
> - Added NB: to comments in patch 2
> - Added Nikolay and Filipe's reviewed-by tags
> 
> Thanks!
> 
> 1: https://lore.kernel.org/linux-btrfs/0bea516a54b26e4e1c42e6fe47548cb48cc4172b.1565112813.git.osandov@fb.com/
> 2: https://lore.kernel.org/linux-btrfs/cover.1565680721.git.osandov@fb.com/
> 
> Omar Sandoval (2):
>   Btrfs: get rid of unique workqueue helper functions
>   Btrfs: get rid of pointless wtag variable in async-thread.c

The patches seem to cause crashes inside the worques, happend several
times in random patches, sample stacktrace below. This blocks me from
testing so I'll move the patches out of misc-next for now and add back
once there's a fix.

btrfs/004               [18:00:37][   70.500719] run fstests btrfs/004 at 2019-08-20 18:00:37
[   70.866378] BTRFS info (device vda): disk space caching is enabled
[   70.868960] BTRFS info (device vda): has skinny extents
[   71.046922] BTRFS: device fsid e4a337ee-c8db-4be4-ac0f-6cd899f74fa8 devid 1 transid 5 /dev/vdb
[   71.065084] BTRFS info (device vdb): turning on discard
[   71.066812] BTRFS info (device vdb): disk space caching is enabled
[   71.068907] BTRFS info (device vdb): has skinny extents
[   71.070589] BTRFS info (device vdb): flagging fs with big metadata feature
[   71.078315] BTRFS info (device vdb): checking UUID tree
[   74.627999] BTRFS info (device vdb): turning on discard
[   74.630041] BTRFS info (device vdb): setting incompat feature flag for COMPRESS_LZO (0x8)
[   74.632869] BTRFS info (device vdb): use lzo compression, level 0
[   74.634472] BTRFS info (device vdb): disk space caching is enabled
[   74.636137] BTRFS info (device vdb): has skinny extents
[   75.038059] fsstress (21349) used greatest stack depth: 10912 bytes left
[   79.343639] BTRFS info (device vdb): turning on discard
[   79.345268] BTRFS info (device vdb): disk space caching is enabled
[   79.346866] BTRFS info (device vdb): has skinny extents
[   90.608845] general protection fault: 0000 [#1] SMP
[   90.611148] CPU: 2 PID: 21318 Comm: kworker/u8:8 Not tainted 5.3.0-rc5-default+ #699
[   90.613973] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.12.1-0-ga5cab58-prebuilt.qemu.org 04/01/2014
[   90.617179] Workqueue: btrfs-worker btrfs_work_helper [btrfs]
[   90.618740] RIP: 0010:__lock_acquire+0x815/0x1100
[   90.624107] RSP: 0018:ffffa5cb05f23cc0 EFLAGS: 00010002
[   90.625652] RAX: 0000000000000000 RBX: 0000000000000086 RCX: 0000000000000000
[   90.627804] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 6b6b6b6b6b6b6ba3
[   90.629171] RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000001
[   90.631566] R10: 6b6b6b6b6b6b6ba3 R11: 0000000000000000 R12: 0000000000000001
[   90.632952] R13: 0000000000000000 R14: 0000000000000000 R15: ffffa0f49efed340
[   90.634019] FS:  0000000000000000(0000) GS:ffffa0f4bd800000(0000) knlGS:0000000000000000
[   90.635313] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   90.636293] CR2: 0000555f9a8d7538 CR3: 0000000072ac8000 CR4: 00000000000006e0
[   90.637987] Call Trace:
[   90.638620]  ? lockdep_hardirqs_on+0x103/0x190
[   90.639745]  ? trace_hardirqs_on_thunk+0x1a/0x20
[   90.640864]  lock_acquire+0x95/0x1a0
[   90.641783]  ? btrfs_work_helper+0x1ba/0x600 [btrfs]
[   90.642883]  _raw_spin_lock_irqsave+0x3c/0x80
[   90.643924]  ? btrfs_work_helper+0x1ba/0x600 [btrfs]
[   90.645129]  btrfs_work_helper+0x1ba/0x600 [btrfs]
[   90.646327]  process_one_work+0x22f/0x5b0
[   90.647325]  worker_thread+0x50/0x3b0
[   90.648228]  ? process_one_work+0x5b0/0x5b0
[   90.649243]  kthread+0x11a/0x130
[   90.649825]  ? kthread_create_worker_on_cpu+0x70/0x70
[   90.650633]  ret_from_fork+0x24/0x30
[   90.651256] Modules linked in: btrfs libcrc32c xor zstd_decompress zstd_compress xxhash raid6_pq loop
[   90.653126] ---[ end trace 5a9019a191ef1b98 ]---
[   90.654215] RIP: 0010:__lock_acquire+0x815/0x1100
[   90.658563] RSP: 0018:ffffa5cb05f23cc0 EFLAGS: 00010002
[   90.659392] RAX: 0000000000000000 RBX: 0000000000000086 RCX: 0000000000000000
[   90.660937] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 6b6b6b6b6b6b6ba3
[   90.662457] RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000001
[   90.663524] R10: 6b6b6b6b6b6b6ba3 R11: 0000000000000000 R12: 0000000000000001
[   90.665197] R13: 0000000000000000 R14: 0000000000000000 R15: ffffa0f49efed340
[   90.667127] FS:  0000000000000000(0000) GS:ffffa0f4bd800000(0000) knlGS:0000000000000000
[   90.669365] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[   90.670966] CR2: 0000555f9a8d7538 CR3: 0000000072ac8000 CR4: 00000000000006e0
[   90.672893] BUG: sleeping function called from invalid context at include/linux/cgroup-defs.h:760
[   90.675438] in_atomic(): 1, irqs_disabled(): 1, pid: 21318, name: kworker/u8:8
[   90.677531] INFO: lockdep is turned off.
[   90.678757] irq event stamp: 94362
[   90.679645] hardirqs last  enabled at (94361): [<ffffffff8e0029da>] trace_hardirqs_on_thunk+0x1a/0x20
[   90.681805] hardirqs last disabled at (94362): [<ffffffff8e73f8d6>] _raw_spin_lock_irqsave+0x16/0x80
[   90.683239] softirqs last  enabled at (94360): [<ffffffff8ea00358>] __do_softirq+0x358/0x52b
[   90.684920] softirqs last disabled at (94079): [<ffffffff8e088d6d>] irq_exit+0x9d/0xb0
[   90.686500] CPU: 2 PID: 21318 Comm: kworker/u8:8 Tainted: G      D           5.3.0-rc5-default+ #699
[   90.687962] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.12.1-0-ga5cab58-prebuilt.qemu.org 04/01/2014
[   90.690382] Workqueue: btrfs-worker btrfs_work_helper [btrfs]
[   90.691953] Call Trace:
[   90.692769]  dump_stack+0x67/0x90
[   90.693816]  ___might_sleep.cold+0x9f/0xf2
[   90.695033]  exit_signals+0x30/0x110
[   90.696030]  do_exit+0xb7/0x920
[   90.696902]  ? kthread+0x11a/0x130
[   90.697799]  rewind_stack_do_exit+0x17/0x20
[   90.699258] note: kworker/u8:8[21318] exited with preempt_count 1
David Sterba Aug. 21, 2019, 1:24 p.m. UTC | #3
On Wed, Aug 21, 2019 at 03:20:21PM +0200, David Sterba wrote:
> On Tue, Aug 13, 2019 at 10:33:42AM -0700, Omar Sandoval wrote:
> > From: Omar Sandoval <osandov@fb.com>
> > 
> > This does some cleanups to the Btrfs workqueue code following my
> > previous fix [1]. Changed since v1 [2]:
> > 
> > - Removed errant Fixes: tag in patch 1
> > - Fixed a comment typo in patch 2
> > - Added NB: to comments in patch 2
> > - Added Nikolay and Filipe's reviewed-by tags
> > 
> > Thanks!
> > 
> > 1: https://lore.kernel.org/linux-btrfs/0bea516a54b26e4e1c42e6fe47548cb48cc4172b.1565112813.git.osandov@fb.com/
> > 2: https://lore.kernel.org/linux-btrfs/cover.1565680721.git.osandov@fb.com/
> > 
> > Omar Sandoval (2):
> >   Btrfs: get rid of unique workqueue helper functions
> >   Btrfs: get rid of pointless wtag variable in async-thread.c
> 
> The patches seem to cause crashes inside the worques, happend several
> times in random patches, sample stacktrace below. This blocks me from
> testing so I'll move the patches out of misc-next for now and add back
> once there's a fix.

Another possibility is that the cleanup patches make it more likely to
happen and the root cause is "Btrfs: fix workqueue deadlock on dependent
filesystems".
David Sterba Aug. 21, 2019, 2:14 p.m. UTC | #4
On Wed, Aug 21, 2019 at 03:24:46PM +0200, David Sterba wrote:
> On Wed, Aug 21, 2019 at 03:20:21PM +0200, David Sterba wrote:
> > On Tue, Aug 13, 2019 at 10:33:42AM -0700, Omar Sandoval wrote:
> > > From: Omar Sandoval <osandov@fb.com>
> > > 
> > > This does some cleanups to the Btrfs workqueue code following my
> > > previous fix [1]. Changed since v1 [2]:
> > > 
> > > - Removed errant Fixes: tag in patch 1
> > > - Fixed a comment typo in patch 2
> > > - Added NB: to comments in patch 2
> > > - Added Nikolay and Filipe's reviewed-by tags
> > > 
> > > Thanks!
> > > 
> > > 1: https://lore.kernel.org/linux-btrfs/0bea516a54b26e4e1c42e6fe47548cb48cc4172b.1565112813.git.osandov@fb.com/
> > > 2: https://lore.kernel.org/linux-btrfs/cover.1565680721.git.osandov@fb.com/
> > > 
> > > Omar Sandoval (2):
> > >   Btrfs: get rid of unique workqueue helper functions
> > >   Btrfs: get rid of pointless wtag variable in async-thread.c
> > 
> > The patches seem to cause crashes inside the worques, happend several
> > times in random patches, sample stacktrace below. This blocks me from
> > testing so I'll move the patches out of misc-next for now and add back
> > once there's a fix.
> 
> Another possibility is that the cleanup patches make it more likely to
> happen and the root cause is "Btrfs: fix workqueue deadlock on dependent
> filesystems".

With just the deadlock fix on top<F2>, crashed in btrfs/011;

[ 2847.115355] general protection fault: 0000 [#1] SMP
[ 2847.120304] CPU: 0 PID: 10594 Comm: kworker/u8:3 Not tainted 5.3.0-rc5-default+ #699
[ 2847.124830] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.12.1-0-ga5cab58-prebuilt.qemu.org 04/01/2014
[ 2847.128328] Workqueue: btrfs-worker btrfs_worker_helper [btrfs]
[ 2847.130135] RIP: 0010:__lock_acquire+0x815/0x1100
[ 2847.136626] RSP: 0018:ffffb3e8c3dbfcd0 EFLAGS: 00010002
[ 2847.137627] RAX: 0000000000000000 RBX: 0000000000000086 RCX: 0000000000000000
[ 2847.138690] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 6b6b6b6b6b6b6ba3
[ 2847.139989] RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000001
[ 2847.141055] R10: 6b6b6b6b6b6b6ba3 R11: 0000000000000000 R12: 0000000000000001
[ 2847.142113] R13: 0000000000000000 R14: 0000000000000000 R15: ffffa26aa1ab29c0
[ 2847.143251] FS:  0000000000000000(0000) GS:ffffa26abd400000(0000) knlGS:0000000000000000
[ 2847.145539] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 2847.147154] CR2: 00007f70c6c2977c CR3: 0000000031011000 CR4: 00000000000006f0
[ 2847.149046] Call Trace:
[ 2847.149683]  ? lockdep_hardirqs_on+0x103/0x190
[ 2847.150482]  ? trace_hardirqs_on_thunk+0x1a/0x20
[ 2847.151355]  lock_acquire+0x95/0x1a0
[ 2847.152205]  ? normal_work_helper+0x264/0x5f0 [btrfs]
[ 2847.153035]  _raw_spin_lock_irqsave+0x3c/0x80
[ 2847.153774]  ? normal_work_helper+0x264/0x5f0 [btrfs]
[ 2847.154597]  normal_work_helper+0x264/0x5f0 [btrfs]
[ 2847.155609]  process_one_work+0x22f/0x5b0
[ 2847.156487]  worker_thread+0x50/0x3b0
[ 2847.157275]  ? process_one_work+0x5b0/0x5b0
[ 2847.158356]  kthread+0x11a/0x130
[ 2847.159035]  ? kthread_create_worker_on_cpu+0x70/0x70
[ 2847.160186]  ret_from_fork+0x24/0x30
[ 2847.160828] Modules linked in: btrfs libcrc32c xor zstd_decompress zstd_compress xxhash raid6_pq loop
[ 2847.162540] ---[ end trace 4b931bab091a0069 ]---
[ 2847.163650] RIP: 0010:__lock_acquire+0x815/0x1100
[ 2847.168293] RSP: 0018:ffffb3e8c3dbfcd0 EFLAGS: 00010002
[ 2847.169286] RAX: 0000000000000000 RBX: 0000000000000086 RCX: 0000000000000000
[ 2847.170348] RDX: 0000000000000000 RSI: 0000000000000000 RDI: 6b6b6b6b6b6b6ba3
[ 2847.171456] RBP: 0000000000000000 R08: 0000000000000001 R09: 0000000000000001
[ 2847.172874] R10: 6b6b6b6b6b6b6ba3 R11: 0000000000000000 R12: 0000000000000001
[ 2847.173937] R13: 0000000000000000 R14: 0000000000000000 R15: ffffa26aa1ab29c0
[ 2847.175727] FS:  0000000000000000(0000) GS:ffffa26abd400000(0000) knlGS:0000000000000000
[ 2847.177985] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
[ 2847.179435] CR2: 00007f70c6c2977c CR3: 0000000031011000 CR4: 00000000000006f0
[ 2847.180657] BUG: sleeping function called from invalid context at include/linux/cgroup-defs.h:760
[ 2847.182056] in_atomic(): 1, irqs_disabled(): 1, pid: 10594, name: kworker/u8:3
[ 2847.183465] INFO: lockdep is turned off.
[ 2847.185401] irq event stamp: 37147954
[ 2847.187257] hardirqs last  enabled at (37147953): [<ffffffffa00029da>] trace_hardirqs_on_thunk+0x1a/0x20
[ 2847.190661] hardirqs last disabled at (37147954): [<ffffffffa073f8d6>] _raw_spin_lock_irqsave+0x16/0x80
[ 2847.193732] softirqs last  enabled at (37147952): [<ffffffffa0a00358>] __do_softirq+0x358/0x52b
[ 2847.196321] softirqs last disabled at (37147905): [<ffffffffa0088d6d>] irq_exit+0x9d/0xb0
[ 2847.198554] CPU: 0 PID: 10594 Comm: kworker/u8:3 Tainted: G      D           5.3.0-rc5-default+ #699
[ 2847.201083] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS rel-1.12.1-0-ga5cab58-prebuilt.qemu.org 04/01/2014
[ 2847.203894] Workqueue: btrfs-worker btrfs_worker_helper [btrfs]
[ 2847.205305] Call Trace:
[ 2847.206010]  dump_stack+0x67/0x90
[ 2847.206943]  ___might_sleep.cold+0x9f/0xf2
[ 2847.208080]  exit_signals+0x30/0x110
[ 2847.209173]  do_exit+0xb7/0x920
[ 2847.210176]  ? kthread+0x11a/0x130
[ 2847.211261]  rewind_stack_do_exit+0x17/0x20
[ 2847.212571] note: kworker/u8:3[10594] exited with preempt_count 1
Josef Bacik Aug. 21, 2019, 3:18 p.m. UTC | #5
On Wed, Aug 21, 2019 at 04:14:53PM +0200, David Sterba wrote:
> On Wed, Aug 21, 2019 at 03:24:46PM +0200, David Sterba wrote:
> > On Wed, Aug 21, 2019 at 03:20:21PM +0200, David Sterba wrote:
> > > On Tue, Aug 13, 2019 at 10:33:42AM -0700, Omar Sandoval wrote:
> > > > From: Omar Sandoval <osandov@fb.com>
> > > > 
> > > > This does some cleanups to the Btrfs workqueue code following my
> > > > previous fix [1]. Changed since v1 [2]:
> > > > 
> > > > - Removed errant Fixes: tag in patch 1
> > > > - Fixed a comment typo in patch 2
> > > > - Added NB: to comments in patch 2
> > > > - Added Nikolay and Filipe's reviewed-by tags
> > > > 
> > > > Thanks!
> > > > 
> > > > 1: https://lore.kernel.org/linux-btrfs/0bea516a54b26e4e1c42e6fe47548cb48cc4172b.1565112813.git.osandov@fb.com/
> > > > 2: https://lore.kernel.org/linux-btrfs/cover.1565680721.git.osandov@fb.com/
> > > > 
> > > > Omar Sandoval (2):
> > > >   Btrfs: get rid of unique workqueue helper functions
> > > >   Btrfs: get rid of pointless wtag variable in async-thread.c
> > > 
> > > The patches seem to cause crashes inside the worques, happend several
> > > times in random patches, sample stacktrace below. This blocks me from
> > > testing so I'll move the patches out of misc-next for now and add back
> > > once there's a fix.
> > 
> > Another possibility is that the cleanup patches make it more likely to
> > happen and the root cause is "Btrfs: fix workqueue deadlock on dependent
> > filesystems".
> 
> With just the deadlock fix on top<F2>, crashed in btrfs/011;
> 

It's because we're doing

wq = work->wq;

after we've done set_bit(WORK_DONE_BIT, &work->flags).  The work could be freed
immediately after this and thus doing work->wq could give you garbage.  Could
you try with this diff applied and verify it goes away?

diff --git a/fs/btrfs/async-thread.c b/fs/btrfs/async-thread.c
index 6b8ad4a1b568..10a04b99798a 100644
--- a/fs/btrfs/async-thread.c
+++ b/fs/btrfs/async-thread.c
@@ -252,9 +252,9 @@ static inline void thresh_exec_hook(struct __btrfs_workqueue *wq)
 	}
 }
 
-static void run_ordered_work(struct btrfs_work *self)
+static void run_ordered_work(struct __btrfs_workqueue *wq,
+			     struct btrfs_work *self)
 {
-	struct __btrfs_workqueue *wq = self->wq;
 	struct list_head *list = &wq->ordered_list;
 	struct btrfs_work *work;
 	spinlock_t *lock = &wq->list_lock;
@@ -356,7 +356,7 @@ static void normal_work_helper(struct btrfs_work *work)
 	work->func(work);
 	if (need_order) {
 		set_bit(WORK_DONE_BIT, &work->flags);
-		run_ordered_work(work);
+		run_ordered_work(wq, work);
 	}
 	if (!need_order)
 		trace_btrfs_all_work_done(wq->fs_info, wtag);