svn commit: r238213 - head/sys/geom

[Pawel Jakub Dawidek]

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
> 
> Log:
>   Add a new GEOM method, resize(), which is called after provider size changes.
>   Add a new routine, g_resize_provider(), to use to notify GEOM about provider
>   change.
>   
>   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 == 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 = arg;
> +	pp = hh->pp;
> +	size = 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 = cp->geom;
> +		if (gp->resize == 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 == pp->mediasize)
> +		return;
> +
> +	hh = g_malloc(sizeof *hh, M_WAITOK | M_ZERO);
> +	hh->pp = pp;

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

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

-- 
Pawel Jakub Dawidek                       http://www.wheelsystems.com
FreeBSD committer                         http://www.FreeBSD.org
Am I Evil? Yes, I Am!                     http://tupytaj.pl
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 196 bytes
Desc: not available
Url : http://lists.freebsd.org/pipermail/svn-src-all/attachments/20120707/f7bce6bd/attachment.pgp