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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





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






[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
[Index of Archives]     [Kernel Development]     [Kernel Newbies]     [IDE]     [Security]     [Git]     [Netfilter]     [Bugtraq]     [Yosemite Info]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux ATA RAID]     [Samba]     [Linux Media]     [Device Mapper]

  Powered by Linux