[PATCH] Avoid reset work on watchdog timer function during error recovery

[PATCH] Avoid reset work on watchdog timer function during error recovery

[Guilherme G. Piccoli]

This patch adds a check on nvme_watchdog_timer() function to avoid the
call to reset_work() when an error recovery process is ongoing on
controller. The check is made by looking at pci_channel_offline()
result.

If we don't check for this on nvme_watchdog_timer(), error recovery
mechanism can't recover well, because reset_work() won't be able to
do its job (since we're in the middle of an error) and so the
controller is removed from the system before error recovery mechanism
can perform slot reset (which would allow the adapter to recover).

Signed-off-by: Guilherme G. Piccoli <gpiccoli at linux.vnet.ibm.com>
---
 drivers/nvme/host/pci.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 24ccda3..3cc146d 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -1357,10 +1357,12 @@ static void nvme_watchdog_timer(unsigned long data)
 
 	/*
 	 * Skip controllers currently under reset.
+	 * Also, skip controllers going through PCI error recovery.
 	 */
 	if (!work_pending(&dev->reset_work) && !work_busy(&dev->reset_work) &&
 	    ((csts & NVME_CSTS_CFS) ||
-	     (dev->subsystem && (csts & NVME_CSTS_NSSRO)))) {
+	    (dev->subsystem && (csts & NVME_CSTS_NSSRO))) &&
+	    !pci_channel_offline(to_pci_dev(dev->dev))) {
 		if (queue_work(nvme_workq, &dev->reset_work)) {
 			dev_warn(dev->dev,
 				"Failed status: 0x%x, reset controller.\n",
-- 
2.1.0

[PATCH] Avoid reset work on watchdog timer function during error recovery

[Keith Busch]

On Wed, Apr 06, 2016@05:30:35PM -0300, Guilherme G. Piccoli wrote:
> This patch adds a check on nvme_watchdog_timer() function to avoid the
> call to reset_work() when an error recovery process is ongoing on
> controller. The check is made by looking at pci_channel_offline()
> result.
> 
> If we don't check for this on nvme_watchdog_timer(), error recovery
> mechanism can't recover well, because reset_work() won't be able to
> do its job (since we're in the middle of an error) and so the
> controller is removed from the system before error recovery mechanism
> can perform slot reset (which would allow the adapter to recover).
> 
> Signed-off-by: Guilherme G. Piccoli <gpiccoli at linux.vnet.ibm.com>

Reviewed-by: Keith Busch <keith.busch at intel.com>

[PATCH] Avoid reset work on watchdog timer function during error recovery

[Johannes Thumshirn]

On Mittwoch, 6. April 2016 17:30:35 CEST Guilherme G. Piccoli wrote:
> This patch adds a check on nvme_watchdog_timer() function to avoid the
> call to reset_work() when an error recovery process is ongoing on
> controller. The check is made by looking at pci_channel_offline()
> result.
> 
> If we don't check for this on nvme_watchdog_timer(), error recovery
> mechanism can't recover well, because reset_work() won't be able to
> do its job (since we're in the middle of an error) and so the
> controller is removed from the system before error recovery mechanism
> can perform slot reset (which would allow the adapter to recover).
> 
> Signed-off-by: Guilherme G. Piccoli <gpiccoli at linux.vnet.ibm.com>

Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>

-- 
Johannes Thumshirn                                          Storage
jthumshirn at suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: Felix Imend?rffer, Jane Smithard, Graham Norton
HRB 21284 (AG N?rnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

[PATCH] Avoid reset work on watchdog timer function during error recovery

[Christoph Hellwig]

On Wed, Apr 06, 2016@05:30:35PM -0300, Guilherme G. Piccoli wrote:
>  	/*
>  	 * Skip controllers currently under reset.
> +	 * Also, skip controllers going through PCI error recovery.
>  	 */
>  	if (!work_pending(&dev->reset_work) && !work_busy(&dev->reset_work) &&
>  	    ((csts & NVME_CSTS_CFS) ||
> -	     (dev->subsystem && (csts & NVME_CSTS_NSSRO)))) {
> +	    (dev->subsystem && (csts & NVME_CSTS_NSSRO))) &&
> +	    !pci_channel_offline(to_pci_dev(dev->dev))) {
>  		if (queue_work(nvme_workq, &dev->reset_work)) {
>  			dev_warn(dev->dev,
>  				"Failed status: 0x%x, reset controller.\n",

This looks correct to me, but the condition is getting basically
unreadable.  Can you factor it out into a little helper returns a
boolean value if we should reset or not, and document each condition,
e.g. something like the function below, just with proper comments:

static bool nvme_should_reset(struct nvme_dev *dev)
{
	u32 csts = readl(dev->bar + NVME_REG_CSTS);

	if (!(csts & NVME_CSTS_CFS) &&
	    !((dev->subsystem && (csts & NVME_CSTS_NSSRO))))
		return false;

	if (work_pending(&dev->reset_work))
		return false;
	if (work_busy(&dev->reset_work))
		return false;
	if (pci_channel_offline(to_pci_dev(dev->dev))
		return false;

	return true;
}

[PATCH] Avoid reset work on watchdog timer function during error recovery

[Guilherme G. Piccoli]

On 04/07/2016 10:11 AM, Christoph Hellwig wrote:
> On Wed, Apr 06, 2016@05:30:35PM -0300, Guilherme G. Piccoli wrote:
>>   	/*
>>   	 * Skip controllers currently under reset.
>> +	 * Also, skip controllers going through PCI error recovery.
>>   	 */
>>   	if (!work_pending(&dev->reset_work) && !work_busy(&dev->reset_work) &&
>>   	    ((csts & NVME_CSTS_CFS) ||
>> -	     (dev->subsystem && (csts & NVME_CSTS_NSSRO)))) {
>> +	    (dev->subsystem && (csts & NVME_CSTS_NSSRO))) &&
>> +	    !pci_channel_offline(to_pci_dev(dev->dev))) {
>>   		if (queue_work(nvme_workq, &dev->reset_work)) {
>>   			dev_warn(dev->dev,
>>   				"Failed status: 0x%x, reset controller.\n",
>
> This looks correct to me, but the condition is getting basically
> unreadable.  Can you factor it out into a little helper returns a
> boolean value if we should reset or not, and document each condition,
> e.g. something like the function below, just with proper comments:
>
> static bool nvme_should_reset(struct nvme_dev *dev)
> {
> 	u32 csts = readl(dev->bar + NVME_REG_CSTS);
>
> 	if (!(csts & NVME_CSTS_CFS) &&
> 	    !((dev->subsystem && (csts & NVME_CSTS_NSSRO))))
> 		return false;
>
> 	if (work_pending(&dev->reset_work))
> 		return false;
> 	if (work_busy(&dev->reset_work))
> 		return false;
> 	if (pci_channel_offline(to_pci_dev(dev->dev))
> 		return false;
>
> 	return true;
> }
>

Heheh agreed, very good suggestion indeed. I found myself struggling to 
understand this huge condition line. I'll improve it following your 
suggestion, thanks.

Cheers,


Guilherme

[PATCH] Avoid reset work on watchdog timer function during error recovery

[Sagi Grimberg]

On 07/04/16 16:11, Christoph Hellwig wrote:
> On Wed, Apr 06, 2016@05:30:35PM -0300, Guilherme G. Piccoli wrote:
>>   	/*
>>   	 * Skip controllers currently under reset.
>> +	 * Also, skip controllers going through PCI error recovery.
>>   	 */
>>   	if (!work_pending(&dev->reset_work) && !work_busy(&dev->reset_work) &&
>>   	    ((csts & NVME_CSTS_CFS) ||
>> -	     (dev->subsystem && (csts & NVME_CSTS_NSSRO)))) {
>> +	    (dev->subsystem && (csts & NVME_CSTS_NSSRO))) &&
>> +	    !pci_channel_offline(to_pci_dev(dev->dev))) {
>>   		if (queue_work(nvme_workq, &dev->reset_work)) {
>>   			dev_warn(dev->dev,
>>   				"Failed status: 0x%x, reset controller.\n",
> This looks correct to me, but the condition is getting basically
> unreadable.  Can you factor it out into a little helper returns a
> boolean value if we should reset or not, and document each condition,
> e.g. something like the function below, just with proper comments:

Agreed, but nvme_should_reset() sounds like you are checking if a reset
is needed, maybe call it nvme_can_reset()?

>
> static bool nvme_should_reset(struct nvme_dev *dev)
> {
> 	u32 csts = readl(dev->bar + NVME_REG_CSTS);
>
> 	if (!(csts & NVME_CSTS_CFS) &&
> 	    !((dev->subsystem && (csts & NVME_CSTS_NSSRO))))
> 		return false;
>
> 	if (work_pending(&dev->reset_work))
> 		return false;
> 	if (work_busy(&dev->reset_work))
> 		return false;
> 	if (pci_channel_offline(to_pci_dev(dev->dev))
> 		return false;
>
> 	return true;
> }
>

[PATCH] Avoid reset work on watchdog timer function during error recovery

[Christoph Hellwig]

On Thu, Apr 07, 2016@04:18:51PM +0300, Sagi Grimberg wrote:
> Agreed, but nvme_should_reset() sounds like you are checking if a reset
> is needed,

But that's exaxtly what it does!  It reads CSTS to check for one of the
two bits in there indicating that we should initiate a reset, except for
various conditions why we think we better shouldn't reset at the
moment..

[PATCH] Avoid reset work on watchdog timer function during error recovery

[Keith Busch]

On Thu, Apr 07, 2016@06:11:31AM -0700, Christoph Hellwig wrote:
> static bool nvme_should_reset(struct nvme_dev *dev)
> {
> 	u32 csts = readl(dev->bar + NVME_REG_CSTS);
> 
> 	if (!(csts & NVME_CSTS_CFS) &&
> 	    !((dev->subsystem && (csts & NVME_CSTS_NSSRO))))
> 		return false;
> 
> 	if (work_pending(&dev->reset_work))
> 		return false;
> 	if (work_busy(&dev->reset_work))
> 		return false;

We could also skip checking work_pending since work_busy does that for us.

[PATCH] Avoid reset work on watchdog timer function during error recovery

[Guilherme G. Piccoli]

On 04/07/2016 11:24 AM, Keith Busch wrote:
> On Thu, Apr 07, 2016@06:11:31AM -0700, Christoph Hellwig wrote:
>> static bool nvme_should_reset(struct nvme_dev *dev)
>> {
>> 	u32 csts = readl(dev->bar + NVME_REG_CSTS);
>>
>> 	if (!(csts & NVME_CSTS_CFS) &&
>> 	    !((dev->subsystem && (csts & NVME_CSTS_NSSRO))))
>> 		return false;
>>
>> 	if (work_pending(&dev->reset_work))
>> 		return false;
>> 	if (work_busy(&dev->reset_work))
>> 		return false;
>
> We could also skip checking work_pending since work_busy does that for us.
>

Great! What name should/can I choose for the function?

Thanks,


Guilherme

[PATCH] Avoid reset work on watchdog timer function during error recovery

[Keith Busch]

On Thu, Apr 07, 2016@12:42:29PM -0300, Guilherme G. Piccoli wrote:
> Great! What name should/can I choose for the function?

I liked Christoph's 'nvme_should_reset' suggestion.

[PATCH] Avoid reset work on watchdog timer function during error recovery

[Guilherme G. Piccoli]

On 04/07/2016 12:46 PM, Keith Busch wrote:
> On Thu, Apr 07, 2016@12:42:29PM -0300, Guilherme G. Piccoli wrote:
>> Great! What name should/can I choose for the function?
>
> I liked Christoph's 'nvme_should_reset' suggestion.
>

Ok, cool. I'll use this name.

Thanks,


Guilherme

[PATCH] Avoid reset work on watchdog timer function during error recovery

[Christoph Hellwig]

On Thu, Apr 07, 2016@01:03:02PM -0300, Guilherme G. Piccoli wrote:
> On 04/07/2016 12:46 PM, Keith Busch wrote:
> >On Thu, Apr 07, 2016@12:42:29PM -0300, Guilherme G. Piccoli wrote:
> >>Great! What name should/can I choose for the function?
> >
> >I liked Christoph's 'nvme_should_reset' suggestion.
> >
> 
> Ok, cool. I'll use this name.

Did you manage to get around resending it?

[PATCH] Avoid reset work on watchdog timer function during error recovery

[Guilherme G. Piccoli]

On 04/12/2016 04:01 PM, Christoph Hellwig wrote:
> Did you manage to get around resending it?
>

Just sent the v2. Sorry for my delay.

Thanks,


Guilherme