| Message ID | 20181119035131.11255-5-damien.lemoal@wdc.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Improve I/O priority handling | expand |
On Mon, Nov 19, 2018 at 12:51:28PM +0900, Damien Le Moal wrote: > Define get_current_ioprio() as an inline helper to obtain the caller > I/O priority from its task I/O context. Use this helper in > blk_init_request_from_bio() to set a request ioprio. > > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de>
Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn@suse.de>
On Mon, 2018-11-19 at 12:51 +0900, Damien Le Moal wrote: > Define get_current_ioprio() as an inline helper to obtain the caller > I/O priority from its task I/O context. Use this helper in > blk_init_request_from_bio() to set a request ioprio. > > Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> > --- > block/blk-core.c | 6 +----- > include/linux/ioprio.h | 13 +++++++++++++ > 2 files changed, 14 insertions(+), 5 deletions(-) > > diff --git a/block/blk-core.c b/block/blk-core.c > index 492648c96992..4450d3c08f25 100644 > --- a/block/blk-core.c > +++ b/block/blk-core.c > @@ -813,18 +813,14 @@ unsigned int blk_plug_queued_count(struct > request_queue *q) > > void blk_init_request_from_bio(struct request *req, struct bio *bio) > { > - struct io_context *ioc = current->io_context; > - > if (bio->bi_opf & REQ_RAHEAD) > req->cmd_flags |= REQ_FAILFAST_MASK; > > req->__sector = bio->bi_iter.bi_sector; > if (ioprio_valid(bio_prio(bio))) > req->ioprio = bio_prio(bio); > - else if (ioc) > - req->ioprio = ioc->ioprio; > else > - req->ioprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_NONE, 0); > + req->ioprio = get_current_ioprio(); > req->write_hint = bio->bi_write_hint; > blk_rq_bio_prep(req->q, req, bio); > } > diff --git a/include/linux/ioprio.h b/include/linux/ioprio.h > index 9e30ed6443db..e9bfe6972aed 100644 > --- a/include/linux/ioprio.h > +++ b/include/linux/ioprio.h > @@ -70,6 +70,19 @@ static inline int task_nice_ioclass(struct > task_struct *task) > return IOPRIO_CLASS_BE; > } > > +/* > + * If the calling process has set an I/O priority, use that. > Otherwise, return > + * the default I/O priority. > + */ > +static inline int get_current_ioprio(void) > +{ > + struct io_context *ioc = current->io_context; > + > + if (ioc) > + return ioc->ioprio; > + return IOPRIO_PRIO_VALUE(IOPRIO_CLASS_NONE, 0); Shouldn't this be IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, IOPRIO_NORM) to be consistent with patch 3? > +} > + > /* > * For inheritance, return the highest of the two given priorities > */
Adam, On 2018/11/20 3:18, Adam Manzanares wrote: > On Mon, 2018-11-19 at 12:51 +0900, Damien Le Moal wrote: >> Define get_current_ioprio() as an inline helper to obtain the caller >> I/O priority from its task I/O context. Use this helper in >> blk_init_request_from_bio() to set a request ioprio. >> >> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> >> --- >> block/blk-core.c | 6 +----- >> include/linux/ioprio.h | 13 +++++++++++++ >> 2 files changed, 14 insertions(+), 5 deletions(-) >> >> diff --git a/block/blk-core.c b/block/blk-core.c >> index 492648c96992..4450d3c08f25 100644 >> --- a/block/blk-core.c >> +++ b/block/blk-core.c >> @@ -813,18 +813,14 @@ unsigned int blk_plug_queued_count(struct >> request_queue *q) >> >> void blk_init_request_from_bio(struct request *req, struct bio *bio) >> { >> - struct io_context *ioc = current->io_context; >> - >> if (bio->bi_opf & REQ_RAHEAD) >> req->cmd_flags |= REQ_FAILFAST_MASK; >> >> req->__sector = bio->bi_iter.bi_sector; >> if (ioprio_valid(bio_prio(bio))) >> req->ioprio = bio_prio(bio); >> - else if (ioc) >> - req->ioprio = ioc->ioprio; >> else >> - req->ioprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_NONE, 0); >> + req->ioprio = get_current_ioprio(); >> req->write_hint = bio->bi_write_hint; >> blk_rq_bio_prep(req->q, req, bio); >> } >> diff --git a/include/linux/ioprio.h b/include/linux/ioprio.h >> index 9e30ed6443db..e9bfe6972aed 100644 >> --- a/include/linux/ioprio.h >> +++ b/include/linux/ioprio.h >> @@ -70,6 +70,19 @@ static inline int task_nice_ioclass(struct >> task_struct *task) >> return IOPRIO_CLASS_BE; >> } >> >> +/* >> + * If the calling process has set an I/O priority, use that. >> Otherwise, return >> + * the default I/O priority. >> + */ >> +static inline int get_current_ioprio(void) >> +{ >> + struct io_context *ioc = current->io_context; >> + >> + if (ioc) >> + return ioc->ioprio; >> + return IOPRIO_PRIO_VALUE(IOPRIO_CLASS_NONE, 0); > > Shouldn't this be IOPRIO_PRIO_VALUE(IOPRIO_CLASS_BE, IOPRIO_NORM) to be > consistent with patch 3? I do not think so. The effective I/O priority used in the case of NONE/0 is determined by the scheduler. See the use of task_nice_ioprio() and task_nice_ioclass() in cfq and bfq. So using NONE/0 is correct here I think. Using BE/NORM would render all ioprio_valid() tests useless as BE/NORM is a valid I/O priority. Thinking more of it now, I think patch 3 should actually return by default IOPRIO_PRIO_VALUE(task_nice_ioprio(), task_nice_ioclass()) rather than BE/NORM as this value is only valid if (1) cfq or bfq are in use AND (2) the task scheduling priority and nice values are the default. In any case, I am dropping patch 3 as suggested by Christoph. We can try to revisit this later making sure in the process that user land does not break (if that is at all possible). Best regards.
diff --git a/block/blk-core.c b/block/blk-core.c index 492648c96992..4450d3c08f25 100644 --- a/block/blk-core.c +++ b/block/blk-core.c @@ -813,18 +813,14 @@ unsigned int blk_plug_queued_count(struct request_queue *q) void blk_init_request_from_bio(struct request *req, struct bio *bio) { - struct io_context *ioc = current->io_context; - if (bio->bi_opf & REQ_RAHEAD) req->cmd_flags |= REQ_FAILFAST_MASK; req->__sector = bio->bi_iter.bi_sector; if (ioprio_valid(bio_prio(bio))) req->ioprio = bio_prio(bio); - else if (ioc) - req->ioprio = ioc->ioprio; else - req->ioprio = IOPRIO_PRIO_VALUE(IOPRIO_CLASS_NONE, 0); + req->ioprio = get_current_ioprio(); req->write_hint = bio->bi_write_hint; blk_rq_bio_prep(req->q, req, bio); } diff --git a/include/linux/ioprio.h b/include/linux/ioprio.h index 9e30ed6443db..e9bfe6972aed 100644 --- a/include/linux/ioprio.h +++ b/include/linux/ioprio.h @@ -70,6 +70,19 @@ static inline int task_nice_ioclass(struct task_struct *task) return IOPRIO_CLASS_BE; } +/* + * If the calling process has set an I/O priority, use that. Otherwise, return + * the default I/O priority. + */ +static inline int get_current_ioprio(void) +{ + struct io_context *ioc = current->io_context; + + if (ioc) + return ioc->ioprio; + return IOPRIO_PRIO_VALUE(IOPRIO_CLASS_NONE, 0); +} + /* * For inheritance, return the highest of the two given priorities */
Define get_current_ioprio() as an inline helper to obtain the caller I/O priority from its task I/O context. Use this helper in blk_init_request_from_bio() to set a request ioprio. Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com> --- block/blk-core.c | 6 +----- include/linux/ioprio.h | 13 +++++++++++++ 2 files changed, 14 insertions(+), 5 deletions(-)