| Message ID | 20230810035111.2236335-1-lilingfeng@huaweicloud.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [-next,v3] block: remove init_mutex and open-code blk_iolatency_try_init | expand |
On Thu, Aug 10, 2023 at 11:51:11AM +0800, Li Lingfeng <lilingfeng@huaweicloud.com> wrote: > --- > v1->v2: open-code blk_iolatency_try_init() > v2->v3: add lockdep check > block/blk-iolatency.c | 35 +++++++++++------------------------ > 1 file changed, 11 insertions(+), 24 deletions(-) Reviewed-by: Michal Koutný <mkoutny@suse.com> Thanks.
在 2023/08/10 11:51, Li Lingfeng 写道: > From: Li Lingfeng <lilingfeng3@huawei.com> > > Commit a13696b83da4 ("blk-iolatency: Make initialization lazy") adds > a mutex named "init_mutex" in blk_iolatency_try_init for the race > condition of initializing RQ_QOS_LATENCY. > Now a new lock has been add to struct request_queue by commit a13bd91be223 > ("block/rq_qos: protect rq_qos apis with a new lock"). And it has been > held in blkg_conf_open_bdev before calling blk_iolatency_init. > So it's not necessary to keep init_mutex in blk_iolatency_try_init, just > remove it. > > Since init_mutex has been removed, blk_iolatency_try_init can be > open-coded back to iolatency_set_limit() like ioc_qos_write(). > Reviewed-by: Yu Kuai <yukuai3@huawei.com> Thanks, Kuai > Signed-off-by: Li Lingfeng <lilingfeng3@huawei.com> > --- > v1->v2: open-code blk_iolatency_try_init() > v2->v3: add lockdep check > block/blk-iolatency.c | 35 +++++++++++------------------------ > 1 file changed, 11 insertions(+), 24 deletions(-) > > diff --git a/block/blk-iolatency.c b/block/blk-iolatency.c > index fd5fec989e39..c16aef4be036 100644 > --- a/block/blk-iolatency.c > +++ b/block/blk-iolatency.c > @@ -824,29 +824,6 @@ 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); > - > - 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) > { > @@ -861,7 +838,17 @@ static ssize_t iolatency_set_limit(struct kernfs_open_file *of, char *buf, > > blkg_conf_init(&ctx, buf); > > - ret = blk_iolatency_try_init(&ctx); > + ret = blkg_conf_open_bdev(&ctx); > + if (ret) > + goto out; > + > + /* > + * blk_iolatency_init() may fail after rq_qos_add() succeeds which can > + * confuse iolat_rq_qos() test. Make the test and init atomic. > + */ > + lockdep_assert_held(ctx.bdev->bd_queue->rq_qos_mutex); > + if (!iolat_rq_qos(ctx.bdev->bd_queue)) > + ret = blk_iolatency_init(ctx.bdev->bd_disk); > if (ret) > goto out; > >
Hi, 在 2023/08/10 21:43, Yu Kuai 写道: > 在 2023/08/10 11:51, Li Lingfeng 写道: >> From: Li Lingfeng <lilingfeng3@huawei.com> >> >> Commit a13696b83da4 ("blk-iolatency: Make initialization lazy") adds >> a mutex named "init_mutex" in blk_iolatency_try_init for the race >> condition of initializing RQ_QOS_LATENCY. >> Now a new lock has been add to struct request_queue by commit >> a13bd91be223 >> ("block/rq_qos: protect rq_qos apis with a new lock"). And it has been >> held in blkg_conf_open_bdev before calling blk_iolatency_init. >> So it's not necessary to keep init_mutex in blk_iolatency_try_init, just >> remove it. >> >> Since init_mutex has been removed, blk_iolatency_try_init can be >> open-coded back to iolatency_set_limit() like ioc_qos_write(). >> > Reviewed-by: Yu Kuai <yukuai3@huawei.com> > > Thanks, > Kuai >> Signed-off-by: Li Lingfeng <lilingfeng3@huawei.com> >> --- >> v1->v2: open-code blk_iolatency_try_init() >> v2->v3: add lockdep check >> block/blk-iolatency.c | 35 +++++++++++------------------------ >> 1 file changed, 11 insertions(+), 24 deletions(-) >> >> diff --git a/block/blk-iolatency.c b/block/blk-iolatency.c >> index fd5fec989e39..c16aef4be036 100644 >> --- a/block/blk-iolatency.c >> +++ b/block/blk-iolatency.c >> @@ -824,29 +824,6 @@ 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); >> - >> - 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) >> { >> @@ -861,7 +838,17 @@ static ssize_t iolatency_set_limit(struct >> kernfs_open_file *of, char *buf, >> blkg_conf_init(&ctx, buf); >> - ret = blk_iolatency_try_init(&ctx); >> + ret = blkg_conf_open_bdev(&ctx); >> + if (ret) >> + goto out; >> + >> + /* >> + * blk_iolatency_init() may fail after rq_qos_add() succeeds >> which can >> + * confuse iolat_rq_qos() test. Make the test and init atomic. >> + */ The original mutex and above comments is used to avoid the problem that blk_iolatency_init() is not atomic: t1: t2: if (!iolat_rq_qos) // not exist blk_iolatency_init rq_qos_add blkcg_activate_policy // failed if (!iolat_rq_qos) // now exist rq_qos_del ... // continue while rq_qos is deleted Now that this problem doesn't exist, I think it's ok just to remove this comment. Thanks, Kuai >> + lockdep_assert_held(ctx.bdev->bd_queue->rq_qos_mutex); >> + if (!iolat_rq_qos(ctx.bdev->bd_queue)) >> + ret = blk_iolatency_init(ctx.bdev->bd_disk); >> if (ret) >> goto out; >> > > . >
On Fri, Aug 11, 2023 at 09:44:55AM +0800, Yu Kuai <yukuai1@huaweicloud.com> wrote: > Now that this problem doesn't exist, I think it's ok just to remove this > comment. Why doesn't the problem exist now? IIUC, it does but it's prevented thanks to outer synchronization via rq_qos_mutex. Hence the comment clarifies why is the lockdep_assert placed here. Michal
在 2023/08/11 16:23, Michal Koutný 写道: > On Fri, Aug 11, 2023 at 09:44:55AM +0800, Yu Kuai <yukuai1@huaweicloud.com> wrote: >> Now that this problem doesn't exist, I think it's ok just to remove this >> comment. > > Why doesn't the problem exist now? > IIUC, it does but it's prevented thanks to outer synchronization via > rq_qos_mutex. Hence the comment clarifies why is the lockdep_assert > placed here. > Yes, it'm implemented in the upper layer that rq_qos_add() and blkcg_activate_policy() should be atmoic, and currently there is no comments for that. Perhaps it's better to add some comments like following in rq_qos_add() instead? diff --git a/block/blk-rq-qos.c b/block/blk-rq-qos.c index 167be74df4ee..8e8c597ea01b 100644 --- a/block/blk-rq-qos.c +++ b/block/blk-rq-qos.c @@ -302,6 +302,11 @@ int rq_qos_add(struct rq_qos *rqos, struct gendisk *disk, enum rq_qos_id id, { struct request_queue *q = disk->queue; + /* + * If rq_qos is used with cgroup policy, and cgroup policy can be + * initialized through cgroupfs, 'rq_qos_mutex' should be held + * untill blkcg_activate_policy() is done after this function return. + */ lockdep_assert_held(&q->rq_qos_mutex); rqos->disk = disk; > > Michal >
On Fri, Aug 11, 2023 at 04:53:44PM +0800, Yu Kuai <yukuai1@huaweicloud.com> wrote: > Yes, it'm implemented in the upper layer that rq_qos_add() and > blkcg_activate_policy() should be atmoic, and currently there is no > comments for that. The check (iolat_rq_qos()) and use (activating the policy) should be the atomic pair. > Perhaps it's better to add some comments like following in rq_qos_add() > instead? Honestly, I find the current variant (v3) good as it is -- closest to the pair of the operations. (But it's merely a comment so ¯\_(ツ)_/¯) Michal
Hi, Michal 在 2023/08/11 17:17, Michal Koutný 写道: > On Fri, Aug 11, 2023 at 04:53:44PM +0800, Yu Kuai <yukuai1@huaweicloud.com> wrote: >> Yes, it'm implemented in the upper layer that rq_qos_add() and >> blkcg_activate_policy() should be atmoic, and currently there is no >> comments for that. > > The check (iolat_rq_qos()) and use (activating the policy) should be the > atomic pair. > >> Perhaps it's better to add some comments like following in rq_qos_add() >> instead? > > Honestly, I find the current variant (v3) good as it is -- closest to > the pair of the operations. > > (But it's merely a comment so ¯\_(ツ)_/¯) Yes, it's just a comment. Lingfeng, can you resend this patch with the following fixed? > lockdep_assert_held(ctx.bdev->bd_queue->rq_qos_mutex); should be: lockdep_assert_held(&ctx.bdev->bd_queue->rq_qos_mutex); And you can keep my review tag. Thanks, Kuai > > Michal >
On Thu, 10 Aug 2023 11:51:11 +0800, Li Lingfeng wrote: > Commit a13696b83da4 ("blk-iolatency: Make initialization lazy") adds > a mutex named "init_mutex" in blk_iolatency_try_init for the race > condition of initializing RQ_QOS_LATENCY. > Now a new lock has been add to struct request_queue by commit a13bd91be223 > ("block/rq_qos: protect rq_qos apis with a new lock"). And it has been > held in blkg_conf_open_bdev before calling blk_iolatency_init. > So it's not necessary to keep init_mutex in blk_iolatency_try_init, just > remove it. > > [...] Applied, thanks! [1/1] block: remove init_mutex and open-code blk_iolatency_try_init commit: 4eb44d10766ac0fae5973998fd2a0103df1d3fe1 Best regards,
Hi, Jens 在 2023/08/11 22:12, Jens Axboe 写道: > > On Thu, 10 Aug 2023 11:51:11 +0800, Li Lingfeng wrote: >> Commit a13696b83da4 ("blk-iolatency: Make initialization lazy") adds >> a mutex named "init_mutex" in blk_iolatency_try_init for the race >> condition of initializing RQ_QOS_LATENCY. >> Now a new lock has been add to struct request_queue by commit a13bd91be223 >> ("block/rq_qos: protect rq_qos apis with a new lock"). And it has been >> held in blkg_conf_open_bdev before calling blk_iolatency_init. >> So it's not necessary to keep init_mutex in blk_iolatency_try_init, just >> remove it. >> >> [...] > > Applied, thanks! > > [1/1] block: remove init_mutex and open-code blk_iolatency_try_init > commit: 4eb44d10766ac0fae5973998fd2a0103df1d3fe1 This version has a minor problem that pss in mutex for lockdep_assert_held() is not a pointer: > lockdep_assert_held(ctx.bdev->bd_queue->rq_qos_mutex); should be: lockdep_assert_held(&ctx.bdev->bd_queue->rq_qos_mutex); Perhaps can you drop this patch for now, and Lingfeng can send a v4? Thanks, Kuai > > Best regards, >
On 8/11/23 7:42 PM, Yu Kuai wrote: > Hi, Jens > > ? 2023/08/11 22:12, Jens Axboe ??: >> >> On Thu, 10 Aug 2023 11:51:11 +0800, Li Lingfeng wrote: >>> Commit a13696b83da4 ("blk-iolatency: Make initialization lazy") adds >>> a mutex named "init_mutex" in blk_iolatency_try_init for the race >>> condition of initializing RQ_QOS_LATENCY. >>> Now a new lock has been add to struct request_queue by commit a13bd91be223 >>> ("block/rq_qos: protect rq_qos apis with a new lock"). And it has been >>> held in blkg_conf_open_bdev before calling blk_iolatency_init. >>> So it's not necessary to keep init_mutex in blk_iolatency_try_init, just >>> remove it. >>> >>> [...] >> >> Applied, thanks! >> >> [1/1] block: remove init_mutex and open-code blk_iolatency_try_init >> commit: 4eb44d10766ac0fae5973998fd2a0103df1d3fe1 > > This version has a minor problem that pss in mutex for > lockdep_assert_held() is not a pointer: > >> lockdep_assert_held(ctx.bdev->bd_queue->rq_qos_mutex); > > should be: > lockdep_assert_held(&ctx.bdev->bd_queue->rq_qos_mutex); Yes, looked like that patch didn't get compiled... Shame. > Perhaps can you drop this patch for now, and Lingfeng can send a v4? I did fix that up 2 days ago myself.
diff --git a/block/blk-iolatency.c b/block/blk-iolatency.c index fd5fec989e39..c16aef4be036 100644 --- a/block/blk-iolatency.c +++ b/block/blk-iolatency.c @@ -824,29 +824,6 @@ 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); - - 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) { @@ -861,7 +838,17 @@ static ssize_t iolatency_set_limit(struct kernfs_open_file *of, char *buf, blkg_conf_init(&ctx, buf); - ret = blk_iolatency_try_init(&ctx); + ret = blkg_conf_open_bdev(&ctx); + if (ret) + goto out; + + /* + * blk_iolatency_init() may fail after rq_qos_add() succeeds which can + * confuse iolat_rq_qos() test. Make the test and init atomic. + */ + lockdep_assert_held(ctx.bdev->bd_queue->rq_qos_mutex); + if (!iolat_rq_qos(ctx.bdev->bd_queue)) + ret = blk_iolatency_init(ctx.bdev->bd_disk); if (ret) goto out;