Re: [virtio-comment] Re: [virtio] Re: [PATCH v10 04/10] admin: introduce virtio admin virtqueues

["Michael S. Tsirkin" <mst@redhat.com>]

On Wed, Mar 08, 2023 at 12:15:23PM -0500, Stefan Hajnoczi wrote:
> > > > > Or we could say that admin commands must complete within bounded time,
> > > > > but I'm not sure that is implementable for some device types like
> > > > > virtio-blk, virtio-scsi, and virtiofs.
> > > > 
> > > > No we can't.
> > > > Some commands, for example FW upgrade can take 10 minutes and it's perfectly
> > > > fine. Other commands like setting feature bit will take 1 millisec.
> > > > Each device implements commands in a different internal logic so we can't
> > > > expect to complete after X time.
> > > 
> > > When I say bounded time, I mean that it finishes in a finite amount of
> > > time. I'm not saying there is a specific time X that all device
> > > implementations must satisfy. Unbounded means it might never finish.
> > 
> > There might be a chance that any command for any virtio device type will
> > never finish. Nothing new here in the adminq.
> > 
> > what one can do is to set a timeout for himself and if this timeout expire -
> > check the device status. If it needs_reset - do a reset. if status is ok,
> > then wait some more time.
> > After X retries, unmap buffers or reset the adminq.
> 
> Michael: What effect does resetting the group owner device have on group
> member devices?

virtio level reset? It's a good question. I'd expect them all to be
reset no?

> I'm concerned that this approach disrupts all group member devices. For
> example, you try to add a new device but the command hangs. In order to
> recover you now have to reset the group owner device and this breaks all
> the group member devices.


I agree. How about a VQ level reset though? Seems like exactly
what's needed here?


> > > 
> > > > Device can go to so FATAL state in case a command is stuck and causing
> > > > internal errors in it.
> > > > 
> > > > > 
> > > > > > For your example, stopping a member is possible even it there are some
> > > > > > errors in the network. You can for example destroy all the connections to
> > > > > > the remote target and complete all the BIOS with some error.
> > > > > 
> > > > > Forgetting about in-flight requests doesn't necessarily make them go
> > > > > away. It creates a race between forgotten requests and reconnection. In
> > > > > the worst case a forgotten write request takes effect after
> > > > > reconnection, causing data corruption.
> > > > 
> > > > For making it work without data corruption we need a cooperation of the
> > > > target side for sure. But this is fine since the target in that case is part
> > > > of the "virtio-blk backend".
> > > > One solution is that the target can decide it will flush all the requests to
> > > > the storage device before accepting new connections.
> > > 
> > > This solution shifts the unbounded time from disconnection to
> > > connection. The Group Member Delete command will complete quickly but a
> > > subsequent Group Member Create command for the same underlying storage
> > > device would need to wait until the requests are done.
> > > 
> > > Therefore I think the admin queue must be designed under the assumption
> > > that some commands take a very long time.
> > 
> > For sure an admin command may take long time. FW upgrade can take 10 minutes
> > for example.
> > But each device is free to implement internal logic as he choose.
> > 
> > Same for live migration, when we stop/quiesce a device we must make sure it
> > doesn't master any DMA operations. Thus, in some implementations we need to
> > wait for all inflights to end fast. In others, we can invalidate the access
> > to host/guest memory and wait for completions until the freeze state.
> > 
> > Bottom line, this is device implementation specific consideration.
> 
> What I'm asking is that the spec clarifies the command completion order
> semantics (in-order or out-of-order), whether there is a mechanism to
> abort commands, etc.
> 
> Device implementers can then take advantage of those aspects to
> implement devices that don't hang (e.g. health monitoring becomes
> unavailable when there is a long running command).
> 
> If the spec doesn't cover this, then device implementers will not be
> able to work around it when implementing standard commands like
> create/delete group member.
> 
> Does that make sense?
> 
> Stefan

It does, to me.