They are wrappers of POSIX fcntl "file private locking", with a
convenient "try lock" wrapper implemented with F_OFD_GETLK.
Signed-off-by: Fam Zheng <famz@redhat.com>
Reviewed-by: Max Reitz <mreitz@redhat.com>
---
include/qemu/osdep.h | 3 +++
util/osdep.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 51 insertions(+)
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 122ff06..1c9f5e2 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -341,6 +341,9 @@ int qemu_close(int fd);
#ifndef _WIN32
int qemu_dup(int fd);
#endif
+int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive);
+int qemu_unlock_fd(int fd, int64_t start, int64_t len);
+int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive);
#if defined(__HAIKU__) && defined(__i386__)
#define FMT_pid "%ld"
diff --git a/util/osdep.c b/util/osdep.c
index 06fb1cf..3de4a18 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -140,6 +140,54 @@ static int qemu_parse_fdset(const char *param)
{
return qemu_parse_fd(param);
}
+
+static int qemu_lock_fcntl(int fd, int64_t start, int64_t len, int fl_type)
+{
+#ifdef F_OFD_SETLK
+ int ret;
+ struct flock fl = {
+ .l_whence = SEEK_SET,
+ .l_start = start,
+ .l_len = len,
+ .l_type = fl_type,
+ };
+ ret = fcntl(fd, F_OFD_SETLK, &fl);
+ return ret == -1 ? -errno : 0;
+#else
+ return -ENOTSUP;
+#endif
+}
+
+int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive)
+{
+ return qemu_lock_fcntl(fd, start, len, exclusive ? F_WRLCK : F_RDLCK);
+}
+
+int qemu_unlock_fd(int fd, int64_t start, int64_t len)
+{
+ return qemu_lock_fcntl(fd, start, len, F_UNLCK);
+}
+
+int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive)
+{
+#ifdef F_OFD_SETLK
+ int ret;
+ struct flock fl = {
+ .l_whence = SEEK_SET,
+ .l_start = start,
+ .l_len = len,
+ .l_type = exclusive ? F_WRLCK : F_RDLCK,
+ };
+ ret = fcntl(fd, F_OFD_GETLK, &fl);
+ if (ret == -1) {
+ return -errno;
+ } else {
+ return fl.l_type == F_UNLCK ? 0 : -EAGAIN;
+ }
+#else
+ return -ENOTSUP;
+#endif
+}
#endif
/*
--
2.9.3
Am 26.04.2017 um 05:34 hat Fam Zheng geschrieben:
> They are wrappers of POSIX fcntl "file private locking", with a
> convenient "try lock" wrapper implemented with F_OFD_GETLK.
>
> Signed-off-by: Fam Zheng <famz@redhat.com>
> Reviewed-by: Max Reitz <mreitz@redhat.com>
> ---
> include/qemu/osdep.h | 3 +++
> util/osdep.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 51 insertions(+)
>
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 122ff06..1c9f5e2 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -341,6 +341,9 @@ int qemu_close(int fd);
> #ifndef _WIN32
> int qemu_dup(int fd);
> #endif
> +int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive);
> +int qemu_unlock_fd(int fd, int64_t start, int64_t len);
> +int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive);
For the record: On IRC, I proposed adding something like the following:
#ifndef F_OFD_SETLK
#define F_OFD_SETLK F_SETLK
#define F_OFD_GETLK F_GETLK
#endif
F_OFD_* are still relatively new and e.g. RHEL 7 doesn't support it yet.
Using process-based locks is suboptimal because we can easily lose them
earlier than we want, but it's still better than nothing and covers the
common simple cases.
Kevin
On Wed, 04/26 14:57, Kevin Wolf wrote:
> Am 26.04.2017 um 05:34 hat Fam Zheng geschrieben:
> > They are wrappers of POSIX fcntl "file private locking", with a
> > convenient "try lock" wrapper implemented with F_OFD_GETLK.
> >
> > Signed-off-by: Fam Zheng <famz@redhat.com>
> > Reviewed-by: Max Reitz <mreitz@redhat.com>
> > ---
> > include/qemu/osdep.h | 3 +++
> > util/osdep.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 51 insertions(+)
> >
> > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > index 122ff06..1c9f5e2 100644
> > --- a/include/qemu/osdep.h
> > +++ b/include/qemu/osdep.h
> > @@ -341,6 +341,9 @@ int qemu_close(int fd);
> > #ifndef _WIN32
> > int qemu_dup(int fd);
> > #endif
> > +int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive);
> > +int qemu_unlock_fd(int fd, int64_t start, int64_t len);
> > +int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive);
>
> For the record: On IRC, I proposed adding something like the following:
>
> #ifndef F_OFD_SETLK
> #define F_OFD_SETLK F_SETLK
> #define F_OFD_GETLK F_GETLK
> #endif
>
> F_OFD_* are still relatively new and e.g. RHEL 7 doesn't support it yet.
> Using process-based locks is suboptimal because we can easily lose them
> earlier than we want, but it's still better than nothing and covers the
> common simple cases.
Yes, we should add that. But I'd prefer:
#ifdef F_OFD_SETLK
#define QEMU_SETLK F_OFD_SETLK
#define QEMU_GETLK F_OFD_GETLK
#else
#define QEMU_SETLK F_SETLK
#define QEMU_GETLK F_GETLK
#endif
to avoid "abusing" the macro name.
Another question is whether we should print a warning to make users aware? Even
the test case in patch 21 can see three "lock losses" on RHEL with posix lock,
and there are way more corner cases, I believe.
We can print a warning to stderr in raw_open_common when F_OFD_GETLK is not
available, I think.
Fam
Am 26.04.2017 um 15:20 hat Fam Zheng geschrieben: > On Wed, 04/26 14:57, Kevin Wolf wrote: > > Am 26.04.2017 um 05:34 hat Fam Zheng geschrieben: > > > They are wrappers of POSIX fcntl "file private locking", with a > > > convenient "try lock" wrapper implemented with F_OFD_GETLK. > > > > > > Signed-off-by: Fam Zheng <famz@redhat.com> > > > Reviewed-by: Max Reitz <mreitz@redhat.com> > > > --- > > > include/qemu/osdep.h | 3 +++ > > > util/osdep.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ > > > 2 files changed, 51 insertions(+) > > > > > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h > > > index 122ff06..1c9f5e2 100644 > > > --- a/include/qemu/osdep.h > > > +++ b/include/qemu/osdep.h > > > @@ -341,6 +341,9 @@ int qemu_close(int fd); > > > #ifndef _WIN32 > > > int qemu_dup(int fd); > > > #endif > > > +int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive); > > > +int qemu_unlock_fd(int fd, int64_t start, int64_t len); > > > +int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive); > > > > For the record: On IRC, I proposed adding something like the following: > > > > #ifndef F_OFD_SETLK > > #define F_OFD_SETLK F_SETLK > > #define F_OFD_GETLK F_GETLK > > #endif > > > > F_OFD_* are still relatively new and e.g. RHEL 7 doesn't support it yet. > > Using process-based locks is suboptimal because we can easily lose them > > earlier than we want, but it's still better than nothing and covers the > > common simple cases. > > Yes, we should add that. But I'd prefer: > > #ifdef F_OFD_SETLK > #define QEMU_SETLK F_OFD_SETLK > #define QEMU_GETLK F_OFD_GETLK > #else > #define QEMU_SETLK F_SETLK > #define QEMU_GETLK F_GETLK > #endif > > to avoid "abusing" the macro name. Makes sense. > Another question is whether we should print a warning to make users > aware? Even the test case in patch 21 can see three "lock losses" on > RHEL with posix lock, and there are way more corner cases, I believe. > > We can print a warning to stderr in raw_open_common when F_OFD_GETLK > is not available, I think. Yes, this sounds reasonable, too. Dependent on s->use_locks, I guess. Kevin
On Wed, Apr 26, 2017 at 09:20:28PM +0800, Fam Zheng wrote: > On Wed, 04/26 14:57, Kevin Wolf wrote: > > Am 26.04.2017 um 05:34 hat Fam Zheng geschrieben: > > > They are wrappers of POSIX fcntl "file private locking", with a > > > convenient "try lock" wrapper implemented with F_OFD_GETLK. > > > > > > Signed-off-by: Fam Zheng <famz@redhat.com> > > > Reviewed-by: Max Reitz <mreitz@redhat.com> > > > --- > > > include/qemu/osdep.h | 3 +++ > > > util/osdep.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ > > > 2 files changed, 51 insertions(+) > > > > > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h > > > index 122ff06..1c9f5e2 100644 > > > --- a/include/qemu/osdep.h > > > +++ b/include/qemu/osdep.h > > > @@ -341,6 +341,9 @@ int qemu_close(int fd); > > > #ifndef _WIN32 > > > int qemu_dup(int fd); > > > #endif > > > +int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive); > > > +int qemu_unlock_fd(int fd, int64_t start, int64_t len); > > > +int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive); > > > > For the record: On IRC, I proposed adding something like the following: > > > > #ifndef F_OFD_SETLK > > #define F_OFD_SETLK F_SETLK > > #define F_OFD_GETLK F_GETLK > > #endif > > > > F_OFD_* are still relatively new and e.g. RHEL 7 doesn't support it yet. > > Using process-based locks is suboptimal because we can easily lose them > > earlier than we want, but it's still better than nothing and covers the > > common simple cases. > > Yes, we should add that. But I'd prefer: > > #ifdef F_OFD_SETLK > #define QEMU_SETLK F_OFD_SETLK > #define QEMU_GETLK F_OFD_GETLK > #else > #define QEMU_SETLK F_SETLK > #define QEMU_GETLK F_GETLK > #endif > > to avoid "abusing" the macro name. > > Another question is whether we should print a warning to make users aware? Even > the test case in patch 21 can see three "lock losses" on RHEL with posix lock, > and there are way more corner cases, I believe. Is it possible to use F_GETLK to detect when we have a missing lock, then add assertions in various key places that call F_GETLK to validate hat the lock still exists & print an warning. I don't like the idea that downstream would be running with locking enabled, but silently loosing locks with no indication at all that this happened, especially when upstream development won't be testing this since those devs will use the F_OFD_SETLK instead. Regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
On Wed, 04/26 15:29, Daniel P. Berrange wrote: > On Wed, Apr 26, 2017 at 09:20:28PM +0800, Fam Zheng wrote: > > On Wed, 04/26 14:57, Kevin Wolf wrote: > > > Am 26.04.2017 um 05:34 hat Fam Zheng geschrieben: > > > > They are wrappers of POSIX fcntl "file private locking", with a > > > > convenient "try lock" wrapper implemented with F_OFD_GETLK. > > > > > > > > Signed-off-by: Fam Zheng <famz@redhat.com> > > > > Reviewed-by: Max Reitz <mreitz@redhat.com> > > > > --- > > > > include/qemu/osdep.h | 3 +++ > > > > util/osdep.c | 48 ++++++++++++++++++++++++++++++++++++++++++++++++ > > > > 2 files changed, 51 insertions(+) > > > > > > > > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h > > > > index 122ff06..1c9f5e2 100644 > > > > --- a/include/qemu/osdep.h > > > > +++ b/include/qemu/osdep.h > > > > @@ -341,6 +341,9 @@ int qemu_close(int fd); > > > > #ifndef _WIN32 > > > > int qemu_dup(int fd); > > > > #endif > > > > +int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive); > > > > +int qemu_unlock_fd(int fd, int64_t start, int64_t len); > > > > +int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive); > > > > > > For the record: On IRC, I proposed adding something like the following: > > > > > > #ifndef F_OFD_SETLK > > > #define F_OFD_SETLK F_SETLK > > > #define F_OFD_GETLK F_GETLK > > > #endif > > > > > > F_OFD_* are still relatively new and e.g. RHEL 7 doesn't support it yet. > > > Using process-based locks is suboptimal because we can easily lose them > > > earlier than we want, but it's still better than nothing and covers the > > > common simple cases. > > > > Yes, we should add that. But I'd prefer: > > > > #ifdef F_OFD_SETLK > > #define QEMU_SETLK F_OFD_SETLK > > #define QEMU_GETLK F_OFD_GETLK > > #else > > #define QEMU_SETLK F_SETLK > > #define QEMU_GETLK F_GETLK > > #endif > > > > to avoid "abusing" the macro name. > > > > Another question is whether we should print a warning to make users aware? Even > > the test case in patch 21 can see three "lock losses" on RHEL with posix lock, > > and there are way more corner cases, I believe. > > Is it possible to use F_GETLK to detect when we have a missing lock, then > add assertions in various key places that call F_GETLK to validate hat > the lock still exists & print an warning. Possibly, but it's ugly to do, and probably isn't worth the effort in finding all the places where losses happen and where locks should be checked against losses. Besides, maybe I'm missing something, it's also hard to use F_GETLK to find lost locks, because either it's lost or not, it can return F_UNLCK. > > I don't like the idea that downstream would be running with locking enabled, > but silently loosing locks with no indication at all that this happened, > especially when upstream development won't be testing this since those devs > will use the F_OFD_SETLK instead. Yes, see my reply to Kevin on the last patch: we could default to disabled if there is no OFD lock (via documented locking=auto, no magic), and avoid surprising users. Fam
© 2016 - 2026 Red Hat, Inc.