From: Mike Galbraith <hidden> Date: 2012-07-12 08:44:08
On Thu, 2012-07-12 at 07:47 +0200, Mike Galbraith wrote:
Greetings,
I'm chasing btrfs critters in an enterprise 3.0-rt kernel, and just
checked to see if they're alive in virgin latest/greatest rt kernel.
Both are indeed alive and well, ie I didn't break it, nor did the
zillion patches in enterprise base kernel, so others may have an
opportunity to meet these critters up close and personal as well.
3.2-rt both explodes and deadlocks as well. 3.0-rt (virgin I mean) does
neither, so with enough re-integrate investment, it might be bisectable.
Rummaging in btrfs, that begins to look down right attractive ;-)
-Mike
From: Mike Galbraith <hidden> Date: 2012-07-12 09:53:27
On Thu, 2012-07-12 at 10:44 +0200, Mike Galbraith wrote:
On Thu, 2012-07-12 at 07:47 +0200, Mike Galbraith wrote:
quoted
Greetings,
I'm chasing btrfs critters in an enterprise 3.0-rt kernel, and just
checked to see if they're alive in virgin latest/greatest rt kernel.
Both are indeed alive and well, ie I didn't break it, nor did the
zillion patches in enterprise base kernel, so others may have an
opportunity to meet these critters up close and personal as well.
3.2-rt both explodes and deadlocks as well. 3.0-rt (virgin I mean) does
neither, so with enough re-integrate investment, it might be bisectable.
Nope, virgin 3.0-rt just didn't feel like it at the time. Booted it
again to run hefty test over lunch, it didn't survive 1 xfstests 006,
much less hundreds.
crash> bt
PID: 7604 TASK: ffff880174238b20 CPU: 0 COMMAND: "btrfs-worker-0"
#0 [ffff88017455d9c8] machine_kexec at ffffffff81025794
#1 [ffff88017455da28] crash_kexec at ffffffff8109781d
#2 [ffff88017455daf8] panic at ffffffff814a0661
#3 [ffff88017455db78] __try_to_take_rt_mutex at ffffffff81086d2f
#4 [ffff88017455dbc8] rt_spin_lock_slowlock at ffffffff814a2670
#5 [ffff88017455dca8] rt_spin_lock at ffffffff814a2db9
#6 [ffff88017455dcb8] schedule_bio at ffffffff81243133
#7 [ffff88017455dcf8] btrfs_map_bio at ffffffff812477be
#8 [ffff88017455dd68] __btree_submit_bio_done at ffffffff812152f6
#9 [ffff88017455dd78] run_one_async_done at ffffffff812148fa
#10 [ffff88017455dd98] run_ordered_completions at ffffffff812493e8
#11 [ffff88017455ddd8] worker_loop at ffffffff81249dc9
#12 [ffff88017455de88] kthread at ffffffff81070266
#13 [ffff88017455df48] kernel_thread_helper at ffffffff814a9be4
crash> struct rt_mutex 0xffff880174530108
struct rt_mutex {
wait_lock = {
raw_lock = {
slock = 7966
}
},
wait_list = {
node_list = {
next = 0xffff880175ecc970,
prev = 0xffff880175ecc970
},
rawlock = 0xffff880175ecc968,
spinlock = 0x0
},
owner = 0x1,
save_state = 0,
file = 0x0,
name = 0xffffffff81763d02 "&(&device->io_lock)->lock",
line = 0,
magic = 0x0
}
From: Thomas Gleixner <hidden> Date: 2012-07-12 11:43:28
On Thu, 12 Jul 2012, Mike Galbraith wrote:
On Thu, 2012-07-12 at 10:44 +0200, Mike Galbraith wrote:
quoted
On Thu, 2012-07-12 at 07:47 +0200, Mike Galbraith wrote:
quoted
Greetings,
I'm chasing btrfs critters in an enterprise 3.0-rt kernel, and just
checked to see if they're alive in virgin latest/greatest rt kernel.
Both are indeed alive and well, ie I didn't break it, nor did the
zillion patches in enterprise base kernel, so others may have an
opportunity to meet these critters up close and personal as well.
3.2-rt both explodes and deadlocks as well. 3.0-rt (virgin I mean) does
neither, so with enough re-integrate investment, it might be bisectable.
Nope, virgin 3.0-rt just didn't feel like it at the time. Booted it
again to run hefty test over lunch, it didn't survive 1 xfstests 006,
much less hundreds.
crash> bt
PID: 7604 TASK: ffff880174238b20 CPU: 0 COMMAND: "btrfs-worker-0"
#0 [ffff88017455d9c8] machine_kexec at ffffffff81025794
#1 [ffff88017455da28] crash_kexec at ffffffff8109781d
#2 [ffff88017455daf8] panic at ffffffff814a0661
#3 [ffff88017455db78] __try_to_take_rt_mutex at ffffffff81086d2f
#4 [ffff88017455dbc8] rt_spin_lock_slowlock at ffffffff814a2670
#5 [ffff88017455dca8] rt_spin_lock at ffffffff814a2db9
#6 [ffff88017455dcb8] schedule_bio at ffffffff81243133
#7 [ffff88017455dcf8] btrfs_map_bio at ffffffff812477be
#8 [ffff88017455dd68] __btree_submit_bio_done at ffffffff812152f6
#9 [ffff88017455dd78] run_one_async_done at ffffffff812148fa
#10 [ffff88017455dd98] run_ordered_completions at ffffffff812493e8
#11 [ffff88017455ddd8] worker_loop at ffffffff81249dc9
#12 [ffff88017455de88] kthread at ffffffff81070266
#13 [ffff88017455df48] kernel_thread_helper at ffffffff814a9be4
crash> struct rt_mutex 0xffff880174530108
struct rt_mutex {
wait_lock = {
raw_lock = {
slock = 7966
}
},
wait_list = {
node_list = {
next = 0xffff880175ecc970,
prev = 0xffff880175ecc970
},
rawlock = 0xffff880175ecc968,
Pointer into lala land again.
rawlock points to ...968 and the node_list to ...970.
struct rt_mutex {
raw_spinlock_t wait_lock;
struct plist_head wait_list;
The raw_lock pointer of the plist_head is initialized in
__rt_mutex_init() so it points to wait_lock.
Can you check the offset of wait_list vs. the rt_mutex itself?
I wouldn't be surprised if it's exactly 8 bytes. And then this thing
looks like a copied lock with stale pointers to hell. Eew.
Thanks,
tglx
From: Mike Galbraith <hidden> Date: 2012-07-12 11:57:22
On Thu, 2012-07-12 at 13:43 +0200, Thomas Gleixner wrote:
On Thu, 12 Jul 2012, Mike Galbraith wrote:
quoted
On Thu, 2012-07-12 at 10:44 +0200, Mike Galbraith wrote:
quoted
On Thu, 2012-07-12 at 07:47 +0200, Mike Galbraith wrote:
quoted
Greetings,
I'm chasing btrfs critters in an enterprise 3.0-rt kernel, and just
checked to see if they're alive in virgin latest/greatest rt kernel.
Both are indeed alive and well, ie I didn't break it, nor did the
zillion patches in enterprise base kernel, so others may have an
opportunity to meet these critters up close and personal as well.
3.2-rt both explodes and deadlocks as well. 3.0-rt (virgin I mean) does
neither, so with enough re-integrate investment, it might be bisectable.
Nope, virgin 3.0-rt just didn't feel like it at the time. Booted it
again to run hefty test over lunch, it didn't survive 1 xfstests 006,
much less hundreds.
crash> bt
PID: 7604 TASK: ffff880174238b20 CPU: 0 COMMAND: "btrfs-worker-0"
#0 [ffff88017455d9c8] machine_kexec at ffffffff81025794
#1 [ffff88017455da28] crash_kexec at ffffffff8109781d
#2 [ffff88017455daf8] panic at ffffffff814a0661
#3 [ffff88017455db78] __try_to_take_rt_mutex at ffffffff81086d2f
#4 [ffff88017455dbc8] rt_spin_lock_slowlock at ffffffff814a2670
#5 [ffff88017455dca8] rt_spin_lock at ffffffff814a2db9
#6 [ffff88017455dcb8] schedule_bio at ffffffff81243133
#7 [ffff88017455dcf8] btrfs_map_bio at ffffffff812477be
#8 [ffff88017455dd68] __btree_submit_bio_done at ffffffff812152f6
#9 [ffff88017455dd78] run_one_async_done at ffffffff812148fa
#10 [ffff88017455dd98] run_ordered_completions at ffffffff812493e8
#11 [ffff88017455ddd8] worker_loop at ffffffff81249dc9
#12 [ffff88017455de88] kthread at ffffffff81070266
#13 [ffff88017455df48] kernel_thread_helper at ffffffff814a9be4
crash> struct rt_mutex 0xffff880174530108
struct rt_mutex {
wait_lock = {
raw_lock = {
slock = 7966
}
},
wait_list = {
node_list = {
next = 0xffff880175ecc970,
prev = 0xffff880175ecc970
},
rawlock = 0xffff880175ecc968,
Pointer into lala land again.
Yeah, and freed again.
rawlock points to ...968 and the node_list to ...970.
struct rt_mutex {
raw_spinlock_t wait_lock;
struct plist_head wait_list;
The raw_lock pointer of the plist_head is initialized in
__rt_mutex_init() so it points to wait_lock.
Can you check the offset of wait_list vs. the rt_mutex itself?
I wouldn't be surprised if it's exactly 8 bytes. And then this thing
looks like a copied lock with stale pointers to hell. Eew.
From: Thomas Gleixner <hidden> Date: 2012-07-12 13:31:06
On Thu, 12 Jul 2012, Mike Galbraith wrote:
On Thu, 2012-07-12 at 13:43 +0200, Thomas Gleixner wrote:
quoted
rawlock points to ...968 and the node_list to ...970.
struct rt_mutex {
raw_spinlock_t wait_lock;
struct plist_head wait_list;
The raw_lock pointer of the plist_head is initialized in
__rt_mutex_init() so it points to wait_lock.
Can you check the offset of wait_list vs. the rt_mutex itself?
I wouldn't be surprised if it's exactly 8 bytes. And then this thing
looks like a copied lock with stale pointers to hell. Eew.
Bingo, that makes it more likely that this is caused by copying w/o
initializing the lock and then freeing the original structure.
A quick check for memcpy finds that __btrfs_close_devices() does a
memcpy of btrfs_device structs w/o initializing the lock in the new
copy, but I have no idea whether that's the place we are looking for.
Thanks,
tglx
From: Mike Galbraith <hidden> Date: 2012-07-12 13:37:50
On Thu, 2012-07-12 at 15:31 +0200, Thomas Gleixner wrote:
On Thu, 12 Jul 2012, Mike Galbraith wrote:
quoted
On Thu, 2012-07-12 at 13:43 +0200, Thomas Gleixner wrote:
quoted
rawlock points to ...968 and the node_list to ...970.
struct rt_mutex {
raw_spinlock_t wait_lock;
struct plist_head wait_list;
The raw_lock pointer of the plist_head is initialized in
__rt_mutex_init() so it points to wait_lock.
Can you check the offset of wait_list vs. the rt_mutex itself?
I wouldn't be surprised if it's exactly 8 bytes. And then this thing
looks like a copied lock with stale pointers to hell. Eew.
Bingo, that makes it more likely that this is caused by copying w/o
initializing the lock and then freeing the original structure.
A quick check for memcpy finds that __btrfs_close_devices() does a
memcpy of btrfs_device structs w/o initializing the lock in the new
copy, but I have no idea whether that's the place we are looking for.
Cool, you found one, thanks! I'm setting boobytraps.
Um, correction, box says I'm setting _buggy_ boobytraps :)
Tomorrow-man will test this and frob traps anew.
From: Thomas Gleixner <hidden> Date: 2012-07-12 13:43:50
On Thu, 12 Jul 2012, Mike Galbraith wrote:
On Thu, 2012-07-12 at 15:31 +0200, Thomas Gleixner wrote:
quoted
A quick check for memcpy finds that __btrfs_close_devices() does a
memcpy of btrfs_device structs w/o initializing the lock in the new
copy, but I have no idea whether that's the place we are looking for.
Cool, you found one, thanks! I'm setting boobytraps.
Um, correction, box says I'm setting _buggy_ boobytraps :)
Tomorrow-man will test this and frob traps anew.
What kind of test setup do you have? i.e. raid, single disk ...
Thanks,
tglx
From: Mike Galbraith <hidden> Date: 2012-07-12 13:48:09
On Thu, 2012-07-12 at 15:43 +0200, Thomas Gleixner wrote:
On Thu, 12 Jul 2012, Mike Galbraith wrote:
quoted
On Thu, 2012-07-12 at 15:31 +0200, Thomas Gleixner wrote:
quoted
A quick check for memcpy finds that __btrfs_close_devices() does a
memcpy of btrfs_device structs w/o initializing the lock in the new
copy, but I have no idea whether that's the place we are looking for.
Cool, you found one, thanks! I'm setting boobytraps.
Um, correction, box says I'm setting _buggy_ boobytraps :)
Tomorrow-man will test this and frob traps anew.
What kind of test setup do you have? i.e. raid, single disk ...
From: Mike Galbraith <hidden> Date: 2012-07-12 13:51:23
On Thu, 2012-07-12 at 15:48 +0200, Mike Galbraith wrote:
On Thu, 2012-07-12 at 15:43 +0200, Thomas Gleixner wrote:
quoted
On Thu, 12 Jul 2012, Mike Galbraith wrote:
quoted
On Thu, 2012-07-12 at 15:31 +0200, Thomas Gleixner wrote:
quoted
A quick check for memcpy finds that __btrfs_close_devices() does a
memcpy of btrfs_device structs w/o initializing the lock in the new
copy, but I have no idea whether that's the place we are looking for.
Cool, you found one, thanks! I'm setting boobytraps.
Um, correction, box says I'm setting _buggy_ boobytraps :)
Tomorrow-man will test this and frob traps anew.
What kind of test setup do you have? i.e. raid, single disk ...
Yeah, megaraid sas.. x3550 M3.
(one disk for OS, one disk for xfstests to mangle)
Urgh. Here is something completely wrong. That should point to
wait_lock, i.e. the rt_mutex itself, but that points into lala land.
This is probably the memcpy you found later this morning, right?
quoted
Reproducer2: dbench -t 30 8
[ 692.857164]
[ 692.857165] ============================================
[ 692.863963] [ BUG: circular locking deadlock detected! ]
[ 692.869264] Not tainted
[ 692.871708] --------------------------------------------
[ 692.877008] btrfs-delayed-m/1404 is deadlocking current task dbench/7937
[ 692.877009]
[ 692.885183]
[ 692.885184] 1) dbench/7937 is trying to acquire this lock:
[ 692.892149] [ffff88014d6aea80] {&(&eb->lock)->lock}
[ 692.897102] .. ->owner: ffff880175808501
[ 692.901018] .. held by: btrfs-delayed-m: 1404 [ffff880175808500, 120]
[ 692.907657]
[ 692.907657] 2) btrfs-delayed-m/1404 is blocked on this lock:
[ 692.914797] [ffff88014bf58d60] {&(&eb->lock)->lock}
[ 692.919751] .. ->owner: ffff880175186101
[ 692.923672] .. held by: dbench: 7937 [ffff880175186100, 120]
[ 692.930309]
[ 692.930309] btrfs-delayed-m/1404's [blocked] stackdump:
Hrmm. Both locks are rw_locks and we prevent multiple readers for the
known reasons in RT. No idea how to deal with that one :(
The reader/writer part in btrfs is just an optimization. If we need
them to be all writer locks for RT purposes, that's not a problem.
But, before we go down that road, we do annotations trying
to make sure lockdep doesn't get confused about lock classes. Basically
the tree is locked level by level. So its safe to take eb->lock while
holding eb->lock as long as you follow the rules.
Are additional annotations required for RT?
-chris
From: Mike Galbraith <hidden> Date: 2012-07-13 06:31:12
On Thu, 2012-07-12 at 15:31 +0200, Thomas Gleixner wrote:
On Thu, 12 Jul 2012, Mike Galbraith wrote:
quoted
On Thu, 2012-07-12 at 13:43 +0200, Thomas Gleixner wrote:
quoted
rawlock points to ...968 and the node_list to ...970.
struct rt_mutex {
raw_spinlock_t wait_lock;
struct plist_head wait_list;
The raw_lock pointer of the plist_head is initialized in
__rt_mutex_init() so it points to wait_lock.
Can you check the offset of wait_list vs. the rt_mutex itself?
I wouldn't be surprised if it's exactly 8 bytes. And then this thing
looks like a copied lock with stale pointers to hell. Eew.
Bingo, that makes it more likely that this is caused by copying w/o
initializing the lock and then freeing the original structure.
A quick check for memcpy finds that __btrfs_close_devices() does a
memcpy of btrfs_device structs w/o initializing the lock in the new
copy, but I have no idea whether that's the place we are looking for.
Thanks a bunch Thomas. I doubt I would have ever figured out that lala
land resulted from _copying_ a lock. That's one I won't be forgetting
any time soon. Box not only survived a few thousand xfstests 006 runs,
dbench seemed disinterested in deadlocking virgin 3.0-rt.
btrfs still locks up in my enterprise kernel, so I suppose I had better
plug your fix into 3.4-rt and see what happens, and go beat hell out of
virgin 3.0-rt again to be sure box really really survives dbench.
From: Thomas Gleixner <hidden> Date: 2012-07-13 09:52:53
On Fri, 13 Jul 2012, Mike Galbraith wrote:
On Thu, 2012-07-12 at 15:31 +0200, Thomas Gleixner wrote:
quoted
Bingo, that makes it more likely that this is caused by copying w/o
initializing the lock and then freeing the original structure.
A quick check for memcpy finds that __btrfs_close_devices() does a
memcpy of btrfs_device structs w/o initializing the lock in the new
copy, but I have no idea whether that's the place we are looking for.
Thanks a bunch Thomas. I doubt I would have ever figured out that lala
land resulted from _copying_ a lock. That's one I won't be forgetting
any time soon. Box not only survived a few thousand xfstests 006 runs,
dbench seemed disinterested in deadlocking virgin 3.0-rt.
Cute. It think that the lock copying caused the deadlock problem as
the list pointed to the wrong place, so we might have ended up with
following down the wrong chain when walking the list as long as the
original struct was not freed. That beast is freed under RCU so there
could be a rcu read side critical section fiddling with the old lock
and cause utter confusion.
/me goes and writes a nastigram^W proper changelog
btrfs still locks up in my enterprise kernel, so I suppose I had better
plug your fix into 3.4-rt and see what happens, and go beat hell out of
virgin 3.0-rt again to be sure box really really survives dbench.
A test against 3.4-rt sans enterprise mess might be nice as well.
Thanks,
tglx
Urgh. Here is something completely wrong. That should point to
wait_lock, i.e. the rt_mutex itself, but that points into lala land.
This is probably the memcpy you found later this morning, right?
As Mike found out, it looks like the culprit.
The reader/writer part in btrfs is just an optimization. If we need
them to be all writer locks for RT purposes, that's not a problem.
But, before we go down that road, we do annotations trying
to make sure lockdep doesn't get confused about lock classes. Basically
the tree is locked level by level. So its safe to take eb->lock while
holding eb->lock as long as you follow the rules.
Are additional annotations required for RT?
I don't think so. I'm sure it has been caused by the lock copying as
well. Walking the wrong list can cause complete confusion all over the
place. So lets wait for Mike beating the hell out of it.
Find the patch with a proper changelog below.
Thanks,
tglx
------------------>
From: Thomas Gleixner <redacted>
Date: Thu, 12 Jul 2012 15:30:02 +0200
Subject: btrfs: Init io_lock after cloning btrfs device struct
__btrfs_close_devices() clones btrfs device structs with
memcpy(). Some of the fields in the clone are reinitialized, but it's
missing to init io_lock. In mainline this goes unnoticed, but on RT it
leaves the plist pointing to the original about to be freed lock
struct.
Initialize io_lock after cloning, so no references to the original
struct are left.
Reported-and-tested-by: Mike Galbraith <redacted>
Cc: stable@vger.kernel.org
Signed-off-by: Thomas Gleixner <redacted>
---
From: Mike Galbraith <hidden> Date: 2012-07-13 10:14:45
On Fri, 2012-07-13 at 11:52 +0200, Thomas Gleixner wrote:
On Fri, 13 Jul 2012, Mike Galbraith wrote:
quoted
On Thu, 2012-07-12 at 15:31 +0200, Thomas Gleixner wrote:
quoted
Bingo, that makes it more likely that this is caused by copying w/o
initializing the lock and then freeing the original structure.
A quick check for memcpy finds that __btrfs_close_devices() does a
memcpy of btrfs_device structs w/o initializing the lock in the new
copy, but I have no idea whether that's the place we are looking for.
Thanks a bunch Thomas. I doubt I would have ever figured out that lala
land resulted from _copying_ a lock. That's one I won't be forgetting
any time soon. Box not only survived a few thousand xfstests 006 runs,
dbench seemed disinterested in deadlocking virgin 3.0-rt.
Cute. It think that the lock copying caused the deadlock problem as
the list pointed to the wrong place, so we might have ended up with
following down the wrong chain when walking the list as long as the
original struct was not freed. That beast is freed under RCU so there
could be a rcu read side critical section fiddling with the old lock
and cause utter confusion.
Virgin 3.0-rt appears to really be solid. But then it doesn't have
pesky rwlocks.
/me goes and writes a nastigram^W proper changelog
quoted
btrfs still locks up in my enterprise kernel, so I suppose I had better
plug your fix into 3.4-rt and see what happens, and go beat hell out of
virgin 3.0-rt again to be sure box really really survives dbench.
A test against 3.4-rt sans enterprise mess might be nice as well.
Enterprise is 3.0-stable with um 555 btrfs patches (oh dear).
Virgin 3.4-rt and 3.2-rt deadlock gripe. Enterprise doesn't gripe, but
deadlocks, so I have another adventure in my future even if I figure out
wth to do about rwlocks.
-Mike
From: Thomas Gleixner <hidden> Date: 2012-07-13 10:26:26
On Fri, 13 Jul 2012, Mike Galbraith wrote:
On Fri, 2012-07-13 at 11:52 +0200, Thomas Gleixner wrote:
quoted
On Fri, 13 Jul 2012, Mike Galbraith wrote:
quoted
On Thu, 2012-07-12 at 15:31 +0200, Thomas Gleixner wrote:
quoted
Bingo, that makes it more likely that this is caused by copying w/o
initializing the lock and then freeing the original structure.
A quick check for memcpy finds that __btrfs_close_devices() does a
memcpy of btrfs_device structs w/o initializing the lock in the new
copy, but I have no idea whether that's the place we are looking for.
Thanks a bunch Thomas. I doubt I would have ever figured out that lala
land resulted from _copying_ a lock. That's one I won't be forgetting
any time soon. Box not only survived a few thousand xfstests 006 runs,
dbench seemed disinterested in deadlocking virgin 3.0-rt.
Cute. It think that the lock copying caused the deadlock problem as
the list pointed to the wrong place, so we might have ended up with
following down the wrong chain when walking the list as long as the
original struct was not freed. That beast is freed under RCU so there
could be a rcu read side critical section fiddling with the old lock
and cause utter confusion.
Virgin 3.0-rt appears to really be solid. But then it doesn't have
pesky rwlocks.
Ah. So 3.0 is not having those rwlock thingies. Bummer.
quoted
/me goes and writes a nastigram^W proper changelog
quoted
btrfs still locks up in my enterprise kernel, so I suppose I had better
plug your fix into 3.4-rt and see what happens, and go beat hell out of
virgin 3.0-rt again to be sure box really really survives dbench.
A test against 3.4-rt sans enterprise mess might be nice as well.
Enterprise is 3.0-stable with um 555 btrfs patches (oh dear).
Virgin 3.4-rt and 3.2-rt deadlock gripe. Enterprise doesn't gripe, but
deadlocks, so I have another adventure in my future even if I figure out
wth to do about rwlocks.
Hrmpf. /me goes to stare into fs/btrfs/ some more.
From: Chris Mason <hidden> Date: 2012-07-13 10:47:41
On Fri, Jul 13, 2012 at 04:26:26AM -0600, Thomas Gleixner wrote:
On Fri, 13 Jul 2012, Mike Galbraith wrote:
quoted
On Fri, 2012-07-13 at 11:52 +0200, Thomas Gleixner wrote:
quoted
On Fri, 13 Jul 2012, Mike Galbraith wrote:
quoted
On Thu, 2012-07-12 at 15:31 +0200, Thomas Gleixner wrote:
quoted
Bingo, that makes it more likely that this is caused by copying w/o
initializing the lock and then freeing the original structure.
A quick check for memcpy finds that __btrfs_close_devices() does a
memcpy of btrfs_device structs w/o initializing the lock in the new
copy, but I have no idea whether that's the place we are looking for.
Thanks a bunch Thomas. I doubt I would have ever figured out that lala
land resulted from _copying_ a lock. That's one I won't be forgetting
any time soon. Box not only survived a few thousand xfstests 006 runs,
dbench seemed disinterested in deadlocking virgin 3.0-rt.
Cute. It think that the lock copying caused the deadlock problem as
the list pointed to the wrong place, so we might have ended up with
following down the wrong chain when walking the list as long as the
original struct was not freed. That beast is freed under RCU so there
could be a rcu read side critical section fiddling with the old lock
and cause utter confusion.
Virgin 3.0-rt appears to really be solid. But then it doesn't have
pesky rwlocks.
Ah. So 3.0 is not having those rwlock thingies. Bummer.
quoted
quoted
/me goes and writes a nastigram^W proper changelog
quoted
btrfs still locks up in my enterprise kernel, so I suppose I had better
plug your fix into 3.4-rt and see what happens, and go beat hell out of
virgin 3.0-rt again to be sure box really really survives dbench.
A test against 3.4-rt sans enterprise mess might be nice as well.
Enterprise is 3.0-stable with um 555 btrfs patches (oh dear).
Virgin 3.4-rt and 3.2-rt deadlock gripe. Enterprise doesn't gripe, but
deadlocks, so I have another adventure in my future even if I figure out
wth to do about rwlocks.
Hrmpf. /me goes to stare into fs/btrfs/ some more.
Please post the deadlocks here, I'll help ;)
-chris
From: Mike Galbraith <hidden> Date: 2012-07-13 12:50:43
On Fri, 2012-07-13 at 06:47 -0400, Chris Mason wrote:
On Fri, Jul 13, 2012 at 04:26:26AM -0600, Thomas Gleixner wrote:
quoted
On Fri, 13 Jul 2012, Mike Galbraith wrote:
quoted
On Fri, 2012-07-13 at 11:52 +0200, Thomas Gleixner wrote:
quoted
On Fri, 13 Jul 2012, Mike Galbraith wrote:
quoted
On Thu, 2012-07-12 at 15:31 +0200, Thomas Gleixner wrote:
quoted
Bingo, that makes it more likely that this is caused by copying w/o
initializing the lock and then freeing the original structure.
A quick check for memcpy finds that __btrfs_close_devices() does a
memcpy of btrfs_device structs w/o initializing the lock in the new
copy, but I have no idea whether that's the place we are looking for.
Thanks a bunch Thomas. I doubt I would have ever figured out that lala
land resulted from _copying_ a lock. That's one I won't be forgetting
any time soon. Box not only survived a few thousand xfstests 006 runs,
dbench seemed disinterested in deadlocking virgin 3.0-rt.
Cute. It think that the lock copying caused the deadlock problem as
the list pointed to the wrong place, so we might have ended up with
following down the wrong chain when walking the list as long as the
original struct was not freed. That beast is freed under RCU so there
could be a rcu read side critical section fiddling with the old lock
and cause utter confusion.
Virgin 3.0-rt appears to really be solid. But then it doesn't have
pesky rwlocks.
Ah. So 3.0 is not having those rwlock thingies. Bummer.
quoted
quoted
/me goes and writes a nastigram^W proper changelog
quoted
btrfs still locks up in my enterprise kernel, so I suppose I had better
plug your fix into 3.4-rt and see what happens, and go beat hell out of
virgin 3.0-rt again to be sure box really really survives dbench.
A test against 3.4-rt sans enterprise mess might be nice as well.
Enterprise is 3.0-stable with um 555 btrfs patches (oh dear).
Virgin 3.4-rt and 3.2-rt deadlock gripe. Enterprise doesn't gripe, but
deadlocks, so I have another adventure in my future even if I figure out
wth to do about rwlocks.
Hrmpf. /me goes to stare into fs/btrfs/ some more.
From: Chris Mason <hidden> Date: 2012-07-13 12:50:43
On Wed, Jul 11, 2012 at 11:47:40PM -0600, Mike Galbraith wrote:
Greetings,
[ deadlocks with btrfs and the recent RT kernels ]
I talked with Thomas about this and I think the problem is the
single-reader nature of the RW rwlocks. The lockdep report below
mentions that btrfs is calling:
In this case, the task has a number of blocking read locks on the btrfs buffers,
and we're trying to turn them back into spinning read locks. Even
though btrfs is taking the read rwlock, it doesn't think of this as a new
lock operation because we were blocking out new writers.
If the second task has taken the spinning read lock, it is going to
prevent that clear_path_blocking operation from progressing, even though
it would have worked on a non-RT kernel.
The solution should be to make the blocking read locks in btrfs honor the
single-reader semantics. This means not allowing more than one blocking
reader and not allowing a spinning reader when there is a blocking
reader. Strictly speaking btrfs shouldn't need recursive readers on a
single lock, so I wouldn't worry about that part.
There is also a chunk of code in btrfs_clear_path_blocking that makes
sure to strictly honor top down locking order during the conversion. It
only does this when lockdep is enabled because in non-RT kernels we
don't need to worry about it. For RT we'll want to enable that as well.
I'll give this a shot later today.
-chris
From: Thomas Gleixner <hidden> Date: 2012-07-13 14:47:29
Chris,
On Fri, 13 Jul 2012, Chris Mason wrote:
On Wed, Jul 11, 2012 at 11:47:40PM -0600, Mike Galbraith wrote:
quoted
Greetings,
[ deadlocks with btrfs and the recent RT kernels ]
I talked with Thomas about this and I think the problem is the
single-reader nature of the RW rwlocks. The lockdep report below
mentions that btrfs is calling:
In this case, the task has a number of blocking read locks on the btrfs buffers,
and we're trying to turn them back into spinning read locks. Even
though btrfs is taking the read rwlock, it doesn't think of this as a new
lock operation because we were blocking out new writers.
If the second task has taken the spinning read lock, it is going to
prevent that clear_path_blocking operation from progressing, even though
it would have worked on a non-RT kernel.
The solution should be to make the blocking read locks in btrfs honor the
single-reader semantics. This means not allowing more than one blocking
reader and not allowing a spinning reader when there is a blocking
reader. Strictly speaking btrfs shouldn't need recursive readers on a
single lock, so I wouldn't worry about that part.
There is also a chunk of code in btrfs_clear_path_blocking that makes
sure to strictly honor top down locking order during the conversion. It
only does this when lockdep is enabled because in non-RT kernels we
don't need to worry about it. For RT we'll want to enable that as well.
thanks for explaining this. I really got lost in that code completely.
I'll give this a shot later today.
Cool.
Aside of that I'm still pondering to experiment with a non-pi variant
of rw locks which allows multiple readers. For such cases as btrfs I
think they would be well suited and avoid the performance overhead of
the single writer restriction. But that's not going to happen before
my vacation, so we'll stick with your workaround for now and let Mike
beat the hell out of it.
Thanks,
Thomas
From: Mike Galbraith <hidden> Date: 2012-07-14 10:14:43
On Fri, 2012-07-13 at 08:50 -0400, Chris Mason wrote:
On Wed, Jul 11, 2012 at 11:47:40PM -0600, Mike Galbraith wrote:
quoted
Greetings,
[ deadlocks with btrfs and the recent RT kernels ]
I talked with Thomas about this and I think the problem is the
single-reader nature of the RW rwlocks. The lockdep report below
mentions that btrfs is calling:
In this case, the task has a number of blocking read locks on the btrfs buffers,
and we're trying to turn them back into spinning read locks. Even
though btrfs is taking the read rwlock, it doesn't think of this as a new
lock operation because we were blocking out new writers.
If the second task has taken the spinning read lock, it is going to
prevent that clear_path_blocking operation from progressing, even though
it would have worked on a non-RT kernel.
The solution should be to make the blocking read locks in btrfs honor the
single-reader semantics. This means not allowing more than one blocking
reader and not allowing a spinning reader when there is a blocking
reader. Strictly speaking btrfs shouldn't need recursive readers on a
single lock, so I wouldn't worry about that part.
There is also a chunk of code in btrfs_clear_path_blocking that makes
sure to strictly honor top down locking order during the conversion. It
only does this when lockdep is enabled because in non-RT kernels we
don't need to worry about it. For RT we'll want to enable that as well.
I'll give this a shot later today.
I took a poke at it. Did I do something similar to what you had in
mind, or just hide behind performance stealing paranoid trylock loops?
Box survived 1000 x xfstests 006 and dbench [-s] massive right off the
bat, so it gets posted despite skepticism.
@@ -77,7 +77,7 @@ noinline void btrfs_clear_path_blocking(struct btrfs_path *p,{inti;-#ifdef CONFIG_DEBUG_LOCK_ALLOC+#if (defined(CONFIG_DEBUG_LOCK_ALLOC) || defined(CONFIG_PREEMPT_RT_BASE))/* lockdep really cares that we take all of these spinlocks*intherightorder.Ifanyofthelocksinthepatharenot*currentlyblocking,itisgoingtocomplain.So,makereally
From: Mike Galbraith <hidden> Date: 2012-07-14 13:38:16
On Fri, 2012-07-13 at 08:50 -0400, Chris Mason wrote:
There is also a chunk of code in btrfs_clear_path_blocking that makes
sure to strictly honor top down locking order during the conversion. It
only does this when lockdep is enabled because in non-RT kernels we
don't need to worry about it. For RT we'll want to enable that as well.
Hm, _seems_ that alone is enough prevent deadlock. Throughput really
sucks though. The other bits of my stab bump throughput for dbench 128
from ~200 mb/s to ~360 mb/s (appears it's the paranoid trylock loops).
ext3 does 775 mb/s with the same kernel. Or, dbench 8 on ext3 gives
~1800 mb/s and ~480 mb/s btrfs. Not exactly wonderful.
Hohum, guess I'll wait and see what your patch looks like. I bet it'll
work a lot better than mine does :)
-Mike
From: Chris Mason <hidden> Date: 2012-07-15 17:56:16
On Sat, Jul 14, 2012 at 04:14:43AM -0600, Mike Galbraith wrote:
On Fri, 2012-07-13 at 08:50 -0400, Chris Mason wrote:
quoted
On Wed, Jul 11, 2012 at 11:47:40PM -0600, Mike Galbraith wrote:
quoted
Greetings,
[ deadlocks with btrfs and the recent RT kernels ]
I talked with Thomas about this and I think the problem is the
single-reader nature of the RW rwlocks. The lockdep report below
mentions that btrfs is calling:
In this case, the task has a number of blocking read locks on the btrfs buffers,
and we're trying to turn them back into spinning read locks. Even
though btrfs is taking the read rwlock, it doesn't think of this as a new
lock operation because we were blocking out new writers.
If the second task has taken the spinning read lock, it is going to
prevent that clear_path_blocking operation from progressing, even though
it would have worked on a non-RT kernel.
The solution should be to make the blocking read locks in btrfs honor the
single-reader semantics. This means not allowing more than one blocking
reader and not allowing a spinning reader when there is a blocking
reader. Strictly speaking btrfs shouldn't need recursive readers on a
single lock, so I wouldn't worry about that part.
There is also a chunk of code in btrfs_clear_path_blocking that makes
sure to strictly honor top down locking order during the conversion. It
only does this when lockdep is enabled because in non-RT kernels we
don't need to worry about it. For RT we'll want to enable that as well.
I'll give this a shot later today.
I took a poke at it. Did I do something similar to what you had in
mind, or just hide behind performance stealing paranoid trylock loops?
Box survived 1000 x xfstests 006 and dbench [-s] massive right off the
bat, so it gets posted despite skepticism.
Great, thanks! I got stuck in bug land on Friday. You mentioned
performance problems earlier on Saturday, did this improve performance?
One other question:
From: Mike Galbraith <hidden> Date: 2012-07-16 02:02:20
On Sun, 2012-07-15 at 13:56 -0400, Chris Mason wrote:
On Sat, Jul 14, 2012 at 04:14:43AM -0600, Mike Galbraith wrote:
quoted
On Fri, 2012-07-13 at 08:50 -0400, Chris Mason wrote:
quoted
On Wed, Jul 11, 2012 at 11:47:40PM -0600, Mike Galbraith wrote:
quoted
Greetings,
[ deadlocks with btrfs and the recent RT kernels ]
I talked with Thomas about this and I think the problem is the
single-reader nature of the RW rwlocks. The lockdep report below
mentions that btrfs is calling:
In this case, the task has a number of blocking read locks on the btrfs buffers,
and we're trying to turn them back into spinning read locks. Even
though btrfs is taking the read rwlock, it doesn't think of this as a new
lock operation because we were blocking out new writers.
If the second task has taken the spinning read lock, it is going to
prevent that clear_path_blocking operation from progressing, even though
it would have worked on a non-RT kernel.
The solution should be to make the blocking read locks in btrfs honor the
single-reader semantics. This means not allowing more than one blocking
reader and not allowing a spinning reader when there is a blocking
reader. Strictly speaking btrfs shouldn't need recursive readers on a
single lock, so I wouldn't worry about that part.
There is also a chunk of code in btrfs_clear_path_blocking that makes
sure to strictly honor top down locking order during the conversion. It
only does this when lockdep is enabled because in non-RT kernels we
don't need to worry about it. For RT we'll want to enable that as well.
I'll give this a shot later today.
I took a poke at it. Did I do something similar to what you had in
mind, or just hide behind performance stealing paranoid trylock loops?
Box survived 1000 x xfstests 006 and dbench [-s] massive right off the
bat, so it gets posted despite skepticism.
Great, thanks! I got stuck in bug land on Friday. You mentioned
performance problems earlier on Saturday, did this improve performance?
Yeah, the read_trylock() seems to improve throughput. That's not
heavily tested, but it certainly looks like it does. No idea why.
WRT performance, dbench isn't thrilled, but btrfs seems to work just
fine for my routine usage, and spinning rust bucket is being all it can
be. I hope I don't have to care overly much about dbench's opinon. It
doesn't make happy multi-thread numbers with btrfs, but those numbers
suddenly look great if you rebase relative to xfs -rt throughput :)
Why use read_trylock() in a loop instead of just trying to take the
lock? Is this an RTism or are there other reasons?
First stab paranoia. It worked, so I removed it. It still worked but
lost throughput, removed all my bits leaving only the lockdep bits, it
still worked.
-Mike
From: Mike Galbraith <hidden> Date: 2012-07-16 13:14:01
On Sat, 2012-07-14 at 12:14 +0200, Mike Galbraith wrote:
On Fri, 2012-07-13 at 08:50 -0400, Chris Mason wrote:
quoted
On Wed, Jul 11, 2012 at 11:47:40PM -0600, Mike Galbraith wrote:
quoted
Greetings,
[ deadlocks with btrfs and the recent RT kernels ]
I talked with Thomas about this and I think the problem is the
single-reader nature of the RW rwlocks. The lockdep report below
mentions that btrfs is calling:
In this case, the task has a number of blocking read locks on the btrfs buffers,
and we're trying to turn them back into spinning read locks. Even
though btrfs is taking the read rwlock, it doesn't think of this as a new
lock operation because we were blocking out new writers.
If the second task has taken the spinning read lock, it is going to
prevent that clear_path_blocking operation from progressing, even though
it would have worked on a non-RT kernel.
The solution should be to make the blocking read locks in btrfs honor the
single-reader semantics. This means not allowing more than one blocking
reader and not allowing a spinning reader when there is a blocking
reader. Strictly speaking btrfs shouldn't need recursive readers on a
single lock, so I wouldn't worry about that part.
There is also a chunk of code in btrfs_clear_path_blocking that makes
sure to strictly honor top down locking order during the conversion. It
only does this when lockdep is enabled because in non-RT kernels we
don't need to worry about it. For RT we'll want to enable that as well.
I'll give this a shot later today.
I took a poke at it. Did I do something similar to what you had in
mind, or just hide behind performance stealing paranoid trylock loops?
Box survived 1000 x xfstests 006 and dbench [-s] massive right off the
bat, so it gets posted despite skepticism.
@@ -77,7 +77,7 @@ noinline void btrfs_clear_path_blocking(struct btrfs_path *p,{inti;-#ifdef CONFIG_DEBUG_LOCK_ALLOC+#if (defined(CONFIG_DEBUG_LOCK_ALLOC) || defined(CONFIG_PREEMPT_RT_BASE))/* lockdep really cares that we take all of these spinlocks*intherightorder.Ifanyofthelocksinthepatharenot*currentlyblocking,itisgoingtocomplain.So,makereally
From: Chris Mason <hidden> Date: 2012-07-16 15:43:04
On Mon, Jul 16, 2012 at 04:55:44AM -0600, Mike Galbraith wrote:
On Sat, 2012-07-14 at 12:14 +0200, Mike Galbraith wrote:
quoted
On Fri, 2012-07-13 at 08:50 -0400, Chris Mason wrote:
quoted
On Wed, Jul 11, 2012 at 11:47:40PM -0600, Mike Galbraith wrote:
quoted
Greetings,
[ deadlocks with btrfs and the recent RT kernels ]
I talked with Thomas about this and I think the problem is the
single-reader nature of the RW rwlocks. The lockdep report below
mentions that btrfs is calling:
In this case, the task has a number of blocking read locks on the btrfs buffers,
and we're trying to turn them back into spinning read locks. Even
though btrfs is taking the read rwlock, it doesn't think of this as a new
lock operation because we were blocking out new writers.
If the second task has taken the spinning read lock, it is going to
prevent that clear_path_blocking operation from progressing, even though
it would have worked on a non-RT kernel.
The solution should be to make the blocking read locks in btrfs honor the
single-reader semantics. This means not allowing more than one blocking
reader and not allowing a spinning reader when there is a blocking
reader. Strictly speaking btrfs shouldn't need recursive readers on a
single lock, so I wouldn't worry about that part.
There is also a chunk of code in btrfs_clear_path_blocking that makes
sure to strictly honor top down locking order during the conversion. It
only does this when lockdep is enabled because in non-RT kernels we
don't need to worry about it. For RT we'll want to enable that as well.
I'll give this a shot later today.
I took a poke at it. Did I do something similar to what you had in
mind, or just hide behind performance stealing paranoid trylock loops?
Box survived 1000 x xfstests 006 and dbench [-s] massive right off the
bat, so it gets posted despite skepticism.
Seems btrfs isn't entirely convinced either.
[ 2292.336229] use_block_rsv: 1810 callbacks suppressed
[ 2292.336231] ------------[ cut here ]------------
[ 2292.336255] WARNING: at fs/btrfs/extent-tree.c:6344 use_block_rsv+0x17d/0x190 [btrfs]()
[ 2292.336257] Hardware name: System x3550 M3 -[7944K3G]-
[ 2292.336259] btrfs: block rsv returned -28
This is unrelated. You got far enough into the benchmark to hit an
ENOSPC warning. This can be ignored (I just deleted it when we used 3.0
for oracle).
re: dbench performance. dbench tends to penalize fairness. I can
imagine RT making it slower in general.
It also triggers lots of lock contention in btrfs because the dataset is
fairly small and the trees don't fan out a lot.
-chris
From: Steven Rostedt <rostedt@goodmis.org> Date: 2012-07-16 16:02:27
On Mon, 2012-07-16 at 04:02 +0200, Mike Galbraith wrote:
quoted
Great, thanks! I got stuck in bug land on Friday. You mentioned
performance problems earlier on Saturday, did this improve performance?
Yeah, the read_trylock() seems to improve throughput. That's not
heavily tested, but it certainly looks like it does. No idea why.
Ouch, you just turned the rt_read_lock() into a spin lock. If a higher
priority process preempted a lower priority process that holds the same
lock, it will deadlock.
I'm not sure why you would get a performance benefit from this, as the
mutex used is an adaptive one (failure to acquire the lock will only
sleep if preempted or if the owner is not running).
We should look at why this performs better (if it really does).
-- Steve
WRT performance, dbench isn't thrilled, but btrfs seems to work just
fine for my routine usage, and spinning rust bucket is being all it can
be. I hope I don't have to care overly much about dbench's opinon. It
doesn't make happy multi-thread numbers with btrfs, but those numbers
suddenly look great if you rebase relative to xfs -rt throughput :)
Why use read_trylock() in a loop instead of just trying to take the
lock? Is this an RTism or are there other reasons?
First stab paranoia. It worked, so I removed it. It still worked but
lost throughput, removed all my bits leaving only the lockdep bits, it
still worked.
-Mike
From: Mike Galbraith <hidden> Date: 2012-07-16 16:16:47
On Mon, 2012-07-16 at 11:43 -0400, Chris Mason wrote:
On Mon, Jul 16, 2012 at 04:55:44AM -0600, Mike Galbraith wrote:
quoted
Seems btrfs isn't entirely convinced either.
[ 2292.336229] use_block_rsv: 1810 callbacks suppressed
[ 2292.336231] ------------[ cut here ]------------
[ 2292.336255] WARNING: at fs/btrfs/extent-tree.c:6344 use_block_rsv+0x17d/0x190 [btrfs]()
[ 2292.336257] Hardware name: System x3550 M3 -[7944K3G]-
[ 2292.336259] btrfs: block rsv returned -28
This is unrelated. You got far enough into the benchmark to hit an
ENOSPC warning. This can be ignored (I just deleted it when we used 3.0
for oracle).
Ah great, thanks. I'll whack it in my tree as well then.
re: dbench performance. dbench tends to penalize fairness. I can
imagine RT making it slower in general.
It seems to work just fine for my normal workloads, and cyclictest is
happy, so I'm happy. Zillion threads is 'keep the pieces' to me ;-)
If you think the patch is ok as is, I'll go ahead and submit it after I
let dbench hammer on it overnight at least.
-Mike
From: Mike Galbraith <hidden> Date: 2012-07-16 16:26:08
On Mon, 2012-07-16 at 12:02 -0400, Steven Rostedt wrote:
On Mon, 2012-07-16 at 04:02 +0200, Mike Galbraith wrote:
quoted
quoted
Great, thanks! I got stuck in bug land on Friday. You mentioned
performance problems earlier on Saturday, did this improve performance?
Yeah, the read_trylock() seems to improve throughput. That's not
heavily tested, but it certainly looks like it does. No idea why.
Ouch, you just turned the rt_read_lock() into a spin lock. If a higher
priority process preempted a lower priority process that holds the same
lock, it will deadlock.
Hm, how, it's doing cpu_chill()?
I'm not sure why you would get a performance benefit from this, as the
mutex used is an adaptive one (failure to acquire the lock will only
sleep if preempted or if the owner is not running).
I'm not attached to it, can whack it in a heartbeat.. especially so it
the thing can deadlock. I've seen enough of those of late.
We should look at why this performs better (if it really does).
Not sure it really does, there's variance, but it looked like it did.
-Mike
From: Chris Mason <hidden> Date: 2012-07-16 16:35:42
On Mon, Jul 16, 2012 at 10:26:08AM -0600, Mike Galbraith wrote:
On Mon, 2012-07-16 at 12:02 -0400, Steven Rostedt wrote:
quoted
On Mon, 2012-07-16 at 04:02 +0200, Mike Galbraith wrote:
quoted
quoted
Great, thanks! I got stuck in bug land on Friday. You mentioned
performance problems earlier on Saturday, did this improve performance?
Yeah, the read_trylock() seems to improve throughput. That's not
heavily tested, but it certainly looks like it does. No idea why.
Ouch, you just turned the rt_read_lock() into a spin lock. If a higher
priority process preempted a lower priority process that holds the same
lock, it will deadlock.
Hm, how, it's doing cpu_chill()?
quoted
I'm not sure why you would get a performance benefit from this, as the
mutex used is an adaptive one (failure to acquire the lock will only
sleep if preempted or if the owner is not running).
I'm not attached to it, can whack it in a heartbeat.. especially so it
the thing can deadlock. I've seen enough of those of late.
quoted
We should look at why this performs better (if it really does).
Not sure it really does, there's variance, but it looked like it did.
I'd use a benchmark that is more consistent than dbench for this. I
love dbench for generating load (and the occasional deadlock) but it
tends to steer you in the wrong direction on performance.
-chris
From: Mike Galbraith <hidden> Date: 2012-07-16 16:36:07
On Mon, 2012-07-16 at 18:26 +0200, Mike Galbraith wrote:
On Mon, 2012-07-16 at 12:02 -0400, Steven Rostedt wrote:
quoted
On Mon, 2012-07-16 at 04:02 +0200, Mike Galbraith wrote:
quoted
quoted
Great, thanks! I got stuck in bug land on Friday. You mentioned
performance problems earlier on Saturday, did this improve performance?
Yeah, the read_trylock() seems to improve throughput. That's not
heavily tested, but it certainly looks like it does. No idea why.
Ouch, you just turned the rt_read_lock() into a spin lock. If a higher
priority process preempted a lower priority process that holds the same
lock, it will deadlock.
Hm, how, it's doing cpu_chill()?
'course PI is toast, so *poof*. Since just enabling the lockdep bits
seems to fix it up, maybe that's the patchlet to submit (less is more).
-Mike
From: Steven Rostedt <rostedt@goodmis.org> Date: 2012-07-16 17:03:41
On Mon, 2012-07-16 at 18:36 +0200, Mike Galbraith wrote:
quoted
quoted
Ouch, you just turned the rt_read_lock() into a spin lock. If a higher
priority process preempted a lower priority process that holds the same
lock, it will deadlock.
Hm, how, it's doing cpu_chill()?
'course PI is toast, so *poof*. Since just enabling the lockdep bits
seems to fix it up, maybe that's the patchlet to submit (less is more).
There's that too. But the issue I was talking about is with all trylock
loops. As holding an rt-mutex now disables migration, if a high priority
process preempts a task that holds the lock, and then the high prio task
starts spinning waiting for that lock to release, the lower priority
process will never get to run to release it. The cpu_chill() doesn't
help.
-- Steve
From: Mike Galbraith <hidden> Date: 2012-07-17 04:18:47
On Mon, 2012-07-16 at 13:03 -0400, Steven Rostedt wrote:
On Mon, 2012-07-16 at 18:36 +0200, Mike Galbraith wrote:
quoted
quoted
quoted
Ouch, you just turned the rt_read_lock() into a spin lock. If a higher
priority process preempted a lower priority process that holds the same
lock, it will deadlock.
Hm, how, it's doing cpu_chill()?
'course PI is toast, so *poof*. Since just enabling the lockdep bits
seems to fix it up, maybe that's the patchlet to submit (less is more).
There's that too. But the issue I was talking about is with all trylock
loops. As holding an rt-mutex now disables migration, if a high priority
process preempts a task that holds the lock, and then the high prio task
starts spinning waiting for that lock to release, the lower priority
process will never get to run to release it. The cpu_chill() doesn't
help.
Hrm. I better go make a testcase, this one definitely wants pounding
through thick skull.
I think all of the chilling in patchlet is really ugly anyway, so would
prefer to trash it all, just enable the lockdep bits. If it turns out
we really do need to bounce off of counts, go get a bigger hammer when
the need arises. For the nonce, the pre-installed hammer _seemed_ big
enough for the job.
What's a good way to beat living hell out of btrfs? I've never been
into destructive fs testing, since they usually lived on my one and only
disk. x3550 has two, and OS clone has already been sacrificed.
-Mike
From: Steven Rostedt <rostedt@goodmis.org> Date: 2012-07-17 04:27:59
On Tue, 2012-07-17 at 06:18 +0200, Mike Galbraith wrote:
quoted
There's that too. But the issue I was talking about is with all trylock
loops. As holding an rt-mutex now disables migration, if a high priority
process preempts a task that holds the lock, and then the high prio task
starts spinning waiting for that lock to release, the lower priority
process will never get to run to release it. The cpu_chill() doesn't
help.
Hrm. I better go make a testcase, this one definitely wants pounding
through thick skull.
Actually, I was mistaken. I forgot that we defined 'cpu_chill()' as
msleep(1) on RT, which would keep a deadlock from happening.
It doesn't explain the performance enhancement you get :-/
-- Steve
From: Steven Rostedt <rostedt@goodmis.org> Date: 2012-07-17 04:34:08
On Tue, 2012-07-17 at 00:27 -0400, Steven Rostedt wrote:
Actually, I was mistaken. I forgot that we defined 'cpu_chill()' as
msleep(1) on RT, which would keep a deadlock from happening.
Perhaps cpu_chill() isn't a good name, as it doesn't really explain what
is happening. Perhaps one of the following?
cpu_rest()
cpu_sleep()
cpu_deep_relax()
cpu_dream()
cpu_hypnotize()
-- Steve
From: Mike Galbraith <hidden> Date: 2012-07-17 04:44:43
On Tue, 2012-07-17 at 00:27 -0400, Steven Rostedt wrote:
On Tue, 2012-07-17 at 06:18 +0200, Mike Galbraith wrote:
quoted
quoted
There's that too. But the issue I was talking about is with all trylock
loops. As holding an rt-mutex now disables migration, if a high priority
process preempts a task that holds the lock, and then the high prio task
starts spinning waiting for that lock to release, the lower priority
process will never get to run to release it. The cpu_chill() doesn't
help.
Hrm. I better go make a testcase, this one definitely wants pounding
through thick skull.
Actually, I was mistaken. I forgot that we defined 'cpu_chill()' as
msleep(1) on RT, which would keep a deadlock from happening.
Whew! There are no stars and moons on my pointy hat, just plain white
cone, so you had me worried I was missing something critical there.
It doesn't explain the performance enhancement you get :-/
No, it doesn't. The only thing I can think of is that while folks are
timed sleeping, they aren't preempting and interleaving IO as much, but
I'm pulling that out of thin air. Timed sleep should be a lot longer
than regular wakeup, so to my mind, there should be less interleave due
to more thumb twiddling.
-Mike
From: Mike Galbraith <hidden> Date: 2012-07-17 04:46:14
On Tue, 2012-07-17 at 00:34 -0400, Steven Rostedt wrote:
On Tue, 2012-07-17 at 00:27 -0400, Steven Rostedt wrote:
quoted
Actually, I was mistaken. I forgot that we defined 'cpu_chill()' as
msleep(1) on RT, which would keep a deadlock from happening.
Perhaps cpu_chill() isn't a good name, as it doesn't really explain what
is happening. Perhaps one of the following?
cpu_rest()
cpu_sleep()
cpu_deep_relax()
cpu_dream()
cpu_hypnotize()
From: Mike Galbraith <hidden> Date: 2012-07-17 12:54:17
On Tue, 2012-07-17 at 06:18 +0200, Mike Galbraith wrote:
On Mon, 2012-07-16 at 13:03 -0400, Steven Rostedt wrote:
quoted
On Mon, 2012-07-16 at 18:36 +0200, Mike Galbraith wrote:
quoted
quoted
quoted
Ouch, you just turned the rt_read_lock() into a spin lock. If a higher
priority process preempted a lower priority process that holds the same
lock, it will deadlock.
Hm, how, it's doing cpu_chill()?
'course PI is toast, so *poof*. Since just enabling the lockdep bits
seems to fix it up, maybe that's the patchlet to submit (less is more).
There's that too. But the issue I was talking about is with all trylock
loops. As holding an rt-mutex now disables migration, if a high priority
process preempts a task that holds the lock, and then the high prio task
starts spinning waiting for that lock to release, the lower priority
process will never get to run to release it. The cpu_chill() doesn't
help.
Hrm. I better go make a testcase, this one definitely wants pounding
through thick skull.
I think all of the chilling in patchlet is really ugly anyway, so would
prefer to trash it all, just enable the lockdep bits. If it turns out
we really do need to bounce off of counts, go get a bigger hammer when
the need arises. For the nonce, the pre-installed hammer _seemed_ big
enough for the job.
All night dbench session, and all day doing many full xfstests runs
(what will run on btrfs), fsstress -p64, and generic beating says the
pre-installed tool is fine all by itself, so here comes a zero line
patch.. the second best kind ;-)
rt,fs,btrfs: fix rt deadlock on extent_buffer->lock
Trivially repeatable deadlock is cured by enabling lockdep code in
btrfs_clear_path_blocking() as suggested by Chris Mason. He also
suggested restricting blocking reader count to one, and not allowing
a spinning reader while blocking reader exists. This has proven to
be unnecessary, the strict lock order enforcement is enough.. or
rather that's my box's opinion after long hours of hard pounding.
Signed-off-by: Mike Galbraith <redacted>
Cc: Chris Mason <redacted>
@@ -72,7 +72,7 @@ noinline void btrfs_clear_path_blocking(struct btrfs_path *p,{inti;-#ifdef CONFIG_DEBUG_LOCK_ALLOC+#if (defined(CONFIG_DEBUG_LOCK_ALLOC) || defined (CONFIG_PREEMPT_RT_BASE))/* lockdep really cares that we take all of these spinlocks*intherightorder.Ifanyofthelocksinthepatharenot*currentlyblocking,itisgoingtocomplain.So,makereally