Re: [PATCH 12/13] loop: remove lo_refcount and avoid lo_mutex in ->open / ->release

[Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>]

On 2022/03/25 2:23, Christoph Hellwig wrote:
> On Thu, Mar 24, 2022 at 11:24:43PM +0900, Tetsuo Handa wrote:
>> On 2022/03/24 23:13, Jan Kara wrote:
>>>> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
>>>> index b3170e8cdbe95..e1eb925d3f855 100644
>>>> --- a/drivers/block/loop.c
>>>> +++ b/drivers/block/loop.c
>>>> @@ -1244,7 +1244,7 @@ static int loop_clr_fd(struct loop_device *lo)
>>>>  	 * <dev>/do something like mkfs/losetup -d <dev> causing the losetup -d
>>>>  	 * command to fail with EBUSY.
>>>>  	 */
>>>> -	if (atomic_read(&lo->lo_refcnt) > 1) {
>>>> +	if (disk_openers(lo->lo_disk) > 1) {
>>
>> This is sometimes "if (0) {" due to not holding disk->open_mutex.
> 
> Yes, as clearly documented in the commit log.  In fact turning it
> into an explicit if 0 (that is removing this code) might be a not
> bad idea either - the code was added to work around a
> 
> 	if (lo->lo_refcnt > 1)  /* we needed one fd for the ioctl */
> 		return -EBUSY;
> 
> that already did not make much sense to start with (but goes
> back to before git history).
> 
> But for now I'd really prefer to stop moving the goalpost further and
> further.

Then, why not kill this code?

 drivers/block/loop.c | 29 +++++++----------------------
 1 file changed, 7 insertions(+), 22 deletions(-)

diff --git a/drivers/block/loop.c b/drivers/block/loop.c
index 3e636a75c83a..26c8808d02d0 100644
--- a/drivers/block/loop.c
+++ b/drivers/block/loop.c
@@ -1205,30 +1205,15 @@ static int loop_clr_fd(struct loop_device *lo)
 	err = mutex_lock_killable(&lo->lo_mutex);
 	if (err)
 		return err;
-	if (lo->lo_state != Lo_bound) {
-		mutex_unlock(&lo->lo_mutex);
-		return -ENXIO;
-	}
-	/*
-	 * If we've explicitly asked to tear down the loop device,
-	 * and it has an elevated reference count, set it for auto-teardown when
-	 * the last reference goes away. This stops $!~#$@ udev from
-	 * preventing teardown because it decided that it needs to run blkid on
-	 * the loopback device whenever they appear. xfstests is notorious for
-	 * failing tests because blkid via udev races with a losetup
-	 * <dev>/do something like mkfs/losetup -d <dev> causing the losetup -d
-	 * command to fail with EBUSY.
-	 */
-	if (atomic_read(&lo->lo_refcnt) > 1) {
-		lo->lo_flags |= LO_FLAGS_AUTOCLEAR;
-		mutex_unlock(&lo->lo_mutex);
-		return 0;
-	}
-	lo->lo_state = Lo_rundown;
+	if (lo->lo_state != Lo_bound)
+		err = -ENXIO;
+	else
+		lo->lo_state = Lo_rundown;
 	mutex_unlock(&lo->lo_mutex);
 
-	__loop_clr_fd(lo, false);
-	return 0;
+	if (!err)
+		__loop_clr_fd(lo, false);
+	return err;
 }
 
 static int
-- 
2.32.0