[PATCH 0/2] FC pass through support via bsg interface

[PATCH 0/2] FC pass through support via bsg interface

[Seokmann Ju]

Hi,

After the RFC, here is the patch.
Again, the implementation has following remarks,
- the SMP support in the SAS transport has been referenced, so the  
layout of the changes are quite similar to it.
- at this stage, the ELS/CT service packet serviced one at a time,  
synchronously.
- in the scsi_transport_fc module, there is dedicated handler in the  
LLDD for ELS and CT, respectively.
- device files are created in following directories for each of  
discovered initiator/targets,
	= /sys/class/bsg/rport-<host_no>:<bus>-<id>
	= dev/rport-<host_no>:<bus>-<id>
- the qla2xxx module has been tested with the simple application that  
issues some of ELS and CT service requests.
- abort mechanism is in place.

This patch was made against scsi-misc.

Thank you,
Seokmann

Re: [PATCH 0/2] FC pass through support via bsg interface

[James Smart]

Seokmann,

There are number of fundamental issues with this patch that need to be 
corrected. I'll be at plumbers conference next week, and will be able to 
work through this. Please look me up if you will be there. Otherwise, 
I'll just post a revised patch.

Here are the issues as I see them:

- There is no concept of a Request Structure, that supplies the type of
   request and other parameters (such as a timeout). This should have
   come from the cdb bytes.  Over time, we can expand this request
   structure for other types of requests, not just els/ct passthru.

- There is no true definition of what the request payload should be.
   It's implied that it should be a fully built frame header, followed by
   payload.  We should be striking the frame header, and just stating
   that for els/ct, the request payload is the tx sequence payload.

- There is no definition of what the response payload should be. Does
   it contain a header too ? or what ? If the request is rejected,
   could it also contain other frame types ? We should simply state
   that it is the rx sequence payload, and only if successful.

- There is no concept or definition of a Reply structure, which should
   be stuffed into the request sense area.  This should be well defined,
   and matched to the request structure. We whould supply status values,
   or response data from RJT's, BSY's, aborted status's, etc.

- Once we follow the above, there's no need for common FC structures
   and so on. Note: if we did need them, rather than redefining them,
   we would premerge the headers from open-fcoe.

- We really should be making the request handling asynchronous in
   nature, rather than following smp's simple synchronous structure.

- Timeout values should be part of the request, not hardcoded in the
   transport. If there is one in the transport, it would be a default,
   used when the app didn't specify it, and should be tunable by sysfs.

- It would be nice, although difficult, to allow the user to abort
   an outstanding service request. We should look into what sgio
   does in this area.

- We really should add support, for the initial merge, to talk to an
   address that doesn't exist via an rport (think fabric service).
   This means adding some request types to the fc_host too.


-- james s


========

More detailed comments:

scsi_transport_fc.c:

- Do we really need a kmem_cache for service structures ?
   Note: given the blocking nature in what you sent, there could only
     be one request at a time being performed on the rport, so unless
     you're talking about a lot of requests to different rports
     simultaneously, why the cache ?  I'd argue even then, the number
     requests is small, so it doesn't need a cache.

- fc_service_handler is done oddly. I know you copied smp's approach,
   but we have no desire for the lld to override the transports handler.
   We would rather want to have the transport siphone of the requests
   it handles, and fall back to a driver handler.

- There's no reason for separate ct and els service handlers. It can
   be consolidated into a single handler, and data in the service
   structure or initial request structure.

- Have you tested how large of a single buffer you can supply and get
   by on the single sg vector ?  For nameserver payloads we should
   support bigger buffers - 4k as a minimum.

qla2xxx implementation:
- Why does the driver re-allocate buffers for the transmit and
   response payloads, and re-dma map them. Why couldn't it use
   the buffers/sg values from the initial request ?

- How does the timeout path ever work ?  with the qla driver blocking
   in it's request handlers - how does the transport wait_completion()
   ever get invoked prior to a request completing ? And if that's true,
   how did the abort request ever do anything ?


Seokmann Ju wrote:
> Hi,
> 
> After the RFC, here is the patch.
> Again, the implementation has following remarks,
> - the SMP support in the SAS transport has been referenced, so the
> layout of the changes are quite similar to it.
> - at this stage, the ELS/CT service packet serviced one at a time,
> synchronously.
> - in the scsi_transport_fc module, there is dedicated handler in the
> LLDD for ELS and CT, respectively.
> - device files are created in following directories for each of
> discovered initiator/targets,
>         = /sys/class/bsg/rport-<host_no>:<bus>-<id>
>         = dev/rport-<host_no>:<bus>-<id>
> - the qla2xxx module has been tested with the simple applikcation that
> issues some of ELS and CT service requests.
> - abort mechanism is in place.
> 
> This patch was made against scsi-misc.
> 
> Thank you,
> Seokmann
> 

Re: [PATCH 0/2] FC pass through support via bsg interface

[Seokmann Ju]

On Sep 12, 2008, at 9:49 AM, James Smart wrote:

> Seokmann,
>
> There are number of fundamental issues with this patch that need to  
> be corrected. I'll be at plumbers conference next week, and will be  
> able to work through this. Please look me up if you will be there.  
> Otherwise, I'll just post a revised patch.
>
> Here are the issues as I see them:
>
> - There is no concept of a Request Structure, that supplies the type  
> of
>  request and other parameters (such as a timeout). This should have
>  come from the cdb bytes.  Over time, we can expand this request
>  structure for other types of requests, not just els/ct passthru.
OK. the FC transport layer will define them in a strucure and they  
will passed to the driver via request entry of the sg_io_v4.

> - There is no true definition of what the request payload should be.
>  It's implied that it should be a fully built frame header, followed  
> by
>  payload.  We should be striking the frame header, and just stating
>  that for els/ct, the request payload is the tx sequence payload.
Yes, the FC frame header will be removed and raw ELS/CT service  
payload only get passed down...

> - There is no definition of what the response payload should be. Does
>  it contain a header too ? or what ? If the request is rejected,
>  could it also contain other frame types ? We should simply state
>  that it is the rx sequence payload, and only if successful.
This response payload contains the actual ELS/CT response for the  
request.
And, yes, it contains raw ELS/CT response payload without header.
When the request rejected, the response payload contains the actual  
reject formation for the request as defined in the FC standard.
Please let me know if I missed any of points you'd mentioned above...,  
(I guess I did some...)

> - There is no concept or definition of a Reply structure, which should
>  be stuffed into the request sense area.  This should be well defined,
>  and matched to the request structure. We whould supply status values,
>  or response data from RJT's, BSY's, aborted status's, etc.
If I understood correctly, those RJT or BSY formation should be  
available on the 'response'.
If this is true, I guess that there are not many of information to be  
passed back to the application via the reply structure, beside,  
completion status, some vendor specific error code (if any), residual.

> - Once we follow the above, there's no need for common FC structures
>  and so on. Note: if we did need them, rather than redefining them,
>  we would premerge the headers from open-fcoe.
Yes, I've got it. There is no need to have ELS/CT service request  
structure.
But, how about structures for response? I think we need to define them  
to check whether or not the completion successful so that we can set  
the reply structure accordingly.

> - We really should be making the request handling asynchronous in
>  nature, rather than following smp's simple synchronous structure.
OK, understood. It will be added it.

> - Timeout values should be part of the request, not hardcoded in the
>  transport. If there is one in the transport, it would be a default,
>  used when the app didn't specify it, and should be tunable by sysfs.
OK, timeout value tunable through sysfs attribute.

> - It would be nice, although difficult, to allow the user to abort
>  an outstanding service request. We should look into what sgio
>  does in this area.
Could you please elaborate a bit further?

> - We really should add support, for the initial merge, to talk to an
>  address that doesn't exist via an rport (think fabric service).
>  This means adding some request types to the fc_host too.
This one too, please.

>
>
>
> -- james s
>
>
> ========
>
> More detailed comments:
>
> scsi_transport_fc.c:
>
> - Do we really need a kmem_cache for service structures ?
>  Note: given the blocking nature in what you sent, there could only
>    be one request at a time being performed on the rport, so unless
>    you're talking about a lot of requests to different rports
>    simultaneously, why the cache ?  I'd argue even then, the number
>    requests is small, so it doesn't need a cache.
Ok. Will make changes.

> - fc_service_handler is done oddly. I know you copied smp's approach,
>  but we have no desire for the lld to override the transports handler.
>  We would rather want to have the transport siphone of the requests
>  it handles, and fall back to a driver handler.
Could you elaborate this a bit?

> - There's no reason for separate ct and els service handlers. It can
>  be consolidated into a single handler, and data in the service
>  structure or initial request structure.
Yes, make sense, will make changes.

> - Have you tested how large of a single buffer you can supply and get
>  by on the single sg vector ?  For nameserver payloads we should
>  support bigger buffers - 4k as a minimum.
OK. will make changes.

> qla2xxx implementation:
> - Why does the driver re-allocate buffers for the transmit and
>  response payloads, and re-dma map them. Why couldn't it use
>  the buffers/sg values from the initial request ?
>
> - How does the timeout path ever work ?  with the qla driver blocking
>  in it's request handlers - how does the transport wait_completion()
>  ever get invoked prior to a request completing ? And if that's true,
>  how did the abort request ever do anything ?
>
>
> Seokmann Ju wrote:
>> Hi,
>> After the RFC, here is the patch.
>> Again, the implementation has following remarks,
>> - the SMP support in the SAS transport has been referenced, so the
>> layout of the changes are quite similar to it.
>> - at this stage, the ELS/CT service packet serviced one at a time,
>> synchronously.
>> - in the scsi_transport_fc module, there is dedicated handler in the
>> LLDD for ELS and CT, respectively.
>> - device files are created in following directories for each of
>> discovered initiator/targets,
>>        = /sys/class/bsg/rport-<host_no>:<bus>-<id>
>>        = dev/rport-<host_no>:<bus>-<id>
>> - the qla2xxx module has been tested with the simple applikcation  
>> that
>> issues some of ELS and CT service requests.
>> - abort mechanism is in place.
>> This patch was made against scsi-misc.
>> Thank you,
>> Seokmann

Re: [PATCH 0/2] FC pass through support via bsg interface

[Sven Schuetz]

Hi Seokmann,

first of all, thanks for your efforts so far, good work!

Regarding the following statement from James and your question to it:

Seokmann Ju wrote:
> 
> On Sep 12, 2008, at 9:49 AM, James Smart wrote:
> 
>> Seokmann,
>>
>> There are number of fundamental issues with this patch that need to be 
>> corrected. I'll be at plumbers conference next week, and will be able 
>> to work through this. Please look me up if you will be there. 
>> Otherwise, I'll just post a revised patch.
>>
>> Here are the issues as I see them:
>>

<snip>

>> - We really should add support, for the initial merge, to talk to an
>>  address that doesn't exist via an rport (think fabric service).
>>  This means adding some request types to the fc_host too.
> This one too, please.
>>
>> -- james s
>>

<snip>


 From what I have seen, that's still missing from your latest submission. What 
James means by that (James, correct me when I am wrong) is that at the moment we 
only have devices for rports under /dev.
That's good for ELS requests, but for CT requests that are not directed to 
individual rports but to some well known address that's not appropriate. I think 
there might also be situations when there are no rports available and yet you 
still want to issue a CT request. In my opinion we would need devices for the 
fc_hosts under /dev as well. I have not looked at it in detail yet but it 
probably means to add a device struct to the fc_host_attrs struct similar as it 
is done for rports and register them with the block layer as well. Maybe it's 
feasible to have a dedicated ELS and CT handler in the transport layer instead 
of a generic service handler than (CT requets come in via /dev/fc_hostx and are 
handled by some fc_ct_service function and ELS requests come in via 
/dev/rportxxx and are handled by sme fc_els_service function), but that's 
something that needs to be looked at.
If you already have some ideas on how to do that, please let me know. I might 
start some experiments as well if I find the time.

Sven

Re: [PATCH 0/2] FC pass through support via bsg interface

[James Smart]

Sven Schuetz wrote:
> Hi Seokmann,
> 
> first of all, thanks for your efforts so far, good work!

I second this!

> Regarding the following statement from James and your question to it:
> 
> Seokmann Ju wrote:
>> On Sep 12, 2008, at 9:49 AM, James Smart wrote:
>>
>>> Seokmann,
>>>
>>> There are number of fundamental issues with this patch that need to be
>>> corrected. I'll be at plumbers conference next week, and will be able
>>> to work through this. Please look me up if you will be there.
>>> Otherwise, I'll just post a revised patch.
>>>
>>> Here are the issues as I see them:
>>>
> 
> <snip>
> 
>>> - We really should add support, for the initial merge, to talk to an
>>>  address that doesn't exist via an rport (think fabric service).
>>>  This means adding some request types to the fc_host too.
>> This one too, please.
>>> -- james s
>>>
> 
> <snip>
> 
> 
>  From what I have seen, that's still missing from your latest submission. What
> James means by that (James, correct me when I am wrong) is that at the moment we
> only have devices for rports under /dev.
> That's good for ELS requests, but for CT requests that are not directed to
> individual rports but to some well known address that's not appropriate. I think
> there might also be situations when there are no rports available and yet you
> still want to issue a CT request. In my opinion we would need devices for the
> fc_hosts under /dev as well. I have not looked at it in detail yet but it
> probably means to add a device struct to the fc_host_attrs struct similar as it
> is done for rports and register them with the block layer as well. Maybe it's
> feasible to have a dedicated ELS and CT handler in the transport layer instead
> of a generic service handler than (CT requets come in via /dev/fc_hostx and are
> handled by some fc_ct_service function and ELS requests come in via
> /dev/rportxxx and are handled by sme fc_els_service function), but that's
> something that needs to be looked at.
> If you already have some ideas on how to do that, please let me know. I might
> start some experiments as well if I find the time.
> 
> Sven

I've started this work, and am merging it with Seokman's patches.  The 
recent respins have reset it.  This work is also good for validating the 
  request/response formats, as it extends the requests to target 
transport handlers that do more than ELS/CT. So it's a good reference to 
ensure things are partitioned correctly.

Seokman:  I applied your last "revised III" patches - and the qla (2nd) 
patch failed miserably against scsi-misc-2.6. What were the patches cut 
against ?

-- james s

Re: [PATCH 0/2] FC pass through support via bsg interface

[Seokmann Ju]

On Nov 3, 2008, at 8:00 AM, James Smart wrote:

>
>
> Sven Schuetz wrote:
>> Hi Seokmann,
>> first of all, thanks for your efforts so far, good work!
>
> I second this!
>
>> Regarding the following statement from James and your question to it:
>> Seokmann Ju wrote:
>>> On Sep 12, 2008, at 9:49 AM, James Smart wrote:
>>>
>>>> Seokmann,
>>>>
>>>> There are number of fundamental issues with this patch that need  
>>>> to be
>>>> corrected. I'll be at plumbers conference next week, and will be  
>>>> able
>>>> to work through this. Please look me up if you will be there.
>>>> Otherwise, I'll just post a revised patch.
>>>>
>>>> Here are the issues as I see them:
>>>>
>> <snip>
>>>> - We really should add support, for the initial merge, to talk to  
>>>> an
>>>> address that doesn't exist via an rport (think fabric service).
>>>> This means adding some request types to the fc_host too.
>>> This one too, please.
>>>> -- james s
>>>>
>> <snip>
>> From what I have seen, that's still missing from your latest  
>> submission. What
>> James means by that (James, correct me when I am wrong) is that at  
>> the moment we
>> only have devices for rports under /dev.
>> That's good for ELS requests, but for CT requests that are not  
>> directed to
>> individual rports but to some well known address that's not  
>> appropriate. I think
>> there might also be situations when there are no rports available  
>> and yet you
>> still want to issue a CT request. In my opinion we would need  
>> devices for the
>> fc_hosts under /dev as well. I have not looked at it in detail yet  
>> but it
>> probably means to add a device struct to the fc_host_attrs struct  
>> similar as it
>> is done for rports and register them with the block layer as well.  
>> Maybe it's
>> feasible to have a dedicated ELS and CT handler in the transport  
>> layer instead
>> of a generic service handler than (CT requets come in via /dev/ 
>> fc_hostx and are
>> handled by some fc_ct_service function and ELS requests come in via
>> /dev/rportxxx and are handled by sme fc_els_service function), but  
>> that's
>> something that needs to be looked at.
>> If you already have some ideas on how to do that, please let me  
>> know. I might
>> start some experiments as well if I find the time.
>> Sven
>
> I've started this work, and am merging it with Seokman's patches.   
> The recent respins have reset it.  This work is also good for  
> validating the  request/response formats, as it extends the requests  
> to target transport handlers that do more than ELS/CT. So it's a  
> good reference to ensure things are partitioned correctly.
>
> Seokman:  I applied your last "revised III" patches - and the qla  
> (2nd) patch failed miserably against scsi-misc-2.6. What were the  
> patches cut against ?
The qla2xxx module patch has created against on top of following  
patch, which is in the queue.
http://marc.info/?t=122420587000004&r=1&w=2

Thank you,
Seokmann

Re: [PATCH 0/2] FC pass through support via bsg interface

[Sven Schuetz]

James Smart wrote:
> 
> 
> Sven Schuetz wrote:
>> Hi Seokmann,
>>
>> first of all, thanks for your efforts so far, good work!
> 
> I second this!
> 
>> Regarding the following statement from James and your question to it:
>>
>> Seokmann Ju wrote:
>>> On Sep 12, 2008, at 9:49 AM, James Smart wrote:
>>>
>>>> Seokmann,
>>>>
>>>> There are number of fundamental issues with this patch that need to be
>>>> corrected. I'll be at plumbers conference next week, and will be able
>>>> to work through this. Please look me up if you will be there.
>>>> Otherwise, I'll just post a revised patch.
>>>>
>>>> Here are the issues as I see them:
>>>>
>>
>> <snip>
>>
>>>> - We really should add support, for the initial merge, to talk to an
>>>>  address that doesn't exist via an rport (think fabric service).
>>>>  This means adding some request types to the fc_host too.
>>> This one too, please.
>>>> -- james s
>>>>
>>
>> <snip>
>>
>>
>>  From what I have seen, that's still missing from your latest
>> submission. What
>> James means by that (James, correct me when I am wrong) is that at the
>> moment we
>> only have devices for rports under /dev.
>> That's good for ELS requests, but for CT requests that are not
>> directed to
>> individual rports but to some well known address that's not
>> appropriate. I think
>> there might also be situations when there are no rports available and
>> yet you
>> still want to issue a CT request. In my opinion we would need devices
>> for the
>> fc_hosts under /dev as well. I have not looked at it in detail yet but it
>> probably means to add a device struct to the fc_host_attrs struct
>> similar as it
>> is done for rports and register them with the block layer as well.
>> Maybe it's
>> feasible to have a dedicated ELS and CT handler in the transport layer
>> instead
>> of a generic service handler than (CT requets come in via
>> /dev/fc_hostx and are
>> handled by some fc_ct_service function and ELS requests come in via
>> /dev/rportxxx and are handled by sme fc_els_service function), but that's
>> something that needs to be looked at.
>> If you already have some ideas on how to do that, please let me know.
>> I might
>> start some experiments as well if I find the time.
>>
>> Sven
> 
> I've started this work, and am merging it with Seokman's patches.  The
> recent respins have reset it.  This work is also good for validating the
>  request/response formats, as it extends the requests to target
> transport handlers that do more than ELS/CT. So it's a good reference to
> ensure things are partitioned correctly.
> 
> Seokman:  I applied your last "revised III" patches - and the qla (2nd)
> patch failed miserably against scsi-misc-2.6. What were the patches cut
> against ?
> 
> -- james s
> 

Hi James, Hi Seokmann,

I have been able to get CT and ELS requests working with our zfcp device driver
with your patch, great!
At the moment there are only two issues left:
- the one I described above (not having an adapter device for CT requests).
James, if you need any help or want me to test your patch, I'd happily do so.
- in our driver we need to distinguish the incoming CT requests so that we can
open the according WKA port and send the request to it. To do this at the
moment, I have to read the first few bytes of the CT frame from the
scatter-gather-list to see what generic service is targeted (byte 5 in the frame
specifies the service). That's bit awkward. What about extending the request
structures Seokmann defined in scsi_transport_fc.h in a manner like:

struct fc_service_request {
	u8   request_type;
	u8   timeout;
	u8   reserved0;
	u8   reserved1;
};

becomes:

struct fc_service_request {
	u8   request_type;
	u8   request_subtype;
	u8   timeout;
	u8   reserved1;
};

Then we could define the type of generic service (directory service,  alias
service etc) in the subtype field, without having to look at the
scatter-gather-list in the driver.

Sven

Re: [PATCH 0/2] FC pass through support via bsg interface

[Seokmann Ju]

On Nov 3, 2008, at 4:14 AM, Sven Schuetz wrote:

> Hi Seokmann,
>
> first of all, thanks for your efforts so far, good work!
>
> Regarding the following statement from James and your question to it:
>
> Seokmann Ju wrote:
>> On Sep 12, 2008, at 9:49 AM, James Smart wrote:
>>> Seokmann,
>>>
>>> There are number of fundamental issues with this patch that need  
>>> to be corrected. I'll be at plumbers conference next week, and  
>>> will be able to work through this. Please look me up if you will  
>>> be there. Otherwise, I'll just post a revised patch.
>>>
>>> Here are the issues as I see them:
>>>
>
> <snip>
>
>>> - We really should add support, for the initial merge, to talk to an
>>> address that doesn't exist via an rport (think fabric service).
>>> This means adding some request types to the fc_host too.
>> This one too, please.
>>>
>>> -- james s
>>>
>
> <snip>
>
>
> From what I have seen, that's still missing from your latest  
> submission. What James means by that (James, correct me when I am  
> wrong) is that at the moment we only have devices for rports under / 
> dev.
> That's good for ELS requests, but for CT requests that are not  
> directed to individual rports but to some well known address that's  
> not appropriate.
Yes, the feature has not added to the submission.
I've been in the middle of figuring out the right way to achieve it.

> I think there might also be situations when there are no rports  
> available and yet you still want to issue a CT request. In my  
> opinion we would need devices for the fc_hosts under /dev as well. I  
> have not looked at it in detail yet but it probably means to add a  
> device struct to the fc_host_attrs struct similar as it is done for  
> rports and register them with the block layer as well. Maybe it's  
> feasible to have a dedicated ELS and CT handler in the transport  
> layer instead of a generic service handler than (CT requets come in  
> via /dev/fc_hostx and are handled by some fc_ct_service function and  
> ELS requests come in via /dev/rportxxx and are handled by sme  
> fc_els_service function), but that's something that needs to be  
> looked at.
> If you already have some ideas on how to do that, please let me  
> know. I might start some experiments as well if I find the time.
I will get back to you as I get some more of feasible way of doing it.

Thank you,
Seokmann

Re: [PATCH 0/2] FC pass through support via bsg interface

[FUJITA Tomonori]

On Fri, 12 Sep 2008 12:49:55 -0400
James Smart <James.Smart@Emulex.Com> wrote:

> Seokmann,
> 
> There are number of fundamental issues with this patch that need to be 
> corrected. I'll be at plumbers conference next week, and will be able to 
> work through this. Please look me up if you will be there. Otherwise, 
> I'll just post a revised patch.
> 
> Here are the issues as I see them:
> 
> - There is no concept of a Request Structure, that supplies the type of
>    request and other parameters (such as a timeout). This should have
>    come from the cdb bytes.  Over time, we can expand this request
>    structure for other types of requests, not just els/ct passthru.
> 
> - There is no true definition of what the request payload should be.
>    It's implied that it should be a fully built frame header, followed by
>    payload.  We should be striking the frame header, and just stating
>    that for els/ct, the request payload is the tx sequence payload.
> 
> - There is no definition of what the response payload should be. Does
>    it contain a header too ? or what ? If the request is rejected,
>    could it also contain other frame types ? We should simply state
>    that it is the rx sequence payload, and only if successful.
> 
> - There is no concept or definition of a Reply structure, which should
>    be stuffed into the request sense area.  This should be well defined,
>    and matched to the request structure. We whould supply status values,
>    or response data from RJT's, BSY's, aborted status's, etc.

If fc needs to handle several types of requests, I think that it would
be make sense to define fc request/response structure. But the timeout
is in bsg's header so I'm not sure adding the timeout to fc request
structure.


> - Have you tested how large of a single buffer you can supply and get
>    by on the single sg vector ?  For nameserver payloads we should
>    support bigger buffers - 4k as a minimum.

bsg uses blk_rq_map_user to map the user pages, so you can handle much
bigger buffers than 4k.

But the above part in the current patch needs to be fixed:

+fc_service_handler(struct Scsi_Host *shost, struct fc_rport *rport,
+			  struct request *req)
+{

...

+	/* ELS doesn't support multiple SG */
+	if (req->bio->bi_vcnt > 1 || rsp->bio->bi_vcnt > 1) {
+		printk(KERN_WARNING "ERROR: multiple segments req %u,"
+		    " rsp %u  are not supported for ELS services\n",
+		    req->bio->bi_vcnt, rsp->bio->bi_vcnt);
+		return -EINVAL;
+	}
+
+	ret = issue_fc_service(rport, bio_data(req->bio), req->data_len,
+			bio_data(rsp->bio), rsp->data_len, req->sense);

This works for SMP that handles only small buffers but it doesn't work
for large buffers.

Re: [PATCH 0/2] FC pass through support via bsg interface

[Seokmann Ju]

On Sep 15, 2008, at 1:06 PM, FUJITA Tomonori wrote:

> On Fri, 12 Sep 2008 12:49:55 -0400
> James Smart <James.Smart@Emulex.Com> wrote:
>
>> Seokmann,
>>
>> There are number of fundamental issues with this patch that need to  
>> be
>> corrected. I'll be at plumbers conference next week, and will be  
>> able to
>> work through this. Please look me up if you will be there. Otherwise,
>> I'll just post a revised patch.
>>
>> Here are the issues as I see them:
>>
>> - There is no concept of a Request Structure, that supplies the  
>> type of
>>   request and other parameters (such as a timeout). This should have
>>   come from the cdb bytes.  Over time, we can expand this request
>>   structure for other types of requests, not just els/ct passthru.
>>
>> - There is no true definition of what the request payload should be.
>>   It's implied that it should be a fully built frame header,  
>> followed by
>>   payload.  We should be striking the frame header, and just stating
>>   that for els/ct, the request payload is the tx sequence payload.
>>
>> - There is no definition of what the response payload should be. Does
>>   it contain a header too ? or what ? If the request is rejected,
>>   could it also contain other frame types ? We should simply state
>>   that it is the rx sequence payload, and only if successful.
>>
>> - There is no concept or definition of a Reply structure, which  
>> should
>>   be stuffed into the request sense area.  This should be well  
>> defined,
>>   and matched to the request structure. We whould supply status  
>> values,
>>   or response data from RJT's, BSY's, aborted status's, etc.
>
> If fc needs to handle several types of requests, I think that it would
> be make sense to define fc request/response structure. But the timeout
> is in bsg's header so I'm not sure adding the timeout to fc request
> structure.
That is right. the timeout entry is available in the sg_io_v4 structure.

>> - Have you tested how large of a single buffer you can supply and get
>>   by on the single sg vector ?  For nameserver payloads we should
>>   support bigger buffers - 4k as a minimum.
>
> bsg uses blk_rq_map_user to map the user pages, so you can handle much
> bigger buffers than 4k.
OK. Thanks for the clarification.

> But the above part in the current patch needs to be fixed:
>
> +fc_service_handler(struct Scsi_Host *shost, struct fc_rport *rport,
> +			  struct request *req)
> +{
>
> ...
>
> +	/* ELS doesn't support multiple SG */
> +	if (req->bio->bi_vcnt > 1 || rsp->bio->bi_vcnt > 1) {
> +		printk(KERN_WARNING "ERROR: multiple segments req %u,"
> +		    " rsp %u  are not supported for ELS services\n",
> +		    req->bio->bi_vcnt, rsp->bio->bi_vcnt);
> +		return -EINVAL;
> +	}
> +
> +	ret = issue_fc_service(rport, bio_data(req->bio), req->data_len,
> +			bio_data(rsp->bio), rsp->data_len, req->sense);
>
> This works for SMP that handles only small buffers but it doesn't work
> for large buffers.
Yes, that is true for the CT services. For ELS services, the SG   
always has to be a single, as far as I understand.
Changes will be made accordingly.

Thank you,
Seokmann