| Message ID | yq1mwd2h3ju.fsf@sermon.lab.mkp.net (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On 6/24/2014 3:53 PM, Martin K. Petersen wrote: >>>>>> "Mike" == Mike Christie <michaelc@cs.wisc.edu> 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_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. Sagi. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 you give me a real reviewed-by? > > Sagi. ---end quoted text--- -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 do not have a 1:1 mapping between the block range > they work on and the payload size (discard, write same). After the > scatterlist has been set up these requests use __data_len to store the > number of bytes to report completion on. This means that callers of > scsi_transfer_length() would get the wrong byte count for these types of > requests. > > To overcome this we make scsi_transfer_length() use the scatterlist > length in the scsi_data_buffer as basis for the wire transfer > calculation instead of __data_len. > > Reported-by: Christoph Hellwig <hch@infradead.org> > Debugged-by: Mike Christie <michaelc@cs.wisc.edu> > Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com> > > diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h > index 42ed789ebafc..e0ae71098144 100644 > --- a/include/scsi/scsi_cmnd.h > +++ b/include/scsi/scsi_cmnd.h > @@ -318,7 +318,7 @@ static inline void set_driver_byte(struct scsi_cmnd *cmd, char status) > > 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_get_prot_op(scmd); > unsigned int sector_size = scmd->device->sector_size; > Reviewed-by: Sagi Grimberg <sagig@mellanox.com> -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Jun 24, 2014, at 7:53 AM, Martin K. Petersen <martin.petersen@oracle.com> wrote: >>>>>> "Mike" == Mike Christie <michaelc@cs.wisc.edu> 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_length() > where the scatterlist length was used in the past. > > How about this? > > > 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 do not have a 1:1 mapping between the block range > they work on and the payload size (discard, write same). After the > scatterlist has been set up these requests use __data_len to store the > number of bytes to report completion on. This means that callers of > scsi_transfer_length() would get the wrong byte count for these types of > requests. > > To overcome this we make scsi_transfer_length() use the scatterlist > length in the scsi_data_buffer as basis for the wire transfer > calculation instead of __data_len. > > Reported-by: Christoph Hellwig <hch@infradead.org> > Debugged-by: Mike Christie <michaelc@cs.wisc.edu> > Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com> > > diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h > index 42ed789ebafc..e0ae71098144 100644 > --- a/include/scsi/scsi_cmnd.h > +++ b/include/scsi/scsi_cmnd.h > @@ -318,7 +318,7 @@ static inline void set_driver_byte(struct scsi_cmnd *cmd, char status) > > 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_get_prot_op(scmd); > unsigned int sector_size = scmd->device->sector_size; Do we need to check for the data direction. Something like if (scmd->sc_data_direction == DMA_TO_DEVICE) xfer_len = scsi_out(scmnd)->length; else xfer_len = scsi_in(scmnd)->length; -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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_get_prot_op(scmd); > > unsigned int sector_size = scmd->device->sector_size; > > > Do we need to check for the data direction. Something like > > if (scmd->sc_data_direction == DMA_TO_DEVICE) > xfer_len = scsi_out(scmnd)->length; > else > xfer_len = scsi_in(scmnd)->length; For non-bidi commands those are the same, but I suspect we'd need something like that for bidi commands. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
>>>>> "Mike" == Michael Christie <michaelc@cs.wisc.edu> 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 wrapper is called in. I think
iscsi is the only place where there's a distinction thanks to bidi.
Looks like there are several places where that's done. In that case I
wonder if we should have explicit scsi_in_transfer_length() and
scsi_out_transfer_length() wrappers?
On 06/24/2014 11:31 AM, Martin K. Petersen wrote: >>>>>> "Mike" == Michael Christie <michaelc@cs.wisc.edu> 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 wrapper is called in. I think > iscsi is the only place where there's a distinction thanks to bidi. > We were using it generically, so we did not check if bidi or t10 pi. We were calling it just expecting it to do the right thing for us. > Looks like there are several places where that's done. In that case I > wonder if we should have explicit scsi_in_transfer_length() and > scsi_out_transfer_length() wrappers? Yeah, maybe. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h index 42ed789ebafc..e0ae71098144 100644 --- a/include/scsi/scsi_cmnd.h +++ b/include/scsi/scsi_cmnd.h @@ -318,7 +318,7 @@ static inline void set_driver_byte(struct scsi_cmnd *cmd, char status) 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_get_prot_op(scmd); unsigned int sector_size = scmd->device->sector_size;