Re: [PATCH v3 1/1] loop: scale loop device by introducing per device lock

[Chaitanya Kulkarni]

On 1/25/21 12:15 PM, Pavel Tatashin wrote:
> Currently, loop device has only one global lock:
> loop_ctl_mutex.
Above line can be :-
Currently, loop device has only one global lock: loop_ctl_mutex.

Also please provide a complete discretion what are the members it protects,
i.e. how big the size of the current locking is, helps the reviewers &
maintainer.
> This becomes hot in scenarios where many loop devices are used.
>
> Scale it by introducing per-device lock: lo_mutex that protects the
> fields in struct loop_device. Keep loop_ctl_mutex to protect global
> data such as loop_index_idr, loop_lookup, loop_add.
When it comes to scaling, lockstat data is more descriptive and useful along
with thetotal time of execution which has contention numbers with increasing
number of threads/devices/users on logarithmic scale, at-least that is
how I've
solved the some of file-systems scaling issues in the past.
>
> Lock ordering: loop_ctl_mutex > lo_mutex.
The above statement needs a in-detail commit log description. Usually >
sort of statements are not a good practice for something as important as
lock priority which was not present in the original code.
> Signed-off-by: Pavel Tatashin <pasha.tatashin@xxxxxxxxxx>
> Reviewed-by: Tyler Hicks <tyhicks@xxxxxxxxxxxxxxxxxxx>
> ---
>  drivers/block/loop.c | 92 +++++++++++++++++++++++++-------------------
>
>
>  
>  	/*
> -	 * Need not hold loop_ctl_mutex to fput backing file.
> -	 * Calling fput holding loop_ctl_mutex triggers a circular
> +	 * Need not hold lo_mutex to fput backing file.
> +	 * Calling fput holding lo_mutex triggers a circular
>  	 * lock dependency possibility warning as fput can take
> -	 * bd_mutex which is usually taken before loop_ctl_mutex.
> +	 * bd_mutex which is usually taken before lo_mutex.
>  	 */
This is not in your patch, but since you are touching this comment can you
please consider this, it save an entire line and the wasted space:-
       /*  
        * Need not hold lo_mutex to fput backing file. Calling fput holding
        * lo_mutex triggers a circular lock dependency possibility
warning as
        * fput can take bd_mutex which is usually take before lo_mutex.
        */

> @@ -1879,27 +1879,33 @@ static int lo_open(struct block_device *bdev, fmode_t mode)
>  	struct loop_device *lo;
>  	int err;
>  
> +	/*
> +	 * take loop_ctl_mutex to protect lo pointer from race with
> +	 * loop_control_ioctl(LOOP_CTL_REMOVE), however, to reduce
> +	 * contention release it prior to updating lo->lo_refcnt.
> +	 */

The above comment could be :-

        /*  
         * Take loop_ctl_mutex to protect lo pointer from race with
         * loop_control_ioctl(LOOP_CTL_REMOVE), however, to reduce
contention
         * release it prior to updating lo->lo_refcnt.
         */
>  	err = mutex_lock_killable(&loop_ctl_mutex);
>  	if (err)