On Tue, Jan 26, 2021 at 4:53 AM Chaitanya Kulkarni <chaitanya.kulka...@wdc.com> wrote: > > 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.
OK > > 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. Sure > > 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. I found this issue using perf that shows profiling. I've previously used lockstat, it is indeed a good tool to work with lock contentions. > > > > 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. OK, I will expand this to clearly state that new lock ordering requirement is that loop_ctl_mutex must be taken before lo_mutex. > > Signed-off-by: Pavel Tatashin <pasha.tatas...@soleen.com> > > Reviewed-by: Tyler Hicks <tyhi...@linux.microsoft.com> > > --- > > 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:- OK > /* > * 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. > */ OK > > err = mutex_lock_killable(&loop_ctl_mutex); > > if (err) I will send an updated patch soon. Thank you, Pasha
