Re: Potential hang on ublk_ctrl_del_dev()

[Ming Lei <ming.lei@redhat.com>]

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