* [PATCH v3] file-posix: detect the lock using the real file
@ 2020-12-10 16:38 Li Feng
2020-12-10 16:54 ` Daniel P. Berrangé
0 siblings, 1 reply; 3+ messages in thread
From: Li Feng @ 2020-12-10 16:38 UTC (permalink / raw)
To: berrange, Kevin Wolf, Max Reitz, open list:raw,
open list:All patches CC here
Cc: lifeng1519, Li Feng, kyle
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
^ permalink raw reply related [flat|nested] 3+ messages in thread* Re: [PATCH v3] file-posix: detect the lock using the real file
2020-12-10 16:38 [PATCH v3] file-posix: detect the lock using the real file Li Feng
@ 2020-12-10 16:54 ` Daniel P. Berrangé
2020-12-15 6:04 ` Feng Li
0 siblings, 1 reply; 3+ messages in thread
From: Daniel P. Berrangé @ 2020-12-10 16:54 UTC (permalink / raw)
To: Li Feng
Cc: Kevin Wolf, lifeng1519, open list:raw,
open list:All patches CC here, Max Reitz, kyle
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 :|
^ permalink raw reply [flat|nested] 3+ messages in thread* Re: [PATCH v3] file-posix: detect the lock using the real file
2020-12-10 16:54 ` Daniel P. Berrangé
@ 2020-12-15 6:04 ` Feng Li
0 siblings, 0 replies; 3+ messages in thread
From: Feng Li @ 2020-12-15 6:04 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Kevin Wolf, open list:raw, open list:All patches CC here,
Max Reitz, kyle@smartx.com, Li Feng
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 :|
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-12-15 6:06 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-12-10 16:38 [PATCH v3] file-posix: detect the lock using the real file Li Feng
2020-12-10 16:54 ` Daniel P. Berrangé
2020-12-15 6:04 ` Feng Li
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).