Re: [PATCH 3/4] nvmet: fix hang in nvmet_ns_disable()

Taehee Yoo
2023-01-04T10:30:24Z

Hi Chaitanya,
Thank you so much for your review!

On 1/4/23 09:32, Chaitanya Kulkarni wrote:
> On 1/3/23 02:03, Taehee Yoo wrote:
>> nvme target namespace is enabled or disabled by nvmet_ns_enable() or
>> nvmet_ns_disable().
>> The subsys->lock is used to disallow to use namespace data while
>> nvmet_ns_enable() or nvmet_ns_disable() are working.
>> The ns->enabled boolean variable prevents using namespace data in wrong
>> state such as uninitialized state.
>>
>> nvmet_ns_disable() acquires ns->lock and set ns->enabled false.
>> Then, it releases ns->lock for a while to wait ns->disable_done
completion.
>> At this point, nvmet_ns_enable() can be worked concurrently and it calls
>> percpu_ref_init().
>> So, ns->disable_done will never be completed.
>> Therefore hang would occur at this point.
>>
>> CPU0 CPU1
>> nvmet_ns_disable();
>> mutex_lock(&subsys->lock); nvmet_ns_enable();
>> mutex_lock(&subsys->lock);
>> ns->enabled = false;
>> mutex_unlock(&subsys->lock);
>> percpu_ref_init();
>> wait_for_completion(&ns->disable_done); <-- infinite wait
>>
>> mutex_lock(&subsys->lock);
>> mutex_unlock(&subsys->lock);
>>
>> INFO: task bash:926 blocked for more than 30 seconds.
>> Tainted: G W 6.1.0+ #17
>> "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this
message.
>> task:bash state:D stack:27200 pid:926 ppid:911
>> flags:0x00004000
>> Call Trace:
>> <TASK>
>> __schedule+0xafc/0x2930
>> ? io_schedule_timeout+0x160/0x160
>> ? _raw_spin_unlock_irq+0x24/0x50
>> ? __wait_for_common+0x39b/0x5c0
>> ? usleep_range_state+0x190/0x190
>> schedule+0x130/0x230
>> schedule_timeout+0x18a/0x240
>> ? usleep_range_state+0x190/0x190
>> ? rcu_read_lock_sched_held+0x12/0x80
>> ? lock_downgrade+0x700/0x700
>> ? do_raw_spin_trylock+0xb5/0x180
>> ? lock_contended+0xdf0/0xdf0
>> ? _raw_spin_unlock_irq+0x24/0x50
>> ? trace_hardirqs_on+0x3c/0x190
>> __wait_for_common+0x1ca/0x5c0
>> ? usleep_range_state+0x190/0x190
>> ? bit_wait_io+0xf0/0xf0
>> ? _raw_spin_unlock_irqrestore+0x59/0x70
>> nvmet_ns_disable+0x288/0x490
>> ? nvmet_ns_enable+0x970/0x970
>> ? lockdep_hardirqs_on_prepare+0x410/0x410
>> ? rcu_read_lock_sched_held+0x12/0x80
>> ? configfs_write_iter+0x1df/0x480
>> ? nvmet_ns_revalidate_size_store+0x220/0x220
>> nvmet_ns_enable_store+0x85/0xe0
>> [ ... ]
>>
>> Fixes: a07b4970f464 ("nvmet: add a generic NVMe target")
>> Signed-off-by: Taehee Yoo <ap420073@gmail.com>
>
> [...]
>
>> diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
>> index 89bedfcd974c..e609787577c6 100644
>> --- a/drivers/nvme/target/nvmet.h
>> +++ b/drivers/nvme/target/nvmet.h
>> @@ -56,6 +56,12 @@
>> #define IPO_IATTR_CONNECT_SQE(x) \
>> (cpu_to_le32(offsetof(struct nvmf_connect_command, x)))
>>
>> +enum nvmet_ns_state {
>> + NVMET_NS_ENABLED,
>> + NVMET_NS_DISABLING,
>> + NVMET_NS_DISABLED
>> +};
>> +
>
> The patch looks good to me, but I'm wondering if there is a way
> we can do this without adding new enable disable states ?
>

I'm not sure, but the workqueue would be possible.
The point is to make enable() and disable() are worked exclusively
(serially).
workqueue will execute enable()/disable() functions serially.

Thanks a lot,
Taehee Yoo