[PATCH 0/3] block: drive-by cleanups for block cgroup

[PATCH 0/3] block: drive-by cleanups for block cgroup

[Johannes Thumshirn]

While reviewing Christoph's bio cleanup series, I noticed
blkcg_bio_issue_check() is way too big for an inline function. When I moved it
into block/blk-cgroup.c I noticed two other functions which only have one or
no callers at all, so I cleaned them up as well.

Johannes Thumshirn (3):
  block: remove blk_queue_root_blkg
  block: move blkcg_bio_issue_check out of line
  block: open-code blkg_path in it's sole caller

 block/bfq-cgroup.c         |  3 +-
 block/blk-cgroup.c         | 56 +++++++++++++++++++++++++
 include/linux/blk-cgroup.h | 85 +-------------------------------------
 3 files changed, 59 insertions(+), 85 deletions(-)

-- 
2.24.1

[PATCH 1/3] block: remove blk_queue_root_blkg

[Johannes Thumshirn]

blk_queue_root_blkg() has no callers, remove it.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 include/linux/blk-cgroup.h | 13 -------------
 1 file changed, 13 deletions(-)

diff --git a/include/linux/blk-cgroup.h b/include/linux/blk-cgroup.h
index 35f8ffe92b70..333885133b1f 100644
--- a/include/linux/blk-cgroup.h
+++ b/include/linux/blk-cgroup.h
@@ -364,17 +364,6 @@ static inline struct blkcg_gq *blkg_lookup(struct blkcg *blkcg,
 	return __blkg_lookup(blkcg, q, false);
 }
 
-/**
- * blk_queue_root_blkg - return blkg for the (blkcg_root, @q) pair
- * @q: request_queue of interest
- *
- * Lookup blkg for @q at the root level. See also blkg_lookup().
- */
-static inline struct blkcg_gq *blk_queue_root_blkg(struct request_queue *q)
-{
-	return q->root_blkg;
-}
-
 /**
  * blkg_to_pdata - get policy private data
  * @blkg: blkg of interest
@@ -707,8 +696,6 @@ static inline bool blk_cgroup_congested(void) { return false; }
 static inline void blkcg_schedule_throttle(struct request_queue *q, bool use_memdelay) { }
 
 static inline struct blkcg_gq *blkg_lookup(struct blkcg *blkcg, void *key) { return NULL; }
-static inline struct blkcg_gq *blk_queue_root_blkg(struct request_queue *q)
-{ return NULL; }
 static inline int blkcg_init_queue(struct request_queue *q) { return 0; }
 static inline void blkcg_exit_queue(struct request_queue *q) { }
 static inline int blkcg_policy_register(struct blkcg_policy *pol) { return 0; }
-- 
2.24.1

[PATCH 2/3] block: move blkcg_bio_issue_check out of line

[Johannes Thumshirn]

blkcg_bio_issue_check() is way to big to be an inline function, move it to
the other blkcg related functions in block/blk-cgroup.c.

According to the bloat-o-meter this brings it's sole caller
generic_make_request_checks() down from 2417 to 1881 bytes (~22%).

$ ./scripts/bloat-o-meter -t vmlinux.old vmlinux
add/remove: 1/0 grow/shrink: 1/1 up/down: 667/-539 (128)
Function                                     old     new   delta
blkcg_bio_issue_check                          -     664    +664
generic_make_request_checks.cold              45      48      +3
generic_make_request_checks                 2372    1833    -539
Total: Before=9624970, After=9625098, chg +0.00%

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 block/blk-cgroup.c         | 56 ++++++++++++++++++++++++++++++++++++
 include/linux/blk-cgroup.h | 58 +-------------------------------------
 2 files changed, 57 insertions(+), 57 deletions(-)

diff --git a/block/blk-cgroup.c b/block/blk-cgroup.c
index c5dc833212e1..9003c76124e8 100644
--- a/block/blk-cgroup.c
+++ b/block/blk-cgroup.c
@@ -1706,6 +1706,62 @@ void blkcg_schedule_throttle(struct request_queue *q, bool use_memdelay)
 	set_notify_resume(current);
 }
 
+bool blkcg_bio_issue_check(struct request_queue *q, struct bio *bio)
+{
+	struct blkcg_gq *blkg;
+	bool throtl = false;
+
+	rcu_read_lock();
+
+	if (!bio->bi_blkg) {
+		char b[BDEVNAME_SIZE];
+
+		WARN_ONCE(1,
+			  "no blkg associated for bio on block-device: %s\n",
+			  bio_devname(bio, b));
+		bio_associate_blkg(bio);
+	}
+
+	blkg = bio->bi_blkg;
+
+	throtl = blk_throtl_bio(q, blkg, bio);
+
+	if (!throtl) {
+		struct blkg_iostat_set *bis;
+		int rwd, cpu;
+
+		if (op_is_discard(bio->bi_opf))
+			rwd = BLKG_IOSTAT_DISCARD;
+		else if (op_is_write(bio->bi_opf))
+			rwd = BLKG_IOSTAT_WRITE;
+		else
+			rwd = BLKG_IOSTAT_READ;
+
+		cpu = get_cpu();
+		bis = per_cpu_ptr(blkg->iostat_cpu, cpu);
+		u64_stats_update_begin(&bis->sync);
+
+		/*
+		 * If the bio is flagged with BIO_QUEUE_ENTERED it means this
+		 * is a split bio and we would have already accounted for the
+		 * size of the bio.
+		 */
+		if (!bio_flagged(bio, BIO_QUEUE_ENTERED))
+			bis->cur.bytes[rwd] += bio->bi_iter.bi_size;
+		bis->cur.ios[rwd]++;
+
+		u64_stats_update_end(&bis->sync);
+		if (cgroup_subsys_on_dfl(io_cgrp_subsys))
+			cgroup_rstat_updated(blkg->blkcg->css.cgroup, cpu);
+		put_cpu();
+	}
+
+	blkcg_bio_issue_init(bio);
+
+	rcu_read_unlock();
+	return !throtl;
+}
+
 /**
  * blkcg_add_delay - add delay to this blkg
  * @blkg: blkg of interest
diff --git a/include/linux/blk-cgroup.h b/include/linux/blk-cgroup.h
index 333885133b1f..b356d4eed08d 100644
--- a/include/linux/blk-cgroup.h
+++ b/include/linux/blk-cgroup.h
@@ -559,63 +559,6 @@ static inline void blkcg_bio_issue_init(struct bio *bio)
 	bio_issue_init(&bio->bi_issue, bio_sectors(bio));
 }
 
-static inline bool blkcg_bio_issue_check(struct request_queue *q,
-					 struct bio *bio)
-{
-	struct blkcg_gq *blkg;
-	bool throtl = false;
-
-	rcu_read_lock();
-
-	if (!bio->bi_blkg) {
-		char b[BDEVNAME_SIZE];
-
-		WARN_ONCE(1,
-			  "no blkg associated for bio on block-device: %s\n",
-			  bio_devname(bio, b));
-		bio_associate_blkg(bio);
-	}
-
-	blkg = bio->bi_blkg;
-
-	throtl = blk_throtl_bio(q, blkg, bio);
-
-	if (!throtl) {
-		struct blkg_iostat_set *bis;
-		int rwd, cpu;
-
-		if (op_is_discard(bio->bi_opf))
-			rwd = BLKG_IOSTAT_DISCARD;
-		else if (op_is_write(bio->bi_opf))
-			rwd = BLKG_IOSTAT_WRITE;
-		else
-			rwd = BLKG_IOSTAT_READ;
-
-		cpu = get_cpu();
-		bis = per_cpu_ptr(blkg->iostat_cpu, cpu);
-		u64_stats_update_begin(&bis->sync);
-
-		/*
-		 * If the bio is flagged with BIO_QUEUE_ENTERED it means this
-		 * is a split bio and we would have already accounted for the
-		 * size of the bio.
-		 */
-		if (!bio_flagged(bio, BIO_QUEUE_ENTERED))
-			bis->cur.bytes[rwd] += bio->bi_iter.bi_size;
-		bis->cur.ios[rwd]++;
-
-		u64_stats_update_end(&bis->sync);
-		if (cgroup_subsys_on_dfl(io_cgrp_subsys))
-			cgroup_rstat_updated(blkg->blkcg->css.cgroup, cpu);
-		put_cpu();
-	}
-
-	blkcg_bio_issue_init(bio);
-
-	rcu_read_unlock();
-	return !throtl;
-}
-
 static inline void blkcg_use_delay(struct blkcg_gq *blkg)
 {
 	if (atomic_add_return(1, &blkg->use_delay) == 1)
@@ -666,6 +609,7 @@ static inline void blkcg_clear_delay(struct blkcg_gq *blkg)
 	}
 }
 
+bool blkcg_bio_issue_check(struct request_queue *q, struct bio *bio);
 void blkcg_add_delay(struct blkcg_gq *blkg, u64 now, u64 delta);
 void blkcg_schedule_throttle(struct request_queue *q, bool use_memdelay);
 void blkcg_maybe_throttle_current(void);
-- 
2.24.1

[PATCH 3/3] block: open-code blkg_path in it's sole caller

[Johannes Thumshirn]

blkg_path() is a trivial one-line helper that only has a single caller,
bfq_bic_update_cgroup().

Remove blkg_path() and open-code it in bfq_bic_update_cgroup().

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 block/bfq-cgroup.c         |  3 ++-
 include/linux/blk-cgroup.h | 14 --------------
 2 files changed, 2 insertions(+), 15 deletions(-)

diff --git a/block/bfq-cgroup.c b/block/bfq-cgroup.c
index 68882b9b8f11..8fe7d47eb4dd 100644
--- a/block/bfq-cgroup.c
+++ b/block/bfq-cgroup.c
@@ -794,7 +794,8 @@ void bfq_bic_update_cgroup(struct bfq_io_cq *bic, struct bio *bio)
 	 * refcounter for bfqg, to let it disappear only after no
 	 * bfq_queue refers to it any longer.
 	 */
-	blkg_path(bfqg_to_blkg(bfqg), bfqg->blkg_path, sizeof(bfqg->blkg_path));
+	cgroup_path(bfqg_to_blkg(bfqg)->blkcg->css.cgroup, bfqg->blkg_path,
+		    sizeof(bfqg->blkg_path));
 	bic->blkcg_serial_nr = serial_nr;
 out:
 	rcu_read_unlock();
diff --git a/include/linux/blk-cgroup.h b/include/linux/blk-cgroup.h
index b356d4eed08d..3e61298fae6a 100644
--- a/include/linux/blk-cgroup.h
+++ b/include/linux/blk-cgroup.h
@@ -433,19 +433,6 @@ static inline void blkcg_unpin_online(struct blkcg *blkcg)
 	} while (blkcg);
 }
 
-/**
- * blkg_path - format cgroup path of blkg
- * @blkg: blkg of interest
- * @buf: target buffer
- * @buflen: target buffer length
- *
- * Format the path of the cgroup of @blkg into @buf.
- */
-static inline int blkg_path(struct blkcg_gq *blkg, char *buf, int buflen)
-{
-	return cgroup_path(blkg->blkcg->css.cgroup, buf, buflen);
-}
-
 /**
  * blkg_get - get a blkg reference
  * @blkg: blkg to get
@@ -655,7 +642,6 @@ static inline struct blkcg *bio_blkcg(struct bio *bio) { return NULL; }
 static inline struct blkg_policy_data *blkg_to_pd(struct blkcg_gq *blkg,
 						  struct blkcg_policy *pol) { return NULL; }
 static inline struct blkcg_gq *pd_to_blkg(struct blkg_policy_data *pd) { return NULL; }
-static inline char *blkg_path(struct blkcg_gq *blkg) { return NULL; }
 static inline void blkg_get(struct blkcg_gq *blkg) { }
 static inline void blkg_put(struct blkcg_gq *blkg) { }
 
-- 
2.24.1

Re: [PATCH 1/3] block: remove blk_queue_root_blkg

[Christoph Hellwig]

On Wed, Apr 29, 2020 at 01:44:32AM +0900, Johannes Thumshirn wrote:
> blk_queue_root_blkg() has no callers, remove it.
> 
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/3] block: move blkcg_bio_issue_check out of line

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Follow up comments below:

> +	rcu_read_lock();
> +
> +	if (!bio->bi_blkg) {
> +		char b[BDEVNAME_SIZE];
> +
> +		WARN_ONCE(1,
> +			  "no blkg associated for bio on block-device: %s\n",
> +			  bio_devname(bio, b));
> +		bio_associate_blkg(bio);
> +	}
> +
> +	blkg = bio->bi_blkg;

We now always assign a bi_blkg, so as a follow on patch we should
probab;y remove this check and assign blkg at the time of declaration.

> +
> +	throtl = blk_throtl_bio(q, blkg, bio);
> +
> +	if (!throtl) {

The empty line hurts my feelings :)

> +		struct blkg_iostat_set *bis;
> +		int rwd, cpu;
> +
> +		if (op_is_discard(bio->bi_opf))
> +			rwd = BLKG_IOSTAT_DISCARD;
> +		else if (op_is_write(bio->bi_opf))
> +			rwd = BLKG_IOSTAT_WRITE;
> +		else
> +			rwd = BLKG_IOSTAT_READ;
> +
> +		cpu = get_cpu();
> +		bis = per_cpu_ptr(blkg->iostat_cpu, cpu);
> +		u64_stats_update_begin(&bis->sync);
> +
> +		/*
> +		 * If the bio is flagged with BIO_QUEUE_ENTERED it means this
> +		 * is a split bio and we would have already accounted for the
> +		 * size of the bio.
> +		 */
> +		if (!bio_flagged(bio, BIO_QUEUE_ENTERED))
> +			bis->cur.bytes[rwd] += bio->bi_iter.bi_size;
> +		bis->cur.ios[rwd]++;
> +
> +		u64_stats_update_end(&bis->sync);
> +		if (cgroup_subsys_on_dfl(io_cgrp_subsys))
> +			cgroup_rstat_updated(blkg->blkcg->css.cgroup, cpu);
> +		put_cpu();

As-is this will clash with my BIO_QUEUE_ENTERED cleanup.

> @@ -666,6 +609,7 @@ static inline void blkcg_clear_delay(struct blkcg_gq *blkg)
>  	}
>  }
>  
> +bool blkcg_bio_issue_check(struct request_queue *q, struct bio *bio);

It might be worth to just throw a IS_ENABLED(CONFIG_BLK_CGROUP) into
the caller and avoid the need for a stub in the header.

Re: [PATCH 3/3] block: open-code blkg_path in it's sole caller

[Christoph Hellwig]

On Wed, Apr 29, 2020 at 01:44:34AM +0900, Johannes Thumshirn wrote:
> blkg_path() is a trivial one-line helper that only has a single caller,
> bfq_bic_update_cgroup().
> 
> Remove blkg_path() and open-code it in bfq_bic_update_cgroup().
> 
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH 2/3] block: move blkcg_bio_issue_check out of line

[Johannes Thumshirn]

On 29/04/2020 09:25, Christoph Hellwig wrote:
> As-is this will clash with my BIO_QUEUE_ENTERED cleanup.

Bah right, I'll re-base once Jens has applied your series.

Re: [PATCH 2/3] block: move blkcg_bio_issue_check out of line

[Johannes Thumshirn]

On 29/04/2020 09:25, Christoph Hellwig wrote:
> Looks good:
> 
> Reviewed-by: Christoph Hellwig <hch@lst.de>
> 
> Follow up comments below:
> 
>> +	rcu_read_lock();
>> +
>> +	if (!bio->bi_blkg) {
>> +		char b[BDEVNAME_SIZE];
>> +
>> +		WARN_ONCE(1,
>> +			  "no blkg associated for bio on block-device: %s\n",
>> +			  bio_devname(bio, b));
>> +		bio_associate_blkg(bio);
>> +	}
>> +
>> +	blkg = bio->bi_blkg;
> 
> We now always assign a bi_blkg, so as a follow on patch we should
> probab;y remove this check and assign blkg at the time of declaration.

But then blkcg_bio_issue_check() can still be called with a bio->bi_blkg 
being NULL.

What am I missing?


>> +
>> +	throtl = blk_throtl_bio(q, blkg, bio);
>> +
>> +	if (!throtl) {
> 
> The empty line hurts my feelings :)

Hehe, fixed.
> 
>> +		struct blkg_iostat_set *bis;
>> +		int rwd, cpu;
>> +
>> +		if (op_is_discard(bio->bi_opf))
>> +			rwd = BLKG_IOSTAT_DISCARD;
>> +		else if (op_is_write(bio->bi_opf))
>> +			rwd = BLKG_IOSTAT_WRITE;
>> +		else
>> +			rwd = BLKG_IOSTAT_READ;
>> +
>> +		cpu = get_cpu();
>> +		bis = per_cpu_ptr(blkg->iostat_cpu, cpu);
>> +		u64_stats_update_begin(&bis->sync);
>> +
>> +		/*
>> +		 * If the bio is flagged with BIO_QUEUE_ENTERED it means this
>> +		 * is a split bio and we would have already accounted for the
>> +		 * size of the bio.
>> +		 */
>> +		if (!bio_flagged(bio, BIO_QUEUE_ENTERED))
>> +			bis->cur.bytes[rwd] += bio->bi_iter.bi_size;
>> +		bis->cur.ios[rwd]++;
>> +
>> +		u64_stats_update_end(&bis->sync);
>> +		if (cgroup_subsys_on_dfl(io_cgrp_subsys))
>> +			cgroup_rstat_updated(blkg->blkcg->css.cgroup, cpu);
>> +		put_cpu();
> 
> As-is this will clash with my BIO_QUEUE_ENTERED cleanup.
> 
>> @@ -666,6 +609,7 @@ static inline void blkcg_clear_delay(struct blkcg_gq *blkg)
>>   	}
>>   }
>>   
>> +bool blkcg_bio_issue_check(struct request_queue *q, struct bio *bio);
> 
> It might be worth to just throw a IS_ENABLED(CONFIG_BLK_CGROUP) into
> the caller and avoid the need for a stub in the header.
> 

Aparently constant propagation doesn't work the why I thought it does:

diff --git a/block/blk-core.c b/block/blk-core.c
index 7f11560bfddb..4111fd759e37 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -988,7 +988,7 @@ generic_make_request_checks(struct bio *bio)
         if (unlikely(!current->io_context))
                 create_task_io_context(current, GFP_ATOMIC, q->node);

-       if (!blkcg_bio_issue_check(q, bio))
+       if (IS_ENABLED(CONFIG_BLK_CGROUP) && !blkcg_bio_issue_check(q, bio))
                 return false;

         if (!bio_flagged(bio, BIO_TRACE_COMPLETION)) {



block/blk-core.c: In function ‘generic_make_request_checks’:
block/blk-core.c:991:40: error: implicit declaration of function 
‘blkcg_bio_issue_check’; did you mean ‘blkcg_bio_issue_init’? 
[-Werror=implicit-function-declaration]
   991 |  if (IS_ENABLED(CONFIG_BLK_CGROUP) && !blkcg_bio_issue_check(q, 
bio))
       |                                        ^~~~~~~~~~~~~~~~~~~~~
       |                                        blkcg_bio_issue_init
cc1: some warnings being treated as errors