Date: Sat, 5 Sep 2009 21:22:51 +0200 From: Pawel Jakub Dawidek <pjd@FreeBSD.org> To: Mel Flynn <mel.flynn+fbsd.fs@mailing.thruhere.net> Cc: freebsd-fs@freebsd.org, freebsd-geom@freebsd.org Subject: Re: Patch to allow gmirror to set priority of a disk Message-ID: <20090905192251.GJ1665@garage.freebsd.pl> In-Reply-To: <200909052111.27667.mel.flynn%2Bfbsd.fs@mailing.thruhere.net> References: <200909030000.11961.mel.flynn%2Bfbsd.fs@mailing.thruhere.net> <200909041016.34677.mel.flynn%2Bfbsd.fs@mailing.thruhere.net> <20090905091030.GC1665@garage.freebsd.pl> <200909052111.27667.mel.flynn%2Bfbsd.fs@mailing.thruhere.net>
next in thread | previous in thread | raw e-mail | index | archive | help
--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--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20090905192251.GJ1665>