Re: [PATCH v2 02/12] loop: Remove the "bool global" function argument

On 7/31/26 1:28 AM, Bart Van Assche wrote:
> Move the code that is protected by a mutex from loop_change_fd() into
> __loop_change_fd(). Move the code that is protected by a mutex from
> loop_configure() into __loop_configure(). Keep the behavior in
> loop_global_lock_killable() for the global == true case. Expand
> loop_global_lock_killable(lo, false) calls into a mutex_lock_killable()
> and a mutex_unlock() call. This patch prepares for adding lock context
> annotations.
> 
> Cc: Haris Iqbal <[email protected]>
> Signed-off-by: Bart Van Assche <[email protected]>
> ---
>   drivers/block/loop.c | 249 ++++++++++++++++++++++++-------------------
>   1 file changed, 138 insertions(+), 111 deletions(-)
> 
> diff --git a/drivers/block/loop.c b/drivers/block/loop.c
> index 1faecef33009..a71fe763c933 100644
> --- a/drivers/block/loop.c
> +++ b/drivers/block/loop.c
> @@ -98,25 +98,22 @@ static DEFINE_MUTEX(loop_validate_mutex);
>    * loop_global_lock_killable() - take locks for safe loop_validate_file() test
>    *
>    * @lo: struct loop_device
> - * @global: true if @lo is about to bind another "struct loop_device", false otherwise
>    *
>    * Returns 0 on success, -EINTR otherwise.
>    *
> - * Since loop_validate_file() traverses on other "struct loop_device" if
> - * is_loop_device() is true, we need a global lock for serializing concurrent
> + * Since loop_validate_file() traverses on other "struct loop_device", we need a
> + * global lock for serializing concurrent
>    * loop_configure()/loop_change_fd()/__loop_clr_fd() calls.
>    */
> -static int loop_global_lock_killable(struct loop_device *lo, bool global)
> +static int loop_global_lock_killable(struct loop_device *lo)
>   {
>   	int err;
>   
> -	if (global) {
> -		err = mutex_lock_killable(&loop_validate_mutex);
> -		if (err)
> -			return err;
> -	}
> +	err = mutex_lock_killable(&loop_validate_mutex);
> +	if (err)
> +		return err;
>   	err = mutex_lock_killable(&lo->lo_mutex);
> -	if (err && global)
> +	if (err)
>   		mutex_unlock(&loop_validate_mutex);
>   	return err;
>   }
> @@ -125,13 +122,11 @@ static int loop_global_lock_killable(struct loop_device *lo, bool global)
>    * loop_global_unlock() - release locks taken by loop_global_lock_killable()
>    *
>    * @lo: struct loop_device
> - * @global: true if @lo was about to bind another "struct loop_device", false otherwise
>    */
> -static void loop_global_unlock(struct loop_device *lo, bool global)
> +static void loop_global_unlock(struct loop_device *lo)
>   {
>   	mutex_unlock(&lo->lo_mutex);
> -	if (global)
> -		mutex_unlock(&loop_validate_mutex);
> +	mutex_unlock(&loop_validate_mutex);
>   }
>   
>   static int max_part;
> @@ -523,6 +518,50 @@ static int loop_check_backing_file(struct file *file)
>   	return 0;
>   }
>   
> +static int __loop_change_fd(struct loop_device *lo, struct block_device *bdev,
> +			    struct file *file, struct file **old_file,
> +			    bool *partscan)
> +	__must_hold(&lo->lo_mutex)
> +{
> +	unsigned int memflags;
> +	int error;
> +
> +	if (lo->lo_state != Lo_bound)
> +		return -ENXIO;
> +
> +	/* the loop device has to be read-only */
> +	if (!(lo->lo_flags & LO_FLAGS_READ_ONLY))
> +		return -EINVAL;
> +
> +	error = loop_validate_file(file, bdev);
> +	if (error)
> +		return error;
> +
> +	*old_file = lo->lo_backing_file;
> +
> +	/* size of the new backing store needs to be the same */
> +	if (lo_calculate_size(lo, file) != lo_calculate_size(lo, *old_file))
> +		return -EINVAL;
> +
> +	/*
> +	 * We might switch to direct I/O mode for the loop device, write back
> +	 * all dirty data the page cache now that so that the individual I/O
> +	 * operations don't have to do that.
> +	 */
> +	vfs_fsync(file, 0);
> +
> +	/* and ... switch */
> +	disk_force_media_change(lo->lo_disk);
> +	memflags = blk_mq_freeze_queue(lo->lo_queue);
> +	mapping_set_gfp_mask((*old_file)->f_mapping, lo->old_gfp_mask);
> +	loop_assign_backing_file(lo, file);
> +	loop_update_dio(lo);
> +	blk_mq_unfreeze_queue(lo->lo_queue, memflags);
> +	*partscan = lo->lo_flags & LO_FLAGS_PARTSCAN;
> +
> +	return 0;
> +}
> +
>   /*
>    * loop_change_fd switched the backing store of a loopback device to
>    * a new file. This is useful for operating system installers to free up
> @@ -536,7 +575,6 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
>   {
>   	struct file *file = fget(arg);
>   	struct file *old_file;
> -	unsigned int memflags;
>   	int error;
>   	bool partscan;
>   	bool is_loop;
> @@ -554,46 +592,21 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
>   	dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 1);
>   
>   	is_loop = is_loop_device(file);
> -	error = loop_global_lock_killable(lo, is_loop);
> +	if (is_loop) {
> +		error = loop_global_lock_killable(lo);
> +		if (error)
> +			goto out_putf;
> +		error = __loop_change_fd(lo, bdev, file, &old_file, &partscan);
> +		loop_global_unlock(lo);
> +	} else {
> +		error = mutex_lock_killable(&lo->lo_mutex);
> +		if (error)
> +			goto out_putf;
> +		error = __loop_change_fd(lo, bdev, file, &old_file, &partscan);
> +		mutex_unlock(&lo->lo_mutex);
> +	}
>   	if (error)
>   		goto out_putf;
> -	error = -ENXIO;
> -	if (lo->lo_state != Lo_bound)
> -		goto out_err;
> -
> -	/* the loop device has to be read-only */
> -	error = -EINVAL;
> -	if (!(lo->lo_flags & LO_FLAGS_READ_ONLY))
> -		goto out_err;
> -
> -	error = loop_validate_file(file, bdev);
> -	if (error)
> -		goto out_err;
> -
> -	old_file = lo->lo_backing_file;
> -
> -	error = -EINVAL;
> -
> -	/* size of the new backing store needs to be the same */
> -	if (lo_calculate_size(lo, file) != lo_calculate_size(lo, old_file))
> -		goto out_err;
> -
> -	/*
> -	 * We might switch to direct I/O mode for the loop device, write back
> -	 * all dirty data the page cache now that so that the individual I/O
> -	 * operations don't have to do that.
> -	 */
> -	vfs_fsync(file, 0);
> -
> -	/* and ... switch */
> -	disk_force_media_change(lo->lo_disk);
> -	memflags = blk_mq_freeze_queue(lo->lo_queue);
> -	mapping_set_gfp_mask(old_file->f_mapping, lo->old_gfp_mask);
> -	loop_assign_backing_file(lo, file);
> -	loop_update_dio(lo);
> -	blk_mq_unfreeze_queue(lo->lo_queue, memflags);
> -	partscan = lo->lo_flags & LO_FLAGS_PARTSCAN;
> -	loop_global_unlock(lo, is_loop);
>   
>   	/*
>   	 * Flush loop_validate_file() before fput(), for l->lo_backing_file
> @@ -618,8 +631,6 @@ static int loop_change_fd(struct loop_device *lo, struct block_device *bdev,
>   	kobject_uevent(&disk_to_dev(lo->lo_disk)->kobj, KOBJ_CHANGE);
>   	return error;
>   
> -out_err:
> -	loop_global_unlock(lo, is_loop);
>   out_putf:
>   	fput(file);
>   	dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 0);
> @@ -974,61 +985,29 @@ static void loop_update_limits(struct loop_device *lo, struct queue_limits *lim,
>   		lim->discard_granularity = 0;
>   }
>   
> -static int loop_configure(struct loop_device *lo, blk_mode_t mode,
> -			  struct block_device *bdev,
> -			  const struct loop_config *config)
> +static int __loop_configure(struct loop_device *lo, blk_mode_t mode,
> +			    struct block_device *bdev,
> +			    const struct loop_config *config, struct file *file,
> +			    bool *partscan)
> +	__must_hold(&lo->lo_mutex)
>   {
> -	struct file *file = fget(config->fd);
>   	struct queue_limits lim;
> -	int error;
>   	loff_t size;
> -	bool partscan;
> -	bool is_loop;
> -
> -	if (!file)
> -		return -EBADF;
> -
> -	error = loop_check_backing_file(file);
> -	if (error) {
> -		fput(file);
> -		return error;
> -	}
> -
> -	is_loop = is_loop_device(file);
> -
> -	/* This is safe, since we have a reference from open(). */
> -	__module_get(THIS_MODULE);
> -
> -	/*
> -	 * If we don't hold exclusive handle for the device, upgrade to it
> -	 * here to avoid changing device under exclusive owner.
> -	 */
> -	if (!(mode & BLK_OPEN_EXCL)) {
> -		error = bd_prepare_to_claim(bdev, loop_configure, NULL);
> -		if (error)
> -			goto out_putf;
> -	}
> -
> -	error = loop_global_lock_killable(lo, is_loop);
> -	if (error)
> -		goto out_bdev;
> +	int error;
>   
> -	error = -EBUSY;
>   	if (lo->lo_state != Lo_unbound)
> -		goto out_unlock;
> +		return -EBUSY;
>   
>   	error = loop_validate_file(file, bdev);
>   	if (error)
> -		goto out_unlock;
> +		return error;
>   
> -	if ((config->info.lo_flags & ~LOOP_CONFIGURE_SETTABLE_FLAGS) != 0) {
> -		error = -EINVAL;
> -		goto out_unlock;
> -	}
> +	if ((config->info.lo_flags & ~LOOP_CONFIGURE_SETTABLE_FLAGS) != 0)
> +		return -EINVAL;
>   
>   	error = loop_set_status_from_info(lo, &config->info);
>   	if (error)
> -		goto out_unlock;
> +		return error;
>   	lo->lo_flags = config->info.lo_flags;
>   
>   	if (!(file->f_mode & FMODE_WRITE) || !(mode & BLK_OPEN_WRITE) ||
> @@ -1039,10 +1018,8 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
>   		lo->workqueue = alloc_workqueue("loop%d",
>   						WQ_UNBOUND | WQ_FREEZABLE,
>   						0, lo->lo_number);
> -		if (!lo->workqueue) {
> -			error = -ENOMEM;
> -			goto out_unlock;
> -		}
> +		if (!lo->workqueue)
> +			return -ENOMEM;
>   	}
>   
>   	/* suppress uevents while reconfiguring the device */
> @@ -1059,7 +1036,7 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
>   	/* No need to freeze the queue as the device isn't bound yet. */
>   	error = queue_limits_commit_update(lo->lo_queue, &lim);
>   	if (error)
> -		goto out_unlock;
> +		return error;
>   
>   	/*
>   	 * We might switch to direct I/O mode for the loop device, write back
> @@ -1080,14 +1057,66 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
>   	WRITE_ONCE(lo->lo_state, Lo_bound);
>   	if (part_shift)
>   		lo->lo_flags |= LO_FLAGS_PARTSCAN;
> -	partscan = lo->lo_flags & LO_FLAGS_PARTSCAN;
> -	if (partscan)
> +	*partscan = lo->lo_flags & LO_FLAGS_PARTSCAN;
> +	if (*partscan)
>   		clear_bit(GD_SUPPRESS_PART_SCAN, &lo->lo_disk->state);
>   
>   	dev_set_uevent_suppress(disk_to_dev(lo->lo_disk), 0);
>   	kobject_uevent(&disk_to_dev(lo->lo_disk)->kobj, KOBJ_CHANGE);
>   
> -	loop_global_unlock(lo, is_loop);
> +	return 0;
> +}
> +
> +static int loop_configure(struct loop_device *lo, blk_mode_t mode,
> +			  struct block_device *bdev,
> +			  const struct loop_config *config)
> +{
> +	struct file *file = fget(config->fd);
> +	int error;
> +	bool partscan;
> +	bool is_loop;
> +
> +	if (!file)
> +		return -EBADF;
> +
> +	error = loop_check_backing_file(file);
> +	if (error) {
> +		fput(file);
> +		return error;
> +	}
> +
> +	is_loop = is_loop_device(file);
> +
> +	/* This is safe, since we have a reference from open(). */
> +	__module_get(THIS_MODULE);
> +
> +	/*
> +	 * If we don't hold exclusive handle for the device, upgrade to it
> +	 * here to avoid changing device under exclusive owner.
> +	 */
> +	if (!(mode & BLK_OPEN_EXCL)) {
> +		error = bd_prepare_to_claim(bdev, loop_configure, NULL);
> +		if (error)
> +			goto out_putf;
> +	}
> +
> +	if (is_loop) {
> +		error = loop_global_lock_killable(lo);
> +		if (error)
> +			goto out_bdev;
> +		error = __loop_configure(lo, mode, bdev, config, file,
> +					 &partscan);
> +		loop_global_unlock(lo);
> +	} else {
> +		error = mutex_lock_killable(&lo->lo_mutex);
> +		if (error)
> +			goto out_bdev;
> +		error = __loop_configure(lo, mode, bdev, config, file,
> +					 &partscan);
> +		mutex_unlock(&lo->lo_mutex);
> +	}
> +	if (error)
> +		goto out_bdev;
>   	if (partscan)
>   		loop_reread_partitions(lo);
>   
> @@ -1096,8 +1125,6 @@ static int loop_configure(struct loop_device *lo, blk_mode_t mode,
>   
>   	return 0;
>   
> -out_unlock:
> -	loop_global_unlock(lo, is_loop);
>   out_bdev:
>   	if (!(mode & BLK_OPEN_EXCL))
>   		bd_abort_claiming(bdev, loop_configure);
> @@ -1194,11 +1221,11 @@ static int loop_clr_fd(struct loop_device *lo)
>   	 * which loop_configure()/loop_change_fd() found via fget() was this
>   	 * loop device.
>   	 */
> -	err = loop_global_lock_killable(lo, true);
> +	err = loop_global_lock_killable(lo);
>   	if (err)
>   		return err;
>   	if (lo->lo_state != Lo_bound) {
> -		loop_global_unlock(lo, true);
> +		loop_global_unlock(lo);
>   		return -ENXIO;
>   	}
>   	/*
> @@ -1210,7 +1237,7 @@ static int loop_clr_fd(struct loop_device *lo)
>   	lo->lo_flags |= LO_FLAGS_AUTOCLEAR;
>   	if (disk_openers(lo->lo_disk) == 1)
>   		WRITE_ONCE(lo->lo_state, Lo_rundown);
> -	loop_global_unlock(lo, true);
> +	loop_global_unlock(lo);
>   
>   	return 0;
>   }

Overall this change looks good to me. But I see, lo->lo_lock
is used to protect lo->lo_backing_file. so shall we annotate
the lo->lo_backing_file using __guarded_by(&lo_lock)?

Thanks,
--Nilay