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. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|