Re: svn commit: r238213 - head/sys/geom
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? ___ svn-src-all@freebsd.org mailing list http://lists.freebsd.org/mailman/listinfo/svn-src-all To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
Re: svn commit: r238213 - head/sys/geom
On Mon, Jul 09, 2012 at 09:19:29AM +0200, Edward Tomasz Napierała wrote: 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. As you know I'm not going to be neither nice nor helpful to you. Just wanted to point out your changes are wrong. That's all I can do. This is my last e-mail on the subject. -- Pawel Jakub Dawidek http://www.wheelsystems.com FreeBSD committer http://www.FreeBSD.org Am I Evil? Yes, I Am! http://tupytaj.pl pgpjEHhwVTWS9.pgp Description: PGP signature
Re: svn commit: r238213 - head/sys/geom
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 pgplErKyGzSCX.pgp Description: PGP signature
svn commit: r238213 - head/sys/geom
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
Modified:
head/sys/geom/geom.h
head/sys/geom/geom_subr.c
Modified: head/sys/geom/geom.h
==
--- head/sys/geom/geom.hSat Jul 7 19:39:08 2012(r238212)
+++ head/sys/geom/geom.hSat Jul 7 20:13:40 2012(r238213)
@@ -79,6 +79,7 @@ typedef void g_attrchanged_t (struct g_c
typedef void g_provgone_t (struct g_provider *);
typedef void g_dumpconf_t (struct sbuf *, const char *indent, struct g_geom *,
struct g_consumer *, struct g_provider *);
+typedef void g_resize_t(struct g_consumer *cp);
/*
* The g_class structure describes a transformation class. In other words
@@ -108,7 +109,7 @@ struct g_class {
g_orphan_t *orphan;
g_ioctl_t *ioctl;
g_provgone_t*providergone;
- void*spare2;
+ g_resize_t *resize;
/*
* The remaining elements are private
*/
@@ -139,7 +140,7 @@ struct g_geom {
g_orphan_t *orphan;
g_ioctl_t *ioctl;
g_provgone_t*providergone;
- void*spare1;
+ g_resize_t *resize;
void*softc;
unsignedflags;
#defineG_GEOM_WITHER 1
@@ -265,6 +266,7 @@ int g_handleattr_str(struct bio *bp, con
struct g_consumer * g_new_consumer(struct g_geom *gp);
struct g_geom * g_new_geomf(struct g_class *mp, const char *fmt, ...);
struct g_provider * g_new_providerf(struct g_geom *gp, const char *fmt, ...);
+void g_resize_provider(struct g_provider *pp, off_t size);
int g_retaste(struct g_class *mp);
void g_spoil(struct g_provider *pp, struct g_consumer *cp);
int g_std_access(struct g_provider *pp, int dr, int dw, int de);
Modified: head/sys/geom/geom_subr.c
==
--- head/sys/geom/geom_subr.c Sat Jul 7 19:39:08 2012(r238212)
+++ head/sys/geom/geom_subr.c Sat Jul 7 20:13:40 2012(r238213)
@@ -68,9 +68,11 @@ static struct g_tailq_head geoms = TAILQ
char *g_wait_event, *g_wait_up, *g_wait_down, *g_wait_sim;
struct g_hh00 {
- struct g_class *mp;
- int error;
- int post;
+ struct g_class *mp;
+ struct g_provider *pp;
+ off_t size;
+ int error;
+ int post;
};
/*
@@ -356,6 +358,7 @@ g_new_geomf(struct g_class *mp, const ch
gp-access = mp-access;
gp-orphan = mp-orphan;
gp-ioctl = mp-ioctl;
+ gp-resize = mp-resize;
return (gp);
}
@@ -601,6 +604,76 @@ g_error_provider(struct g_provider *pp,
pp-error = error;
}
+static void
+g_resize_provider_event(void *arg, int flag)
+{
+ struct g_hh00 *hh;
+ struct g_class *mp;
+ struct g_geom *gp;
+ struct g_provider *pp;
+ struct g_consumer *cp, *cp2;
+ off_t size;
+
+ g_topology_assert();
+ if (flag == EV_CANCEL)
+ return;
+ if (g_shutdown)
+ return;
+
+ hh = arg;
+ pp = hh-pp;
+ size = hh-size;
+
+ G_VALID_PROVIDER(pp);
+ g_trace(G_T_TOPOLOGY, g_resize_provider_event(%p), pp);
+
+ LIST_FOREACH_SAFE(cp, pp-consumers, consumers, cp2) {
+ gp = cp-geom;
+ if (gp-resize == NULL size pp-mediasize)
+ cp-geom-orphan(cp);
+ }
+
+ pp-mediasize = size;
+
+ LIST_FOREACH_SAFE(cp, pp-consumers, consumers, cp2) {
+ gp = cp-geom;
+ if (gp-resize != NULL)
+ gp-resize(cp);
+ }
+
+ /*
+* After resizing, the previously invalid GEOM class metadata
+* might become valid. This means we should retaste.
+*/
+ LIST_FOREACH(mp, g_classes, class) {
+ if (mp-taste == NULL)
+ continue;
+ LIST_FOREACH(cp, pp-consumers, consumers)
+ if (cp-geom-class == mp)
+ break;
+ if (cp != NULL)
+ continue;
+ mp-taste(mp, pp, 0);
+ g_topology_assert();
+ }
+}
+
+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;
+
Re: svn commit: r238213 - head/sys/geom
On Sat, Jul 07, 2012 at 08:13:41PM +, 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.
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.
+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?
+ hh = arg;
+ pp = hh-pp;
+ size = hh-size;
Where do you free the memory allocated for 'hh'?
+ G_VALID_PROVIDER(pp);
Is this your protection from a provider going away?
+ 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.
+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()?
+ hh-size = size;
+ g_post_event(g_resize_provider_event, hh, M_WAITOK, NULL);
--
Pawel Jakub Dawidek http://www.wheelsystems.com
FreeBSD committer http://www.FreeBSD.org
Am I Evil? Yes, I Am! http://tupytaj.pl
pgp3T2FQNL3Os.pgp
Description: PGP signature
Re: svn commit: r238213 - head/sys/geom
Wiadomość napisana przez Pawel Jakub Dawidek w dniu 7 lip 2012, o godz. 23:54:
On Sat, Jul 07, 2012 at 08:13:41PM +, 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?
___
svn-src-all@freebsd.org mailing list
http://lists.freebsd.org/mailman/listinfo/svn-src-all
To unsubscribe, send any mail to svn-src-all-unsubscr...@freebsd.org
