| Message ID | 1432329245-5844-29-git-send-email-tj@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri 22-05-15 17:13:42, Tejun Heo wrote: > blk_{set|clear}_queue_congested() take @q and set or clear, > respectively, the congestion state of its bdi's root wb. Because bdi > used to be able to handle congestion state only on the root wb, the > callers of those functions tested whether the congestion is on the > root blkcg and skipped if not. > > This is cumbersome and makes implementation of per cgroup > bdi_writeback congestion state propagation difficult. This patch > renames blk_{set|clear}_queue_congested() to > blk_{set|clear}_congested(), and makes them take request_list instead > of request_queue and test whether the specified request_list is the > root one before updating bdi_writeback congestion state. This makes > the tests in the callers unnecessary and simplifies them. > > As there are no external users of these functions, the definitions are > moved from include/linux/blkdev.h to block/blk-core.c. > > This patch doesn't introduce any noticeable behavior difference. Looks good. You can add: Reviewed-by: Jan Kara <jack@suse.com> BTW, I'd prefer if this was merged with the following patch. I was wondering for a while about the condition at the beginning of blk_clear_congested() only to learn it gets modified to the one I'd expect in the following patch :) Honza > > Signed-off-by: Tejun Heo <tj@kernel.org> > Cc: Jens Axboe <axboe@kernel.dk> > Cc: Jan Kara <jack@suse.cz> > Cc: Vivek Goyal <vgoyal@redhat.com> > --- > block/blk-core.c | 62 ++++++++++++++++++++++++++++++-------------------- > include/linux/blkdev.h | 19 ---------------- > 2 files changed, 37 insertions(+), 44 deletions(-) > > diff --git a/block/blk-core.c b/block/blk-core.c > index e0f726f..b457c4f 100644 > --- a/block/blk-core.c > +++ b/block/blk-core.c > @@ -63,6 +63,28 @@ struct kmem_cache *blk_requestq_cachep; > */ > static struct workqueue_struct *kblockd_workqueue; > > +static void blk_clear_congested(struct request_list *rl, int sync) > +{ > + if (rl != &rl->q->root_rl) > + return; > +#ifdef CONFIG_CGROUP_WRITEBACK > + clear_wb_congested(rl->blkg->wb_congested, sync); > +#else > + clear_wb_congested(rl->q->backing_dev_info.wb.congested, sync); > +#endif > +} > + > +static void blk_set_congested(struct request_list *rl, int sync) > +{ > + if (rl != &rl->q->root_rl) > + return; > +#ifdef CONFIG_CGROUP_WRITEBACK > + set_wb_congested(rl->blkg->wb_congested, sync); > +#else > + set_wb_congested(rl->q->backing_dev_info.wb.congested, sync); > +#endif > +} > + > void blk_queue_congestion_threshold(struct request_queue *q) > { > int nr; > @@ -841,13 +863,8 @@ static void __freed_request(struct request_list *rl, int sync) > { > struct request_queue *q = rl->q; > > - /* > - * bdi isn't aware of blkcg yet. As all async IOs end up root > - * blkcg anyway, just use root blkcg state. > - */ > - if (rl == &q->root_rl && > - rl->count[sync] < queue_congestion_off_threshold(q)) > - blk_clear_queue_congested(q, sync); > + if (rl->count[sync] < queue_congestion_off_threshold(q)) > + blk_clear_congested(rl, sync); > > if (rl->count[sync] + 1 <= q->nr_requests) { > if (waitqueue_active(&rl->wait[sync])) > @@ -880,25 +897,25 @@ static void freed_request(struct request_list *rl, unsigned int flags) > int blk_update_nr_requests(struct request_queue *q, unsigned int nr) > { > struct request_list *rl; > + int on_thresh, off_thresh; > > spin_lock_irq(q->queue_lock); > q->nr_requests = nr; > blk_queue_congestion_threshold(q); > + on_thresh = queue_congestion_on_threshold(q); > + off_thresh = queue_congestion_off_threshold(q); > > - /* congestion isn't cgroup aware and follows root blkcg for now */ > - rl = &q->root_rl; > - > - if (rl->count[BLK_RW_SYNC] >= queue_congestion_on_threshold(q)) > - blk_set_queue_congested(q, BLK_RW_SYNC); > - else if (rl->count[BLK_RW_SYNC] < queue_congestion_off_threshold(q)) > - blk_clear_queue_congested(q, BLK_RW_SYNC); > + blk_queue_for_each_rl(rl, q) { > + if (rl->count[BLK_RW_SYNC] >= on_thresh) > + blk_set_congested(rl, BLK_RW_SYNC); > + else if (rl->count[BLK_RW_SYNC] < off_thresh) > + blk_clear_congested(rl, BLK_RW_SYNC); > > - if (rl->count[BLK_RW_ASYNC] >= queue_congestion_on_threshold(q)) > - blk_set_queue_congested(q, BLK_RW_ASYNC); > - else if (rl->count[BLK_RW_ASYNC] < queue_congestion_off_threshold(q)) > - blk_clear_queue_congested(q, BLK_RW_ASYNC); > + if (rl->count[BLK_RW_ASYNC] >= on_thresh) > + blk_set_congested(rl, BLK_RW_ASYNC); > + else if (rl->count[BLK_RW_ASYNC] < off_thresh) > + blk_clear_congested(rl, BLK_RW_ASYNC); > > - blk_queue_for_each_rl(rl, q) { > if (rl->count[BLK_RW_SYNC] >= q->nr_requests) { > blk_set_rl_full(rl, BLK_RW_SYNC); > } else { > @@ -1008,12 +1025,7 @@ static struct request *__get_request(struct request_list *rl, int rw_flags, > } > } > } > - /* > - * bdi isn't aware of blkcg yet. As all async IOs end up > - * root blkcg anyway, just use root blkcg state. > - */ > - if (rl == &q->root_rl) > - blk_set_queue_congested(q, is_sync); > + blk_set_congested(rl, is_sync); > } > > /* > diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h > index 89bdef0..3d1065c 100644 > --- a/include/linux/blkdev.h > +++ b/include/linux/blkdev.h > @@ -794,25 +794,6 @@ extern int sg_scsi_ioctl(struct request_queue *, struct gendisk *, fmode_t, > > extern void blk_queue_bio(struct request_queue *q, struct bio *bio); > > -/* > - * A queue has just exitted congestion. Note this in the global counter of > - * congested queues, and wake up anyone who was waiting for requests to be > - * put back. > - */ > -static inline void blk_clear_queue_congested(struct request_queue *q, int sync) > -{ > - clear_bdi_congested(&q->backing_dev_info, sync); > -} > - > -/* > - * A queue has just entered congestion. Flag that in the queue's VM-visible > - * state flags and increment the global gounter of congested queues. > - */ > -static inline void blk_set_queue_congested(struct request_queue *q, int sync) > -{ > - set_bdi_congested(&q->backing_dev_info, sync); > -} > - > extern void blk_start_queue(struct request_queue *q); > extern void blk_stop_queue(struct request_queue *q); > extern void blk_sync_queue(struct request_queue *q); > -- > 2.4.0 >
Hello, Jan. On Tue, Jun 30, 2015 at 05:02:54PM +0200, Jan Kara wrote: > BTW, I'd prefer if this was merged with the following patch. I was > wondering for a while about the condition at the beginning of > blk_clear_congested() only to learn it gets modified to the one I'd expect > in the following patch :) The patches are already merged, it's a bit too late to discuss but I usually try to keep each step quite granular. e.g. I try hard to avoid combining code relocation / restructuring with actual functional changes so that the code change A -> B -> C where B is functionally identical to A and C is different from B only where the actual functional changes occur. I think your argument is that as C is the final form, introducing B is actually harder for reviewing. I have to disagree with that pretty strongly. When you only think about the functional transformations A -> C might seem easier but given that we also want to verify the changes - both during development and review - it's far more beneficial to go through the intermediate stage as that isolates functional changes from mere code transformation. Another thing to consider is that there's a difference when one is reviewing a patch series as a whole tracking the development of big picture and later when somebody tries to debug or bisect a bug the patchset introduces. At that point, the general larger flow isn't really in the picture and combining structural and functional changes may make understanding what's going on significantly harder in addition to making such errors more likely and less detectable in the first place. Thanks.
On Wed 01-07-15 21:38:15, Tejun Heo wrote: > Hello, Jan. > > On Tue, Jun 30, 2015 at 05:02:54PM +0200, Jan Kara wrote: > > BTW, I'd prefer if this was merged with the following patch. I was > > wondering for a while about the condition at the beginning of > > blk_clear_congested() only to learn it gets modified to the one I'd expect > > in the following patch :) > > The patches are already merged, it's a bit too late to discuss but I > usually try to keep each step quite granular. e.g. I try hard to > avoid combining code relocation / restructuring with actual functional > changes so that the code change A -> B -> C where B is functionally > identical to A and C is different from B only where the actual > functional changes occur. Yeah, I didn't mean this comment as something you should change even if the series wasn't applied yet (it isn't that bad). I meant it more as a feedback for future. > I think your argument is that as C is the final form, introducing B is > actually harder for reviewing. I have to disagree with that pretty > strongly. When you only think about the functional transformations A > -> C might seem easier but given that we also want to verify the > changes - both during development and review - it's far more > beneficial to go through the intermediate stage as that isolates > functional changes from mere code transformation. > > Another thing to consider is that there's a difference when one is > reviewing a patch series as a whole tracking the development of big > picture and later when somebody tries to debug or bisect a bug the > patchset introduces. At that point, the general larger flow isn't > really in the picture and combining structural and functional changes > may make understanding what's going on significantly harder in > addition to making such errors more likely and less detectable in the > first place. In general I agree with you - separating refactoring from functional changes is useful. I just think you took it a bit to the extreme in this series :) When I'm reviewing patches, I'm also checking whether the function does what it's "supposed" to do. So in case of splitting patches like this I have to go through the series and verify that in the end we end up with what one would expect. And sometimes the correctness is so much easier to verify when changes are split that the extra patch chasing is worth it. But in simple cases like this one, merged patch would have been easier for me. I guess it's a matter of taste... Honza
diff --git a/block/blk-core.c b/block/blk-core.c index e0f726f..b457c4f 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -63,6 +63,28 @@ struct kmem_cache *blk_requestq_cachep; */ static struct workqueue_struct *kblockd_workqueue; +static void blk_clear_congested(struct request_list *rl, int sync) +{ + if (rl != &rl->q->root_rl) + return; +#ifdef CONFIG_CGROUP_WRITEBACK + clear_wb_congested(rl->blkg->wb_congested, sync); +#else + clear_wb_congested(rl->q->backing_dev_info.wb.congested, sync); +#endif +} + +static void blk_set_congested(struct request_list *rl, int sync) +{ + if (rl != &rl->q->root_rl) + return; +#ifdef CONFIG_CGROUP_WRITEBACK + set_wb_congested(rl->blkg->wb_congested, sync); +#else + set_wb_congested(rl->q->backing_dev_info.wb.congested, sync); +#endif +} + void blk_queue_congestion_threshold(struct request_queue *q) { int nr; @@ -841,13 +863,8 @@ static void __freed_request(struct request_list *rl, int sync) { struct request_queue *q = rl->q; - /* - * bdi isn't aware of blkcg yet. As all async IOs end up root - * blkcg anyway, just use root blkcg state. - */ - if (rl == &q->root_rl && - rl->count[sync] < queue_congestion_off_threshold(q)) - blk_clear_queue_congested(q, sync); + if (rl->count[sync] < queue_congestion_off_threshold(q)) + blk_clear_congested(rl, sync); if (rl->count[sync] + 1 <= q->nr_requests) { if (waitqueue_active(&rl->wait[sync])) @@ -880,25 +897,25 @@ static void freed_request(struct request_list *rl, unsigned int flags) int blk_update_nr_requests(struct request_queue *q, unsigned int nr) { struct request_list *rl; + int on_thresh, off_thresh; spin_lock_irq(q->queue_lock); q->nr_requests = nr; blk_queue_congestion_threshold(q); + on_thresh = queue_congestion_on_threshold(q); + off_thresh = queue_congestion_off_threshold(q); - /* congestion isn't cgroup aware and follows root blkcg for now */ - rl = &q->root_rl; - - if (rl->count[BLK_RW_SYNC] >= queue_congestion_on_threshold(q)) - blk_set_queue_congested(q, BLK_RW_SYNC); - else if (rl->count[BLK_RW_SYNC] < queue_congestion_off_threshold(q)) - blk_clear_queue_congested(q, BLK_RW_SYNC); + blk_queue_for_each_rl(rl, q) { + if (rl->count[BLK_RW_SYNC] >= on_thresh) + blk_set_congested(rl, BLK_RW_SYNC); + else if (rl->count[BLK_RW_SYNC] < off_thresh) + blk_clear_congested(rl, BLK_RW_SYNC); - if (rl->count[BLK_RW_ASYNC] >= queue_congestion_on_threshold(q)) - blk_set_queue_congested(q, BLK_RW_ASYNC); - else if (rl->count[BLK_RW_ASYNC] < queue_congestion_off_threshold(q)) - blk_clear_queue_congested(q, BLK_RW_ASYNC); + if (rl->count[BLK_RW_ASYNC] >= on_thresh) + blk_set_congested(rl, BLK_RW_ASYNC); + else if (rl->count[BLK_RW_ASYNC] < off_thresh) + blk_clear_congested(rl, BLK_RW_ASYNC); - blk_queue_for_each_rl(rl, q) { if (rl->count[BLK_RW_SYNC] >= q->nr_requests) { blk_set_rl_full(rl, BLK_RW_SYNC); } else { @@ -1008,12 +1025,7 @@ static struct request *__get_request(struct request_list *rl, int rw_flags, } } } - /* - * bdi isn't aware of blkcg yet. As all async IOs end up - * root blkcg anyway, just use root blkcg state. - */ - if (rl == &q->root_rl) - blk_set_queue_congested(q, is_sync); + blk_set_congested(rl, is_sync); } /* diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index 89bdef0..3d1065c 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -794,25 +794,6 @@ extern int sg_scsi_ioctl(struct request_queue *, struct gendisk *, fmode_t, extern void blk_queue_bio(struct request_queue *q, struct bio *bio); -/* - * A queue has just exitted congestion. Note this in the global counter of - * congested queues, and wake up anyone who was waiting for requests to be - * put back. - */ -static inline void blk_clear_queue_congested(struct request_queue *q, int sync) -{ - clear_bdi_congested(&q->backing_dev_info, sync); -} - -/* - * A queue has just entered congestion. Flag that in the queue's VM-visible - * state flags and increment the global gounter of congested queues. - */ -static inline void blk_set_queue_congested(struct request_queue *q, int sync) -{ - set_bdi_congested(&q->backing_dev_info, sync); -} - extern void blk_start_queue(struct request_queue *q); extern void blk_stop_queue(struct request_queue *q); extern void blk_sync_queue(struct request_queue *q);
blk_{set|clear}_queue_congested() take @q and set or clear, respectively, the congestion state of its bdi's root wb. Because bdi used to be able to handle congestion state only on the root wb, the callers of those functions tested whether the congestion is on the root blkcg and skipped if not. This is cumbersome and makes implementation of per cgroup bdi_writeback congestion state propagation difficult. This patch renames blk_{set|clear}_queue_congested() to blk_{set|clear}_congested(), and makes them take request_list instead of request_queue and test whether the specified request_list is the root one before updating bdi_writeback congestion state. This makes the tests in the callers unnecessary and simplifies them. As there are no external users of these functions, the definitions are moved from include/linux/blkdev.h to block/blk-core.c. This patch doesn't introduce any noticeable behavior difference. Signed-off-by: Tejun Heo <tj@kernel.org> Cc: Jens Axboe <axboe@kernel.dk> Cc: Jan Kara <jack@suse.cz> Cc: Vivek Goyal <vgoyal@redhat.com> --- block/blk-core.c | 62 ++++++++++++++++++++++++++++++-------------------- include/linux/blkdev.h | 19 ---------------- 2 files changed, 37 insertions(+), 44 deletions(-)