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>