scsi_debug: several bug fixes and enable clustering support
v2 cover scsi_debug: several bug fixes and enable clustering support
This patch set includes several bug fixes and patches for enabling clustering support for scsi_debug.
The reason why I would like to enable clustering is to test commands with huge transfer length. Without enabling clustering support, the transfer length for read and write scsi commands is limited upto 8MB when page size is 4KB and sg_tablesize is 2048
(= SCSI_MAX_SG_CHAIN_SEGMENTS).
Changes from v1:
- remove unnecessary irqsave/irqrestore while iterating sg mapping
- protect device access with atomic_rw lock
- make clustering module parameter writable
Akinobu Mita (7):
scsi_debug: fix false positive logical block reference tag check fail
scsi_debug: make pseudo_primary static
scsi_debug: fix duplicate dif_errors increment
scsi_debug: fix resp_xdwriteread() return value when running out of
memory
scsi_debug: prepare to enable clustering
scsi_debug: protect device access with atomic_rw lock
scsi_debug: add ability to enable clustering
drivers/scsi/scsi_debug.c | 141 +++++++++++++++++++++++++++------------------- 1 file changed, 82 insertions(+), 59 deletions(-)
Signature · 1 line
1.8.3.2
PATCH v2 1/7 scsi_debug: fix false positive logical block reference tag check fail
Reading partially unwritten sectors generates a false positive logical block reference tag check failure when DIF is enabled.
This bug is caused by missing ei_lba increment in loop of dif_verify() when unwritten sector is skipped.
---
No change from previous version
drivers/scsi/scsi_debug.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-)
drivers/scsi/scsi_debug.c +1 -3
@@ -1832,7 +1832,7 @@ static int prot_verify_read(struct scsi_cmnd *SCpnt, sector_t start_sec,struct sd_dif_tuple *sdt;sector_t sector; -for (i =0 ; i < sectors; i++) { +for (i =0 ; i < sectors; i++, ei_lba++) {int ret; sector = start_sec + i;
@@ -1846,8 +1846,6 @@ static int prot_verify_read(struct scsi_cmnd *SCpnt, sector_t start_sec, dif_errors++;return ret; } - - ei_lba++; }dif_copy_prot (SCpnt, start_sec, sectors, true);
Signature · 1 line
1.8.3.2
PATCH v2 2/7 scsi_debug: make pseudo_primary static
In reply to Akinobu MitaAs pseudo_primary is only used in scsi_debug.c, it should be static.
---
No change from previous version
drivers/scsi/scsi_debug.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/scsi/scsi_debug.c +1 -1
@@ -3246,7 +3246,7 @@ static struct attribute *sdebug_drv_attrs[] = { };ATTRIBUTE_GROUPS (sdebug_drv); -struct device *pseudo_primary; +static struct device *pseudo_primary;static int __init scsi_debug_init (void ) {
Signature · 1 line
1.8.3.2
PATCH v2 3/7 scsi_debug: fix duplicate dif_errors increment
In reply to Akinobu MitaIt is unnecessary to increase dif_errors in dif_verify(), because the caller will increment it when dif_verify() detects failure.
This bug was introduced by commit beb40ea42bd6 ("[SCSI] scsi_debug:
reduce duplication between prot_verify_read and prot_verify_write")
---
No change from previous version
drivers/scsi/scsi_debug.c | 1 - 1 file changed, 1 deletion(-)
drivers/scsi/scsi_debug.c +0 -1
@@ -1780,7 +1780,6 @@ static int dif_verify(struct sd_dif_tuple *sdt, const void *data, be32_to_cpu(sdt->ref_tag) != ei_lba) {pr_err ("%s: REF check failed on sector %lu\n" , __func__, (unsigned long )sector); - dif_errors++;return 0x03 ; }return 0 ;
Signature · 1 line
1.8.3.2
PATCH v2 4/7 scsi_debug: fix resp_xdwriteread() return value when running out of memory
In reply to Akinobu MitaWhen resp_xdwriteread() can't allocate temporary buffer, it returns -1. But the return value is used as scsi status code and -1 is not interpreted as correct code.
target_core_mod has similar xdwriteread emulation code. So this mimics what target_core_mod does for xdwriteread when running out of memory.
---
No change from previous version
drivers/scsi/scsi_debug.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
drivers/scsi/scsi_debug.c +6 -2
@@ -64,6 +64,7 @@ static const char * scsi_debug_version_date = "20100324";/* Additional Sense Code (ASC) */ #define NO_ADDITIONAL_SENSE 0x0#define LOGICAL_UNIT_NOT_READY 0x4 +#define LOGICAL_UNIT_COMMUNICATION_FAILURE 0x8#define UNRECOVERED_READ_ERR 0x11#define PARAMETER_LIST_LENGTH_ERR 0x1a#define INVALID_OPCODE 0x20
@@ -2318,8 +2319,11 @@ static int resp_xdwriteread(struct scsi_cmnd *scp, unsigned long long lba,/* better not to use temporary buffer. */ buf =kmalloc (scsi_bufflen (scp),GFP_ATOMIC ); -if (!buf) -return ret; +if (!buf) { +mk_sense_buffer (devip,NOT_READY , +LOGICAL_UNIT_COMMUNICATION_FAILURE ,0 ); +return check_condition_result; + }scsi_sg_copy_to_buffer (scp, buf,scsi_bufflen (scp));
Signature · 1 line
1.8.3.2
PATCH v2 5/7 scsi_debug: prepare to enable clustering
In reply to Akinobu MitaCurrently, clustering support for scsi_debug is disabled. This is because there are for_each_sg() loops which assume that each sg list element is consisted with a single page. But enabling clustering support, each sg list element for scsi commands can be consisted with multiple pages.
This replaces these for_each_sg() loops with sg mapping iterator which is capable of handling each sg list element is consisted with multiple pages.
---
Change from previous version:
- remove unnecessary irqsave/irqrestore while iterating sg mapping
drivers/scsi/scsi_debug.c | 109 +++++++++++++++++++++++++--------------------- 1 file changed, 59 insertions(+), 50 deletions(-)
drivers/scsi/scsi_debug.c +59 -50
@@ -1789,23 +1789,27 @@ static int dif_verify(struct sd_dif_tuple *sdt, const void *data,static void dif_copy_prot (struct scsi_cmnd *SCpnt,sector_t sector,unsigned int sectors,bool read) { -unsigned int i, resid; -struct scatterlist *psgl; +size_t resid;void *paddr;const void *dif_store_end = dif_storep + sdebug_store_sectors; +struct sg_mapping_iter miter;/* Bytes of protection data to copy into sgl */ resid = sectors *sizeof (*dif_storep); -scsi_for_each_prot_sg (SCpnt, psgl,scsi_prot_sg_count (SCpnt), i) { -int len =min (psgl->length, resid); +sg_miter_start (&miter,scsi_prot_sglist (SCpnt), +scsi_prot_sg_count (SCpnt),SG_MITER_ATOMIC | + (read ?SG_MITER_TO_SG :SG_MITER_FROM_SG )); + +while (sg_miter_next (&miter) && resid >0 ) { +size_t len =min (miter.length, resid);void *start =dif_store (sector); -int rest =0 ; +size_t rest =0 ;if (dif_store_end < start + len) rest = start + len - dif_store_end; - paddr =kmap_atomic (sg_page (psgl)) + psgl->offset; + paddr = miter.addr;if (read)memcpy (paddr, start, len - rest);
@@ -1821,8 +1825,8 @@ static void dif_copy_prot(struct scsi_cmnd *SCpnt, sector_t sector, sector += len /sizeof (*dif_storep); resid -= len; -kunmap_atomic (paddr); } +sg_miter_stop (&miter); } static int prot_verify_read(struct scsi_cmnd *SCpnt, sector_t start_sec,
@@ -1929,55 +1933,62 @@ void dump_sector(unsigned char *buf, int len)static int prot_verify_write (struct scsi_cmnd *SCpnt,sector_t start_sec,unsigned int sectors,u32 ei_lba) { -int i, j, ret; +int ret;struct sd_dif_tuple *sdt; -struct scatterlist *dsgl; -struct scatterlist *psgl =scsi_prot_sglist (SCpnt); -void *daddr, *paddr; +void *daddr;sector_t sector = start_sec;int ppage_offset; +int dpage_offset; +struct sg_mapping_iter diter; +struct sg_mapping_iter piter;BUG_ON (scsi_sg_count (SCpnt) ==0 );BUG_ON (scsi_prot_sg_count (SCpnt) ==0 ); - ppage_offset =0 ; - -/* For each data page */ -scsi_for_each_sg (SCpnt, dsgl,scsi_sg_count (SCpnt), i) { - daddr =kmap_atomic (sg_page (dsgl)) + dsgl->offset; - paddr =kmap_atomic (sg_page (psgl)) + psgl->offset; - -/* For each sector-sized chunk in data page */ -for (j =0 ; j < dsgl->length; j += scsi_debug_sector_size) { +sg_miter_start (&piter,scsi_prot_sglist (SCpnt), +scsi_prot_sg_count (SCpnt), +SG_MITER_ATOMIC |SG_MITER_FROM_SG ); +sg_miter_start (&diter,scsi_sglist (SCpnt),scsi_sg_count (SCpnt), +SG_MITER_ATOMIC |SG_MITER_FROM_SG ); + +/* For each protection page */ +while (sg_miter_next (&piter)) { + dpage_offset =0 ; +if (WARN_ON (!sg_miter_next (&diter))) { + ret =0x01 ; + goto out; + } +for (ppage_offset =0 ; ppage_offset < piter.length; + ppage_offset +=sizeof (struct sd_dif_tuple )) {/* If we're at the end of the current -* protection page advance to the next one +* data page advance to the next one */ -if (ppage_offset >= psgl->length) { -kunmap_atomic (paddr); - psgl =sg_next (psgl); -BUG_ON (psgl ==NULL ); - paddr =kmap_atomic (sg_page (psgl)) - + psgl->offset; - ppage_offset =0 ; +if (dpage_offset >= diter.length) { +if (WARN_ON (!sg_miter_next (&diter))) { + ret =0x01 ; + goto out; + } + dpage_offset =0 ; } - sdt = paddr + ppage_offset; + sdt = piter.addr + ppage_offset; + daddr = diter.addr + dpage_offset; - ret =dif_verify (sdt, daddr + j, sector, ei_lba); + ret =dif_verify (sdt, daddr, sector, ei_lba);if (ret) { -dump_sector (daddr + j, scsi_debug_sector_size); +dump_sector (daddr, scsi_debug_sector_size); goto out; } sector++; ei_lba++; - ppage_offset +=sizeof (struct sd_dif_tuple ); + dpage_offset += scsi_debug_sector_size; } - -kunmap_atomic (paddr); -kunmap_atomic (daddr); + diter.consumed = dpage_offset; +sg_miter_stop (&diter); } +sg_miter_stop (&piter);dif_copy_prot (SCpnt, start_sec, sectors, false); dix_writes++;
@@ -1986,8 +1997,8 @@ static int prot_verify_write(struct scsi_cmnd *SCpnt, sector_t start_sec, out: dif_errors++; -kunmap_atomic (paddr); -kunmap_atomic (daddr); +sg_miter_stop (&diter); +sg_miter_stop (&piter);return ret; }
@@ -2311,11 +2322,11 @@ static int resp_report_luns(struct scsi_cmnd * scp,static int resp_xdwriteread (struct scsi_cmnd *scp,unsigned long long lba,unsigned int num,struct sdebug_dev_info *devip) { -int i, j, ret =-1 ; +int j;unsigned char *kaddr, *buf;unsigned int offset; -struct scatterlist *sg;struct scsi_data_buffer *sdb =scsi_in (scp); +struct sg_mapping_iter miter;/* better not to use temporary buffer. */ buf =kmalloc (scsi_bufflen (scp),GFP_ATOMIC );
@@ -2328,22 +2339,20 @@ static int resp_xdwriteread(struct scsi_cmnd *scp, unsigned long long lba,scsi_sg_copy_to_buffer (scp, buf,scsi_bufflen (scp)); offset =0 ; -for_each_sg (sdb->table.sgl, sg, sdb->table.nents, i) { - kaddr = (unsigned char *)kmap_atomic (sg_page (sg)); -if (!kaddr) - goto out; +sg_miter_start (&miter, sdb->table.sgl, sdb->table.nents, +SG_MITER_ATOMIC |SG_MITER_TO_SG ); -for (j =0 ; j < sg->length; j++) - *(kaddr + sg->offset + j) ^= *(buf + offset + j); +while (sg_miter_next (&miter)) { + kaddr = miter.addr; +for (j =0 ; j < miter.length; j++) + *(kaddr + j) ^= *(buf + offset + j); - offset += sg->length; -kunmap_atomic (kaddr); + offset += miter.length; } - ret =0 ; -out: +sg_miter_stop (&miter);kfree (buf); -return ret; +return 0 ; }/* When timer goes off this function is called. */
Signature · 1 line
1.8.3.2
PATCH v2 6/7 scsi_debug: protect device access with atomic_rw lock
In reply to Akinobu MitaThis change ensures that concurrent device access including ramdisk storage, protection info, and provisioning map by read, write, and unmap commands are protected with atomic_rw spinlock.
---
New patch from this version
drivers/scsi/scsi_debug.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-)
drivers/scsi/scsi_debug.c +10 -2
@@ -1888,17 +1888,19 @@ static int resp_read(struct scsi_cmnd *SCpnt, unsigned long long lba,return check_condition_result; } +read_lock_irqsave (&atomic_rw, iflags); +/* DIX + T10 DIF */ if (scsi_debug_dix &&scsi_prot_sg_count (SCpnt)) {int prot_ret =prot_verify_read (SCpnt, lba, num, ei_lba);if (prot_ret) { +read_unlock_irqrestore (&atomic_rw, iflags);mk_sense_buffer (devip,ABORTED_COMMAND ,0x10 , prot_ret);return illegal_condition_result; } } -read_lock_irqsave (&atomic_rw, iflags); ret =do_device_access (SCpnt, devip, lba, num,0 );read_unlock_irqrestore (&atomic_rw, iflags);if (ret ==-1 )
@@ -2098,17 +2100,19 @@ static int resp_write(struct scsi_cmnd *SCpnt, unsigned long long lba,if (ret)return ret; +write_lock_irqsave (&atomic_rw, iflags); +/* DIX + T10 DIF */ if (scsi_debug_dix &&scsi_prot_sg_count (SCpnt)) {int prot_ret =prot_verify_write (SCpnt, lba, num, ei_lba);if (prot_ret) { +write_unlock_irqrestore (&atomic_rw, iflags);mk_sense_buffer (devip,ILLEGAL_REQUEST ,0x10 , prot_ret);return illegal_condition_result; } } -write_lock_irqsave (&atomic_rw, iflags); ret =do_device_access (SCpnt, devip, lba, num,1 );if (scsi_debug_lbp ())map_region (lba, num);
@@ -2187,6 +2191,7 @@ static int resp_unmap(struct scsi_cmnd * scmd, struct sdebug_dev_info * devip)struct unmap_block_desc *desc;unsigned int i, payload_len, descriptors;int ret; +unsigned long iflags; ret =check_readiness (scmd,1 , devip);if (ret)
@@ -2208,6 +2213,8 @@ static int resp_unmap(struct scsi_cmnd * scmd, struct sdebug_dev_info * devip) desc = (void *)&buf[8 ]; +write_lock_irqsave (&atomic_rw, iflags); +for (i =0 ; i < descriptors ; i++) {unsigned long long lba =get_unaligned_be64 (&desc[i].lba);unsigned int num =get_unaligned_be32 (&desc[i].blocks);
@@ -2222,6 +2229,7 @@ static int resp_unmap(struct scsi_cmnd * scmd, struct sdebug_dev_info * devip) ret =0 ; out: +write_unlock_irqrestore (&atomic_rw, iflags);kfree (buf);return ret;
Signature · 1 line
1.8.3.2
PATCH v2 7/7 scsi_debug: add ability to enable clustering
In reply to Akinobu MitaThis adds a module parameter to enable clustering.
Without enabling clustering support, the transfer length for read and write scsi commands is limited upto 8MB when page size is 4KB and sg_tablesize is 2048 (= SCSI_MAX_SG_CHAIN_SEGMENTS). I would like to test commands with more than that transfer length.
---
Change from previous version:
- make clustering module parameter writable
drivers/scsi/scsi_debug.c | 5 +++++ 1 file changed, 5 insertions(+)
drivers/scsi/scsi_debug.c +5 -0
@@ -196,6 +196,7 @@ static unsigned int scsi_debug_unmap_max_blocks = DEF_UNMAP_MAX_BLOCKS;static unsigned int scsi_debug_unmap_max_desc =DEF_UNMAP_MAX_DESC ;static unsigned int scsi_debug_write_same_length =DEF_WRITESAME_LENGTH ;static bool scsi_debug_removable =DEF_REMOVABLE ; +static bool scsi_debug_clustering;static int scsi_debug_cmnd_count =0 ;
@@ -2762,6 +2763,7 @@ static int schedule_resp(struct scsi_cmnd * cmnd,*/ module_param_named (add_host, scsi_debug_add_host, int,S_IRUGO |S_IWUSR );module_param_named (ato, scsi_debug_ato, int,S_IRUGO ); +module_param_named (clustering, scsi_debug_clustering, bool,S_IRUGO |S_IWUSR );module_param_named (delay, scsi_debug_delay, int,S_IRUGO |S_IWUSR );module_param_named (dev_size_mb, scsi_debug_dev_size_mb, int,S_IRUGO );module_param_named (dif, scsi_debug_dif, int,S_IRUGO );
@@ -2805,6 +2807,7 @@ MODULE_VERSION(SCSI_DEBUG_VERSION);MODULE_PARM_DESC (add_host,"0..127 hosts allowed(def=1)" );MODULE_PARM_DESC (ato,"application tag ownership: 0=disk 1=host (def=1)" ); +MODULE_PARM_DESC (clustering,"when set enables larger transfers (def=0)" );MODULE_PARM_DESC (delay,"# of jiffies to delay response(def=1)" );MODULE_PARM_DESC (dev_size_mb,"size in MB of ram shared by devs(def=8)" );MODULE_PARM_DESC (dif,"data integrity field type: 0-3 (def=0)" );
@@ -3952,6 +3955,8 @@ static int sdebug_driver_probe(struct device * dev) sdbg_host =to_sdebug_host (dev); sdebug_driver_template.can_queue = scsi_debug_max_queue; +if (scsi_debug_clustering) + sdebug_driver_template.use_clustering =ENABLE_CLUSTERING ; hpnt =scsi_host_alloc (&sdebug_driver_template,sizeof (sdbg_host));if (NULL == hpnt) {printk (KERN_ERR "%s: scsi_register failed\n" , __func__);
Signature · 1 line
1.8.3.2
Quoted 24 lines · Akinobu Mita
This patch set includes several bug fixes and patches for enabling clustering support for
scsi_debug.The reason why I would like to enable clustering is to test commands with huge transfer length. Without enabling clustering support, the transfer length for read and write scsi commands is limited upto 8MB when page size is 4KB and
sg_tablesizeis 2048
(=SCSI_MAX_SG_CHAIN_SEGMENTS).Changes from v1:
- remove unnecessary irqsave/irqrestore while iterating sg mapping
- protect device access withatomic_rwlock
- make clustering module parameter writableAkinobu Mita (7):
scsi_debug: fix false positive logical block reference tag check fail
scsi_debug: makepseudo_primarystatic
scsi_debug: fix duplicatedif_errorsincrement
scsi_debug: fixresp_xdwriteread()return value when running out of
memory
scsi_debug: prepare to enable clustering
scsi_debug: protect device access withatomic_rwlock
scsi_debug: add ability to enable clustering
Quoted 7 lines
drivers/scsi/scsi_debug.c | 141 +++++++++++++++++++++++++++------------------- 1 file changed, 82 insertions(+), 59 deletions(-)Cc "James E.J. Bottomley" <[email protected]>Cc Douglas Gilbert <[email protected]>Cc "Martin K. Petersen" <[email protected]>
The parts of this patch that I understand look fine. Perhaps Martin should "ack" it, if he sees fit.
Martin K. Petersen
Reviewed-byQuoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
"Doug" == Douglas Gilbert <[email protected]> writes:
Doug> The parts of this patch that I understand look fine. Perhaps Doug> Martin should "ack" it, if he sees fit.
Just had another look at the updated pieces. Patch series looks good to me.
Signature · 1 line
Martin K. Petersen Oracle Linux Engineering