On Tue 04-08-26 14:53:11, Yu Kuai wrote:
> From: Yu Kuai <[email protected]>
>
> blk-cgroup users open-code bio->bi_blkg throughout the policy and
> accounting paths. Some users need the blkg itself, while others
> immediately dereference it to get the associated blkcg. The bio release
> paths also open-code the CONFIG_BLK_CGROUP guarded blkg reference drop
> and field clear.
>
> Add bio_blkg(), bio_blkcg() and bio_clear_blkg() helpers. Convert the
> read-side users to use the accessors, and use bio_clear_blkg() from
> bio_uninit() and bio_endio() so the release path no longer needs to know
> about CONFIG_BLK_CGROUP. Keep the direct bio->bi_blkg stores in the
> association and initialization paths, as those paths still assign the
> stored association.
>
> This keeps the current behavior unchanged while preparing for changing
> what cgroup state a bio stores internally.
>
> Signed-off-by: Yu Kuai <[email protected]>
Mostly looks good. Just I think bio_blkcg() should gracefully handle the
case where bio->bi_blkg is NULL (and return NULL in that case). That way
you can also get rid of somewhat odd pattern:
if (!bio_blkg(bio))
return ...;
do something with bio_blkcg(bio)
You can then just check bio_blkcg(bio) directly which is much more obvious.
Honza
> ---
> block/bfq-cgroup.c | 4 ++--
> block/bio.c | 14 ++------------
> block/blk-cgroup-fc-appid.c | 11 +++++++++--
> block/blk-cgroup.c | 19 ++++++++++---------
> block/blk-cgroup.h | 19 ++++++++++++++++++-
> block/blk-iocost.c | 6 +++---
> block/blk-iolatency.c | 6 +++---
> block/blk-ioprio.c | 2 +-
> block/blk-throttle.c | 2 +-
> block/blk-throttle.h | 2 +-
> include/linux/bio.h | 10 ++++++++++
> 11 files changed, 60 insertions(+), 35 deletions(-)
>
> diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c
> index e82ff03bda02..7e65fe6844ee 100644
> --- a/block/bfq-cgroup.c
> +++ b/block/bfq-cgroup.c
> @@ -363,7 +363,7 @@ void bfqg_and_blkg_put(struct bfq_group *bfqg)
>
> void bfqg_stats_update_legacy_io(struct request_queue *q, struct request *rq)
> {
> - struct bfq_group *bfqg = blkg_to_bfqg(rq->bio->bi_blkg);
> + struct bfq_group *bfqg = blkg_to_bfqg(bio_blkg(rq->bio));
>
> if (!bfqg)
> return;
> @@ -606,7 +606,7 @@ static void bfq_link_bfqg(struct bfq_data *bfqd, struct
> bfq_group *bfqg)
>
> struct bfq_group *bfq_bio_bfqg(struct bfq_data *bfqd, struct bio *bio)
> {
> - struct blkcg_gq *blkg = bio->bi_blkg;
> + struct blkcg_gq *blkg = bio_blkg(bio);
> struct bfq_group *bfqg;
>
> while (blkg) {
> diff --git a/block/bio.c b/block/bio.c
> index 6a2f6fc3413e..c207b248edba 100644
> --- a/block/bio.c
> +++ b/block/bio.c
> @@ -179,12 +179,7 @@ static inline gfp_t try_alloc_gfp(gfp_t gfp)
>
> void bio_uninit(struct bio *bio)
> {
> -#ifdef CONFIG_BLK_CGROUP
> - if (bio->bi_blkg) {
> - blkg_put(bio->bi_blkg);
> - bio->bi_blkg = NULL;
> - }
> -#endif
> + bio_clear_blkg(bio);
> if (bio_integrity(bio))
> bio_integrity_free(bio);
>
> @@ -1803,17 +1798,12 @@ void bio_endio(struct bio *bio)
> goto again;
> }
>
> -#ifdef CONFIG_BLK_CGROUP
> /*
> * Release cgroup info. We shouldn't have to do this here, but quite
> * a few callers of bio_init fail to call bio_uninit, so we cover up
> * for that here at least for now.
> */
> - if (bio->bi_blkg) {
> - blkg_put(bio->bi_blkg);
> - bio->bi_blkg = NULL;
> - }
> -#endif
> + bio_clear_blkg(bio);
>
> if (bio->bi_end_io)
> bio->bi_end_io(bio);
> diff --git a/block/blk-cgroup-fc-appid.c b/block/blk-cgroup-fc-appid.c
> index 3ec21333f393..b2e16e9a7a6c 100644
> --- a/block/blk-cgroup-fc-appid.c
> +++ b/block/blk-cgroup-fc-appid.c
> @@ -50,8 +50,15 @@ EXPORT_SYMBOL_GPL(blkcg_set_fc_appid);
> */
> char *blkcg_get_fc_appid(struct bio *bio)
> {
> - if (!bio->bi_blkg || bio->bi_blkg->blkcg->fc_app_id[0] == '\0')
> + struct blkcg *blkcg;
> +
> + if (!bio_blkg(bio))
> return NULL;
> - return bio->bi_blkg->blkcg->fc_app_id;
> +
> + blkcg = bio_blkcg(bio);
> + if (blkcg->fc_app_id[0] == '\0')
> + return NULL;
> +
> + return blkcg->fc_app_id;
> }
> EXPORT_SYMBOL_GPL(blkcg_get_fc_appid);
> diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
> index 354637f3b158..753a3bdd0e8c 100644
> --- a/block/blk-cgroup.c
> +++ b/block/blk-cgroup.c
> @@ -239,7 +239,7 @@ static void blkg_async_bio_workfn(struct work_struct
> *work)
> */
> void blkcg_punt_bio_submit(struct bio *bio)
> {
> - struct blkcg_gq *blkg = bio->bi_blkg;
> + struct blkcg_gq *blkg = bio_blkg(bio);
>
> if (blkg->parent) {
> spin_lock(&blkg->async_bio_lock);
> @@ -275,9 +275,9 @@ subsys_initcall(blkcg_punt_bio_init);
> */
> struct cgroup_subsys_state *bio_blkcg_css(struct bio *bio)
> {
> - if (!bio || !bio->bi_blkg)
> + if (!bio || !bio_blkg(bio))
> return NULL;
> - return &bio->bi_blkg->blkcg->css;
> + return &bio_blkcg(bio)->css;
> }
> EXPORT_SYMBOL_GPL(bio_blkcg_css);
>
> @@ -2118,8 +2118,8 @@ static inline struct blkcg_gq
> *blkg_tryget_closest(struct bio *bio,
> void bio_associate_blkg_from_css(struct bio *bio,
> struct cgroup_subsys_state *css)
> {
> - if (bio->bi_blkg)
> - blkg_put(bio->bi_blkg);
> + if (bio_blkg(bio))
> + blkg_put(bio_blkg(bio));
>
> if (css && css->parent) {
> bio->bi_blkg = blkg_tryget_closest(bio, css);
> @@ -2146,7 +2146,7 @@ void bio_associate_blkg(struct bio *bio)
> if (blk_op_is_passthrough(bio->bi_opf))
> return;
>
> - if (bio->bi_blkg) {
> + if (bio_blkg(bio)) {
> css = bio_blkcg_css(bio);
> bio_associate_blkg_from_css(bio, css);
> } else {
> @@ -2170,7 +2170,7 @@ EXPORT_SYMBOL_GPL(bio_associate_blkg);
> */
> void bio_clone_blkg_association(struct bio *dst, struct bio *src)
> {
> - if (src->bi_blkg)
> + if (bio_blkg(src))
> bio_associate_blkg_from_css(dst, bio_blkcg_css(src));
> }
> EXPORT_SYMBOL_GPL(bio_clone_blkg_association);
> @@ -2186,7 +2186,8 @@ static int blk_cgroup_io_type(struct bio *bio)
>
> void blk_cgroup_bio_start(struct bio *bio)
> {
> - struct blkcg *blkcg = bio->bi_blkg->blkcg;
> + struct blkcg_gq *blkg = bio_blkg(bio);
> + struct blkcg *blkcg = bio_blkcg(bio);
> int rwd = blk_cgroup_io_type(bio), cpu;
> struct blkg_iostat_set *bis;
> unsigned long flags;
> @@ -2199,7 +2200,7 @@ void blk_cgroup_bio_start(struct bio *bio)
> return;
>
> cpu = get_cpu();
> - bis = per_cpu_ptr(bio->bi_blkg->iostat_cpu, cpu);
> + bis = per_cpu_ptr(blkg->iostat_cpu, cpu);
> flags = u64_stats_update_begin_irqsave(&bis->sync);
>
> /*
> diff --git a/block/blk-cgroup.h b/block/blk-cgroup.h
> index 615390f751aa..1e80b0a73233 100644
> --- a/block/blk-cgroup.h
> +++ b/block/blk-cgroup.h
> @@ -124,6 +124,11 @@ static inline struct blkcg *css_to_blkcg(struct
> cgroup_subsys_state *css)
> return css ? container_of(css, struct blkcg, css) : NULL;
> }
>
> +static inline struct blkcg *bio_blkcg(struct bio *bio)
> +{
> + return bio_blkg(bio)->blkcg;
> +}
> +
> /*
> * A blkcg_gq (blkg) is association between a block cgroup (blkcg) and a
> * request_queue (q). This is used by blkcg policies which need to track
> @@ -343,6 +348,16 @@ static inline void blkg_put(struct blkcg_gq *blkg)
> percpu_ref_put(&blkg->refcnt);
> }
>
> +static inline void bio_clear_blkg(struct bio *bio)
> +{
> + struct blkcg_gq *blkg = bio_blkg(bio);
> +
> + if (blkg) {
> + blkg_put(blkg);
> + bio->bi_blkg = NULL;
> + }
> +}
> +
> /**
> * blkg_for_each_descendant_pre - pre-order walk of a blkg's descendants
> * @d_blkg: loop cursor pointing to the current descendant
> @@ -455,7 +470,7 @@ static inline void blkcg_clear_delay(struct blkcg_gq
> *blkg)
> */
> static inline bool blk_cgroup_mergeable(struct request *rq, struct bio *bio)
> {
> - return rq->bio->bi_blkg == bio->bi_blkg &&
> + return bio_blkg(rq->bio) == bio_blkg(bio) &&
> bio_issue_as_root_blkg(rq->bio) == bio_issue_as_root_blkg(bio);
> }
>
> @@ -481,6 +496,7 @@ struct blkcg_policy {
> struct blkcg {
> };
>
> +static inline struct blkcg *bio_blkcg(struct bio *bio) { return NULL; }
> static inline struct blkcg_gq *blkg_lookup(struct blkcg *blkcg, void *key) {
> return NULL; }
> static inline void blkg_init_queue(struct request_queue *q) { }
> static inline int blkcg_init_disk(struct gendisk *disk) { return 0; }
> @@ -497,6 +513,7 @@ static inline struct blkg_policy_data *blkg_to_pd(struct
> blkcg_gq *blkg,
> static inline struct blkcg_gq *pd_to_blkg(struct blkg_policy_data *pd) {
> return NULL; }
> static inline void blkg_get(struct blkcg_gq *blkg) { }
> static inline void blkg_put(struct blkcg_gq *blkg) { }
> +static inline void bio_clear_blkg(struct bio *bio) { }
> static inline void blk_cgroup_bio_start(struct bio *bio) { }
> static inline bool blk_cgroup_mergeable(struct request *rq, struct bio *bio)
> { return true; }
>
> diff --git a/block/blk-iocost.c b/block/blk-iocost.c
> index 8b2aeba2e1e3..d4470476bcd0 100644
> --- a/block/blk-iocost.c
> +++ b/block/blk-iocost.c
> @@ -2686,7 +2686,7 @@ iocg_handle_over_budget(struct rq_qos *rqos, struct
> ioc_gq *iocg,
>
> static void ioc_rqos_throttle(struct rq_qos *rqos, struct bio *bio)
> {
> - struct blkcg_gq *blkg = bio->bi_blkg;
> + struct blkcg_gq *blkg = bio_blkg(bio);
> struct ioc *ioc = rqos_to_ioc(rqos);
> struct ioc_gq *iocg = blkg_to_iocg(blkg);
> struct ioc_now now;
> @@ -2775,7 +2775,7 @@ static void ioc_rqos_throttle(struct rq_qos *rqos,
> struct bio *bio)
> static void ioc_rqos_merge(struct rq_qos *rqos, struct request *rq,
> struct bio *bio)
> {
> - struct ioc_gq *iocg = blkg_to_iocg(bio->bi_blkg);
> + struct ioc_gq *iocg = blkg_to_iocg(bio_blkg(bio));
> struct ioc *ioc = rqos_to_ioc(rqos);
> sector_t bio_end = bio_end_sector(bio);
> struct ioc_now now;
> @@ -2833,7 +2833,7 @@ static void ioc_rqos_merge(struct rq_qos *rqos, struct
> request *rq,
>
> static void ioc_rqos_done_bio(struct rq_qos *rqos, struct bio *bio)
> {
> - struct ioc_gq *iocg = blkg_to_iocg(bio->bi_blkg);
> + struct ioc_gq *iocg = blkg_to_iocg(bio_blkg(bio));
>
> if (iocg && bio->bi_iocost_cost)
> atomic64_add(bio->bi_iocost_cost, &iocg->done_vtime);
> diff --git a/block/blk-iolatency.c b/block/blk-iolatency.c
> index cef02b6c5fa9..c0d8d5f6bdba 100644
> --- a/block/blk-iolatency.c
> +++ b/block/blk-iolatency.c
> @@ -463,7 +463,7 @@ static void check_scale_change(struct iolatency_grp
> *iolat)
> static void blkcg_iolatency_throttle(struct rq_qos *rqos, struct bio *bio)
> {
> struct blk_iolatency *blkiolat = BLKIOLATENCY(rqos);
> - struct blkcg_gq *blkg = bio->bi_blkg;
> + struct blkcg_gq *blkg = bio_blkg(bio);
> bool issue_as_root = bio_issue_as_root_blkg(bio);
>
> if (!blkiolat->enabled)
> @@ -590,11 +590,11 @@ static void blkcg_iolatency_done_bio(struct rq_qos
> *rqos, struct bio *bio)
> bool issue_as_root = bio_issue_as_root_blkg(bio);
> int inflight = 0;
>
> - blkg = bio->bi_blkg;
> + blkg = bio_blkg(bio);
> if (!blkg || !bio_flagged(bio, BIO_QOS_THROTTLED))
> return;
>
> - iolat = blkg_to_lat(bio->bi_blkg);
> + iolat = blkg_to_lat(blkg);
> if (!iolat)
> return;
>
> diff --git a/block/blk-ioprio.c b/block/blk-ioprio.c
> index 8fa8bca35062..5e3f8e49550d 100644
> --- a/block/blk-ioprio.c
> +++ b/block/blk-ioprio.c
> @@ -132,7 +132,7 @@ static struct blkcg_policy ioprio_policy = {
>
> void blkcg_set_ioprio(struct bio *bio)
> {
> - struct ioprio_blkcg *blkcg = blkcg_to_ioprio_blkcg(bio->bi_blkg->blkcg);
> + struct ioprio_blkcg *blkcg = blkcg_to_ioprio_blkcg(bio_blkcg(bio));
> u16 prio;
>
> if (!blkcg || blkcg->prio_policy == POLICY_NO_CHANGE)
> diff --git a/block/blk-throttle.c b/block/blk-throttle.c
> index ffc3b70065d4..3828c3857900 100644
> --- a/block/blk-throttle.c
> +++ b/block/blk-throttle.c
> @@ -1792,7 +1792,7 @@ static bool tg_within_limit(struct throtl_grp *tg,
> struct bio *bio, bool rw)
> bool __blk_throtl_bio(struct bio *bio)
> {
> struct request_queue *q = bdev_get_queue(bio->bi_bdev);
> - struct blkcg_gq *blkg = bio->bi_blkg;
> + struct blkcg_gq *blkg = bio_blkg(bio);
> struct throtl_qnode *qn = NULL;
> struct throtl_grp *tg = blkg_to_tg(blkg);
> struct throtl_service_queue *sq;
> diff --git a/block/blk-throttle.h b/block/blk-throttle.h
> index 9d7a42c039a1..609a126c7ccb 100644
> --- a/block/blk-throttle.h
> +++ b/block/blk-throttle.h
> @@ -173,7 +173,7 @@ static inline bool blk_should_throtl(struct bio *bio)
> if (!blk_throtl_activated(bio->bi_bdev->bd_queue))
> return false;
>
> - tg = blkg_to_tg(bio->bi_blkg);
> + tg = blkg_to_tg(bio_blkg(bio));
> if (!cgroup_subsys_on_dfl(io_cgrp_subsys)) {
> if (!bio_flagged(bio, BIO_CGROUP_ACCT)) {
> bio_set_flag(bio, BIO_CGROUP_ACCT);
> diff --git a/include/linux/bio.h b/include/linux/bio.h
> index 8f33f717b14f..dc4baa3602b7 100644
> --- a/include/linux/bio.h
> +++ b/include/linux/bio.h
> @@ -503,12 +503,22 @@ static inline void bio_release_pages(struct bio *bio,
> bool mark_dirty)
> disk_devt((bio)->bi_bdev->bd_disk)
>
> #ifdef CONFIG_BLK_CGROUP
> +static inline struct blkcg_gq *bio_blkg(struct bio *bio)
> +{
> + return bio->bi_blkg;
> +}
> +
> void bio_associate_blkg(struct bio *bio);
> void bio_associate_blkg_from_css(struct bio *bio,
> struct cgroup_subsys_state *css);
> void bio_clone_blkg_association(struct bio *dst, struct bio *src);
> void blkcg_punt_bio_submit(struct bio *bio);
> #else /* CONFIG_BLK_CGROUP */
> +static inline struct blkcg_gq *bio_blkg(struct bio *bio)
> +{
> + return NULL;
> +}
> +
> static inline void bio_associate_blkg(struct bio *bio) { }
> static inline void bio_associate_blkg_from_css(struct bio *bio,
> struct cgroup_subsys_state *css)
> --
> 2.51.0
>
--
Jan Kara <[email protected]>
SUSE Labs, CR
