Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Subject: Re: [PATCH 4/4] blk-iolatency: Make initialization lazy
To:     Tejun Heo <tj@kernel.org>, axboe@kernel.dk, josef@toxicpanda.com,
        hch@lst.de
Cc:     linux-block@vger.kernel.org, linux-kernel@vger.kernel.org,
        yukuai1@huaweicloud.com, "yukuai (C)" <yukuai3@huawei.com>
References: <20230413000649.115785-1-tj@kernel.org>
 <20230413000649.115785-5-tj@kernel.org>
From:   Yu Kuai <yukuai1@huaweicloud.com>
Message-ID: <661dc6d6-2786-cbe5-e751-95947a1608cc@huaweicloud.com>
Date:   Thu, 13 Apr 2023 10:09:21 +0800
User-Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:60.0) Gecko/20100101
 Thunderbird/60.8.0
MIME-Version: 1.0
In-Reply-To: <20230413000649.115785-5-tj@kernel.org>
Content-Type: text/plain; charset=gbk; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

Hi,

?? 2023/04/13 8:06, Tejun Heo д??:
> Other rq_qos policies such as wbt and iocost are lazy-initialized when they
> are configured for the first time for the device but iolatency is
> initialized unconditionally from blkcg_init_disk() during gendisk init. Lazy
> init is beneficial because rq_qos policies add runtime overhead when
> initialized as every IO has to walk all registered rq_qos callbacks.
> 
> This patch switches iolatency to lazy initialization too so that it only
> registered its rq_qos policy when it is first configured.
> 
> Note that there is a known race condition between blkcg config file writes
> and del_gendisk() and this patch makes iolatency susceptible to it by
> exposing the init path to race against the deletion path. However, that
> problem already exists in iocost and is being worked on.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> Cc: Josef Bacik <josef@toxicpanda.com>
> ---
>   block/blk-cgroup.c    |  8 --------
>   block/blk-iolatency.c | 29 ++++++++++++++++++++++++++++-
>   block/blk.h           |  6 ------
>   3 files changed, 28 insertions(+), 15 deletions(-)
> 
> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index c154b08a7e92..1c1ebeb51003 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -33,7 +33,6 @@
>   #include "blk-cgroup.h"
>   #include "blk-ioprio.h"
>   #include "blk-throttle.h"
> -#include "blk-rq-qos.h"
>   
>   /*
>    * blkcg_pol_mutex protects blkcg_policy[] and policy [de]activation.
> @@ -1350,14 +1349,8 @@ int blkcg_init_disk(struct gendisk *disk)
>   	if (ret)
>   		goto err_ioprio_exit;
>   
> -	ret = blk_iolatency_init(disk);
> -	if (ret)
> -		goto err_throtl_exit;
> -
>   	return 0;
>   
> -err_throtl_exit:
> -	blk_throtl_exit(disk);
>   err_ioprio_exit:
>   	blk_ioprio_exit(disk);
>   err_destroy_all:
> @@ -1373,7 +1366,6 @@ int blkcg_init_disk(struct gendisk *disk)
>   void blkcg_exit_disk(struct gendisk *disk)
>   {
>   	blkg_destroy_all(disk);
> -	rq_qos_exit(disk->queue);
>   	blk_throtl_exit(disk);
>   }
>   
> diff --git a/block/blk-iolatency.c b/block/blk-iolatency.c
> index 2560708b9109..fd5fec989e39 100644
> --- a/block/blk-iolatency.c
> +++ b/block/blk-iolatency.c
> @@ -755,7 +755,7 @@ static void blkiolatency_enable_work_fn(struct work_struct *work)
>   	}
>   }
>   
> -int blk_iolatency_init(struct gendisk *disk)
> +static int blk_iolatency_init(struct gendisk *disk)
>   {
>   	struct blk_iolatency *blkiolat;
>   	int ret;
> @@ -824,6 +824,29 @@ static void iolatency_clear_scaling(struct blkcg_gq *blkg)
>   	}
>   }
>   
> +static int blk_iolatency_try_init(struct blkg_conf_ctx *ctx)
> +{
> +	static DEFINE_MUTEX(init_mutex);
> +	int ret;
> +
> +	ret = blkg_conf_open_bdev(ctx);
> +	if (ret)
> +		return ret;
> +
> +	/*
> +	 * blk_iolatency_init() may fail after rq_qos_add() succeeds which can
> +	 * confuse iolat_rq_qos() test. Make the test and init atomic.
> +	 */
> +	mutex_lock(&init_mutex);

Can we just convert the return value from -EBUSY to 0 in this case, so
that the global lock won't be needed.

Thanks,
Kuai
> +
> +	if (!iolat_rq_qos(ctx->bdev->bd_queue))
> +		ret = blk_iolatency_init(ctx->bdev->bd_disk);
> +
> +	mutex_unlock(&init_mutex);
> +
> +	return ret;
> +}
> +
>   static ssize_t iolatency_set_limit(struct kernfs_open_file *of, char *buf,
>   			     size_t nbytes, loff_t off)
>   {
> @@ -838,6 +861,10 @@ static ssize_t iolatency_set_limit(struct kernfs_open_file *of, char *buf,
>   
>   	blkg_conf_init(&ctx, buf);
>   
> +	ret = blk_iolatency_try_init(&ctx);
> +	if (ret)
> +		goto out;
> +
>   	ret = blkg_conf_prep(blkcg, &blkcg_policy_iolatency, &ctx);
>   	if (ret)
>   		goto out;
> diff --git a/block/blk.h b/block/blk.h
> index d65d96994a94..62fca868bc61 100644
> --- a/block/blk.h
> +++ b/block/blk.h
> @@ -399,12 +399,6 @@ static inline struct bio *blk_queue_bounce(struct bio *bio,
>   	return bio;
>   }
>   
> -#ifdef CONFIG_BLK_CGROUP_IOLATENCY
> -int blk_iolatency_init(struct gendisk *disk);
> -#else
> -static inline int blk_iolatency_init(struct gendisk *disk) { return 0; };
> -#endif
> -
>   #ifdef CONFIG_BLK_DEV_ZONED
>   void disk_free_zone_bitmaps(struct gendisk *disk);
>   void disk_clear_zone_settings(struct gendisk *disk);
>