Skip site navigation (1)Skip section navigation (2)
Date:      Sat, 7 Jul 2012 23:54:25 +0200
From:      Pawel Jakub Dawidek <pjd@FreeBSD.org>
To:        Edward Tomasz Napierala <trasz@FreeBSD.org>
Cc:        svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org
Subject:   Re: svn commit: r238213 - head/sys/geom
Message-ID:  <20120707215424.GE1437@garage.freebsd.pl>
In-Reply-To: <201207072013.q67KDfHN082943@svn.freebsd.org>
References:  <201207072013.q67KDfHN082943@svn.freebsd.org>

next in thread | previous in thread | raw e-mail | index | archive | help

--qOrJKOH36bD5yhNe
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Sat, Jul 07, 2012 at 08:13:41PM +0000, Edward Tomasz Napierala wrote:
> Author: trasz
> Date: Sat Jul  7 20:13:40 2012
> New Revision: 238213
> URL: http://svn.freebsd.org/changeset/base/238213
>=20
> Log:
>   Add a new GEOM method, resize(), which is called after provider size ch=
anges.
>   Add a new routine, g_resize_provider(), to use to notify GEOM about pro=
vider
>   change.
>  =20
>   Reviewed by:	mav
>   Sponsored by:	FreeBSD Foundation
[...]
> -	void			*spare2;
> +	g_resize_t		*resize;
[...]
> -	void			*spare1;
> +	g_resize_t		*resize;
[...]

If you take the time to actually read the commit log from the change
that added those spare fields, you will notice they were not added for
you to consume them.

You will also notice that one of those fields were left for more
universal method to handle various provider's property changes (ie.
provider's name, apart from its mediasize). The initial patch was even
published a year ago:

	http://people.freebsd.org/~pjd/patches/geom_property_change.patch

Even if it was somehow totally not reusable it would at least give you a
hint that mediasize is not the only thing that can change and if we are
making that change it should be done right.

> +static void
> +g_resize_provider_event(void *arg, int flag)
> +{
[...]
> +	if (flag =3D=3D EV_CANCEL)
> +		return;

How it can be canceled? Because I'm clearly missing something. You post
this event without giving any pointers, so how g_cancel_event() can find
this event can cancel it?

> +	hh =3D arg;
> +	pp =3D hh->pp;
> +	size =3D hh->size;

Where do you free the memory allocated for 'hh'?

> +	G_VALID_PROVIDER(pp);

Is this your protection from a provider going away?

> +	LIST_FOREACH_SAFE(cp, &pp->consumers, consumers, cp2) {
> +		gp =3D cp->geom;
> +		if (gp->resize =3D=3D NULL && size < pp->mediasize)
> +			cp->geom->orphan(cp);
> +	}

Why is this safe to call the orphan method directly and not use
g_orphan_provider()? I don't know if using g_orphan_provider() is safe
to use here either, but I'm under impression that you assume no orphan
method will ever drop the topology lock? We have tools to assert that,
no need to introduce such weak assumptions.

> +void
> +g_resize_provider(struct g_provider *pp, off_t size)
> +{
> +	struct g_hh00 *hh;
> +
> +	G_VALID_PROVIDER(pp);
> +
> +	if (size =3D=3D pp->mediasize)
> +		return;
> +
> +	hh =3D g_malloc(sizeof *hh, M_WAITOK | M_ZERO);
> +	hh->pp =3D pp;

Care to explain why the provider can't disappear between now and the
event thread calling g_resize_provider_event()?

> +	hh->size =3D size;
> +	g_post_event(g_resize_provider_event, hh, M_WAITOK, NULL);

--=20
Pawel Jakub Dawidek                       http://www.wheelsystems.com
FreeBSD committer                         http://www.FreeBSD.org
Am I Evil? Yes, I Am!                     http://tupytaj.pl

--qOrJKOH36bD5yhNe
Content-Type: application/pgp-signature

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2.0.19 (FreeBSD)

iEYEARECAAYFAk/4sBAACgkQForvXbEpPzTuBgCg5ONWpK+E0E3m67EjNXYif4YZ
bREAoIeDpaV+3k3CFUYa+0yTeJkuz13D
=WhYV
-----END PGP SIGNATURE-----

--qOrJKOH36bD5yhNe--



Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20120707215424.GE1437>