Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752608AbbEAUzx (ORCPT ); Fri, 1 May 2015 16:55:53 -0400 Received: from mx1.redhat.com ([209.132.183.28]:50475 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751988AbbEAUzt (ORCPT ); Fri, 1 May 2015 16:55:49 -0400 From: Jeff Moyer To: Shaohua Li Cc: , , , Subject: Re: [PATCH 5/5] blk-mq: make plug work for mutiple disks and queues References: <9cb12b99feb2886a21084bf3f93fab773c93c465.1430414610.git.shli@fb.com> X-PGP-KeyID: 1F78E1B4 X-PGP-CertKey: F6FE 280D 8293 F72C 65FD 5A58 1FF8 A7CA 1F78 E1B4 X-PCLoadLetter: What the f**k does that mean? Date: Fri, 01 May 2015 16:55:39 -0400 In-Reply-To: <9cb12b99feb2886a21084bf3f93fab773c93c465.1430414610.git.shli@fb.com> (Shaohua Li's message of "Thu, 30 Apr 2015 10:45:18 -0700") Message-ID: User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.3 (gnu/linux) MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 2762 Lines: 68 Shaohua Li 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 > Cc: Christoph Hellwig > Signed-off-by: Shaohua Li > --- > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/