Re: [PATCH 15/47] scsi_transport_fc: Use fc_rport as argument for fc_block_scsi_eh

Steffen Maier
2017-07-24T16:34:13Z

On 06/28/2017 10:32 AM, Hannes Reinecke wrote:
> fc_block_scsi_eh() works on a remote port, so we should be using
> that as an argument, and not the scsi command.

> diff --git a/drivers/s390/scsi/zfcp_scsi.c b/drivers/s390/scsi/zfcp_scsi.c
> index d7d4a63..e3160fc 100644
> --- a/drivers/s390/scsi/zfcp_scsi.c
> +++ b/drivers/s390/scsi/zfcp_scsi.c
> @@ -150,6 +150,7 @@ static int zfcp_scsi_eh_abort_handler(struct scsi_cmnd *scpnt)
> struct zfcp_adapter *adapter =
> (struct zfcp_adapter *) scsi_host->hostdata[0];
> struct zfcp_fsf_req *old_req, *abrt_req;
> + struct fc_rport *rport = starget_to_rport(scsi_target(scpnt->device));
> unsigned long flags;
> unsigned long old_reqid = (unsigned long) scpnt->host_scribble;
> int retval = SUCCESS, ret;
> @@ -176,7 +177,7 @@ static int zfcp_scsi_eh_abort_handler(struct scsi_cmnd *scpnt)
> break;
>
> zfcp_erp_wait(adapter);
> - ret = fc_block_scsi_eh(scpnt);
> + ret = fc_block_scsi_eh(rport);

I think for abort handlers some original fc_block_scsi_eh with scsi_cmnd
as argument would be perfectly valid as this really has the scope of a
particular single command.

Do you really want to touch all abort handlers just because of this?

In my zfcp decoupling I had created a new fc_block_rport for
TMFs/host_reset and kept using the old fc_block_scsi_eh (which does the
argument lifting and simply delegates to the new fc_block_rport) for the
abort handler.
Admittedly, of course, I also did so because I did not touch all other
users/callers of the old function with the scsi_cmnd argument.

> if (ret) {
> zfcp_dbf_scsi_abort("abrt_bl", scpnt, NULL);
> return ret;
> @@ -262,6 +263,7 @@ static int zfcp_task_mgmt_function(struct scsi_cmnd *scpnt, u8 tm_flags)
> {
> struct zfcp_scsi_dev *zfcp_sdev = sdev_to_zfcp(scpnt->device);
> struct zfcp_adapter *adapter = zfcp_sdev->port->adapter;
> + struct fc_port *rport = zfcp_sdev->port->rport;

port->rport could be NULL

[async zfcp_scsi_rport_block()]

> struct zfcp_fsf_req *fsf_req = NULL;
> int retval = SUCCESS, ret;
> int retry = 3;
> @@ -272,7 +274,7 @@ static int zfcp_task_mgmt_function(struct scsi_cmnd *scpnt, u8 tm_flags)
> break;
>
> zfcp_erp_wait(adapter);
> - ret = fc_block_scsi_eh(scpnt);
> + ret = fc_block_scsi_eh(rport);
> if (ret)
> return ret;
>

> diff --git a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
> index 6dd0922..f83e512 100644
> --- a/drivers/scsi/scsi_transport_fc.c
> +++ b/drivers/scsi/scsi_transport_fc.c
> @@ -3273,7 +3273,7 @@ struct fc_rport *
>
> /**
> * fc_block_scsi_eh - Block SCSI eh thread for blocked fc_rport
> - * @cmnd: SCSI command that scsi_eh is trying to recover
> + * @rport: remote port to be checked
> *
> * This routine can be called from a FC LLD scsi_eh callback. It
> * blocks the scsi_eh thread until the fc_rport leaves the
> @@ -3285,10 +3285,9 @@ struct fc_rport *
> * FAST_IO_FAIL if the fast_io_fail_tmo fired, this should be
> * passed back to scsi_eh.
> */
> -int fc_block_scsi_eh(struct scsi_cmnd *cmnd)
> +int fc_block_scsi_eh(struct fc_rport *rport)
> {
> - struct Scsi_Host *shost = cmnd->device->host;
> - struct fc_rport *rport = starget_to_rport(scsi_target(cmnd->device));
> + struct Scsi_Host *shost = rport_to_shost(rport);
> unsigned long flags;
>
> spin_lock_irqsave(shost->host_lock, flags);
> diff --git a/include/scsi/scsi_transport_fc.h b/include/scsi/scsi_transport_fc.h
> index e308cd5..5b26943 100644
> --- a/include/scsi/scsi_transport_fc.h
> +++ b/include/scsi/scsi_transport_fc.h
> @@ -804,7 +804,7 @@ void fc_host_post_vendor_event(struct Scsi_Host *shost, u32 event_number,
> struct fc_vport *fc_vport_create(struct Scsi_Host *shost, int channel,
> struct fc_vport_identifiers *);
> int fc_vport_terminate(struct fc_vport *vport);
> -int fc_block_scsi_eh(struct scsi_cmnd *cmnd);
> +int fc_block_scsi_eh(struct fc_rport *rport);
> enum blk_eh_timer_return fc_eh_timed_out(struct scsi_cmnd *scmd);
>
> static inline struct Scsi_Host *fc_bsg_to_shost(struct bsg_job *job)
>

Aside from above thoughts, the coding of the argument change seems good.

--
Mit freundlichen Grüßen / Kind regards
Steffen Maier

Linux on z Systems Development

IBM Deutschland Research & Development GmbH
Vorsitzende des Aufsichtsrats: Martina Koederitz
Geschaeftsfuehrung: Dirk Wittkopp
Sitz der Gesellschaft: Boeblingen
Registergericht: Amtsgericht Stuttgart, HRB 243294