Re: [Virtio-fs] [PATCH 07/13] virtiofsd: Release file locks using F_UNLCK

[Christophe de Dinechin <dinechin@redhat.com>]

On 2021-09-30 at 11:30 -04, Vivek Goyal <vgoyal@redhat.com> wrote...
> We are emulating posix locks for guest using open file description locks
> in virtiofsd. When any of the fd is closed in guest, we find associated
> OFD lock fd (if there is one) and close it to release all the locks.
>
> Assumption here is that there is no other thread using lo_inode_plock
> structure or plock->fd, hence it is safe to do so.
>
> But now we are about to introduce blocking variant of locks (SETLKW),
> and that means we might be waiting to a lock to be available and
> using plock->fd. And that means there are still users of plock
> structure.
>
> So release locks using fcntl(SETLK, F_UNLCK) instead of closing fd
> and plock will be freed later when lo_inode is being freed.
>
> Signed-off-by: Vivek Goyal <vgoyal@redhat.com>
> Signed-off-by: Ioannis Angelakopoulos <iangelak@redhat.com>
> ---
>  tools/virtiofsd/passthrough_ll.c | 21 +++++++++++++++++----
>  1 file changed, 17 insertions(+), 4 deletions(-)
>
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index 38b2af8599..6928662e22 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -1557,9 +1557,6 @@ static void unref_inode(struct lo_data *lo, struct lo_inode *inode, uint64_t n)
>          lo_map_remove(&lo->ino_map, inode->fuse_ino);
>          g_hash_table_remove(lo->inodes, &inode->key);
>          if (lo->posix_lock) {
> -            if (g_hash_table_size(inode->posix_locks)) {
> -                fuse_log(FUSE_LOG_WARNING, "Hash table is not empty\n");
> -            }
>              g_hash_table_destroy(inode->posix_locks);
>              pthread_mutex_destroy(&inode->plock_mutex);
>          }
> @@ -2266,6 +2263,8 @@ static void lo_flush(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
>      (void)ino;
>      struct lo_inode *inode;
>      struct lo_data *lo = lo_data(req);
> +    struct lo_inode_plock *plock;
> +    struct flock flock;
>
>      inode = lo_inode(req, ino);
>      if (!inode) {
> @@ -2282,8 +2281,22 @@ static void lo_flush(fuse_req_t req, fuse_ino_t ino, struct fuse_file_info *fi)
>      /* An fd is going away. Cleanup associated posix locks */
>      if (lo->posix_lock) {
>          pthread_mutex_lock(&inode->plock_mutex);
> -        g_hash_table_remove(inode->posix_locks,

I'm curious why the g_hash_table_remove above is not in the 'if' below?

> +        plock = g_hash_table_lookup(inode->posix_locks,
>              GUINT_TO_POINTER(fi->lock_owner));
> +
> +        if (plock) {
> +            /*
> +             * An fd is being closed. For posix locks, this means
> +             * drop all the associated locks.
> +             */
> +            memset(&flock, 0, sizeof(struct flock));
> +            flock.l_type = F_UNLCK;
> +            flock.l_whence = SEEK_SET;
> +            /* Unlock whole file */
> +            flock.l_start = flock.l_len = 0;
> +            fcntl(plock->fd, F_OFD_SETLK, &flock);
> +        }
> +
>          pthread_mutex_unlock(&inode->plock_mutex);
>      }
>      res = close(dup(lo_fi_fd(req, fi)));


--
Cheers,
Christophe de Dinechin (IRC c3d)