Patch to allow gmirror to set priority of a disk

[Pawel Jakub Dawidek]

On Sat, Sep 05, 2009 at 09:11:27PM +0200, Mel Flynn wrote:
> 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"

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?

-- 
Pawel Jakub Dawidek                       http://www.wheel.pl
pjd at FreeBSD.org                           http://www.FreeBSD.org
FreeBSD committer                         Am I Evil? Yes, I Am!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 187 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/freebsd-geom/attachments/20090905/6e2fc844/attachment.pgp