Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1500529 > unrolled thread

Re: [PATCH v4 1/4] block: Add iocontext priority to request

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH v4 1/4] block: Add iocontext priority to request Dan Williams <dan.j.williams@intel.com> - 2016-10-13 22:20 +0200
    Re: [PATCH v4 1/4] block: Add iocontext priority to request Dan Williams <dan.j.williams@intel.com> - 2016-10-13 22:20 +0200
      Re: [PATCH v4 1/4] block: Add iocontext priority to request Jens Axboe <axboe@kernel.dk> - 2016-10-13 22:50 +0200
        Re: [PATCH v4 1/4] block: Add iocontext priority to request Dan Williams <dan.j.williams@intel.com> - 2016-10-13 23:10 +0200
          Re: [PATCH v4 1/4] block: Add iocontext priority to request Jens Axboe <axboe@kernel.dk> - 2016-10-13 23:10 +0200
    Re: [PATCH v4 1/4] block: Add iocontext priority to request Jens Axboe <axboe@kernel.dk> - 2016-10-13 22:20 +0200

#1500529 — Re: [PATCH v4 1/4] block: Add iocontext priority to request

On Thu, Oct 13, 2016 at 12:53 PM, Adam Manzanares
<adam.manzanares@hgst.com> wrote:
> Patch adds an association between iocontext ioprio and the ioprio of a
> request. This value is set in blk_rq_set_prio which takes the request and
> the ioc as arguments. If the ioc is valid in blk_rq_set_prio then the
> iopriority of the request is set as the iopriority of the ioc. In
> init_request_from_bio a check is made to see if the ioprio of the bio is
> valid and if so then the request prio comes from the bio.
>
> Signed-off-by: Adam Manzananares <adam.manzanares@wdc.com>
> ---
>  block/blk-core.c       |  4 +++-
>  include/linux/blkdev.h | 14 ++++++++++++++
>  2 files changed, 17 insertions(+), 1 deletion(-)
>
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 14d7c07..361b1b9 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -1153,6 +1153,7 @@ static struct request *__get_request(struct request_list *rl, int op,
>
>         blk_rq_init(q, rq);
>         blk_rq_set_rl(rq, rl);
> +       blk_rq_set_prio(rq, ioc);
>         req_set_op_attrs(rq, op, op_flags | REQ_ALLOCED);
>
>         /* init elvpriv */
> @@ -1656,7 +1657,8 @@ void init_request_from_bio(struct request *req, struct bio *bio)
>
>         req->errors = 0;
>         req->__sector = bio->bi_iter.bi_sector;
> -       req->ioprio = bio_prio(bio);
> +       if (ioprio_valid(bio_prio(bio)))
> +               req->ioprio = bio_prio(bio);

Should we use ioprio_best() here?  If req->ioprio and bio_prio()
disagree one side has explicitly asked for a higher priority.

[toc] | [next] | [standalone]

#1500531

On Thu, Oct 13, 2016 at 1:09 PM, Jens Axboe <axboe@kernel.dk> wrote:
> On 10/13/2016 02:06 PM, Dan Williams wrote:
>>
>> On Thu, Oct 13, 2016 at 12:53 PM, Adam Manzanares
>> <adam.manzanares@hgst.com> wrote:
>>>
>>> Patch adds an association between iocontext ioprio and the ioprio of a
>>> request. This value is set in blk_rq_set_prio which takes the request and
>>> the ioc as arguments. If the ioc is valid in blk_rq_set_prio then the
>>> iopriority of the request is set as the iopriority of the ioc. In
>>> init_request_from_bio a check is made to see if the ioprio of the bio is
>>> valid and if so then the request prio comes from the bio.
>>>
>>> Signed-off-by: Adam Manzananares <adam.manzanares@wdc.com>
>>> ---
>>>  block/blk-core.c       |  4 +++-
>>>  include/linux/blkdev.h | 14 ++++++++++++++
>>>  2 files changed, 17 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/block/blk-core.c b/block/blk-core.c
>>> index 14d7c07..361b1b9 100644
>>> --- a/block/blk-core.c
>>> +++ b/block/blk-core.c
>>> @@ -1153,6 +1153,7 @@ static struct request *__get_request(struct
>>> request_list *rl, int op,
>>>
>>>         blk_rq_init(q, rq);
>>>         blk_rq_set_rl(rq, rl);
>>> +       blk_rq_set_prio(rq, ioc);
>>>         req_set_op_attrs(rq, op, op_flags | REQ_ALLOCED);
>>>
>>>         /* init elvpriv */
>>> @@ -1656,7 +1657,8 @@ void init_request_from_bio(struct request *req,
>>> struct bio *bio)
>>>
>>>         req->errors = 0;
>>>         req->__sector = bio->bi_iter.bi_sector;
>>> -       req->ioprio = bio_prio(bio);
>>> +       if (ioprio_valid(bio_prio(bio)))
>>> +               req->ioprio = bio_prio(bio);
>>
>>
>> Should we use ioprio_best() here?  If req->ioprio and bio_prio()
>> disagree one side has explicitly asked for a higher priority.
>
>
> It's a good question - but if priority has been set in the bio, it makes
> sense that that would take priority over the general setting for the
> task/io context. So I think the patch is correct as-is.

Assuming you always trust the kernel to know the right priority...

[toc] | [prev] | [next] | [standalone]

#1500554

On 10/13/2016 02:19 PM, Dan Williams wrote:
> On Thu, Oct 13, 2016 at 1:09 PM, Jens Axboe <axboe@kernel.dk> wrote:
>> On 10/13/2016 02:06 PM, Dan Williams wrote:
>>>
>>> On Thu, Oct 13, 2016 at 12:53 PM, Adam Manzanares
>>> <adam.manzanares@hgst.com> wrote:
>>>>
>>>> Patch adds an association between iocontext ioprio and the ioprio of a
>>>> request. This value is set in blk_rq_set_prio which takes the request and
>>>> the ioc as arguments. If the ioc is valid in blk_rq_set_prio then the
>>>> iopriority of the request is set as the iopriority of the ioc. In
>>>> init_request_from_bio a check is made to see if the ioprio of the bio is
>>>> valid and if so then the request prio comes from the bio.
>>>>
>>>> Signed-off-by: Adam Manzananares <adam.manzanares@wdc.com>
>>>> ---
>>>>  block/blk-core.c       |  4 +++-
>>>>  include/linux/blkdev.h | 14 ++++++++++++++
>>>>  2 files changed, 17 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/block/blk-core.c b/block/blk-core.c
>>>> index 14d7c07..361b1b9 100644
>>>> --- a/block/blk-core.c
>>>> +++ b/block/blk-core.c
>>>> @@ -1153,6 +1153,7 @@ static struct request *__get_request(struct
>>>> request_list *rl, int op,
>>>>
>>>>         blk_rq_init(q, rq);
>>>>         blk_rq_set_rl(rq, rl);
>>>> +       blk_rq_set_prio(rq, ioc);
>>>>         req_set_op_attrs(rq, op, op_flags | REQ_ALLOCED);
>>>>
>>>>         /* init elvpriv */
>>>> @@ -1656,7 +1657,8 @@ void init_request_from_bio(struct request *req,
>>>> struct bio *bio)
>>>>
>>>>         req->errors = 0;
>>>>         req->__sector = bio->bi_iter.bi_sector;
>>>> -       req->ioprio = bio_prio(bio);
>>>> +       if (ioprio_valid(bio_prio(bio)))
>>>> +               req->ioprio = bio_prio(bio);
>>>
>>>
>>> Should we use ioprio_best() here?  If req->ioprio and bio_prio()
>>> disagree one side has explicitly asked for a higher priority.
>>
>>
>> It's a good question - but if priority has been set in the bio, it makes
>> sense that that would take priority over the general setting for the
>> task/io context. So I think the patch is correct as-is.
>
> Assuming you always trust the kernel to know the right priority...

If it set it in the bio, it better know what it's doing. Besides,
there's nothing stopping the caller from checking the task priority when
it sets it. If we do ioprio_best(), then we are effectively preventing
anyone from submitting a bio with a lower priority than the task has
generally set.

Now, this depends on the priority being set in req->ioprio is
exclusively inherited from ioc->ioprio. That's the case for file system
requests, but if someone allocated a request and set the priority
otherwise, then ioprio_best() would be correct.

-- 
Jens Axboe

[toc] | [prev] | [next] | [standalone]

#1500561

On Thu, Oct 13, 2016 at 1:24 PM, Jens Axboe <axboe@kernel.dk> wrote:
> On 10/13/2016 02:19 PM, Dan Williams wrote:
>>
>> On Thu, Oct 13, 2016 at 1:09 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>
>>> On 10/13/2016 02:06 PM, Dan Williams wrote:
>>>>
>>>>
>>>> On Thu, Oct 13, 2016 at 12:53 PM, Adam Manzanares
>>>> <adam.manzanares@hgst.com> wrote:
>>>>>
>>>>>
>>>>> Patch adds an association between iocontext ioprio and the ioprio of a
>>>>> request. This value is set in blk_rq_set_prio which takes the request
>>>>> and
>>>>> the ioc as arguments. If the ioc is valid in blk_rq_set_prio then the
>>>>> iopriority of the request is set as the iopriority of the ioc. In
>>>>> init_request_from_bio a check is made to see if the ioprio of the bio
>>>>> is
>>>>> valid and if so then the request prio comes from the bio.
>>>>>
>>>>> Signed-off-by: Adam Manzananares <adam.manzanares@wdc.com>
>>>>> ---
>>>>>  block/blk-core.c       |  4 +++-
>>>>>  include/linux/blkdev.h | 14 ++++++++++++++
>>>>>  2 files changed, 17 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/block/blk-core.c b/block/blk-core.c
>>>>> index 14d7c07..361b1b9 100644
>>>>> --- a/block/blk-core.c
>>>>> +++ b/block/blk-core.c
>>>>> @@ -1153,6 +1153,7 @@ static struct request *__get_request(struct
>>>>> request_list *rl, int op,
>>>>>
>>>>>         blk_rq_init(q, rq);
>>>>>         blk_rq_set_rl(rq, rl);
>>>>> +       blk_rq_set_prio(rq, ioc);
>>>>>         req_set_op_attrs(rq, op, op_flags | REQ_ALLOCED);
>>>>>
>>>>>         /* init elvpriv */
>>>>> @@ -1656,7 +1657,8 @@ void init_request_from_bio(struct request *req,
>>>>> struct bio *bio)
>>>>>
>>>>>         req->errors = 0;
>>>>>         req->__sector = bio->bi_iter.bi_sector;
>>>>> -       req->ioprio = bio_prio(bio);
>>>>> +       if (ioprio_valid(bio_prio(bio)))
>>>>> +               req->ioprio = bio_prio(bio);
>>>>
>>>>
>>>>
>>>> Should we use ioprio_best() here?  If req->ioprio and bio_prio()
>>>> disagree one side has explicitly asked for a higher priority.
>>>
>>>
>>>
>>> It's a good question - but if priority has been set in the bio, it makes
>>> sense that that would take priority over the general setting for the
>>> task/io context. So I think the patch is correct as-is.
>>
>>
>> Assuming you always trust the kernel to know the right priority...
>
>
> If it set it in the bio, it better know what it's doing. Besides,
> there's nothing stopping the caller from checking the task priority when
> it sets it. If we do ioprio_best(), then we are effectively preventing
> anyone from submitting a bio with a lower priority than the task has
> generally set.

Ah, that makes sense.  Move the ioprio_best() decision out to whatever
code is setting bio_prio() to allow for cases where the kernel knows
best.

[toc] | [prev] | [next] | [standalone]

#1500562

On 10/13/2016 02:59 PM, Dan Williams wrote:
> On Thu, Oct 13, 2016 at 1:24 PM, Jens Axboe <axboe@kernel.dk> wrote:
>> On 10/13/2016 02:19 PM, Dan Williams wrote:
>>>
>>> On Thu, Oct 13, 2016 at 1:09 PM, Jens Axboe <axboe@kernel.dk> wrote:
>>>>
>>>> On 10/13/2016 02:06 PM, Dan Williams wrote:
>>>>>
>>>>>
>>>>> On Thu, Oct 13, 2016 at 12:53 PM, Adam Manzanares
>>>>> <adam.manzanares@hgst.com> wrote:
>>>>>>
>>>>>>
>>>>>> Patch adds an association between iocontext ioprio and the ioprio of a
>>>>>> request. This value is set in blk_rq_set_prio which takes the request
>>>>>> and
>>>>>> the ioc as arguments. If the ioc is valid in blk_rq_set_prio then the
>>>>>> iopriority of the request is set as the iopriority of the ioc. In
>>>>>> init_request_from_bio a check is made to see if the ioprio of the bio
>>>>>> is
>>>>>> valid and if so then the request prio comes from the bio.
>>>>>>
>>>>>> Signed-off-by: Adam Manzananares <adam.manzanares@wdc.com>
>>>>>> ---
>>>>>>  block/blk-core.c       |  4 +++-
>>>>>>  include/linux/blkdev.h | 14 ++++++++++++++
>>>>>>  2 files changed, 17 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/block/blk-core.c b/block/blk-core.c
>>>>>> index 14d7c07..361b1b9 100644
>>>>>> --- a/block/blk-core.c
>>>>>> +++ b/block/blk-core.c
>>>>>> @@ -1153,6 +1153,7 @@ static struct request *__get_request(struct
>>>>>> request_list *rl, int op,
>>>>>>
>>>>>>         blk_rq_init(q, rq);
>>>>>>         blk_rq_set_rl(rq, rl);
>>>>>> +       blk_rq_set_prio(rq, ioc);
>>>>>>         req_set_op_attrs(rq, op, op_flags | REQ_ALLOCED);
>>>>>>
>>>>>>         /* init elvpriv */
>>>>>> @@ -1656,7 +1657,8 @@ void init_request_from_bio(struct request *req,
>>>>>> struct bio *bio)
>>>>>>
>>>>>>         req->errors = 0;
>>>>>>         req->__sector = bio->bi_iter.bi_sector;
>>>>>> -       req->ioprio = bio_prio(bio);
>>>>>> +       if (ioprio_valid(bio_prio(bio)))
>>>>>> +               req->ioprio = bio_prio(bio);
>>>>>
>>>>>
>>>>>
>>>>> Should we use ioprio_best() here?  If req->ioprio and bio_prio()
>>>>> disagree one side has explicitly asked for a higher priority.
>>>>
>>>>
>>>>
>>>> It's a good question - but if priority has been set in the bio, it makes
>>>> sense that that would take priority over the general setting for the
>>>> task/io context. So I think the patch is correct as-is.
>>>
>>>
>>> Assuming you always trust the kernel to know the right priority...
>>
>>
>> If it set it in the bio, it better know what it's doing. Besides,
>> there's nothing stopping the caller from checking the task priority when
>> it sets it. If we do ioprio_best(), then we are effectively preventing
>> anyone from submitting a bio with a lower priority than the task has
>> generally set.
>
> Ah, that makes sense.  Move the ioprio_best() decision out to whatever
> code is setting bio_prio() to allow for cases where the kernel knows
> best.

Yes, precisely.

-- 
Jens Axboe

[toc] | [prev] | [next] | [standalone]

#1500537

On 10/13/2016 02:06 PM, Dan Williams wrote:
> On Thu, Oct 13, 2016 at 12:53 PM, Adam Manzanares
> <adam.manzanares@hgst.com> wrote:
>> Patch adds an association between iocontext ioprio and the ioprio of a
>> request. This value is set in blk_rq_set_prio which takes the request and
>> the ioc as arguments. If the ioc is valid in blk_rq_set_prio then the
>> iopriority of the request is set as the iopriority of the ioc. In
>> init_request_from_bio a check is made to see if the ioprio of the bio is
>> valid and if so then the request prio comes from the bio.
>>
>> Signed-off-by: Adam Manzananares <adam.manzanares@wdc.com>
>> ---
>>  block/blk-core.c       |  4 +++-
>>  include/linux/blkdev.h | 14 ++++++++++++++
>>  2 files changed, 17 insertions(+), 1 deletion(-)
>>
>> diff --git a/block/blk-core.c b/block/blk-core.c
>> index 14d7c07..361b1b9 100644
>> --- a/block/blk-core.c
>> +++ b/block/blk-core.c
>> @@ -1153,6 +1153,7 @@ static struct request *__get_request(struct request_list *rl, int op,
>>
>>         blk_rq_init(q, rq);
>>         blk_rq_set_rl(rq, rl);
>> +       blk_rq_set_prio(rq, ioc);
>>         req_set_op_attrs(rq, op, op_flags | REQ_ALLOCED);
>>
>>         /* init elvpriv */
>> @@ -1656,7 +1657,8 @@ void init_request_from_bio(struct request *req, struct bio *bio)
>>
>>         req->errors = 0;
>>         req->__sector = bio->bi_iter.bi_sector;
>> -       req->ioprio = bio_prio(bio);
>> +       if (ioprio_valid(bio_prio(bio)))
>> +               req->ioprio = bio_prio(bio);
>
> Should we use ioprio_best() here?  If req->ioprio and bio_prio()
> disagree one side has explicitly asked for a higher priority.

It's a good question - but if priority has been set in the bio, it makes
sense that that would take priority over the general setting for the
task/io context. So I think the patch is correct as-is.

Adam, you'll want to rewrite the commit message though. A good commit
message should explain WHY the change is made, not detail the code
implementation of it.

-- 
Jens Axboe

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web