On Wed, Nov 18, 2020 at 02:10:36PM -0500, Masayoshi Mizuma wrote:
> 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.miz...@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.miz...@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.
The man page of fcntl(2) says:
EINVAL The value specified in cmd is not recognized by this kernel.
So I think EINVAL is good enough to check whether the filesystem supports
OFD locks or not...
Thanks,
Masa
> >
> > 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
