Date: Sat, 5 Sep 2009 11:10:30 +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: <20090905091030.GC1665@garage.freebsd.pl> In-Reply-To: <200909041016.34677.mel.flynn%2Bfbsd.fs@mailing.thruhere.net> References: <200909030000.11961.mel.flynn%2Bfbsd.fs@mailing.thruhere.net> <200909031548.37887.mel.flynn%2Bfbsd.fs@mailing.thruhere.net> <20090903135741.GK1821@garage.freebsd.pl> <200909041016.34677.mel.flynn%2Bfbsd.fs@mailing.thruhere.net>
next in thread | previous in thread | raw e-mail | index | archive | help
--BwCQnh7xodEAoBMC
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable
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 a=
s 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.
>=20
> Ok, new patch. I gathered from configure_slice, that my assumption was ab=
out=20
> 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
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=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 196803)
> +++ 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,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
> =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=
=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=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 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 =3D 0, dirty =3D 1;
> + int *nargs, *priority, do_sync =3D 0, dirty =3D 1, do_priority =3D 0;
> =20
> nargs =3D gctl_get_paraml(req, "nargs", sizeof(*nargs));
> if (nargs =3D=3D NULL) {
> @@ -149,6 +149,27 @@
> gctl_error(req, "No '%s' argument.", "dynamic");
> return;
> }
> + priority =3D gctl_get_paraml(req, "priority", sizeof(*priority));
> + if (priority =3D=3D 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 !=3D -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 !=3D -1.
*/
> + prov =3D gctl_get_asciiparam(req, "arg1");
> + if( *priority > -1 ) {
Style:
if (*priority > -1) {
> + if( prov =3D=3D NULL ) {
Style:
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");
> @@ -233,6 +254,11 @@
> disk->d_flags &=3D ~G_MIRROR_DISK_FLAG_HARDCODED;
> if (!dirty)
> disk->d_flags &=3D ~G_MIRROR_DISK_FLAG_DIRTY;
> + if (do_priority) {
> + if (strcmp(disk->d_name, prov) =3D=3D 0) {
> + disk->d_priority =3D *priority;
> + }
Style, you can drop { } here:
if (strcmp(disk->d_name, prov) =3D=3D 0)
disk->d_priority =3D *priority;
> + }
> g_mirror_update_metadata(disk);
> if (do_sync) {
> if (disk->d_state =3D=3D 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.
--=20
Pawel Jakub Dawidek http://www.wheel.pl
pjd@FreeBSD.org http://www.FreeBSD.org
FreeBSD committer Am I Evil? Yes, I Am!
--BwCQnh7xodEAoBMC
Content-Type: application/pgp-signature
Content-Disposition: inline
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.4 (FreeBSD)
iD8DBQFKoisGForvXbEpPzQRAgdSAKDocMkpE/BjAhd6QIoENWG9akbmpACg0IBr
F3pPqlL3ZenVeyBi0HK1jjQ=
=p00c
-----END PGP SIGNATURE-----
--BwCQnh7xodEAoBMC--
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20090905091030.GC1665>