Re: [PATCH v3 1/1] vfio-ccw: Prevent quiesce function going into an infinite loop

[Farhan Ali <alifm@linux.ibm.com>]

On 04/23/2019 01:42 PM, Halil Pasic wrote:
> One thing I'm confused about is, that we don't seem to prevent
> new I/O being submitted. That is we could still loop indefinitely
> if we get new IO after the 'kill I/O on the subchannel' is done but
> before the msch() with the disable is issued.

So the quiesce function will be called in the remove, release functions 
and also in the mdev reset callback via an ioctl VFIO_DEVICE_RESET.

Now the release function is invoked in cases when we hot unplug the 
device or the guest is gone (or anything that will close the vfio mdev 
file descriptor, I believe). In such scenarios it's really the userspace 
which is asking to release the device. Similar for remove, where the 
user has to explicitly write to the remove file for the mdev to invoke 
it. Under normal conditions no sane userspace should be doing 
release/remove while there are still on going I/Os :)

Me and Conny had some discussion on this in v1 of this patch:
https://marc.info/?l=kvm&m=155437117823248&w=2

> 
> The 'flush all I/O' parts in the commit message and in the code make
> this even more confusing.

Maybe...if it's too confusing it could be fixed, but IMHO I don't think 
it's a dealbreaker. If anyone else thinks otherwise, I can go ahead and 
change it.

> 
> Another thing that I don't quite understand is injecting interrupts
> into QEMU for stuff that is actually not guest initiated.

As mentioned above under normal conditions we shouldn't be doing 
quiesce. But wouldn't those interrupts just be unsolicited interrupts 
for the guest?

> 
> Furthermore I find how cio_cancel_halt_clear() quite confusing. We
> usually return EBUSY to say that the function was suppressed because,
> well, the resource is busy. For cio_cancel_halt_clear() EBUSY means:
> * either a xsch() was effectively suppressed because status pending
> * or an hsch() or csch() was actually successfully executed, and the
> client code is supposed to wait for the assync clear/halt function
> to complete. This gets even more confusing when one reads:
> 
> /**
>   * cio_cancel_halt_clear - Cancel running I/O by performing cancel, halt
>   * and clear ordinally if subchannel is valid.
>   * @sch: subchannel on which to perform the cancel_halt_clear operation
>   * @iretry: the number of the times remained to retry the next operation
>   *
>   * This should be called repeatedly since halt/clear are asynchronous
> 
> This is simply not true. Because halt/clear is async, we may not know
> if we managed to accomplish canceling the running I/O. But the next
> call of this function won't detect and say 'yep it worked, we are good
> now' and return 0. This is the responsibility of the caller.
> 
> If it were like that, the current code would have been fine!
>             
>   * operations. We do one try with cio_cancel, three tries with cio_halt,
>   * 255 tries with cio_clear. The caller should initialize @iretry with
>   * the value 255 for its first call to this, and keep using the same
>   * @iretry in the subsequent calls until it gets a non -EBUSY return.
>   *
>   * Returns 0 if device now idle, -ENODEV for device not operational,
>   * -EBUSY if an interrupt is expected (either from halt/clear or from a
>   * status pending), and -EIO if out of retries.
>   */
> int cio_cancel_halt_clear(struct subchannel *sch, int *iretry
> 
> TL;DR:
> 
> I welcome  this batch (with an r-b) but I would like the commit message
> and the comment changed so that the misleading 'flush all I/O in the
> workqueue'.
> 
> I think 'vfio-ccw: fix cio_cancel_halt_clear() usage' would reflect the
> content of this patch better, because reasoning about the upper limit,
> and what happens if this upper limit is hit is not what this patch is
> about. It is about a client code bug that rendered iretry ineffective.
> 

I politely disagree with the change in subject line. I think the current 
subject line describe what we are trying to prevent with this patch. But 
again if anyone else feels otherwise, I will go ahead and change :)

Thanks
Farhan


> Regards,
> Halil
> 
>