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
