Re: blk_mq_freeze_queue hang and possible race in percpu-refcount
Hi Tejun,
Thanks, I see I missed the RCU part.
I'll try the force atomic thing.
Though so far I haven't been able to reproduce it yet.
Thanks,
David
2018-03-14 8:43 GMT-07:00 Tejun Heo :
> Hello, David.
>
> On Tue, Mar 13, 2018 at 03:50:47PM -0700, David Chen wrote:
>>
>> CPU A CPU B
>> - -
>> percpu_ref_kill() percpu_ref_tryget_live()
>> {
>> if (__ref_is_percpu())
>> set __PERCPU_REF_DEAD;
>> __percpu_ref_switch_mode();
>>^ sum up current percpu_count
>> this_cpu_inc(*percpu_count); <- this
>> increment got leaked.
>>
>>
>>
>> So if later CPU B later does percpu_ref_put, it will cause ref->count
>> to drop to -1.
>> And thus causing the above hung task issue.
>>
>> Do you think this theory is correct, or am I missing something?
>> Please tell me what do you think.
>
> The switching to atomic mode does something like the following.
>
> 1. Mark the refcnt so that __ref_is_percpu() is false.
>
> 2. Wait for RCU grace period so that everyone including
>percpu_ref_tryget_live() which has seen true __ref_is_percpu() is
>done with its operation.
>
> 3. Now that it knows nobody is operating on the assumption that the
>counter is in percpu mode, it adds up all the percpu counters.
>
> So, provided there aren't some silly bugs, what you described
> shouldn't happen. Can you force the refcnt into atomic mode w/
> PERCPU_REF_INIT_ATOMIC and see whether the problem persists?
>
> Thanks.
>
> --
> tejun
Re: blk_mq_freeze_queue hang and possible race in percpu-refcount
Hello, David.
On Tue, Mar 13, 2018 at 03:50:47PM -0700, David Chen wrote:
>
> CPU A CPU B
> - -
> percpu_ref_kill() percpu_ref_tryget_live()
> {
> if (__ref_is_percpu())
> set __PERCPU_REF_DEAD;
> __percpu_ref_switch_mode();
>^ sum up current percpu_count
> this_cpu_inc(*percpu_count); <- this
> increment got leaked.
>
>
>
> So if later CPU B later does percpu_ref_put, it will cause ref->count
> to drop to -1.
> And thus causing the above hung task issue.
>
> Do you think this theory is correct, or am I missing something?
> Please tell me what do you think.
The switching to atomic mode does something like the following.
1. Mark the refcnt so that __ref_is_percpu() is false.
2. Wait for RCU grace period so that everyone including
percpu_ref_tryget_live() which has seen true __ref_is_percpu() is
done with its operation.
3. Now that it knows nobody is operating on the assumption that the
counter is in percpu mode, it adds up all the percpu counters.
So, provided there aren't some silly bugs, what you described
shouldn't happen. Can you force the refcnt into atomic mode w/
PERCPU_REF_INIT_ATOMIC and see whether the problem persists?
Thanks.
--
tejun
blk_mq_freeze_queue hang and possible race in percpu-refcount
Hi Tejun,
We recently hit an issue where several tasks hung in blk_mq_freeze_queue_wait.
All the task have the same stack trace as the one below, which is
doing fput on loop device.
Which is strange because the task should be no more than a open(2), a
couple pread(2),
and close(2) on a loop device.
"echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
zpool D0 32416 32156 0x0080
967e4b0a1740 967e4b0a1740 967dcdd5 967dcd8825c0
967eafad9780 b7ab84c5fd18 938789dc 0286
0286 967dcdd5 967ea7d691e8 967ea6df2528
Call Trace:
[] ? __schedule+0x21c/0x680
[] schedule+0x36/0x80
[] blk_mq_freeze_queue_wait+0x3c/0x90
[] ? prepare_to_wait_event+0x110/0x110
[] blk_mq_freeze_queue+0x1a/0x20
[] lo_release+0x61/0x70
[] __blkdev_put+0x225/0x280
[] blkdev_put+0x4c/0x110
[] blkdev_close+0x25/0x30
[] __fput+0xe7/0x220
[] fput+0xe/0x10
[] task_work_run+0x83/0xb0
[] exit_to_usermode_loop+0x66/0x92
[] do_syscall_64+0x165/0x180
[] entry_SYSCALL64_slow_path+0x25/0x25
I look into the percpu-refcount code, and found a place that might
have a race that might cause this.
Consider a concurrent percpu_ref_kill and percpu_ref_tryget_live:
CPU A CPU B
- -
percpu_ref_kill() percpu_ref_tryget_live()
{
if (__ref_is_percpu())
set __PERCPU_REF_DEAD;
__percpu_ref_switch_mode();
^ sum up current percpu_count
this_cpu_inc(*percpu_count); <- this
increment got leaked.
So if later CPU B later does percpu_ref_put, it will cause ref->count
to drop to -1.
And thus causing the above hung task issue.
Do you think this theory is correct, or am I missing something?
Please tell me what do you think.
Thanks,
David
