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.
> @@ -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.
> @@ -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;
Finally, please re-order your series as Hannes suggested.
Thanks!
Jeff
--
To unsubscribe from this list: send the line "unsubscribe linux-block" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
