svn commit: r238213 - head/sys/geom

[Edward Tomasz Napierała]

Wiadomość napisana przez Pawel Jakub Dawidek w dniu 8 lip 2012, o godz. 23:38:
> On Sun, Jul 08, 2012 at 12:53:37AM +0200, Edward Tomasz Napierała wrote:
>> Wiadomość napisana przez Pawel Jakub Dawidek w dniu 7 lip 2012, o godz. 23:54:
>>> 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. [...]
> 
> I'm afraid that's a lie. From IRC logs:
> 
> <pjd> rwatson: http://people.freebsd.org/~pjd/patches/geom_property_change.patch
> <pjd> rwatson: Not tested.
> <trasz> pjd: shouldn't there also be a flag for geom to veto resizing?
> <trasz> pjd: for classes that can't handle their consumers changing size?
> [the discussion was pretty long]

And when exactly was that?  A year ago?

>> [...] 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.
> 
> What attributes? The ones handled by BIO_GETATTR? They are about
> something totally different.

Yes, but they could be used to notify about mediasize change.

Now, I'm not sure if I like your approach.  You're trying to generalize
from a single case.  And even for that single case your approach would
require a special case to retaste the provider after updating mediasize,
and perhaps do the orphanisation stuff before.

Also, why would we want this generalisation?  Resize handling is completely
different from e.g. rename handling; why should we have a single method
to do both?  It's not that geom structures are like mbufs or vnodes, where
every byte counts.

>>>> +	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?
> 
> I can only suggest not to rewrite GEOM because you didn't take the time
> to understand it.

Let me quote a comment from the top of geom_event.c:

/*
 * XXX: How do we in general know that objects referenced in events
 * have not been destroyed before we get around to handle the event ?
 */

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

>>> 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.
> 
> By introducing such hacks you make the code unpredictable. The way
> g_orphan_provider() works is more than just calling geom's orphan
> method. Also, until now, when orphan method was called it meant that
> provider is going away, which is not true anymore. I'd like to believe
> that you carefully analysed what you changed here is safe, but based on
> your understanding of GEOM, I doubt that.

Look, I really appreciate you're looking at this just six months after
explicitly refusing to talk to me about the design, but it would be great
if it was a _technical_ discussion.

Now, the only reason for the orphaning during resizing is backward
compatibility with classes that don't know anything about the resize()
method, to make sure the provider doesn't get shrunk without them knowing. 
Your patch didn't do that, and perhaps we could just get rid of it.
I think the current approach is safer, though.

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