On 12/12/19 5:39 PM, Dr. David Alan Gilbert (git) wrote:
> From: "Dr. David Alan Gilbert" <dgilbert@redhat.com>
>
> lo_destroy was relying on some implicit knowledge of the locking;
> we can avoid this if we create an unref_inode that doesn't take
> the lock and then grab it for the whole of the lo_destroy.
>
> Suggested-by: Vivek Goyal <vgoyal@redhat.com>
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> ---
> tools/virtiofsd/passthrough_ll.c | 31 +++++++++++++++++--------------
> 1 file changed, 17 insertions(+), 14 deletions(-)
>
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index 38f4948e61..c37f57157e 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -1328,14 +1328,13 @@ static void lo_unlink(fuse_req_t req, fuse_ino_t parent, const char *name)
> lo_inode_put(lo, &inode);
> }
>
> -static void unref_inode_lolocked(struct lo_data *lo, struct lo_inode *inode,
> - uint64_t n)
> +/* To be called with lo->mutex held */
> +static void unref_inode(struct lo_data *lo, struct lo_inode *inode, uint64_t n)
> {
> if (!inode) {
> return;
> }
>
> - pthread_mutex_lock(&lo->mutex);
> assert(inode->nlookup >= n);
> inode->nlookup -= n;
> if (!inode->nlookup) {
> @@ -1346,15 +1345,24 @@ static void unref_inode_lolocked(struct lo_data *lo, struct lo_inode *inode,
> }
> g_hash_table_destroy(inode->posix_locks);
> pthread_mutex_destroy(&inode->plock_mutex);
> - pthread_mutex_unlock(&lo->mutex);
>
> /* Drop our refcount from lo_do_lookup() */
> lo_inode_put(lo, &inode);
> - } else {
> - pthread_mutex_unlock(&lo->mutex);
> }
> }
>
> +static void unref_inode_lolocked(struct lo_data *lo, struct lo_inode *inode,
> + uint64_t n)
> +{
> + if (!inode) {
> + return;
> + }
> +
> + pthread_mutex_lock(&lo->mutex);
> + unref_inode(lo, inode, n);
> + pthread_mutex_unlock(&lo->mutex);
> +}
> +
> static void lo_forget_one(fuse_req_t req, fuse_ino_t ino, uint64_t nlookup)
> {
> struct lo_data *lo = lo_data(req);
> @@ -2441,13 +2449,7 @@ static void lo_destroy(void *userdata)
> {
> struct lo_data *lo = (struct lo_data *)userdata;
>
> - /*
> - * Normally lo->mutex must be taken when traversing lo->inodes but
> - * lo_destroy() is a serialized request so no races are possible here.
> - *
> - * In addition, we cannot acquire lo->mutex since unref_inode() takes it
> - * too and this would result in a recursive lock.
> - */
> + pthread_mutex_lock(&lo->mutex);
> while (true) {
> GHashTableIter iter;
> gpointer key, value;
> @@ -2458,8 +2460,9 @@ static void lo_destroy(void *userdata)
> }
>
> struct lo_inode *inode = value;
> - unref_inode_lolocked(lo, inode, inode->nlookup);
> + unref_inode(lo, inode, inode->nlookup);
> }
> + pthread_mutex_unlock(&lo->mutex);
> }
>
> static struct fuse_lowlevel_ops lo_oper = {
>
Reviewed-by: Philippe Mathieu-Daudé <philmd@redhat.com>