| Message ID | 20230124065716.152286-6-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/15] blk-cgroup: don't defer blkg_free to a workqueue | expand |
On Tue, Jan 24, 2023 at 07:57:05AM +0100, Christoph Hellwig wrote: > void blkcg_maybe_throttle_current(void) > { > - struct request_queue *q = current->throttle_queue; > + struct gendisk *disk = current->throttle_disk; > struct blkcg *blkcg; > struct blkcg_gq *blkg; > bool use_memdelay = current->use_memdelay; > > - if (!q) > + if (!disk) > return; > > - current->throttle_queue = NULL; > + current->throttle_disk = NULL; > current->use_memdelay = false; > > + if (test_bit(GD_DEAD, &disk->state)) > + goto out_put_disk; ... > @@ -1835,18 +1839,15 @@ void blkcg_maybe_throttle_current(void) > */ > void blkcg_schedule_throttle(struct gendisk *disk, bool use_memdelay) > { > - struct request_queue *q = disk->queue; > - > if (unlikely(current->flags & PF_KTHREAD)) > return; > > - if (current->throttle_queue != q) { > - if (!blk_get_queue(q)) > - return; > + if (current->throttle_disk != disk) { > + get_device(disk_to_dev(disk)); So, we're shifting the dead test from schedule to the actual throttle path, which makes sense but I think this should at least be mentioned in the description if not put in its own patch. Thanks.
On Fri, Jan 27, 2023 at 01:40:00PM -1000, Tejun Heo wrote: > So, we're shifting the dead test from schedule to the actual throttle path, > which makes sense but I think this should at least be mentioned in the > description if not put in its own patch. That's what I tried to say with: "Move the check for the dead disk to the latest place now that is is unbundled from the reference grab." what else would you want me to write?
Sorry about late reply. On Wed, Feb 01, 2023 at 09:06:35AM +0100, Christoph Hellwig wrote: > On Fri, Jan 27, 2023 at 01:40:00PM -1000, Tejun Heo wrote: > > So, we're shifting the dead test from schedule to the actual throttle path, > > which makes sense but I think this should at least be mentioned in the > > description if not put in its own patch. > > That's what I tried to say with: > > "Move the check for the dead disk to the latest place now that is is > unbundled from the reference grab." > > what else would you want me to write? It'd be better if the paragraph actually said where that change is being made along with why and whether it causes any behavior difference. This seems safe to me but I can't tell why it's being moved. Maybe it's better because that's "latest" but that's a lot hanging on that single word. If this is better because we're testing it later instead of to accomodate other changes being made, this should be a separate patch, right? It's a subtle behavior change which is buried with a bunch of other mechanical changes. Thanks.
diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c index f5a634ed098db0..603e911d1350db 100644 --- a/block/blk-cgroup.c +++ b/block/blk-cgroup.c @@ -1334,9 +1334,9 @@ static void blkcg_bind(struct cgroup_subsys_state *root_css) static void blkcg_exit(struct task_struct *tsk) { - if (tsk->throttle_queue) - blk_put_queue(tsk->throttle_queue); - tsk->throttle_queue = NULL; + if (tsk->throttle_disk) + put_disk(tsk->throttle_disk); + tsk->throttle_disk = NULL; } struct cgroup_subsys io_cgrp_subsys = { @@ -1778,29 +1778,32 @@ static void blkcg_maybe_throttle_blkg(struct blkcg_gq *blkg, bool use_memdelay) * * This is only called if we've been marked with set_notify_resume(). Obviously * we can be set_notify_resume() for reasons other than blkcg throttling, so we - * check to see if current->throttle_queue is set and if not this doesn't do + * check to see if current->throttle_disk is set and if not this doesn't do * anything. This should only ever be called by the resume code, it's not meant * to be called by people willy-nilly as it will actually do the work to * throttle the task if it is setup for throttling. */ void blkcg_maybe_throttle_current(void) { - struct request_queue *q = current->throttle_queue; + struct gendisk *disk = current->throttle_disk; struct blkcg *blkcg; struct blkcg_gq *blkg; bool use_memdelay = current->use_memdelay; - if (!q) + if (!disk) return; - current->throttle_queue = NULL; + current->throttle_disk = NULL; current->use_memdelay = false; + if (test_bit(GD_DEAD, &disk->state)) + goto out_put_disk; + rcu_read_lock(); blkcg = css_to_blkcg(blkcg_css()); if (!blkcg) goto out; - blkg = blkg_lookup(blkcg, q); + blkg = blkg_lookup(blkcg, disk->queue); if (!blkg) goto out; if (!blkg_tryget(blkg)) @@ -1809,11 +1812,12 @@ void blkcg_maybe_throttle_current(void) blkcg_maybe_throttle_blkg(blkg, use_memdelay); blkg_put(blkg); - blk_put_queue(q); + put_disk(disk); return; out: rcu_read_unlock(); - blk_put_queue(q); +out_put_disk: + put_disk(disk); } /** @@ -1835,18 +1839,15 @@ void blkcg_maybe_throttle_current(void) */ void blkcg_schedule_throttle(struct gendisk *disk, bool use_memdelay) { - struct request_queue *q = disk->queue; - if (unlikely(current->flags & PF_KTHREAD)) return; - if (current->throttle_queue != q) { - if (!blk_get_queue(q)) - return; + if (current->throttle_disk != disk) { + get_device(disk_to_dev(disk)); - if (current->throttle_queue) - blk_put_queue(current->throttle_queue); - current->throttle_queue = q; + if (current->throttle_disk) + put_disk(current->throttle_disk); + current->throttle_disk = disk; } if (use_memdelay) diff --git a/include/linux/sched.h b/include/linux/sched.h index 853d08f7562bda..6f6ce9ca709798 100644 --- a/include/linux/sched.h +++ b/include/linux/sched.h @@ -1436,7 +1436,7 @@ struct task_struct { #endif #ifdef CONFIG_BLK_CGROUP - struct request_queue *throttle_queue; + struct gendisk *throttle_disk; #endif #ifdef CONFIG_UPROBES diff --git a/kernel/fork.c b/kernel/fork.c index 9f7fe354189785..d9c97704b7c9a4 100644 --- a/kernel/fork.c +++ b/kernel/fork.c @@ -1044,7 +1044,7 @@ static struct task_struct *dup_task_struct(struct task_struct *orig, int node) #endif #ifdef CONFIG_BLK_CGROUP - tsk->throttle_queue = NULL; + tsk->throttle_disk = NULL; tsk->use_memdelay = 0; #endif diff --git a/mm/swapfile.c b/mm/swapfile.c index 908a529bca12c9..3e0a742fb7bbff 100644 --- a/mm/swapfile.c +++ b/mm/swapfile.c @@ -3642,7 +3642,7 @@ void __cgroup_throttle_swaprate(struct page *page, gfp_t gfp_mask) * We've already scheduled a throttle, avoid taking the global swap * lock. */ - if (current->throttle_queue) + if (current->throttle_disk) return; spin_lock(&swap_avail_lock);