block/file-posix.c | 32 +++++++++++++++------------- include/qemu/osdep.h | 2 +- tests/test-image-locking.c | 2 +- util/osdep.c | 43 ++++++++++++++++++++++++-------------- 4 files changed, 47 insertions(+), 32 deletions(-)
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, make the 'qemu_has_ofd_lock' with a filename be more generic
and reasonable.
Signed-off-by: Li Feng <fengli@smartx.com>
---
block/file-posix.c | 32 +++++++++++++++-------------
include/qemu/osdep.h | 2 +-
tests/test-image-locking.c | 2 +-
util/osdep.c | 43 ++++++++++++++++++++++++--------------
4 files changed, 47 insertions(+), 32 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index 806764f7e3..03be1b188c 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -595,7 +595,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
switch (locking) {
case ON_OFF_AUTO_ON:
s->use_lock = true;
- if (!qemu_has_ofd_lock()) {
+ if (!qemu_has_ofd_lock(filename)) {
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 "
@@ -606,7 +606,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
s->use_lock = false;
break;
case ON_OFF_AUTO_AUTO:
- s->use_lock = qemu_has_ofd_lock();
+ s->use_lock = qemu_has_ofd_lock(filename);
break;
default:
abort();
@@ -2388,6 +2388,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 +2429,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_ofd_lock(file_opts->filename);
+ 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;
+ }
}
/* Clear the file by truncating it to 0 */
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index f9ec8c84e9..349adad465 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(const char *filename);
#endif
#if defined(__HAIKU__) && defined(__i386__)
diff --git a/tests/test-image-locking.c b/tests/test-image-locking.c
index ba057bd66c..3e80246081 100644
--- a/tests/test-image-locking.c
+++ b/tests/test-image-locking.c
@@ -149,7 +149,7 @@ int main(int argc, char **argv)
g_test_init(&argc, &argv, NULL);
- if (qemu_has_ofd_lock()) {
+ if (qemu_has_ofd_lock(NULL)) {
g_test_add_func("/image-locking/basic", test_image_locking_basic);
g_test_add_func("/image-locking/set-perm-abort", test_set_perm_abort);
}
diff --git a/util/osdep.c b/util/osdep.c
index 66d01b9160..e7e502edd1 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -42,6 +42,7 @@ extern int madvise(char *, size_t, int);
static bool fips_enabled = false;
static const char *hw_version = QEMU_HW_VERSION;
+static const char *null_device = "/dev/null";
int socket_set_cork(int fd, int v)
{
@@ -187,11 +188,10 @@ static int qemu_parse_fdset(const char *param)
return qemu_parse_fd(param);
}
-static void qemu_probe_lock_ops(void)
+static void qemu_probe_lock_ops_fd(int fd)
{
if (fcntl_op_setlk == -1) {
#ifdef F_OFD_SETLK
- int fd;
int ret;
struct flock fl = {
.l_whence = SEEK_SET,
@@ -200,17 +200,7 @@ static void qemu_probe_lock_ops(void)
.l_type = F_WRLCK,
};
- fd = open("/dev/null", O_RDWR);
- if (fd < 0) {
- 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;
@@ -225,9 +215,30 @@ static void qemu_probe_lock_ops(void)
}
}
-bool qemu_has_ofd_lock(void)
+static void qemu_probe_lock_ops(const char *filename)
+{
+ int fd;
+ if (filename) {
+ fd = open(filename, O_RDWR);
+ } else {
+ fd = open(null_device, O_RDONLY);
+ }
+ if (fd < 0) {
+ fprintf(stderr,
+ "Failed to open %s for OFD lock probing: %s\n",
+ filename ? filename : null_device,
+ strerror(errno));
+ fcntl_op_setlk = F_SETLK;
+ fcntl_op_getlk = F_GETLK;
+ return;
+ }
+ qemu_probe_lock_ops_fd(fd);
+ close(fd);
+}
+
+bool qemu_has_ofd_lock(const char *filename)
{
- qemu_probe_lock_ops();
+ qemu_probe_lock_ops(filename);
#ifdef F_OFD_SETLK
return fcntl_op_setlk == F_OFD_SETLK;
#else
@@ -244,7 +255,7 @@ static int qemu_lock_fcntl(int fd, int64_t start, int64_t len, int fl_type)
.l_len = len,
.l_type = fl_type,
};
- qemu_probe_lock_ops();
+ qemu_probe_lock_ops_fd(fd);
do {
ret = fcntl(fd, fcntl_op_setlk, &fl);
} while (ret == -1 && errno == EINTR);
@@ -270,7 +281,7 @@ int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive)
.l_len = len,
.l_type = exclusive ? F_WRLCK : F_RDLCK,
};
- qemu_probe_lock_ops();
+ qemu_probe_lock_ops_fd(fd);
ret = fcntl(fd, fcntl_op_getlk, &fl);
if (ret == -1) {
return -errno;
--
2.24.3
On Tue, Dec 08, 2020 at 08:59:37PM +0800, 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.
IIUC, the problem you're describing is one of whether the filesystem
supports fcntl locking at all, which is indeed a per-FS check.
The QEMU code being changed though is just about detecting whether
the host OS supports OFD to not, which is supposed to be a kernel
level feature applied universally to all FS types.
>
> In this patch, make the 'qemu_has_ofd_lock' with a filename be more generic
> and reasonable.
>
> Signed-off-by: Li Feng <fengli@smartx.com>
> ---
> block/file-posix.c | 32 +++++++++++++++-------------
> include/qemu/osdep.h | 2 +-
> tests/test-image-locking.c | 2 +-
> util/osdep.c | 43 ++++++++++++++++++++++++--------------
> 4 files changed, 47 insertions(+), 32 deletions(-)
>
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 806764f7e3..03be1b188c 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -595,7 +595,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
> switch (locking) {
> case ON_OFF_AUTO_ON:
> s->use_lock = true;
> - if (!qemu_has_ofd_lock()) {
> + if (!qemu_has_ofd_lock(filename)) {
> 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 "
> @@ -606,7 +606,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
> s->use_lock = false;
> break;
> case ON_OFF_AUTO_AUTO:
> - s->use_lock = qemu_has_ofd_lock();
> + s->use_lock = qemu_has_ofd_lock(filename);
> break;
> default:
> abort();
> @@ -2388,6 +2388,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 +2429,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_ofd_lock(file_opts->filename);
> + 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;
> + }
> }
>
> /* Clear the file by truncating it to 0 */
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index f9ec8c84e9..349adad465 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(const char *filename);
> #endif
>
> #if defined(__HAIKU__) && defined(__i386__)
> diff --git a/tests/test-image-locking.c b/tests/test-image-locking.c
> index ba057bd66c..3e80246081 100644
> --- a/tests/test-image-locking.c
> +++ b/tests/test-image-locking.c
> @@ -149,7 +149,7 @@ int main(int argc, char **argv)
>
> g_test_init(&argc, &argv, NULL);
>
> - if (qemu_has_ofd_lock()) {
> + if (qemu_has_ofd_lock(NULL)) {
> g_test_add_func("/image-locking/basic", test_image_locking_basic);
> g_test_add_func("/image-locking/set-perm-abort", test_set_perm_abort);
> }
> diff --git a/util/osdep.c b/util/osdep.c
> index 66d01b9160..e7e502edd1 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -42,6 +42,7 @@ extern int madvise(char *, size_t, int);
> static bool fips_enabled = false;
>
> static const char *hw_version = QEMU_HW_VERSION;
> +static const char *null_device = "/dev/null";
>
> int socket_set_cork(int fd, int v)
> {
> @@ -187,11 +188,10 @@ static int qemu_parse_fdset(const char *param)
> return qemu_parse_fd(param);
> }
>
> -static void qemu_probe_lock_ops(void)
> +static void qemu_probe_lock_ops_fd(int fd)
> {
> if (fcntl_op_setlk == -1) {
> #ifdef F_OFD_SETLK
> - int fd;
> int ret;
> struct flock fl = {
> .l_whence = SEEK_SET,
> @@ -200,17 +200,7 @@ static void qemu_probe_lock_ops(void)
> .l_type = F_WRLCK,
> };
>
> - fd = open("/dev/null", O_RDWR);
> - if (fd < 0) {
> - 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;
> @@ -225,9 +215,30 @@ static void qemu_probe_lock_ops(void)
> }
> }
>
> -bool qemu_has_ofd_lock(void)
> +static void qemu_probe_lock_ops(const char *filename)
> +{
> + int fd;
> + if (filename) {
> + fd = open(filename, O_RDWR);
> + } else {
> + fd = open(null_device, O_RDONLY);
> + }
> + if (fd < 0) {
> + fprintf(stderr,
> + "Failed to open %s for OFD lock probing: %s\n",
> + filename ? filename : null_device,
> + strerror(errno));
> + fcntl_op_setlk = F_SETLK;
> + fcntl_op_getlk = F_GETLK;
> + return;
> + }
> + qemu_probe_lock_ops_fd(fd);
> + close(fd);
> +}
> +
This method now does a test whose results will vary based on the
filename passed in, but it is updating a global variable to say
whether to use OFD locks. This is looks badly broken when using
files across different filesystems.
IMHO the raw_co_create method just needs to use a dedicated method
to check whether fcntl locks are supposed, and all this broken
refactoring of the OFD check should be removed.
> +bool qemu_has_ofd_lock(const char *filename)
> {
> - qemu_probe_lock_ops();
> + qemu_probe_lock_ops(filename);
> #ifdef F_OFD_SETLK
> return fcntl_op_setlk == F_OFD_SETLK;
> #else
> @@ -244,7 +255,7 @@ static int qemu_lock_fcntl(int fd, int64_t start, int64_t len, int fl_type)
> .l_len = len,
> .l_type = fl_type,
> };
> - qemu_probe_lock_ops();
> + qemu_probe_lock_ops_fd(fd);
> do {
> ret = fcntl(fd, fcntl_op_setlk, &fl);
> } while (ret == -1 && errno == EINTR);
> @@ -270,7 +281,7 @@ int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive)
> .l_len = len,
> .l_type = exclusive ? F_WRLCK : F_RDLCK,
> };
> - qemu_probe_lock_ops();
> + qemu_probe_lock_ops_fd(fd);
> ret = fcntl(fd, fcntl_op_getlk, &fl);
> if (ret == -1) {
> return -errno;
> --
> 2.24.3
>
>
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 :|
Daniel P. Berrangé <berrange@redhat.com> 于2020年12月8日周二 下午9:45写道：
>
> On Tue, Dec 08, 2020 at 08:59:37PM +0800, 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.
>
> IIUC, the problem you're describing is one of whether the filesystem
> supports fcntl locking at all, which is indeed a per-FS check.
>
> The QEMU code being changed though is just about detecting whether
> the host OS supports OFD to not, which is supposed to be a kernel
> level feature applied universally to all FS types.
>
> >
> > In this patch, make the 'qemu_has_ofd_lock' with a filename be more generic
> > and reasonable.
> >
> > Signed-off-by: Li Feng <fengli@smartx.com>
> > ---
> > block/file-posix.c | 32 +++++++++++++++-------------
> > include/qemu/osdep.h | 2 +-
> > tests/test-image-locking.c | 2 +-
> > util/osdep.c | 43 ++++++++++++++++++++++++--------------
> > 4 files changed, 47 insertions(+), 32 deletions(-)
> >
> > diff --git a/block/file-posix.c b/block/file-posix.c
> > index 806764f7e3..03be1b188c 100644
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
> > @@ -595,7 +595,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
> > switch (locking) {
> > case ON_OFF_AUTO_ON:
> > s->use_lock = true;
> > - if (!qemu_has_ofd_lock()) {
> > + if (!qemu_has_ofd_lock(filename)) {
> > 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 "
> > @@ -606,7 +606,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
> > s->use_lock = false;
> > break;
> > case ON_OFF_AUTO_AUTO:
> > - s->use_lock = qemu_has_ofd_lock();
> > + s->use_lock = qemu_has_ofd_lock(filename);
> > break;
> > default:
> > abort();
> > @@ -2388,6 +2388,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 +2429,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_ofd_lock(file_opts->filename);
> > + 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;
> > + }
> > }
> >
> > /* Clear the file by truncating it to 0 */
> > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > index f9ec8c84e9..349adad465 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(const char *filename);
> > #endif
> >
> > #if defined(__HAIKU__) && defined(__i386__)
> > diff --git a/tests/test-image-locking.c b/tests/test-image-locking.c
> > index ba057bd66c..3e80246081 100644
> > --- a/tests/test-image-locking.c
> > +++ b/tests/test-image-locking.c
> > @@ -149,7 +149,7 @@ int main(int argc, char **argv)
> >
> > g_test_init(&argc, &argv, NULL);
> >
> > - if (qemu_has_ofd_lock()) {
> > + if (qemu_has_ofd_lock(NULL)) {
> > g_test_add_func("/image-locking/basic", test_image_locking_basic);
> > g_test_add_func("/image-locking/set-perm-abort", test_set_perm_abort);
> > }
> > diff --git a/util/osdep.c b/util/osdep.c
> > index 66d01b9160..e7e502edd1 100644
> > --- a/util/osdep.c
> > +++ b/util/osdep.c
> > @@ -42,6 +42,7 @@ extern int madvise(char *, size_t, int);
> > static bool fips_enabled = false;
> >
> > static const char *hw_version = QEMU_HW_VERSION;
> > +static const char *null_device = "/dev/null";
> >
> > int socket_set_cork(int fd, int v)
> > {
> > @@ -187,11 +188,10 @@ static int qemu_parse_fdset(const char *param)
> > return qemu_parse_fd(param);
> > }
> >
> > -static void qemu_probe_lock_ops(void)
> > +static void qemu_probe_lock_ops_fd(int fd)
> > {
> > if (fcntl_op_setlk == -1) {
> > #ifdef F_OFD_SETLK
> > - int fd;
> > int ret;
> > struct flock fl = {
> > .l_whence = SEEK_SET,
> > @@ -200,17 +200,7 @@ static void qemu_probe_lock_ops(void)
> > .l_type = F_WRLCK,
> > };
> >
> > - fd = open("/dev/null", O_RDWR);
> > - if (fd < 0) {
> > - 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;
> > @@ -225,9 +215,30 @@ static void qemu_probe_lock_ops(void)
> > }
> > }
> >
> > -bool qemu_has_ofd_lock(void)
> > +static void qemu_probe_lock_ops(const char *filename)
> > +{
> > + int fd;
> > + if (filename) {
> > + fd = open(filename, O_RDWR);
> > + } else {
> > + fd = open(null_device, O_RDONLY);
> > + }
> > + if (fd < 0) {
> > + fprintf(stderr,
> > + "Failed to open %s for OFD lock probing: %s\n",
> > + filename ? filename : null_device,
> > + strerror(errno));
> > + fcntl_op_setlk = F_SETLK;
> > + fcntl_op_getlk = F_GETLK;
> > + return;
> > + }
> > + qemu_probe_lock_ops_fd(fd);
> > + close(fd);
> > +}
> > +
>
> This method now does a test whose results will vary based on the
> filename passed in, but it is updating a global variable to say
> whether to use OFD locks. This is looks badly broken when using
> files across different filesystems.
Daniel, thanks for your reply.
I understand your concern, however, this global variable actually maybe
not suitable if files are across different filesystems.
>
> IMHO the raw_co_create method just needs to use a dedicated method
> to check whether fcntl locks are supposed, and all this broken
> refactoring of the OFD check should be removed.
>
OK, I will just keep the fixed part in v2.
> > +bool qemu_has_ofd_lock(const char *filename)
> > {
> > - qemu_probe_lock_ops();
> > + qemu_probe_lock_ops(filename);
> > #ifdef F_OFD_SETLK
> > return fcntl_op_setlk == F_OFD_SETLK;
> > #else
> > @@ -244,7 +255,7 @@ static int qemu_lock_fcntl(int fd, int64_t start, int64_t len, int fl_type)
> > .l_len = len,
> > .l_type = fl_type,
> > };
> > - qemu_probe_lock_ops();
> > + qemu_probe_lock_ops_fd(fd);
> > do {
> > ret = fcntl(fd, fcntl_op_setlk, &fl);
> > } while (ret == -1 && errno == EINTR);
> > @@ -270,7 +281,7 @@ int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive)
> > .l_len = len,
> > .l_type = exclusive ? F_WRLCK : F_RDLCK,
> > };
> > - qemu_probe_lock_ops();
> > + qemu_probe_lock_ops_fd(fd);
> > ret = fcntl(fd, fcntl_op_getlk, &fl);
> > if (ret == -1) {
> > return -errno;
> > --
> > 2.24.3
> >
> >
>
> 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 :|
>
Am 08.12.2020 um 13:59 hat Li Feng geschrieben: > 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, make the 'qemu_has_ofd_lock' with a filename be more generic > and reasonable. > > Signed-off-by: Li Feng <fengli@smartx.com> Do you know any way how I could configure either the NFS server or the NFS client such that locking would fail? For any patch related to this, it would be good if I could even test the scenario. For this specific patch, I think Daniel has already provided a good explanation of the fundamental problems it has. Kevin
On Tue, Dec 08, 2020 at 03:38:22PM +0100, Kevin Wolf wrote: > Am 08.12.2020 um 13:59 hat Li Feng geschrieben: > > 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, make the 'qemu_has_ofd_lock' with a filename be more generic > > and reasonable. > > > > Signed-off-by: Li Feng <fengli@smartx.com> > > Do you know any way how I could configure either the NFS server or the > NFS client such that locking would fail? For any patch related to this, > it would be good if I could even test the scenario. One could write a qtest that uses an LD_PRELOAD to replace the standard glibc fcntl() function with one that returns an error for locking commands. 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 :|
Am 09.12.2020 um 10:33 hat Daniel P. Berrangé geschrieben: > On Tue, Dec 08, 2020 at 03:38:22PM +0100, Kevin Wolf wrote: > > Am 08.12.2020 um 13:59 hat Li Feng geschrieben: > > > 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, make the 'qemu_has_ofd_lock' with a filename be more generic > > > and reasonable. > > > > > > Signed-off-by: Li Feng <fengli@smartx.com> > > > > Do you know any way how I could configure either the NFS server or the > > NFS client such that locking would fail? For any patch related to this, > > it would be good if I could even test the scenario. > > One could write a qtest that uses an LD_PRELOAD to replace the standard > glibc fcntl() function with one that returns an error for locking commands. Sounds a bit ugly, but for regression testing it could make sense. However, part of the testing would be to verify that we our checks actually match the kernel code, which this approach couldn't solve. Kevin
Kevin Wolf <kwolf@redhat.com> 于2020年12月10日周四 上午1:43写道： > > Am 09.12.2020 um 10:33 hat Daniel P. Berrangé geschrieben: > > On Tue, Dec 08, 2020 at 03:38:22PM +0100, Kevin Wolf wrote: > > > Am 08.12.2020 um 13:59 hat Li Feng geschrieben: > > > > 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, make the 'qemu_has_ofd_lock' with a filename be more generic > > > > and reasonable. > > > > > > > > Signed-off-by: Li Feng <fengli@smartx.com> > > > > > > Do you know any way how I could configure either the NFS server or the > > > NFS client such that locking would fail? For any patch related to this, > > > it would be good if I could even test the scenario. > > > > One could write a qtest that uses an LD_PRELOAD to replace the standard > > glibc fcntl() function with one that returns an error for locking commands. > > Sounds a bit ugly, but for regression testing it could make sense. > > However, part of the testing would be to verify that we our checks > actually match the kernel code, which this approach couldn't solve. > Hi, Kevin and Daniel. How about this patch? I think it's very straightforward. Except we need a mock syscall test case. > Kevin >
On Thu, Dec 10, 2020 at 10:56:59PM +0800, Li Feng wrote: > Kevin Wolf <kwolf@redhat.com> 于2020年12月10日周四 上午1:43写道： > > > > Am 09.12.2020 um 10:33 hat Daniel P. Berrangé geschrieben: > > > On Tue, Dec 08, 2020 at 03:38:22PM +0100, Kevin Wolf wrote: > > > > Am 08.12.2020 um 13:59 hat Li Feng geschrieben: > > > > > 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, make the 'qemu_has_ofd_lock' with a filename be more generic > > > > > and reasonable. > > > > > > > > > > Signed-off-by: Li Feng <fengli@smartx.com> > > > > > > > > Do you know any way how I could configure either the NFS server or the > > > > NFS client such that locking would fail? For any patch related to this, > > > > it would be good if I could even test the scenario. > > > > > > One could write a qtest that uses an LD_PRELOAD to replace the standard > > > glibc fcntl() function with one that returns an error for locking commands. > > > > Sounds a bit ugly, but for regression testing it could make sense. > > > > However, part of the testing would be to verify that we our checks > > actually match the kernel code, which this approach couldn't solve. > > > Hi, Kevin and Daniel. > > How about this patch? I think it's very straightforward. > Except we need a mock syscall test case. You don't seem to have attached any patch here. 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 :|
My mistake, you are not on the receiver list of my v2. I use the get_maintainer.sh to generate the cc list. I will resend it to you. Daniel P. Berrangé <berrange@redhat.com> 于2020年12月10日周四 下午11:29写道： > > On Thu, Dec 10, 2020 at 10:56:59PM +0800, Li Feng wrote: > > Kevin Wolf <kwolf@redhat.com> 于2020年12月10日周四 上午1:43写道： > > > > > > Am 09.12.2020 um 10:33 hat Daniel P. Berrangé geschrieben: > > > > On Tue, Dec 08, 2020 at 03:38:22PM +0100, Kevin Wolf wrote: > > > > > Am 08.12.2020 um 13:59 hat Li Feng geschrieben: > > > > > > 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, make the 'qemu_has_ofd_lock' with a filename be more generic > > > > > > and reasonable. > > > > > > > > > > > > Signed-off-by: Li Feng <fengli@smartx.com> > > > > > > > > > > Do you know any way how I could configure either the NFS server or the > > > > > NFS client such that locking would fail? For any patch related to this, > > > > > it would be good if I could even test the scenario. > > > > > > > > One could write a qtest that uses an LD_PRELOAD to replace the standard > > > > glibc fcntl() function with one that returns an error for locking commands. > > > > > > Sounds a bit ugly, but for regression testing it could make sense. > > > > > > However, part of the testing would be to verify that we our checks > > > actually match the kernel code, which this approach couldn't solve. > > > > > Hi, Kevin and Daniel. > > > > How about this patch? I think it's very straightforward. > > Except we need a mock syscall test case. > > You don't seem to have attached any patch here. > > 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 Thu, Dec 10, 2020 at 11:53:09PM +0800, Feng Li wrote: > My mistake, you are not on the receiver list of my v2. > I use the get_maintainer.sh to generate the cc list. > I will resend it to you. No, need. I've seen 2, but didn't think you were referring to that as it still has most of the flaws I pointed out in v1 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 :|
Kevin Wolf <kwolf@redhat.com> 于2020年12月8日周二 下午10:38写道： > > Am 08.12.2020 um 13:59 hat Li Feng geschrieben: > > 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, make the 'qemu_has_ofd_lock' with a filename be more generic > > and reasonable. > > > > Signed-off-by: Li Feng <fengli@smartx.com> > > Do you know any way how I could configure either the NFS server or the > NFS client such that locking would fail? For any patch related to this, > it would be good if I could even test the scenario. > Hi Kevin, currently our SmartX ZBS storage NFS server doesn't support the file lock and the lock operation will return failure. I have tried the kernel NFS server, and it works well. I don't have more kinds of NFS servers. > For this specific patch, I think Daniel has already provided a good > explanation of the fundamental problems it has. > > Kevin >
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, make the 'qemu_has_ofd_lock' with a filename be more
generic and reasonable.
Signed-off-by: Li Feng <fengli@smartx.com>
---
v2: remove the refactoring.
---
block/file-posix.c | 32 ++++++++++++++++++--------------
include/qemu/osdep.h | 2 +-
tests/test-image-locking.c | 2 +-
util/osdep.c | 19 ++++++++++++-------
4 files changed, 32 insertions(+), 23 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index 806764f7e3..03be1b188c 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -595,7 +595,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
switch (locking) {
case ON_OFF_AUTO_ON:
s->use_lock = true;
- if (!qemu_has_ofd_lock()) {
+ if (!qemu_has_ofd_lock(filename)) {
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 "
@@ -606,7 +606,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
s->use_lock = false;
break;
case ON_OFF_AUTO_AUTO:
- s->use_lock = qemu_has_ofd_lock();
+ s->use_lock = qemu_has_ofd_lock(filename);
break;
default:
abort();
@@ -2388,6 +2388,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 +2429,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_ofd_lock(file_opts->filename);
+ 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;
+ }
}
/* Clear the file by truncating it to 0 */
diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index f9ec8c84e9..349adad465 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(const char *filename);
#endif
#if defined(__HAIKU__) && defined(__i386__)
diff --git a/tests/test-image-locking.c b/tests/test-image-locking.c
index ba057bd66c..3e80246081 100644
--- a/tests/test-image-locking.c
+++ b/tests/test-image-locking.c
@@ -149,7 +149,7 @@ int main(int argc, char **argv)
g_test_init(&argc, &argv, NULL);
- if (qemu_has_ofd_lock()) {
+ if (qemu_has_ofd_lock(NULL)) {
g_test_add_func("/image-locking/basic", test_image_locking_basic);
g_test_add_func("/image-locking/set-perm-abort", test_set_perm_abort);
}
diff --git a/util/osdep.c b/util/osdep.c
index 66d01b9160..20119aa9ae 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -187,7 +187,7 @@ static int qemu_parse_fdset(const char *param)
return qemu_parse_fd(param);
}
-static void qemu_probe_lock_ops(void)
+static void qemu_probe_lock_ops(const char *filename)
{
if (fcntl_op_setlk == -1) {
#ifdef F_OFD_SETLK
@@ -200,10 +200,15 @@ static void qemu_probe_lock_ops(void)
.l_type = F_WRLCK,
};
- fd = open("/dev/null", O_RDWR);
+ if (filename) {
+ fd = open(filename, O_RDWR);
+ } else {
+ fd = open("/dev/null", O_RDONLY);
+ }
if (fd < 0) {
fprintf(stderr,
- "Failed to open /dev/null for OFD lock probing: %s\n",
+ "Failed to open %s for OFD lock probing: %s\n",
+ filename ? filename : "/dev/null",
strerror(errno));
fcntl_op_setlk = F_SETLK;
fcntl_op_getlk = F_GETLK;
@@ -225,9 +230,9 @@ static void qemu_probe_lock_ops(void)
}
}
-bool qemu_has_ofd_lock(void)
+bool qemu_has_ofd_lock(const char *filename)
{
- qemu_probe_lock_ops();
+ qemu_probe_lock_ops(filename);
#ifdef F_OFD_SETLK
return fcntl_op_setlk == F_OFD_SETLK;
#else
@@ -244,7 +249,7 @@ static int qemu_lock_fcntl(int fd, int64_t start, int64_t len, int fl_type)
.l_len = len,
.l_type = fl_type,
};
- qemu_probe_lock_ops();
+ qemu_probe_lock_ops(NULL);
do {
ret = fcntl(fd, fcntl_op_setlk, &fl);
} while (ret == -1 && errno == EINTR);
@@ -270,7 +275,7 @@ int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive)
.l_len = len,
.l_type = exclusive ? F_WRLCK : F_RDLCK,
};
- qemu_probe_lock_ops();
+ qemu_probe_lock_ops(NULL);
ret = fcntl(fd, fcntl_op_getlk, &fl);
if (ret == -1) {
return -errno;
--
2.24.3
On Wed, Dec 09, 2020 at 12:54:48PM +0800, 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, make the 'qemu_has_ofd_lock' with a filename be more
> generic and reasonable.
>
> Signed-off-by: Li Feng <fengli@smartx.com>
> ---
> v2: remove the refactoring.
> ---
> block/file-posix.c | 32 ++++++++++++++++++--------------
> include/qemu/osdep.h | 2 +-
> tests/test-image-locking.c | 2 +-
> util/osdep.c | 19 ++++++++++++-------
> 4 files changed, 32 insertions(+), 23 deletions(-)
>
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 806764f7e3..03be1b188c 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -595,7 +595,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
> switch (locking) {
> case ON_OFF_AUTO_ON:
> s->use_lock = true;
> - if (!qemu_has_ofd_lock()) {
> + if (!qemu_has_ofd_lock(filename)) {
> 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 "
> @@ -606,7 +606,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
> s->use_lock = false;
> break;
> case ON_OFF_AUTO_AUTO:
> - s->use_lock = qemu_has_ofd_lock();
> + s->use_lock = qemu_has_ofd_lock(filename);
> break;
> default:
> abort();
> @@ -2388,6 +2388,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 +2429,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_ofd_lock(file_opts->filename);
> + 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;
> + }
> }
>
> /* Clear the file by truncating it to 0 */
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index f9ec8c84e9..349adad465 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(const char *filename);
> #endif
>
> #if defined(__HAIKU__) && defined(__i386__)
> diff --git a/tests/test-image-locking.c b/tests/test-image-locking.c
> index ba057bd66c..3e80246081 100644
> --- a/tests/test-image-locking.c
> +++ b/tests/test-image-locking.c
> @@ -149,7 +149,7 @@ int main(int argc, char **argv)
>
> g_test_init(&argc, &argv, NULL);
>
> - if (qemu_has_ofd_lock()) {
> + if (qemu_has_ofd_lock(NULL)) {
> g_test_add_func("/image-locking/basic", test_image_locking_basic);
> g_test_add_func("/image-locking/set-perm-abort", test_set_perm_abort);
> }
> diff --git a/util/osdep.c b/util/osdep.c
> index 66d01b9160..20119aa9ae 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -187,7 +187,7 @@ static int qemu_parse_fdset(const char *param)
> return qemu_parse_fd(param);
> }
>
> -static void qemu_probe_lock_ops(void)
> +static void qemu_probe_lock_ops(const char *filename)
> {
> if (fcntl_op_setlk == -1) {
> #ifdef F_OFD_SETLK
> @@ -200,10 +200,15 @@ static void qemu_probe_lock_ops(void)
> .l_type = F_WRLCK,
> };
>
> - fd = open("/dev/null", O_RDWR);
> + if (filename) {
> + fd = open(filename, O_RDWR);
> + } else {
> + fd = open("/dev/null", O_RDONLY);
> + }
> if (fd < 0) {
> fprintf(stderr,
> - "Failed to open /dev/null for OFD lock probing: %s\n",
> + "Failed to open %s for OFD lock probing: %s\n",
> + filename ? filename : "/dev/null",
> strerror(errno));
> fcntl_op_setlk = F_SETLK;
> fcntl_op_getlk = F_GETLK;
This is still just as broken as v1, because it is setting
a global variable fcntl_op_getlk, based on result of an
operation whose result will vary depending on the filename
parameter.
If you want to test whether a filesystem supports fcntl
locks in general, don't touch qemu_has_ofd_lock or
qemu_probe_lock_ops methods at all. Those are just for
probing regular fcntl vs OFD fcntl locks.
> @@ -225,9 +230,9 @@ static void qemu_probe_lock_ops(void)
> }
> }
>
> -bool qemu_has_ofd_lock(void)
> +bool qemu_has_ofd_lock(const char *filename)
> {
> - qemu_probe_lock_ops();
> + qemu_probe_lock_ops(filename);
> #ifdef F_OFD_SETLK
> return fcntl_op_setlk == F_OFD_SETLK;
> #else
> @@ -244,7 +249,7 @@ static int qemu_lock_fcntl(int fd, int64_t start, int64_t len, int fl_type)
> .l_len = len,
> .l_type = fl_type,
> };
> - qemu_probe_lock_ops();
> + qemu_probe_lock_ops(NULL);
> do {
> ret = fcntl(fd, fcntl_op_setlk, &fl);
> } while (ret == -1 && errno == EINTR);
> @@ -270,7 +275,7 @@ int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive)
> .l_len = len,
> .l_type = exclusive ? F_WRLCK : F_RDLCK,
> };
> - qemu_probe_lock_ops();
> + qemu_probe_lock_ops(NULL);
> ret = fcntl(fd, fcntl_op_getlk, &fl);
> if (ret == -1) {
> return -errno;
> --
> 2.24.3
>
>
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 :|
Ok, I see.
I will split the detect lock without changing the global variable to a
separate function.
Don't call the qemu_has_ofd_lock.
Thanks.
Daniel P. Berrangé <berrange@redhat.com> 于2020年12月10日周四 下午11:59写道：
>
> On Wed, Dec 09, 2020 at 12:54:48PM +0800, 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, make the 'qemu_has_ofd_lock' with a filename be more
> > generic and reasonable.
> >
> > Signed-off-by: Li Feng <fengli@smartx.com>
> > ---
> > v2: remove the refactoring.
> > ---
> > block/file-posix.c | 32 ++++++++++++++++++--------------
> > include/qemu/osdep.h | 2 +-
> > tests/test-image-locking.c | 2 +-
> > util/osdep.c | 19 ++++++++++++-------
> > 4 files changed, 32 insertions(+), 23 deletions(-)
> >
> > diff --git a/block/file-posix.c b/block/file-posix.c
> > index 806764f7e3..03be1b188c 100644
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
> > @@ -595,7 +595,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
> > switch (locking) {
> > case ON_OFF_AUTO_ON:
> > s->use_lock = true;
> > - if (!qemu_has_ofd_lock()) {
> > + if (!qemu_has_ofd_lock(filename)) {
> > 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 "
> > @@ -606,7 +606,7 @@ static int raw_open_common(BlockDriverState *bs, QDict *options,
> > s->use_lock = false;
> > break;
> > case ON_OFF_AUTO_AUTO:
> > - s->use_lock = qemu_has_ofd_lock();
> > + s->use_lock = qemu_has_ofd_lock(filename);
> > break;
> > default:
> > abort();
> > @@ -2388,6 +2388,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 +2429,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_ofd_lock(file_opts->filename);
> > + 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;
> > + }
> > }
> >
> > /* Clear the file by truncating it to 0 */
> > diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> > index f9ec8c84e9..349adad465 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(const char *filename);
> > #endif
> >
> > #if defined(__HAIKU__) && defined(__i386__)
> > diff --git a/tests/test-image-locking.c b/tests/test-image-locking.c
> > index ba057bd66c..3e80246081 100644
> > --- a/tests/test-image-locking.c
> > +++ b/tests/test-image-locking.c
> > @@ -149,7 +149,7 @@ int main(int argc, char **argv)
> >
> > g_test_init(&argc, &argv, NULL);
> >
> > - if (qemu_has_ofd_lock()) {
> > + if (qemu_has_ofd_lock(NULL)) {
> > g_test_add_func("/image-locking/basic", test_image_locking_basic);
> > g_test_add_func("/image-locking/set-perm-abort", test_set_perm_abort);
> > }
> > diff --git a/util/osdep.c b/util/osdep.c
> > index 66d01b9160..20119aa9ae 100644
> > --- a/util/osdep.c
> > +++ b/util/osdep.c
> > @@ -187,7 +187,7 @@ static int qemu_parse_fdset(const char *param)
> > return qemu_parse_fd(param);
> > }
> >
> > -static void qemu_probe_lock_ops(void)
> > +static void qemu_probe_lock_ops(const char *filename)
> > {
> > if (fcntl_op_setlk == -1) {
> > #ifdef F_OFD_SETLK
> > @@ -200,10 +200,15 @@ static void qemu_probe_lock_ops(void)
> > .l_type = F_WRLCK,
> > };
> >
> > - fd = open("/dev/null", O_RDWR);
> > + if (filename) {
> > + fd = open(filename, O_RDWR);
> > + } else {
> > + fd = open("/dev/null", O_RDONLY);
> > + }
> > if (fd < 0) {
> > fprintf(stderr,
> > - "Failed to open /dev/null for OFD lock probing: %s\n",
> > + "Failed to open %s for OFD lock probing: %s\n",
> > + filename ? filename : "/dev/null",
> > strerror(errno));
> > fcntl_op_setlk = F_SETLK;
> > fcntl_op_getlk = F_GETLK;
>
> This is still just as broken as v1, because it is setting
> a global variable fcntl_op_getlk, based on result of an
> operation whose result will vary depending on the filename
> parameter.
>
>
> If you want to test whether a filesystem supports fcntl
> locks in general, don't touch qemu_has_ofd_lock or
> qemu_probe_lock_ops methods at all. Those are just for
> probing regular fcntl vs OFD fcntl locks.
>
>
>
> > @@ -225,9 +230,9 @@ static void qemu_probe_lock_ops(void)
> > }
> > }
> >
> > -bool qemu_has_ofd_lock(void)
> > +bool qemu_has_ofd_lock(const char *filename)
> > {
> > - qemu_probe_lock_ops();
> > + qemu_probe_lock_ops(filename);
> > #ifdef F_OFD_SETLK
> > return fcntl_op_setlk == F_OFD_SETLK;
> > #else
> > @@ -244,7 +249,7 @@ static int qemu_lock_fcntl(int fd, int64_t start, int64_t len, int fl_type)
> > .l_len = len,
> > .l_type = fl_type,
> > };
> > - qemu_probe_lock_ops();
> > + qemu_probe_lock_ops(NULL);
> > do {
> > ret = fcntl(fd, fcntl_op_setlk, &fl);
> > } while (ret == -1 && errno == EINTR);
> > @@ -270,7 +275,7 @@ int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive)
> > .l_len = len,
> > .l_type = exclusive ? F_WRLCK : F_RDLCK,
> > };
> > - qemu_probe_lock_ops();
> > + qemu_probe_lock_ops(NULL);
> > ret = fcntl(fd, fcntl_op_getlk, &fl);
> > if (ret == -1) {
> > return -errno;
> > --
> > 2.24.3
> >
> >
>
> 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 :|
>
© 2016 - 2024 Red Hat, Inc.