Re: [RFC v2 2/3] vfio-ccw: Prevent quiesce function going into an infinite loop

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

On 04/12/2019 04:10 AM, Cornelia Huck wrote:
> On Thu, 11 Apr 2019 16:30:44 -0400
> Farhan Ali <alifm@linux.ibm.com> wrote:
> 
>> On 04/11/2019 12:24 PM, Cornelia Huck wrote:
>>> On Mon,  8 Apr 2019 17:05:32 -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>
>>>> ---
>>>>    drivers/s390/cio/vfio_ccw_drv.c | 27 ++++++++++++---------------
>>>>    1 file changed, 12 insertions(+), 15 deletions(-)
>>>>
>>>> diff --git a/drivers/s390/cio/vfio_ccw_drv.c b/drivers/s390/cio/vfio_ccw_drv.c
>>>> index 5aca475..4405f2a 100644
>>>> --- a/drivers/s390/cio/vfio_ccw_drv.c
>>>> +++ b/drivers/s390/cio/vfio_ccw_drv.c
>>>> @@ -43,26 +43,23 @@ 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);
>>>> -
>>>> +		/*
>>>> +		 * Flush all I/O and wait for
>>>> +		 * cancel/halt/clear completion.
>>>> +		 */
>>>> +		private->completion = &completion;
>>>> +		spin_unlock_irq(sch->lock);
>>>> +
>>>> +		if (ret == -EBUSY)
>>>
>>> I don't think you need to do the unlock/lock and change
>>> private->completion if you don't actually wait, no?
>>
>> If we don't end up waiting, then changing private->completion would not
>> be needed. But we would still need to release the spinlock due to [1].
>>
>>>
>>> Looking at the possible return codes:
>>> * -ENODEV -> device is not operational anyway, in theory you should even
>>>      not need to bother with disabling the subchannel
>>> * -EIO -> we've run out of retries, and the subchannel still is not
>>>     idle; I'm not sure if we could do anything here, as disable is
>>>     unlikely to work, either
>>
>> We could break out of the loop early for these cases. My thinking was I
>> wanted to depend on the result of trying to disable, because ultimately
>> that's what we want.
>>
>> I can add the cases to break out of the loop early.
> 
> The -ENODEV case does not really hurt, as it will get us out of the
> loop anyway. But for the -EIO case, I think we'll get -EBUSY from the
> disable and stay within the loop endlessly?
> 
>>
>>
>>> * -EBUSY -> we expect an interrupt (or a timeout), the loop looks fine
>>>     for that
>>> * 0 -> the one thing that might happen is that we get an unsolicited
>>>     interrupt between the successful cancel_halt_clear and the disable;
>>>     not even giving up the lock here might even be better here?
>>
>> I didn't think of this case, but if cancel_halt_clear succeeds with 0
>> then we should wait, no?
> 
> For 0 I don't expect a solicited interrupt (documentation for the
> functions says that the subchannel is idle in that case); it's just the
> unsolicited interrupt that might get into the way.
> 
>>
>>>
>>> I think this loop will probably work as it is after this patch, but
>>> giving up the lock when not really needed makes me a bit queasy... what
>>> do others think?
>>>    
>>>>    			wait_for_completion_timeout(&completion, 3*HZ);
>>>>    
>>>> -			private->completion = NULL;
>>>> -			flush_workqueue(vfio_ccw_work_q);
>>>> -			spin_lock_irq(sch->lock);
>>>> -			ret = cio_cancel_halt_clear(sch, &iretry);
>>>> -		};
>>>> -
>>>> +		private->completion = NULL;
>>
>> [1]  flush_workqueue can go to sleep so we would still need to release
>> spinlock and reacquire it again to try disabling the subchannel.
> 
> Grr, I thought we could skip the flush in the !-EBUSY case, but I think
> we can't due to the possibility of an unsolicited interrupt... what
> simply adding handling for -EIO (although I'm not sure what we can
> sensibly do in that case) and leave the other cases as they are now?
> 

Thinking a little bit more about EIO, if the return code is EIO then it 
means we have exhausted all our options with cancel_halt_clear and the 
subchannel/device is still status pending, right?

I think we should still continue to try and disable the subchannel, 
because if not then the subchannel/device could in some point of time 
come back and bite us. So we really should protect the system from this 
behavior.

I think for EIO we should log an error message, but still try to 
continue with disabling the subchannel. What do you or others think?




>>
>>>> +		flush_workqueue(vfio_ccw_work_q);
>>>> +		spin_lock_irq(sch->lock);
>>>>    		ret = cio_disable_subchannel(sch);
>>>>    	} while (ret == -EBUSY);
>>>>    out_unlock:
>>>
>>>    
>>
> 
>