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