[PATCH v3 0/3 (one patch for 6.6.0)] Fix and rework resctrl locking
v3: - put the fixing code first, so it can be pushed in the freeze - get rid of unnecessary code - CI check: https://gitlab.com/nertpinx/libvirt/-/pipelines/172040463 v2: - Split changes into separate patches - https://www.redhat.com/archives/libvir-list/2020-July/msg00601.html v1: - https://www.redhat.com/archives/libvir-list/2020-July/msg00560.html Martin Kletzander (3): resctrl: Use exclusive lock for /sys/fs/resctrl util: Get rid of virFileFlock() resctrl: Rename virResctrlLockWrite -> virResctrlLock src/libvirt_private.syms | 1 - src/util/virfile.c | 31 +------------------------------ src/util/virfile.h | 2 -- src/util/virresctrl.c | 23 ++++++++++++++++++----- 4 files changed, 19 insertions(+), 38 deletions(-) -- 2.28.0
That's the way it should've been all the time. It was originally the case, but then the rework to virFileFlock() made the function ambiguous when it was created in commit 5a0a5f7fb5f5, and due to that it was misused in commit 657ddeff2313 and since then the lock being taken was shared rather than exclusive. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/util/virresctrl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 3563fc560db5..9b78a6cb159b 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -463,7 +463,7 @@ virResctrlLockWrite(void) return -1; } - if (virFileFlock(fd, true, true) < 0) { + if (virFileFlock(fd, true, false) < 0) { virReportSystemError(errno, "%s", _("Cannot lock resctrl")); VIR_FORCE_CLOSE(fd); return -1; -- 2.28.0
On Wed, 2020-07-29 at 13:43 +0200, Martin Kletzander wrote:
That's the way it should've been all the time. It was originally the case, but then the rework to virFileFlock() made the function ambiguous when it was created in commit 5a0a5f7fb5f5, and due to that it was misused in commit 657ddeff2313 and since then the lock being taken was shared rather than exclusive.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/util/virresctrl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Consider this Reviewed-by: Andrea Bolognani <abologna@redhat.com> and safe for freeze, but honestly this issue has been around for more than two years at this point so I'm not entirely convinced it can't just wait for the merge window to open again. Up to you. -- Andrea Bolognani / Red Hat / Virtualization
On Wed, Jul 29, 2020 at 02:09:20PM +0200, Andrea Bolognani wrote:
On Wed, 2020-07-29 at 13:43 +0200, Martin Kletzander wrote:
That's the way it should've been all the time. It was originally the case, but then the rework to virFileFlock() made the function ambiguous when it was created in commit 5a0a5f7fb5f5, and due to that it was misused in commit 657ddeff2313 and since then the lock being taken was shared rather than exclusive.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/util/virresctrl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Consider this
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
and safe for freeze, but honestly this issue has been around for more than two years at this point so I'm not entirely convinced it can't just wait for the merge window to open again. Up to you.
I agree and I'm fine with that.
-- Andrea Bolognani / Red Hat / Virtualization
On a Wednesday in 2020, Andrea Bolognani wrote:
On Wed, 2020-07-29 at 13:43 +0200, Martin Kletzander wrote:
That's the way it should've been all the time. It was originally the case, but then the rework to virFileFlock() made the function ambiguous when it was created in commit 5a0a5f7fb5f5, and due to that it was misused in commit 657ddeff2313 and since then the lock being taken was shared rather than exclusive.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/util/virresctrl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
Consider this
Reviewed-by: Andrea Bolognani <abologna@redhat.com>
and safe for freeze, but honestly this issue has been around for more than two years at this point so I'm not entirely convinced it can't just wait for the merge window to open again. Up to you.
The merge window is open. We are in a feature freeze, meaning we are free to push bug fixes. Possibly trivial typos in the CI configuration. A freeze meaing "we do not push anything at all" is pointless. Jano
-- Andrea Bolognani / Red Hat / Virtualization
It was created to get rid of conditional compilation in the resctrl code and make it usable anywhere else. However this is not something that is going to be used in other places because it is not portable and resctrl is just very specific in this regard. And there is no reason why there could not be a preprocessor conditional in the resctrl code. Also the interface of virFileFlock() was very ambiguous which lead to some issues. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/libvirt_private.syms | 1 - src/util/virfile.c | 31 +------------------------------ src/util/virfile.h | 2 -- src/util/virresctrl.c | 17 +++++++++++++++-- 4 files changed, 16 insertions(+), 35 deletions(-) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index eea31a736def..fbbe07becbcf 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2074,7 +2074,6 @@ virFileFindHugeTLBFS; virFileFindMountPoint; virFileFindResource; virFileFindResourceFull; -virFileFlock; virFileFreeACLs; virFileGetACLs; virFileGetDefaultHugepage; diff --git a/src/util/virfile.c b/src/util/virfile.c index af150421e726..b1ca3adcff04 100644 --- a/src/util/virfile.c +++ b/src/util/virfile.c @@ -463,30 +463,9 @@ int virFileUnlock(int fd, off_t start, off_t len) } -/** - * virFileFlock: - * @fd: file descriptor to call flock on - * @lock: true for lock, false for unlock - * @shared: true if shared, false for exclusive, ignored if `@lock == false` - * - * This is just a simple wrapper around flock(2) that errors out on unsupported - * platforms. - * - * The lock will be released when @fd is closed or this function is called with - * `@lock == false`. - * - * Returns 0 on success, -1 otherwise (with errno set) - */ -int virFileFlock(int fd, bool lock, bool shared) -{ - if (lock) - return flock(fd, shared ? LOCK_SH : LOCK_EX); - - return flock(fd, LOCK_UN); -} - #else /* WIN32 */ + int virFileLock(int fd G_GNUC_UNUSED, bool shared G_GNUC_UNUSED, off_t start G_GNUC_UNUSED, @@ -505,14 +484,6 @@ int virFileUnlock(int fd G_GNUC_UNUSED, } -int virFileFlock(int fd G_GNUC_UNUSED, - bool lock G_GNUC_UNUSED, - bool shared G_GNUC_UNUSED) -{ - errno = ENOSYS; - return -1; -} - #endif /* WIN32 */ diff --git a/src/util/virfile.h b/src/util/virfile.h index cb0e586a7d11..2eec89598f6b 100644 --- a/src/util/virfile.h +++ b/src/util/virfile.h @@ -118,8 +118,6 @@ int virFileLock(int fd, bool shared, off_t start, off_t len, bool waitForLock) int virFileUnlock(int fd, off_t start, off_t len) G_GNUC_NO_INLINE; -int virFileFlock(int fd, bool lock, bool shared); - typedef int (*virFileRewriteFunc)(int fd, const void *opaque); int virFileRewrite(const char *path, mode_t mode, diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 9b78a6cb159b..2129630a190f 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -456,6 +456,8 @@ VIR_ONCE_GLOBAL_INIT(virResctrl); static int virResctrlLockWrite(void) { +#ifndef WIN32 + int fd = open(SYSFS_RESCTRL_PATH, O_RDONLY | O_CLOEXEC); if (fd < 0) { @@ -463,13 +465,20 @@ virResctrlLockWrite(void) return -1; } - if (virFileFlock(fd, true, false) < 0) { + if (flock(fd, LOCK_EX) < 0) { virReportSystemError(errno, "%s", _("Cannot lock resctrl")); VIR_FORCE_CLOSE(fd); return -1; } return fd; + +#else /* WIN32 */ + + virReportSystemError(ENOSYS, "%s", _("Cannot lock resctrl")); + return -1; + +#endif } @@ -484,10 +493,14 @@ virResctrlUnlock(int fd) if (VIR_CLOSE(fd) < 0) { virReportSystemError(errno, "%s", _("Cannot close resctrl")); +#ifndef WIN32 + /* Trying to save the already broken */ - if (virFileFlock(fd, false, false) < 0) + if (flock(fd, LOCK_UN) < 0) virReportSystemError(errno, "%s", _("Cannot unlock resctrl")); +#endif + return -1; } -- 2.28.0
On Wed, 2020-07-29 at 13:43 +0200, Martin Kletzander wrote:
+++ b/src/util/virresctrl.c @@ -456,6 +456,8 @@ VIR_ONCE_GLOBAL_INIT(virResctrl); static int virResctrlLockWrite(void) { +#ifndef WIN32 + int fd = open(SYSFS_RESCTRL_PATH, O_RDONLY | O_CLOEXEC);
if (fd < 0) { @@ -463,13 +465,20 @@ virResctrlLockWrite(void) return -1; }
- if (virFileFlock(fd, true, false) < 0) { + if (flock(fd, LOCK_EX) < 0) { virReportSystemError(errno, "%s", _("Cannot lock resctrl")); VIR_FORCE_CLOSE(fd); return -1; }
return fd; + +#else /* WIN32 */ + + virReportSystemError(ENOSYS, "%s", _("Cannot lock resctrl")); + return -1; + +#endif }
@@ -484,10 +493,14 @@ virResctrlUnlock(int fd) if (VIR_CLOSE(fd) < 0) { virReportSystemError(errno, "%s", _("Cannot close resctrl"));
+#ifndef WIN32 + /* Trying to save the already broken */ - if (virFileFlock(fd, false, false) < 0) + if (flock(fd, LOCK_UN) < 0) virReportSystemError(errno, "%s", _("Cannot unlock resctrl"));
+#endif + return -1; }
So in the end you decided to go for the nuclear option :D I'm okay with the approach, but I would prefer if you stubbed out the functions completely, eg. #ifndef WIN32 static int virResctrlLockWrite(void) { /* do stuff */ } static int virResctrlUnlock(int fd) { /* do stuff */ } #else static int virResctrlLockWrite(void) { virReportSystemError(ENOSYS, "%s", _("resctrl locking is not supported " "on this platform")); return -1; } static int virResctrlUnlock(int fd) { virReportSystemError(ENOSYS, "%s", _("resctrl locking is not supported " "on this platform")); return -1; } #endif Also, since AFAIU resctrl is Linux-only, perhaps a better preprocessor guard would be #ifdef __linux__ so that we (correctly) stub the functions out on FreeBSD and macOS too. -- Andrea Bolognani / Red Hat / Virtualization
On a Wednesday in 2020, Andrea Bolognani wrote:
On Wed, 2020-07-29 at 13:43 +0200, Martin Kletzander wrote:
+++ b/src/util/virresctrl.c @@ -456,6 +456,8 @@ VIR_ONCE_GLOBAL_INIT(virResctrl); static int virResctrlLockWrite(void) { +#ifndef WIN32 + int fd = open(SYSFS_RESCTRL_PATH, O_RDONLY | O_CLOEXEC);
if (fd < 0) { @@ -463,13 +465,20 @@ virResctrlLockWrite(void) return -1; }
- if (virFileFlock(fd, true, false) < 0) { + if (flock(fd, LOCK_EX) < 0) { virReportSystemError(errno, "%s", _("Cannot lock resctrl")); VIR_FORCE_CLOSE(fd); return -1; }
return fd; + +#else /* WIN32 */ + + virReportSystemError(ENOSYS, "%s", _("Cannot lock resctrl")); + return -1; + +#endif }
@@ -484,10 +493,14 @@ virResctrlUnlock(int fd) if (VIR_CLOSE(fd) < 0) { virReportSystemError(errno, "%s", _("Cannot close resctrl"));
+#ifndef WIN32 + /* Trying to save the already broken */ - if (virFileFlock(fd, false, false) < 0) + if (flock(fd, LOCK_UN) < 0) virReportSystemError(errno, "%s", _("Cannot unlock resctrl"));
+#endif + return -1; }
So in the end you decided to go for the nuclear option :D
I'm okay with the approach, but I would prefer if you stubbed out the functions completely, eg.
Yes, please. With that, the whole series: Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
On Wed, Jul 29, 2020 at 02:35:01PM +0200, Ján Tomko wrote:
On a Wednesday in 2020, Andrea Bolognani wrote:
On Wed, 2020-07-29 at 13:43 +0200, Martin Kletzander wrote:
+++ b/src/util/virresctrl.c @@ -456,6 +456,8 @@ VIR_ONCE_GLOBAL_INIT(virResctrl); static int virResctrlLockWrite(void) { +#ifndef WIN32 + int fd = open(SYSFS_RESCTRL_PATH, O_RDONLY | O_CLOEXEC);
if (fd < 0) { @@ -463,13 +465,20 @@ virResctrlLockWrite(void) return -1; }
- if (virFileFlock(fd, true, false) < 0) { + if (flock(fd, LOCK_EX) < 0) { virReportSystemError(errno, "%s", _("Cannot lock resctrl")); VIR_FORCE_CLOSE(fd); return -1; }
return fd; + +#else /* WIN32 */ + + virReportSystemError(ENOSYS, "%s", _("Cannot lock resctrl")); + return -1; + +#endif }
@@ -484,10 +493,14 @@ virResctrlUnlock(int fd) if (VIR_CLOSE(fd) < 0) { virReportSystemError(errno, "%s", _("Cannot close resctrl"));
+#ifndef WIN32 + /* Trying to save the already broken */ - if (virFileFlock(fd, false, false) < 0) + if (flock(fd, LOCK_UN) < 0) virReportSystemError(errno, "%s", _("Cannot unlock resctrl"));
+#endif + return -1; }
So in the end you decided to go for the nuclear option :D
I'm okay with the approach, but I would prefer if you stubbed out the functions completely, eg.
Yes, please.
I guess with two functions it's fine to do it like that. I thought it was one, then it became two, you know the drill.
With that, the whole series:
Reviewed-by: Ján Tomko <jtomko@redhat.com>
Kiitos
Jano
On Wed, Jul 29, 2020 at 02:20:50PM +0200, Andrea Bolognani wrote:
On Wed, 2020-07-29 at 13:43 +0200, Martin Kletzander wrote:
+++ b/src/util/virresctrl.c @@ -456,6 +456,8 @@ VIR_ONCE_GLOBAL_INIT(virResctrl); static int virResctrlLockWrite(void) { +#ifndef WIN32 + int fd = open(SYSFS_RESCTRL_PATH, O_RDONLY | O_CLOEXEC);
if (fd < 0) { @@ -463,13 +465,20 @@ virResctrlLockWrite(void) return -1; }
- if (virFileFlock(fd, true, false) < 0) { + if (flock(fd, LOCK_EX) < 0) { virReportSystemError(errno, "%s", _("Cannot lock resctrl")); VIR_FORCE_CLOSE(fd); return -1; }
return fd; + +#else /* WIN32 */ + + virReportSystemError(ENOSYS, "%s", _("Cannot lock resctrl")); + return -1; + +#endif }
@@ -484,10 +493,14 @@ virResctrlUnlock(int fd) if (VIR_CLOSE(fd) < 0) { virReportSystemError(errno, "%s", _("Cannot close resctrl"));
+#ifndef WIN32 + /* Trying to save the already broken */ - if (virFileFlock(fd, false, false) < 0) + if (flock(fd, LOCK_UN) < 0) virReportSystemError(errno, "%s", _("Cannot unlock resctrl"));
+#endif + return -1; }
So in the end you decided to go for the nuclear option :D
I'm okay with the approach, but I would prefer if you stubbed out the functions completely, eg.
#ifndef WIN32
static int virResctrlLockWrite(void) { /* do stuff */ }
static int virResctrlUnlock(int fd) { /* do stuff */ }
#else
static int virResctrlLockWrite(void) { virReportSystemError(ENOSYS, "%s", _("resctrl locking is not supported " "on this platform")); return -1; }
static int virResctrlUnlock(int fd) { virReportSystemError(ENOSYS, "%s", _("resctrl locking is not supported " "on this platform")); return -1; }
#endif
Also, since AFAIU resctrl is Linux-only, perhaps a better preprocessor guard would be
#ifdef __linux__
so that we (correctly) stub the functions out on FreeBSD and macOS too.
Well, in case they get any similar feature (since it is actually Intel-only AFAIK) I just wanted to stub it out only for platforms where flock(2) is not available.
-- Andrea Bolognani / Red Hat / Virtualization
On Wed, 2020-07-29 at 15:10 +0200, Martin Kletzander wrote:
On Wed, Jul 29, 2020 at 02:20:50PM +0200, Andrea Bolognani wrote:
Also, since AFAIU resctrl is Linux-only, perhaps a better preprocessor guard would be
#ifdef __linux__
so that we (correctly) stub the functions out on FreeBSD and macOS too.
Well, in case they get any similar feature (since it is actually Intel-only AFAIK) I just wanted to stub it out only for platforms where flock(2) is not available.
Right, I confused Intel-only with Linux-only, sorry. The preprocessor guard is fine as it is then. -- Andrea Bolognani / Red Hat / Virtualization
There is no distinction between Read/Write locks for resctrl from libvirt's point of view any more. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/util/virresctrl.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/util/virresctrl.c b/src/util/virresctrl.c index 2129630a190f..954e4fd506f7 100644 --- a/src/util/virresctrl.c +++ b/src/util/virresctrl.c @@ -454,7 +454,7 @@ VIR_ONCE_GLOBAL_INIT(virResctrl); /* Common functions */ static int -virResctrlLockWrite(void) +virResctrlLock(void) { #ifndef WIN32 @@ -2404,7 +2404,7 @@ virResctrlAllocCreate(virResctrlInfoPtr resctrl, if (STREQ(alloc->path, SYSFS_RESCTRL_PATH)) return 0; - lockfd = virResctrlLockWrite(); + lockfd = virResctrlLock(); if (lockfd < 0) goto cleanup; @@ -2597,7 +2597,7 @@ virResctrlMonitorCreate(virResctrlMonitorPtr monitor, if (virResctrlMonitorDeterminePath(monitor, machinename) < 0) return -1; - lockfd = virResctrlLockWrite(); + lockfd = virResctrlLock(); if (lockfd < 0) return -1; -- 2.28.0
On Wed, 2020-07-29 at 13:43 +0200, Martin Kletzander wrote:
There is no distinction between Read/Write locks for resctrl from libvirt's point of view any more.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- src/util/virresctrl.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
Reviewed-by: Andrea Bolognani <abologna@redhat.com> -- Andrea Bolognani / Red Hat / Virtualization
participants (3)
-
Andrea Bolognani -
Ján Tomko -
Martin Kletzander