Hi Tetsuo,
I think this approach is fine, but we can further simplify it.
> + /*
> + * Since ioctl(LOOP_CLR_FD) depends on lo->lo_state == Lo_bound, it is
> + * a sign of something going wrong if lo->lo_backing_file was not
> + * assigned by ioctl(LOOP_SET_FD) or ioctl(LOOP_CONFIGURE).
> + */
> filp = lo->lo_backing_file;
> - if (filp == NULL) {
> - err = -EINVAL;
> - goto out_unlock;
> - }
> + BUG_ON(!filp);
I'd just drop the check here entirely.
> if (test_bit(QUEUE_FLAG_WC, &lo->lo_queue->queue_flags))
> blk_queue_write_cache(lo->lo_queue, false, false);
> @@ -1121,7 +1125,20 @@ static int __loop_clr_fd(struct loop_device *lo, bool release)
> /* freeze request queue during the transition */
> blk_mq_freeze_queue(lo->lo_queue);
>
> + /*
> + * To avoid circular locking dependency, call destroy_workqueue()
> + * without holding lo->lo_mutex.
> + */
> + mutex_unlock(&lo->lo_mutex);
> destroy_workqueue(lo->workqueue);
> + mutex_lock(&lo->lo_mutex);
As far as I can tell there is absolutely no need to hold lo_mutex
above these changes at all, as the Lo_rundown check prevents
access to all the other fields we're changing. So I think we can
drop this entire critical section and just keep the one at the
end of the funtion where lo_state is changed.
