As I understand, the index field of g_consumer counts in-flight transactions. When index == 0 and private is not NULL, the consumer should be 'closed' (I am not sure that this is right term).

This is not done there for case when index reaches 0.

Also, wouldn't it me cleaner to initialize index with 1 and do not touch private at all, just changing the construct in swapgeom_orphan to cp->index--; destroy = cp->index == 0; ?

Note that swapgeom_strategy() uses sleepable bio allocation call g_alloc_bio() for read requests. I think this is incompatible with the direct dispatch, but I may be wrong.

You may be right about this. Let me study the code further to see if I can simplify it and have things still work.

Since this is the strategy routine, before we get into geom, I think it is OK. We aren't even dispatched yet and haven't sent the bio into geom yet.

This fragment still lacks handling of cp->index == 0.

Why does this segment need cp->index == 0 handling? It should never be zero here because we can't start new I/O while closing. Se set sp->sw_id to NULL in swapgeom_orphan when we drop the last reference. This means that as soon as we get the lock in swapgeom_strategy, cp->index can't be zero when we increment it because sp->sw_id is not NULL. I can add a kassert to this effect befpre the cp->index++. That is, unless I've misunderstood something and you see some path I'm missing where this wouldn't be true.

Because there could be in-flight bios while we are in _orphan() ? Otherwise, why would we need to handle the case of cp->index != 0 in orphan or cp->index == 0 in _done() ?

Sounds like it would be better to check for == 0 before we increment it then, since we're closing...

Please follow style, return (1);

return (0);

Why is it impossible for closing decrement to happen after the sw_dev_mtx is unlocked at line 2588 ? I.e., why not just put your _check_close() call there and be done ?

Heh, I hadn't realized my comments about side effects had made such a lasting impression. :)

In general I do prefer to avoid conditions on side effects. It is more obvious with long
expressions:

if ((error = call_some_thing(lots_of_arguments, and_other + expressions,
    even_more_args, this_gets_long)) != 0)

vs.

error = call_some_thing(....);
if (error != 0)

(Although assigning the result of getopt() in the while condition is one time I always break this.)

This one is probably fine, though I find that it requires the reader to exercise a few more brain cells (the -- is prefix, so does it happen before or after? oh, right before, so this will fire when going down to 0) whereas if you break it up you don't have to do that exercise in your head. It is plainly obvious what the order of the decrement is to the comparison:

cp->index--;
if (cp->index == 0)

G_CF_RECEIVE on a consumer controls direct dispatch on the completion path. It is safe to do that. So I think that bit is OK. Am I misunderstanding the code in geom_io.c?

I totally agree that a sleepable alloc may backfire. That's a problem in the current system. That's a weakness I hope to address, but not here.

I think having a pool of bios set aside for when we can't allocate out of the normal pool is a good idea. Maybe even one or two chains on that other work I've done would suffice (we can allocate them statically, and then on the completion path have a special case to return the chain to a good state, possibly for the next I/O). This would limit the I/O we could do to one read and one right (in my thinking), but that would guarantee progress when there's no memory for a BIO which would solve the current worst-case scenario where we need to do I/O to recover memory.

I'm not sure I understand the last bit, though. Why is better asynch queueing in GEOM required for us to safely do direct dispatch of down and up? I'm not sure I understand the point well enough since I'm having trouble drawing the connection. Can you clarify perhaps?

The swap geom strategy does not sleep on the write, i.e. when the swap is the producer of the free memory. When b_iocmd is BIO_WRITE, g_new_bio() is used, which uses M_NOWAIT.

When that change (to not sleep for bio allocation) in swap geom was introduced, M_NOWAIT combined the semantic of not sleepable allocation and hint to UMA that the request should drain the reservation of free pages deeper than other requests. This was later split into M_USE_RESERVE malloc flag, which is currently not used by g_new_bio().

I fully agree with the note that swap geom bio allocation should be allowed to drain reserve deeper, that the bio zone should have a reserve itself (but I do not see a good way to prevent the reserve drain from the non-priority bio allocations), and ultimately that the whole bio allocation on the write path is just architectural bug.