[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [PATCH 1/2] file-posix: Use OFD lock only if the filesystem supports
From: |
Masayoshi Mizuma |
Subject: |
Re: [PATCH 1/2] file-posix: Use OFD lock only if the filesystem supports the lock |
Date: |
Wed, 18 Nov 2020 14:03:41 -0500 |
On Wed, Nov 18, 2020 at 03:16:53PM +0000, Daniel P. Berrangé wrote:
> On Thu, Nov 05, 2020 at 11:01:01PM -0500, Masayoshi Mizuma wrote:
> > 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>
> > ---
> > block/file-posix.c | 56 ++++++++--------
> > include/qemu/osdep.h | 2 +-
> > util/osdep.c | 149 ++++++++++++++++++++++++++++---------------
> > 3 files changed, 125 insertions(+), 82 deletions(-)
>
>
> > 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);
>
> Consider that we're *not* using OFD locks, and QEMU already
> has 'foo.qcow2' open for an existing disk backend, and it is
> locked.
>
> Now someone tries to hot-add 'foo.qcow2' for a second disk
> by mistake. Doing this qemu_dup + qemu_close will cause
> the existing locks to be removed AFAICT.
Thank you for pointing it out. I'll remove this qemu_dup() and
check orig_fd directly.
>
> > + if (fd >= 0) {
> > + ret = fcntl_setfl(fd, O_RDONLY);
> > + 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);
> > +
> > + if (ret == 0) {
> > + return true;
> > + } else {
> > + return false;
> > + }
> > #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);
>
> THis loop is confusing to read. I'd suggest creating a
> wrapper
>
> qemu_fcntl()
>
> that does the while (ret == -1 && errno == EINTR) loop,
> so that this locking code can be clearer without the
> loop.
Great idea. I'll make qemu_fcntl().
Thanks!
Masa
>
> > +
> > + 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
>
>
> 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 :|
>