Re: [PATCH RFC v3 07/41] fnic: use reserved commands

From: Ming Lei <ming.lei@redhat.com>
To: Hannes Reinecke <hare@suse.de>
Cc: "Martin K. Petersen" <martin.petersen@oracle.com>,
	Christoph Hellwig <hch@lst.de>,
	James Bottomley <james.bottomley@hansenpartnership.com>,
	John Garry <john.garry@huawei.com>,
	Bart van Assche <bvanassche@acm.org>,
	linux-scsi@vger.kernel.org, Hannes Reinecke <hare@suse.com>
Subject: Re: [PATCH RFC v3 07/41] fnic: use reserved commands
Date: Thu, 30 Apr 2020 23:24:17 +0800	[thread overview]
Message-ID: <20200430152417.GD1005453@T590> (raw)
In-Reply-To: <20200430131904.5847-8-hare@suse.de>

On Thu, Apr 30, 2020 at 03:18:30PM +0200, Hannes Reinecke wrote:
> From: Hannes Reinecke <hare@suse.com>
> 
> Remove hack to get tag for the reset command by using reserved
> commands.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.com>
> ---
>  drivers/scsi/fnic/fnic_scsi.c | 147 ++++++++++++++----------------------------
>  1 file changed, 49 insertions(+), 98 deletions(-)
> 
> diff --git a/drivers/scsi/fnic/fnic_scsi.c b/drivers/scsi/fnic/fnic_scsi.c
> index b60795893994..228729013e21 100644
> --- a/drivers/scsi/fnic/fnic_scsi.c
> +++ b/drivers/scsi/fnic/fnic_scsi.c
> @@ -101,7 +101,7 @@ static const char *fnic_fcpio_status_to_str(unsigned int status)
>  	return fcpio_status_str[status];
>  }
>  
> -static void fnic_cleanup_io(struct fnic *fnic, int exclude_id);
> +static void fnic_cleanup_io(struct fnic *fnic);
>  
>  static inline spinlock_t *fnic_io_lock_hash(struct fnic *fnic,
>  					    struct scsi_cmnd *sc)
> @@ -637,7 +637,7 @@ static int fnic_fcpio_fw_reset_cmpl_handler(struct fnic *fnic,
>  	atomic64_inc(&reset_stats->fw_reset_completions);
>  
>  	/* Clean up all outstanding io requests */
> -	fnic_cleanup_io(fnic, SCSI_NO_TAG);
> +	fnic_cleanup_io(fnic);
>  
>  	atomic64_set(&fnic->fnic_stats.fw_stats.active_fw_reqs, 0);
>  	atomic64_set(&fnic->fnic_stats.io_stats.active_ios, 0);
> @@ -1359,7 +1359,7 @@ int fnic_wq_copy_cmpl_handler(struct fnic *fnic, int copy_work_to_do)
>  	return wq_work_done;
>  }
>  
> -static void fnic_cleanup_io(struct fnic *fnic, int exclude_id)
> +static void fnic_cleanup_io(struct fnic *fnic)
>  {
>  	int i;
>  	struct fnic_io_req *io_req;
> @@ -1370,9 +1370,6 @@ static void fnic_cleanup_io(struct fnic *fnic, int exclude_id)
>  	struct fnic_stats *fnic_stats = &fnic->fnic_stats;
>  
>  	for (i = 0; i < fnic->fnic_max_tag_id; i++) {
> -		if (i == exclude_id)
> -			continue;
> -
>  		io_lock = fnic_io_lock_tag(fnic, i);
>  		spin_lock_irqsave(io_lock, flags);
>  		sc = scsi_host_find_tag(fnic->lport->host, i);
> @@ -2125,9 +2122,7 @@ static inline int fnic_queue_dr_io_req(struct fnic *fnic,
>   * successfully aborted, 1 otherwise
>   */
>  static int fnic_clean_pending_aborts(struct fnic *fnic,
> -				     struct scsi_cmnd *lr_sc,
> -					 bool new_sc)
> -
> +				     struct scsi_cmnd *lr_sc)
>  {
>  	int tag, abt_tag;
>  	struct fnic_io_req *io_req;
> @@ -2148,7 +2143,7 @@ static int fnic_clean_pending_aborts(struct fnic *fnic,
>  		 * ignore this lun reset cmd if issued using new SC
>  		 * or cmds that do not belong to this lun
>  		 */
> -		if (!sc || ((sc == lr_sc) && new_sc) || sc->device != lun_dev) {
> +		if (!sc || sc == lr_sc || sc->device != lun_dev) {
>  			spin_unlock_irqrestore(io_lock, flags);
>  			continue;
>  		}
> @@ -2287,38 +2282,6 @@ static int fnic_clean_pending_aborts(struct fnic *fnic,
>  	return ret;
>  }
>  
> -/**
> - * fnic_scsi_host_start_tag
> - * Allocates tagid from host's tag list
> - **/
> -static inline int
> -fnic_scsi_host_start_tag(struct fnic *fnic, struct scsi_cmnd *sc)
> -{
> -	struct request_queue *q = sc->request->q;
> -	struct request *dummy;
> -
> -	dummy = blk_mq_alloc_request(q, REQ_OP_WRITE, BLK_MQ_REQ_NOWAIT);
> -	if (IS_ERR(dummy))
> -		return SCSI_NO_TAG;
> -
> -	sc->tag = sc->request->tag = dummy->tag;
> -	sc->host_scribble = (unsigned char *)dummy;
> -
> -	return dummy->tag;
> -}
> -
> -/**
> - * fnic_scsi_host_end_tag
> - * frees tag allocated by fnic_scsi_host_start_tag.
> - **/
> -static inline void
> -fnic_scsi_host_end_tag(struct fnic *fnic, struct scsi_cmnd *sc)
> -{
> -	struct request *dummy = (struct request *)sc->host_scribble;
> -
> -	blk_mq_free_request(dummy);
> -}
> -
>  /*
>   * SCSI Eh thread issues a Lun Reset when one or more commands on a LUN
>   * fail to get aborted. It calls driver's eh_device_reset with a SCSI command
> @@ -2335,19 +2298,19 @@ int fnic_device_reset(struct scsi_cmnd *sc)
>  	spinlock_t *io_lock;
>  	unsigned long flags;
>  	unsigned long start_time = 0;
> +	struct scsi_device *sdev = sc->device;
>  	struct scsi_lun fc_lun;
>  	struct fnic_stats *fnic_stats;
>  	struct reset_stats *reset_stats;
>  	int tag = 0;
>  	DECLARE_COMPLETION_ONSTACK(tm_done);
> -	int tag_gen_flag = 0;   /*to track tags allocated by fnic driver*/
> -	bool new_sc = 0;
> +	struct scsi_cmnd *reset_sc = NULL;
>  
>  	/* Wait for rport to unblock */
>  	fc_block_scsi_eh(sc);
>  
>  	/* Get local-port, check ready and link up */
> -	lp = shost_priv(sc->device->host);
> +	lp = shost_priv(sdev->host);
>  
>  	fnic = lport_priv(lp);
>  	fnic_stats = &fnic->fnic_stats;
> @@ -2355,10 +2318,10 @@ int fnic_device_reset(struct scsi_cmnd *sc)
>  
>  	atomic64_inc(&reset_stats->device_resets);
>  
> -	rport = starget_to_rport(scsi_target(sc->device));
> +	rport = starget_to_rport(scsi_target(sdev));
>  	FNIC_SCSI_DBG(KERN_DEBUG, fnic->lport->host,
>  		      "Device reset called FCID 0x%x, LUN 0x%llx sc 0x%p\n",
> -		      rport->port_id, sc->device->lun, sc);
> +		      rport->port_id, sdev->lun, sc);
>  
>  	if (lp->state != LPORT_ST_READY || !(lp->link_up))
>  		goto fnic_device_reset_end;
> @@ -2369,42 +2332,31 @@ int fnic_device_reset(struct scsi_cmnd *sc)
>  		goto fnic_device_reset_end;
>  	}
>  
> -	CMD_FLAGS(sc) = FNIC_DEVICE_RESET;
> -	/* Allocate tag if not present */
> +	reset_sc = scsi_get_reserved_cmd(sdev, DMA_NONE);

scsi_get_reserved_cmd() returns NULL if .nr_reserved_cmds isn't passed,
so I guess you forget to do that?

Thanks,
Ming