Re: [RFC] FC pass thru - Rev V

[Boaz Harrosh <bharrosh@panasas.com>]

James Smart wrote:
> Trying to kick-start this again...  
> I've updated the prior RFC with the comments from Seokmann,
> SvenFujita, and Boaz. I would still like review on the
> blk_xxx completion calls in the std and error paths.
> 
> It currently expects that blk_end_request() has been updated
> by Fujita's patch to incorporate blk_end_bidi_request()
> functionality :
> http://marc.info/?l-linux-scsi&m=122785157116659&w=2
> 

I did not accept this patch and it did not go in right?
I still don't like it, it's a performance regression.

> -- james s
> 
> ======
> 
> All,
> 
> I've reworked Seokmann's patch for the following items:
> - Add an fchost interface for bsg requests
> 
> - Formalized the request/response structures that I expect
>   to have us stuff into the bsg cmd/sense data areas. These
>   are now genericized so we can essentially pass any kind of
>   transaction. It can be a request that has no transmit or
>   receive payload, and simply returns a response.
> 
> - A new file was created, scsi_bsg_fc.h, which contains the
>   request/response data structures that should be shared
>   between the application and the kernel entities. 
> 
> - I stripped out some things that were in the request
>   structure that were actually LLD fields. Instead, I added
>   a dd_bsgsize structure to the template, so the transport
>   will allocate LLD work space along with the job structure.
>   I expect the missing fields to move to this area.
> 
> - I've made a strong attempt at ensuring that the request
>   has all the information necessary for the LLD, so that
>   there is no need to have the LLD remap the transmit payload
>   to figure things out. Granted, this comes at the cost of
>   replicating some data items.
> 
>   Sven, I've added the CT information you needed as part of this.
> 
> - I've renamed things quite a bit, hoping to make it clarity
>   better. The "service" struct is now a job. I still have
>   headaches with "request" (is it the blk request, or the job
>   request, or what..)
> 
> - The CT/ELS response is a bit funky. I've noted that the
>   way Emulex returns a response, vs Qlogic is a bit different,
>   thus the 2 ways to indicate "reject".
> 
> - fixed a couple of bugs in Seokmann's code, in the teardown,
>   error flows, request que dma settings, etc.
> 
> - I added a "vendor_id" field to the scsi_host_template to
>   use when verifying that the recipient knows how to decode
>   vendor-specific message. I didn't do this with the netlink
>   things as I was prepping it to not break kabi in existing
>   and older kernels. But, I believe this is a good time to
>   add it.
> 
> - I've started the Documentation/scsi/scsi_transport_fc.txt
>   documentation, but punted finishing it in lieu of sending
>   this RFC. I'm starting from Seokman's original emails and
>   will be updating for this reformat.
> 
> I'm only starting to debug this, so user beware.
> 
> I could really use some code review from Fujita or Boaz, to
> make sure I'm calling the right blk_xx completion functions
> relative to the setup flow, and to ensure that the "goose"
> when I jump out while the rport is blocked is correct.
> 
> Comments welcome
> 
> -- james s
> 
> 
> 
> 
> 
>  Signed-off-by: James Smart <james.smart@emulex.com>
> 
>  ---
> 
>  Documentation/scsi/scsi_fc_transport.txt |   11 
>  Documentation/scsi/scsi_mid_low_api.txt  |    5 
>  drivers/scsi/scsi_transport_fc.c         |  614 ++++++++++++++++++++++++++++++-
>  include/scsi/scsi_bsg_fc.h               |  322 ++++++++++++++++
>  include/scsi/scsi_host.h                 |    9 
>  include/scsi/scsi_transport_fc.h         |   52 ++
>  6 files changed, 1009 insertions(+), 4 deletions(-)
> 
> 
> diff -upNr a/Documentation/scsi/scsi_fc_transport.txt b/Documentation/scsi/scsi_fc_transport.txt
> --- a/Documentation/scsi/scsi_fc_transport.txt	2009-01-27 09:44:22.000000000 -0500
> +++ b/Documentation/scsi/scsi_fc_transport.txt	2009-02-10 10:34:42.000000000 -0500
> @@ -1,10 +1,11 @@
>                               SCSI FC Tansport
>                   =============================================
>  
> -Date:  4/12/2007
> +Date:  11/18/2008
>  Kernel Revisions for features:
>    rports : <<TBS>>
>    vports : 2.6.22 (? TBD)
> +  bsg support : 2.6.29 (?TBD)
>  
>  
>  Introduction
> @@ -15,6 +16,7 @@ The FC transport can be found at:
>    drivers/scsi/scsi_transport_fc.c
>    include/scsi/scsi_transport_fc.h
>    include/scsi/scsi_netlink_fc.h
> +  include/scsi/scsi_bsg_fc.h
>  
>  This file is found at Documentation/scsi/scsi_fc_transport.txt
>  
> @@ -472,6 +474,13 @@ int
>  fc_vport_terminate(struct fc_vport *vport)
>  
>  
> +FC BSG support (CT & ELS passthru, and more)
> +========================================================================
> +<< To Be Supplied >>
> +
> +
> +
> +
>  Credits
>  =======
>  The following people have contributed to this document:
> diff -upNr a/Documentation/scsi/scsi_mid_low_api.txt b/Documentation/scsi/scsi_mid_low_api.txt
> --- a/Documentation/scsi/scsi_mid_low_api.txt	2008-09-23 15:11:57.000000000 -0400
> +++ b/Documentation/scsi/scsi_mid_low_api.txt	2009-02-10 10:34:42.000000000 -0500
> @@ -1271,6 +1271,11 @@ of interest:
>      hostdata[0]  - area reserved for LLD at end of struct Scsi_Host. Size
>                     is set by the second argument (named 'xtr_bytes') to
>                     scsi_host_alloc() or scsi_register().
> +    vendor_id    - a unique value that identifies the vendor supplying
> +                   the LLD for the Scsi_Host.  Used most often in validating
> +                   vendor-specific message requests.  Value consists of an
> +                   identifier type and a vendor-specific value.
> +                   See scsi_netlink.h for a description of valid formats.
>  
>  The scsi_host structure is defined in include/scsi/scsi_host.h
>  
> diff -upNr a/drivers/scsi/scsi_transport_fc.c b/drivers/scsi/scsi_transport_fc.c
> --- a/drivers/scsi/scsi_transport_fc.c	2009-01-27 09:44:31.000000000 -0500
> +++ b/drivers/scsi/scsi_transport_fc.c	2009-02-10 14:58:39.000000000 -0500
> @@ -35,6 +35,7 @@
>  #include <linux/netlink.h>
>  #include <net/netlink.h>
>  #include <scsi/scsi_netlink_fc.h>
> +#include <scsi/scsi_bsg_fc.h>
>  #include "scsi_priv.h"
>  #include "scsi_transport_fc_internal.h"
>  
> @@ -43,6 +44,10 @@ static void fc_vport_sched_delete(struct
>  static int fc_vport_setup(struct Scsi_Host *shost, int channel,
>  	struct device *pdev, struct fc_vport_identifiers  *ids,
>  	struct fc_vport **vport);
> +static int fc_bsg_hostadd(struct Scsi_Host *, struct fc_host_attrs *);
> +static int fc_bsg_rportadd(struct Scsi_Host *, struct fc_rport *);
> +static void fc_bsg_remove(struct request_queue *);
> +static void fc_bsg_goose_queue(struct fc_rport *);
>  
>  /*
>   * Redefine so that we can have same named attributes in the
> @@ -411,13 +416,26 @@ static int fc_host_setup(struct transpor
>  		return -ENOMEM;
>  	}
>  
> +	fc_bsg_hostadd(shost, fc_host);
> +	/* ignore any bsg add error - we just can't do sgio */
> +
> +	return 0;
> +}
> +
> +static int fc_host_remove(struct transport_container *tc, struct device *dev,
> +			 struct device *cdev)
> +{
> +	struct Scsi_Host *shost = dev_to_shost(dev);
> +	struct fc_host_attrs *fc_host = shost_to_fc_host(shost);
> +
> +	fc_bsg_remove(fc_host->rqst_q);
>  	return 0;
>  }
>  
>  static DECLARE_TRANSPORT_CLASS(fc_host_class,
>  			       "fc_host",
>  			       fc_host_setup,
> -			       NULL,
> +			       fc_host_remove,
>  			       NULL);
>  
>  /*
> @@ -2383,6 +2401,7 @@ fc_rport_final_delete(struct work_struct
>  		scsi_flush_work(shost);
>  
>  	fc_terminate_rport_io(rport);
> +
>  	/*
>  	 * Cancel any outstanding timers. These should really exist
>  	 * only when rmmod'ing the LLDD and we're asking for
> @@ -2415,6 +2434,8 @@ fc_rport_final_delete(struct work_struct
>  	    (i->f->dev_loss_tmo_callbk))
>  		i->f->dev_loss_tmo_callbk(rport);
>  
> +	fc_bsg_remove(rport->rqst_q);
> +
>  	transport_remove_device(dev);
>  	device_del(dev);
>  	transport_destroy_device(dev);
> @@ -2502,6 +2523,9 @@ fc_rport_create(struct Scsi_Host *shost,
>  	transport_add_device(dev);
>  	transport_configure_device(dev);
>  
> +	fc_bsg_rportadd(shost, rport);
> +	/* ignore any bsg add error - we just can't do sgio */
> +
>  	if (rport->roles & FC_PORT_ROLE_FCP_TARGET) {
>  		/* initiate a scan of the target */
>  		rport->flags |= FC_RPORT_SCAN_PENDING;
> @@ -2666,6 +2690,8 @@ fc_remote_port_add(struct Scsi_Host *sho
>  					spin_unlock_irqrestore(shost->host_lock,
>  							flags);
>  
> +				fc_bsg_goose_queue(rport);
> +
>  				return rport;
>  			}
>  		}
> @@ -3351,6 +3377,592 @@ fc_vport_sched_delete(struct work_struct
>  }
>  
>  
> +/*
> + * BSG support
> + */
> +
> +
> +/**
> + * fc_destroy_bsgjob - routine to teardown/delete a fc bsg job
> + * @job:	fc_bsg_job that is to be torn down
> + */
> +static void
> +fc_destroy_bsgjob(struct fc_bsg_job *job)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&job->job_lock, flags);
> +	if (job->ref_cnt) {
> +		spin_unlock_irqrestore(&job->job_lock, flags);
> +		return;
> +	}
> +	spin_unlock_irqrestore(&job->job_lock, flags);
> +
> +	put_device(job->dev);	/* release reference for the request */
> +
> +	kfree(job->request_payload.sg_list);
> +	kfree(job->reply_payload.sg_list);
> +	kfree(job);
> +}
> +
> +
> +/**
> + * fc_bsg_jobdone - completion routine for bsg requests that the LLD has
> + *                  completed
> + * @job:	fc_bsg_job that is complete
> + */
> +static void
> +fc_bsg_jobdone(struct fc_bsg_job *job)
> +{
> +	struct request *req = job->req;
> +	struct request *rsp = req->next_rq;
> +	unsigned long flags;
> +	unsigned rsp_len;
-	unsigned rsp_len;
+	unsigned rsp_len = 0;
+	unsigned req_len = 0;

See below

> +	int err;
> +
> +	spin_lock_irqsave(&job->job_lock, flags);
> +	job->state_flags |= FC_RQST_STATE_DONE;
> +	job->ref_cnt--;
> +	spin_unlock_irqrestore(&job->job_lock, flags);
> +
> +	err = job->req->errors = job->reply->result;
> +	if (err < 0)
> +		/* we're only returning the result field in the reply */
> +		job->req->sense_len = sizeof(uint32_t);
> +	else
> +		job->req->sense_len = job->reply_len;
> +

Why use of  job->req when you have req = job->req above, it's confusing.

> +	if (rsp) {
> +		rsp_len = blk_rq_bytes(rsp);
> +		BUG_ON(job->reply->reply_payload_rcv_len > rsp_len);
> +		/* set reply (bidi) residual */
> +		rsp->data_len = (rsp_len - job->reply->reply_payload_rcv_len);
> +	}
> +
> +	/* we assume all request payload was transferred */
> +	blk_end_request(req, err, blk_rq_bytes(req));

Don't you need residual count in bsg caller?

This code will always return full request->data_len as residual count
of out/uni_in to caller of bsg SG_IO caller.

and it will only work with the un-accepted patch above just do:

+	if (rsp) {
+		rsp_len = blk_rq_bytes(rsp);
+		BUG_ON(job->reply->reply_payload_rcv_len > rsp_len);
+		/* set reply (bidi) residual */
+		rsp->data_len = (rsp_len - job->reply->reply_payload_rcv_len);
+	}
+
+	req_len = blk_rq_bytes(req);
+	/* we assume all request payload was transferred */
+	req->data_len = 0;
+	blk_end_bidi_request(req, err, req_len, rsp_len);

This will handle all cases today without need of above patch.

> +
> +	fc_destroy_bsgjob(job);
> +}
> +
> +
<snip>

Thanks
Boaz