Re: kernel deadlock on fstchg with vnd

[Taylor R Campbell <campbell+netbsd-current-users%mumble.net@localhost>]

> Date: Mon, 30 May 2022 14:33:42 +0200
> From: "J. Hannken-Illjes" <hannken%mailbox.org@localhost>
> 
> >>  1767          /* Nuke the vnodes for any open instances */
> >>  1768          for (i = 0; i < MAXPARTITIONS; i++) {
> >>  1769                  mn = DISKMINOR(device_unit(vnd->sc_dev), i);
> >>  1770                  vdevgone(bmaj, mn, mn, VBLK);
> >>  1771                  if (mn != myminor) /* XXX avoid to kill own vnode */
> >>  1772                          vdevgone(cmaj, mn, mn, VCHR);
> >>  1773          }
> >> 
> >> The "skip myself" on lines 1771/1772 is responsible for this behaviour.
> > 
> > Yes and doing the same for block devices avoids the issue.
> > But Taylor is reluctant to commit this hack.
> 
> And he is right.  It smells fishy to detach a (pseudo) device from
> an open instance of itself, either with ioctl or close.
> 
> Why do we detach on last close -- isn't it sufficient to detach
> either explicit with drvctl(8) or on module unload?
> 
> The attached diff moves vdevgone() to vnd_detach() and no longer
> detaches on last close -- comments?

Sorry, I got detached from this thread and committed the workaround
already before I noticed your patch.

The idea of these devices -- vnd, as well as cgd and ccd, and possibly
others I'm not thinking of -- is that they are created on-demand, and
persist only as long as they are in use.

With your patch, there is no path in normal vnconfig usage that
detaches the device; operators would have to go out of their way to
use `drvctl -d' (and drvctl itself is racy with detach -- that also
needs to be fixed) to free up any resources used by no-longer-used
vnd(4) autoconf instances.

That's not to say the current state of affairs is good -- it's
obviously broken in some ways (either the revoke is unnecessary, or
(a) it's necessary, and (b) failing to revoke the chardev is
insufficient because the chardev can have more than one open), and
every driver handles the state differently, so the autoconf API we
have is not serving the needs of drivers to allocate minor numbers.

I think that it would probably be better if we had a matching pair of
operations, say spawn and unspawn, that can be used like so:

xyzopen(dev_t dev)
{
	device_t self;
	struct xyz_softc *sc;

	self = config_pseudo_spawn(xyzunit(dev));
	sc = device_private(self);
	...
}

xyzclose(dev_t dev)      // d_close callback, invoked on last close
{
	device_t self = device_lookup(xyzunit(dev));
	struct xyz_softc *sc = device_private(self);

	if (!sc->sc_configured)
		config_pseudo_unspawn(self);
}

This would need to be integrated into autoconf/specfs to avoid races
in config_pseudo_spawn, sc->sc_configured, and config_pseudo_unspawn,
and if there's any revoke involved, the unspawn might be scheduled
asynchronously to make it work.  Or, maybe spawn/unspawn should be
done in ioctl VNDIOCSET and VNDIOCCLR, and open/close can be empty
stubs.

But I haven't yet thought through the details of a good way to deal
with these pseudo-devices -- I put it off during the recent
open/detach rototill.