diff mbox series

[v3] qla2xxx: Return EBUSY on fcport deletion

Message ID 20201012173524.46544-1-dwagner@suse.de (mailing list archive)
State Superseded
Headers show
Series [v3] qla2xxx: Return EBUSY on fcport deletion | expand

Commit Message

Daniel Wagner Oct. 12, 2020, 5:35 p.m. UTC
When the fcport is about to be deleted we should return EBUSY instead
of ENODEV. Only for EBUSY the request will be requeued in a multipath
setup.

Also in case we have a valid qpair but the firmware has not yet
started return EBUSY to avoid dropping the request.

Signed-off-by: Daniel Wagner <dwagner@suse.de>
---

v3: simplify test logic as suggested by Arun.
v2: rebased on mkp/staging

 drivers/scsi/qla2xxx/qla_nvme.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Arun Easi Oct. 12, 2020, 6:14 p.m. UTC | #1
On Mon, 12 Oct 2020, 10:35am, Daniel Wagner wrote:

> ----------------------------------------------------------------------
> When the fcport is about to be deleted we should return EBUSY instead
> of ENODEV. Only for EBUSY the request will be requeued in a multipath
> setup.
> 
> Also in case we have a valid qpair but the firmware has not yet
> started return EBUSY to avoid dropping the request.
> 
> Signed-off-by: Daniel Wagner <dwagner@suse.de>
> ---
> 
> v3: simplify test logic as suggested by Arun.
> v2: rebased on mkp/staging
> 
>  drivers/scsi/qla2xxx/qla_nvme.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_nvme.c b/drivers/scsi/qla2xxx/qla_nvme.c
> index 2cd9bd288910..1fa457a5736e 100644
> --- a/drivers/scsi/qla2xxx/qla_nvme.c
> +++ b/drivers/scsi/qla2xxx/qla_nvme.c
> @@ -555,10 +555,12 @@ static int qla_nvme_post_cmd(struct nvme_fc_local_port *lport,
>  
>  	fcport = qla_rport->fcport;
>  
> -	if (!qpair || !fcport || (qpair && !qpair->fw_started) ||
> -	    (fcport && fcport->deleted))
> +	if (!qpair || !fcport)
>  		return -ENODEV;
>  
> +	if (!qpair->fw_started || fcport->deleted)
> +		return -EBUSY;
> +
>  	vha = fcport->vha;
>  
>  	if (!(fcport->nvme_flag & NVME_FLAG_REGISTERED))
> 

Thanks Daniel.

Reviewed-by: Arun Easi <aeasi@marvell.com>
Finn Thain Oct. 12, 2020, 11:59 p.m. UTC | #2
On Mon, 12 Oct 2020, Daniel Wagner wrote:

> When the fcport is about to be deleted we should return EBUSY instead
> of ENODEV. Only for EBUSY the request will be requeued in a multipath
> setup.
> 
> Also in case we have a valid qpair but the firmware has not yet
> started return EBUSY to avoid dropping the request.
> 
> Signed-off-by: Daniel Wagner <dwagner@suse.de>
> ---
> 
> v3: simplify test logic as suggested by Arun.

Not exactly a "simplification": there was a change of behaviour between v2 
and v3. It seems the commit log no longer reflects the code.

> v2: rebased on mkp/staging
> 
>  drivers/scsi/qla2xxx/qla_nvme.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_nvme.c b/drivers/scsi/qla2xxx/qla_nvme.c
> index 2cd9bd288910..1fa457a5736e 100644
> --- a/drivers/scsi/qla2xxx/qla_nvme.c
> +++ b/drivers/scsi/qla2xxx/qla_nvme.c
> @@ -555,10 +555,12 @@ static int qla_nvme_post_cmd(struct nvme_fc_local_port *lport,
>  
>  	fcport = qla_rport->fcport;
>  
> -	if (!qpair || !fcport || (qpair && !qpair->fw_started) ||
> -	    (fcport && fcport->deleted))
> +	if (!qpair || !fcport)
>  		return -ENODEV;
>  
> +	if (!qpair->fw_started || fcport->deleted)
> +		return -EBUSY;
> +
>  	vha = fcport->vha;
>  
>  	if (!(fcport->nvme_flag & NVME_FLAG_REGISTERED))
>
Daniel Wagner Oct. 13, 2020, 6:52 a.m. UTC | #3
On Tue, Oct 13, 2020 at 10:59:18AM +1100, Finn Thain wrote:
> 
> On Mon, 12 Oct 2020, Daniel Wagner wrote:
> 
> > When the fcport is about to be deleted we should return EBUSY instead
> > of ENODEV. Only for EBUSY the request will be requeued in a multipath
> > setup.
> > 
> > Also in case we have a valid qpair but the firmware has not yet
> > started return EBUSY to avoid dropping the request.
> > 
> > Signed-off-by: Daniel Wagner <dwagner@suse.de>
> > ---
> > 
> > v3: simplify test logic as suggested by Arun.
> 
> Not exactly a "simplification": there was a change of behaviour between v2 
> and v3. It seems the commit log no longer reflects the code.

How so? I am struggling to see how it could be a change in behavior. But
then I sometimes fail at simple logic ;)

v2 and v3 will return ENODEV if qpair or fcport are invalid and for
EBUSY one of the other condition needs be true. The difference between
v2 and v3 should only be the order how tests are executed. The outcome
should be the same.
Finn Thain Oct. 14, 2020, 12:57 a.m. UTC | #4
On Tue, 13 Oct 2020, Daniel Wagner wrote:

> On Tue, Oct 13, 2020 at 10:59:18AM +1100, Finn Thain wrote:
> > 
> > On Mon, 12 Oct 2020, Daniel Wagner wrote:
> > 
> > > When the fcport is about to be deleted we should return EBUSY 
> > > instead of ENODEV. Only for EBUSY the request will be requeued in a 
> > > multipath setup.
> > > 
> > > Also in case we have a valid qpair but the firmware has not yet 
> > > started return EBUSY to avoid dropping the request.
> > > 
> > > Signed-off-by: Daniel Wagner <dwagner@suse.de>
> > > ---
> > > 
> > > v3: simplify test logic as suggested by Arun.
> > 
> > Not exactly a "simplification": there was a change of behaviour 
> > between v2 and v3. It seems the commit log no longer reflects the 
> > code.
> 
> How so? I am struggling to see how it could be a change in behavior. But 
> then I sometimes fail at simple logic ;)
> 

Me too, so I confirmed the result by executing the code snippets.

> v2 and v3 will return ENODEV if qpair or fcport are invalid and for 
> EBUSY one of the other condition needs be true. The difference between 
> v2 and v3 should only be the order how tests are executed. The outcome 
> should be the same.
> 

Here's a truth table for v2:

qpair fw_started fcport deleted result
---------------------------------------
F       X       F       X       -ENODEV
F       F       T       F       -ENODEV
F       F       T       T       -EBUSY  *
F       T       T       F       -ENODEV
F       T       T       T       -EBUSY  *
T       F       F       X       -EBUSY  *
T       F       T       X       -EBUSY
T       T       F       X       -ENODEV
T       T       T       F       neither
T       T       T       T       -EBUSY

Here's a truth table for v3:

qpair fw_started fcport deleted result
---------------------------------------
F       X       F       X       -ENODEV
F       F       T       F       -ENODEV
F       F       T       T       -ENODEV *
F       T       T       F       -ENODEV
F       T       T       T       -ENODEV *
T       F       F       X       -ENODEV *
T       F       T       X       -EBUSY
T       T       F       X       -ENODEV
T       T       T       F       neither
T       T       T       T       -EBUSY

The asterisks mark the changed rows.

I don't know whether the changes in v3 are desirable or not, I was just 
pointing out that the commit log ("valid qpair but the firmware has not 
yet started return EBUSY") now seems to disagree with the code.
Daniel Wagner Oct. 14, 2020, 7:06 a.m. UTC | #5
Hi Finn,

On Wed, Oct 14, 2020 at 11:57:04AM +1100, Finn Thain wrote:
> 
> On Tue, 13 Oct 2020, Daniel Wagner wrote:
> > How so? I am struggling to see how it could be a change in behavior. But 
> > then I sometimes fail at simple logic ;)
> > 
> 
> Me too, so I confirmed the result by executing the code snippets.

Thanks for taking the time to explain it to me!

> I don't know whether the changes in v3 are desirable or not, I was just 
> pointing out that the commit log ("valid qpair but the firmware has not 
> yet started return EBUSY") now seems to disagree with the code.

I see where I did my thinko. In v3 we have more ENODEV due to the fact
that we test the pointers first (qpair, fpcort). If either of them is
invalid we get the ENODEV. Actually, it's makes more sense and is likely
to be 'more correct'.

Anyway, let me update the commit log.

Thanks a lot!
Daniel
diff mbox series

Patch

diff --git a/drivers/scsi/qla2xxx/qla_nvme.c b/drivers/scsi/qla2xxx/qla_nvme.c
index 2cd9bd288910..1fa457a5736e 100644
--- a/drivers/scsi/qla2xxx/qla_nvme.c
+++ b/drivers/scsi/qla2xxx/qla_nvme.c
@@ -555,10 +555,12 @@  static int qla_nvme_post_cmd(struct nvme_fc_local_port *lport,
 
 	fcport = qla_rport->fcport;
 
-	if (!qpair || !fcport || (qpair && !qpair->fw_started) ||
-	    (fcport && fcport->deleted))
+	if (!qpair || !fcport)
 		return -ENODEV;
 
+	if (!qpair->fw_started || fcport->deleted)
+		return -EBUSY;
+
 	vha = fcport->vha;
 
 	if (!(fcport->nvme_flag & NVME_FLAG_REGISTERED))