| Message ID | 1608016169-5639-1-git-send-email-fengli@smartx.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] file-posix: detect the lock using the real file | expand |
On Tue, Dec 15, 2020 at 03:09:28PM +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> > --- > 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 | 66 +++++++++++++++++++++++++------------------- > include/qemu/osdep.h | 1 + > util/osdep.c | 19 +++++++++++++ > 3 files changed, 58 insertions(+), 28 deletions(-) > > diff --git a/block/file-posix.c b/block/file-posix.c > index 806764f7e3..9708212f01 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); > @@ -607,6 +622,13 @@ static int raw_open_common(BlockDriverState *bs, QDict *options, > break; > case ON_OFF_AUTO_AUTO: > s->use_lock = qemu_has_ofd_lock(); > + if (s->use_lock && !qemu_has_file_lock(s->fd)) { > + /* > + * When the os supports the OFD lock, but the filesystem doesn't > + * support, just disable the file lock. > + */ > + s->use_lock = false; > + } This should just be s->use_lock = qemu_has_file_lock(s->fd) && qemu_has_ofd_lock(); > diff --git a/util/osdep.c b/util/osdep.c > index 66d01b9160..07de97e2c5 100644 > --- a/util/osdep.c > +++ b/util/osdep.c > @@ -225,6 +225,25 @@ static void qemu_probe_lock_ops(void) > } > } > > +bool qemu_has_file_lock(int fd) > +{ > +#ifdef F_OFD_SETLK > + int cmd = F_OFD_GETLK; > +#else > + int cmd = F_GETLK; > +#endif This should unconditionally use F_GETLK. > + int ret; > + struct flock fl = { > + .l_whence = SEEK_SET, > + .l_start = 0, > + .l_len = 0, > + .l_type = F_WRLCK, > + }; > + > + ret = fcntl(fd, cmd, &fl); > + return ret == 0; > +} Regards, Daniel
Daniel P. Berrangé <berrange@redhat.com> 于2020年12月15日周二 下午6:08写道： > > On Tue, Dec 15, 2020 at 03:09:28PM +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> > > --- > > 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 | 66 +++++++++++++++++++++++++------------------- > > include/qemu/osdep.h | 1 + > > util/osdep.c | 19 +++++++++++++ > > 3 files changed, 58 insertions(+), 28 deletions(-) > > > > diff --git a/block/file-posix.c b/block/file-posix.c > > index 806764f7e3..9708212f01 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); > > @@ -607,6 +622,13 @@ static int raw_open_common(BlockDriverState *bs, QDict *options, > > break; > > case ON_OFF_AUTO_AUTO: > > s->use_lock = qemu_has_ofd_lock(); > > + if (s->use_lock && !qemu_has_file_lock(s->fd)) { > > + /* > > + * When the os supports the OFD lock, but the filesystem doesn't > > + * support, just disable the file lock. > > + */ > > + s->use_lock = false; > > + } > > This should just be > > s->use_lock = qemu_has_file_lock(s->fd) && qemu_has_ofd_lock(); > Acked. > > > > diff --git a/util/osdep.c b/util/osdep.c > > index 66d01b9160..07de97e2c5 100644 > > --- a/util/osdep.c > > +++ b/util/osdep.c > > @@ -225,6 +225,25 @@ static void qemu_probe_lock_ops(void) > > } > > } > > > > +bool qemu_has_file_lock(int fd) > > +{ > > +#ifdef F_OFD_SETLK > > + int cmd = F_OFD_GETLK; > > +#else > > + int cmd = F_GETLK; > > +#endif > > This should unconditionally use F_GETLK. Acked. > > > + int ret; > > + struct flock fl = { > > + .l_whence = SEEK_SET, > > + .l_start = 0, > > + .l_len = 0, > > + .l_type = F_WRLCK, > > + }; > > + > > + ret = fcntl(fd, cmd, &fl); > > + return ret == 0; > > +} > > 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 :| >
diff --git a/block/file-posix.c b/block/file-posix.c index 806764f7e3..9708212f01 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); @@ -607,6 +622,13 @@ static int raw_open_common(BlockDriverState *bs, QDict *options, break; case ON_OFF_AUTO_AUTO: s->use_lock = qemu_has_ofd_lock(); + if (s->use_lock && !qemu_has_file_lock(s->fd)) { + /* + * When the os supports the OFD lock, but the filesystem doesn't + * support, just disable the file lock. + */ + s->use_lock = false; + } break; default: abort(); @@ -625,22 +647,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 +2394,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 +2435,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..07de97e2c5 100644 --- a/util/osdep.c +++ b/util/osdep.c @@ -225,6 +225,25 @@ static void qemu_probe_lock_ops(void) } } +bool qemu_has_file_lock(int fd) +{ +#ifdef F_OFD_SETLK + int cmd = F_OFD_GETLK; +#else + int cmd = F_GETLK; +#endif + int ret; + struct flock fl = { + .l_whence = SEEK_SET, + .l_start = 0, + .l_len = 0, + .l_type = F_WRLCK, + }; + + ret = fcntl(fd, cmd, &fl); + return ret == 0; +} + bool qemu_has_ofd_lock(void) { qemu_probe_lock_ops();
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> --- 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 | 66 +++++++++++++++++++++++++------------------- include/qemu/osdep.h | 1 + util/osdep.c | 19 +++++++++++++ 3 files changed, 58 insertions(+), 28 deletions(-)