Re: [PATCH] loop: replace loop_ctl_mutex with loop_idr_spinlock

[Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp>]

On 2021/08/28 10:10, Tetsuo Handa wrote:
> If we don't ignore forced module unload, we could update my patch to keep only
> mutex_destroy() and kfree() deferred by a refcount, for only lo->lo_state,
> lo->lo_refcnt and lo->lo_encryption would be accessed under lo->lo_mutex
> serialization. There is no need to defer "del_gendisk() + idr_remove()"
> sequence for concurrent callers.
> 

OK, here is a delta patch to make it no longer best effort.
We can consider removal of cryptoloop module after this patch,
starting from a printk() for deprecated message.

 drivers/block/loop.c | 31 +++++++++++++------------------
 1 file changed, 13 insertions(+), 18 deletions(-)

--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -2113,7 +2113,11 @@ int loop_register_transfer(struct loop_func_table *funcs)
 	return 0;
 }
 
-static void loop_remove(struct loop_device *lo);
+static void loop_destroy(struct loop_device *lo)
+{
+	mutex_destroy(&lo->lo_mutex);
+	kfree(lo);
+}
 
 int loop_unregister_transfer(int number)
 {
@@ -2137,7 +2141,7 @@ int loop_unregister_transfer(int number)
 			loop_release_xfer(lo);
 		mutex_unlock(&lo->lo_mutex);
 		if (refcount_dec_and_test(&lo->idr_visible))
-			loop_remove(lo);
+			loop_destroy(lo);
 		spin_lock(&loop_idr_spinlock);
 	}
 	spin_unlock(&loop_idr_spinlock);
@@ -2426,9 +2430,6 @@ static void loop_remove(struct loop_device *lo)
 	spin_lock(&loop_idr_spinlock);
 	idr_remove(&loop_index_idr, lo->lo_number);
 	spin_unlock(&loop_idr_spinlock);
-	/* There is no route which can find this loop device. */
-	mutex_destroy(&lo->lo_mutex);
-	kfree(lo);
 }
 
 static void loop_probe(dev_t dev)
@@ -2452,7 +2453,7 @@ static int loop_control_remove(int idx)
 
 	/*
 	 * Identify the loop device to remove. Skip the device if it is owned by
-	 * loop_remove()/loop_add() where it is not safe to access lo_mutex.
+	 * loop_add() where it is not safe to access lo_mutex.
 	 */
 	spin_lock(&loop_idr_spinlock);
 	lo = idr_find(&loop_index_idr, idx);
@@ -2479,19 +2480,11 @@ static int loop_control_remove(int idx)
 	mutex_unlock(&lo->lo_mutex);
 	/* Hide this loop device. */
 	refcount_dec(&lo->idr_visible);
-	/*
-	 * Try to wait for concurrent callers (they should complete shortly due to
-	 * lo->lo_state == Lo_deleting) operating on this loop device, in order to
-	 * help that subsequent loop_add() will not to fail with -EEXIST.
-	 * Note that this is best effort.
-	 */
-	for (ret = 0; refcount_read(&lo->idr_visible) != 1 && ret < HZ; ret++)
-		schedule_timeout_killable(1);
-	ret = 0;
+	/* Remove this loop device, but wait concurrent callers before destroy. */
+	loop_remove(lo);
 out:
-	/* Remove this loop device. */
 	if (refcount_dec_and_test(&lo->idr_visible))
-		loop_remove(lo);
+		loop_destroy(lo);
 	return ret;
 }
 
@@ -2623,8 +2616,10 @@ static void __exit loop_exit(void)
 	 * There is no need to use loop_idr_spinlock here, for nobody else can
 	 * access loop_index_idr when this module is unloading.
 	 */
-	idr_for_each_entry(&loop_index_idr, lo, id)
+	idr_for_each_entry(&loop_index_idr, lo, id) {
 		loop_remove(lo);
+		loop_destroy(lo);
+	}
 
 	idr_destroy(&loop_index_idr);
 }
-- 
2.25.1