Messages in this thread | | | From | Jeff Moyer <> | Subject | Re: [PATCH 5/5] blk-mq: make plug work for mutiple disks and queues | Date | Fri, 01 May 2015 16:55:39 -0400 |
| |
Shaohua Li <shli@fb.com> writes:
> Last patch makes plug work for multiple queue case. However it only > works for single disk case, because it assumes only one request in the > plug list. If a task is accessing multiple disks, eg MD/DM, the > assumption is wrong. Let blk_attempt_plug_merge() record request from > the same queue.
I understand the desire to be performant, and that's why you piggy-backed the same_queue_rq onto this function, but it sure looks hackish. I'd almost rather walk the list a second time instead of add warts like this. To be perfectly clear, this is what I really don't like about it: we're relying on the nuance that we will only add a single request per queue to the plug_list in the mq case. When you start to look at what happens for the sq case, it doesn't make much sense at all, as you'll just return the first entry in the list (last one visited walking the list backwards) that has the same request queue. That's fine if this code were mq specific, but it isn't. It will just lead to confusion for others reviewing the code, and may trip up anyone modifying the mq plugging code.
I'll leave it up to Jens to decide if this is fit for inclusion. The patch /functionally/ looks fine to me.
> Cc: Jens Axboe <axboe@fb.com> > Cc: Christoph Hellwig <hch@lst.de> > Signed-off-by: Shaohua Li <shli@fb.com> > --- > block/blk-core.c | 10 +++++++--- > block/blk-mq.c | 11 ++++++----- > block/blk.h | 3 ++- > 3 files changed, 15 insertions(+), 9 deletions(-) > > diff --git a/block/blk-core.c b/block/blk-core.c > index d51ed61..a5e1574 100644 > --- a/block/blk-core.c > +++ b/block/blk-core.c > @@ -1521,7 +1521,8 @@ bool bio_attempt_front_merge(struct request_queue *q, struct request *req, > * Caller must ensure !blk_queue_nomerges(q) beforehand. > */ > bool blk_attempt_plug_merge(struct request_queue *q, struct bio *bio, > - unsigned int *request_count) > + unsigned int *request_count, > + struct request **same_queue_rq) > { > struct blk_plug *plug; > struct request *rq; > @@ -1541,8 +1542,10 @@ bool blk_attempt_plug_merge(struct request_queue *q, struct bio *bio, > list_for_each_entry_reverse(rq, plug_list, queuelist) { > int el_ret; > > - if (rq->q == q) > + if (rq->q == q) { > (*request_count)++; > + *same_queue_rq = rq; > + }
Out of the 3 callers of blk_attempt_plug_merge, only one will use the result, yet all of them have to provide the argument. How about just handling NULL in there?
Cheers, Jeff
| |