Re: [PATCH 1/2] file-posix: Use OFD lock only if the filesystem supports the lock

[Kevin Wolf]

Am 06.11.2020 um 05:01 hat Masayoshi Mizuma geschrieben:
> From: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>
> 
> locking=auto doesn't work if the filesystem doesn't support OFD lock.
> In that situation, following error happens:
> 
>   qemu-system-x86_64: -blockdev 
> driver=qcow2,node-name=disk,file.driver=file,file.filename=/mnt/guest.qcow2,file.locking=auto:
>  Failed to lock byte 100
> 
> qemu_probe_lock_ops() judges whether qemu can use OFD lock
> or not with doing fcntl(F_OFD_GETLK) to /dev/null. So the
> error happens if /dev/null supports OFD lock, but the filesystem
> doesn't support the lock.
> 
> Lock the actual file, not /dev/null, using F_OFD_SETLK and if that
> fails, then fallback to F_SETLK.
> 
> Signed-off-by: Masayoshi Mizuma <m.mizuma@jp.fujitsu.com>

CCing qemu-block, which is the relevant mailing list. You can use the
scripts/get_maintainer.pl script to find out who should be CCed on your
patches.

As qemu-devel itself is a very high traffic list, it's easy for a patch
to get lost if it's only sent there.

> diff --git a/util/osdep.c b/util/osdep.c
> index 66d01b9160..454e8ef9f4 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -117,9 +117,6 @@ int qemu_mprotect_none(void *addr, size_t size)
>  
>  #ifndef _WIN32
>  
> -static int fcntl_op_setlk = -1;
> -static int fcntl_op_getlk = -1;
> -
>  /*
>   * Dups an fd and sets the flags
>   */
> @@ -187,68 +184,87 @@ static int qemu_parse_fdset(const char *param)
>      return qemu_parse_fd(param);
>  }
>  
> -static void qemu_probe_lock_ops(void)
> +bool qemu_has_ofd_lock(int orig_fd)
>  {
> -    if (fcntl_op_setlk == -1) {
>  #ifdef F_OFD_SETLK
> -        int fd;
> -        int ret;
> -        struct flock fl = {
> -            .l_whence = SEEK_SET,
> -            .l_start  = 0,
> -            .l_len    = 0,
> -            .l_type   = F_WRLCK,
> -        };
> -
> -        fd = open("/dev/null", O_RDWR);
> -        if (fd < 0) {
> +    int fd;
> +    int ret;
> +    struct flock fl = {
> +        .l_whence = SEEK_SET,
> +        .l_start  = 0,
> +        .l_len    = 0,
> +        .l_type   = F_RDLCK,
> +    };
> +
> +    fd = qemu_dup(orig_fd);
> +    if (fd >= 0) {
> +        ret = fcntl_setfl(fd, O_RDONLY);

I don't understand this part. Why are you trying to reopen the file
descriptor read-only? Shouldn't the test work fine with a read-write
file descriptor? /dev/null was opened O_RDWR in the old code.

> +        if (ret) {
>              fprintf(stderr,
> -                    "Failed to open /dev/null for OFD lock probing: %s\n",
> -                    strerror(errno));
> -            fcntl_op_setlk = F_SETLK;
> -            fcntl_op_getlk = F_GETLK;
> -            return;
> -        }
> -        ret = fcntl(fd, F_OFD_GETLK, &fl);
> -        close(fd);
> -        if (!ret) {
> -            fcntl_op_setlk = F_OFD_SETLK;
> -            fcntl_op_getlk = F_OFD_GETLK;
> -        } else {
> -            fcntl_op_setlk = F_SETLK;
> -            fcntl_op_getlk = F_GETLK;
> +                    "Failed to fcntl for OFD lock probing.\n");
> +            qemu_close(fd);
> +            return false;
>          }
> +    }
> +
> +    ret = fcntl(fd, F_OFD_GETLK, &fl);
> +    qemu_close(fd);

F_OFD_GETLK doesn't modify the state, so it seems to me that even the
qemu_dup() is unnecessary and we could just directly try F_OFD_GETLK on
the passed file descriptor (orig_fd).

> +
> +    if (ret == 0) {
> +        return true;
> +    } else {
> +        return false;
> +    }

This should be written shorter as return ret == 0;

>  #else
> -        fcntl_op_setlk = F_SETLK;
> -        fcntl_op_getlk = F_GETLK;
> +    return false;
>  #endif
> -    }
>  }
>  
> -bool qemu_has_ofd_lock(void)
> -{
> -    qemu_probe_lock_ops();
>  #ifdef F_OFD_SETLK
> -    return fcntl_op_setlk == F_OFD_SETLK;
> +static int _qemu_lock_fcntl(int fd, struct flock *fl)
> +{
> +    int ret;
> +    bool ofd_lock = true;
> +
> +    do {
> +        if (ofd_lock) {
> +            ret = fcntl(fd, F_OFD_SETLK, fl);
> +            if ((ret == -1) && (errno == EINVAL)) {
> +                ofd_lock = false;
> +            }
> +        }
> +
> +        if (!ofd_lock) {
> +            /* Fallback to POSIX lock */
> +            ret = fcntl(fd, F_SETLK, fl);
> +        }
> +    } while (ret == -1 && errno == EINTR);
> +
> +    return ret == -1 ? -errno : 0;
> +}
>  #else
> -    return false;
> -#endif
> +static int _qemu_lock_fcntl(int fd, struct flock *fl)
> +{
> +    int ret;
> +
> +    do {
> +        ret = fcntl(fd, F_SETLK, fl);
> +    } while (ret == -1 && errno == EINTR);
> +
> +    return ret == -1 ? -errno : 0;
>  }
> +#endif

The logic looks fine to me, at least assuming that EINVAL is really what
we will consistently get from the kernel if OFD locks are not supported.
Is this documented anywhere? The fcntl manpage doesn't seem to mention
this case.

Anyway, I think I would try to minimise the duplication by having only
a small #ifdef section inside the function, maybe like this:

#ifdef F_OFD_SETLK
            ret = fcntl(fd, F_OFD_SETLK, fl);
            if ((ret == -1) && (errno == EINVAL)) {
                ofd_lock = false;
            }
#else
            ofd_lock = false;
#endif

>  static int qemu_lock_fcntl(int fd, int64_t start, int64_t len, int fl_type)
>  {
> -    int ret;
>      struct flock fl = {
>          .l_whence = SEEK_SET,
>          .l_start  = start,
>          .l_len    = len,
>          .l_type   = fl_type,
>      };
> -    qemu_probe_lock_ops();
> -    do {
> -        ret = fcntl(fd, fcntl_op_setlk, &fl);
> -    } while (ret == -1 && errno == EINTR);
> -    return ret == -1 ? -errno : 0;
> +
> +    return _qemu_lock_fcntl(fd, &fl);
>  }
>  
>  int qemu_lock_fd(int fd, int64_t start, int64_t len, bool exclusive)
> @@ -261,22 +277,49 @@ int qemu_unlock_fd(int fd, int64_t start, int64_t len)
>      return qemu_lock_fcntl(fd, start, len, F_UNLCK);
>  }
>  
> -int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive)
> +#ifdef F_OFD_SETLK
> +static int _qemu_lock_fd_test(int fd, struct flock *fl)
>  {
>      int ret;
> +
> +    ret = fcntl(fd, F_OFD_GETLK, fl);
> +    if ((ret == -1) && (errno != EINVAL)) {
> +        return -errno;
> +

Please remove this empty line.

The parentheses in the condition (above and below) are not stricly
necessary.

> +    } else if ((ret == -1) && (errno == EINVAL)) {
> +        /* Fallback to POSIX lock */
> +        ret = fcntl(fd, F_GETLK, fl);
> +        if (ret == -1) {
> +            return -errno;
> +        }
> +    }
> +
> +    return fl->l_type == F_UNLCK ? 0 : -EAGAIN;
> +}
> +#else
> +static int _qemu_lock_fd_test(int fd, struct flock *fl)
> +{
> +    int ret;
> +
> +    ret = fcntl(fd, F_GETLK, fl);
> +    if (ret == -1) {
> +        return -errno;
> +    } else {
> +        return fl->l_type == F_UNLCK ? 0 : -EAGAIN;
> +    }
> +}
> +#endif

Same idea as above: #ifdef only around the fcntl(F_OFD_GETLK) call can
minimise the code duplication.

> +int qemu_lock_fd_test(int fd, int64_t start, int64_t len, bool exclusive)
> +{
>      struct flock fl = {
>          .l_whence = SEEK_SET,
>          .l_start  = start,
>          .l_len    = len,
>          .l_type   = exclusive ? F_WRLCK : F_RDLCK,
>      };
> -    qemu_probe_lock_ops();
> -    ret = fcntl(fd, fcntl_op_getlk, &fl);
> -    if (ret == -1) {
> -        return -errno;
> -    } else {
> -        return fl.l_type == F_UNLCK ? 0 : -EAGAIN;
> -    }
> +
> +    return _qemu_lock_fd_test(fd, &fl);
>  }
>  #endif

After moving the #ifdef into the function, you can inline
_qemu_lock_fd_test() and and _qemu_lock_fcntl() again. This is also good
because identifiers starting with an underscore are reserved in the C
standard.

Kevin