From b59ce01eadad2199873d3c9bcbeb9bc7b41780e9 Mon Sep 17 00:00:00 2001 From: Vladislav Bolkhovitin Date: Fri, 1 May 2015 23:49:20 +0000 Subject: [PATCH 1/6] Decrease time cpu_cmd_counter is held taken for suspending case for debug mode to minimize race window described in http://sourceforge.net/p/scst/mailman/message/34074831/ git-svn-id: http://svn.code.sf.net/p/scst/svn/trunk@6193 d57e44dd-8a1f-0410-8b47-8ef2f437770f --- scst/src/scst_targ.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/scst/src/scst_targ.c b/scst/src/scst_targ.c index 90a85a6d3..6f97d960a 100644 --- a/scst/src/scst_targ.c +++ b/scst/src/scst_targ.c @@ -4461,8 +4461,8 @@ static int scst_translate_lun(struct scst_cmd *cmd) scst_put(cmd->cpu_cmd_counter); } } else { - TRACE_MGMT_DBG("%s", "FLAG SUSPENDED set, skipping"); scst_put(cmd->cpu_cmd_counter); + TRACE_MGMT_DBG("%s", "FLAG SUSPENDED set, skipping"); res = 1; } @@ -5044,8 +5044,8 @@ static int scst_get_mgmt(struct scst_mgmt_cmd *mcmd) if (unlikely(test_bit(SCST_FLAG_SUSPENDED, &scst_flags) && !test_bit(SCST_FLAG_SUSPENDING, &scst_flags))) { - TRACE_MGMT_DBG("%s", "FLAG SUSPENDED set, skipping"); scst_put(mcmd->cpu_cmd_counter); + TRACE_MGMT_DBG("%s", "FLAG SUSPENDED set, skipping"); res = 1; goto out; } From a3f8a4eff1dc7dc8c4cc48823c5bb293bc2e1c22 Mon Sep 17 00:00:00 2001 From: Vladislav Bolkhovitin Date: Fri, 1 May 2015 23:52:51 +0000 Subject: [PATCH 2/6] scst: Log a warning if the block size is modified A block size change is an important change so log such changes. Signed-off-by: Bart Van Assche git-svn-id: http://svn.code.sf.net/p/scst/svn/trunk@6194 d57e44dd-8a1f-0410-8b47-8ef2f437770f --- scst/src/dev_handlers/scst_cdrom.c | 15 ++++++++++----- scst/src/dev_handlers/scst_disk.c | 15 ++++++++++----- scst/src/dev_handlers/scst_modisk.c | 15 ++++++++++----- scst/src/dev_handlers/scst_user.c | 17 ++++++++++------- 4 files changed, 40 insertions(+), 22 deletions(-) diff --git a/scst/src/dev_handlers/scst_cdrom.c b/scst/src/dev_handlers/scst_cdrom.c index a7f7354f4..1e5826d77 100644 --- a/scst/src/dev_handlers/scst_cdrom.c +++ b/scst/src/dev_handlers/scst_cdrom.c @@ -177,15 +177,20 @@ out: static void cdrom_set_block_shift(struct scst_cmd *cmd, int block_shift) { struct scst_device *dev = cmd->dev; + int new_block_shift; + /* * No need for locks here, since *_detach() can not be * called, when there are existing commands. */ - if (block_shift != 0) - dev->block_shift = block_shift; - else - dev->block_shift = CDROM_DEF_BLOCK_SHIFT; - dev->block_size = 1 << dev->block_shift; + new_block_shift = block_shift ? : CDROM_DEF_BLOCK_SHIFT; + if (dev->block_shift != new_block_shift) { + PRINT_INFO("%s: Changed block shift from %d into %d / %d", + dev->virt_name, dev->block_shift, block_shift, + new_block_shift); + dev->block_shift = new_block_shift; + dev->block_size = 1 << dev->block_shift; + } return; } diff --git a/scst/src/dev_handlers/scst_disk.c b/scst/src/dev_handlers/scst_disk.c index 3b85ae15b..9487c005a 100644 --- a/scst/src/dev_handlers/scst_disk.c +++ b/scst/src/dev_handlers/scst_disk.c @@ -269,15 +269,20 @@ out: static void disk_set_block_shift(struct scst_cmd *cmd, int block_shift) { struct scst_device *dev = cmd->dev; + int new_block_shift; + /* * No need for locks here, since *_detach() can not be * called, when there are existing commands. */ - if (block_shift != 0) - dev->block_shift = block_shift; - else - dev->block_shift = DISK_DEF_BLOCK_SHIFT; - dev->block_size = 1 << dev->block_shift; + new_block_shift = block_shift ? : DISK_DEF_BLOCK_SHIFT; + if (dev->block_shift != new_block_shift) { + PRINT_INFO("%s: Changed block shift from %d into %d / %d", + dev->virt_name, dev->block_shift, block_shift, + new_block_shift); + dev->block_shift = new_block_shift; + dev->block_size = 1 << dev->block_shift; + } return; } diff --git a/scst/src/dev_handlers/scst_modisk.c b/scst/src/dev_handlers/scst_modisk.c index b6837727b..b3461b1a9 100644 --- a/scst/src/dev_handlers/scst_modisk.c +++ b/scst/src/dev_handlers/scst_modisk.c @@ -276,15 +276,20 @@ out: static void modisk_set_block_shift(struct scst_cmd *cmd, int block_shift) { struct scst_device *dev = cmd->dev; + int new_block_shift; + /* * No need for locks here, since *_detach() can not be * called, when there are existing commands. */ - if (block_shift != 0) - dev->block_shift = block_shift; - else - dev->block_shift = MODISK_DEF_BLOCK_SHIFT; - dev->block_size = 1 << dev->block_shift; + new_block_shift = block_shift ? : MODISK_DEF_BLOCK_SHIFT; + if (dev->block_shift != new_block_shift) { + PRINT_INFO("%s: Changed block shift from %d into %d / %d", + dev->virt_name, dev->block_shift, block_shift, + new_block_shift); + dev->block_shift = new_block_shift; + dev->block_size = 1 << dev->block_shift; + } return; } diff --git a/scst/src/dev_handlers/scst_user.c b/scst/src/dev_handlers/scst_user.c index 12ef87e71..0125f7ee6 100644 --- a/scst/src/dev_handlers/scst_user.c +++ b/scst/src/dev_handlers/scst_user.c @@ -1020,6 +1020,8 @@ out_reply: static void dev_user_set_block_shift(struct scst_cmd *cmd, int block_shift) { struct scst_device *dev = cmd->dev; + struct scst_user_dev *udev = cmd->dev->dh_priv; + int new_block_shift; TRACE_ENTRY(); @@ -1027,14 +1029,15 @@ static void dev_user_set_block_shift(struct scst_cmd *cmd, int block_shift) * No need for locks here, since *_detach() can not be * called, when there are existing commands. */ - TRACE_DBG("dev %p, new block shift %d", dev, block_shift); - if (block_shift != 0) - dev->block_shift = block_shift; - else { - struct scst_user_dev *udev = cmd->dev->dh_priv; - dev->block_shift = scst_calc_block_shift(udev->def_block_size); + new_block_shift = block_shift ? : + scst_calc_block_shift(udev->def_block_size); + if (dev->block_shift != new_block_shift) { + PRINT_INFO("%s: Changed block shift from %d into %d / %d", + dev->virt_name, dev->block_shift, block_shift, + new_block_shift); + dev->block_shift = new_block_shift; + dev->block_size = 1 << dev->block_shift; } - dev->block_size = 1 << dev->block_shift; TRACE_EXIT(); return; From 734bd675bd3a4c06dd786fcccd422c530f91a3cd Mon Sep 17 00:00:00 2001 From: Vladislav Bolkhovitin Date: Sat, 2 May 2015 00:11:22 +0000 Subject: [PATCH 3/6] Cleanup Since SAM_STAT_GOOD is 0, it does not change any functionality git-svn-id: http://svn.code.sf.net/p/scst/svn/trunk@6195 d57e44dd-8a1f-0410-8b47-8ef2f437770f --- scst/src/dev_handlers/scst_disk.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/scst/src/dev_handlers/scst_disk.c b/scst/src/dev_handlers/scst_disk.c index 9487c005a..9ef9e8682 100644 --- a/scst/src/dev_handlers/scst_disk.c +++ b/scst/src/dev_handlers/scst_disk.c @@ -360,11 +360,11 @@ static void disk_cmd_done(void *data, char *sense, int result, int resid) TRACE_DBG("work %p, cmd %p, left %d, result %d, sense %p, resid %d", work, work->cmd, work->left, result, sense, resid); + work->result = result; + if (result == SAM_STAT_GOOD) goto out_complete; - work->result = result; - disk_restore_sg(work); scst_pass_through_cmd_done(work->cmd, sense, result, resid + work->left); @@ -497,8 +497,7 @@ split: rc = scst_scsi_exec_async(cmd, &work, disk_cmd_done); if (unlikely(rc != 0)) { - PRINT_ERROR("scst_scsi_exec_async() failed: %d", - rc); + PRINT_ERROR("scst_scsi_exec_async() failed: %d", rc); goto out_err_restore; } From 80be46ae7befbedc07eab27eb628d9a7bdac16bd Mon Sep 17 00:00:00 2001 From: Vladislav Bolkhovitin Date: Sat, 2 May 2015 01:01:15 +0000 Subject: [PATCH 4/6] scst_targ: Avoid incorrect block size changes The scst_block_generic_dev_done() function parses the READ CAPACITY response without checking whether the response buffer has been initialized. This can lead to incorrect block size changes and also to the following kernel warning: WARNING: at scst/src/scst_lib.c:7353 scst_calc_block_shift+0x7f/0xb0 [scst]() 1 << 23 != 12595456 [] dump_stack+0x19/0x1b [] warn_slowpath_common+0x7d/0xc0 [] warn_slowpath_fmt+0x46/0x50 [] scst_calc_block_shift+0x7f/0xb0 [scst] [] scst_block_generic_dev_done.part.39+0x6a/0x7d [scst] [] scst_block_generic_dev_done+0x34/0x40 [scst] [] dev_user_disk_done+0x15/0x20 [scst_user] [] scst_dev_done+0x49/0x150 [scst] [] scst_process_active_cmd+0x240/0x390 [scst] [] scst_process_redirect_cmd+0x121/0x1e0 [scst] [] scst_cmd_done_local+0x76/0x120 [scst] [] dev_user_process_reply_exec+0x8a/0x370 [scst_user] [] dev_user_process_reply+0x242/0x2e0 [scst_user] [] dev_user_reply_get_cmd.isra.17+0x100/0x2d0 [scst_user] [] dev_user_ioctl+0x157/0x428 [scst_user] [] do_vfs_ioctl+0x7a/0x2e0 [] SyS_ioctl+0x91/0xb0 [] system_call_fastpath+0x16/0x1b Avoid this by additionally checking cmd->completed Reported-by: Gal Rosen Reported-by: Abacus Liang Reported-by: Shahar Salzman git-svn-id: http://svn.code.sf.net/p/scst/svn/trunk@6196 d57e44dd-8a1f-0410-8b47-8ef2f437770f --- scst/include/scst.h | 6 ++++++ scst/src/scst_lib.c | 5 ++--- scst/src/scst_targ.c | 2 +- 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/scst/include/scst.h b/scst/include/scst.h index e9e8c5dc9..b0cd1e388 100644 --- a/scst/include/scst.h +++ b/scst/include/scst.h @@ -3958,6 +3958,12 @@ static inline bool scst_cmd_atomic(struct scst_cmd *cmd) return res; } +/* Returns TRUE if cmd completed with SAM_STAT_GOOD */ +static inline bool scst_cmd_completed_good(struct scst_cmd *cmd) +{ + return cmd->completed && (cmd->status == SAM_STAT_GOOD); +} + /* * Returns TRUE if cmd has been preliminary completed, i.e. completed or * aborted. diff --git a/scst/src/scst_lib.c b/scst/src/scst_lib.c index 3f38d6d60..79e1a9588 100644 --- a/scst/src/scst_lib.c +++ b/scst/src/scst_lib.c @@ -10933,7 +10933,6 @@ int scst_block_generic_dev_done(struct scst_cmd *cmd, void (*set_block_shift)(struct scst_cmd *cmd, int block_shift)) { int opcode = cmd->cdb[0]; - int status = cmd->status; int res = SCST_CMD_STATE_DEFAULT; TRACE_ENTRY(); @@ -10945,7 +10944,7 @@ int scst_block_generic_dev_done(struct scst_cmd *cmd, */ if (unlikely(opcode == READ_CAPACITY)) { - if ((status == SAM_STAT_GOOD) || (status == SAM_STAT_CONDITION_MET)) { + if (scst_cmd_completed_good(cmd)) { /* Always keep track of disk capacity */ int buffer_size, sector_size, sh; uint8_t *buffer; @@ -11003,7 +11002,7 @@ int scst_tape_generic_dev_done(struct scst_cmd *cmd, * therefore change them only if necessary */ - if (unlikely(cmd->status != SAM_STAT_GOOD)) + if (unlikely(!scst_cmd_completed_good(cmd))) goto out; switch (opcode) { diff --git a/scst/src/scst_targ.c b/scst/src/scst_targ.c index 6f97d960a..9dd794190 100644 --- a/scst/src/scst_targ.c +++ b/scst/src/scst_targ.c @@ -3590,7 +3590,7 @@ next: goto out; } - if (likely(cmd->status == SAM_STAT_GOOD)) { + if (likely(scst_cmd_completed_good(cmd))) { if (cmd->deferred_dif_read_check) { int rc = scst_dif_process_read(cmd); if (unlikely(rc != 0)) { From 1ecb7f1fe399d0fc483cbfd5ced42573d812f762 Mon Sep 17 00:00:00 2001 From: Vladislav Bolkhovitin Date: Sat, 2 May 2015 01:08:25 +0000 Subject: [PATCH 5/6] qla2x00t: Fix source code indentation Signed-off-by: Bart Van Assche git-svn-id: http://svn.code.sf.net/p/scst/svn/trunk@6197 d57e44dd-8a1f-0410-8b47-8ef2f437770f --- qla2x00t/qla_os.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/qla2x00t/qla_os.c b/qla2x00t/qla_os.c index 6772523be..b9a07695d 100644 --- a/qla2x00t/qla_os.c +++ b/qla2x00t/qla_os.c @@ -3972,10 +3972,9 @@ void qla2x00_relogin(struct scsi_qla_host *vha) if (fcport->flags & FCF_FCP2_DEVICE) opts |= BIT_1; - status2 = - qla2x00_get_port_database( - vha, fcport, - opts); + status2 = + qla2x00_get_port_database( + vha, fcport, opts); if (status2 != QLA_SUCCESS) status = 1; } From c9de099c7400546d2d7078d06921470201f034f1 Mon Sep 17 00:00:00 2001 From: Vladislav Bolkhovitin Date: Sat, 2 May 2015 01:13:08 +0000 Subject: [PATCH 6/6] Cleanups git-svn-id: http://svn.code.sf.net/p/scst/svn/trunk@6198 d57e44dd-8a1f-0410-8b47-8ef2f437770f --- qla2x00t/qla2x00-target/qla2x00t.c | 3 +-- scst/src/dev_handlers/scst_vdisk.c | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/qla2x00t/qla2x00-target/qla2x00t.c b/qla2x00t/qla2x00-target/qla2x00t.c index 8966d3a2b..d047c5c7e 100644 --- a/qla2x00t/qla2x00-target/qla2x00t.c +++ b/qla2x00t/qla2x00-target/qla2x00t.c @@ -6588,8 +6588,7 @@ static ssize_t q2t_show_expl_conf_enabled(struct kobject *kobj, res = scnprintf(buffer, PAGE_SIZE, "%d\n%s", vha->hw->enable_explicit_conf, - vha->hw->enable_explicit_conf ? - SCST_SYSFS_KEY_MARK "\n" : ""); + vha->hw->enable_explicit_conf ? SCST_SYSFS_KEY_MARK "\n" : ""); out: return res; diff --git a/scst/src/dev_handlers/scst_vdisk.c b/scst/src/dev_handlers/scst_vdisk.c index a00e6be26..34508ada0 100644 --- a/scst/src/dev_handlers/scst_vdisk.c +++ b/scst/src/dev_handlers/scst_vdisk.c @@ -9470,8 +9470,7 @@ static ssize_t vdev_dif_filename_show(struct kobject *kobj, virt_dev = dev->dh_priv; pos = sprintf(buf, "%s\n%s", virt_dev->dif_filename, - (virt_dev->dif_filename != NULL) ? - SCST_SYSFS_KEY_MARK "\n" : ""); + (virt_dev->dif_filename != NULL) ? SCST_SYSFS_KEY_MARK "\n" : ""); TRACE_EXIT_RES(pos); return pos;