geom_disk: use a preallocated geom_event for disk destruction.

[imp]

Summary

Preallocate a geom_event (using the new geom_alloc_event) when we create
a disk. When we create the disk, we're going to be in a sleepable
context, so we can always allocate this extra bit of memory. Then use
this preallocated memory to free the disk. CAM can try to free the disk
from an unsleepable context if there was I/O outstanding when the disk
was destroyted (say because the SIM said it had gone away). The I/O
context isn't sleepable. Rather than trying to invent a retry mechanism
and making sure all the other geom_disk consumers did it properly,
preallocating the event ensure that the geom_disk will be properly torn
down, even when there's memory pressure when the disk departs.

Sponsored by: Netflix

Test Plan

I've only lightly tested this, but what I really need feedback on is the name for the send event function that takes pre-allocated storage.

[imp]

Also looking for feedback as to one commit or two. The new API seems like it should be a separate commit maybe, but it's on the edge of what we've done that for in the past.

[jhb]

I would definitely do two commits: one for the API and one for just g_disk. I'm not keen on the "_storage" suffix, but I don't really have better suggestions. 'g_post_static_event' implies it's a global which isn't true, and 'g_post_allocated_event' is a mouthful. 'g_post_event2' would not be great either. :-P

sys/geom/geom_event.c

345

Do we need a g_free_event as well? In your case in geom_disk you are going to consume it always, but my other case in g_vfs_done() is optional and I will need to be able to free it in g_vfs_close() when a g_vfs object is freed without having its underlying storage destroyed.

You could also perhaps forward declare 'struct g_event' in geom.h while leaving it opaque so that you could have static type-checking for the new functions (g_alloc_event, g_free_event, g_post_event_storage).

383

Why did you expose wuflag in this API? I would perhaps instead just have a new g_waitfor_event_storage if someone needs the EV_WAKEUP variant so that it mirrors the g_post_event/g_waitfor_event API. The 'waitfor' variant could be added in the future when it is needed.

Actually, we probably won't ever need the 'waitfor' variant as if you can sleep, you can likely likely use M_WAITOK to alloc the event in the first place. I'm somewhat surprised 'waitfor' even takes a malloc flag. Indeed, every caller uses M_WAITOK.

[imp]

In D30544#688278, @jhb wrote:

I would definitely do two commits: one for the API and one for just g_disk. I'm not keen on the "_storage" suffix, but I don't really have better suggestions. 'g_post_static_event' implies it's a global which isn't true, and 'g_post_allocated_event' is a mouthful. 'g_post_event2' would not be great either. :-P

g_post_event_ep has been the least sucky I've come up with :(

sys/geom/geom_event.c

345

Do we need a g_free_event as well? In your case in geom_disk you are going to consume it always, but my other case in g_vfs_done() is optional and I will need to be able to free it in g_vfs_close() when a g_vfs object is freed without having its underlying storage destroyed.

geom_disk just posts the event, it doesn't wait for it. As such, it doesn't need to free it. In other waiters, we already expect them to g_free() the memory. This is no different than that.

I'm unfamiliar with your needs here, though... Can you explain it more, since I can't puzzle out why you'd need it there...

You could also perhaps forward declare 'struct g_event' in geom.h while leaving it opaque so that you could have static type-checking for the new functions (g_alloc_event, g_free_event, g_post_event_storage).

That's a good idea.

383

I'll just remove the wuflag. It was mostly because g_post_event_x took it. But your logic makes sense.

[jhb]

sys/geom/geom_event.c

345

Do we need a g_free_event as well? In your case in geom_disk you are going to consume it always, but my other case in g_vfs_done() is optional and I will need to be able to free it in g_vfs_close() when a g_vfs object is freed without having its underlying storage destroyed.

geom_disk just posts the event, it doesn't wait for it. As such, it doesn't need to free it. In other waiters, we already expect them to g_free() the memory. This is no different than that.

I'm unfamiliar with your needs here, though... Can you explain it more, since I can't puzzle out why you'd need it there...

g_vfs won't always post the event is the problem. The event I'm preallocating is for this code at the end of g_vfs_done:

	if (destroy)
		g_post_event(g_vfs_destroy, cp, M_WAITOK, NULL);

That is conditional as it only kicks in if a disk is destroyed out from under a filesystem. If you just unmount a filesystem while the disk is still present, the event won't be used. For that latter case where the event isn't used, I need a way to free the event in g_vfs_close(). Now, perhaps what I can do instead is allocate the event in g_vfs_orphan(), I'm just not sure if it's save to do M_WAITOK in g_vfs_orphan vs pre-allocating in g_vfs_open() where M_WAITOK is definitely safe. (Though it looks like it might be the case that I can sleep in g_vfs_orphan since it is always called with the topology lock held which I think is an sx lock?)

Oh, I think due to the opaque use of void * I was worried you couldn't just use g_free() directly, but I guess it would be ok to use g_free() directly for an event allocated by g_alloc_event(). If you document this in a manpage maybe note that an event allocated by g_alloc_event() can be freed via g_free() if it isn't posted.

[imp]

update, per jhb. hopefully this catches everything...
also a subtle bug nobody noticed (of passing a va into ...)

[jhb]

Please do the geom_disk bits as a second commit if you can. Thanks!

[imp]

I'll do this in 2 commits, but I've found a couple of bugs in testing I need to fix first.