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@xxxxxxx>
>> ---
>> 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.
--
Damien Le Moal
Western Digital Research
