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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 26 Jan 2021 09:29:28 -0500
From:   Pavel Tatashin <pasha.tatashin@...een.com>
To:     Chaitanya Kulkarni <Chaitanya.Kulkarni@....com>
Cc:     "tyhicks@...ux.microsoft.com" <tyhicks@...ux.microsoft.com>,
        "axboe@...nel.dk" <axboe@...nel.dk>,
        "linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "sashal@...nel.org" <sashal@...nel.org>,
        "jmorris@...ei.org" <jmorris@...ei.org>,
        "lukas.bulwahn@...il.com" <lukas.bulwahn@...il.com>,
        "hch@....de" <hch@....de>, "pvorel@...e.cz" <pvorel@...e.cz>,
        "ming.lei@...hat.com" <ming.lei@...hat.com>,
        "mzxreary@...inter.de" <mzxreary@...inter.de>,
        "mcgrof@...nel.org" <mcgrof@...nel.org>,
        "zhengbin13@...wei.com" <zhengbin13@...wei.com>,
        "maco@...roid.com" <maco@...roid.com>,
        "colin.king@...onical.com" <colin.king@...onical.com>,
        "evgreen@...omium.org" <evgreen@...omium.org>
Subject: Re: [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@....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@...een.com>
> > Reviewed-by: Tyler Hicks <tyhicks@...ux.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