Hello
There are two mutex used in fss.c. First, fss_device_lock protects all
fss units. We hold it when walking the device list, for instnace.
Second, sc_slock is a softc member that protects the specific softc.
It is used when accessing softc members, but I have the feeling its
use is perfectible.
In multiple places, we lock sc_slock, perform some action the softc,
then unlock sc_slock to call a function that will lock it again.
That seems to leave the possibility of another thread acting in
the middle of the sequence, and I am not sure such a case would
be safe. Here is such a case:
if (sc->sc_state != FSS_IDLE &&
(sc->sc_uflags & FSS_UNCONFIG_ON_CLOSE) != 0) {
sc->sc_uflags &= ~FSS_UNCONFIG_ON_CLOSE;
mutex_exit(&sc->sc_slock);
error = fss_ioctl(dev, FSSIOCCLR, NULL, FWRITE, l);
goto restart;
}
Fortunately such a concurent scenario does not happen because
the sequence is done with fss_device_lock held. That leads to
the question: is the sc_slock mutex useful at all?
If the answer is yes, the roles of fss_device_lock and sc_slock
should be made more clear so that we can lock sc_slock without
locking fss_device_lock. I also think we should split many
functions into a locking wrapper and an inner function that
assumes a locked softc, so that we avoid the unlock/call/lock
dance as in the example above.
I also note there is a softc modification without holding
any lock in fss_create_files(). This works because the code is
within snapshot serialization based on fss_device_cv. Do we
really need snapshots to be serialized? Certainly for a given
filesystem, but I understand there is no easy way to do that
in the fss device code. Perhaps that should be enforced at
filesystem level?
--
Emmanuel Dreyfus
m...@netbsd.org
