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)
