From patchwork Thu May 14 06:14:15 2009 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kiyoshi Ueda X-Patchwork-Id: 23697 X-Patchwork-Delegate: agk@redhat.com Received: from hormel.redhat.com (hormel1.redhat.com [209.132.177.33]) by demeter.kernel.org (8.14.2/8.14.2) with ESMTP id n4E6Eewx021493 for ; Thu, 14 May 2009 06:14:40 GMT Received: from listman.util.phx.redhat.com (listman.util.phx.redhat.com [10.8.4.110]) by hormel.redhat.com (Postfix) with ESMTP id 15C92619E1D; Thu, 14 May 2009 02:14:37 -0400 (EDT) Received: from int-mx1.corp.redhat.com (int-mx1.corp.redhat.com [172.16.52.254]) by listman.util.phx.redhat.com (8.13.1/8.13.1) with ESMTP id n4E6EZWa026836 for ; Thu, 14 May 2009 02:14:35 -0400 Received: from mx1.redhat.com (mx1.redhat.com [172.16.48.31]) by int-mx1.corp.redhat.com (8.13.1/8.13.1) with ESMTP id n4E6EYoK023100; Thu, 14 May 2009 02:14:34 -0400 Received: from tyo202.gate.nec.co.jp (TYO202.gate.nec.co.jp [202.32.8.206]) by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id n4E6EKMQ013228; Thu, 14 May 2009 02:14:21 -0400 Received: from mailgate3.nec.co.jp ([10.7.69.162]) by tyo202.gate.nec.co.jp (8.13.8/8.13.4) with ESMTP id n4E6EGGT016903; Thu, 14 May 2009 15:14:16 +0900 (JST) Received: (from root@localhost) by mailgate3.nec.co.jp (8.11.7/3.7W-MAILGATE-NEC) id n4E6EG501950; Thu, 14 May 2009 15:14:16 +0900 (JST) Received: from mailsv.linux.bs1.fc.nec.co.jp (mailsv.linux.bs1.fc.nec.co.jp [10.34.125.2]) by mailsv4.nec.co.jp (8.13.8/8.13.4) with ESMTP id n4E6EGwF018596; Thu, 14 May 2009 15:14:16 +0900 (JST) Received: from elcondor.linux.bs1.fc.nec.co.jp (elcondor.linux.bs1.fc.nec.co.jp [10.34.125.195]) by mailsv.linux.bs1.fc.nec.co.jp (Postfix) with ESMTP id E4647E482A3; Thu, 14 May 2009 15:14:15 +0900 (JST) Message-ID: <4A0BB6B7.9020503@ct.jp.nec.com> Date: Thu, 14 May 2009 15:14:15 +0900 From: Kiyoshi Ueda User-Agent: Thunderbird 2.0.0.21 (X11/20090320) MIME-Version: 1.0 To: Alasdair Kergon References: <49F1729A.1000906@ct.jp.nec.com> <49F1733E.70006@ct.jp.nec.com> <20090509003136.GI1320@agk-dp.fab.redhat.com> In-Reply-To: <20090509003136.GI1320@agk-dp.fab.redhat.com> X-RedHat-Spam-Score: -0.256 X-Scanned-By: MIMEDefang 2.58 on 172.16.52.254 X-Scanned-By: MIMEDefang 2.63 on 172.16.48.31 X-loop: dm-devel@redhat.com Cc: device-mapper development , stefan.bader@canonical.com Subject: [dm-devel] Re: [PATCH 2/3] dm-mpath: add queue-length oriented dynamic load balancer X-BeenThere: dm-devel@redhat.com X-Mailman-Version: 2.1.5 Precedence: junk Reply-To: device-mapper development List-Id: device-mapper development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: dm-devel-bounces@redhat.com Errors-To: dm-devel-bounces@redhat.com Hi Alasdair, Thank you for the review and comments. I'm totally agree with your comments. I found that you have already updated the patch in your editing tree, thanks. I attached patches for remaining parts of your comments. Please review and apply. On 05/09/2009 09:31 AM +0900, Alasdair G Kergon wrote: > On Fri, Apr 24, 2009 at 05:07:26PM +0900, Kiyoshi Ueda wrote: >> + unsigned int repeat_count; > > Just 'unsigned repeat_count' OK. (You have already fixed this.) >> + struct selector *s = (struct selector *) ps->context; > > Drop the cast when it's from void. > > struct selector *s = ps->context; OK. (You have already fixed this.) >> +static int ql_status(struct path_selector *ps, struct dm_path *path, >> + status_type_t type, char *result, unsigned int maxlen) > > unsigned maxlen OK, fixed in rqdm-dlb-02-queue-length-dlb-type-fixes.patch > (We're doing things like this in all new patches, but only changing existing > code when it's touched for some other reason.) OK, I followed the style of dm-round-robin and I understand your preference now. I'll also check the request-based dm patch-set, too, when I update and repost it. >> + int sz = 0; > > signed or unsigned sz? > (It's already used inconsistently, I know - but is unsigned better here?) Yes, I think unsigned is better. (You have already fixed this.) >> + case STATUSTYPE_INFO: >> + DMEMIT("%u ", atomic_read(&pi->qlen)); > > Signed or unsigned? qlen should be always >= 0, but atomic_t is defined as signed. So I use signed here. Fixed in rqdm-dlb-02-queue-length-dlb-type-fixes.patch >> + /* Parse the arguments */ > > Please document parameters and selection algorithm in Documentation dir and > maybe in inline comments too - it's not immediately obvious exactly how it > behaves. > >> +static struct dm_path *ql_select_path(struct path_selector *ps, >> + unsigned *repeat_count, size_t nr_bytes) OK, added comments/documentaion in rqdm-dlb-02-queue-length-dlb-document.patch. Thanks, Kiyoshi Ueda o Use 'unsigned' instead of 'unsigned int' for maxlen in dm-queue-length. o Fix the print format to use %d for qlen in dm-queue-length, since atomic_t is defined as 'int', not 'unsigned int'. Signed-off-by: Kiyoshi Ueda Signed-off-by: Jun'ichi Nomura --- drivers/md/dm-queue-length.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) Add documents/comments for dm-queue-length. Signed-off-by: Kiyoshi Ueda Signed-off-by: Jun'ichi Nomura --- Documentation/device-mapper/dm-queue-length.txt | 39 ++++++++++++++++++++++++ drivers/md/dm-queue-length.c | 12 +++++-- 2 files changed, 48 insertions(+), 3 deletions(-) Index: 2.6.30-rc5/Documentation/device-mapper/dm-queue-length.txt =================================================================== --- /dev/null +++ 2.6.30-rc5/Documentation/device-mapper/dm-queue-length.txt @@ -0,0 +1,39 @@ +dm-queue-length +=============== + +dm-queue-length is a path selector module for device-mapper targets, +which selects a path having the least number of in-flight I/Os. +The path selector name is 'queue-length'. + +Table parameters for each path: [] + : The number of I/Os to dispatch using the selected + path before switching to the next path. + If not given, internal default is used. To check + the default value, see the activated table. + +Status for each path: + : 'A' if the path is active, 'F' if the path is failed. + : The number of path failures. + : The number of in-flight I/Os on the path. + + +Algorithm +========= + +dm-queue-length increments/decrements 'in-flight' when an I/O is +dispatched/completed respectively. +dm-queue-length selects a path having minimum 'in-flight'. + + +Examples +======== +In case that 2 paths (sda and sdb) are used with repeat_count == 128. + +# echo "0 10 multipath 0 0 1 1 queue-length 0 2 1 8:0 128 8:16 128" \ + dmsetup create test +# +# dmsetup table +test: 0 10 multipath 0 0 1 1 queue-length 0 2 1 8:0 128 8:16 128 +# +# dmsetup status +test: 0 10 multipath 2 0 0 0 1 1 E 0 2 1 8:0 A 0 0 8:16 A 0 0 Index: 2.6.30-rc5/drivers/md/dm-queue-length.c =================================================================== --- 2.6.30-rc5.orig/drivers/md/dm-queue-length.c +++ 2.6.30-rc5/drivers/md/dm-queue-length.c @@ -35,7 +35,7 @@ struct path_info { struct list_head list; struct dm_path *path; unsigned repeat_count; - atomic_t qlen; + atomic_t qlen; /* the number of in-flight I/Os */ }; static struct selector *alloc_selector(void) @@ -113,13 +113,16 @@ static int ql_add_path(struct path_selec struct path_info *pi; unsigned repeat_count = QL_MIN_IO; - /* Parse the arguments */ + /* + * Arguments: [] + * : The number of I/Os before switching path. + * If not given, default (QL_MIN_IO) is used. + */ if (argc > 1) { *error = "queue-length ps: incorrect number of arguments"; return -EINVAL; } - /* First path argument is number of I/Os before switching path. */ if ((argc == 1) && (sscanf(argv[0], "%u", &repeat_count) != 1)) { *error = "queue-length ps: invalid repeat count"; return -EINVAL; @@ -161,6 +164,9 @@ static int ql_reinstate_path(struct path return 0; } +/* + * Select a path having the minimum number of in-flight I/Os + */ static struct dm_path *ql_select_path(struct path_selector *ps, unsigned *repeat_count, size_t nr_bytes) { -- dm-devel mailing list dm-devel@redhat.com https://www.redhat.com/mailman/listinfo/dm-devel Index: 2.6.30-rc5/drivers/md/dm-queue-length.c =================================================================== --- 2.6.30-rc5.orig/drivers/md/dm-queue-length.c +++ 2.6.30-rc5/drivers/md/dm-queue-length.c @@ -82,7 +82,7 @@ static void ql_destroy(struct path_selec } static int ql_status(struct path_selector *ps, struct dm_path *path, - status_type_t type, char *result, unsigned int maxlen) + status_type_t type, char *result, unsigned maxlen) { unsigned sz = 0; struct path_info *pi; @@ -95,7 +95,7 @@ static int ql_status(struct path_selecto switch (type) { case STATUSTYPE_INFO: - DMEMIT("%u ", atomic_read(&pi->qlen)); + DMEMIT("%d ", atomic_read(&pi->qlen)); break; case STATUSTYPE_TABLE: DMEMIT("%u ", pi->repeat_count);