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

[Vivek Goyal <vgoyal@redhat.com>]

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.

[..]
> > @@ -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.

Vivek