From: "yukuai (C)" <yukuai3@huawei.com> To: "Michal Koutný" <mkoutny@suse.com> Cc: <hch@infradead.org>, <tj@kernel.org>, <axboe@kernel.dk>, <cgroups@vger.kernel.org>, <linux-block@vger.kernel.org>, <linux-kernel@vger.kernel.org>, <yi.zhang@huawei.com> Subject: Re: [PATCH v4 2/2] block: cancel all throttled bios in del_gendisk() Date: Fri, 3 Dec 2021 15:50:01 +0800 [thread overview] Message-ID: <95825098-a532-a0e4-9ed0-0b5f2a0e5f04@huawei.com> (raw) In-Reply-To: <20211202144818.GB16798@blackbody.suse.cz> 在 2021/12/02 22:48, Michal Koutný 写道: > Hello Kuai. > > On Thu, Dec 02, 2021 at 09:04:40PM +0800, Yu Kuai <yukuai3@huawei.com> wrote: >> For example, if user thread is throttled with low bps while it's >> issuing large io, and the device is deleted. The user thread will >> wait for a long time for io to return. > > Do I understand correctly the "long time" here is > outstanding_IO_size/throttled_bandwidth? Or are you getting at some Hi, Michal Yes, this is exactly what I mean. > other cause/longer time? > >> +void blk_throtl_cancel_bios(struct request_queue *q) >> +{ >> + struct throtl_data *td = q->td; >> + struct bio_list bio_list_on_stack; >> + struct blkcg_gq *blkg; >> + struct cgroup_subsys_state *pos_css; >> + struct bio *bio; >> + int rw; >> + >> + bio_list_init(&bio_list_on_stack); >> + >> + /* >> + * hold queue_lock to prevent concurrent with dispatching >> + * throttled bios by timer. >> + */ >> + spin_lock_irq(&q->queue_lock); > > You've replaced the rcu_read_lock() with the queue lock but... > >> + >> + /* >> + * Drain each tg while doing post-order walk on the blkg tree, so >> + * that all bios are propagated to td->service_queue. It'd be >> + * better to walk service_queue tree directly but blkg walk is >> + * easier. >> + */ >> + blkg_for_each_descendant_post(blkg, pos_css, td->queue->root_blkg) >> + tg_drain_bios(&blkg_to_tg(blkg)->service_queue); > > ...you also need the rcu_read_lock() here since you may encounter a > (descendant) blkcg that's removed concurrently. blkg_destroy() is protected by the queue_lock,so I think queue_lock can protect such concurrent scenario. Thanks, Kuai > > (I may miss some consequences of doing this under the queue_lock so if > the concurrent removal is ruled out, please make a comment about it.) > > > Regards, > Michal >
WARNING: multiple messages have this Message-ID (diff)
From: "yukuai (C)" <yukuai3@huawei.com> To: "Michal Koutný" <mkoutny@suse.com> Cc: hch@infradead.org, tj@kernel.org, axboe@kernel.dk, cgroups@vger.kernel.org, linux-block@vger.kernel.org, linux-kernel@vger.kernel.org, yi.zhang@huawei.com Subject: Re: [PATCH v4 2/2] block: cancel all throttled bios in del_gendisk() Date: Fri, 3 Dec 2021 15:50:01 +0800 [thread overview] Message-ID: <95825098-a532-a0e4-9ed0-0b5f2a0e5f04@huawei.com> (raw) In-Reply-To: <20211202144818.GB16798@blackbody.suse.cz> 在 2021/12/02 22:48, Michal Koutný 写道: > Hello Kuai. > > On Thu, Dec 02, 2021 at 09:04:40PM +0800, Yu Kuai <yukuai3@huawei.com> wrote: >> For example, if user thread is throttled with low bps while it's >> issuing large io, and the device is deleted. The user thread will >> wait for a long time for io to return. > > Do I understand correctly the "long time" here is > outstanding_IO_size/throttled_bandwidth? Or are you getting at some Hi, Michal Yes, this is exactly what I mean. > other cause/longer time? > >> +void blk_throtl_cancel_bios(struct request_queue *q) >> +{ >> + struct throtl_data *td = q->td; >> + struct bio_list bio_list_on_stack; >> + struct blkcg_gq *blkg; >> + struct cgroup_subsys_state *pos_css; >> + struct bio *bio; >> + int rw; >> + >> + bio_list_init(&bio_list_on_stack); >> + >> + /* >> + * hold queue_lock to prevent concurrent with dispatching >> + * throttled bios by timer. >> + */ >> + spin_lock_irq(&q->queue_lock); > > You've replaced the rcu_read_lock() with the queue lock but... > >> + >> + /* >> + * Drain each tg while doing post-order walk on the blkg tree, so >> + * that all bios are propagated to td->service_queue. It'd be >> + * better to walk service_queue tree directly but blkg walk is >> + * easier. >> + */ >> + blkg_for_each_descendant_post(blkg, pos_css, td->queue->root_blkg) >> + tg_drain_bios(&blkg_to_tg(blkg)->service_queue); > > ...you also need the rcu_read_lock() here since you may encounter a > (descendant) blkcg that's removed concurrently. blkg_destroy() is protected by the queue_lock,so I think queue_lock can protect such concurrent scenario. Thanks, Kuai > > (I may miss some consequences of doing this under the queue_lock so if > the concurrent removal is ruled out, please make a comment about it.) > > > Regards, > Michal >
next prev parent reply other threads:[~2021-12-03 7:50 UTC|newest] Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top 2021-12-02 13:04 [PATCH v4 0/2] cancel all throttled bios in del_gendisk() Yu Kuai 2021-12-02 13:04 ` Yu Kuai 2021-12-02 13:04 ` [PATCH v4 1/2] blk-throtl: move WARN_ON_ONCE() from throtl_rb_first() to it's caller Yu Kuai 2021-12-02 13:04 ` Yu Kuai 2021-12-02 13:04 ` [PATCH v4 2/2] block: cancel all throttled bios in del_gendisk() Yu Kuai 2021-12-02 13:04 ` Yu Kuai 2021-12-02 14:48 ` Michal Koutný 2021-12-02 14:48 ` Michal Koutný 2021-12-03 7:50 ` yukuai (C) [this message] 2021-12-03 7:50 ` yukuai (C) 2021-12-03 10:27 ` Michal Koutný 2021-12-03 10:27 ` Michal Koutný 2021-12-04 8:03 ` yukuai (C) 2021-12-04 8:03 ` yukuai (C) 2021-12-06 15:43 ` Michal Koutný 2021-12-06 15:43 ` Michal Koutný
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=95825098-a532-a0e4-9ed0-0b5f2a0e5f04@huawei.com \ --to=yukuai3@huawei.com \ --cc=axboe@kernel.dk \ --cc=cgroups@vger.kernel.org \ --cc=hch@infradead.org \ --cc=linux-block@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=mkoutny@suse.com \ --cc=tj@kernel.org \ --cc=yi.zhang@huawei.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.