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@xxxxxxxxxx> 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
