From 1dc9a7627fe7be18615c5234e8bcbae66e3ad398 Mon Sep 17 00:00:00 2001 From: Shuhei Matsumoto Date: Tue, 15 Oct 2019 16:46:15 +0900 Subject: [PATCH] lib/scsi: Check pending tasks for the SCSI device only from the specific initiator Refine the public helper function spdk_scsi_dev_has_pending_tasks to be able to check tasks only from the specific initiator. Then use the function in iSCSI target to fix the issue. Besides add UT code to test the updated spdk_scsi_dev_has_pending_tasks(). Automated multi hosts test is much better but some UT code will be of any help to mitigate the risk of degradation. Fixes #985 Signed-off-by: Shuhei Matsumoto Change-Id: I50afb940de7174360c8a30479450850002a3e525 Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/471337 Tested-by: SPDK CI Jenkins Reviewed-by: Jim Harris Reviewed-by: Ben Walker Reviewed-by: Changpeng Liu --- include/spdk/scsi.h | 5 ++- lib/iscsi/conn.c | 6 ++-- lib/scsi/dev.c | 7 ++-- lib/vhost/vhost_scsi.c | 3 +- test/unit/lib/iscsi/conn.c/conn_ut.c | 3 +- test/unit/lib/scsi/dev.c/dev_ut.c | 52 ++++++++++++++++++++++++---- 6 files changed, 61 insertions(+), 15 deletions(-) diff --git a/include/spdk/scsi.h b/include/spdk/scsi.h index e3f9a0d20..1b23a083e 100644 --- a/include/spdk/scsi.h +++ b/include/spdk/scsi.h @@ -233,10 +233,13 @@ struct spdk_scsi_lun *spdk_scsi_dev_get_lun(struct spdk_scsi_dev *dev, int lun_i * Check whether the SCSI device has any pending task. * * \param dev SCSI device. + * \param initiator_port Check tasks only from the initiator if specified, or + * all all tasks otherwise. * * \return true if the SCSI device has any pending task, or false otherwise. */ -bool spdk_scsi_dev_has_pending_tasks(const struct spdk_scsi_dev *dev); +bool spdk_scsi_dev_has_pending_tasks(const struct spdk_scsi_dev *dev, + const struct spdk_scsi_port *initiator_port); /** * Destruct the SCSI decice. diff --git a/lib/iscsi/conn.c b/lib/iscsi/conn.c index 3339fe7df..8af08fce4 100644 --- a/lib/iscsi/conn.c +++ b/lib/iscsi/conn.c @@ -660,7 +660,8 @@ _iscsi_conn_check_pending_tasks(void *arg) { struct spdk_iscsi_conn *conn = arg; - if (conn->dev != NULL && spdk_scsi_dev_has_pending_tasks(conn->dev)) { + if (conn->dev != NULL && + spdk_scsi_dev_has_pending_tasks(conn->dev, conn->initiator_port)) { return 1; } @@ -685,7 +686,8 @@ spdk_iscsi_conn_destruct(struct spdk_iscsi_conn *conn) iscsi_conn_cleanup_backend(conn); } - if (conn->dev != NULL && spdk_scsi_dev_has_pending_tasks(conn->dev)) { + if (conn->dev != NULL && + spdk_scsi_dev_has_pending_tasks(conn->dev, conn->initiator_port)) { conn->shutdown_timer = spdk_poller_register(_iscsi_conn_check_pending_tasks, conn, 1000); } else { _iscsi_conn_destruct(conn); diff --git a/lib/scsi/dev.c b/lib/scsi/dev.c index 1ea722553..8857d7d1a 100644 --- a/lib/scsi/dev.c +++ b/lib/scsi/dev.c @@ -419,14 +419,15 @@ spdk_scsi_dev_get_lun(struct spdk_scsi_dev *dev, int lun_id) } bool -spdk_scsi_dev_has_pending_tasks(const struct spdk_scsi_dev *dev) +spdk_scsi_dev_has_pending_tasks(const struct spdk_scsi_dev *dev, + const struct spdk_scsi_port *initiator_port) { int i; for (i = 0; i < SPDK_SCSI_DEV_MAX_LUN; ++i) { if (dev->lun[i] && - (spdk_scsi_lun_has_pending_tasks(dev->lun[i], NULL) || - spdk_scsi_lun_has_pending_mgmt_tasks(dev->lun[i], NULL))) { + (spdk_scsi_lun_has_pending_tasks(dev->lun[i], initiator_port) || + spdk_scsi_lun_has_pending_mgmt_tasks(dev->lun[i], initiator_port))) { return true; } } diff --git a/lib/vhost/vhost_scsi.c b/lib/vhost/vhost_scsi.c index 155b284fa..2fa099974 100644 --- a/lib/vhost/vhost_scsi.c +++ b/lib/vhost/vhost_scsi.c @@ -250,7 +250,8 @@ process_removed_devs(struct spdk_vhost_scsi_session *svsession) state = &svsession->scsi_dev_state[i]; dev = state->dev; - if (dev && state->status == VHOST_SCSI_DEV_REMOVING && !spdk_scsi_dev_has_pending_tasks(dev)) { + if (dev && state->status == VHOST_SCSI_DEV_REMOVING && + !spdk_scsi_dev_has_pending_tasks(dev, NULL)) { /* detach the device from this session */ spdk_scsi_dev_free_io_channels(dev); state->dev = NULL; diff --git a/test/unit/lib/iscsi/conn.c/conn_ut.c b/test/unit/lib/iscsi/conn.c/conn_ut.c index fc0705371..0e26dea00 100644 --- a/test/unit/lib/iscsi/conn.c/conn_ut.c +++ b/test/unit/lib/iscsi/conn.c/conn_ut.c @@ -90,7 +90,8 @@ DEFINE_STUB(spdk_scsi_dev_get_lun, struct spdk_scsi_lun *, (struct spdk_scsi_dev *dev, int lun_id), NULL); DEFINE_STUB(spdk_scsi_dev_has_pending_tasks, bool, - (const struct spdk_scsi_dev *dev), true); + (const struct spdk_scsi_dev *dev, const struct spdk_scsi_port *initiator_port), + true); DEFINE_STUB(spdk_scsi_lun_open, int, (struct spdk_scsi_lun *lun, spdk_scsi_lun_remove_cb_t hotremove_cb, diff --git a/test/unit/lib/scsi/dev.c/dev_ut.c b/test/unit/lib/scsi/dev.c/dev_ut.c index 16e4b5434..922f4dd94 100644 --- a/test/unit/lib/scsi/dev.c/dev_ut.c +++ b/test/unit/lib/scsi/dev.c/dev_ut.c @@ -53,6 +53,9 @@ static struct spdk_bdev g_bdevs[] = { {"malloc1"}, }; +static struct spdk_scsi_port *g_initiator_port_with_pending_tasks = NULL; +static struct spdk_scsi_port *g_initiator_port_with_pending_mgmt_tasks = NULL; + const char * spdk_bdev_get_name(const struct spdk_bdev *bdev) { @@ -118,10 +121,6 @@ DEFINE_STUB_V(spdk_scsi_lun_append_mgmt_task, DEFINE_STUB_V(spdk_scsi_lun_execute_mgmt_task, (struct spdk_scsi_lun *lun)); -DEFINE_STUB(spdk_scsi_lun_has_pending_mgmt_tasks, bool, - (const struct spdk_scsi_lun *lun, const struct spdk_scsi_port *initiator_port), - false); - DEFINE_STUB_V(spdk_scsi_lun_append_task, (struct spdk_scsi_lun *lun, struct spdk_scsi_task *task)); @@ -132,9 +131,19 @@ DEFINE_STUB(_spdk_scsi_lun_allocate_io_channel, int, DEFINE_STUB_V(_spdk_scsi_lun_free_io_channel, (struct spdk_scsi_lun *lun)); -DEFINE_STUB(spdk_scsi_lun_has_pending_tasks, bool, - (const struct spdk_scsi_lun *lun, const struct spdk_scsi_port *initiator_port), - false); +bool +spdk_scsi_lun_has_pending_mgmt_tasks(const struct spdk_scsi_lun *lun, + const struct spdk_scsi_port *initiator_port) +{ + return (g_initiator_port_with_pending_mgmt_tasks == initiator_port); +} + +bool +spdk_scsi_lun_has_pending_tasks(const struct spdk_scsi_lun *lun, + const struct spdk_scsi_port *initiator_port) +{ + return (g_initiator_port_with_pending_tasks == initiator_port); +} static void dev_destruct_null_dev(void) @@ -603,6 +612,33 @@ dev_add_lun_success2(void) spdk_scsi_dev_destruct(&dev, NULL, NULL); } +static void +dev_check_pending_tasks(void) +{ + struct spdk_scsi_dev dev = {}; + struct spdk_scsi_lun lun = {}; + struct spdk_scsi_port initiator_port = {}; + + g_initiator_port_with_pending_tasks = NULL; + g_initiator_port_with_pending_mgmt_tasks = NULL; + + CU_ASSERT(spdk_scsi_dev_has_pending_tasks(&dev, NULL) == false); + + dev.lun[SPDK_SCSI_DEV_MAX_LUN - 1] = &lun; + + CU_ASSERT(spdk_scsi_dev_has_pending_tasks(&dev, NULL) == true); + CU_ASSERT(spdk_scsi_dev_has_pending_tasks(&dev, &initiator_port) == false); + + g_initiator_port_with_pending_tasks = &initiator_port; + CU_ASSERT(spdk_scsi_dev_has_pending_tasks(&dev, NULL) == true); + CU_ASSERT(spdk_scsi_dev_has_pending_tasks(&dev, &initiator_port) == true); + + g_initiator_port_with_pending_tasks = NULL; + g_initiator_port_with_pending_mgmt_tasks = &initiator_port; + CU_ASSERT(spdk_scsi_dev_has_pending_tasks(&dev, NULL) == true); + CU_ASSERT(spdk_scsi_dev_has_pending_tasks(&dev, &initiator_port) == true); +} + int main(int argc, char **argv) { @@ -666,6 +702,8 @@ main(int argc, char **argv) dev_add_lun_success1) == NULL || CU_add_test(suite, "dev add lun - success 2", dev_add_lun_success2) == NULL + || CU_add_test(suite, "dev check pending tasks", + dev_check_pending_tasks) == NULL ) { CU_cleanup_registry(); return CU_get_error();