From 1fc1068049bd363f450ba9e8199e5f3eacd5ed96 Mon Sep 17 00:00:00 2001 From: Dariusz Stojaczyk Date: Tue, 25 Jul 2017 19:26:55 +0200 Subject: [PATCH] vhost_scsi: vhost_scsi_dev_add_dev now takes vhost_dev param Continuation of patch 94afad5a [1]. That function could be called from outside of the vhost reactor, causing data races and possible segfaults. Now, after this patch, all ctrlr-changing functions take spdk_vhost_dev parameter, meaning they should be called via external event API [1], which soon will be the only way of obtaining spdk_vhost_dev pointer. [1] 94afad5a ("vhost: added external API to call spdk_events on vdev reactor") Change-Id: I40ea66ad09fb5c433dd897a4e22aedeb423f9b4b Signed-off-by: Dariusz Stojaczyk Reviewed-on: https://review.gerrithub.io/371013 Tested-by: SPDK Automated Test System Reviewed-by: Jim Harris Reviewed-by: Daniel Verkamp Reviewed-by: Changpeng Liu --- include/spdk/vhost.h | 3 ++- lib/vhost/vhost_rpc.c | 10 ++++++- lib/vhost/vhost_scsi.c | 24 ++++++++--------- .../lib/vhost/vhost_scsi.c/vhost_scsi_ut.c | 27 +++++++++---------- 4 files changed, 34 insertions(+), 30 deletions(-) diff --git a/include/spdk/vhost.h b/include/spdk/vhost.h index 9432aac70..74ca34b64 100644 --- a/include/spdk/vhost.h +++ b/include/spdk/vhost.h @@ -71,7 +71,8 @@ int spdk_vhost_scsi_controller_construct(void); int spdk_vhost_scsi_dev_construct(const char *name, const char *cpumask); int spdk_vhost_scsi_dev_remove(struct spdk_vhost_dev *vdev); struct spdk_scsi_dev *spdk_vhost_scsi_dev_get_dev(struct spdk_vhost_dev *ctrl, uint8_t num); -int spdk_vhost_scsi_dev_add_dev(const char *name, unsigned scsi_dev_num, const char *lun_name); +int spdk_vhost_scsi_dev_add_dev(struct spdk_vhost_dev *vdev, unsigned scsi_dev_num, + const char *lun_name); int spdk_vhost_scsi_dev_remove_dev(struct spdk_vhost_dev *vdev, unsigned scsi_dev_num); int spdk_vhost_blk_construct(const char *name, const char *cpumask, const char *dev_name, diff --git a/lib/vhost/vhost_rpc.c b/lib/vhost/vhost_rpc.c index f6d073947..38a9be339 100644 --- a/lib/vhost/vhost_rpc.c +++ b/lib/vhost/vhost_rpc.c @@ -186,6 +186,7 @@ spdk_rpc_add_vhost_scsi_lun(struct spdk_jsonrpc_request *request, { struct rpc_add_vhost_scsi_ctrlr_lun req = {0}; struct spdk_json_write_ctx *w; + struct spdk_vhost_dev *vdev; int rc; char buf[64]; @@ -197,7 +198,14 @@ spdk_rpc_add_vhost_scsi_lun(struct spdk_jsonrpc_request *request, goto invalid; } - rc = spdk_vhost_scsi_dev_add_dev(req.ctrlr, req.scsi_dev_num, req.lun_name); + vdev = spdk_vhost_dev_find(req.ctrlr); + if (vdev == NULL) { + SPDK_ERRLOG("Controller %s is not defined.\n", req.ctrlr); + rc = -ENODEV; + goto invalid; + } + + rc = spdk_vhost_scsi_dev_add_dev(vdev, req.scsi_dev_num, req.lun_name); if (rc < 0) { goto invalid; } diff --git a/lib/vhost/vhost_scsi.c b/lib/vhost/vhost_scsi.c index 1df6551e8..2f06c607b 100644 --- a/lib/vhost/vhost_scsi.c +++ b/lib/vhost/vhost_scsi.c @@ -801,17 +801,17 @@ spdk_vhost_scsi_lun_hotremove(const struct spdk_scsi_lun *lun, void *arg) } int -spdk_vhost_scsi_dev_add_dev(const char *ctrlr_name, unsigned scsi_dev_num, const char *lun_name) +spdk_vhost_scsi_dev_add_dev(struct spdk_vhost_dev *vdev, unsigned scsi_dev_num, + const char *lun_name) { struct spdk_vhost_scsi_dev *svdev; - struct spdk_vhost_dev *vdev; struct spdk_scsi_dev *scsi_dev; char dev_name[SPDK_SCSI_DEV_MAX_NAME]; int lun_id_list[1]; char *lun_names_list[1]; - if (ctrlr_name == NULL) { - SPDK_ERRLOG("No controller name\n"); + svdev = to_scsi_dev(vdev); + if (svdev == NULL) { return -EINVAL; } @@ -829,12 +829,6 @@ spdk_vhost_scsi_dev_add_dev(const char *ctrlr_name, unsigned scsi_dev_num, const return -1; } - vdev = spdk_vhost_dev_find(ctrlr_name); - if (vdev == NULL) { - SPDK_ERRLOG("Controller %s is not defined.\n", ctrlr_name); - return -ENODEV; - } - svdev = to_scsi_dev(vdev); if (svdev == NULL) { return -EINVAL; @@ -842,12 +836,12 @@ spdk_vhost_scsi_dev_add_dev(const char *ctrlr_name, unsigned scsi_dev_num, const if (vdev->lcore != -1 && !spdk_vhost_dev_has_feature(vdev, VIRTIO_SCSI_F_HOTPLUG)) { SPDK_ERRLOG("%s: 'Dev %u' is in use and hot-attach is not enabled for this controller\n", - ctrlr_name, scsi_dev_num); + vdev->name, scsi_dev_num); return -ENOTSUP; } if (svdev->scsi_dev[scsi_dev_num] != NULL) { - SPDK_ERRLOG("Controller %s dev %u already occupied\n", ctrlr_name, scsi_dev_num); + SPDK_ERRLOG("Controller %s dev %u already occupied\n", vdev->name, scsi_dev_num); return -EEXIST; } @@ -926,6 +920,7 @@ int spdk_vhost_scsi_controller_construct(void) { struct spdk_conf_section *sp = spdk_conf_first_section(NULL); + struct spdk_vhost_dev *vdev; int i, dev_num; unsigned ctrlr_num = 0; char *lun_name, *dev_num_str; @@ -951,6 +946,9 @@ spdk_vhost_scsi_controller_construct(void) return -1; } + vdev = spdk_vhost_dev_find(name); + assert(vdev); + for (i = 0; spdk_conf_section_get_nval(sp, "Dev", i) != NULL; i++) { dev_num_str = spdk_conf_section_get_nmval(sp, "Dev", i, 0); if (dev_num_str == NULL) { @@ -968,7 +966,7 @@ spdk_vhost_scsi_controller_construct(void) return -1; } - if (spdk_vhost_scsi_dev_add_dev(name, dev_num, lun_name) < 0) { + if (spdk_vhost_scsi_dev_add_dev(vdev, dev_num, lun_name) < 0) { return -1; } } diff --git a/test/unit/lib/vhost/vhost_scsi.c/vhost_scsi_ut.c b/test/unit/lib/vhost/vhost_scsi.c/vhost_scsi_ut.c index a669e81bd..61e50e038 100644 --- a/test/unit/lib/vhost/vhost_scsi.c/vhost_scsi_ut.c +++ b/test/unit/lib/vhost/vhost_scsi.c/vhost_scsi_ut.c @@ -305,52 +305,49 @@ vhost_scsi_dev_add_dev_test(void) int rc; char long_name[SPDK_SCSI_DEV_MAX_NAME + 1]; struct spdk_vhost_scsi_dev *svdev; + struct spdk_vhost_dev *vdev; struct spdk_scsi_dev *scsi_dev; /* Add device to controller without name */ rc = spdk_vhost_scsi_dev_add_dev(NULL, 0, "Malloc0"); CU_ASSERT(rc == -EINVAL); + svdev = alloc_svdev(); + vdev = &svdev->vdev; + MOCK_SET(spdk_vhost_dev_has_feature, bool, false); + /* Add device when max devices is reached */ - rc = spdk_vhost_scsi_dev_add_dev("vhost.0", + rc = spdk_vhost_scsi_dev_add_dev(vdev, SPDK_VHOST_SCSI_CTRLR_MAX_DEVS + 1, "Malloc0"); CU_ASSERT(rc == -EINVAL); /* Add device but lun has no name */ - rc = spdk_vhost_scsi_dev_add_dev("vhost.0", 0, NULL); + rc = spdk_vhost_scsi_dev_add_dev(vdev, 0, NULL); CU_ASSERT(rc == -EINVAL); /* Add device but lun has too long name */ memset(long_name, 'x', sizeof(long_name)); long_name[SPDK_SCSI_DEV_MAX_NAME] = 0; - rc = spdk_vhost_scsi_dev_add_dev("vhost.0", 0, long_name); + rc = spdk_vhost_scsi_dev_add_dev(vdev, 0, long_name); CU_ASSERT(rc != 0); - /* Add device to not defined controller */ - MOCK_SET_P(spdk_vhost_dev_find, struct spdk_vhost_dev *, NULL); - rc = spdk_vhost_scsi_dev_add_dev("vhost.0", 0, "Malloc0"); - CU_ASSERT(rc == -ENODEV); - /* Add device to a controller which is in use */ - svdev = alloc_svdev(); svdev->vdev.lcore = 0; - MOCK_SET_P(spdk_vhost_dev_find, struct spdk_vhost_dev *, &svdev->vdev); - MOCK_SET(spdk_vhost_dev_has_feature, bool, false); - rc = spdk_vhost_scsi_dev_add_dev("vhost.0", 0, "Malloc0"); + rc = spdk_vhost_scsi_dev_add_dev(vdev, 0, "Malloc0"); CU_ASSERT(rc == -ENOTSUP); /* Add device to controller with already occupied device */ - svdev->vdev.lcore = -1; + vdev->lcore = -1; scsi_dev = alloc_scsi_dev(); svdev->scsi_dev[0] = scsi_dev; - rc = spdk_vhost_scsi_dev_add_dev("vhost.0", 0, "Malloc0"); + rc = spdk_vhost_scsi_dev_add_dev(vdev, 0, "Malloc0"); CU_ASSERT(rc == -EEXIST); free(scsi_dev); svdev->scsi_dev[0] = NULL; /* Failed to create device */ MOCK_SET_P(spdk_scsi_dev_construct, struct spdk_scsi_dev *, NULL); - rc = spdk_vhost_scsi_dev_add_dev("vhost.0", 0, "Malloc0"); + rc = spdk_vhost_scsi_dev_add_dev(vdev, 0, "Malloc0"); CU_ASSERT(rc == -EINVAL); free(svdev);