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

From: Chaitanya Kulkarni <Chaitanya.Kulkarni@wdc.com>
To: Pavel Tatashin <pasha.tatashin@soleen.com>,
	"tyhicks@linux.microsoft.com" <tyhicks@linux.microsoft.com>,
	"axboe@kernel.dk" <axboe@kernel.dk>,
	"linux-block@vger.kernel.org" <linux-block@vger.kernel.org>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"sashal@kernel.org" <sashal@kernel.org>,
	"jmorris@namei.org" <jmorris@namei.org>,
	"lukas.bulwahn@gmail.com" <lukas.bulwahn@gmail.com>,
	"hch@lst.de" <hch@lst.de>, "pvorel@suse.cz" <pvorel@suse.cz>,
	"ming.lei@redhat.com" <ming.lei@redhat.com>,
	"mzxreary@0pointer.de" <mzxreary@0pointer.de>,
	"mcgrof@kernel.org" <mcgrof@kernel.org>,
	"zhengbin13@huawei.com" <zhengbin13@huawei.com>,
	"maco@android.com" <maco@android.com>,
	"colin.king@canonical.com" <colin.king@canonical.com>,
	"evgreen@chromium.org" <evgreen@chromium.org>
Subject: Re: [PATCH v3 1/1] loop: scale loop device by introducing per device lock
Date: Tue, 26 Jan 2021 09:53:51 +0000	[thread overview]
Message-ID: <BYAPR04MB4965A6FB4ED51882E326EC1A86BC9@BYAPR04MB4965.namprd04.prod.outlook.com> (raw)
In-Reply-To: 20210125201156.1330164-2-pasha.tatashin@soleen.com

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@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:-
       /*  
        * 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)