Date: Mon, 9 Jul 2012 09:19:29 +0200 From: =?iso-8859-2?Q?Edward_Tomasz_Napiera=B3a?= <trasz@FreeBSD.org> To: Pawel Jakub Dawidek <pjd@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: <739062A0-4339-46A3-AF8E-193BC57758BA@FreeBSD.org> In-Reply-To: <20120708213846.GF1437@garage.freebsd.pl> References: <201207072013.q67KDfHN082943@svn.freebsd.org> <20120707215424.GE1437@garage.freebsd.pl> <280C8AEE-F7E8-4AAE-87BF-E59D0249B74F@FreeBSD.org> <20120708213846.GF1437@garage.freebsd.pl>
next in thread | previous in thread | raw e-mail | index | archive | help
Wiadomo=B6=E6 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=B3a = wrote: >> Wiadomo=B6=E6 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: >>>=20 >>> = http://people.freebsd.org/~pjd/patches/geom_property_change.patch >>>=20 >>> 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. >>=20 >> I was not aware of that patch. [...] >=20 > I'm afraid that's a lie. =46rom IRC logs: >=20 > <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. >=20 > 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); >>>=20 >>> Is this your protection from a provider going away? >>=20 >> Can you suggest a way to do it in a safe way that doesn't involve >> rewriting most of GEOM? >=20 > 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. >>=20 >> 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. >=20 > 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.=20 Your patch didn't do that, and perhaps we could just get rid of it. I think the current approach is safer, though. --=20 If you cut off my head, what would I say? Me and my head, or me and my = body?
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?739062A0-4339-46A3-AF8E-193BC57758BA>