Patch to allow gmirror to set priority of a disk

Pawel Jakub Dawidek pjd at FreeBSD.org
Sat Sep 5 09:11:41 UTC 2009


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?).

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.

> +.It Fl p Ar priority
> +Specify priority for given
> +.Ar prov

s/for given/for the given/ ?

> Index: sys/geom/mirror/g_mirror_ctl.c
> ===================================================================
> --- sys/geom/mirror/g_mirror_ctl.c	(revision 196803)
> +++ 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;
>  
>  	nargs = gctl_get_paraml(req, "nargs", sizeof(*nargs));
>  	if (nargs == NULL) {
> @@ -149,6 +149,27 @@
>  		gctl_error(req, "No '%s' argument.", "dynamic");
>  		return;
>  	}
> +	priority = gctl_get_paraml(req, "priority", sizeof(*priority));
> +	if (priority == NULL ) {

Style, extra space after NULL.

> +		gctl_error(req, "No '%s' argument.", "priority");
> +		return;
> +	}
> +	if( *priority < -1 || *priority > 255 ) {

Style, should be:

	if (*priority < -1 || *priority > 255) {

> +		gctl_error(req, "Priority range is 0 to 255, %i given",
> +		    *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. */

Style, multi-line comment should look like this:

	/*
	 * 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 ) {

Style:

	if (*priority > -1) {

> +		if( prov == NULL ) {

Style:

	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");
> @@ -233,6 +254,11 @@
>  			disk->d_flags &= ~G_MIRROR_DISK_FLAG_HARDCODED;
>  		if (!dirty)
>  			disk->d_flags &= ~G_MIRROR_DISK_FLAG_DIRTY;
> +		if (do_priority) {
> +			if (strcmp(disk->d_name, prov) == 0) {
> +				disk->d_priority = *priority;
> +			}

Style, you can drop { } here:

			if (strcmp(disk->d_name, prov) == 0)
				disk->d_priority = *priority;

> +		}
>  		g_mirror_update_metadata(disk);
>  		if (do_sync) {
>  			if (disk->d_state == G_MIRROR_DISK_STATE_STALE) {

Other than those small style issues good job. Now we just have to decide
about mixing per-mirror options with per-provider options.

-- 
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/abba6ecf/attachment.pgp


More information about the freebsd-geom mailing list