Locking in src/sys/dev/fss.c

[Emmanuel Dreyfus <manu%netbsd.org@localhost>]

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
manu%netbsd.org@localhost