mpt2sas: For >2TB volumes, DirectDrive support sends IO's with LBA bit 31 to IR FW instead of DirectDrive
PATCH RESEND 6/10 mpt2sas: For >2TB volumes, DirectDrive support sends IO's with LBA bit 31 to IR FW instead of DirectDrive
There was a down casting of the volume max LBA from a U64 to a U32, which is taken out and now the max LBA is set appropriately to U64.
--- drivers/scsi/mpt2sas/mpt2sas_scsih.c | 14 ++++++++++---- 1 files changed, 10 insertions(+), 4 deletions(-)
drivers/scsi/mpt2sas/mpt2sas_scsih.c +10 -4
@@ -3865,7 +3865,8 @@ _scsih_setup_direct_io(struct MPT2SAS_ADAPTER *ioc, struct scsi_cmnd *scmd, struct _raid_device *raid_device, Mpi2SCSIIORequest_t *mpi_request, u16 smid) { - u32 v_lba, p_lba, stripe_off, stripe_unit, column, io_size; + u32 p_lba, stripe_off, stripe_unit, column, io_size; +u64 v_lba;u32 stripe_sz, stripe_exp;u8 num_pds, *cdb_ptr, i;u8 cdb0 = scmd->cmnd[0 ];
@@ -3882,12 +3883,17 @@ _scsih_setup_direct_io(struct MPT2SAS_ADAPTER *ioc, struct scsi_cmnd *scmd, | cdb_ptr[5])) { io_size =scsi_bufflen (scmd) >> raid_device->block_exponent; - i = (cdb0 <READ_16 ) ?2 :6 ; +/* get virtual lba */ - v_lba =be32_to_cpu (*(__be32 *)(&cdb_ptr[i])); +if (cdb0 <READ_16 ) + v_lba =be32_to_cpu (*(__be32 *)(&cdb_ptr[2 ])); +else + v_lba =be64_to_cpu (*(__be64 *)(&cdb_ptr[2 ])); + + i = (cdb0 <READ_16 ) ?2 :6 ;if (((u64 )v_lba + (u64 )io_size -1 ) <= - (u32 )raid_device->max_lba) { + raid_device->max_lba) { stripe_sz = raid_device->stripe_sz; stripe_exp = raid_device->stripe_exponent; stripe_off = v_lba & (stripe_sz -1 );
Signature · 1 line
1.7.1
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Sreekanth" == Reddy, Sreekanth <[email protected]> writes:
drivers/scsi/mpt2sas/mpt2sas_scsih.c +7 -3
@@ -3865,7 +3865,8 @@ _scsih_setup_direct_io(struct MPT2SAS_ADAPTER *ioc, struct scsi_cmnd *scmd, struct _raid_device *raid_device, Mpi2SCSIIORequest_t *mpi_request, u16 smid) { - u32 v_lba, p_lba, stripe_off, stripe_unit, column, io_size; + u32 p_lba, stripe_off, stripe_unit, column, io_size; +u64 v_lba;u32 stripe_sz, stripe_exp;u8 num_pds, *cdb_ptr, i;u8 cdb0 = scmd->cmnd[0 ];
@@ -3882,12 +3883,17 @@ _scsih_setup_direct_io(struct MPT2SAS_ADAPTER *ioc, struct scsi_cmnd *scmd, | cdb_ptr[5])) { io_size =scsi_bufflen (scmd) >> raid_device->block_exponent; - i = (cdb0 <READ_16 ) ?2 :6 ; +/* get virtual lba */ - v_lba =be32_to_cpu (*(__be32 *)(&cdb_ptr[i])); +if (cdb0 <READ_16 ) + v_lba =be32_to_cpu (*(__be32 *)(&cdb_ptr[2 ])); +else + v_lba =be64_to_cpu (*(__be64 *)(&cdb_ptr[2 ]));
Why aren't you using scsi_get_lba() instead of all this nasty CDB parsing?
+
+ i = (cdb0 < READ_16) ? 2 : 6;
What about WRITE_16? WRITE_16 > READ_16.
if (((u64)v_lba + (u64)io_size - 1) <=
- (u32)raid_device->max_lba) {
+ raid_device->max_lba) {
stripe_sz = raid_device->stripe_sz; stripe_exp = raid_device->stripe_exponent; stripe_off = v_lba & (stripe_sz - 1);
Also, this is not touched by the patch, but you're then doing:
(*(__be32 *)(&cdb_ptr[i])) = cpu_to_be32(p_lba);
What if this is a 6-byte READ/WRITE command? You'll end up exceeding the size of the LBA field.
What if you're using a 16-byte CDB and the target device LBA is > 2TB?
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering
Quoted 5 lines
-----Original Message-----
From: [email protected] [mailto:linux-scsi- [email protected]] On Behalf Of Martin K. Petersen
...
Quoted 6 lines
Also, this is not touched by the patch, but you're then doing:
(*(__be32 *)(&cdb_ptr[i])) = cpu_to_be32(p_lba);What if this is a 6-byte READ/WRITE command? You'll end up exceeding the size of the LBA field.
All this is inside:
if (cdb0 == READ_16 || cdb0 == READ_10 ||
cdb0 == WRITE_16 || cdb0 == WRITE_10) {so READ_6 and WRITE_6 and all their oddities are not a problem here.
A message that is not in the archive (sent off-list or not yet received): CAK=zhgosXeox35kVvuiTm8EZaD2-e_Lxz79uFFAP0c1cMJNbcg@mail.gmail.com
A message that is not in the archive (sent off-list or not yet received): CAK=zhgq6bXRBapfBy+Nx5uFFLUuj1cH78x0RXCU1Vk8Y9NWheA@mail.gmail.com
A message that is not in the archive (sent off-list or not yet received): [email protected]
A message that is not in the archive (sent off-list or not yet received): CAK=zhgrUWOS-pPjamF+z+AZZvdXz_JxyKgdTv6Zae0oLk=qQog@mail.gmail.com
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Sreekanth" == Sreekanth Reddy <[email protected]> writes:
Sreekanth> Also coming to first point i.e. the usage of scsi_get_lba() Sreekanth> API instead of parsing the CDB. During an experiment I Sreekanth> observe that when I use the 'sg_dd if=/dev/sg0 of=/dev/null Sreekanth> count=1' command for read operation then the output of this Sreekanth> API is all FFFF's i.e. 0xffffffff (i.e. LBA is 0xffffffff Sreekanth> instead of 0x00000000 for 32 bit LBA).
scsi_get_lba() returns a sector_t. I suspect it is all the type casting that is messing things up.
Quoted 2 lines
Quoted 2 lines
p_lbais 32-bit but READ(16)/WRITE(16) take an 8-byte LBA. Your patch addresses thev_lbabeing 64-bit but notp_lba.
I now see that you have two almost identical code paths to deal with p_lba.
How about something like this (untested):
drivers/scsi/mpt2sas/mpt2sas_scsih.c +35 -77
@@ -55,6 +55,8 @@#include <linux/raid_class.h> #include <linux/slab.h> +#include <asm/unaligned.h> +#include "mpt2sas_base.h" MODULE_AUTHOR (MPT2SAS_AUTHOR );
@@ -3857,85 +3859,41 @@ _scsih_setup_direct_io(struct MPT2SAS_ADAPTER *ioc, struct scsi_cmnd *scmd, struct _raid_device *raid_device, Mpi2SCSIIORequest_t *mpi_request, u16 smid) { - u32 v_lba, p_lba, stripe_off, stripe_unit, column, io_size; + sector_t v_lba, p_lba, stripe_off, stripe_unit, column, io_size;u32 stripe_sz, stripe_exp; -u8 num_pds, *cdb_ptr, i; -u8 cdb0 = scmd->cmnd[0 ]; -u64 v_llba; +u8 num_pds, cmd = scmd->cmnd[0 ]; -/* -* Try Direct I/O to RAID memeber disks -*/ -if (cdb0 ==READ_16 || cdb0 ==READ_10 || - cdb0 ==WRITE_16 || cdb0 ==WRITE_10 ) { - cdb_ptr = mpi_request->CDB.CDB32; - -if ((cdb0 <READ_16 ) || !(cdb_ptr[2 ] | cdb_ptr[3 ] | cdb_ptr[4 ] - | cdb_ptr[5 ])) { - io_size =scsi_bufflen (scmd) >> - raid_device->block_exponent; - i = (cdb0 <READ_16 ) ?2 :6 ; -/* get virtual lba */ - v_lba =be32_to_cpu (*(__be32 *)(&cdb_ptr[i])); - -if (((u64 )v_lba + (u64 )io_size -1 ) <= - (u32 )raid_device->max_lba) { - stripe_sz = raid_device->stripe_sz; - stripe_exp = raid_device->stripe_exponent; - stripe_off = v_lba & (stripe_sz -1 ); - -/* Check whether IO falls within a stripe */ -if ((stripe_off + io_size) <= stripe_sz) { - num_pds = raid_device->num_pds; - p_lba = v_lba >> stripe_exp; - stripe_unit = p_lba / num_pds; - column = p_lba % num_pds; - p_lba = (stripe_unit << stripe_exp) + - stripe_off; - mpi_request->DevHandle = -cpu_to_le16 (raid_device-> - pd_handle[column]); - (*(__be32 *)(&cdb_ptr[i])) = -cpu_to_be32 (p_lba); -/* -* WD: To indicate this I/O is directI/O -*/ -_scsih_scsi_direct_io_set (ioc, smid,1 ); - } - } - }else { - io_size =scsi_bufflen (scmd) >> - raid_device->block_exponent; -/* get virtual lba */ - v_llba =be64_to_cpu (*(__be64 *)(&cdb_ptr[2 ])); - -if ((v_llba + (u64 )io_size -1 ) <= - raid_device->max_lba) { - stripe_sz = raid_device->stripe_sz; - stripe_exp = raid_device->stripe_exponent; - stripe_off = (u32) (v_llba & (stripe_sz -1 )); - -/* Check whether IO falls within a stripe */ -if ((stripe_off + io_size) <= stripe_sz) { - num_pds = raid_device->num_pds; - p_lba = (u32)(v_llba >> stripe_exp); - stripe_unit = p_lba / num_pds; - column = p_lba % num_pds; - p_lba = (stripe_unit << stripe_exp) + - stripe_off; - mpi_request->DevHandle = -cpu_to_le16 (raid_device-> - pd_handle[column]); - (*(__be64 *)(&cdb_ptr[2 ])) = -cpu_to_be64 ((u64 )p_lba); -/* -* WD: To indicate this I/O is directI/O -*/ -_scsih_scsi_direct_io_set (ioc, smid,1 ); - } - } - } - } +if (cmd !=READ_10 && cmd !=WRITE_10 && + cmd !=READ_16 && cmd !=WRITE_16 ) +return ; + + v_lba =scsi_get_lba (scmd); + io_size =scsi_bufflen (scmd) >> raid_device->block_exponent; + +if (v_lba + io_size -1 > raid_device->max_lba) +return ; + + stripe_sz = raid_device->stripe_sz; + stripe_exp = raid_device->stripe_exponent; + stripe_off = v_lba & (stripe_sz -1 ); + +/* Return unless IO falls within a stripe */ +if (stripe_off + io_size > stripe_sz) +return ; + + num_pds = raid_device->num_pds; + p_lba = v_lba >> stripe_exp; + stripe_unit = p_lba / num_pds; + column = p_lba % num_pds; + p_lba = (stripe_unit << stripe_exp) + stripe_off; + mpi_request->DevHandle =cpu_to_le16 (raid_device->pd_handle[column]); + +if (cmd ==READ_10 || cmd ==WRITE_10 ) +put_unaligned_be32 (lower_32_bits (p_lba), mpi_request->CDB.CDB32); +else +put_unaligned_be64 (p_lba, mpi_request->CDB.CDB32); + +_scsih_scsi_direct_io_set (ioc, smid,1 ); }/**
A message that is not in the archive (sent off-list or not yet received): CAK=zhgrE0QBgyyLTU0Jdq2cAbKbd=wY4zfMGo2ux9nWz9oBgiQ@mail.gmail.com
A message that is not in the archive (sent off-list or not yet received): CAK=zhgpqcFu5WKLEOD9E=K9DURO9HEtXE8VDVpScLiZK+3JsAQ@mail.gmail.com
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Sreekanth" == Sreekanth Reddy <[email protected]> writes:
Sreekanth> I have tried with this diff. And I still observe the invalid Sreekanth> LBA (i.e. 0xFFFFFFFF) with scsi_get_lba() API when I used Sreekanth> the sg_dd with input file specified as /dev/sg0.
Yeah, I now remember that scsi_get_lba() fails for PC commands.
Sreekanth> Also below section of code will overwrite the CDB operation Sreekanth> code. and the final CDB has the some junk values.
Yep, already fixed that.
Please try the updated patch below.
drivers/scsi/mpt2sas/mpt2sas_scsih.c +41 -77
@@ -55,6 +55,8 @@#include <linux/raid_class.h> #include <linux/slab.h> +#include <asm/unaligned.h> +#include "mpt2sas_base.h" MODULE_AUTHOR (MPT2SAS_AUTHOR );
@@ -3857,85 +3859,47 @@ _scsih_setup_direct_io(struct MPT2SAS_ADAPTER *ioc, struct scsi_cmnd *scmd, struct _raid_device *raid_device, Mpi2SCSIIORequest_t *mpi_request, u16 smid) { - u32 v_lba, p_lba, stripe_off, stripe_unit, column, io_size; + sector_t v_lba, p_lba, stripe_off, stripe_unit, column, io_size;u32 stripe_sz, stripe_exp; -u8 num_pds, *cdb_ptr, i; -u8 cdb0 = scmd->cmnd[0 ]; -u64 v_llba; +u8 num_pds, cmd = scmd->cmnd[0 ]; -/* -* Try Direct I/O to RAID memeber disks -*/ -if (cdb0 ==READ_16 || cdb0 ==READ_10 || - cdb0 ==WRITE_16 || cdb0 ==WRITE_10 ) { - cdb_ptr = mpi_request->CDB.CDB32; - -if ((cdb0 <READ_16 ) || !(cdb_ptr[2 ] | cdb_ptr[3 ] | cdb_ptr[4 ] - | cdb_ptr[5 ])) { - io_size =scsi_bufflen (scmd) >> - raid_device->block_exponent; - i = (cdb0 <READ_16 ) ?2 :6 ; -/* get virtual lba */ - v_lba =be32_to_cpu (*(__be32 *)(&cdb_ptr[i])); - -if (((u64 )v_lba + (u64 )io_size -1 ) <= - (u32 )raid_device->max_lba) { - stripe_sz = raid_device->stripe_sz; - stripe_exp = raid_device->stripe_exponent; - stripe_off = v_lba & (stripe_sz -1 ); - -/* Check whether IO falls within a stripe */ -if ((stripe_off + io_size) <= stripe_sz) { - num_pds = raid_device->num_pds; - p_lba = v_lba >> stripe_exp; - stripe_unit = p_lba / num_pds; - column = p_lba % num_pds; - p_lba = (stripe_unit << stripe_exp) + - stripe_off; - mpi_request->DevHandle = -cpu_to_le16 (raid_device-> - pd_handle[column]); - (*(__be32 *)(&cdb_ptr[i])) = -cpu_to_be32 (p_lba); -/* -* WD: To indicate this I/O is directI/O -*/ -_scsih_scsi_direct_io_set (ioc, smid,1 ); - } - } - }else { - io_size =scsi_bufflen (scmd) >> - raid_device->block_exponent; -/* get virtual lba */ - v_llba =be64_to_cpu (*(__be64 *)(&cdb_ptr[2 ])); - -if ((v_llba + (u64 )io_size -1 ) <= - raid_device->max_lba) { - stripe_sz = raid_device->stripe_sz; - stripe_exp = raid_device->stripe_exponent; - stripe_off = (u32) (v_llba & (stripe_sz -1 )); - -/* Check whether IO falls within a stripe */ -if ((stripe_off + io_size) <= stripe_sz) { - num_pds = raid_device->num_pds; - p_lba = (u32)(v_llba >> stripe_exp); - stripe_unit = p_lba / num_pds; - column = p_lba % num_pds; - p_lba = (stripe_unit << stripe_exp) + - stripe_off; - mpi_request->DevHandle = -cpu_to_le16 (raid_device-> - pd_handle[column]); - (*(__be64 *)(&cdb_ptr[2 ])) = -cpu_to_be64 ((u64 )p_lba); -/* -* WD: To indicate this I/O is directI/O -*/ -_scsih_scsi_direct_io_set (ioc, smid,1 ); - } - } - } - } +if (cmd !=READ_10 && cmd !=WRITE_10 && + cmd !=READ_16 && cmd !=WRITE_16 ) +return ; + +if (cmd ==READ_10 || cmd ==WRITE_10 ) + v_lba =get_unaligned_be32 (&mpi_request->CDB.CDB32[2 ]); +else + v_lba =get_unaligned_be64 (&mpi_request->CDB.CDB32[2 ]); + + v_lba =scsi_get_lba (scmd); + io_size =scsi_bufflen (scmd) >> raid_device->block_exponent; + +if (v_lba + io_size -1 > raid_device->max_lba) +return ; + + stripe_sz = raid_device->stripe_sz; + stripe_exp = raid_device->stripe_exponent; + stripe_off = v_lba & (stripe_sz -1 ); + +/* Return unless IO falls within a stripe */ +if (stripe_off + io_size > stripe_sz) +return ; + + num_pds = raid_device->num_pds; + p_lba = v_lba >> stripe_exp; + stripe_unit = p_lba / num_pds; + column = p_lba % num_pds; + p_lba = (stripe_unit << stripe_exp) + stripe_off; + mpi_request->DevHandle =cpu_to_le16 (raid_device->pd_handle[column]); + +if (cmd ==READ_10 || cmd ==WRITE_10 ) +put_unaligned_be32 (lower_32_bits (p_lba), + &mpi_request->CDB.CDB32[2 ]); +else +put_unaligned_be64 (p_lba, &mpi_request->CDB.CDB32[2 ]); + +_scsih_scsi_direct_io_set (ioc, smid,1 ); }/**
A message that is not in the archive (sent off-list or not yet received): CAK=zhgoNm=STXjBOOqXHaz4yPuRHQ0avwgUOD_H0B==sgt6vEg@mail.gmail.com
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Sreekanth" == Sreekanth Reddy <[email protected]> writes:
Sreekanth> But still I observe the invalid LBA when we send IOs using Sreekanth> sg_dd with input file specified as /dev/sg0 as shown below
In the latest patch I get the LBA from CDB32 in the MPI request so that shouldn't happen.
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering