From af2600c52f6ec959036accadf2c1d47252b72d20 Mon Sep 17 00:00:00 2001 From: Mykola Golub Date: Mon, 2 Nov 2020 17:50:54 +0000 Subject: [PATCH 1/7] rbd-nbd: use pidfd_open syscall if available If it is not fallback to just periodically polling the process still exists. Signed-off-by: Mykola Golub --- src/tools/rbd_nbd/rbd-nbd.cc | 48 +++++++++++++++++++++++++++++------- 1 file changed, 39 insertions(+), 9 deletions(-) diff --git a/src/tools/rbd_nbd/rbd-nbd.cc b/src/tools/rbd_nbd/rbd-nbd.cc index 17592f01594db..01be6ce0be7a9 100644 --- a/src/tools/rbd_nbd/rbd-nbd.cc +++ b/src/tools/rbd_nbd/rbd-nbd.cc @@ -34,6 +34,7 @@ #include #include #include +#include #include "nbd-netlink.h" #include @@ -1431,24 +1432,53 @@ static void run_server(Preforker& forker, NBDServer *server, bool netlink_used) shutdown_async_signal_handler(); } -// Eventually it should be replaced with glibc' pidfd_open -// when it is widely available. -static int -pidfd_open(pid_t pid, unsigned int) +// Eventually it should be removed when pidfd_open is widely supported. + +static int wait_for_terminate_legacy(int pid, int timeout) { - std::string path = "/proc/" + stringify(pid); - int fd = open(path.c_str(), O_RDONLY); - if (fd == -1 && errno == ENOENT) { - errno = ESRCH; + for (int i = 0; ; i++) { + if (kill(pid, 0) == -1) { + if (errno == ESRCH) { + return 0; + } + int r = -errno; + cerr << "rbd-nbd: kill(" << pid << ", 0) failed: " + << cpp_strerror(r) << std::endl; + return r; + } + if (i >= timeout * 2) { + break; + } + usleep(500000); } - return fd; + cerr << "rbd-nbd: waiting for process exit timed out" << std::endl; + return -ETIMEDOUT; } +// Eventually it should be replaced with glibc' pidfd_open +// when it is widely available. + +#ifdef __NR_pidfd_open +static int pidfd_open(pid_t pid, unsigned int flags) +{ + return syscall(__NR_pidfd_open, pid, flags); +} +#else +static int pidfd_open(pid_t pid, unsigned int flags) +{ + errno = ENOSYS; + return -1; +} +#endif + static int wait_for_terminate(int pid, int timeout) { int fd = pidfd_open(pid, 0); if (fd == -1) { + if (errno == ENOSYS) { + return wait_for_terminate_legacy(pid, timeout); + } if (errno == -ESRCH) { return 0; } From 0cf593f27a233e2592ecfc9db7ce3ffe28d92344 Mon Sep 17 00:00:00 2001 From: Mykola Golub Date: Mon, 2 Nov 2020 17:56:56 +0000 Subject: [PATCH 2/7] rbd-nbd: unmap wrongly skipped wait for terminate if netlink disconnect succeeded. Also make sure wait_for_terminate returns 0 on success. Signed-off-by: Mykola Golub --- src/tools/rbd_nbd/rbd-nbd.cc | 41 +++++++++++++++++++++--------------- 1 file changed, 24 insertions(+), 17 deletions(-) diff --git a/src/tools/rbd_nbd/rbd-nbd.cc b/src/tools/rbd_nbd/rbd-nbd.cc index 01be6ce0be7a9..edec26932b5b9 100644 --- a/src/tools/rbd_nbd/rbd-nbd.cc +++ b/src/tools/rbd_nbd/rbd-nbd.cc @@ -1479,7 +1479,7 @@ static int wait_for_terminate(int pid, int timeout) if (errno == ENOSYS) { return wait_for_terminate_legacy(pid, timeout); } - if (errno == -ESRCH) { + if (errno == ESRCH) { return 0; } int r = -errno; @@ -1499,6 +1499,8 @@ static int wait_for_terminate(int pid, int timeout) cerr << "rbd-nbd: failed to poll rbd-nbd process: " << cpp_strerror(r) << std::endl; goto done; + } else { + r = 0; } if ((poll_fds[0].revents & POLLIN) == 0) { @@ -1730,34 +1732,39 @@ static int do_detach(Config *cfg) static int do_unmap(Config *cfg) { - int r, nbd; - /* * The netlink disconnect call supports devices setup with netlink or ioctl, * so we always try that first. */ - r = netlink_disconnect_by_path(cfg->devpath); - if (r != 1) + int r = netlink_disconnect_by_path(cfg->devpath); + if (r < 0) { return r; - - nbd = open(cfg->devpath.c_str(), O_RDWR); - if (nbd < 0) { - cerr << "rbd-nbd: failed to open device: " << cfg->devpath << std::endl; - return nbd; } - r = ioctl(nbd, NBD_DISCONNECT); - if (r < 0) { + if (r == 1) { + int nbd = open(cfg->devpath.c_str(), O_RDWR); + if (nbd < 0) { + cerr << "rbd-nbd: failed to open device: " << cfg->devpath << std::endl; + return nbd; + } + + r = ioctl(nbd, NBD_DISCONNECT); + if (r < 0) { cerr << "rbd-nbd: the device is not used" << std::endl; - } + } - close(nbd); + close(nbd); - if (r < 0) { - return r; + if (r < 0) { + return r; + } } - return wait_for_terminate(cfg->pid, cfg->reattach_timeout); + if (cfg->pid > 0) { + r = wait_for_terminate(cfg->pid, cfg->reattach_timeout); + } + + return 0; } static int parse_imgpath(const std::string &imgpath, Config *cfg, From 4cda97a8228fa362405bb5a9497731edafcdc3d8 Mon Sep 17 00:00:00 2001 From: Mykola Golub Date: Mon, 2 Nov 2020 19:07:03 +0000 Subject: [PATCH 3/7] rbd-nbd: when unmapping or detaching by device try to find process For `detach` failing to find the process is fatal while unmap will still try to send disconnect to the device. Signed-off-by: Mykola Golub --- qa/workunits/rbd/rbd-nbd.sh | 4 ++++ src/tools/rbd_nbd/rbd-nbd.cc | 33 +++++++++++++++++++++++++++------ 2 files changed, 31 insertions(+), 6 deletions(-) diff --git a/qa/workunits/rbd/rbd-nbd.sh b/qa/workunits/rbd/rbd-nbd.sh index 879094dc92bbf..fd79ba5fe4dec 100755 --- a/qa/workunits/rbd/rbd-nbd.sh +++ b/qa/workunits/rbd/rbd-nbd.sh @@ -321,6 +321,10 @@ _sudo rbd-nbd detach ${POOL}/${IMAGE} expect_false get_pid _sudo rbd-nbd attach --device ${DEV} ${POOL}/${IMAGE} get_pid +_sudo rbd-nbd detach ${DEV} +expect_false get_pid +_sudo rbd-nbd attach --device ${DEV} ${POOL}/${IMAGE} +get_pid ls ${TEMPDIR}/mnt/ dd if=${TEMPDIR}/mnt/test of=/dev/null bs=1M count=1 _sudo dd if=${DATA} of=${TEMPDIR}/mnt/test1 bs=1M count=1 oflag=direct diff --git a/src/tools/rbd_nbd/rbd-nbd.cc b/src/tools/rbd_nbd/rbd-nbd.cc index edec26932b5b9..a4fb7c10101fb 100644 --- a/src/tools/rbd_nbd/rbd-nbd.cc +++ b/src/tools/rbd_nbd/rbd-nbd.cc @@ -1864,6 +1864,17 @@ static bool find_mapped_dev_by_spec(Config *cfg, int skip_pid=-1) { return false; } +static int find_proc_by_dev(Config *cfg) { + Config c; + NBDListIterator it; + while (it.get(&c)) { + if (c.devpath == cfg->devpath) { + *cfg = c; + return true; + } + } + return false; +} static int parse_args(vector& args, std::ostream *err_msg, Config *cfg) { @@ -2077,8 +2088,14 @@ static int rbd_nbd(int argc, const char *argv[]) return -EINVAL; break; case Detach: - if (cfg.devpath.empty() && !find_mapped_dev_by_spec(&cfg)) { - cerr << "rbd-nbd: " << cfg.image_spec() << " is not mapped" + if (cfg.devpath.empty()) { + if (!find_mapped_dev_by_spec(&cfg)) { + cerr << "rbd-nbd: " << cfg.image_spec() << " is not mapped" + << std::endl; + return -ENOENT; + } + } else if (!find_proc_by_dev(&cfg)) { + cerr << "rbd-nbd: no process attached to " << cfg.devpath << " found" << std::endl; return -ENOENT; } @@ -2087,10 +2104,14 @@ static int rbd_nbd(int argc, const char *argv[]) return -EINVAL; break; case Unmap: - if (cfg.devpath.empty() && !find_mapped_dev_by_spec(&cfg)) { - cerr << "rbd-nbd: " << cfg.image_spec() << " is not mapped" - << std::endl; - return -ENOENT; + if (cfg.devpath.empty()) { + if (!find_mapped_dev_by_spec(&cfg)) { + cerr << "rbd-nbd: " << cfg.image_spec() << " is not mapped" + << std::endl; + return -ENOENT; + } + } else if (!find_proc_by_dev(&cfg)) { + // still try to send disconnect to the device } r = do_unmap(&cfg); if (r < 0) From 746ac7265dd4835998f2d47803d23dc017e0ed69 Mon Sep 17 00:00:00 2001 From: Mykola Golub Date: Thu, 5 Nov 2020 17:46:19 +0000 Subject: [PATCH 4/7] rbd-nbd: don't look for cmdline in /proc/[tid] subdirectories The proc fs contains both /proc/[pid] and /proc/[tid] subdirectories. The /proc/[tid] subdirectories are not visible when listing /proc. But when checking pid for reattached process, get_mapped_info directly tries to open /proc/[pid]/cmdline, and it may actually be tid belonging to some other rbd-nbd process. Try to avoid this by checking /proc/[pid]/comm first -- for pid we expect to find "rbd-nbd" here. Signed-off-by: Mykola Golub --- src/tools/rbd_nbd/rbd-nbd.cc | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/tools/rbd_nbd/rbd-nbd.cc b/src/tools/rbd_nbd/rbd-nbd.cc index a4fb7c10101fb..eee700a5ee8f2 100644 --- a/src/tools/rbd_nbd/rbd-nbd.cc +++ b/src/tools/rbd_nbd/rbd-nbd.cc @@ -827,8 +827,19 @@ class NBDListIterator { m_mapped_info_cache[pid] = {}; int r; - std::string path = "/proc/" + stringify(pid) + "/cmdline"; + std::string path = "/proc/" + stringify(pid) + "/comm"; std::ifstream ifs; + std::string comm; + ifs.open(path.c_str(), std::ifstream::in); + if (!ifs.is_open()) + return -1; + ifs >> comm; + if (comm != "rbd-nbd") { + return -EINVAL; + } + ifs.close(); + + path = "/proc/" + stringify(pid) + "/cmdline"; std::string cmdline; std::vector args; From 5dd897cc4871d0d7248376ceff57ebf4410fba56 Mon Sep 17 00:00:00 2001 From: Mykola Golub Date: Thu, 5 Nov 2020 17:56:25 +0000 Subject: [PATCH 5/7] rbd-nbd: always check device when searching re-attached process Previously, if the same image was mapped twice and then one wanted to re-attach it could try to use a wrong device. Also list-mapped could produce incorrect listing after re-attach. Signed-off-by: Mykola Golub --- src/tools/rbd_nbd/rbd-nbd.cc | 31 ++++++++++++++++++++----------- 1 file changed, 20 insertions(+), 11 deletions(-) diff --git a/src/tools/rbd_nbd/rbd-nbd.cc b/src/tools/rbd_nbd/rbd-nbd.cc index eee700a5ee8f2..713a0b83b63ca 100644 --- a/src/tools/rbd_nbd/rbd-nbd.cc +++ b/src/tools/rbd_nbd/rbd-nbd.cc @@ -815,9 +815,11 @@ class NBDListIterator { std::map m_mapped_info_cache; int get_mapped_info(int pid, Config *cfg) { + ceph_assert(!cfg->devpath.empty()); + auto it = m_mapped_info_cache.find(pid); if (it != m_mapped_info_cache.end()) { - if (it->second.devpath.empty()) { + if (it->second.devpath != cfg->devpath) { return -EINVAL; } *cfg = it->second; @@ -868,22 +870,28 @@ class NBDListIterator { } std::ostringstream err_msg; - r = parse_args(args, &err_msg, cfg); + Config c; + r = parse_args(args, &err_msg, &c); if (r < 0) { return r; } - if (cfg->command != Map && cfg->command != Attach) { + if (c.command != Map && c.command != Attach) { return -ENOENT; } - cfg->pid = pid; - + c.pid = pid; m_mapped_info_cache.erase(pid); - if (!cfg->devpath.empty()) { - m_mapped_info_cache[pid] = *cfg; + if (!c.devpath.empty()) { + m_mapped_info_cache[pid] = c; + if (c.devpath != cfg->devpath) { + return -ENOENT; + } + } else { + c.devpath = cfg->devpath; } + *cfg = c; return 0; } @@ -901,8 +909,8 @@ class NBDListIterator { } Config cfg; - if (get_mapped_info(pid, &cfg) >=0 && cfg.devpath == devpath && - cfg.command == Attach) { + cfg.devpath = devpath; + if (get_mapped_info(pid, &cfg) >=0 && cfg.command == Attach) { return cfg.pid; } } @@ -1867,7 +1875,8 @@ static bool find_mapped_dev_by_spec(Config *cfg, int skip_pid=-1) { while (it.get(&c)) { if (c.pid != skip_pid && c.poolname == cfg->poolname && c.nsname == cfg->nsname && - c.imgname == cfg->imgname && c.snapname == cfg->snapname) { + c.imgname == cfg->imgname && c.snapname == cfg->snapname && + (cfg->devpath.empty() || c.devpath == cfg->devpath)) { *cfg = c; return true; } @@ -2014,7 +2023,7 @@ static int parse_args(vector& args, std::ostream *err_msg, if (cfg->devpath.empty()) { *err_msg << "rbd-nbd: must specify device to attach"; return -EINVAL; - } + } [[fallthrough]]; case Map: if (args.begin() == args.end()) { From 9c9b14fab6f648b7f0bb63b299eac4e281d24994 Mon Sep 17 00:00:00 2001 From: Mykola Golub Date: Thu, 5 Nov 2020 18:37:45 +0000 Subject: [PATCH 6/7] qa/workunits/rbd: improve regex for parsing ps output On some platforms the pid may be indented with spaces. Signed-off-by: Mykola Golub --- qa/workunits/rbd/rbd-nbd.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/qa/workunits/rbd/rbd-nbd.sh b/qa/workunits/rbd/rbd-nbd.sh index fd79ba5fe4dec..37718e3b557c2 100755 --- a/qa/workunits/rbd/rbd-nbd.sh +++ b/qa/workunits/rbd/rbd-nbd.sh @@ -108,7 +108,7 @@ unmap_device() _sudo rbd-nbd unmap ${dev} rbd-nbd list-mapped | expect_false grep "^${pid}\\b" || return 1 - ps -C rbd-nbd | expect_false grep "^${pid}\\b" || return 1 + ps -C rbd-nbd | expect_false grep "^ *${pid}\\b" || return 1 # workaround possible race between unmap and following map sleep 0.5 From be4f1126467e93d0984628a60d860cfa1ab1a771 Mon Sep 17 00:00:00 2001 From: Mykola Golub Date: Mon, 9 Nov 2020 09:08:39 +0000 Subject: [PATCH 7/7] rbd-nbd: try to disconnect before terminating when read from nbd socket returned error Signed-off-by: Mykola Golub --- src/tools/rbd_nbd/rbd-nbd.cc | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-) diff --git a/src/tools/rbd_nbd/rbd-nbd.cc b/src/tools/rbd_nbd/rbd-nbd.cc index 713a0b83b63ca..11ccdf0e4b4db 100644 --- a/src/tools/rbd_nbd/rbd-nbd.cc +++ b/src/tools/rbd_nbd/rbd-nbd.cc @@ -181,6 +181,7 @@ static EventSocket terminate_event_sock; static int parse_args(vector& args, std::ostream *err_msg, Config *cfg); +static int netlink_disconnect(int index); static int netlink_resize(int nbd_index, uint64_t size); static int run_quiesce_hook(const std::string &quiesce_hook, @@ -361,7 +362,7 @@ class NBDServer } r = -errno; derr << "failed to poll nbd: " << cpp_strerror(r) << dendl; - goto signal; + goto error; } if ((poll_fds[1].revents & POLLIN) != 0) { @@ -378,7 +379,7 @@ class NBDServer if (r < 0) { derr << "failed to read nbd request header: " << cpp_strerror(r) << dendl; - goto signal; + goto error; } if (ctx->request.magic != htonl(NBD_REQUEST_MAGIC)) { @@ -409,7 +410,7 @@ class NBDServer if (r < 0) { derr << *ctx << ": failed to read nbd request data: " << cpp_strerror(r) << dendl; - goto signal; + goto error; } ctx->data.push_back(ptr); break; @@ -439,6 +440,13 @@ class NBDServer goto signal; } } +error: + { + int r = netlink_disconnect(nbd_index); + if (r == 1) { + ioctl(nbd, NBD_DISCONNECT); + } + } signal: std::lock_guard l{lock}; terminated = true;