From c049304a957377442731c31f617c1abf79c4a406 Mon Sep 17 00:00:00 2001 From: Darek Stojaczyk Date: Mon, 2 Sep 2019 11:35:33 +0200 Subject: [PATCH] env: add spdk_pci_device_unclaim() spdk_pci_device_claim() could create a file on the filesystem that couldn't be deleted programatically. It could only be overwritten - e.g. by another spdk instance - but this didn't really work if that another instance had less privileges and hence no access to the previous file. This is exactly the case we're seeing on our CI when running SPDK as non-root. In general it's a good idea not to leave any leftover files, so now we'll delete the pci claim file when the spdk process exits. spdk_pci_device_claim() used to return a file descriptor that could be simply closed to "un-claim" the device. It'll now return only a return code. The fd will be stored inside spdk_pci_device and will be closed either when user calls the newly introduced spdk_pci_device_unclaim(), or when the device is detached. We'll still need to clean up those files somewhere in our test scripts (probably ./setup.sh cleanup) to clean up after crashed processes or so - but we don't necessarily want to run such scripts inside the autotest whenever a non-root spdk is about to be started. Change-Id: I797e079417bb56491013cc5b92f0f0d14f451d18 Signed-off-by: Darek Stojaczyk Reviewed-on: https://review.gerrithub.io/c/spdk/spdk/+/467107 Tested-by: SPDK CI Jenkins Reviewed-by: Ben Walker Reviewed-by: Jim Harris --- include/spdk/env.h | 22 +++++++-- lib/env_dpdk/pci.c | 46 +++++++++++++++---- lib/nvme/nvme_pcie.c | 33 +++++-------- module/copy/ioat/copy_engine_ioat.c | 2 +- test/env/pci/pci_ut.c | 16 +++---- test/unit/lib/nvme/nvme_pcie.c/nvme_pcie_ut.c | 8 +++- 6 files changed, 79 insertions(+), 48 deletions(-) diff --git a/include/spdk/env.h b/include/spdk/env.h index b2b99ed7c..7214fe532 100644 --- a/include/spdk/env.h +++ b/include/spdk/env.h @@ -669,6 +669,8 @@ struct spdk_pci_device { struct _spdk_pci_device_internal { struct spdk_pci_driver *driver; bool attached; + /* optional fd for exclusive access to this device on this process */ + int claim_fd; bool pending_removal; /* The device was successfully removed on a DPDK interrupt thread, * but to prevent data races we couldn't remove it from the global @@ -895,13 +897,23 @@ int spdk_pci_device_get_serial_number(struct spdk_pci_device *dev, char *sn, siz * As long as this file remains open with the lock acquired, other processes will * not be able to successfully call this function on the same PCI device. * - * \param pci_addr PCI address of the device to claim + * The device can be un-claimed by the owning process with spdk_pci_device_unclaim(). + * It will be also unclaimed automatically when detached. * - * \return -1 if the device has already been claimed, an fd otherwise. This fd - * should be closed when the application no longer needs access to the PCI device - * (including when it is hot removed). + * \param dev PCI device to claim. + * + * \return -EACCES if the device has already been claimed, + * negative errno on unexpected errors, + * 0 on success. */ -int spdk_pci_device_claim(const struct spdk_pci_addr *pci_addr); +int spdk_pci_device_claim(struct spdk_pci_device *dev); + +/** + * Undo spdk_pci_device_claim(). + * + * \param dev PCI device to unclaim. + */ +void spdk_pci_device_unclaim(struct spdk_pci_device *dev); /** * Release all resources associated with the given device and detach it. As long diff --git a/lib/env_dpdk/pci.c b/lib/env_dpdk/pci.c index b4af0d18e..fad7ea8bb 100644 --- a/lib/env_dpdk/pci.c +++ b/lib/env_dpdk/pci.c @@ -342,6 +342,7 @@ spdk_pci_device_init(struct rte_pci_driver *_drv, dev->detach = spdk_detach_rte; dev->internal.driver = driver; + dev->internal.claim_fd = -1; if (driver->cb_fn != NULL) { rc = driver->cb_fn(driver->cb_arg, dev); @@ -387,6 +388,11 @@ void spdk_pci_device_detach(struct spdk_pci_device *dev) { assert(dev->internal.attached); + + if (dev->internal.claim_fd >= 0) { + spdk_pci_device_unclaim(dev); + } + dev->internal.attached = false; dev->detach(dev); @@ -728,7 +734,7 @@ spdk_pci_addr_compare(const struct spdk_pci_addr *a1, const struct spdk_pci_addr #ifdef __linux__ int -spdk_pci_device_claim(const struct spdk_pci_addr *pci_addr) +spdk_pci_device_claim(struct spdk_pci_device *dev) { int dev_fd; char dev_name[64]; @@ -741,20 +747,19 @@ spdk_pci_device_claim(const struct spdk_pci_addr *pci_addr) .l_len = 0, }; - snprintf(dev_name, sizeof(dev_name), "/tmp/spdk_pci_lock_%04x:%02x:%02x.%x", pci_addr->domain, - pci_addr->bus, - pci_addr->dev, pci_addr->func); + snprintf(dev_name, sizeof(dev_name), "/tmp/spdk_pci_lock_%04x:%02x:%02x.%x", + dev->addr.domain, dev->addr.bus, dev->addr.dev, dev->addr.func); dev_fd = open(dev_name, O_RDWR | O_CREAT, S_IRUSR | S_IWUSR); if (dev_fd == -1) { fprintf(stderr, "could not open %s\n", dev_name); - return -1; + return -errno; } if (ftruncate(dev_fd, sizeof(int)) != 0) { fprintf(stderr, "could not truncate %s\n", dev_name); close(dev_fd); - return -1; + return -errno; } dev_map = mmap(NULL, sizeof(int), PROT_READ | PROT_WRITE, @@ -762,7 +767,7 @@ spdk_pci_device_claim(const struct spdk_pci_addr *pci_addr) if (dev_map == MAP_FAILED) { fprintf(stderr, "could not mmap dev %s (%d)\n", dev_name, errno); close(dev_fd); - return -1; + return -errno; } if (fcntl(dev_fd, F_SETLK, &pcidev_lock) != 0) { @@ -771,23 +776,44 @@ spdk_pci_device_claim(const struct spdk_pci_addr *pci_addr) " process %d has claimed it\n", dev_name, pid); munmap(dev_map, sizeof(int)); close(dev_fd); - return -1; + /* F_SETLK returns unspecified errnos, normalize them */ + return -EACCES; } *(int *)dev_map = (int)getpid(); munmap(dev_map, sizeof(int)); + dev->internal.claim_fd = dev_fd; /* Keep dev_fd open to maintain the lock. */ - return dev_fd; + return 0; +} + +void +spdk_pci_device_unclaim(struct spdk_pci_device *dev) +{ + char dev_name[64]; + + snprintf(dev_name, sizeof(dev_name), "/tmp/spdk_pci_lock_%04x:%02x:%02x.%x", + dev->addr.domain, dev->addr.bus, dev->addr.dev, dev->addr.func); + + close(dev->internal.claim_fd); + dev->internal.claim_fd = -1; + unlink(dev_name); } #endif /* __linux__ */ #ifdef __FreeBSD__ int -spdk_pci_device_claim(const struct spdk_pci_addr *pci_addr) +spdk_pci_device_claim(struct spdk_pci_device *dev) { /* TODO */ return 0; } + +void +spdk_pci_device_unclaim(struct spdk_pci_device *dev) +{ + /* TODO */ +} #endif /* __FreeBSD__ */ int diff --git a/lib/nvme/nvme_pcie.c b/lib/nvme/nvme_pcie.c index b250e4bd7..4f6550a48 100644 --- a/lib/nvme/nvme_pcie.c +++ b/lib/nvme/nvme_pcie.c @@ -39,6 +39,7 @@ #include "spdk/stdinc.h" #include "spdk/env.h" #include "spdk/likely.h" +#include "spdk/string.h" #include "nvme_internal.h" #include "nvme_uevent.h" @@ -101,9 +102,6 @@ struct nvme_pcie_ctrlr { /* Opaque handle to associated PCI device. */ struct spdk_pci_device *devhandle; - /* File descriptor returned from spdk_pci_device_claim(). Closed when ctrlr is detached. */ - int claim_fd; - /* Flag to indicate the MMIO register has been remapped */ bool is_remapped; }; @@ -808,25 +806,20 @@ struct spdk_nvme_ctrlr *nvme_pcie_ctrlr_construct(const struct spdk_nvme_transpo union spdk_nvme_cap_register cap; union spdk_nvme_vs_register vs; uint32_t cmd_reg; - int rc, claim_fd; + int rc; struct spdk_pci_id pci_id; - struct spdk_pci_addr pci_addr; - if (spdk_pci_addr_parse(&pci_addr, trid->traddr)) { - SPDK_ERRLOG("could not parse pci address\n"); - return NULL; - } - - claim_fd = spdk_pci_device_claim(&pci_addr); - if (claim_fd < 0) { - SPDK_ERRLOG("could not claim device %s\n", trid->traddr); + rc = spdk_pci_device_claim(pci_dev); + if (rc < 0) { + SPDK_ERRLOG("could not claim device %s (%s)\n", + trid->traddr, spdk_strerror(-rc)); return NULL; } pctrlr = spdk_zmalloc(sizeof(struct nvme_pcie_ctrlr), 64, NULL, SPDK_ENV_SOCKET_ID_ANY, SPDK_MALLOC_SHARE); if (pctrlr == NULL) { - close(claim_fd); + spdk_pci_device_unclaim(pci_dev); SPDK_ERRLOG("could not allocate ctrlr\n"); return NULL; } @@ -836,19 +829,18 @@ struct spdk_nvme_ctrlr *nvme_pcie_ctrlr_construct(const struct spdk_nvme_transpo pctrlr->ctrlr.trid.trtype = SPDK_NVME_TRANSPORT_PCIE; pctrlr->devhandle = devhandle; pctrlr->ctrlr.opts = *opts; - pctrlr->claim_fd = claim_fd; memcpy(&pctrlr->ctrlr.trid, trid, sizeof(pctrlr->ctrlr.trid)); rc = nvme_ctrlr_construct(&pctrlr->ctrlr); if (rc != 0) { - close(claim_fd); + spdk_pci_device_unclaim(pci_dev); spdk_free(pctrlr); return NULL; } rc = nvme_pcie_ctrlr_allocate_bars(pctrlr); if (rc != 0) { - close(claim_fd); + spdk_pci_device_unclaim(pci_dev); spdk_free(pctrlr); return NULL; } @@ -860,14 +852,14 @@ struct spdk_nvme_ctrlr *nvme_pcie_ctrlr_construct(const struct spdk_nvme_transpo if (nvme_ctrlr_get_cap(&pctrlr->ctrlr, &cap)) { SPDK_ERRLOG("get_cap() failed\n"); - close(claim_fd); + spdk_pci_device_unclaim(pci_dev); spdk_free(pctrlr); return NULL; } if (nvme_ctrlr_get_vs(&pctrlr->ctrlr, &vs)) { SPDK_ERRLOG("get_vs() failed\n"); - close(claim_fd); + spdk_pci_device_unclaim(pci_dev); spdk_free(pctrlr); return NULL; } @@ -938,8 +930,6 @@ nvme_pcie_ctrlr_destruct(struct spdk_nvme_ctrlr *ctrlr) struct nvme_pcie_ctrlr *pctrlr = nvme_pcie_ctrlr(ctrlr); struct spdk_pci_device *devhandle = nvme_ctrlr_proc_get_devhandle(ctrlr); - close(pctrlr->claim_fd); - if (ctrlr->adminq) { nvme_pcie_qpair_destroy(ctrlr->adminq); } @@ -951,6 +941,7 @@ nvme_pcie_ctrlr_destruct(struct spdk_nvme_ctrlr *ctrlr) nvme_pcie_ctrlr_free_bars(pctrlr); if (devhandle) { + spdk_pci_device_unclaim(devhandle); spdk_pci_device_detach(devhandle); } diff --git a/module/copy/ioat/copy_engine_ioat.c b/module/copy/ioat/copy_engine_ioat.c index 4eebe3d10..5c0e7f07b 100644 --- a/module/copy/ioat/copy_engine_ioat.c +++ b/module/copy/ioat/copy_engine_ioat.c @@ -256,7 +256,7 @@ probe_cb(void *cb_ctx, struct spdk_pci_device *pci_dev) } /* Claim the device in case conflict with other process */ - if (spdk_pci_device_claim(&pci_addr) < 0) { + if (spdk_pci_device_claim(pci_dev) < 0) { return false; } diff --git a/test/env/pci/pci_ut.c b/test/env/pci/pci_ut.c index 9e6105e85..c2dfac3c9 100644 --- a/test/env/pci/pci_ut.c +++ b/test/env/pci/pci_ut.c @@ -38,25 +38,19 @@ #include "env_dpdk/pci.c" static void -pci_claim_test(void) +pci_claim_test(struct spdk_pci_device *dev) { int rc = 0; pid_t childPid; int status, ret; - struct spdk_pci_addr pci_addr; - pci_addr.domain = 0x0; - pci_addr.bus = 0x5; - pci_addr.dev = 0x4; - pci_addr.func = 1; - - rc = spdk_pci_device_claim(&pci_addr); + rc = spdk_pci_device_claim(dev); CU_ASSERT(rc >= 0); childPid = fork(); CU_ASSERT(childPid >= 0); if (childPid == 0) { - ret = spdk_pci_device_claim(&pci_addr); + ret = spdk_pci_device_claim(dev); CU_ASSERT(ret == -1); exit(0); } else { @@ -206,6 +200,9 @@ pci_hook_test(void) rc = spdk_pci_device_map_bar(&ut_dev.pci, 1, &bar0_vaddr, &bar0_paddr, &bar0_size); CU_ASSERT(rc != 0); + /* test spdk_pci_device_claim() */ + pci_claim_test(&ut_dev.pci); + /* detach and verify our callback was called */ spdk_pci_device_detach(&ut_dev.pci); CU_ASSERT(!ut_dev.attached); @@ -235,7 +232,6 @@ int main(int argc, char **argv) } if ( - CU_add_test(suite, "pci_claim", pci_claim_test) == NULL || CU_add_test(suite, "pci_hook", pci_hook_test) == NULL ) { CU_cleanup_registry(); diff --git a/test/unit/lib/nvme/nvme_pcie.c/nvme_pcie_ut.c b/test/unit/lib/nvme/nvme_pcie.c/nvme_pcie_ut.c index 937bd4255..1c7dcd668 100644 --- a/test/unit/lib/nvme/nvme_pcie.c/nvme_pcie_ut.c +++ b/test/unit/lib/nvme/nvme_pcie.c/nvme_pcie_ut.c @@ -153,7 +153,13 @@ spdk_pci_device_cfg_write32(struct spdk_pci_device *dev, uint32_t value, uint32_ } int -spdk_pci_device_claim(const struct spdk_pci_addr *pci_addr) +spdk_pci_device_claim(struct spdk_pci_device *dev) +{ + abort(); +} + +void +spdk_pci_device_unclaim(struct spdk_pci_device *dev) { abort(); }