mbox series

[0/3] fnic: use scsi_host_busy_iter() to traverse commands

Message ID 20210429122517.39659-1-hare@suse.de
Headers show
Series fnic: use scsi_host_busy_iter() to traverse commands | expand

Message

Hannes Reinecke April 29, 2021, 12:25 p.m. UTC 
Hi all,

the fnic driver is walking the list of tags manually, causing frequent
crashes as the block layer doesn't necessarily cleans up requests after
usage.
So switch to scsi_host_busy_iter() to traverse commands avoiding this
problem.

As usual, comments and reviews are welcome.

Hannes Reinecke (3):
  fnic: kill 'exclude_id' argument to fnic_cleanup_io()
  fnic: use scsi_host_busy_iter() to traverse commands
  fnic: check for started requests in fnic_wq_copy_cleanup_handler()

 drivers/scsi/fnic/fnic_scsi.c | 830 ++++++++++++++++------------------
 1 file changed, 378 insertions(+), 452 deletions(-)

Comments

Ming Lei April 30, 2021, 6:10 a.m. UTC | #1 
On Thu, Apr 29, 2021 at 02:25:15PM +0200, Hannes Reinecke wrote:
> 'exclude_id' is always SCSI_NO_TAG, which will never be reached

> when traversing the list of tags.

> 

> Signed-off-by: Hannes Reinecke <hare@suse.de>

> ---

>  drivers/scsi/fnic/fnic_scsi.c | 9 +++------

>  1 file changed, 3 insertions(+), 6 deletions(-)

> 

> diff --git a/drivers/scsi/fnic/fnic_scsi.c b/drivers/scsi/fnic/fnic_scsi.c

> index e619a82f921b..f41d1b1c2e39 100644

> --- a/drivers/scsi/fnic/fnic_scsi.c

> +++ b/drivers/scsi/fnic/fnic_scsi.c

> @@ -102,7 +102,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)

> @@ -638,7 +638,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);

> @@ -1361,7 +1361,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;

> @@ -1372,9 +1372,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);

> -- 

> 2.29.2

> 


Reviewed-by: Ming Lei <ming.lei@redhat.com>


-- 
Ming
Ming Lei April 30, 2021, 6:17 a.m. UTC | #2 
On Thu, Apr 29, 2021 at 02:25:16PM +0200, Hannes Reinecke wrote:
> Use scsi_host_busy_iter() to traverse commands instead of

> hand-crafted routines walking the command list.

> 

> Signed-off-by: Hannes Reinecke <hare@suse.de>

> ---

>  drivers/scsi/fnic/fnic_scsi.c | 821 ++++++++++++++++------------------

>  1 file changed, 375 insertions(+), 446 deletions(-)


Looks fine:

Reviewed-by: Ming Lei <ming.lei@redhat.com>


-- 
Ming