svn commit: r238213 - head/sys/geom

[Pawel Jakub Dawidek]

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]

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

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

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

-- 
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/20120708/7728b153/attachment.pgp