This is a note to let you know that I've just added the patch titled bfq: Get rid of __bio_blkcg() usage to the 5.4-stable tree which can be found at: http://www.kernel.org/git/?p=linux/kernel/git/stable/stable-queue.git;a=summary The filename of the patch is: bfq-get-rid-of-__bio_blkcg-usage.patch and it can be found in the queue-5.4 subdirectory. If you, or anyone else, feels it should not be added to the stable tree, please let <stable@xxxxxxxxxxxxxxx> know about it. >From foo@baz Tue Jun 7 11:57:10 AM CEST 2022 From: Jan Kara <jack@xxxxxxx> Date: Tue, 7 Jun 2022 11:15:12 +0200 Subject: bfq: Get rid of __bio_blkcg() usage To: stable@xxxxxxxxxxxxxxx Cc: Jens Axboe <axboe@xxxxxxxxx>, Paolo Valente <paolo.valente@xxxxxxxxxx>, <linux-block@xxxxxxxxxxxxxxx>, Jan Kara <jack@xxxxxxx>, "yukuai (C)" <yukuai3@xxxxxxxxxx>, Christoph Hellwig <hch@xxxxxx> Message-ID: <20220607091528.11906-4-jack@xxxxxxx> From: Jan Kara <jack@xxxxxxx> commit 4e54a2493e582361adc3bfbf06c7d50d19d18837 upstream. BFQ usage of __bio_blkcg() is a relict from the past. Furthermore if bio would not be associated with any blkcg, the usage of __bio_blkcg() in BFQ is prone to races with the task being migrated between cgroups as __bio_blkcg() calls at different places could return different blkcgs. Convert BFQ to the new situation where bio->bi_blkg is initialized in bio_set_dev() and thus practically always valid. This allows us to save blkcg_gq lookup and noticeably simplify the code. CC: stable@xxxxxxxxxxxxxxx Fixes: 0fe061b9f03c ("blkcg: fix ref count issue with bio_blkcg() using task_css") Tested-by: "yukuai (C)" <yukuai3@xxxxxxxxxx> Signed-off-by: Jan Kara <jack@xxxxxxx> Reviewed-by: Christoph Hellwig <hch@xxxxxx> Link: https://lore.kernel.org/r/20220401102752.8599-8-jack@xxxxxxx Signed-off-by: Jens Axboe <axboe@xxxxxxxxx> Signed-off-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> --- block/bfq-cgroup.c | 63 ++++++++++++++++++---------------------------------- block/bfq-iosched.c | 10 -------- block/bfq-iosched.h | 3 -- 3 files changed, 25 insertions(+), 51 deletions(-) --- a/block/bfq-cgroup.c +++ b/block/bfq-cgroup.c @@ -565,27 +565,11 @@ static void bfq_group_set_parent(struct entity->sched_data = &parent->sched_data; } -static struct bfq_group *bfq_lookup_bfqg(struct bfq_data *bfqd, - struct blkcg *blkcg) +static void bfq_link_bfqg(struct bfq_data *bfqd, struct bfq_group *bfqg) { - struct blkcg_gq *blkg; - - blkg = blkg_lookup(blkcg, bfqd->queue); - if (likely(blkg)) - return blkg_to_bfqg(blkg); - return NULL; -} - -struct bfq_group *bfq_find_set_group(struct bfq_data *bfqd, - struct blkcg *blkcg) -{ - struct bfq_group *bfqg, *parent; + struct bfq_group *parent; struct bfq_entity *entity; - bfqg = bfq_lookup_bfqg(bfqd, blkcg); - if (unlikely(!bfqg)) - return NULL; - /* * Update chain of bfq_groups as we might be handling a leaf group * which, along with some of its relatives, has not been hooked yet @@ -602,8 +586,15 @@ struct bfq_group *bfq_find_set_group(str bfq_group_set_parent(curr_bfqg, parent); } } +} - return bfqg; +struct bfq_group *bfq_bio_bfqg(struct bfq_data *bfqd, struct bio *bio) +{ + struct blkcg_gq *blkg = bio->bi_blkg; + + if (!blkg) + return bfqd->root_group; + return blkg_to_bfqg(blkg); } /** @@ -679,25 +670,15 @@ void bfq_bfqq_move(struct bfq_data *bfqd * Move bic to blkcg, assuming that bfqd->lock is held; which makes * sure that the reference to cgroup is valid across the call (see * comments in bfq_bic_update_cgroup on this issue) - * - * NOTE: an alternative approach might have been to store the current - * cgroup in bfqq and getting a reference to it, reducing the lookup - * time here, at the price of slightly more complex code. */ -static struct bfq_group *__bfq_bic_change_cgroup(struct bfq_data *bfqd, - struct bfq_io_cq *bic, - struct blkcg *blkcg) +static void *__bfq_bic_change_cgroup(struct bfq_data *bfqd, + struct bfq_io_cq *bic, + struct bfq_group *bfqg) { struct bfq_queue *async_bfqq = bic_to_bfqq(bic, 0); struct bfq_queue *sync_bfqq = bic_to_bfqq(bic, 1); - struct bfq_group *bfqg; struct bfq_entity *entity; - bfqg = bfq_find_set_group(bfqd, blkcg); - - if (unlikely(!bfqg)) - bfqg = bfqd->root_group; - if (async_bfqq) { entity = &async_bfqq->entity; @@ -749,20 +730,24 @@ static struct bfq_group *__bfq_bic_chang void bfq_bic_update_cgroup(struct bfq_io_cq *bic, struct bio *bio) { struct bfq_data *bfqd = bic_to_bfqd(bic); - struct bfq_group *bfqg = NULL; + struct bfq_group *bfqg = bfq_bio_bfqg(bfqd, bio); uint64_t serial_nr; - rcu_read_lock(); - serial_nr = __bio_blkcg(bio)->css.serial_nr; + serial_nr = bfqg_to_blkg(bfqg)->blkcg->css.serial_nr; /* * Check whether blkcg has changed. The condition may trigger * spuriously on a newly created cic but there's no harm. */ if (unlikely(!bfqd) || likely(bic->blkcg_serial_nr == serial_nr)) - goto out; + return; - bfqg = __bfq_bic_change_cgroup(bfqd, bic, __bio_blkcg(bio)); + /* + * New cgroup for this process. Make sure it is linked to bfq internal + * cgroup hierarchy. + */ + bfq_link_bfqg(bfqd, bfqg); + __bfq_bic_change_cgroup(bfqd, bic, bfqg); /* * Update blkg_path for bfq_log_* functions. We cache this * path, and update it here, for the following @@ -815,8 +800,6 @@ void bfq_bic_update_cgroup(struct bfq_io */ blkg_path(bfqg_to_blkg(bfqg), bfqg->blkg_path, sizeof(bfqg->blkg_path)); bic->blkcg_serial_nr = serial_nr; -out: - rcu_read_unlock(); } /** @@ -1433,7 +1416,7 @@ void bfq_end_wr_async(struct bfq_data *b bfq_end_wr_async_queues(bfqd, bfqd->root_group); } -struct bfq_group *bfq_find_set_group(struct bfq_data *bfqd, struct blkcg *blkcg) +struct bfq_group *bfq_bio_bfqg(struct bfq_data *bfqd, struct bio *bio) { return bfqd->root_group; } --- a/block/bfq-iosched.c +++ b/block/bfq-iosched.c @@ -5158,14 +5158,7 @@ static struct bfq_queue *bfq_get_queue(s struct bfq_queue *bfqq; struct bfq_group *bfqg; - rcu_read_lock(); - - bfqg = bfq_find_set_group(bfqd, __bio_blkcg(bio)); - if (!bfqg) { - bfqq = &bfqd->oom_bfqq; - goto out; - } - + bfqg = bfq_bio_bfqg(bfqd, bio); if (!is_sync) { async_bfqq = bfq_async_queue_prio(bfqd, bfqg, ioprio_class, ioprio); @@ -5209,7 +5202,6 @@ static struct bfq_queue *bfq_get_queue(s out: bfqq->ref++; /* get a process reference to this queue */ bfq_log_bfqq(bfqd, bfqq, "get_queue, at end: %p, %d", bfqq, bfqq->ref); - rcu_read_unlock(); return bfqq; } --- a/block/bfq-iosched.h +++ b/block/bfq-iosched.h @@ -978,8 +978,7 @@ void bfq_bfqq_move(struct bfq_data *bfqd void bfq_init_entity(struct bfq_entity *entity, struct bfq_group *bfqg); void bfq_bic_update_cgroup(struct bfq_io_cq *bic, struct bio *bio); void bfq_end_wr_async(struct bfq_data *bfqd); -struct bfq_group *bfq_find_set_group(struct bfq_data *bfqd, - struct blkcg *blkcg); +struct bfq_group *bfq_bio_bfqg(struct bfq_data *bfqd, struct bio *bio); struct blkcg_gq *bfqg_to_blkg(struct bfq_group *bfqg); struct bfq_group *bfqq_group(struct bfq_queue *bfqq); struct bfq_group *bfq_create_group_hierarchy(struct bfq_data *bfqd, int node); Patches currently in stable-queue which might be from jack@xxxxxxx are queue-5.4/bfq-avoid-merging-queues-with-different-parents.patch queue-5.4/ext4-avoid-cycles-in-directory-h-tree.patch queue-5.4/bfq-drop-pointless-unlock-lock-pair.patch queue-5.4/bfq-split-shared-queues-on-move-between-cgroups.patch queue-5.4/bfq-remove-pointless-bfq_init_rq-calls.patch queue-5.4/dax-fix-cache-flush-on-pmd-mapped-pages.patch queue-5.4/fsnotify-fix-wrong-lockdep-annotations.patch queue-5.4/bfq-track-whether-bfq_group-is-still-online.patch queue-5.4/inotify-show-inotify-mask-flags-in-proc-fdinfo.patch queue-5.4/bfq-make-sure-bfqg-for-which-we-are-queueing-requests-is-online.patch queue-5.4/ext4-fix-bug_on-in-ext4_writepages.patch queue-5.4/fs-writeback-writeback_sb_inodes-recalculate-wrote-according-skipped-pages.patch queue-5.4/ext4-verify-dir-block-before-splitting-it.patch queue-5.4/ext4-fix-use-after-free-in-ext4_rename_dir_prepare.patch queue-5.4/bfq-get-rid-of-__bio_blkcg-usage.patch queue-5.4/bfq-update-cgroup-information-before-merging-bio.patch queue-5.4/block-fix-bio_clone_blkg_association-to-associate-with-proper-blkcg_gq.patch