svn commit: r238213 - head/sys/geom

[Edward Tomasz Napierała]

Wiadomość napisana przez Pawel Jakub Dawidek w dniu 7 lip 2012, o godz. 23:54:
> 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.

Perhaps it wasn't your original intent, but they are spares.  One of these
was already reused for some other task, btw.

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

I was not aware of that patch.  What I've considered was to use attributes
instead, but that would complicate notifying consumers about resizing
and would require some special-casing in the attribute code.

>> +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?

Copy-pasto, my bad.  Thanks for spotting this.

>> +	hh = arg;
>> +	pp = hh->pp;
>> +	size = hh->size;
> 
> Where do you free the memory allocated for 'hh'?

It should be here, and it will be added soon.

>> +	G_VALID_PROVIDER(pp);
> 
> Is this your protection from a provider going away?

Can you suggest a way to do it in a safe way that doesn't involve
rewriting most of GEOM?

>> +	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.

It's not that using g_orphan_provider() would be safer here - it simply
wouldn't work.  The way it works is by adding providers to a queue
(g_doorstep).  _Providers_, and we need to orphan individual consumers.
So, this would involve rewriting the orphanisation mechanism.  Also,
most of the classes were fixed by mav@ to handle this correctly, IIRC.

>> +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()?

See above.

-- 
If you cut off my head, what would I say?  Me and my head, or me and my body?