Date: Sun, 8 Jul 2012 00:53:37 +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: <280C8AEE-F7E8-4AAE-87BF-E59D0249B74F@FreeBSD.org> In-Reply-To: <20120707215424.GE1437@garage.freebsd.pl> References: <201207072013.q67KDfHN082943@svn.freebsd.org> <20120707215424.GE1437@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 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 >>=20 >> 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. >>=20 >> Reviewed by: mav >> Sponsored by: FreeBSD Foundation > [...] >> - void *spare2; >> + g_resize_t *resize; > [...] >> - void *spare1; >> + g_resize_t *resize; > [...] >=20 > 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: >=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. 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 =3D=3D EV_CANCEL) >> + return; >=20 > 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 =3D arg; >> + pp =3D hh->pp; >> + size =3D hh->size; >=20 > Where do you free the memory allocated for 'hh'? It should be here, and it will be added soon. >> + G_VALID_PROVIDER(pp); >=20 > 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 =3D cp->geom; >> + if (gp->resize =3D=3D NULL && size < pp->mediasize) >> + cp->geom->orphan(cp); >> + } >=20 > 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 =3D=3D pp->mediasize) >> + return; >> + >> + hh =3D g_malloc(sizeof *hh, M_WAITOK | M_ZERO); >> + hh->pp =3D pp; >=20 > Care to explain why the provider can't disappear between now and the > event thread calling g_resize_provider_event()? See above. --=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?280C8AEE-F7E8-4AAE-87BF-E59D0249B74F>