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

2016-10-15 Thread Hannes Reinecke 

On 10/14/2016 08:35 PM, Adam Manzananares wrote:

The 10/14/2016 07:54, Hannes Reinecke wrote:

On 10/13/2016 09:53 PM, Adam Manzanares 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 
---
 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);
blk_rq_bio_prep(req->q, req, bio);
 }

diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index c47c358..9a0ceaa 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -934,6 +934,20 @@ static inline unsigned int blk_rq_count_bios(struct 
request *rq)
 }

 /*
+ * blk_rq_set_prio - associate a request with prio from ioc
+ * @rq: request of interest
+ * @ioc: target iocontext
+ *
+ * Assocate request prio with ioc prio so request based drivers
+ * can leverage priority information.
+ */
+static inline void blk_rq_set_prio(struct request *rq, struct io_context *ioc)
+{
+   if (ioc)
+   rq->ioprio = ioc->ioprio;
+}
+
+/*
  * Request issue related functions.
  */
 extern struct request *blk_peek_request(struct request_queue *q);


Don't you need to check for 'ioprio_valid()' here, too?


I poked around and it should be safe to not check for ioprio valid
at this point. ioprio_valid only checks to see if the ioprio is
not IOPRIO_CLASS_NONE. The request by default has a ioprio of none
so if the ioc has ioprio of none we are not changing anything.

The locations in the code that I found where the ioc prio is set are
either filtered through the syscall handler, which checks for invalid
priority combinations, or have valid priority values.


Thanks for the confirmation, that'll clarifies things.

Reviewed-by: Hannes Reinecke 

Cheers,

Hannes
--
Dr. Hannes Reinecke   zSeries & Storage
h...@suse.de  +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-10-14 Thread Adam Manzananares 
The 10/14/2016 07:54, Hannes Reinecke wrote:
> On 10/13/2016 09:53 PM, Adam Manzanares 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 
> > ---
> >  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);
> > blk_rq_bio_prep(req->q, req, bio);
> >  }
> >  
> > diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> > index c47c358..9a0ceaa 100644
> > --- a/include/linux/blkdev.h
> > +++ b/include/linux/blkdev.h
> > @@ -934,6 +934,20 @@ static inline unsigned int blk_rq_count_bios(struct 
> > request *rq)
> >  }
> >  
> >  /*
> > + * blk_rq_set_prio - associate a request with prio from ioc
> > + * @rq: request of interest
> > + * @ioc: target iocontext
> > + *
> > + * Assocate request prio with ioc prio so request based drivers
> > + * can leverage priority information.
> > + */
> > +static inline void blk_rq_set_prio(struct request *rq, struct io_context 
> > *ioc)
> > +{
> > +   if (ioc)
> > +   rq->ioprio = ioc->ioprio;
> > +}
> > +
> > +/*
> >   * Request issue related functions.
> >   */
> >  extern struct request *blk_peek_request(struct request_queue *q);
> > 
> Don't you need to check for 'ioprio_valid()' here, too?

I poked around and it should be safe to not check for ioprio valid
at this point. ioprio_valid only checks to see if the ioprio is 
not IOPRIO_CLASS_NONE. The request by default has a ioprio of none
so if the ioc has ioprio of none we are not changing anything. 

The locations in the code that I found where the ioc prio is set are 
either filtered through the syscall handler, which checks for invalid 
priority combinations, or have valid priority values. 

Take care,
Adam

> 
> Cheers,
> 
> Hannes
> -- 
> Dr. Hannes Reinecke  Teamlead Storage & Networking
> h...@suse.de +49 911 74053 688
> SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
> GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
> HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-10-13 Thread Hannes Reinecke 
On 10/13/2016 09:53 PM, Adam Manzanares 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 
> ---
>  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);
>   blk_rq_bio_prep(req->q, req, bio);
>  }
>  
> diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
> index c47c358..9a0ceaa 100644
> --- a/include/linux/blkdev.h
> +++ b/include/linux/blkdev.h
> @@ -934,6 +934,20 @@ static inline unsigned int blk_rq_count_bios(struct 
> request *rq)
>  }
>  
>  /*
> + * blk_rq_set_prio - associate a request with prio from ioc
> + * @rq: request of interest
> + * @ioc: target iocontext
> + *
> + * Assocate request prio with ioc prio so request based drivers
> + * can leverage priority information.
> + */
> +static inline void blk_rq_set_prio(struct request *rq, struct io_context 
> *ioc)
> +{
> + if (ioc)
> + rq->ioprio = ioc->ioprio;
> +}
> +
> +/*
>   * Request issue related functions.
>   */
>  extern struct request *blk_peek_request(struct request_queue *q);
> 
Don't you need to check for 'ioprio_valid()' here, too?

Cheers,

Hannes
-- 
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-10-13 Thread Adam Manzananares 
The 10/13/2016 14:09, Jens Axboe wrote:
> On 10/13/2016 02:06 PM, Dan Williams wrote:
> >On Thu, Oct 13, 2016 at 12:53 PM, Adam Manzanares
> > 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 
> >>---
> >> 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.

Got it I'll send something out soon.

> 
> -- 
> Jens Axboe
> 

Take care,
Adam
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-10-13 Thread Jens Axboe 

On 10/13/2016 02:59 PM, Dan Williams wrote:

On Thu, Oct 13, 2016 at 1:24 PM, Jens Axboe  wrote:

On 10/13/2016 02:19 PM, Dan Williams wrote:


On Thu, Oct 13, 2016 at 1:09 PM, Jens Axboe  wrote:


On 10/13/2016 02:06 PM, Dan Williams wrote:



On Thu, Oct 13, 2016 at 12:53 PM, Adam Manzanares
 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 
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-10-13 Thread Dan Williams 
On Thu, Oct 13, 2016 at 1:24 PM, Jens Axboe  wrote:
> On 10/13/2016 02:19 PM, Dan Williams wrote:
>>
>> On Thu, Oct 13, 2016 at 1:09 PM, Jens Axboe  wrote:
>>>
>>> On 10/13/2016 02:06 PM, Dan Williams wrote:


 On Thu, Oct 13, 2016 at 12:53 PM, Adam Manzanares
  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 
> ---
>  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.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-10-13 Thread Jens Axboe 

On 10/13/2016 02:19 PM, Dan Williams wrote:

On Thu, Oct 13, 2016 at 1:09 PM, Jens Axboe  wrote:

On 10/13/2016 02:06 PM, Dan Williams wrote:


On Thu, Oct 13, 2016 at 12:53 PM, Adam Manzanares
 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 
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-10-13 Thread Dan Williams 
On Thu, Oct 13, 2016 at 1:09 PM, Jens Axboe  wrote:
> On 10/13/2016 02:06 PM, Dan Williams wrote:
>>
>> On Thu, Oct 13, 2016 at 12:53 PM, Adam Manzanares
>>  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 
>>> ---
>>>  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...
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-10-13 Thread Dan Williams 
On Thu, Oct 13, 2016 at 12:53 PM, Adam Manzanares
 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 
> ---
>  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.
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-10-13 Thread Jens Axboe 

On 10/13/2016 02:06 PM, Dan Williams wrote:

On Thu, Oct 13, 2016 at 12:53 PM, Adam Manzanares
 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 
---
 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

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2016-10-13 Thread Adam Manzanares 
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 
---
 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);
blk_rq_bio_prep(req->q, req, bio);
 }
 
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index c47c358..9a0ceaa 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -934,6 +934,20 @@ static inline unsigned int blk_rq_count_bios(struct 
request *rq)
 }
 
 /*
+ * blk_rq_set_prio - associate a request with prio from ioc
+ * @rq: request of interest
+ * @ioc: target iocontext
+ *
+ * Assocate request prio with ioc prio so request based drivers
+ * can leverage priority information.
+ */
+static inline void blk_rq_set_prio(struct request *rq, struct io_context *ioc)
+{
+   if (ioc)
+   rq->ioprio = ioc->ioprio;
+}
+
+/*
  * Request issue related functions.
  */
 extern struct request *blk_peek_request(struct request_queue *q);
-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html