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 <fen...@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 :|
