On Thu, Mar 09, 2023 at 07:35:42AM -0500, Stefan Hajnoczi wrote:
> On Wed, Mar 08, 2023 at 12:21:46PM -0500, Michael S. Tsirkin wrote:
>
> 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?
>
> Yes, a new virtqueue-level reset feature would take care of this case.
>
> Stefan
Why would we need a new one? We already have VIRTIO_F_RING_RESET.
--
MST
