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

[Halil Pasic <pasic@linux.ibm.com>]

On Wed, 17 Apr 2019 09:58:24 -0400
Eric Farman <farman@linux.ibm.com> wrote:

> 
> 
> On 4/17/19 5:03 AM, Cornelia Huck wrote:
> > On Tue, 16 Apr 2019 17:23:14 -0400
> > Farhan Ali <alifm@linux.ibm.com> wrote:
> > 
> >> The quiesce function calls cio_cancel_halt_clear() and if we
> >> get an -EBUSY we go into a loop where we:
> >> 	- wait for any interrupts
> >> 	- flush all I/O in the workqueue
> >> 	- retry cio_cancel_halt_clear
> >>
> >> During the period where we are waiting for interrupts or
> >> flushing all I/O, the channel subsystem could have completed
> >> a halt/clear action and turned off the corresponding activity
> >> control bits in the subchannel status word. This means the next
> >> time we call cio_cancel_halt_clear(), we will again start by
> >> calling cancel subchannel and so we can be stuck between calling
> >> cancel and halt forever.
> >>
> >> Rather than calling cio_cancel_halt_clear() immediately after
> >> waiting, let's try to disable the subchannel. If we succeed in
> >> disabling the subchannel then we know nothing else can happen
> >> with the device.
> >>
> >> Suggested-by: Eric Farman <farman@linux.ibm.com>
> >> Signed-off-by: Farhan Ali <alifm@linux.ibm.com>
> >> ---
> >> ChangeLog:
> >> v2 -> v3
> >>     - Log an error message when cio_cancel_halt_clear
> >>       returns EIO and break out of the loop.
> >>     
> >>     - Did not include past change log as the other patches
> >>       of the original series have been queued by Conny.
> >>       Old series (v2) can be found here:
> >>       https://marc.info/?l=kvm&m=155475754101769&w=2
> >>
> >>   drivers/s390/cio/vfio_ccw_drv.c | 32 ++++++++++++++++++--------------
> >>   1 file changed, 18 insertions(+), 14 deletions(-)
> >>
> >> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
> >> index 78517aa..66a66ac 100644
> >> --- a/drivers/s390/cio/vfio_ccw_drv.c
> >> +++ b/drivers/s390/cio/vfio_ccw_drv.c
> >> @@ -43,26 +43,30 @@ int vfio_ccw_sch_quiesce(struct subchannel *sch)
> >>   	if (ret != -EBUSY)
> >>   		goto out_unlock;
> >>   
> >> +	iretry = 255;
> >>   	do {
> >> -		iretry = 255;
> >>   
> >>   		ret = cio_cancel_halt_clear(sch, &iretry);
> >> -		while (ret == -EBUSY) {
> >> -			/*
> >> -			 * Flush all I/O and wait for
> >> -			 * cancel/halt/clear completion.
> >> -			 */
> >> -			private->completion = &completion;
> >> -			spin_unlock_irq(sch->lock);
> >>   
> >> -			wait_for_completion_timeout(&completion, 3*HZ);
> >> +		if (ret == -EIO) {
> >> +			pr_err("vfio_ccw: could not quiesce subchannel 0.%x.%04x!\n",
> >> +			       sch->schid.ssid, sch->schid.sch_no);
> > 
> > What about using
> > 	dev_err(&sch->dev, "could not quiesce");
> > instead?
> 
> +1
> 
> > 
> > (Can make that change while applying, no need to resend for that.)
> > 
> >> +			break;
> >> +		}
> >> +
> >> +		/*
> >> +		 * Flush all I/O and wait for
> >> +		 * cancel/halt/clear completion.
> >> +		 */
> >> +		private->completion = &completion;
> >> +		spin_unlock_irq(sch->lock);
> >>   
> >> -			private->completion = NULL;
> >> -			flush_workqueue(vfio_ccw_work_q);
> >> -			spin_lock_irq(sch->lock);
> >> -			ret = cio_cancel_halt_clear(sch, &iretry);
> >> -		};
> >> +		if (ret == -EBUSY)
> >> +			wait_for_completion_timeout(&completion, 3*HZ);
> >>   
> >> +		private->completion = NULL;
> >> +		flush_workqueue(vfio_ccw_work_q);
> >> +		spin_lock_irq(sch->lock);
> >>   		ret = cio_disable_subchannel(sch);
> >>   	} while (ret == -EBUSY);
> >>   out_unlock:
> > 
> > Otherwise, looks good to me. Will queue when I get some ack/r-b.
> > 
> 
> I like it, but I feel weird giving an r-b to something I suggested:
> 
> Acked-by: Eric Farman <farman@linux.ibm.com>
> 

I think r-b is fine. You did verify both the design and the
implementation I guess. So I don't see why not.

How urgent is this. I could give this some love till the end of the
week. Should I @Connie,@Farhan?

I was mostly ignoring these patches so I can't capitalize on my
understanding from reviewing the previous versions and need some time to
say something about it.

Regards,
Halil