[PATCH 1/2] file-posix: Use OFD lock only if the filesystem supports the lock

* [PATCH 1/2] file-posix: Use OFD lock only if the filesystem supports the lock
@ 2020-11-06  4:01 Masayoshi Mizuma
  2020-11-06  4:01 ` [PATCH 2/2] tests/test-image-locking: Pass the fd to the argument of qemu_has_ofd_lock() Masayoshi Mizuma
                   ` (3 more replies)
  0 siblings, 4 replies; 15+ messages in thread
From: Masayoshi Mizuma @ 2020-11-06  4:01 UTC (permalink / raw)
  To: qemu-devel; +Cc: Masayoshi Mizuma, Masayoshi Mizuma

From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>

locking=auto doesn't work if the filesystem doesn't support OFD lock.
In that situation, following error happens:

  qemu-system-x86_64: -blockdev driver=qcow2,node-name=disk,file.driver=file,file.filename=/mnt/guest.qcow2,file.locking=auto: Failed to lock byte 100

qemu_probe_lock_ops() judges whether qemu can use OFD lock
or not with doing fcntl(F_OFD_GETLK) to /dev/null. So the
error happens if /dev/null supports OFD lock, but the filesystem
doesn't support the lock.

Lock the actual file, not /dev/null, using F_OFD_SETLK and if that
fails, then fallback to F_SETLK.

Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
---
 block/file-posix.c   |  56 ++++++++--------
 include/qemu/osdep.h |   2 +-
 util/osdep.c         | 149 ++++++++++++++++++++++++++++---------------
 3 files changed, 125 insertions(+), 82 deletions(-)

diff --git a/block/file-posix.c b/block/file-posix.c
index c63926d592..a568dbf125 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -584,34 +584,6 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
     s->use_linux_io_uring = (aio == BLOCKDEV_AIO_OPTIONS_IO_URING);
 #endif
 
-    locking = qapi_enum_parse(&OnOffAuto_lookup,
-                              qemu_opt_get(opts, "locking"),
-                              ON_OFF_AUTO_AUTO, &local_err);
-    if (local_err) {
-        error_propagate(errp, local_err);
-        ret = -EINVAL;
-        goto fail;
-    }
-    switch (locking) {
-    case ON_OFF_AUTO_ON:
-        s->use_lock = true;
-        if (!qemu_has_ofd_lock()) {
-            warn_report("File lock requested but OFD locking syscall is "
-                        "unavailable, falling back to POSIX file locks");
-            error_printf("Due to the implementation, locks can be lost "
-                         "unexpectedly.\n");
-        }
-        break;
-    case ON_OFF_AUTO_OFF:
-        s->use_lock = false;
-        break;
-    case ON_OFF_AUTO_AUTO:
-        s->use_lock = qemu_has_ofd_lock();
-        break;
-    default:
-        abort();
-    }
-
     str = qemu_opt_get(opts, "pr-manager");
     if (str) {
         s->pr_mgr = pr_manager_lookup(str, &local_err);
@@ -641,6 +613,34 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
     }
     s->fd = fd;
 
+    locking = qapi_enum_parse(&OnOffAuto_lookup,
+                              qemu_opt_get(opts, "locking"),
+                              ON_OFF_AUTO_AUTO, &local_err);
+    if (local_err) {
+        error_propagate(errp, local_err);
+        ret = -EINVAL;
+        goto fail;
+    }
+    switch (locking) {
+    case ON_OFF_AUTO_ON:
+        s->use_lock = true;
+        if (!qemu_has_ofd_lock(s->fd)) {
+            warn_report("File lock requested but OFD locking syscall is "
+                        "unavailable, falling back to POSIX file locks");
+            error_printf("Due to the implementation, locks can be lost "
+                         "unexpectedly.\n");
+        }
+        break;
+    case ON_OFF_AUTO_OFF:
+        s->use_lock = false;
+        break;
+    case ON_OFF_AUTO_AUTO:
+        s->use_lock = qemu_has_ofd_lock(s->fd);
+        break;
+    default:
+        abort();
+    }
+
     /* Check s->open_flags rather than bdrv_flags due to auto-read-only */
     if (s->open_flags & O_RDWR) {
         ret = check_hdev_writable(s->fd);
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index f9ec8c84e9..222138a81a 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -512,7 +512,7 @@ int qemu_dup(int fd);
 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);
-bool qemu_has_ofd_lock(void);
+bool qemu_has_ofd_lock(int orig_fd);
 #endif
 
 #if defined(__HAIKU__) && defined(__i386__)
diff --git a/util/osdep.c b/util/osdep.c
index 66d01b9160..454e8ef9f4 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -117,9 +117,6 @@ int qemu_mprotect_none(void *addr, size_t size)
 
 #ifndef _WIN32
 
-static int fcntl_op_setlk = -1;
-static int fcntl_op_getlk = -1;
-
 /*
  * Dups an fd and sets the flags
  */
@@ -187,68 +184,87 @@ static int qemu_parse_fdset(const char *param)
     return qemu_parse_fd(param);
 }
 
-static void qemu_probe_lock_ops(void)
+bool qemu_has_ofd_lock(int orig_fd)
 {
-    if (fcntl_op_setlk == -1) {
 #ifdef F_OFD_SETLK
-        int fd;
-        int ret;
-        struct flock fl = {
-            .l_whence = SEEK_SET,
-            .l_start  = 0,
-            .l_len    = 0,
-            .l_type   = F_WRLCK,
-        };
-
-        fd = open("/dev/null", O_RDWR);
-        if (fd < 0) {
+    int fd;
+    int ret;
+    struct flock fl = {
+        .l_whence = SEEK_SET,
+        .l_start  = 0,
+        .l_len    = 0,
+        .l_type   = F_RDLCK,
+    };
+
+    fd = qemu_dup(orig_fd);
+    if (fd >= 0) {
+        ret = fcntl_setfl(fd, O_RDONLY);
+        if (ret) {
             fprintf(stderr,
-                    "Failed to open /dev/null for OFD lock probing: %s\n",
-                    strerror(errno));
-            fcntl_op_setlk = F_SETLK;
-            fcntl_op_getlk = F_GETLK;
-            return;
-        }
-        ret = fcntl(fd, F_OFD_GETLK, &fl);
-        close(fd);
-        if (!ret) {
-            fcntl_op_setlk = F_OFD_SETLK;
-            fcntl_op_getlk = F_OFD_GETLK;
-        } else {
-            fcntl_op_setlk = F_SETLK;
-            fcntl_op_getlk = F_GETLK;
+                    "Failed to fcntl for OFD lock probing.\n");
+            qemu_close(fd);
+            return false;
         }
+    }
+
+    ret = fcntl(fd, F_OFD_GETLK, &fl);
+    qemu_close(fd);
+
+    if (ret == 0) {
+        return true;
+    } else {
+        return false;
+    }
 #else
-        fcntl_op_setlk = F_SETLK;
-        fcntl_op_getlk = F_GETLK;
+    return false;
 #endif
-    }
 }
 
-bool qemu_has_ofd_lock(void)
-{
-    qemu_probe_lock_ops();
 #ifdef F_OFD_SETLK
-    return fcntl_op_setlk == F_OFD_SETLK;
+static int _qemu_lock_fcntl(int fd, struct flock *fl)
+{
+    int ret;
+    bool ofd_lock = true;
+
+    do {
+        if (ofd_lock) {
+            ret = fcntl(fd, F_OFD_SETLK, fl);
+            if ((ret == -1) && (errno == EINVAL)) {
+                ofd_lock = false;
+            }
+        }
+
+        if (!ofd_lock) {
+            /* Fallback to POSIX lock */
+            ret = fcntl(fd, F_SETLK, fl);
+        }
+    } while (ret == -1 && errno == EINTR);
+
+    return ret == -1 ? -errno : 0;
+}
 #else
-    return false;
-#endif
+static int _qemu_lock_fcntl(int fd, struct flock *fl)
+{
+    int ret;
+
+    do {
+        ret = fcntl(fd, F_SETLK, fl);
+    } while (ret == -1 && errno == EINTR);
+
+    return ret == -1 ? -errno : 0;
 }
+#endif
 
 static int qemu_lock_fcntl(int fd, int64_t start, int64_t len, int fl_type)
 {
-    int ret;
     struct flock fl = {
         .l_whence = SEEK_SET,
         .l_start  = start,
         .l_len    = len,
         .l_type   = fl_type,
     };
-    qemu_probe_lock_ops();
-    do {
-        ret = fcntl(fd, fcntl_op_setlk, &fl);
-    } while (ret == -1 && errno == EINTR);
-    return ret == -1 ? -errno : 0;
+
+    return _qemu_lock_fcntl(fd, &fl);
 }
 
 int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive)
@@ -261,22 +277,49 @@ 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
+static int _qemu_lock_fd_test(int fd, struct flock *fl)
 {
     int ret;
+
+    ret = fcntl(fd, F_OFD_GETLK, fl);
+    if ((ret == -1) && (errno != EINVAL)) {
+        return -errno;
+
+    } else if ((ret == -1) && (errno == EINVAL)) {
+        /* Fallback to POSIX lock */
+        ret = fcntl(fd, F_GETLK, fl);
+        if (ret == -1) {
+            return -errno;
+        }
+    }
+
+    return fl->l_type == F_UNLCK ? 0 : -EAGAIN;
+}
+#else
+static int _qemu_lock_fd_test(int fd, struct flock *fl)
+{
+    int ret;
+
+    ret = fcntl(fd, F_GETLK, fl);
+    if (ret == -1) {
+        return -errno;
+    } else {
+        return fl->l_type == F_UNLCK ? 0 : -EAGAIN;
+    }
+}
+#endif
+
+int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive)
+{
     struct flock fl = {
         .l_whence = SEEK_SET,
         .l_start  = start,
         .l_len    = len,
         .l_type   = exclusive ? F_WRLCK : F_RDLCK,
     };
-    qemu_probe_lock_ops();
-    ret = fcntl(fd, fcntl_op_getlk, &fl);
-    if (ret == -1) {
-        return -errno;
-    } else {
-        return fl.l_type == F_UNLCK ? 0 : -EAGAIN;
-    }
+
+    return _qemu_lock_fd_test(fd, &fl);
 }
 #endif
 
-- 
2.27.0



^ permalink raw reply related	[flat|nested] 15+ messages in thread