Re: [PATCH v2 3/3] qla2xxx: Fix NVME cmd and LS cmd timeout race condition

[Quinn Tran <qutran@xxxxxxxxxxx>]

Bart,

Yes. It does make sense to combine SRB into nvme_private/scsi_cmnd private.  That's is a larger discussion that require our team to buy in + test effort.  We are actively discussing this subject. 

In the mean time, we like other NVME users to not run into this crash until the next submission window.

Regards,
Quinn Tran 

On 6/19/19, 1:36 PM, "linux-scsi-owner@xxxxxxxxxxxxxxx on behalf of Bart Van Assche" <linux-scsi-owner@xxxxxxxxxxxxxxx on behalf of bvanassche@xxxxxxx> wrote:

    On 6/18/19 11:10 AM, Himanshu Madhani wrote:
    > diff --git a/drivers/scsi/qla2xxx/qla_nvme.h b/drivers/scsi/qla2xxx/qla_nvme.h
    > index 2d088add7011..67bb4a2a3742 100644
    > --- a/drivers/scsi/qla2xxx/qla_nvme.h
    > +++ b/drivers/scsi/qla2xxx/qla_nvme.h
    > @@ -34,6 +34,7 @@ struct nvme_private {
    >   	struct work_struct ls_work;
    >   	struct work_struct abort_work;
    >   	int comp_status;
    > +	spinlock_t cmd_lock;
    >   };
    
    Hi Himanshu and Quinn,
    
     From the qla2xxx driver:
    
    static struct nvme_fc_port_template qla_nvme_fc_transport = {
    	[ ... ]
    	.lsrqst_priv_sz = sizeof(struct nvme_private),
    	.fcprqst_priv_sz = sizeof(struct nvme_private),
    };
    [ ... ]
    struct nvme_private {
    	struct srb	*sp;
    	struct nvmefc_ls_req *fd;
    	struct work_struct ls_work;
    	struct work_struct abort_work;
    	int comp_status;
    };
    
    Has it been considered to change "struct srb *sp" into "struct srb srb"? 
    That would guarantee that the srb and nvme_private data structures have 
    the same lifetime. As a result using the srb reference count would no 
    longer be necessary for NVMe and no new locking would have to be 
    introduced in the NVMe-FC completion paths.
    
    I think a similar approach is possible for the SCSI-FC code.
    
    Does this make sense to you?
    
    Thanks,
    
    Bart.