From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jan Kara Subject: Re: [PATCH 28/51] writeback, blkcg: restructure blk_{set|clear}_queue_congested() Date: Tue, 30 Jun 2015 17:02:54 +0200 Message-ID: <20150630150254.GN7252@quack.suse.cz> References: <1432329245-5844-1-git-send-email-tj@kernel.org> <1432329245-5844-29-git-send-email-tj@kernel.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: axboe-tSWWG44O7X1aa/9Udqfwiw@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, jack-AlSwsSmVLrQ@public.gmane.org, hch-wEGCiKHe2LqWVfeAwA7xHQ@public.gmane.org, hannes-druUgvl0LCNAfugRpC6u6w@public.gmane.org, linux-fsdevel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, vgoyal-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, lizefan-hv44wF8Li93QT0dZR+AlfA@public.gmane.org, cgroups-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-mm-Bw31MaZKKs3YtjvyW6yDsg@public.gmane.org, mhocko-AlSwsSmVLrQ@public.gmane.org, clm-b10kYP2dOMg@public.gmane.org, fengguang.wu-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org, david-FqsqvQoI3Ljby3iVrkZq2A@public.gmane.org, gthelen-hpIqsD4AKlfQT0dZR+AlfA@public.gmane.org, khlebnikov-XoJtRXgx1JseBXzfvpsJ4g@public.gmane.org To: Tejun Heo Return-path: Content-Disposition: inline In-Reply-To: <1432329245-5844-29-git-send-email-tj-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> Sender: cgroups-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: linux-fsdevel.vger.kernel.org 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 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 > Cc: Jens Axboe > Cc: Jan Kara > Cc: Vivek Goyal > --- > 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 > -- Jan Kara SUSE Labs, CR