15.12.2020 13:53, Li Feng wrote:
This patch addresses this issue:
When accessing a volume on an NFS filesystem without supporting the file lock,
tools, like qemu-img, will complain "Failed to lock byte 100".
In the original code, the qemu_has_ofd_lock will test the lock on the
"/dev/null" pseudo-file. Actually, the file.locking is per-drive property,
which depends on the underlay filesystem.
In this patch, add a new 'qemu_has_file_lock' to detect whether the
file supports the file lock. And disable the lock when the underlay file
system doesn't support locks.
Signed-off-by: Li Feng <fengli@smartx.com>
---
v5: simplify the code.
v4: use the fd as the qemu_has_file_lock argument.
v3: don't call the qemu_has_ofd_lock, use a new function instead.
v2: remove the refactoring.
---
block/file-posix.c | 61 +++++++++++++++++++++++---------------------
include/qemu/osdep.h | 1 +
util/osdep.c | 14 ++++++++++
3 files changed, 47 insertions(+), 29 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index 806764f7e3..4e00111031 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -584,6 +584,21 @@ static int raw_open_common(BlockDriverState *bs, QDict
*options,
s->use_linux_io_uring = (aio == BLOCKDEV_AIO_OPTIONS_IO_URING);
#endif
+ s->open_flags = open_flags;
+ raw_parse_flags(bdrv_flags, &s->open_flags, false);
+
+ s->fd = -1;
+ fd = qemu_open(filename, s->open_flags, errp);
+ ret = fd < 0 ? -errno : 0;
+
+ if (ret < 0) {
+ if (ret == -EROFS) {
+ ret = -EACCES;
+ }
+ goto fail;
+ }
+ s->fd = fd;
+
locking = qapi_enum_parse(&OnOffAuto_lookup,
qemu_opt_get(opts, "locking"),
ON_OFF_AUTO_AUTO, &local_err);
@@ -606,7 +621,7 @@ static int raw_open_common(BlockDriverState *bs, QDict
*options,
s->use_lock = false;
break;
In case of ON_OFF_AUTO_ON: we do check qemu_has_ofd_lock() and print a warning.
Probably we can also check new qemu_has_file_lock() and just do early fail, not
waiting for permissions update..
case ON_OFF_AUTO_AUTO:
- s->use_lock = qemu_has_ofd_lock();
+ s->use_lock = qemu_has_file_lock(s->fd) && qemu_has_ofd_lock();
break;
default:
abort();
@@ -625,22 +640,6 @@ static int raw_open_common(BlockDriverState *bs, QDict
*options,
s->drop_cache = qemu_opt_get_bool(opts, "drop-cache", true);
s->check_cache_dropped = qemu_opt_get_bool(opts,
"x-check-cache-dropped",
false);
-
- s->open_flags = open_flags;
- raw_parse_flags(bdrv_flags, &s->open_flags, false);
-
- s->fd = -1;
- fd = qemu_open(filename, s->open_flags, errp);
- ret = fd < 0 ? -errno : 0;
-
- if (ret < 0) {
- if (ret == -EROFS) {
- ret = -EACCES;
- }
- goto fail;
- }
- s->fd = fd;
-
/* 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);
@@ -2388,6 +2387,7 @@ raw_co_create(BlockdevCreateOptions *options, Error
**errp)
int fd;
uint64_t perm, shared;
int result = 0;
+ bool use_lock;
/* Validate options and set default values */
assert(options->driver == BLOCKDEV_DRIVER_FILE);
@@ -2428,19 +2428,22 @@ raw_co_create(BlockdevCreateOptions *options, Error
**errp)
perm = BLK_PERM_WRITE | BLK_PERM_RESIZE;
shared = BLK_PERM_ALL & ~BLK_PERM_RESIZE;
- /* Step one: Take locks */
- result = raw_apply_lock_bytes(NULL, fd, perm, ~shared, false, errp);
- if (result < 0) {
- goto out_close;
- }
+ use_lock = qemu_has_file_lock(fd);
+ if (use_lock) {
+ /* Step one: Take locks */
+ result = raw_apply_lock_bytes(NULL, fd, perm, ~shared, false, errp);
+ if (result < 0) {
+ goto out_close;
+ }
- /* Step two: Check that nobody else has taken conflicting locks */
- result = raw_check_lock_bytes(fd, perm, shared, errp);
- if (result < 0) {
- error_append_hint(errp,
- "Is another process using the image [%s]?\n",
- file_opts->filename);
- goto out_unlock;
+ /* Step two: Check that nobody else has taken conflicting locks */
+ result = raw_check_lock_bytes(fd, perm, shared, errp);
+ if (result < 0) {
+ error_append_hint(errp,
+ "Is another process using the image [%s]?\n",
+ file_opts->filename);
+ goto out_unlock;
+ }
}
In raw_co_create(), I think you should also update code under "out_unlock:", we
shouldn't
call raw_apply_lock_bytes(), when use_lock is false.
Another thing is call to raw_apply_lock_bytes() in raw_check_perm(). Looks like a
preexisting bug. Why don't we check for s->use_lock?
/* Clear the file by truncating it to 0 */
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index f9ec8c84e9..c7587be99d 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -513,6 +513,7 @@ 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_file_lock(int fd);
#endif
#if defined(__HAIKU__) && defined(__i386__)
diff --git a/util/osdep.c b/util/osdep.c
index 66d01b9160..dee1f076da 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -225,6 +225,20 @@ static void qemu_probe_lock_ops(void)
}
}
+bool qemu_has_file_lock(int fd)
+{
+ int ret;
+ struct flock fl = {
+ .l_whence = SEEK_SET,
+ .l_start = 0,
+ .l_len = 0,
+ .l_type = F_WRLCK,
+ };
+
+ ret = fcntl(fd, F_GETLK, &fl);
I think we need
qemu_probe_lock_ops();
ret = fcntl(fd, fcntl_op_getlk, &fl);
pattern instead, like in qemu_lock_fd_test(). Otherwise, what we check may
differ with what we are going to use.