[RFC,v2,2/3] vfio-ccw: Prevent quiesce function going into an infinite loop
diff mbox series

Message ID 2c17cf29fbce8fc1cfbf60cfd04559d00c8eeac0.1554756534.git.alifm@linux.ibm.com
State New
Headers show
Series
  • fio-ccw fixes for kernel stacktraces
Related show

Commit Message

Farhan Ali April 8, 2019, 9:05 p.m. UTC
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(-)

Comments

Cornelia Huck April 11, 2019, 4:24 p.m. UTC | #1
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?

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
* -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 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;
> +		flush_workqueue(vfio_ccw_work_q);
> +		spin_lock_irq(sch->lock);
>  		ret = cio_disable_subchannel(sch);
>  	} while (ret == -EBUSY);
>  out_unlock:
Farhan Ali April 11, 2019, 8:30 p.m. UTC | #2
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.


> * -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?

> 
> 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.

>> +		flush_workqueue(vfio_ccw_work_q);
>> +		spin_lock_irq(sch->lock);
>>   		ret = cio_disable_subchannel(sch);
>>   	} while (ret == -EBUSY);
>>   out_unlock:
> 
>
Cornelia Huck April 12, 2019, 8:10 a.m. UTC | #3
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?

> 
> >> +		flush_workqueue(vfio_ccw_work_q);
> >> +		spin_lock_irq(sch->lock);
> >>   		ret = cio_disable_subchannel(sch);
> >>   	} while (ret == -EBUSY);
> >>   out_unlock:  
> > 
> >   
>
Farhan Ali April 12, 2019, 2:38 p.m. UTC | #4
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:
>>>
>>>    
>>
> 
>
Cornelia Huck April 15, 2019, 8:13 a.m. UTC | #5
On Fri, 12 Apr 2019 10:38:50 -0400
Farhan Ali <alifm@linux.ibm.com> wrote:

> 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:

> >>> 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  

(...)

> 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?

Yes.

> 
> 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 trying to disable the subchannel does not really hurt, but I
fear it won't succeed in that case...

> 
> 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?

Logging an error may be useful (it's really fouled up at that time), but...

> 
> 
> 
> 
> >>  
> >>>> +		flush_workqueue(vfio_ccw_work_q);
> >>>> +		spin_lock_irq(sch->lock);
> >>>>    		ret = cio_disable_subchannel(sch);

...there's a good chance that we'd get -EBUSY here, which would keep us
in the loop. We probably need to break out after we got -EIO from
cancel_halt_clear, regardless of which return code we get from the
disable.

(It will be "interesting" to see what happens with such a stuck
subchannel in the calling code; but I don't really see many options.
Panic seems way too strong; maybe mark the subchannel as "broken; no
idea how to fix"? But that would be a follow-on patch; I think if we
avoid the endless loop here, this patch is a real improvement and
should just go in.)

> >>>>    	} while (ret == -EBUSY);
> >>>>    out_unlock:  
> >>>
> >>>      
> >>  
> > 
> >   
>
Farhan Ali April 15, 2019, 1:38 p.m. UTC | #6
On 04/15/2019 04:13 AM, Cornelia Huck wrote:
> On Fri, 12 Apr 2019 10:38:50 -0400
> Farhan Ali <alifm@linux.ibm.com> wrote:
> 
>> 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:
> 
>>>>> 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
> 
> (...)
> 
>> 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?
> 
> Yes.
> 
>>
>> 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 trying to disable the subchannel does not really hurt, but I
> fear it won't succeed in that case...
> 
>>
>> 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?
> 
> Logging an error may be useful (it's really fouled up at that time), but...
> 
>>
>>
>>
>>
>>>>   
>>>>>> +		flush_workqueue(vfio_ccw_work_q);
>>>>>> +		spin_lock_irq(sch->lock);
>>>>>>     		ret = cio_disable_subchannel(sch);
> 
> ...there's a good chance that we'd get -EBUSY here, which would keep us
> in the loop. We probably need to break out after we got -EIO from
> cancel_halt_clear, regardless of which return code we get from the
> disable.

Okay, for EIO we can log an error message and break out of the loop.

I will send a v3. Are you going to queue patch 1 or patch 3 soon? If you 
are then I will just send this patch separately.

Thanks
Farhan

> 
> (It will be "interesting" to see what happens with such a stuck
> subchannel in the calling code; but I don't really see many options.
> Panic seems way too strong; maybe mark the subchannel as "broken; no
> idea how to fix"? But that would be a follow-on patch; I think if we
> avoid the endless loop here, this patch is a real improvement and
> should just go in.)
> 
>>>>>>     	} while (ret == -EBUSY);
>>>>>>     out_unlock:
>>>>>
>>>>>       
>>>>   
>>>
>>>    
>>
> 
>
Cornelia Huck April 15, 2019, 2:18 p.m. UTC | #7
On Mon, 15 Apr 2019 09:38:37 -0400
Farhan Ali <alifm@linux.ibm.com> wrote:

> On 04/15/2019 04:13 AM, Cornelia Huck wrote:
> > On Fri, 12 Apr 2019 10:38:50 -0400
> > Farhan Ali <alifm@linux.ibm.com> wrote:
> >   
> >> 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:  
> >   
> >>>>> 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  
> > 
> > (...)
> >   
> >> 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?  
> > 
> > Yes.
> >   
> >>
> >> 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 trying to disable the subchannel does not really hurt, but I
> > fear it won't succeed in that case...
> >   
> >>
> >> 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?  
> > 
> > Logging an error may be useful (it's really fouled up at that time), but...
> >   
> >>
> >>
> >>
> >>  
> >>>>     
> >>>>>> +		flush_workqueue(vfio_ccw_work_q);
> >>>>>> +		spin_lock_irq(sch->lock);
> >>>>>>     		ret = cio_disable_subchannel(sch);  
> > 
> > ...there's a good chance that we'd get -EBUSY here, which would keep us
> > in the loop. We probably need to break out after we got -EIO from
> > cancel_halt_clear, regardless of which return code we get from the
> > disable.  
> 
> Okay, for EIO we can log an error message and break out of the loop.
> 
> I will send a v3. Are you going to queue patch 1 or patch 3 soon? If you 
> are then I will just send this patch separately.

Yes, please do send it separately. I'm currently testing patch 1 and 3
on top of my patchset, will queue either with or without the halt/clear
patches proper, depending on how soon I get acks/r-bs (hint, hint :)

> 
> Thanks
> Farhan
> 
> > 
> > (It will be "interesting" to see what happens with such a stuck
> > subchannel in the calling code; but I don't really see many options.
> > Panic seems way too strong; maybe mark the subchannel as "broken; no
> > idea how to fix"? But that would be a follow-on patch; I think if we
> > avoid the endless loop here, this patch is a real improvement and
> > should just go in.)
> >   
> >>>>>>     	} while (ret == -EBUSY);
> >>>>>>     out_unlock:  
> >>>>>
> >>>>>         
> >>>>     
> >>>
> >>>      
> >>  
> > 
> >   
>
Farhan Ali April 15, 2019, 2:24 p.m. UTC | #8
On 04/15/2019 10:18 AM, Cornelia Huck wrote:
> On Mon, 15 Apr 2019 09:38:37 -0400
> Farhan Ali <alifm@linux.ibm.com> wrote:
> 
>> On 04/15/2019 04:13 AM, Cornelia Huck wrote:
>>> On Fri, 12 Apr 2019 10:38:50 -0400
>>> Farhan Ali <alifm@linux.ibm.com> wrote:
>>>    
>>>> 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:
>>>    
>>>>>>> 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
>>>
>>> (...)
>>>    
>>>> 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?
>>>
>>> Yes.
>>>    
>>>>
>>>> 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 trying to disable the subchannel does not really hurt, but I
>>> fear it won't succeed in that case...
>>>    
>>>>
>>>> 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?
>>>
>>> Logging an error may be useful (it's really fouled up at that time), but...
>>>    
>>>>
>>>>
>>>>
>>>>   
>>>>>>      
>>>>>>>> +		flush_workqueue(vfio_ccw_work_q);
>>>>>>>> +		spin_lock_irq(sch->lock);
>>>>>>>>      		ret = cio_disable_subchannel(sch);
>>>
>>> ...there's a good chance that we'd get -EBUSY here, which would keep us
>>> in the loop. We probably need to break out after we got -EIO from
>>> cancel_halt_clear, regardless of which return code we get from the
>>> disable.
>>
>> Okay, for EIO we can log an error message and break out of the loop.
>>
>> I will send a v3. Are you going to queue patch 1 or patch 3 soon? If you
>> are then I will just send this patch separately.
> 
> Yes, please do send it separately. I'm currently testing patch 1 and 3
> on top of my patchset, will queue either with or without the halt/clear
> patches proper, depending on how soon I get acks/r-bs (hint, hint :)

I am going through your patches 4 and 6 and hopefully will get back to 
you by the end of the day :).

> 
>>
>> Thanks
>> Farhan
>>
>>>
>>> (It will be "interesting" to see what happens with such a stuck
>>> subchannel in the calling code; but I don't really see many options.
>>> Panic seems way too strong; maybe mark the subchannel as "broken; no
>>> idea how to fix"? But that would be a follow-on patch; I think if we
>>> avoid the endless loop here, this patch is a real improvement and
>>> should just go in.)
>>>    
>>>>>>>>      	} while (ret == -EBUSY);
>>>>>>>>      out_unlock:
>>>>>>>
>>>>>>>          
>>>>>>      
>>>>>
>>>>>       
>>>>   
>>>
>>>    
>>
> 
>
Cornelia Huck April 15, 2019, 2:44 p.m. UTC | #9
On Mon, 15 Apr 2019 10:24:53 -0400
Farhan Ali <alifm@linux.ibm.com> wrote:

> On 04/15/2019 10:18 AM, Cornelia Huck wrote:

> > Yes, please do send it separately. I'm currently testing patch 1 and 3
> > on top of my patchset, will queue either with or without the halt/clear
> > patches proper, depending on how soon I get acks/r-bs (hint, hint :)  
> 
> I am going through your patches 4 and 6 and hopefully will get back to 
> you by the end of the day :).

Awesome, thanks!

Patch
diff mbox series

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)
 			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;
+		flush_workqueue(vfio_ccw_work_q);
+		spin_lock_irq(sch->lock);
 		ret = cio_disable_subchannel(sch);
 	} while (ret == -EBUSY);
 out_unlock: