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]

Message-ID: <BYAPR04MB4965A6FB4ED51882E326EC1A86BC9@BYAPR04MB4965.namprd04.prod.outlook.com>
Date:   Tue, 26 Jan 2021 09:53:51 +0000
From:   Chaitanya Kulkarni <Chaitanya.Kulkarni@....com>
To:     Pavel Tatashin <pasha.tatashin@...een.com>,
        "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 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@...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:-
       /*  
        * 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)