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@xxxxxxxxxx> > > --- > > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
