geom_slice: do not destroy softc until providers are gone

[avg]

Summary

At present, g_slice_orphan and g_slice_spoiled destroy the softc
(struct g_slicer) even before calling g_wither_geom, so there can
be active and incoming io requests at that time and g_slice_start
can access the softc.

There are several ways to fix this problem.
I decided to use providergone callback to destroy the softc only after
all providers have detached. It would have been sufficient to wait
until the providers leave the doorstep (upon nend reaching nstart),
but there is no notification for that.

Test Plan

This change has been stress-tested by Ben RUBSON <[e-mail]>.
That was done by destroying iSCSI disks with GEOM labels on top of them
and intensive I/O through the label providers (using ZFS).

[smh]

LGTM

[imp]

Unsure what makes this protocol safe... Some better comments might help, or else there seems to be an issue with the code.

sys/geom/geom_slice.c

501–505

Doesn't this SET WITHER?

504

and then you free here.

538

why do you need to check wither here?
How do you keep a double free from happening here, since the above path sets wither too...

[avg]

sys/geom/geom_slice.c

501–505

It does, of course.

504

But only if nprovider is zero. This is important.

538

1. I do not want to destroy softc only because all providers got unconfigured. For example, if all partitions are deleted. I want to destroy softc only when the geom is really being removed.

2. The above path frees softc only if there are no providers. In that case this callback is never called.

[mav]

It would have been sufficient to wait until the providers leave the doorstep (upon nend reaching nstart), but there is no notification for that.

Because that is not a normal operation logic, but a dirty hacky workaround for broken classes, that do not wait for I/O completion before closing provider (there were some, most IIRC are fixed now). This code just should not be there, and that is why it has no notification.

I have no specific objections about this patch, but I am not sure it really needs to use providergone method, that was not in original GEOM design and was added as workaround. softc could be freed on the last close. In case of gmirror, for example, providergone has to be used, since it uses custom locks and require topology lock to be dropped during g_mirror_access() call, that created a race window. This class seems less complicated, there is no any lock magic, so it could work in original GEOM logic, tracking opens instead.

[avg]

@mav thank you for the suggestion! Just to clarify, do you mean adding tracking of the open provider count into g_slice_access ?

[mav]

In D12809#266148, @avg wrote:

Just to clarify, do you mean adding tracking of the open provider count into g_slice_access ?

Yes. You may see, for example, how gmultipath uses sc->sc_opened.

[avg]

Track opens instead of depending on providergone, per mav's suggestion.

This is more lightweight and it should be just as good.

[avg]

In D12809#266149, @mav wrote:

In D12809#266148, @avg wrote:

Just to clarify, do you mean adding tracking of the open provider count into g_slice_access ?

Yes. You may see, for example, how gmultipath uses sc->sc_opened.

Thank you! How does the new version look?

[mav]

The idea looks fine, but logic of gsp->nopen and d_nopen in g_slice_access() looks over-engineered. I suppose gsp->nopen will never be above 1 here. I think you could even use just (cp->acr == 0 && cp->acw == 0 && cp->ace == 0) expression instead of additional open counter.

[avg]

In D12809#266162, @mav wrote:

The idea looks fine, but logic of gsp->nopen and d_nopen in g_slice_access() looks over-engineered. I suppose gsp->nopen will never be above 1 here.

I think that gsp->nopen can be as high as slices parameter passed to g_slice_new that is greater than one for several geom classes.
There is some confusing naming, but gsp does not correspond to a single slice, it represents a "slicer" with potentially many slices.

I think you could even use just (cp->acr == 0 && cp->acw == 0 && cp->ace == 0) expression instead of additional open counter.

Do you mean cp as the consumer used by g_slice to access the underlying provider?
Looks like that can work, let me think about it.

Thanks!

[mav]

In D12809#266212, @avg wrote:

In D12809#266162, @mav wrote:

The idea looks fine, but logic of gsp->nopen and d_nopen in g_slice_access() looks over-engineered. I suppose gsp->nopen will never be above 1 here.

I think that gsp->nopen can be as high as slices parameter passed to g_slice_new that is greater than one for several geom classes.
There is some confusing naming, but gsp does not correspond to a single slice, it represents a "slicer" with potentially many slices.

But (cp->acr == 0 && cp->acw == 0 && cp->ace == 0) in g_slice_access() will be true only once until all slices will be closed, since cp here is a lower level consumer and it is opened by every slice.

I think you could even use just (cp->acr == 0 && cp->acw == 0 && cp->ace == 0) expression instead of additional open counter.

Do you mean cp as the consumer used by g_slice to access the underlying provider?

Yes, I am talking about line 127 as an example.

[avg]

Further simplify based on suggestions from mav; no need for nopen.

[mav]

Looks good to me. Thank you.

[smh]

Nice improvements, I learned some interesting details while following these improvements :-)