From owner-freebsd-geom@FreeBSD.ORG Sat Sep 5 19:23:08 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 7F452106566B; Sat, 5 Sep 2009 19:23:08 +0000 (UTC) (envelope-from pjd@garage.freebsd.pl) Received: from mail.garage.freebsd.pl (chello087206049004.chello.pl [87.206.49.4]) by mx1.freebsd.org (Postfix) with ESMTP id 11EEE8FC1C; Sat, 5 Sep 2009 19:23:06 +0000 (UTC) Received: by mail.garage.freebsd.pl (Postfix, from userid 65534) id E2FE045E6F; Sat, 5 Sep 2009 21:23:01 +0200 (CEST) Received: from localhost (chello087206049004.chello.pl [87.206.49.4]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mail.garage.freebsd.pl (Postfix) with ESMTP id A8FE145C89; Sat, 5 Sep 2009 21:22:53 +0200 (CEST) Date: Sat, 5 Sep 2009 21:22:51 +0200 From: Pawel Jakub Dawidek To: Mel Flynn Message-ID: <20090905192251.GJ1665@garage.freebsd.pl> References: <200909030000.11961.mel.flynn+fbsd.fs@mailing.thruhere.net> <200909041016.34677.mel.flynn+fbsd.fs@mailing.thruhere.net> <20090905091030.GC1665@garage.freebsd.pl> <200909052111.27667.mel.flynn+fbsd.fs@mailing.thruhere.net> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="hl1kWnBARzJiTscN" Content-Disposition: inline In-Reply-To: <200909052111.27667.mel.flynn+fbsd.fs@mailing.thruhere.net> User-Agent: Mutt/1.4.2.3i X-PGP-Key-URL: http://people.freebsd.org/~pjd/pjd.asc X-OS: FreeBSD 8.0-CURRENT i386 X-Spam-Checker-Version: SpamAssassin 3.0.4 (2005-06-05) on mail.garage.freebsd.pl X-Spam-Level: X-Spam-Status: No, score=-0.6 required=4.5 tests=BAYES_00,RCVD_IN_SORBS_DUL autolearn=no version=3.0.4 Cc: freebsd-fs@freebsd.org, 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:23:08 -0000 --hl1kWnBARzJiTscN Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sat, Sep 05, 2009 at 09:11:27PM +0200, Mel Flynn wrote: > Hi Pawel, >=20 > I'll post-process in the future. I'm so used to if( cond ), cause it allo= ws me=20 > to more easily spot the outer bounds of the condition, it's hard to confo= rm to=20 > style(9) for me. :) >=20 > 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, b= ut > > > > > > 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 t= hat > > > > > 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 > > > =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D > > > --- 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[] =3D "split", configure_balance[] =3D "no= ne"; > > > static intmax_t label_slice =3D 4096, configure_slice =3D -1; > > > -static intmax_t insert_priority =3D 0; > > > +static intmax_t insert_priority =3D 0, configure_priority =3D -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?). >=20 > Right, I found it convenient, but now that I think it about it, it's conf= using=20 > from user perspective. >=20 > > 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. >=20 > 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 ..." > }, >=20 > But it errors if -p is given with illegal option, so either g_command nee= ds to=20 > grow a second usage string, we need to do something like below, or we car= e=20 > less about usage and more about the man page. >=20 > @@ -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" We could also do the following: NULL, "[-adfFhnv] [-b balance] [-s slice] name\n" "-p priority name prov" And tech core geom(8) utility to split usage based on \n. usage_command() function from sbin/geom/core/geom.c would have to be modified. If you agree with this idea, would you also like to work on this? --=20 Pawel Jakub Dawidek http://www.wheel.pl pjd@FreeBSD.org http://www.FreeBSD.org FreeBSD committer Am I Evil? Yes, I Am! --hl1kWnBARzJiTscN Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.4 (FreeBSD) iD8DBQFKorqLForvXbEpPzQRAsJOAJ9x4xCw1I3h+YJ1ycuF1XdC5IsW0ACgxuma BCY5L/33FNiyzNlVRR1Nfg0= =IG5+ -----END PGP SIGNATURE----- --hl1kWnBARzJiTscN--