diff --git a/scst/README b/scst/README index 59094498c..7f1b3d827 100644 --- a/scst/README +++ b/scst/README @@ -676,15 +676,18 @@ debug2perf Makefile target. - Make sure that your target hardware (e.g. target FC or network card) and underlaying IO hardware (e.g. IO card, like SATA, SCSI or RAID to - which your disks connected) don't share the same PCI bus. They have - to work in parallel, so it will be better if they don't compete for - the bus. The problem is not only in the bandwidth, which they have to - share, but also in the interaction between cards during that - competition. This is very important, because in some cases if target - and backend storage controllers share the same PCI bus, it could lead - up to 5-10 times less performance, than expected. If you have no - choice, but PCI bus sharing, set in the BIOS PCI latency as low as - possible. + which your disks connected) don't share the same PCI bus. You can + check it using lspci utility. They have to work in parallel, so it + will be better if they don't compete for the bus. The problem is not + only in the bandwidth, which they have to share, but also in the + interaction between cards during that competition. This is very + important, because in some cases if target and backend storage + controllers share the same PCI bus, it could lead up to 5-10 times + less performance, than expected. Moreover, some motherboard (by + Supermicro, particularly) have serious stability issues if there are + several high speed devices on the same bus working in parallel. If + you have no choice, but PCI bus sharing, set in the BIOS PCI latency + as low as possible. IMPORTANT: If you use on initiator some versions of Windows (at least W2K) ========= you can't get good write performance for VDISK FILEIO devices with diff --git a/scst/include/scsi_tgt.h b/scst/include/scsi_tgt.h index cd32e7443..4f25a7e03 100644 --- a/scst/include/scsi_tgt.h +++ b/scst/include/scsi_tgt.h @@ -1355,8 +1355,8 @@ struct scst_device struct list_head dev_list_entry; /* - * List of tgt_dev's, one per session, protected by scst_mutex and - * suspended activity + * List of tgt_dev's, one per session, protected by scst_mutex or + * dev_lock for reads and both for writes */ struct list_head dev_tgt_dev_list; diff --git a/scst/src/scst_lib.c b/scst/src/scst_lib.c index 7bc260422..7a96da6e0 100644 --- a/scst/src/scst_lib.c +++ b/scst/src/scst_lib.c @@ -206,8 +206,7 @@ void scst_free_device(struct scst_device *dev) #ifdef EXTRACHECKS if (!list_empty(&dev->dev_tgt_dev_list) || - !list_empty(&dev->dev_acg_dev_list)) - { + !list_empty(&dev->dev_acg_dev_list)) { PRINT_ERROR("%s: dev_tgt_dev_list or dev_acg_dev_list " "is not empty!", __FUNCTION__); sBUG(); @@ -311,8 +310,7 @@ int scst_destroy_acg(struct scst_acg *acg) /* Freeing acg_devs */ list_for_each_entry_safe(acg_dev, acg_dev_tmp, &acg->acg_dev_list, - acg_dev_list_entry) - { + acg_dev_list_entry) { struct scst_tgt_dev *tgt_dev, *tt; list_for_each_entry_safe(tgt_dev, tt, &acg_dev->dev->dev_tgt_dev_list, @@ -325,8 +323,7 @@ int scst_destroy_acg(struct scst_acg *acg) /* Freeing names */ list_for_each_entry_safe(n, nn, &acg->acn_list, - acn_list_entry) - { + acn_list_entry) { list_del(&n->acn_list_entry); kfree(n->name); kfree(n); @@ -339,10 +336,7 @@ out: return res; } -/* - * No spin locks supposed to be held, scst_mutex - held. - * The activity is suspended. - */ +/* scst_mutex supposed to be held, there must not be parallel activity in this sess */ static struct scst_tgt_dev *scst_alloc_add_tgt_dev(struct scst_session *sess, struct scst_acg_dev *acg_dev) { @@ -479,10 +473,12 @@ static struct scst_tgt_dev *scst_alloc_add_tgt_dev(struct scst_session *sess, goto out_thr_free; } } - + + spin_lock_bh(&dev->dev_lock); list_add_tail(&tgt_dev->dev_tgt_dev_list_entry, &dev->dev_tgt_dev_list); if (dev->dev_reserved) __set_bit(SCST_TGT_DEV_RESERVED, &tgt_dev->tgt_dev_flags); + spin_unlock_bh(&dev->dev_lock); sess_tgt_dev_list_head = &sess->sess_tgt_dev_list_hash[HASH_VAL(tgt_dev->lun)]; @@ -508,10 +504,7 @@ out_free: static void scst_clear_reservation(struct scst_tgt_dev *tgt_dev); -/* - * No locks supposed to be held, scst_mutex - held. - * The activity is suspended. - */ +/* No locks supposed to be held, scst_mutex - held */ void scst_nexus_loss(struct scst_tgt_dev *tgt_dev) { TRACE_ENTRY(); @@ -533,10 +526,7 @@ void scst_nexus_loss(struct scst_tgt_dev *tgt_dev) return; } -/* - * No locks supposed to be held, scst_mutex - held. - * The activity is suspended. - */ +/* scst_mutex supposed to be held, there must not be parallel activity in this sess */ static void scst_free_tgt_dev(struct scst_tgt_dev *tgt_dev) { struct scst_device *dev = tgt_dev->dev; @@ -546,7 +536,10 @@ static void scst_free_tgt_dev(struct scst_tgt_dev *tgt_dev) tm_dbg_deinit_tgt_dev(tgt_dev); + spin_lock_bh(&dev->dev_lock); list_del(&tgt_dev->dev_tgt_dev_list_entry); + spin_unlock_bh(&dev->dev_lock); + list_del(&tgt_dev->sess_tgt_dev_list_entry); scst_clear_reservation(tgt_dev); @@ -572,7 +565,7 @@ static void scst_free_tgt_dev(struct scst_tgt_dev *tgt_dev) return; } -/* The activity supposed to be suspended and scst_mutex held */ +/* scst_mutex supposed to be held */ int scst_sess_alloc_tgt_devs(struct scst_session *sess) { int res = 0; @@ -582,8 +575,7 @@ int scst_sess_alloc_tgt_devs(struct scst_session *sess) TRACE_ENTRY(); list_for_each_entry(acg_dev, &sess->acg->acg_dev_list, - acg_dev_list_entry) - { + acg_dev_list_entry) { tgt_dev = scst_alloc_add_tgt_dev(sess, acg_dev); if (tgt_dev == NULL) { res = -ENOMEM; @@ -600,7 +592,7 @@ out_free: goto out; } -/* scst_mutex supposed to be held and activity suspended */ +/* scst_mutex supposed to be held, there must not be parallel activity in this sess */ void scst_sess_free_tgt_devs(struct scst_session *sess) { int i; @@ -1062,15 +1054,17 @@ out: } #endif /* LINUX_VERSION_CODE < KERNEL_VERSION(2,6,18) */ +/* scst_mutex supposed to be held */ static void scst_clear_reservation(struct scst_tgt_dev *tgt_dev) { struct scst_device *dev = tgt_dev->dev; + int release = 0; TRACE_ENTRY(); + spin_lock_bh(&dev->dev_lock); if (dev->dev_reserved && - !test_bit(SCST_TGT_DEV_RESERVED, &tgt_dev->tgt_dev_flags)) - { + !test_bit(SCST_TGT_DEV_RESERVED, &tgt_dev->tgt_dev_flags)) { /* This is one who holds the reservation */ struct scst_tgt_dev *tgt_dev_tmp; list_for_each_entry(tgt_dev_tmp, &dev->dev_tgt_dev_list, @@ -1079,9 +1073,11 @@ static void scst_clear_reservation(struct scst_tgt_dev *tgt_dev) &tgt_dev_tmp->tgt_dev_flags); } dev->dev_reserved = 0; - - scst_send_release(tgt_dev); } + spin_unlock_bh(&dev->dev_lock); + + if (release) + scst_send_release(tgt_dev); TRACE_EXIT(); return; @@ -1149,7 +1145,6 @@ void scst_free_session(struct scst_session *sess) { TRACE_ENTRY(); - scst_suspend_activity(); mutex_lock(&scst_mutex); TRACE_DBG("Removing sess %p from the list", sess); @@ -1162,7 +1157,6 @@ void scst_free_session(struct scst_session *sess) wake_up_all(&sess->tgt->unreg_waitQ); mutex_unlock(&scst_mutex); - scst_resume_activity(); kfree(sess->initiator_name); kmem_cache_free(scst_sess_cachep, sess); @@ -2332,7 +2326,6 @@ void scst_process_reset(struct scst_device *dev, /* Clear RESERVE'ation, if necessary */ if (dev->dev_reserved) { - /* Either scst_mutex held or exclude_cmd non-NULL */ list_for_each_entry(tgt_dev, &dev->dev_tgt_dev_list, dev_tgt_dev_list_entry) { TRACE(TRACE_MGMT, "Clearing RESERVE'ation for tgt_dev " @@ -2452,7 +2445,7 @@ out_unlock: goto out; } -/* Called under dev_lock, tgt_dev_lock and BH off */ +/* Called under tgt_dev_lock and BH off */ void scst_alloc_set_UA(struct scst_tgt_dev *tgt_dev, const uint8_t *sense, int sense_len, int head) { @@ -2515,7 +2508,7 @@ void scst_check_set_UA(struct scst_tgt_dev *tgt_dev, return; } -/* No locks, but the activity must not get suspended while inside this function */ +/* Called under dev_lock and BH off */ void scst_dev_check_set_local_UA(struct scst_device *dev, struct scst_cmd *exclude, const uint8_t *sense, int sense_len) { @@ -2676,11 +2669,6 @@ void scst_dev_del_all_thr_data(struct scst_device *dev) TRACE_ENTRY(); - /* - * This is read-only function for dev->dev_tgt_dev_list, so - * suspending the activity isn't necessary. - */ - mutex_lock(&scst_mutex); list_for_each_entry(tgt_dev, &dev->dev_tgt_dev_list, diff --git a/scst/src/scst_main.c b/scst/src/scst_main.c index 0008272d7..6248d8756 100644 --- a/scst/src/scst_main.c +++ b/scst/src/scst_main.c @@ -920,7 +920,7 @@ void scst_unregister_virtual_dev_driver(struct scst_dev_type *dev_type) return; } -/* Called under scst_mutex and suspended activity */ +/* Called under scst_mutex */ int scst_add_dev_threads(struct scst_device *dev, int num) { int i, res = 0; @@ -992,7 +992,7 @@ out: return res; } -/* Called under scst_mutex and suspended activity */ +/* Called under scst_mutex */ void scst_del_dev_threads(struct scst_device *dev, int num) { struct scst_cmd_thread_t *ct, *tmp; diff --git a/scst/src/scst_priv.h b/scst/src/scst_priv.h index 061a4f107..1af029654 100644 --- a/scst/src/scst_priv.h +++ b/scst/src/scst_priv.h @@ -366,6 +366,7 @@ static inline void scst_dev_check_set_UA(struct scst_device *dev, } void scst_dev_check_set_local_UA(struct scst_device *dev, struct scst_cmd *exclude, const uint8_t *sense, int sense_len); + void scst_check_set_UA(struct scst_tgt_dev *tgt_dev, const uint8_t *sense, int sense_len, int head); void scst_alloc_set_UA(struct scst_tgt_dev *tgt_dev, const uint8_t *sense, diff --git a/scst/src/scst_targ.c b/scst/src/scst_targ.c index bbdcb3820..ed3538c3e 100644 --- a/scst/src/scst_targ.c +++ b/scst/src/scst_targ.c @@ -1368,8 +1368,7 @@ static int scst_reserve_local(struct scst_cmd *cmd) } list_for_each_entry(tgt_dev_tmp, &dev->dev_tgt_dev_list, - dev_tgt_dev_list_entry) - { + dev_tgt_dev_list_entry) { if (cmd->tgt_dev != tgt_dev_tmp) set_bit(SCST_TGT_DEV_RESERVED, &tgt_dev_tmp->tgt_dev_flags); @@ -2140,17 +2139,22 @@ static int scst_done_cmd_check(struct scst_cmd *cmd, int *pres) if (!test_bit(SCST_TGT_DEV_RESERVED, &cmd->tgt_dev->tgt_dev_flags)) { struct scst_tgt_dev *tgt_dev_tmp; + struct scst_device *dev = cmd->dev; + TRACE(TRACE_SCSI, "Real RESERVE failed lun=%Ld, status=%x", (uint64_t)cmd->lun, cmd->status); TRACE_BUFF_FLAG(TRACE_SCSI, "Sense", cmd->sense_buffer, sizeof(cmd->sense_buffer)); + /* Clearing the reservation */ - list_for_each_entry(tgt_dev_tmp, &cmd->dev->dev_tgt_dev_list, + spin_lock_bh(&dev->dev_lock); + list_for_each_entry(tgt_dev_tmp, &dev->dev_tgt_dev_list, dev_tgt_dev_list_entry) { clear_bit(SCST_TGT_DEV_RESERVED, &tgt_dev_tmp->tgt_dev_flags); } - cmd->dev->dev_reserved = 0; + dev->dev_reserved = 0; + spin_unlock_bh(&dev->dev_lock); } } @@ -2203,7 +2207,8 @@ static int scst_mode_select_checks(struct scst_cmd *cmd) if (unlikely((cmd->cdb[0] == MODE_SELECT) || (cmd->cdb[0] == MODE_SELECT_10) || (cmd->cdb[0] == LOG_SELECT))) { - if (atomic && (cmd->dev->scsi_dev != NULL)) { + struct scst_device *dev = cmd->dev; + if (atomic && (dev->scsi_dev != NULL)) { TRACE_DBG("%s", "MODE/LOG SELECT: thread " "context required"); res = SCST_CMD_STATE_RES_NEED_THREAD; @@ -2214,7 +2219,8 @@ static int scst_mode_select_checks(struct scst_cmd *cmd) "setting the SELECT UA (lun=%Ld)", (uint64_t)cmd->lun); - spin_lock_bh(&scst_temp_UA_lock); + spin_lock_bh(&dev->dev_lock); + spin_lock(&scst_temp_UA_lock); if (cmd->cdb[0] == LOG_SELECT) { scst_set_sense(scst_temp_UA, sizeof(scst_temp_UA), @@ -2224,12 +2230,13 @@ static int scst_mode_select_checks(struct scst_cmd *cmd) sizeof(scst_temp_UA), UNIT_ATTENTION, 0x2a, 0x01); } - scst_dev_check_set_local_UA(cmd->dev, cmd, scst_temp_UA, + scst_dev_check_set_local_UA(dev, cmd, scst_temp_UA, sizeof(scst_temp_UA)); - spin_unlock_bh(&scst_temp_UA_lock); + spin_unlock(&scst_temp_UA_lock); + spin_unlock_bh(&dev->dev_lock); - if (cmd->dev->scsi_dev != NULL) - scst_obtain_device_parameters(cmd->dev); + if (dev->scsi_dev != NULL) + scst_obtain_device_parameters(dev); } } else if ((cmd->status == SAM_STAT_CHECK_CONDITION) && SCST_SENSE_VALID(cmd->sense_buffer) && @@ -3499,9 +3506,10 @@ static int scst_clear_task_set(struct scst_mgmt_cmd *mcmd) __scst_abort_task_set(mcmd, mcmd->mcmd_tgt_dev, 0, 0); + mutex_lock(&scst_mutex); + list_for_each_entry(tgt_dev, &dev->dev_tgt_dev_list, - dev_tgt_dev_list_entry) - { + dev_tgt_dev_list_entry) { struct scst_session *sess = tgt_dev->sess; struct scst_cmd *cmd; int aborted = 0; @@ -3528,6 +3536,8 @@ static int scst_clear_task_set(struct scst_mgmt_cmd *mcmd) &UA_tgt_devs); } + mutex_unlock(&scst_mutex); + scst_unblock_aborted_cmds(0); if (!dev->tas) { @@ -3686,8 +3696,7 @@ static int scst_target_reset(struct scst_mgmt_cmd *mcmd) cont = 0; c = 0; list_for_each_entry(tgt_dev, &dev->dev_tgt_dev_list, - dev_tgt_dev_list_entry) - { + dev_tgt_dev_list_entry) { cont = 1; rc = scst_call_dev_task_mgmt_fn(mcmd, tgt_dev, 0); if (rc == SCST_DEV_TM_NOT_COMPLETED) @@ -4378,7 +4387,6 @@ static int scst_init_session(struct scst_session *sess) TRACE_ENTRY(); - scst_suspend_activity(); mutex_lock(&scst_mutex); if (sess->initiator_name) @@ -4401,7 +4409,6 @@ static int scst_init_session(struct scst_session *sess) res = scst_sess_alloc_tgt_devs(sess); mutex_unlock(&scst_mutex); - scst_resume_activity(); if (sess->init_result_fn) { TRACE_DBG("Calling init_result_fn(%p)", sess);