Hello Jeff,
The 10/06/2016 15:46, Jeff Moyer wrote:
> Hi, Adam,
>
> Adam Manzanares <adam.manzanares@xxxxxxxx> writes:
>
> > Patch adds an association between iocontext ioprio and the ioprio of
> > a request. This feature is only enabled if a queue flag is set to
> > indicate that requests should have ioprio associated with them. The
> > queue flag is exposed as the req_prio queue sysfs entry.
> >
> > Signed-off-by: Adam Mananzanares <adam.manzanares@xxxxxxxx>
>
> I like the idea of the patch, but I have a few comments.
>
> First, don't add a tunable, there's no need for it. (And in the future,
> if you do add tunables, document them.) That should make your patch
> much smaller.
>
I have a strong preference for making this a tunable for the following
reason. I am concerned that this could negatively impact performance if this
feature is not properly implemented on a device. In addition, this feature
can make a dramatic difference in the performance of prioritized vs
non-prioritized IO. Priority IO is improved, but it comes at the cost of
non-prioritized IO. If someone has tuned a system in such a way that things
work well as is, I do not want to cause any surprises.
I can see the argument for not having the tunable in the block layer, but
then we need to add a tunable to all request based drivers that may leverage
the iopriority information. This has the potential to generate a lot more
code and documentation. I also would like to use the tunable when the
iopriority is set on the request so we can preserve the default behavior.
This can be a concern when we have drivers that use request iopriority
information, such as the fusion mptsas driver.
I will also document the tunable :) if we agree that it is necessary.
> > @@ -1648,6 +1649,7 @@ out:
> >
> > void init_request_from_bio(struct request *req, struct bio *bio)
> > {
> > + struct io_context *ioc = rq_ioc(bio);
>
> That can return NULL, and you blindly dereference it later.
>
Ouch, this will be cleaned up in the next revision.
> > @@ -1656,7 +1658,11 @@ 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 (blk_queue_req_prio(req->q))
> > + req->ioprio = ioprio_best(bio_prio(bio), ioc->ioprio);
> > + else
> > + req->ioprio = bio_prio(bio);
> > +
>
> If the bio actually has an ioprio (only happens for bcache at this
> point), you should use it. Something like this:
>
> req->ioprio = bio_prio(bio);
> if (!req->ioprio && ioc)
> req->ioprio = ioc->ioprio;
>
I caught this in the explanation of the first patch I sent out. I am still
assuming that this will be a tunable, but I will have the bio_prio take
precedence in the next patch.
> Finally, please re-order your series as Hannes suggested.
Will do.
>
> Thanks!
> Jeff
Take care,
Adam
--
To unsubscribe from this list: send the line "unsubscribe linux-ide" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
