Date: Sun, 6 Sep 2009 08:52:17 +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: <20090906065217.GL1665@garage.freebsd.pl> In-Reply-To: <200909060236.11224.mel.flynn%2Bfbsd.fs@mailing.thruhere.net> References: <200909030000.11961.mel.flynn%2Bfbsd.fs@mailing.thruhere.net> <200909052111.27667.mel.flynn%2Bfbsd.fs@mailing.thruhere.net> <20090905192251.GJ1665@garage.freebsd.pl> <200909060236.11224.mel.flynn%2Bfbsd.fs@mailing.thruhere.net>
next in thread | previous in thread | raw e-mail | index | archive | help
--9ToWwKEyhugL+MAz Content-Type: text/plain; charset=us-ascii Content-Disposition: inline Content-Transfer-Encoding: quoted-printable On Sun, Sep 06, 2009 at 02:36:10AM +0200, Mel Flynn wrote: > Hi, >=20 > new patch containing everything and points for elegance. Double checked f= or=20 > style, so I'm sure there's one or two left ;). Yeah:) I'll fixed what's left by myself and tested it. With few little changes it works fine, good job. Below, for the record, you can find what I changed. Patch committed. > Since it's applies nearly clean (1 hunk -6 lines offset) on 7-STABLE, I c= an=20 > runtime test it tomorrow (tonight is lvl 0 backup). > Index: sys/geom/mirror/g_mirror_ctl.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 > --- sys/geom/mirror/g_mirror_ctl.c (revision 196868) > +++ sys/geom/mirror/g_mirror_ctl.c (working copy) > @@ -93,12 +93,12 @@ > { > struct g_mirror_softc *sc; > struct g_mirror_disk *disk; > - const char *name, *balancep; > + const char *name, *balancep, *prov; > intmax_t *slicep; > uint32_t slice; > uint8_t balance; > int *autosync, *noautosync, *failsync, *nofailsync, *hardcode, *dynamic; > - int *nargs, do_sync =3D 0, dirty =3D 1; > + int *nargs, *priority, do_sync =3D 0, dirty =3D 1, do_priority =3D 0; priority should be 'intmax_t *' here. > nargs =3D gctl_get_paraml(req, "nargs", sizeof(*nargs)); > if (nargs =3D=3D NULL) { We need to accept 1 or 2 arguments few lines below. > @@ -149,6 +149,29 @@ > gctl_error(req, "No '%s' argument.", "dynamic"); > return; > } > + priority =3D gctl_get_paraml(req, "priority", sizeof(*priority)); > + if (priority =3D=3D NULL) { > + gctl_error(req, "No '%s' argument.", "priority"); > + return; > + } > + if (*priority < -1 || *priority > 255) { > + gctl_error(req, "Priority range is 0 to 255, %i given", %d instead of %i for consistency. > + *priority); > + return; > + } > + /*=20 > + * Since we have a priority, we also need a provider now. > + * Note: be WARNS safe, by always assigning prov and only throw an > + * error if *priority !=3D -1. > + */ > + prov =3D gctl_get_asciiparam(req, "arg1"); > + if (*priority > -1) { > + if (prov =3D=3D NULL) { > + gctl_error(req, "Priority needs a disk name"); > + return; > + } > + do_priority =3D 1; > + } > if (*autosync && *noautosync) { > gctl_error(req, "'%s' and '%s' specified.", "autosync", > "noautosync"); > @@ -189,6 +212,14 @@ > slice =3D sc->sc_slice; > else > slice =3D *slicep; > + /* Enforce usage() of -p not allowing any other options */ Missing '.' at the end of the sentence. > + if (do_priority && (*autosync || *noautosync || *failsync || > + *nofailsync || *hardcode || *dynamic || *slicep !=3D -1 || > + (strcmp(balancep, "none")))) { One tab too much. strcmp() returns int, not bool, so we have to check it against 0. Extra () around strcmp(). > + gctl_error(req, "only -p accepted when setting priority"); > + sx_xunlock(&sc->sc_lock); There's no need to hold the lock during gctl_error(). > + return; > + } > if (g_mirror_ndisks(sc, -1) < sc->sc_ndisks) { > sx_xunlock(&sc->sc_lock); > gctl_error(req, "Not all disks connected. Try 'forget' command " > @@ -197,7 +228,7 @@ > } > if (sc->sc_balance =3D=3D balance && sc->sc_slice =3D=3D slice && !*aut= osync && > !*noautosync && !*failsync && !*nofailsync && !*hardcode && > - !*dynamic) { > + !*dynamic && !do_priority) { > sx_xunlock(&sc->sc_lock); > gctl_error(req, "Nothing has changed."); > return; I also added the following check: if ((!do_priority && *nargs !=3D 1) || (do_priority && *nargs !=3D 2)) { sx_xunlock(&sc->sc_lock); gctl_error(req, "Invalid number of arguments."); return; } > @@ -223,6 +254,19 @@ > } > } > LIST_FOREACH(disk, &sc->sc_disks, d_next) { > + /* > + * Handle priority first, since we only need one disk, do one > + * operation on it and then we're done. No need to check other > + * flags, as usage doesn't allow it. > + */ > + if (do_priority) { > + if (strcmp(disk->d_name, prov) =3D=3D 0) { > + disk->d_priority =3D *priority; > + g_mirror_update_metadata(disk); > + break; > + } To be consistent with other option I changed it to: if (strcmp(disk->d_name, prov) =3D=3D 0) { if (disk->d_priority =3D=3D *priority) gctl_error(req, "Nothing has changed."); else { disk->d_priority =3D *priority; g_mirror_update_metadata(disk); } break; } > + continue; > + } > if (do_sync) { > if (disk->d_state =3D=3D G_MIRROR_DISK_STATE_SYNCHRONIZING) > disk->d_flags &=3D ~G_MIRROR_DISK_FLAG_FORCE_SYNC; > Index: sbin/geom/core/geom.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/core/geom.c (revision 196868) > +++ sbin/geom/core/geom.c (working copy) > @@ -98,11 +98,25 @@ > struct g_option *opt; > unsigned i; > =20 > - fprintf(stderr, "%s %s %s", prefix, comm, cmd->gc_name); > if (cmd->gc_usage !=3D NULL) { > - fprintf(stderr, " %s\n", cmd->gc_usage); > + char *pos, *ptr, *sptr; > + > + ptr =3D strdup(cmd->gc_usage); > + sptr =3D ptr; > + while ((pos =3D strchr(ptr, '\n')) !=3D NULL) { > + *pos =3D '\0'; > + fprintf(stderr, "%s %s %s", prefix, comm, cmd->gc_name); > + fprintf(stderr, "%s\n", ptr); We need a space between cmd->gc_name and ptr here. > + ptr =3D pos + 1; > + } > + /* Tail or no \n at all */ > + fprintf(stderr, "%s %s %s", prefix, comm, cmd->gc_name); > + fprintf(stderr, " %s\n", ptr); > + free(sptr); I went with strsep(3) version I proposed, I think you missed it. > return; > } > + > + fprintf(stderr, "%s %s %s", prefix, comm, cmd->gc_name); > if ((cmd->gc_flags & G_FLAG_VERBOSE) !=3D 0) > fprintf(stderr, " [-v]"); > for (i =3D 0; ; i++) { > 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 196868) > +++ sbin/geom/class/mirror/geom_mirror.c (working copy) > @@ -47,7 +47,7 @@ > =20 > static char label_balance[] =3D "split", configure_balance[] =3D "none"; > 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; > =20 > static void mirror_main(struct gctl_req *req, unsigned flags); > static void mirror_activate(struct gctl_req *req); > @@ -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" > + "-p priority name prov" I added '[-v]' here as it i a valid option. > }, > { "deactivate", G_FLAG_VERBOSE, NULL, G_NULL_OPTS, NULL, > "[-v] name prov ..." > Index: sbin/geom/class/mirror/gmirror.8 > =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/gmirror.8 (revision 196868) > +++ sbin/geom/class/mirror/gmirror.8 (working copy) > @@ -49,6 +49,12 @@ > .Op Fl s Ar slice > .Ar name > .Nm > +.Cm configure > +.Op Fl v > +.Fl p Ar priority > +.Ar name > +.Ar prov > +.Nm > .Cm rebuild > .Op Fl v > .Ar name > @@ -115,8 +121,8 @@ > .It Cm label > Create a mirror. > The order of components is important, because a component's priority is = based on its position > -(starting from 0). > -The component with the biggest priority is used by the > +(starting from 0 to 255). > +The component with the biggest priority (the lowest number) is used by t= he > .Cm prefer > balance algorithm > and is also used as a master component when resynchronization is needed, > @@ -175,6 +181,9 @@ We also need: -.Bl -tag -width ".Fl b Ar balance" +.Bl -tag -width ".Fl p Ar priority" > Hardcode providers' names in metadata. > .It Fl n > Turn off autosynchronization of stale components. > +.It Fl p Ar priority > +Specifies priority for the given component > +.Ar prov . > .It Fl s Ar slice > Specifies slice size for > .Cm split --=20 Pawel Jakub Dawidek http://www.wheel.pl pjd@FreeBSD.org http://www.FreeBSD.org FreeBSD committer Am I Evil? Yes, I Am! --9ToWwKEyhugL+MAz Content-Type: application/pgp-signature Content-Disposition: inline -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.4 (FreeBSD) iD8DBQFKo1whForvXbEpPzQRAtt7AJ0RB3pQw7DilUiqBOqgM3LQuEgdNgCgxueH 1QW+WmTfmOoc44Fmz7DiHUo= =/9zD -----END PGP SIGNATURE----- --9ToWwKEyhugL+MAz--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20090906065217.GL1665>