From owner-freebsd-geom@FreeBSD.ORG Sat Sep 5 19:11:33 2009 Return-Path: Delivered-To: freebsd-geom@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id 0F63B10656FA; Sat, 5 Sep 2009 19:11:33 +0000 (UTC) (envelope-from mel.flynn+fbsd.fs@mailing.thruhere.net) Received: from mailhub.rachie.is-a-geek.net (rachie.is-a-geek.net [66.230.99.27]) by mx1.freebsd.org (Postfix) with ESMTP id A69848FC14; Sat, 5 Sep 2009 19:11:31 +0000 (UTC) Received: from smoochies.rachie.is-a-geek.net (mailhub.rachie.is-a-geek.net [192.168.2.11]) by mailhub.rachie.is-a-geek.net (Postfix) with ESMTP id 785277E818; Sat, 5 Sep 2009 11:11:42 -0800 (AKDT) From: Mel Flynn To: freebsd-fs@freebsd.org Date: Sat, 5 Sep 2009 21:11:27 +0200 User-Agent: KMail/1.11.4 (FreeBSD/8.0-BETA3; KDE/4.2.4; i386; ; ) References: <200909030000.11961.mel.flynn+fbsd.fs@mailing.thruhere.net> <200909041016.34677.mel.flynn+fbsd.fs@mailing.thruhere.net> <20090905091030.GC1665@garage.freebsd.pl> In-Reply-To: <20090905091030.GC1665@garage.freebsd.pl> MIME-Version: 1.0 Content-Type: Text/Plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200909052111.27667.mel.flynn+fbsd.fs@mailing.thruhere.net> Cc: Pawel Jakub Dawidek , freebsd-geom@freebsd.org Subject: Re: Patch to allow gmirror to set priority of a disk X-BeenThere: freebsd-geom@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: GEOM-specific discussions and implementations List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 05 Sep 2009 19:11:33 -0000 Hi Pawel, I'll post-process in the future. I'm so used to if( cond ), cause it allows me to more easily spot the outer bounds of the condition, it's hard to conform to style(9) for me. :) On Saturday 05 September 2009 11:10:30 Pawel Jakub Dawidek wrote: > On Fri, Sep 04, 2009 at 10:16:34AM +0200, Mel Flynn wrote: > > On Thursday 03 September 2009 15:57:41 Pawel Jakub Dawidek wrote: > > > On Thu, Sep 03, 2009 at 03:48:37PM +0200, Mel Flynn wrote: > > > > On Thursday 03 September 2009 14:44:07 Pawel Jakub Dawidek wrote: > > > > > I'd suggest doing this not as separate gmirror(8) subcommand, but > > > > > as an extension to 'configure' subcommand, where one can provide > > > > > priority by giving '-p' argument. > > > > > > > > Except I didn't see how configure was implemented. Am I correct that > > > > this is g_mirror_ctl_configure in sys/geom/mirror/g_mirror_ctl.c? > > > > > > Yes, you are correct. > > > > Ok, new patch. I gathered from configure_slice, that my assumption was > > about flag handling is correct. Patch is only compile tested. > > In general the patch looks good, although I haven't tested it yet. I do > have some comments though. > > > Index: sbin/geom/class/mirror/geom_mirror.c > > =================================================================== > > --- sbin/geom/class/mirror/geom_mirror.c (revision 196803) > > +++ sbin/geom/class/mirror/geom_mirror.c (working copy) > > @@ -47,7 +47,7 @@ > > > > static char label_balance[] = "split", configure_balance[] = "none"; > > static intmax_t label_slice = 4096, configure_slice = -1; > > -static intmax_t insert_priority = 0; > > +static intmax_t insert_priority = 0, configure_priority = -1; > > > > static void mirror_main(struct gctl_req *req, unsigned flags); > > static void mirror_activate(struct gctl_req *req); > > @@ -71,10 +71,11 @@ > > { 'F', "nofailsync", NULL, G_TYPE_BOOL }, > > { 'h', "hardcode", NULL, G_TYPE_BOOL }, > > { 'n', "noautosync", NULL, G_TYPE_BOOL }, > > + { 'p', "priority", &configure_priority, G_TYPE_NUMBER }, > > { 's', "slice", &configure_slice, G_TYPE_NUMBER }, > > G_OPT_SENTINEL > > }, > > - NULL, "[-adfFhnv] [-b balance] [-s slice] name" > > + NULL, "[-adfFhnv] [-b balance] [-s slice] [-p prio] name [prov]" > > Style, extra space before '[-p prio]'. > > I also wonder if something like this should be possible: > > # gmirror configure -b round-robin -p 1 gm0 da0 > > In you current implementation it will change balance algorithm for all > the providers and in addition priority of da0 provider. This doesn't > look clear for my taste (will balance algorithm be changed on da0 only > or on all providers?). Right, I found it convenient, but now that I think it about it, it's confusing from user perspective. > I'd prefer: > > gmirror configure [-adfFhnv] [-b balance] [-s slice] name > gmirror configure -p prio name prov > > Although I'm not sure if the infrastructure can actually print two usage > patterns. I tried two g_command structs for the same command, like so: @@ -76,6 +76,13 @@ }, NULL, "[-adfFhnv] [-b balance] [-s slice] name" }, + { "configure", G_FLAG_VERBOSE, NULL, + { + { 'p', "priority", &configure_priority, G_TYPE_NUMBER }, + G_OPT_SENTINEL + }, + NULL, "-p prio name prov" + }, { "deactivate", G_FLAG_VERBOSE, NULL, G_NULL_OPTS, NULL, "[-v] name prov ..." }, But it errors if -p is given with illegal option, so either g_command needs to grow a second usage string, we need to do something like below, or we care less about usage and more about the man page. @@ -71,10 +71,12 @@ { 'F', "nofailsync", NULL, G_TYPE_BOOL }, { 'h', "hardcode", NULL, G_TYPE_BOOL }, { 'n', "noautosync", NULL, G_TYPE_BOOL }, + { 'p', "priority", &configure_priority, G_TYPE_NUMBER }, { 's', "slice", &configure_slice, G_TYPE_NUMBER }, G_OPT_SENTINEL }, - NULL, "[-adfFhnv] [-b balance] [-s slice] name" + NULL, "[-adfFhnv] [-b balance] [-s slice] name\n" + " gmirror configure -p priority name prov" }, { "deactivate", G_FLAG_VERBOSE, NULL, G_NULL_OPTS, NULL, "[-v] name prov ..." Fixing style issues now and reimplementing. -- Mel