From mboxrd@z Thu Jan 1 00:00:00 1970 Date: Fri, 7 Jun 2019 19:30:40 +0100 From: "Dr. David Alan Gilbert" Message-ID: <20190607183039.GX2631@work-vm> References: <20190530181141.GA9848@redhat.com> <20190607151517.GH2631@work-vm> <20190607182129.GB23125@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190607182129.GB23125@redhat.com> Subject: Re: [Virtio-fs] [RFC PATCH] virtiofsd: Provide support for posix locks List-Id: Development discussions about virtio-fs List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Vivek Goyal Cc: virtio-fs@redhat.com, Miklos Szeredi * Vivek Goyal (vgoyal@redhat.com) wrote: > On Fri, Jun 07, 2019 at 04:15:18PM +0100, Dr. David Alan Gilbert wrote: > [..] > > > Index: qemu/contrib/virtiofsd/passthrough_ll.c > > > =================================================================== > > > --- qemu.orig/contrib/virtiofsd/passthrough_ll.c 2019-04-25 10:49:14.103386416 -0400 > > > +++ qemu/contrib/virtiofsd/passthrough_ll.c 2019-05-30 14:02:55.598483536 -0400 > > > @@ -58,6 +58,12 @@ > > > #include > > > #include "seccomp.h" > > > > > > +/* Keep track of inode posix locks for each owner. */ > > > +struct lo_inode_plock { > > > + uint64_t lock_owner; > > > + int fd; /* fd for OFD locks */ > > > +}; > > > + > > > struct lo_map_elem { > > > union { > > > struct lo_inode *inode; > > > @@ -86,6 +92,8 @@ struct lo_inode { > > > struct lo_key key; > > > uint64_t refcount; /* protected by lo->mutex */ > > > fuse_ino_t fuse_ino; > > > + pthread_mutex_t mutex; > > > > If this mutex is purely for posix_locks then there's probably a better > > name. > > Hi Dave, > > As of now it is only for posix locks. But it could be used for locking > other inode specific data structures as well. That's why I left it with > a generic name. But if it bothers you, I can open to change the name > to something else. Well the important thing is that it's not the whole lo_inode; someone who didn't know the code might think it was. > [..] > > > @@ -1038,6 +1064,10 @@ static void unref_inode(struct lo_data * > > > if (!inode->refcount) { > > > lo_map_remove(&lo->ino_map, inode->fuse_ino); > > > g_hash_table_remove(lo->inodes, &inode->key); > > > + if (g_hash_table_size(inode->posix_locks)) { > > > + warn("Hash table is not empty\n"); > > > + } > > > + g_hash_table_destroy(inode->posix_locks); > > > pthread_mutex_unlock(&lo->mutex); > > > close(inode->fd); > > > > pthread_mutex_destroy(&inode->mutex) ? > > So is it necessary to call this? Is it about that if same inode is > allocated next time and if we call pthread_mutex_init(), it might > have issues? > > IOW, trying to understand why one should call pthread_mutex_destroy() > before freeing inode. My understanding is that it's always best to destroy a mutex before freeing the memory it's allocated in. Dave > Vivek -- Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK