Patch to allow gmirror to set priority of a disk
Pawel Jakub Dawidek
pjd at FreeBSD.org
Sat Sep 5 19:23:08 UTC 2009
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-fs/attachments/20090905/6e2fc844/attachment.pgp
More information about the freebsd-fs
mailing list