block/file-posix.c | 61 +++++++++++++++++++++++--------------------- include/qemu/osdep.h | 1 + util/osdep.c | 14 ++++++++++ 3 files changed, 47 insertions(+), 29 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, 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;
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;
+ }
}
/* 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);
+ return ret == 0;
+}
+
bool qemu_has_ofd_lock(void)
{
qemu_probe_lock_ops();
--
2.24.3
On Tue, Dec 15, 2020 at 06:53:56PM +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, 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(-) Reviewed-by: Daniel P. Berrangé <berrange@redhat.com> 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 :|
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.
Also, it's mostly a duplication of qemu_lock_fd_test(), so some refactoring may be done, but it may be done later.
> + return ret == 0;
> +}
> +
> bool qemu_has_ofd_lock(void)
> {
> qemu_probe_lock_ops();
>
--
Best regards,
Vladimir
On Wed, Dec 16, 2020 at 11:22:38AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 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.
No, we explicitly do *not* want that. This function is *only*
about checking whether traditional fcntl locks work or not on
this specific file handle.
Support for OFD locks is a separate check, and its result
applies system wide.
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 :|
16.12.2020 12:49, Daniel P. Berrangé wrote:
> On Wed, Dec 16, 2020 at 11:22:38AM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> 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.
>
> No, we explicitly do *not* want that. This function is *only*
> about checking whether traditional fcntl locks work or not on
> this specific file handle.
Hmm, than may be name the function qemu_has_posix_lock(), to stress that fact? All other qemu*lock*(fd) API functions do rely on fcnt_op_getlk/fcntl_op_setlk and work with lock type determined by qemu_probe_lock_ops().
>
> Support for OFD locks is a separate check, and its result
> applies system wide.
>
Still, I don't follow, why should we check posix lock, when we are going to use ofd locks. What if OFD locks are supported by kernel, but specific file-system supports posix lock, but not ofd? Than we'll fail the same way as described in commit message and the patch doesn't help. Or what I miss?
--
Best regards,
Vladimir
On Wed, Dec 16, 2020 at 01:25:36PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 16.12.2020 12:49, Daniel P. Berrangé wrote:
> > On Wed, Dec 16, 2020 at 11:22:38AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > 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.
> >
> > No, we explicitly do *not* want that. This function is *only*
> > about checking whether traditional fcntl locks work or not on
> > this specific file handle.
>
> Hmm, than may be name the function qemu_has_posix_lock(), to stress that fact? All other qemu*lock*(fd) API functions do rely on fcnt_op_getlk/fcntl_op_setlk and work with lock type determined by qemu_probe_lock_ops().
>
> >
> > Support for OFD locks is a separate check, and its result
> > applies system wide.
> >
>
> Still, I don't follow, why should we check posix lock, when we are
> going to use ofd locks. What if OFD locks are supported by kernel,
> but specific file-system supports posix lock, but not ofd? Than
> we'll fail the same way as described in commit message and the
> patch doesn't help. Or what I miss?
That's not a scenario that exists. OFD locks are implemented by the
kernel in the generic VFS layer, so apply to all filesystems. The
filesystems merely have to support traditiaonl fcntl locks, and then
they get OFD for free.
IOW, there are two separate questions the code needs answers to
1. Does this specific filesystem support locking
2. Does the operating system support OFD locking
The problem in the commit message is because the original code was asking
question 2 only and geting the correct answer that the OS supports OFD.
The image was stored on a filesystem, however, that does not support fnctl
locks at all and hence locking failed. This failure would happen regardless
of whether OFD or traditional fcntl locks were used.
The problem is solved by also asking the first question before enabling
use of locks.
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 :|
16.12.2020 13:41, Daniel P. Berrangé wrote:
> On Wed, Dec 16, 2020 at 01:25:36PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> 16.12.2020 12:49, Daniel P. Berrangé wrote:
>>> On Wed, Dec 16, 2020 at 11:22:38AM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>> 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.
>>>
>>> No, we explicitly do *not* want that. This function is *only*
>>> about checking whether traditional fcntl locks work or not on
>>> this specific file handle.
>>
>> Hmm, than may be name the function qemu_has_posix_lock(), to stress that fact? All other qemu*lock*(fd) API functions do rely on fcnt_op_getlk/fcntl_op_setlk and work with lock type determined by qemu_probe_lock_ops().
>>
>>>
>>> Support for OFD locks is a separate check, and its result
>>> applies system wide.
>>>
>>
>> Still, I don't follow, why should we check posix lock, when we are
>> going to use ofd locks. What if OFD locks are supported by kernel,
>> but specific file-system supports posix lock, but not ofd? Than
>> we'll fail the same way as described in commit message and the
>> patch doesn't help. Or what I miss?
>
> That's not a scenario that exists. OFD locks are implemented by the
> kernel in the generic VFS layer, so apply to all filesystems. The
> filesystems merely have to support traditiaonl fcntl locks, and then
> they get OFD for free.
>
> IOW, there are two separate questions the code needs answers to
>
> 1. Does this specific filesystem support locking
> 2. Does the operating system support OFD locking
>
> The problem in the commit message is because the original code was asking
> question 2 only and geting the correct answer that the OS supports OFD.
> The image was stored on a filesystem, however, that does not support fnctl
> locks at all and hence locking failed. This failure would happen regardless
> of whether OFD or traditional fcntl locks were used.
>
> The problem is solved by also asking the first question before enabling
> use of locks.
>
OK, thanks for explanation. Sorry, I was not attentive to previous versions, but I remember that something about this was discussed, so probably you explain this thing not the first time;( Hmm, still, what's wrong with checking fs by OFD lock trying? It will fail anyway? Or, it will NOT fail, because OFD knows that there is no locks, and will not ask filesystem driver and we will fail only later, when try to set the lock? If so, it worth a comment in file-posix.c.. As it looks like a design flaw of OFD locks.
--
Best regards,
Vladimir
On Wed, Dec 16, 2020 at 03:03:08PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> 16.12.2020 13:41, Daniel P. Berrangé wrote:
> > On Wed, Dec 16, 2020 at 01:25:36PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > 16.12.2020 12:49, Daniel P. Berrangé wrote:
> > > > On Wed, Dec 16, 2020 at 11:22:38AM +0300, Vladimir Sementsov-Ogievskiy wrote:
> > > > > 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.
> > > > > >
> > > > > > 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.
> > > >
> > > > No, we explicitly do *not* want that. This function is *only*
> > > > about checking whether traditional fcntl locks work or not on
> > > > this specific file handle.
> > >
> > > Hmm, than may be name the function qemu_has_posix_lock(), to stress that fact? All other qemu*lock*(fd) API functions do rely on fcnt_op_getlk/fcntl_op_setlk and work with lock type determined by qemu_probe_lock_ops().
> > >
> > > >
> > > > Support for OFD locks is a separate check, and its result
> > > > applies system wide.
> > > >
> > >
> > > Still, I don't follow, why should we check posix lock, when we are
> > > going to use ofd locks. What if OFD locks are supported by kernel,
> > > but specific file-system supports posix lock, but not ofd? Than
> > > we'll fail the same way as described in commit message and the
> > > patch doesn't help. Or what I miss?
> >
> > That's not a scenario that exists. OFD locks are implemented by the
> > kernel in the generic VFS layer, so apply to all filesystems. The
> > filesystems merely have to support traditiaonl fcntl locks, and then
> > they get OFD for free.
> >
> > IOW, there are two separate questions the code needs answers to
> >
> > 1. Does this specific filesystem support locking
> > 2. Does the operating system support OFD locking
> >
> > The problem in the commit message is because the original code was asking
> > question 2 only and geting the correct answer that the OS supports OFD.
> > The image was stored on a filesystem, however, that does not support fnctl
> > locks at all and hence locking failed. This failure would happen regardless
> > of whether OFD or traditional fcntl locks were used.
> >
> > The problem is solved by also asking the first question before enabling
> > use of locks.
> >
>
> OK, thanks for explanation. Sorry, I was not attentive to previous versions, but I remember that something about this was discussed, so probably you explain this thing not the first time;( Hmm, still, what's wrong with checking fs by OFD lock trying? It will fail anyway? Or, it will NOT fail, because OFD knows that there is no locks, and will not ask filesystem driver and we will fail only later, when try to set the lock? If so, it worth a comment in file-posix.c.. As it looks like a design flaw of OFD locks.
We want to cache the OFD result check so we don't have to repeat the
probe every time. If we check OFD per-FS it makes caching much more
complex than it needs to be.
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 :|
16.12.2020 15:57, Daniel P. Berrangé wrote:
> On Wed, Dec 16, 2020 at 03:03:08PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>> 16.12.2020 13:41, Daniel P. Berrangé wrote:
>>> On Wed, Dec 16, 2020 at 01:25:36PM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>> 16.12.2020 12:49, Daniel P. Berrangé wrote:
>>>>> On Wed, Dec 16, 2020 at 11:22:38AM +0300, Vladimir Sementsov-Ogievskiy wrote:
>>>>>> 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.
>>>>>>>
>
>
>>>>>>> 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.
>>>>>
>>>>> No, we explicitly do *not* want that. This function is *only*
>>>>> about checking whether traditional fcntl locks work or not on
>>>>> this specific file handle.
>>>>
>>>> Hmm, than may be name the function qemu_has_posix_lock(), to stress that fact? All other qemu*lock*(fd) API functions do rely on fcnt_op_getlk/fcntl_op_setlk and work with lock type determined by qemu_probe_lock_ops().
>>>>
>>>>>
>>>>> Support for OFD locks is a separate check, and its result
>>>>> applies system wide.
>>>>>
>>>>
>>>> Still, I don't follow, why should we check posix lock, when we are
>>>> going to use ofd locks. What if OFD locks are supported by kernel,
>>>> but specific file-system supports posix lock, but not ofd? Than
>>>> we'll fail the same way as described in commit message and the
>>>> patch doesn't help. Or what I miss?
>>>
>>> That's not a scenario that exists. OFD locks are implemented by the
>>> kernel in the generic VFS layer, so apply to all filesystems. The
>>> filesystems merely have to support traditiaonl fcntl locks, and then
>>> they get OFD for free.
>>>
>>> IOW, there are two separate questions the code needs answers to
>>>
>>> 1. Does this specific filesystem support locking
>>> 2. Does the operating system support OFD locking
>>>
>>> The problem in the commit message is because the original code was asking
>>> question 2 only and geting the correct answer that the OS supports OFD.
>>> The image was stored on a filesystem, however, that does not support fnctl
>>> locks at all and hence locking failed. This failure would happen regardless
>>> of whether OFD or traditional fcntl locks were used.
>>>
>>> The problem is solved by also asking the first question before enabling
>>> use of locks.
>>>
>>
>> OK, thanks for explanation. Sorry, I was not attentive to previous versions, but I remember that something about this was discussed, so probably you explain this thing not the first time;( Hmm, still, what's wrong with checking fs by OFD lock trying? It will fail anyway? Or, it will NOT fail, because OFD knows that there is no locks, and will not ask filesystem driver and we will fail only later, when try to set the lock? If so, it worth a comment in file-posix.c.. As it looks like a design flaw of OFD locks.
>
> We want to cache the OFD result check so we don't have to repeat the
> probe every time. If we check OFD per-FS it makes caching much more
> complex than it needs to be.
>
So, to not check OFD per fd, we check posix locks, and use the knowledge that OFD rely on posix locks anyway? I don't see why is it simpler..
I don't suggest to modify qemu_probe_lock_ops(). It chose ofd or posix locks (and set fcntl_op_getlk) system wide on first call, and it's OK.
I just don't see, why can't we use chosen fcntl_op_getlk to do per-fd check for lock-supporting introduced in this patch. If for some reason using fcntl_op_getlk will result in false-positive, it worth a comment.
--
Best regards,
Vladimir
© 2016 - 2024 Red Hat, Inc.