[PATCHv2 0/2] Support VM core dump to block device
When the destination of a VM core dump is a block device (e.g. NBD) avoid file operations that are unnecessary. Simon Rowe (2): iohelper: skip lseek() and ftruncate() on block devices qemu: check unlink hint from virQEMUFileOpenAs() src/qemu/qemu_driver.c | 8 +++----- src/util/iohelper.c | 10 ++++++++-- 2 files changed, 11 insertions(+), 7 deletions(-) -- 2.22.3
Signed-off-by: Simon Rowe <simon.rowe@nutanix.com> --- src/util/iohelper.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/util/iohelper.c b/src/util/iohelper.c index b8810d16d3..e6eb178fde 100644 --- a/src/util/iohelper.c +++ b/src/util/iohelper.c @@ -28,6 +28,8 @@ #include <unistd.h> #include <fcntl.h> #include <stdlib.h> +#include <sys/types.h> +#include <sys/stat.h> #include "virthread.h" #include "virfile.h" @@ -56,6 +58,8 @@ runIO(const char *path, int fd, int oflags) unsigned long long total = 0; bool direct = O_DIRECT && ((oflags & O_DIRECT) != 0); off_t end = 0; + struct stat sb; + bool isBlockDev = false; #if WITH_POSIX_MEMALIGN if (posix_memalign(&base, alignMask + 1, buflen)) @@ -86,9 +90,11 @@ runIO(const char *path, int fd, int oflags) fdinname = "stdin"; fdout = fd; fdoutname = path; + if (fstat(fd, &sb) == 0) + isBlockDev = S_ISBLK(sb.st_mode); /* To make the implementation simpler, we give up on any * attempt to use O_DIRECT in a non-trivial manner. */ - if (direct && (end = lseek(fd, 0, SEEK_END)) != 0) { + if (!isBlockDev && direct && (end = lseek(fd, 0, SEEK_END)) != 0) { virReportSystemError(end < 0 ? errno : EINVAL, "%s", _("O_DIRECT write needs empty seekable file")); goto cleanup; @@ -140,7 +146,7 @@ runIO(const char *path, int fd, int oflags) goto cleanup; } - if (ftruncate(fd, total) < 0) { + if (!isBlockDev && ftruncate(fd, total) < 0) { virReportSystemError(errno, _("Unable to truncate %s"), fdoutname); goto cleanup; } -- 2.22.3
On 8/23/21 5:40 PM, Simon Rowe wrote:
Signed-off-by: Simon Rowe <simon.rowe@nutanix.com> --- src/util/iohelper.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/src/util/iohelper.c b/src/util/iohelper.c index b8810d16d3..e6eb178fde 100644 --- a/src/util/iohelper.c +++ b/src/util/iohelper.c @@ -28,6 +28,8 @@ #include <unistd.h> #include <fcntl.h> #include <stdlib.h> +#include <sys/types.h> +#include <sys/stat.h>
#include "virthread.h" #include "virfile.h" @@ -56,6 +58,8 @@ runIO(const char *path, int fd, int oflags) unsigned long long total = 0; bool direct = O_DIRECT && ((oflags & O_DIRECT) != 0); off_t end = 0; + struct stat sb; + bool isBlockDev = false;
#if WITH_POSIX_MEMALIGN if (posix_memalign(&base, alignMask + 1, buflen)) @@ -86,9 +90,11 @@ runIO(const char *path, int fd, int oflags) fdinname = "stdin"; fdout = fd; fdoutname = path; + if (fstat(fd, &sb) == 0) + isBlockDev = S_ISBLK(sb.st_mode);
I think we can check for this before this switch() and report error if fstat() fails. That way we can ..
/* To make the implementation simpler, we give up on any * attempt to use O_DIRECT in a non-trivial manner. */ - if (direct && (end = lseek(fd, 0, SEEK_END)) != 0) { + if (!isBlockDev && direct && (end = lseek(fd, 0, SEEK_END)) != 0) {
.. do this change for O_RDONLY case too. IOW, I suggest squashing this in: diff --git i/src/util/iohelper.c w/src/util/iohelper.c index e6eb178fde..2c91bf4f93 100644 --- i/src/util/iohelper.c +++ w/src/util/iohelper.c @@ -71,6 +71,14 @@ runIO(const char *path, int fd, int oflags) buf = (char *) (((intptr_t) base + alignMask) & ~alignMask); #endif + if (fstat(fd, &sb) < 0) { + virReportSystemError(errno, + _("Unable to access file descriptor %d path %s"), + fd, path); + goto cleanup; + } + isBlockDev = S_ISBLK(sb.st_mode); + switch (oflags & O_ACCMODE) { case O_RDONLY: fdin = fd; @@ -79,7 +87,7 @@ runIO(const char *path, int fd, int oflags) fdoutname = "stdout"; /* To make the implementation simpler, we give up on any * attempt to use O_DIRECT in a non-trivial manner. */ - if (direct && ((end = lseek(fd, 0, SEEK_CUR)) != 0)) { + if (!isBlockDev && direct && ((end = lseek(fd, 0, SEEK_CUR)) != 0)) { virReportSystemError(end < 0 ? errno : EINVAL, "%s", _("O_DIRECT read needs entire seekable file")); goto cleanup; @@ -90,8 +98,6 @@ runIO(const char *path, int fd, int oflags) fdinname = "stdin"; fdout = fd; fdoutname = path; - if (fstat(fd, &sb) == 0) - isBlockDev = S_ISBLK(sb.st_mode); /* To make the implementation simpler, we give up on any * attempt to use O_DIRECT in a non-trivial manner. */ if (!isBlockDev && direct && (end = lseek(fd, 0, SEEK_END)) != 0) { No need to resend, just let me know if you're okay with the suggested change and I'll squash it in before pushing. Michal
That's fine with me, thanks Simon ________________________________ From: Michal Prívozník <mprivozn@redhat.com> Sent: 24 August 2021 14:37 To: Simon Rowe <simon.rowe@nutanix.com>; libvir-list@redhat.com <libvir-list@redhat.com> Subject: Re: [PATCHv2 1/2] iohelper: skip lseek() and ftruncate() on block devices On 8/23/21 5:40 PM, Simon Rowe wrote:
Signed-off-by: Simon Rowe <simon.rowe@nutanix.com> --- src/util/iohelper.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/src/util/iohelper.c b/src/util/iohelper.c index b8810d16d3..e6eb178fde 100644 --- a/src/util/iohelper.c +++ b/src/util/iohelper.c @@ -28,6 +28,8 @@ #include <unistd.h> #include <fcntl.h> #include <stdlib.h> +#include <sys/types.h> +#include <sys/stat.h>
#include "virthread.h" #include "virfile.h" @@ -56,6 +58,8 @@ runIO(const char *path, int fd, int oflags) unsigned long long total = 0; bool direct = O_DIRECT && ((oflags & O_DIRECT) != 0); off_t end = 0; + struct stat sb; + bool isBlockDev = false;
#if WITH_POSIX_MEMALIGN if (posix_memalign(&base, alignMask + 1, buflen)) @@ -86,9 +90,11 @@ runIO(const char *path, int fd, int oflags) fdinname = "stdin"; fdout = fd; fdoutname = path; + if (fstat(fd, &sb) == 0) + isBlockDev = S_ISBLK(sb.st_mode);
I think we can check for this before this switch() and report error if fstat() fails. That way we can ..
/* To make the implementation simpler, we give up on any * attempt to use O_DIRECT in a non-trivial manner. */ - if (direct && (end = lseek(fd, 0, SEEK_END)) != 0) { + if (!isBlockDev && direct && (end = lseek(fd, 0, SEEK_END)) != 0) {
.. do this change for O_RDONLY case too. IOW, I suggest squashing this in: diff --git i/src/util/iohelper.c w/src/util/iohelper.c index e6eb178fde..2c91bf4f93 100644 --- i/src/util/iohelper.c +++ w/src/util/iohelper.c @@ -71,6 +71,14 @@ runIO(const char *path, int fd, int oflags) buf = (char *) (((intptr_t) base + alignMask) & ~alignMask); #endif + if (fstat(fd, &sb) < 0) { + virReportSystemError(errno, + _("Unable to access file descriptor %d path %s"), + fd, path); + goto cleanup; + } + isBlockDev = S_ISBLK(sb.st_mode); + switch (oflags & O_ACCMODE) { case O_RDONLY: fdin = fd; @@ -79,7 +87,7 @@ runIO(const char *path, int fd, int oflags) fdoutname = "stdout"; /* To make the implementation simpler, we give up on any * attempt to use O_DIRECT in a non-trivial manner. */ - if (direct && ((end = lseek(fd, 0, SEEK_CUR)) != 0)) { + if (!isBlockDev && direct && ((end = lseek(fd, 0, SEEK_CUR)) != 0)) { virReportSystemError(end < 0 ? errno : EINVAL, "%s", _("O_DIRECT read needs entire seekable file")); goto cleanup; @@ -90,8 +98,6 @@ runIO(const char *path, int fd, int oflags) fdinname = "stdin"; fdout = fd; fdoutname = path; - if (fstat(fd, &sb) == 0) - isBlockDev = S_ISBLK(sb.st_mode); /* To make the implementation simpler, we give up on any * attempt to use O_DIRECT in a non-trivial manner. */ if (!isBlockDev && direct && (end = lseek(fd, 0, SEEK_END)) != 0) { No need to resend, just let me know if you're okay with the suggested change and I'll squash it in before pushing. Michal
Signed-off-by: Simon Rowe <simon.rowe@nutanix.com> --- src/qemu/qemu_driver.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) Changes from v1: * only unlink if virQEMUFileOpenAs() created the file diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 444e9e5cbc..6e83d7e068 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -3201,6 +3201,7 @@ doCoreDump(virQEMUDriver *driver, int rc = -1; virFileWrapperFd *wrapperFd = NULL; int directFlag = 0; + bool needUnlink = false; unsigned int flags = VIR_FILE_WRAPPER_NON_BLOCKING; const char *memory_dump_format = NULL; g_autoptr(virQEMUDriverConfig) cfg = virQEMUDriverGetConfig(driver); @@ -3224,12 +3225,9 @@ doCoreDump(virQEMUDriver *driver, goto cleanup; } } - /* Core dumps usually imply last-ditch analysis efforts are - * desired, so we intentionally do not unlink even if a file was - * created. */ if ((fd = virQEMUFileOpenAs(cfg->user, cfg->group, false, path, O_CREAT | O_TRUNC | O_WRONLY | directFlag, - NULL)) < 0) + &needUnlink)) < 0) goto cleanup; if (!(wrapperFd = virFileWrapperFdNew(&fd, path, flags))) @@ -3282,7 +3280,7 @@ doCoreDump(virQEMUDriver *driver, if (qemuDomainFileWrapperFDClose(vm, wrapperFd) < 0) ret = -1; virFileWrapperFdFree(wrapperFd); - if (ret != 0) + if (ret != 0 && needUnlink) unlink(path); return ret; } -- 2.22.3
On 8/23/21 5:40 PM, Simon Rowe wrote:
When the destination of a VM core dump is a block device (e.g. NBD) avoid file operations that are unnecessary.
Simon Rowe (2): iohelper: skip lseek() and ftruncate() on block devices qemu: check unlink hint from virQEMUFileOpenAs()
src/qemu/qemu_driver.c | 8 +++----- src/util/iohelper.c | 10 ++++++++-- 2 files changed, 11 insertions(+), 7 deletions(-)
Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
On 8/23/21 5:40 PM, Simon Rowe wrote:
When the destination of a VM core dump is a block device (e.g. NBD) avoid file operations that are unnecessary.
Simon Rowe (2): iohelper: skip lseek() and ftruncate() on block devices qemu: check unlink hint from virQEMUFileOpenAs()
src/qemu/qemu_driver.c | 8 +++----- src/util/iohelper.c | 10 ++++++++-- 2 files changed, 11 insertions(+), 7 deletions(-)
Sqaushed in suggested diff and pushed. Congratulations on your first libvirt contribution! Reviewed-by: Michal Privoznik <mprivozn@redhat.com> Michal
participants (2)
-
Michal Prívozník -
Simon Rowe