lkml.org 
[lkml]   [2021]   [Jan]   [26]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [PATCH v3 1/1] loop: scale loop device by introducing per device lock
On Tue, Jan 26, 2021 at 4:53 AM Chaitanya Kulkarni
<Chaitanya.Kulkarni@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.tatashin@soleen.com>
> > Reviewed-by: Tyler Hicks <tyhicks@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

\
 
 \ /
  Last update: 2021-01-26 15:32    [W:0.056 / U:0.040 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site