Re: [Qemu-block] [Qemu-devel] [PATCH v4 08/27] osdep: Add qemu_lock_fd and qemu_unlock_fd

[Fam Zheng]

On Tue, 05/10 09:57, Daniel P. Berrange wrote:
> On Tue, May 10, 2016 at 10:50:40AM +0800, Fam Zheng wrote:
> > They are wrappers of POSIX fcntl file locking, with the additional
> > interception of open/close (through qemu_open and qemu_close) to offer a
> > better semantics that preserves the locks across multiple life cycles of
> > different fds on the same file.  The reason to make this semantics
> > change over the fcntl locks is to make the API cleaner for QEMU
> > subsystems.
> > 
> > More precisely, we remove this "feature" of fcntl lock:
> > 
> >     If a process closes any file descriptor referring to a file, then
> >     all of the process's locks on that file are released, regardless of
> >     the file descriptor(s) on which the locks were obtained.
> > 
> > as long as the fd is always open/closed through qemu_open and
> > qemu_close.
> 
> You're not actually really removing that problem - this is just hacking
> around it in a manner which is racy & susceptible to silent failure.
> 
> 
> > +static int qemu_fd_close(int fd)
> > +{
> > +    LockEntry *ent, *tmp;
> > +    LockRecord *rec;
> > +    char *path;
> > +    int ret;
> > +
> > +    assert(fd_to_path);
> > +    path = g_hash_table_lookup(fd_to_path, GINT_TO_POINTER(fd));
> > +    assert(path);
> > +    g_hash_table_remove(fd_to_path, GINT_TO_POINTER(fd));
> > +    rec = g_hash_table_lookup(lock_map, path);
> > +    ret = close(fd);
> > +
> > +    if (rec) {
> > +        QLIST_FOREACH_SAFE(ent, &rec->records, next, tmp) {
> > +            int r;
> > +            if (ent->fd == fd) {
> > +                QLIST_REMOVE(ent, next);
> > +                g_free(ent);
> > +                continue;
> > +            }
> > +            r = qemu_lock_do(ent->fd, ent->start, ent->len,
> > +                             ent->readonly ? F_RDLCK : F_WRLCK);
> > +            if (r == -1) {
> > +                fprintf(stderr, "Failed to acquire lock on fd %d: %s\n",
> > +                        ent->fd, strerror(errno));
> > +            }
> 
> If another app has acquired the lock between the close and this attempt
> to re-acquire the lock, then QEMU is silently carrying on without any
> lock being held. For something that's intending to provide protection
> against concurrent use I think this is not an acceptable failure
> scenario.
> 
> 
> > +int qemu_open(const char *name, int flags, ...)
> > +{
> > +    int mode = 0;
> > +    int ret;
> > +
> > +    if (flags & O_CREAT) {
> > +        va_list ap;
> > +
> > +        va_start(ap, flags);
> > +        mode = va_arg(ap, int);
> > +        va_end(ap);
> > +    }
> > +    ret = qemu_fd_open(name, flags, mode);
> > +    if (ret >= 0) {
> > +        qemu_fd_add_record(ret, name);
> > +    }
> > +    return ret;
> > +}
> 
> I think the approach you have here is fundamentally not usable with
> fcntl locks, because it is still using the pattern
> 
>    fd = open(path)
>    lock(fd)
>    if failed lock
>       close(fd)
>    ...do stuff.
> 
> 
> As mentioned in previous posting I believe the block layer must be
> checking whether it already has a lock held against "path" *before*
> even attempting to open the file. Once QEMU has the file descriptor
> open it is too later, because closing the FD will release pre-existing
> locks QEMU has.

There will still be valid close() calls, that will release necessary locks
temporarily.  You are right the "close + re-acquire" in qemu_fd_close() is a
racy problem. Any suggestion how this could be fixed?  Something like
introducing a "close2" syscall that doesn't drop locks on other fds?

Fam