Patch to allow gmirror to set priority of a disk
Pawel Jakub Dawidek
pjd at FreeBSD.org
Sun Sep 6 06:52:25 UTC 2009
On Sun, Sep 06, 2009 at 02:36:10AM +0200, Mel Flynn wrote:
> Hi,
>
> new patch containing everything and points for elegance. Double checked for
> 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 can
> runtime test it tomorrow (tonight is lvl 0 backup).
> Index: sys/geom/mirror/g_mirror_ctl.c
> ===================================================================
> --- 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 = 0, dirty = 1;
> + int *nargs, *priority, do_sync = 0, dirty = 1, do_priority = 0;
priority should be 'intmax_t *' here.
> nargs = gctl_get_paraml(req, "nargs", sizeof(*nargs));
> if (nargs == 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 = gctl_get_paraml(req, "priority", sizeof(*priority));
> + if (priority == 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;
> + }
> + /*
> + * 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 != -1.
> + */
> + prov = gctl_get_asciiparam(req, "arg1");
> + if (*priority > -1) {
> + if (prov == NULL) {
> + gctl_error(req, "Priority needs a disk name");
> + return;
> + }
> + do_priority = 1;
> + }
> if (*autosync && *noautosync) {
> gctl_error(req, "'%s' and '%s' specified.", "autosync",
> "noautosync");
> @@ -189,6 +212,14 @@
> slice = sc->sc_slice;
> else
> slice = *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 != -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 == balance && sc->sc_slice == slice && !*autosync &&
> !*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 != 1) || (do_priority && *nargs != 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) == 0) {
> + disk->d_priority = *priority;
> + g_mirror_update_metadata(disk);
> + break;
> + }
To be consistent with other option I changed it to:
if (strcmp(disk->d_name, prov) == 0) {
if (disk->d_priority == *priority)
gctl_error(req, "Nothing has changed.");
else {
disk->d_priority = *priority;
g_mirror_update_metadata(disk);
}
break;
}
> + continue;
> + }
> if (do_sync) {
> if (disk->d_state == G_MIRROR_DISK_STATE_SYNCHRONIZING)
> disk->d_flags &= ~G_MIRROR_DISK_FLAG_FORCE_SYNC;
> Index: sbin/geom/core/geom.c
> ===================================================================
> --- sbin/geom/core/geom.c (revision 196868)
> +++ sbin/geom/core/geom.c (working copy)
> @@ -98,11 +98,25 @@
> struct g_option *opt;
> unsigned i;
>
> - fprintf(stderr, "%s %s %s", prefix, comm, cmd->gc_name);
> if (cmd->gc_usage != NULL) {
> - fprintf(stderr, " %s\n", cmd->gc_usage);
> + char *pos, *ptr, *sptr;
> +
> + ptr = strdup(cmd->gc_usage);
> + sptr = ptr;
> + while ((pos = strchr(ptr, '\n')) != NULL) {
> + *pos = '\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 = 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) != 0)
> fprintf(stderr, " [-v]");
> for (i = 0; ; i++) {
> Index: sbin/geom/class/mirror/geom_mirror.c
> ===================================================================
> --- sbin/geom/class/mirror/geom_mirror.c (revision 196868)
> +++ 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,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
> ===================================================================
> --- 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 the
> .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
--
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/20090906/2355b9f4/attachment.pgp
More information about the freebsd-fs
mailing list