g_slice_orphan is unsafe? orphanization in general?

[Andriy Gapon]

void
g_slice_orphan(struct g_consumer *cp)
{
        struct g_slicer *gsp;

        g_trace(G_T_TOPOLOGY, "g_slice_orphan(%p/%s)", cp, cp->provider->name);
        g_topology_assert();

        /* XXX: Not good enough we leak the softc and its suballocations */
        gsp = cp->geom->softc;
        cp->geom->softc = NULL;	<--- I think that doing this is unsafe
        g_slice_free(gsp);	<--- and this
        g_wither_geom(cp->geom, ENXIO);
}

I do not see how g_slice_orphan() - or, in fact, the whole orphanization process
- is synchronized with g_io_request() and g_io_schedule_down().  I think that it
is quite possible for a geom to become orphaned after g_io_check() succeeds and
before geom->start is executed (or while it's being executed).
I see that there is an attempt to prevent orphanization while a geom is in use.
The code in one_event() does not call geom->orphan until pp->nstart == pp->nend.
 But there is no common lock to actually synchronize the check and updates of
the variables.  Atomic operations are not used either.  So, in my opinion, the
check is not really safe.

It looks like r162200 attempted to fix the general problem but either it was not
a complete solution or the I/O code was later changed:
https://svnweb.freebsd.org/base?view=revision&revision=162200
https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=102766

Back to g_slice, I see that g_slice_start() makes use of the softc.  So, this is
why I believe that destroying it right in g_slice_orphan is not correct.

It seems that other GEOM classes go to greater lengths to ensure that there is
no race between orphanization and I/O processing.  Typically an internal lock
and a flag seems to be used.

I wonder if g_slice should follow that technique.
Or maybe it's better to use a lock or atomic operations at GEOM infrastructure
level to ensure that changes to pp->error, pp->flags, pp->nstart and pp->nend
are really seen by other threads and in the correct order.

-- 
Andriy Gapon