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

[Masayoshi Mizuma]

On Wed, Nov 18, 2020 at 04:42:47PM +0100, Kevin Wolf wrote:
> 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.

Thank you for letting me know.
I'll do scripts/get_maintainer.pl to get the mailing list before posting 
patches.

> 
> > 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).

OK, I'll change to try F_OFD_GETLK of orig_fd directly.

> 
> > +
> > +    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

Great! I'll make this.

> 
> >  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.

Got it.

> 
> > +    } 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.

Got it, thanks! I'll post v2.

- Masa