Potential hang on ublk_ctrl_del_dev()

Potential hang on ublk_ctrl_del_dev()

[Nadav Amit]

Hello Ming,

I am trying the ublk and it seems very exciting.

However, I encounter an issue when I remove a ublk device that is mounted or
in use.

In ublk_ctrl_del_dev(), shouldn’t we *not* wait if ublk_idr_freed() is false?
It seems to me that it is saner to return -EBUSY in such a case and let
userspace deal with the results.

For instance, if I run the following (using ubdsrv):

 $ mkfs.ext4 /dev/ram0
 $ ./ublk add -t loop -f /dev/ram0
 $ sudo mount /dev/ublkb0 tmp
 $ sudo ./ublk del -a

ublk_ctrl_del_dev() would not be done until the partition is unmounted, and you
can get a splat that is similar to the one below.

What do you say? Would you agree to change the behavior to return -EBUSY?

Thanks,
Nadav


[  974.149938] INFO: task ublk:2250 blocked for more than 120 seconds.
[  974.157786]       Not tainted 6.1.0 #30
[  974.162369] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
[  974.171417] task:ublk            state:D stack:0     pid:2250  ppid:2249   flags:0x00004004
[  974.181054] Call Trace:
[  974.184097]  <TASK>
[  974.186726]  __schedule+0x37e/0xe10
[  974.190915]  ? __this_cpu_preempt_check+0x13/0x20
[  974.196463]  ? lock_release+0x133/0x2a0
[  974.201043]  schedule+0x67/0xe0
[  974.204846]  ublk_ctrl_uring_cmd+0xf45/0x1110
[  974.210016]  ? lock_is_held_type+0xdd/0x130
[  974.214990]  ? var_wake_function+0x60/0x60
[  974.219872]  ? rcu_read_lock_sched_held+0x4f/0x80
[  974.225443]  io_uring_cmd+0x9a/0x130
[  974.229743]  ? io_uring_cmd_prep+0xf0/0xf0
[  974.234638]  io_issue_sqe+0xfe/0x340
[  974.238946]  io_submit_sqes+0x231/0x750
[  974.243553]  __x64_sys_io_uring_enter+0x22b/0x640
[  974.249134]  ? trace_hardirqs_on+0x3c/0xe0
[  974.254042]  do_syscall_64+0x35/0x80
[  974.258361]  entry_SYSCALL_64_after_hwframe+0x46/0xb0
[  974.264335] RIP: 0033:0x7f1dc2958efd
[  974.268657] RSP: 002b:00007ffdfd22d638 EFLAGS: 00000246 ORIG_RAX: 00000000000001aa
[  974.277471] RAX: ffffffffffffffda RBX: 00005592eabe7f60 RCX: 00007f1dc2958efd
[  974.285800] RDX: 0000000000000000 RSI: 0000000000000001 RDI: 0000000000000004
[  974.294139] RBP: 00005592eabe7f60 R08: 0000000000000000 R09: 0000000000000008
[  974.302473] R10: 0000000000000000 R11: 0000000000000246 R12: 0000000000000000
[  974.310811] R13: 0000000000000000 R14: 00000000ffffffff R15: 0000000000000000
[  974.319168]  </TASK>
[  974.321982] 
[  974.321982] Showing all locks held in the system:
[  974.329776] 1 lock held by rcu_tasks_kthre/12:
[  974.335443]  #0: ffffffff82f6e890 (rcu_tasks.tasks_gp_mutex){+.+.}-{3:3}, at: rcu_tasks_one_gp+0x2d/0x3f0
[  974.346935] 1 lock held by rcu_tasks_rude_/13:
[  974.352573]  #0: ffffffff82f6e610 (rcu_tasks_rude.tasks_gp_mutex){+.+.}-{3:3}, at: rcu_tasks_one_gp+0x2d/0x3f0
[  974.364522] 1 lock held by rcu_tasks_trace/14:
[  974.370246]  #0: ffffffff82f6e350 (rcu_tasks_trace.tasks_gp_mutex){+.+.}-{3:3}, at: rcu_tasks_one_gp+0x2d/0x3f0
[  974.382331] 1 lock held by khungtaskd/310:
[  974.387730]  #0: ffffffff82f6f2a0 (rcu_read_lock){....}-{1:2}, at: debug_show_all_locks+0x23/0x17e
[  974.398598] 5 locks held by kworker/8:1/330:
[  974.404176] 1 lock held by systemd-journal/761:
[  974.410003] 1 lock held by in:imklog/1390:
[  974.415337]  #0: ffff88810ead82e8 (&f->f_pos_lock){+.+.}-{3:3}, at: __fdget_pos+0x45/0x50
[  974.425284] 2 locks held by ublk/2250:
[  974.430167]  #0: ffff8881764e68a8 (&ctx->uring_lock){+.+.}-{3:3}, at: __x64_sys_io_uring_enter+0x21f/0x640
[  974.441708]  #1: ffffffff83106368 (ublk_ctl_mutex){+.+.}-{3:3}, at: ublk_ctrl_uring_cmd+0x6e4/0x1110
[  974.452674] 
[  974.455090] =============================================

Re: Potential hang on ublk_ctrl_del_dev()

[Jens Axboe]

On 1/3/23 2:47?PM, Nadav Amit wrote:
> Hello Ming,
> 
> I am trying the ublk and it seems very exciting.
> 
> However, I encounter an issue when I remove a ublk device that is mounted or
> in use.
> 
> In ublk_ctrl_del_dev(), shouldn?t we *not* wait if ublk_idr_freed() is false?
> It seems to me that it is saner to return -EBUSY in such a case and let
> userspace deal with the results.
> 
> For instance, if I run the following (using ubdsrv):
> 
>  $ mkfs.ext4 /dev/ram0
>  $ ./ublk add -t loop -f /dev/ram0
>  $ sudo mount /dev/ublkb0 tmp
>  $ sudo ./ublk del -a
> 
> ublk_ctrl_del_dev() would not be done until the partition is unmounted, and you
> can get a splat that is similar to the one below.
> 
> What do you say? Would you agree to change the behavior to return -EBUSY?
> 
> Thanks,
> Nadav
> 
> 
> [  974.149938] INFO: task ublk:2250 blocked for more than 120 seconds.
> [  974.157786]       Not tainted 6.1.0 #30
> [  974.162369] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> [  974.171417] task:ublk            state:D stack:0     pid:2250  ppid:2249   flags:0x00004004
> [  974.181054] Call Trace:
> [  974.184097]  <TASK>
> [  974.186726]  __schedule+0x37e/0xe10
> [  974.190915]  ? __this_cpu_preempt_check+0x13/0x20
> [  974.196463]  ? lock_release+0x133/0x2a0
> [  974.201043]  schedule+0x67/0xe0
> [  974.204846]  ublk_ctrl_uring_cmd+0xf45/0x1110
> [  974.210016]  ? lock_is_held_type+0xdd/0x130
> [  974.214990]  ? var_wake_function+0x60/0x60
> [  974.219872]  ? rcu_read_lock_sched_held+0x4f/0x80
> [  974.225443]  io_uring_cmd+0x9a/0x130
> [  974.229743]  ? io_uring_cmd_prep+0xf0/0xf0
> [  974.234638]  io_issue_sqe+0xfe/0x340
> [  974.238946]  io_submit_sqes+0x231/0x750
> [  974.243553]  __x64_sys_io_uring_enter+0x22b/0x640
> [  974.249134]  ? trace_hardirqs_on+0x3c/0xe0
> [  974.254042]  do_syscall_64+0x35/0x80
> [  974.258361]  entry_SYSCALL_64_after_hwframe+0x46/0xb0

Ming, this also looks like ublk doesn't always honor
IO_URING_F_NONBLOCK, we can't be sleeping like that under issue. Then it
should be bounced with -EAGAIN and retried from an io-wq worker.

-- 
Jens Axboe

Re: Potential hang on ublk_ctrl_del_dev()

[Ming Lei]

On Tue, Jan 03, 2023 at 01:47:37PM -0800, Nadav Amit wrote:
> Hello Ming,
> 
> I am trying the ublk and it seems very exciting.
> 
> However, I encounter an issue when I remove a ublk device that is mounted or
> in use.
> 
> In ublk_ctrl_del_dev(), shouldn’t we *not* wait if ublk_idr_freed() is false?
> It seems to me that it is saner to return -EBUSY in such a case and let
> userspace deal with the results.
> 
> For instance, if I run the following (using ubdsrv):
> 
>  $ mkfs.ext4 /dev/ram0
>  $ ./ublk add -t loop -f /dev/ram0
>  $ sudo mount /dev/ublkb0 tmp
>  $ sudo ./ublk del -a
> 
> ublk_ctrl_del_dev() would not be done until the partition is unmounted, and you
> can get a splat that is similar to the one below.

The splat itself can be avoided easily by replace wait_event with
wait_event_timeout() plus loop, but I guess you think the sync delete
isn't good too?

> 
> What do you say? Would you agree to change the behavior to return -EBUSY?

It is designed in this way from beginning, and I believe it is just for
the sake of simplicity, and one point is that the device number needs
to be freed after 'ublk del' returns.

But if it isn't friendly from user's viewpoint, we can change to return
-EBUSY. One simple solution is to check if the ublk block device
is opened before running any deletion action, if yes, stop to delete it
and return -EBUSY; otherwise go ahead and stop & delete the pair of devices.
And the userspace part(ublk utility) needs update too.

However, -EBUSY isn't perfect too, cause user has to retry the delete
command manually.

thanks,
Ming

Re: Potential hang on ublk_ctrl_del_dev()

[Ming Lei]

On Tue, Jan 03, 2023 at 02:51:20PM -0700, Jens Axboe wrote:
> On 1/3/23 2:47?PM, Nadav Amit wrote:
> > Hello Ming,
> > 
> > I am trying the ublk and it seems very exciting.
> > 
> > However, I encounter an issue when I remove a ublk device that is mounted or
> > in use.
> > 
> > In ublk_ctrl_del_dev(), shouldn?t we *not* wait if ublk_idr_freed() is false?
> > It seems to me that it is saner to return -EBUSY in such a case and let
> > userspace deal with the results.
> > 
> > For instance, if I run the following (using ubdsrv):
> > 
> >  $ mkfs.ext4 /dev/ram0
> >  $ ./ublk add -t loop -f /dev/ram0
> >  $ sudo mount /dev/ublkb0 tmp
> >  $ sudo ./ublk del -a
> > 
> > ublk_ctrl_del_dev() would not be done until the partition is unmounted, and you
> > can get a splat that is similar to the one below.
> > 
> > What do you say? Would you agree to change the behavior to return -EBUSY?
> > 
> > Thanks,
> > Nadav
> > 
> > 
> > [  974.149938] INFO: task ublk:2250 blocked for more than 120 seconds.
> > [  974.157786]       Not tainted 6.1.0 #30
> > [  974.162369] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
> > [  974.171417] task:ublk            state:D stack:0     pid:2250  ppid:2249   flags:0x00004004
> > [  974.181054] Call Trace:
> > [  974.184097]  <TASK>
> > [  974.186726]  __schedule+0x37e/0xe10
> > [  974.190915]  ? __this_cpu_preempt_check+0x13/0x20
> > [  974.196463]  ? lock_release+0x133/0x2a0
> > [  974.201043]  schedule+0x67/0xe0
> > [  974.204846]  ublk_ctrl_uring_cmd+0xf45/0x1110
> > [  974.210016]  ? lock_is_held_type+0xdd/0x130
> > [  974.214990]  ? var_wake_function+0x60/0x60
> > [  974.219872]  ? rcu_read_lock_sched_held+0x4f/0x80
> > [  974.225443]  io_uring_cmd+0x9a/0x130
> > [  974.229743]  ? io_uring_cmd_prep+0xf0/0xf0
> > [  974.234638]  io_issue_sqe+0xfe/0x340
> > [  974.238946]  io_submit_sqes+0x231/0x750
> > [  974.243553]  __x64_sys_io_uring_enter+0x22b/0x640
> > [  974.249134]  ? trace_hardirqs_on+0x3c/0xe0
> > [  974.254042]  do_syscall_64+0x35/0x80
> > [  974.258361]  entry_SYSCALL_64_after_hwframe+0x46/0xb0
> 
> Ming, this also looks like ublk doesn't always honor
> IO_URING_F_NONBLOCK, we can't be sleeping like that under issue. Then it
> should be bounced with -EAGAIN and retried from an io-wq worker.

Yeah, you are right, and looks the following change is needed and all
ublk control commands are actually handled in sync style from userspace.

diff --git a/drivers/block/ublk_drv.c b/drivers/block/ublk_drv.c
index 144eda037646..8011ae1f20d5 100644
--- a/drivers/block/ublk_drv.c
+++ b/drivers/block/ublk_drv.c
@@ -2264,6 +2264,9 @@ static int ublk_ctrl_uring_cmd(struct io_uring_cmd *cmd,
 	struct ublk_device *ub = NULL;
 	int ret = -EINVAL;
 
+	if (issue_flags & IO_URING_F_NONBLOCK)
+		return -EAGAIN;
+
 	ublk_ctrl_cmd_dump(cmd);
 
 	if (!(issue_flags & IO_URING_F_SQE128))

thanks,
Ming

Re: Potential hang on ublk_ctrl_del_dev()

[Nadav Amit]

> On Jan 3, 2023, at 9:42 PM, Ming Lei <ming.lei@redhat.com> wrote:
> 
> On Tue, Jan 03, 2023 at 01:47:37PM -0800, Nadav Amit wrote:
>> Hello Ming,
>> 
>> I am trying the ublk and it seems very exciting.
>> 
>> However, I encounter an issue when I remove a ublk device that is mounted or
>> in use.
>> 
>> In ublk_ctrl_del_dev(), shouldn’t we *not* wait if ublk_idr_freed() is false?
>> It seems to me that it is saner to return -EBUSY in such a case and let
>> userspace deal with the results.
>> 
>> For instance, if I run the following (using ubdsrv):
>> 
>> $ mkfs.ext4 /dev/ram0
>> $ ./ublk add -t loop -f /dev/ram0
>> $ sudo mount /dev/ublkb0 tmp
>> $ sudo ./ublk del -a
>> 
>> ublk_ctrl_del_dev() would not be done until the partition is unmounted, and you
>> can get a splat that is similar to the one below.
> 
> The splat itself can be avoided easily by replace wait_event with
> wait_event_timeout() plus loop, but I guess you think the sync delete
> isn't good too?

I don’t think the splat is the issue. The issue is the blocking behavior,
which is both unconditional and unbounded in time, and (worse) takes place
without relinquishing the locks. wait_event_timeout() is therefore not a
valid solution IMHO.

> 
>> 
>> What do you say? Would you agree to change the behavior to return -EBUSY?
> 
> It is designed in this way from beginning, and I believe it is just for
> the sake of simplicity, and one point is that the device number needs
> to be freed after 'ublk del' returns.
> 
> But if it isn't friendly from user's viewpoint, we can change to return
> -EBUSY. One simple solution is to check if the ublk block device
> is opened before running any deletion action, if yes, stop to delete it
> and return -EBUSY; otherwise go ahead and stop & delete the pair of devices.
> And the userspace part(ublk utility) needs update too.
> 
> However, -EBUSY isn't perfect too, cause user has to retry the delete
> command manually.

I understand your considerations. My intuition is that just as umount
cannot be done while a file is opened and would return -EBUSY, so should
deleting the ublock while the ublk is in use.

So as I see it, there are 2 possible options for proper deletion of ublk,
and actually both can be implemented and distinguished with a new flag
(UBLK_F_*):

1. Blocking - similar to the way it is done today, but (hopefully) without
   holding locks, and with using wait_event_interruptible() instead of
   wait_event() to allow interruption (and return EINTR if interrupted).

2. Best-effort - returning EBUSY if it cannot be removed.

I can imagine use-cases for both, and it would also allow you not to
change ubdsrv if you choose so.

Does it make sense?

Re: Potential hang on ublk_ctrl_del_dev()

[Ming Lei]

On Wed, Jan 04, 2023 at 10:13:05AM -0800, Nadav Amit wrote:
> 
> 
> > On Jan 3, 2023, at 9:42 PM, Ming Lei <ming.lei@redhat.com> wrote:
> > 
> > On Tue, Jan 03, 2023 at 01:47:37PM -0800, Nadav Amit wrote:
> >> Hello Ming,
> >> 
> >> I am trying the ublk and it seems very exciting.
> >> 
> >> However, I encounter an issue when I remove a ublk device that is mounted or
> >> in use.
> >> 
> >> In ublk_ctrl_del_dev(), shouldn’t we *not* wait if ublk_idr_freed() is false?
> >> It seems to me that it is saner to return -EBUSY in such a case and let
> >> userspace deal with the results.
> >> 
> >> For instance, if I run the following (using ubdsrv):
> >> 
> >> $ mkfs.ext4 /dev/ram0
> >> $ ./ublk add -t loop -f /dev/ram0
> >> $ sudo mount /dev/ublkb0 tmp
> >> $ sudo ./ublk del -a
> >> 
> >> ublk_ctrl_del_dev() would not be done until the partition is unmounted, and you
> >> can get a splat that is similar to the one below.
> > 
> > The splat itself can be avoided easily by replace wait_event with
> > wait_event_timeout() plus loop, but I guess you think the sync delete
> > isn't good too?
> 
> I don’t think the splat is the issue. The issue is the blocking behavior,
> which is both unconditional and unbounded in time, and (worse) takes place
> without relinquishing the locks. wait_event_timeout() is therefore not a
> valid solution IMHO.
> 
> > 
> >> 
> >> What do you say? Would you agree to change the behavior to return -EBUSY?
> > 
> > It is designed in this way from beginning, and I believe it is just for
> > the sake of simplicity, and one point is that the device number needs
> > to be freed after 'ublk del' returns.
> > 
> > But if it isn't friendly from user's viewpoint, we can change to return
> > -EBUSY. One simple solution is to check if the ublk block device
> > is opened before running any deletion action, if yes, stop to delete it
> > and return -EBUSY; otherwise go ahead and stop & delete the pair of devices.
> > And the userspace part(ublk utility) needs update too.
> > 
> > However, -EBUSY isn't perfect too, cause user has to retry the delete
> > command manually.
> 
> I understand your considerations. My intuition is that just as umount
> cannot be done while a file is opened and would return -EBUSY, so should
> deleting the ublock while the ublk is in use.
> 
> So as I see it, there are 2 possible options for proper deletion of ublk,
> and actually both can be implemented and distinguished with a new flag
> (UBLK_F_*):
> 
> 1. Blocking - similar to the way it is done today, but (hopefully) without
>    holding locks, and with using wait_event_interruptible() instead of
>    wait_event() to allow interruption (and return EINTR if interrupted).
> 
> 2. Best-effort - returning EBUSY if it cannot be removed.
> 
> I can imagine use-cases for both, and it would also allow you not to
> change ubdsrv if you choose so.
> 
> Does it make sense?
 
I prefer to the 1st approach:

1) the wait event is still one positive signal for user to cleanup the
device use, since the correct step is to umount ublk disk before deleting
the device.

2) the wait still can avoid the current context to reuse the device
number

3) after switching to wait_event_interruptible(), we need to avoid
double delete, and one flag of UB_STATE_DELETED can be used for failing
new delete command.

4) IMO new flag(UBLK_F_*) isn't needed to distinguish this change
with current behavior.

Please let us know if you'd like to cook one patch for improving
the delete handling.

BTW, there could be another option, such as, 'ublk delete --no-wait' just
run the remove and without waiting at all, but not sure if it is useful.


thanks,
Ming

Re: Potential hang on ublk_ctrl_del_dev()

[Nadav Amit]

> On Jan 4, 2023, at 7:16 PM, Ming Lei <ming.lei@redhat.com> wrote:
> 
> On Wed, Jan 04, 2023 at 10:13:05AM -0800, Nadav Amit wrote:
>> 
>> 
>>> On Jan 3, 2023, at 9:42 PM, Ming Lei <ming.lei@redhat.com> wrote:
>>> 
>>> On Tue, Jan 03, 2023 at 01:47:37PM -0800, Nadav Amit wrote:
>>>> Hello Ming,
>>>> 
>>>> I am trying the ublk and it seems very exciting.
>>>> 
>>>> However, I encounter an issue when I remove a ublk device that is mounted or
>>>> in use.
>>>> 
>>>> In ublk_ctrl_del_dev(), shouldn’t we *not* wait if ublk_idr_freed() is false?
>>>> It seems to me that it is saner to return -EBUSY in such a case and let
>>>> userspace deal with the results.
>>>> 
>>>> For instance, if I run the following (using ubdsrv):
>>>> 
>>>> $ mkfs.ext4 /dev/ram0
>>>> $ ./ublk add -t loop -f /dev/ram0
>>>> $ sudo mount /dev/ublkb0 tmp
>>>> $ sudo ./ublk del -a
>>>> 
>>>> ublk_ctrl_del_dev() would not be done until the partition is unmounted, and you
>>>> can get a splat that is similar to the one below.
>>> 
>>> The splat itself can be avoided easily by replace wait_event with
>>> wait_event_timeout() plus loop, but I guess you think the sync delete
>>> isn't good too?
>> 
>> I don’t think the splat is the issue. The issue is the blocking behavior,
>> which is both unconditional and unbounded in time, and (worse) takes place
>> without relinquishing the locks. wait_event_timeout() is therefore not a
>> valid solution IMHO.
>> 
>>> 
>>>> 
>>>> What do you say? Would you agree to change the behavior to return -EBUSY?
>>> 
>>> It is designed in this way from beginning, and I believe it is just for
>>> the sake of simplicity, and one point is that the device number needs
>>> to be freed after 'ublk del' returns.
>>> 
>>> But if it isn't friendly from user's viewpoint, we can change to return
>>> -EBUSY. One simple solution is to check if the ublk block device
>>> is opened before running any deletion action, if yes, stop to delete it
>>> and return -EBUSY; otherwise go ahead and stop & delete the pair of devices.
>>> And the userspace part(ublk utility) needs update too.
>>> 
>>> However, -EBUSY isn't perfect too, cause user has to retry the delete
>>> command manually.
>> 
>> I understand your considerations. My intuition is that just as umount
>> cannot be done while a file is opened and would return -EBUSY, so should
>> deleting the ublock while the ublk is in use.
>> 
>> So as I see it, there are 2 possible options for proper deletion of ublk,
>> and actually both can be implemented and distinguished with a new flag
>> (UBLK_F_*):
>> 
>> 1. Blocking - similar to the way it is done today, but (hopefully) without
>>   holding locks, and with using wait_event_interruptible() instead of
>>   wait_event() to allow interruption (and return EINTR if interrupted).
>> 
>> 2. Best-effort - returning EBUSY if it cannot be removed.
>> 
>> I can imagine use-cases for both, and it would also allow you not to
>> change ubdsrv if you choose so.
>> 
>> Does it make sense?
> 
> I prefer to the 1st approach:
> 
> 1) the wait event is still one positive signal for user to cleanup the
> device use, since the correct step is to umount ublk disk before deleting
> the device.
> 
> 2) the wait still can avoid the current context to reuse the device
> number
> 
> 3) after switching to wait_event_interruptible(), we need to avoid
> double delete, and one flag of UB_STATE_DELETED can be used for failing
> new delete command.
> 
> 4) IMO new flag(UBLK_F_*) isn't needed to distinguish this change
> with current behavior.
> 
> Please let us know if you'd like to cook one patch for improving
> the delete handling.

I can take a stab on it, but only in about 2 weeks time.

> 
> BTW, there could be another option, such as, 'ublk delete --no-wait' just
> run the remove and without waiting at all, but not sure if it is useful.
> 

I considered the userspace ublk as one possible implementation. I am not
sure this affects the kernel interfaces that are needed.

Re: Potential hang on ublk_ctrl_del_dev()

[Ming Lei]

On Thu, Jan 05, 2023 at 09:52:00AM -0800, Nadav Amit wrote:
> 
> 
> > On Jan 4, 2023, at 7:16 PM, Ming Lei <ming.lei@redhat.com> wrote:
> > 
> > On Wed, Jan 04, 2023 at 10:13:05AM -0800, Nadav Amit wrote:
> >> 
> >> 
> >>> On Jan 3, 2023, at 9:42 PM, Ming Lei <ming.lei@redhat.com> wrote:
> >>> 
> >>> On Tue, Jan 03, 2023 at 01:47:37PM -0800, Nadav Amit wrote:
> >>>> Hello Ming,
> >>>> 
> >>>> I am trying the ublk and it seems very exciting.
> >>>> 
> >>>> However, I encounter an issue when I remove a ublk device that is mounted or
> >>>> in use.
> >>>> 
> >>>> In ublk_ctrl_del_dev(), shouldn’t we *not* wait if ublk_idr_freed() is false?
> >>>> It seems to me that it is saner to return -EBUSY in such a case and let
> >>>> userspace deal with the results.
> >>>> 
> >>>> For instance, if I run the following (using ubdsrv):
> >>>> 
> >>>> $ mkfs.ext4 /dev/ram0
> >>>> $ ./ublk add -t loop -f /dev/ram0
> >>>> $ sudo mount /dev/ublkb0 tmp
> >>>> $ sudo ./ublk del -a
> >>>> 
> >>>> ublk_ctrl_del_dev() would not be done until the partition is unmounted, and you
> >>>> can get a splat that is similar to the one below.
> >>> 
> >>> The splat itself can be avoided easily by replace wait_event with
> >>> wait_event_timeout() plus loop, but I guess you think the sync delete
> >>> isn't good too?
> >> 
> >> I don’t think the splat is the issue. The issue is the blocking behavior,
> >> which is both unconditional and unbounded in time, and (worse) takes place
> >> without relinquishing the locks. wait_event_timeout() is therefore not a
> >> valid solution IMHO.
> >> 
> >>> 
> >>>> 
> >>>> What do you say? Would you agree to change the behavior to return -EBUSY?
> >>> 
> >>> It is designed in this way from beginning, and I believe it is just for
> >>> the sake of simplicity, and one point is that the device number needs
> >>> to be freed after 'ublk del' returns.
> >>> 
> >>> But if it isn't friendly from user's viewpoint, we can change to return
> >>> -EBUSY. One simple solution is to check if the ublk block device
> >>> is opened before running any deletion action, if yes, stop to delete it
> >>> and return -EBUSY; otherwise go ahead and stop & delete the pair of devices.
> >>> And the userspace part(ublk utility) needs update too.
> >>> 
> >>> However, -EBUSY isn't perfect too, cause user has to retry the delete
> >>> command manually.
> >> 
> >> I understand your considerations. My intuition is that just as umount
> >> cannot be done while a file is opened and would return -EBUSY, so should
> >> deleting the ublock while the ublk is in use.
> >> 
> >> So as I see it, there are 2 possible options for proper deletion of ublk,
> >> and actually both can be implemented and distinguished with a new flag
> >> (UBLK_F_*):
> >> 
> >> 1. Blocking - similar to the way it is done today, but (hopefully) without
> >>   holding locks, and with using wait_event_interruptible() instead of
> >>   wait_event() to allow interruption (and return EINTR if interrupted).
> >> 
> >> 2. Best-effort - returning EBUSY if it cannot be removed.
> >> 
> >> I can imagine use-cases for both, and it would also allow you not to
> >> change ubdsrv if you choose so.
> >> 
> >> Does it make sense?
> > 
> > I prefer to the 1st approach:
> > 
> > 1) the wait event is still one positive signal for user to cleanup the
> > device use, since the correct step is to umount ublk disk before deleting
> > the device.
> > 
> > 2) the wait still can avoid the current context to reuse the device
> > number
> > 
> > 3) after switching to wait_event_interruptible(), we need to avoid
> > double delete, and one flag of UB_STATE_DELETED can be used for failing
> > new delete command.
> > 
> > 4) IMO new flag(UBLK_F_*) isn't needed to distinguish this change
> > with current behavior.
> > 
> > Please let us know if you'd like to cook one patch for improving
> > the delete handling.
> 
> I can take a stab on it, but only in about 2 weeks time.
> 
> > 
> > BTW, there could be another option, such as, 'ublk delete --no-wait' just
> > run the remove and without waiting at all, but not sure if it is useful.
> > 
> 
> I considered the userspace ublk as one possible implementation. I am not
> sure this affects the kernel interfaces that are needed.

ublk driver needs to be told if --no-wait is applied, so either one
UBLK_F_* or a parameter of UBLK_CMD_DEL_DEV is needed.


Thanks,
Ming