On 29/04/2020 09:25, Christoph Hellwig wrote:
> Looks good:
>
> Reviewed-by: Christoph Hellwig <hch@xxxxxx>
>
> 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
