block/file-posix.c | 30 +++++++++++++++++------------- include/qemu/osdep.h | 1 + util/osdep.c | 29 +++++++++++++++++++++++++++++ 3 files changed, 47 insertions(+), 13 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".
Add a new function 'qemu_has_file_lock' to detect if the filesystem supports locks
or not.
And when the drive is auto mode, use the 'qemu_has_file_lock' to set the toggle.
Signed-off-by: Li Feng <fengli@smartx.com>
---
v3: don't call the qemu_has_ofd_lock, use a new function instead.
v2: remove the refactoring.
---
block/file-posix.c | 30 +++++++++++++++++-------------
include/qemu/osdep.h | 1 +
util/osdep.c | 29 +++++++++++++++++++++++++++++
3 files changed, 47 insertions(+), 13 deletions(-)
diff --git a/block/file-posix.c b/block/file-posix.c
index 806764f7e3..48f9a32de2 100644
--- a/block/file-posix.c
+++ b/block/file-posix.c
@@ -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_file_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_file_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..593ae0f4d2 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(const char *filename);
#endif
#if defined(__HAIKU__) && defined(__i386__)
diff --git a/util/osdep.c b/util/osdep.c
index 66d01b9160..5d9961d261 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -225,6 +225,35 @@ static void qemu_probe_lock_ops(void)
}
}
+bool qemu_has_file_lock(const char *filename)
+{
+#ifdef F_OFD_SETLK
+ int cmd = F_OFD_GETLK;
+#else
+ int cmd = F_GETLK;
+#endif
+ int fd;
+ int ret;
+ struct flock fl = {
+ .l_whence = SEEK_SET,
+ .l_start = 0,
+ .l_len = 0,
+ .l_type = F_WRLCK,
+ };
+
+ fd = open(filename, O_RDWR);
+ if (fd < 0) {
+ fprintf(stderr,
+ "Failed to open %s for OFD lock probing: %s\n",
+ filename,
+ strerror(errno));
+ return false;
+ }
+ ret = fcntl(fd, cmd, &fl);
+ close(fd);
+ return ret == 0;
+}
+
bool qemu_has_ofd_lock(void)
{
qemu_probe_lock_ops();
--
2.24.3
On Fri, Dec 11, 2020 at 12:38:19AM +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".
>
> Add a new function 'qemu_has_file_lock' to detect if the filesystem supports locks
> or not.
> And when the drive is auto mode, use the 'qemu_has_file_lock' to set the toggle.
>
> Signed-off-by: Li Feng <fengli@smartx.com>
> ---
> v3: don't call the qemu_has_ofd_lock, use a new function instead.
> v2: remove the refactoring.
> ---
> block/file-posix.c | 30 +++++++++++++++++-------------
> include/qemu/osdep.h | 1 +
> util/osdep.c | 29 +++++++++++++++++++++++++++++
> 3 files changed, 47 insertions(+), 13 deletions(-)
>
> diff --git a/block/file-posix.c b/block/file-posix.c
> index 806764f7e3..48f9a32de2 100644
> --- a/block/file-posix.c
> +++ b/block/file-posix.c
> @@ -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_file_lock(filename);
This is not good - it causes us to always use locks by default, where
as previously we only used them if OFD was available. It neds to test
both here, except opening + closing filename to test for fnctl support
risks releasing any locks QEMU already holds on filename if OFD is not
supported.
> 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_file_lock(file_opts->filename);
This cause QEMU to open and close filename. If another thread
already had filename open, and OFD is not support, we've just
lock the locks we held. We need to use 'fd' which is already
open.
> + 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 */
> +bool qemu_has_file_lock(const char *filename)
IMO thisshould just accept a pre-opened 'int fd'
> +{
> +#ifdef F_OFD_SETLK
> + int cmd = F_OFD_GETLK;
> +#else
> + int cmd = F_GETLK;
> +#endif
> + int fd;
> + int ret;
> + struct flock fl = {
> + .l_whence = SEEK_SET,
> + .l_start = 0,
> + .l_len = 0,
> + .l_type = F_WRLCK,
> + };
> +
> + fd = open(filename, O_RDWR);
> + if (fd < 0) {
> + fprintf(stderr,
> + "Failed to open %s for OFD lock probing: %s\n",
> + filename,
> + strerror(errno));
> + return false;
> + }
> + ret = fcntl(fd, cmd, &fl);
> + close(fd);
> + return ret == 0;
> +}
> +
> bool qemu_has_ofd_lock(void)
> {
> qemu_probe_lock_ops();
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 :|
Hi, Daniel
Thanks for your reply.
I have just ended my trip, sorry for my late response.
I will send out the v4.
Daniel P. Berrangé <berrange@redhat.com> 于2020年12月11日周五 上午12:55写道：
>
> On Fri, Dec 11, 2020 at 12:38:19AM +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".
> >
> > Add a new function 'qemu_has_file_lock' to detect if the filesystem supports locks
> > or not.
> > And when the drive is auto mode, use the 'qemu_has_file_lock' to set the toggle.
> >
> > Signed-off-by: Li Feng <fengli@smartx.com>
> > ---
> > v3: don't call the qemu_has_ofd_lock, use a new function instead.
> > v2: remove the refactoring.
> > ---
> > block/file-posix.c | 30 +++++++++++++++++-------------
> > include/qemu/osdep.h | 1 +
> > util/osdep.c | 29 +++++++++++++++++++++++++++++
> > 3 files changed, 47 insertions(+), 13 deletions(-)
> >
> > diff --git a/block/file-posix.c b/block/file-posix.c
> > index 806764f7e3..48f9a32de2 100644
> > --- a/block/file-posix.c
> > +++ b/block/file-posix.c
> > @@ -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_file_lock(filename);
>
> This is not good - it causes us to always use locks by default, where
> as previously we only used them if OFD was available. It neds to test
> both here, except opening + closing filename to test for fnctl support
> risks releasing any locks QEMU already holds on filename if OFD is not
> supported.
Yes, check the qemu_has_ofd_lock and qemu_has_file_lock both, and
set the use_lock to false when the os supports the OFD lock, but the
filesystem doesn't support.
>
> > 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_file_lock(file_opts->filename);
>
> This cause QEMU to open and close filename. If another thread
> already had filename open, and OFD is not support, we've just
> lock the locks we held. We need to use 'fd' which is already
> open.
Acked.
>
> > + 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 */
>
>
> > +bool qemu_has_file_lock(const char *filename)
>
> IMO thisshould just accept a pre-opened 'int fd'
Acked.
>
> > +{
> > +#ifdef F_OFD_SETLK
> > + int cmd = F_OFD_GETLK;
> > +#else
> > + int cmd = F_GETLK;
> > +#endif
> > + int fd;
> > + int ret;
> > + struct flock fl = {
> > + .l_whence = SEEK_SET,
> > + .l_start = 0,
> > + .l_len = 0,
> > + .l_type = F_WRLCK,
> > + };
> > +
> > + fd = open(filename, O_RDWR);
> > + if (fd < 0) {
> > + fprintf(stderr,
> > + "Failed to open %s for OFD lock probing: %s\n",
> > + filename,
> > + strerror(errno));
> > + return false;
> > + }
> > + ret = fcntl(fd, cmd, &fl);
> > + close(fd);
> > + return ret == 0;
> > +}
> > +
> > bool qemu_has_ofd_lock(void)
> > {
> > qemu_probe_lock_ops();
>
> 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.