From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 39E05C54E76 for ; Thu, 5 Jan 2023 01:35:33 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229486AbjAEBfb (ORCPT ); Wed, 4 Jan 2023 20:35:31 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:59692 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229441AbjAEBf3 (ORCPT ); Wed, 4 Jan 2023 20:35:29 -0500 Received: from dggsgout11.his.huawei.com (unknown [45.249.212.51]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id CB1C02009; Wed, 4 Jan 2023 17:35:27 -0800 (PST) Received: from mail02.huawei.com (unknown [172.30.67.153]) by dggsgout11.his.huawei.com (SkyGuard) with ESMTP id 4NnTYk4Nc5z4f3jJ4; Thu, 5 Jan 2023 09:35:22 +0800 (CST) Received: from [10.174.176.73] (unknown [10.174.176.73]) by APP1 (Coremail) with SMTP id cCh0CgCHpjBaKbZjMF4wBA--.2841S3; Thu, 05 Jan 2023 09:35:23 +0800 (CST) Subject: Re: [PATCH -next 3/4] block/rq_qos: use a global mutex to protect rq_qos apis To: Tejun Heo , Yu Kuai Cc: hch@infradead.org, josef@toxicpanda.com, axboe@kernel.dk, cgroups@vger.kernel.org, linux-block@vger.kernel.org, linux-kernel@vger.kernel.org, yi.zhang@huawei.com, yangerkun@huawei.com, "yukuai (C)" References: <20230104085354.2343590-1-yukuai1@huaweicloud.com> <20230104085354.2343590-4-yukuai1@huaweicloud.com> From: Yu Kuai Message-ID: Date: Thu, 5 Jan 2023 09:35: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: Content-Type: text/plain; charset=gbk; format=flowed Content-Transfer-Encoding: 8bit X-CM-TRANSID: cCh0CgCHpjBaKbZjMF4wBA--.2841S3 X-Coremail-Antispam: 1UD129KBjvJXoW7CrWrtrWftry5Cr4fWr4kZwb_yoW8try5pF Z0g3Z8Kw4kGrsava17Z3yrXrySyrWkG3y3JrW8Gr4jk3s0kryqvr9avF47KFW5XFZ5Gr4j v3yrKF93GryUCFJanT9S1TB71UUUUUUqnTZGkaVYY2UrUUUUjbIjqfuFe4nvWSU5nxnvy2 9KBjDU0xBIdaVrnRJUUU9214x267AKxVW8JVW5JwAFc2x0x2IEx4CE42xK8VAvwI8IcIk0 rVWrJVCq3wAFIxvE14AKwVWUJVWUGwA2ocxC64kIII0Yj41l84x0c7CEw4AK67xGY2AK02 1l84ACjcxK6xIIjxv20xvE14v26w1j6s0DM28EF7xvwVC0I7IYx2IY6xkF7I0E14v26r4U JVWxJr1l84ACjcxK6I8E87Iv67AKxVW0oVCq3wA2z4x0Y4vEx4A2jsIEc7CjxVAFwI0_Gc CE3s1le2I262IYc4CY6c8Ij28IcVAaY2xG8wAqx4xG64xvF2IEw4CE5I8CrVC2j2WlYx0E 2Ix0cI8IcVAFwI0_Jr0_Jr4lYx0Ex4A2jsIE14v26r1j6r4UMcvjeVCFs4IE7xkEbVWUJV W8JwACjcxG0xvEwIxGrwACjI8F5VA0II8E6IAqYI8I648v4I1lFIxGxcIEc7CjxVA2Y2ka 0xkIwI1lc7I2V7IY0VAS07AlzVAYIcxG8wCF04k20xvY0x0EwIxGrwCFx2IqxVCFs4IE7x kEbVWUJVW8JwC20s026c02F40E14v26r1j6r18MI8I3I0E7480Y4vE14v26r106r1rMI8E 67AF67kF1VAFwI0_Jw0_GFylIxkGc2Ij64vIr41lIxAIcVC0I7IYx2IY67AKxVWUJVWUCw CI42IY6xIIjxv20xvEc7CjxVAFwI0_Gr0_Cr1lIxAIcVCF04k26cxKx2IYs7xG6rWUJVWr Zr1UMIIF0xvEx4A2jsIE14v26r1j6r4UMIIF0xvEx4A2jsIEc7CjxVAFwI0_Gr0_Gr1UYx BIdaVFxhVjvjDU0xZFpf9x0JUdHUDUUUUU= X-CM-SenderInfo: 51xn3trlr6x35dzhxuhorxvhhfrp/ X-CFilter-Loop: Reflected Precedence: bulk List-ID: X-Mailing-List: linux-block@vger.kernel.org Hi, ÔÚ 2023/01/05 8:28, Tejun Heo дµÀ: > Hello, again. > > On Wed, Jan 04, 2023 at 11:39:47AM -1000, Tejun Heo wrote: >>> 2) rq_qos_add() and blkcg_activate_policy() is not atomic, if >>> rq_qos_exit() is done before blkcg_activate_policy(), >>> null-ptr-deference can be triggered. >> >> I'm not sure this part does. I think it'd be better to guarantee that device >> destruction is blocked while these configuration operations are in progress >> which can be built into blkg_conf helpers. > > A bit more explanation: > > Usually, this would be handled in the core - when a device goes away, its > sysfs files get shut down before stuff gets freed and the sysfs file removal > waits for in-flight operations to finish and prevents new ones from > starting, so we don't have to worry about in-flight config file operations > racing against device removal. > > Here, the problem isn't solved by that because the config files live on > cgroupfs and their lifetimes are not coupled with the block devices'. So, we > need to synchronize manually. And, given that, the right place to do is the > blkg config helpers cuz they're the ones which establish the connection > between cgroup and block layer. Thanks for the explanation, I agree with that. > > Can you please take a look at the following patchset I just posted: > > https://lkml.kernel.org/r/20230105002007.157497-1-tj@kernel.org > > After that, all these configuration operations are wrapped between > blkg_conf_init() and blkg_conf_exit() which probably are the right place to > implement the synchronization. I see that, blkg_conf_init() and blkg_conf_exit() is good, however there are some details I want to confirm: 1) rq_qos_add() can be called from iocost/iolatency, where blkg_conf_init() will be called first, while rq_qos_add() can also be called from wbt, where there is no blkg_conf_init(). Hence it seems to me we need two locks here, one to protect rq_qos apis; one to synchronize policy configuration and device removal. 2) If you agree with 1), it seems better to use the other lock in device level, consider that there is no need to synchronize confituration for different devices. Thanks, Kuai > > Thanks. > From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yu Kuai Subject: Re: [PATCH -next 3/4] block/rq_qos: use a global mutex to protect rq_qos apis Date: Thu, 5 Jan 2023 09:35:21 +0800 Message-ID: References: <20230104085354.2343590-1-yukuai1@huaweicloud.com> <20230104085354.2343590-4-yukuai1@huaweicloud.com> Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Return-path: In-Reply-To: List-ID: Content-Type: text/plain; charset="iso-8859-1"; format="flowed" To: Tejun Heo , Yu Kuai Cc: hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org, josef-DigfWCa+lFGyeJad7bwFQA@public.gmane.org, axboe-tSWWG44O7X1aa/9Udqfwiw@public.gmane.org, cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-block-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, yi.zhang-hv44wF8Li93QT0dZR+AlfA@public.gmane.org, yangerkun-hv44wF8Li93QT0dZR+AlfA@public.gmane.org, "yukuai (C)" Hi, =D4=DA 2023/01/05 8:28, Tejun Heo =D0=B4=B5=C0: > Hello, again. >=20 > On Wed, Jan 04, 2023 at 11:39:47AM -1000, Tejun Heo wrote: >>> 2) rq_qos_add() and blkcg_activate_policy() is not atomic, if >>> rq_qos_exit() is done before blkcg_activate_policy(), >>> null-ptr-deference can be triggered. >> >> I'm not sure this part does. I think it'd be better to guarantee that de= vice >> destruction is blocked while these configuration operations are in progr= ess >> which can be built into blkg_conf helpers. >=20 > A bit more explanation: >=20 > Usually, this would be handled in the core - when a device goes away, its > sysfs files get shut down before stuff gets freed and the sysfs file remo= val > waits for in-flight operations to finish and prevents new ones from > starting, so we don't have to worry about in-flight config file operations > racing against device removal. >=20 > Here, the problem isn't solved by that because the config files live on > cgroupfs and their lifetimes are not coupled with the block devices'. So,= we > need to synchronize manually. And, given that, the right place to do is t= he > blkg config helpers cuz they're the ones which establish the connection > between cgroup and block layer. Thanks for the explanation, I agree with that. >=20 > Can you please take a look at the following patchset I just posted: >=20 > https://lkml.kernel.org/r/20230105002007.157497-1-tj-DgEjT+Ai2ygdnm+yR= OfE0A@public.gmane.org >=20 > After that, all these configuration operations are wrapped between > blkg_conf_init() and blkg_conf_exit() which probably are the right place = to > implement the synchronization. I see that, blkg_conf_init() and blkg_conf_exit() is good, however there are some details I want to confirm: 1) rq_qos_add() can be called from iocost/iolatency, where blkg_conf_init() will be called first, while rq_qos_add() can also be called from wbt, where there is no blkg_conf_init(). Hence it seems to me we need two locks here, one to protect rq_qos apis; one to synchronize policy configuration and device removal. 2) If you agree with 1), it seems better to use the other lock in device level, consider that there is no need to synchronize confituration for different devices. Thanks, Kuai >=20 > Thanks. >=20