Re: [Virtio-fs] [RFC PATCH] virtiofsd: Provide support for posix locks

["Dr. David Alan Gilbert" <dgilbert@redhat.com>]

* 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 <gmodule.h>
> > >  #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