Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

2014-08-06 Thread Sagi Grimberg
On 7/27/2014 12:11 PM, Boaz Harrosh wrote: On 06/25/2014 01:32 PM, Sagi Grimberg wrote: On 6/25/2014 11:48 AM, Sagi Grimberg wrote: I made the patch below which should fix both bidi support in iscsi and also WRITE_SAME (and similar commands) support. I'm a bit confused, for all commands (

Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

2014-07-27 Thread Christoph Hellwig
On Sun, Jul 27, 2014 at 12:11:11PM +0300, Boaz Harrosh wrote: > If you have a tree that you want me to test I will be glad too. > >From this thread I'm confused as to what patches you want me to > test? please point me to a tree you need testing. You can bug me > any time, any tree. I will be happy

Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

2014-07-27 Thread Boaz Harrosh
On 06/25/2014 01:32 PM, Sagi Grimberg wrote: > On 6/25/2014 11:48 AM, Sagi Grimberg wrote: > > >> >>> I made the patch below which should fix both bidi >>> support in iscsi and also WRITE_SAME (and similar commands) support. >> >> I'm a bit confused, for all commands (bidi or not) the iscsi heade

Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

2014-07-27 Thread Boaz Harrosh
On 06/25/2014 04:17 AM, Vladislav Bolkhovitin wrote: > Martin K. Petersen, on 06/23/2014 06:58 PM wrote: >>> "Mike" == Mike Christie writes: + unsigned int xfer_len = blk_rq_bytes(scmd->request); >> >> Mike> Can you do bidi and dif/dix? >> >> Nope. > > Correction: at the moment. > > The

Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

2014-07-13 Thread Martin K. Petersen
> "Christoph" == Christoph Hellwig writes: Christoph> Sagi, can you send an update for the core-for-3.17 tree to Christoph> pass a dma_direction or the scsi data buffer to Christoph> scsi_transfer_length so we can make safe for bidi usage in Christoph> the future? I have cleaned this up in m

Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

2014-07-13 Thread Christoph Hellwig
Sagi, can you send an update for the core-for-3.17 tree to pass a dma_direction or the scsi data buffer to scsi_transfer_length so we can make safe for bidi usage in the future? -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.ker

Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

2014-06-26 Thread Atchley, Scott
On Jun 26, 2014, at 12:38 PM, James Bottomley wrote: > On June 26, 2014 11:41:48 AM EDT, "Atchley, Scott" wrote: >> On Jun 26, 2014, at 10:55 AM, James Bottomley >> wrote: >> >>> On Thu, 2014-06-26 at 16:53 +0200, Bart Van Assche wrote: On 06/11/14 11:09, Sagi Grimberg wrote: > + ret

Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

2014-06-26 Thread James Bottomley
On June 26, 2014 11:41:48 AM EDT, "Atchley, Scott" wrote: >On Jun 26, 2014, at 10:55 AM, James Bottomley > wrote: > >> On Thu, 2014-06-26 at 16:53 +0200, Bart Van Assche wrote: >>> On 06/11/14 11:09, Sagi Grimberg wrote: + return xfer_len + (xfer_len >> ilog2(sector_size)) * 8; >>> >>> So

Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

2014-06-26 Thread Atchley, Scott
On Jun 26, 2014, at 10:55 AM, James Bottomley wrote: > On Thu, 2014-06-26 at 16:53 +0200, Bart Van Assche wrote: >> On 06/11/14 11:09, Sagi Grimberg wrote: >>> + return xfer_len + (xfer_len >> ilog2(sector_size)) * 8; >> >> Sorry that I just noticed this now, but why is a shift-right and ilog

Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

2014-06-26 Thread James Bottomley
On Thu, 2014-06-26 at 16:53 +0200, Bart Van Assche wrote: > On 06/11/14 11:09, Sagi Grimberg wrote: > > + return xfer_len + (xfer_len >> ilog2(sector_size)) * 8; > > Sorry that I just noticed this now, but why is a shift-right and ilog2() > used in the above expression instead of just dividing t

Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

2014-06-26 Thread Bart Van Assche
On 06/11/14 11:09, Sagi Grimberg wrote: > + return xfer_len + (xfer_len >> ilog2(sector_size)) * 8; Sorry that I just noticed this now, but why is a shift-right and ilog2() used in the above expression instead of just dividing the transfer length by the sector size ? Thanks, Bart. -- To uns

Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

2014-06-25 Thread Michael Christie
On Jun 25, 2014, at 6:35 AM, Christoph Hellwig wrote: > On Wed, Jun 25, 2014 at 01:32:39PM +0300, Sagi Grimberg wrote: >> So I tested a bidirectional command using: >> $ sg_raw --infile=/root/filex --send=1024 --request=1024 >> --outfile=/root/filex "/dev/bsg/7:0:0:0" 53 00 00 00 00 00 00 00 02

Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

2014-06-25 Thread Christoph Hellwig
On Wed, Jun 25, 2014 at 01:32:39PM +0300, Sagi Grimberg wrote: > So I tested a bidirectional command using: > $ sg_raw --infile=/root/filex --send=1024 --request=1024 > --outfile=/root/filex "/dev/bsg/7:0:0:0" 53 00 00 00 00 00 00 00 02 00 > > And I see: > kernel: session1: iscsi_prep_scsi_cmd_pdu

Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

2014-06-25 Thread Martin K. Petersen
> "Christoph" == Christoph Hellwig writes: Christoph> So here we use blk_rq_bytes still, which is incorrect for Christoph> WRITE SAME. Yeah, scsi_transfer_length() needs to go away completely if we go with the in and out variants. Christoph> I think the easiest fix is to just pass a scsi_da

Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

2014-06-25 Thread Sagi Grimberg
On 6/25/2014 11:48 AM, Sagi Grimberg wrote: I made the patch below which should fix both bidi support in iscsi and also WRITE_SAME (and similar commands) support. I'm a bit confused, for all commands (bidi or not) the iscsi header data_length should describe the scsi_data_buffer length, bi

Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

2014-06-25 Thread Christoph Hellwig
> >Sagi's patch was not correct because scsi_in was hardcoded to the transfer > >len when bidi was used. > > Right, should have condition that in the direction. something like: > > transfer_length = sc->sc_data_direction == DMA_TO_DEVICE ? > scsi_out(sc)->length : scsi_in(sc)->length; > > would

Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

2014-06-25 Thread Christoph Hellwig
Mike, I'd prefer a fix on top of the core-for-3.16 branch in my scsi-queue tree, which already has the fix from Martin. I also really don't like these three confusing helpers: > +static inline unsigned scsi_transfer_length(struct scsi_cmnd *scmd) > +{ > + return __scsi_calculate_transfer_leng

Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

2014-06-25 Thread Sagi Grimberg
On 6/25/2014 6:32 AM, Mike Christie wrote: On 06/24/2014 12:08 PM, Mike Christie wrote: On 06/24/2014 12:00 PM, Mike Christie wrote: On 06/24/2014 11:30 AM, Christoph Hellwig wrote: On Tue, Jun 24, 2014 at 07:27:46PM +0300, Sagi Grimberg wrote: This condition only matters in the bidi case, wh

Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

2014-06-24 Thread Mike Christie
On 06/24/2014 12:08 PM, Mike Christie wrote: > On 06/24/2014 12:00 PM, Mike Christie wrote: >> On 06/24/2014 11:30 AM, Christoph Hellwig wrote: >>> On Tue, Jun 24, 2014 at 07:27:46PM +0300, Sagi Grimberg wrote: This condition only matters in the bidi case, which is not relevant for the P

Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

2014-06-24 Thread Vladislav Bolkhovitin
Martin K. Petersen, on 06/23/2014 06:58 PM wrote: "Mike" == Mike Christie writes: + unsigned int xfer_len = blk_rq_bytes(scmd->request); Mike> Can you do bidi and dif/dix? Nope. Correction: at the moment. There is a proposal of READ GATHERED command, which is bidirectional and potentially

Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

2014-06-24 Thread Mike Christie
On 06/24/2014 12:00 PM, Mike Christie wrote: > On 06/24/2014 11:30 AM, Christoph Hellwig wrote: >> On Tue, Jun 24, 2014 at 07:27:46PM +0300, Sagi Grimberg wrote: >>> This condition only matters in the bidi case, which is not relevant for the >>> PI case. >>> I suggested to condition that in libiscs

Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

2014-06-24 Thread Mike Christie
On 06/24/2014 11:31 AM, Martin K. Petersen wrote: >> "Mike" == Michael Christie writes: > > Mike> Do we need to check for the data direction. Something like > > Mike> if (scmd->sc_data_direction == DMA_TO_DEVICE) > Mike> xfer_len = scsi_out(scmnd)->length; > Mike> else > Mike>

Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

2014-06-24 Thread Martin K. Petersen
> "Mike" == Mike Christie writes: Mike> It would be nice to just have one function to call and it just do Mike> the right thing for the drivers. But what is the right thing when there are buffers for both directions? -- Martin K. Petersen Oracle Linux Engineering -- To unsubscribe fro

Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

2014-06-24 Thread Mike Christie
On 06/24/2014 11:30 AM, Christoph Hellwig wrote: > On Tue, Jun 24, 2014 at 07:27:46PM +0300, Sagi Grimberg wrote: >> This condition only matters in the bidi case, which is not relevant for the >> PI case. >> I suggested to condition that in libiscsi (posted in the second thread, >> copy-paste below

Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

2014-06-24 Thread Martin K. Petersen
> "Mike" == Michael Christie writes: Mike> Do we need to check for the data direction. Something like Mike> if (scmd->sc_data_direction == DMA_TO_DEVICE) Mike> xfer_len = scsi_out(scmnd)->length; Mike> else Mike> xfer_len = scsi_in(scmnd)->length; I guess that depends on the context the

Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

2014-06-24 Thread Christoph Hellwig
On Tue, Jun 24, 2014 at 07:27:46PM +0300, Sagi Grimberg wrote: > This condition only matters in the bidi case, which is not relevant for the > PI case. > I suggested to condition that in libiscsi (posted in the second thread, > copy-paste below). > Although I do agree that scsi_transfer_length() he

Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

2014-06-24 Thread Sagi Grimberg
On 6/24/2014 7:08 PM, Michael Christie wrote: On Jun 24, 2014, at 7:53 AM, Martin K. Petersen wrote: "Mike" == Mike Christie writes: Mike> The problem is WRITE_SAME requests are setup so that Mike> req->__data_len is the value of the entire request when the setup Mike> is completed but duri

Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

2014-06-24 Thread Christoph Hellwig
On Tue, Jun 24, 2014 at 11:08:54AM -0500, Michael Christie wrote: > > static inline unsigned scsi_transfer_length(struct scsi_cmnd *scmd) > > { > > - unsigned int xfer_len = blk_rq_bytes(scmd->request); > > + unsigned int xfer_len = scsi_out(scmd)->length; > > unsigned int prot_op = scsi_ge

Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

2014-06-24 Thread Michael Christie
On Jun 24, 2014, at 7:53 AM, Martin K. Petersen wrote: >> "Mike" == Mike Christie writes: > > Mike> The problem is WRITE_SAME requests are setup so that > Mike> req->__data_len is the value of the entire request when the setup > Mike> is completed but during the setup process it's value c

Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

2014-06-24 Thread sagi grimberg
On 6/24/2014 3:53 PM, Martin K. Petersen wrote: SCSI: Use SCSI data buffer length to extract transfer size Commit 8846bab180fa introduced a helper that can be used to query the wire transfer size for a SCSI command taking protection information into account. However, some commands

Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

2014-06-24 Thread Christoph Hellwig
On Tue, Jun 24, 2014 at 04:08:25PM +0300, Sagi Grimberg wrote: > >Oh, I see. So things break because iSCSI uses scsi_transfer_length() > >where the scatterlist length was used in the past. > > Ohhh, didn't notice this one and sent out a duplicate... > > The patch looks good to me obviously. Can

Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

2014-06-24 Thread Christoph Hellwig
On Tue, Jun 24, 2014 at 08:53:25AM -0400, Martin K. Petersen wrote: > Oh, I see. So things break because iSCSI uses scsi_transfer_length() > where the scatterlist length was used in the past. > > How about this? This fixes the regression for me. -- To unsubscribe from this list: send the line "u

Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

2014-06-24 Thread Sagi Grimberg
On 6/24/2014 3:53 PM, Martin K. Petersen wrote: "Mike" == Mike Christie writes: Mike> The problem is WRITE_SAME requests are setup so that Mike> req->__data_len is the value of the entire request when the setup Mike> is completed but during the setup process it's value changes Oh, I see. So th

Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

2014-06-24 Thread sagi grimberg
On 6/24/2014 9:54 AM, Mike Christie wrote: On 06/11/2014 04:09 AM, Sagi Grimberg wrote: In case protection information exists on the wire scsi transports should include it in the transfer byte count (even if protection information does not exist in the host memory space). This helper will comput

Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

2014-06-24 Thread Martin K. Petersen
> "Mike" == Mike Christie writes: Mike> The problem is WRITE_SAME requests are setup so that Mike> req->__data_len is the value of the entire request when the setup Mike> is completed but during the setup process it's value changes Oh, I see. So things break because iSCSI uses scsi_transfer_

Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

2014-06-23 Thread Mike Christie
On 06/11/2014 04:09 AM, Sagi Grimberg wrote: > In case protection information exists on the wire > scsi transports should include it in the transfer > byte count (even if protection information does not > exist in the host memory space). This helper will > compute the total transfer length from the

Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

2014-06-23 Thread Martin K. Petersen
> "Mike" == Mike Christie writes: >> + unsigned int xfer_len = blk_rq_bytes(scmd->request); Mike> Can you do bidi and dif/dix? Nope. -- Martin K. Petersen Oracle Linux Engineering -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to maj

Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

2014-06-23 Thread Mike Christie
On 06/11/2014 04:09 AM, Sagi Grimberg wrote: > In case protection information exists on the wire > scsi transports should include it in the transfer > byte count (even if protection information does not > exist in the host memory space). This helper will > compute the total transfer length from the

Re: [PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

2014-06-11 Thread Martin K. Petersen
> "Sagi" == Sagi Grimberg writes: Sagi> In case protection information exists on the wire scsi transports Sagi> should include it in the transfer byte count (even if protection Sagi> information does not exist in the host memory space). This helper Sagi> will compute the total transfer length

[PATCH v2 1/3] scsi_cmnd: Introduce scsi_transfer_length helper

2014-06-11 Thread Sagi Grimberg
In case protection information exists on the wire scsi transports should include it in the transfer byte count (even if protection information does not exist in the host memory space). This helper will compute the total transfer length from the scsi command data length and protection attributes. S