Re: [UNIKRAFT PATCH v2 9/9] lib/9pfs: Simplify the logic for removes

[Costin Lupu <costin.lupu@xxxxxxxxx>]

Reviewed-by: Costin Lupu <costin.lupu@xxxxxxxxx>

On 5/9/20 7:44 PM, Cristian Banu wrote:
> The logic for unlinking is simplified, either:
> - there are no open files, remove now;
> - there are open files, defer until all open files are closed
> 
> This is now possible since successive open and closes update
> the dentry reference count correctly, so VOP_INACTIVE is called
> in time to free the fid before the qid starts being reused
> on the server-side (and we get an inode collision).
> 
> This also means that on unmounting, releasing all non-root
> dentries and calling vfscore_release_mp_dentries() for m_covered
> and m_root is enough to ensure all fids are clunked before
> disconnecting.
> 
> Signed-off-by: Cristian Banu <cristb@xxxxxxxxx>
> ---
>  lib/9pfs/9pfs_vfsops.c |  7 +++----
>  lib/9pfs/9pfs_vnops.c  | 37 +++++++++++--------------------------
>  2 files changed, 14 insertions(+), 30 deletions(-)
> 
> diff --git a/lib/9pfs/9pfs_vfsops.c b/lib/9pfs/9pfs_vfsops.c
> index accb3a5..5293b22 100644
> --- a/lib/9pfs/9pfs_vfsops.c
> +++ b/lib/9pfs/9pfs_vfsops.c
> @@ -169,11 +169,10 @@ static void uk_9pfs_release_tree_fids(struct dentry *d)
>  {
>       struct dentry *p;
>  
> -     uk_list_for_each_entry(p, &d->d_child_list, d_child_link)
> +     uk_list_for_each_entry(p, &d->d_child_list, d_child_link) {
>               uk_9pfs_release_tree_fids(p);
> -
> -     if (d->d_vnode->v_data)
> -             uk_9pfs_free_vnode_data(d->d_vnode);
> +             drele(p);
> +     }
>  }
>  
>  static int uk_9pfs_unmount(struct mount *mp, int flags __unused)
> diff --git a/lib/9pfs/9pfs_vnops.c b/lib/9pfs/9pfs_vnops.c
> index 6d3ece3..c81c82a 100644
> --- a/lib/9pfs/9pfs_vnops.c
> +++ b/lib/9pfs/9pfs_vnops.c
> @@ -147,7 +147,7 @@ void uk_9pfs_free_vnode_data(struct vnode *vp)
>       struct uk_9pdev *dev = UK_9PFS_MD(vp->v_mount)->dev;
>       struct uk_9pfs_node_data *nd = UK_9PFS_ND(vp);
>  
> -     if (nd->nb_open_files > 0)
> +     if (!vp->v_data)
>               return;
>  
>       if (nd->removed)
> @@ -158,20 +158,6 @@ void uk_9pfs_free_vnode_data(struct vnode *vp)
>       vp->v_data = NULL;
>  }
>  
> -/*
> - * The closing variant of the function will enforce freeing the associated
> - * resources only if the vnode was removed via an unlink/rmdir operation.
> - */
> -static void uk_9pfs_free_vnode_data_closing(struct vnode *vp)
> -{
> -     struct uk_9pfs_node_data *nd = UK_9PFS_ND(vp);
> -
> -     if (!nd->removed)
> -             return;
> -
> -     uk_9pfs_free_vnode_data(vp);
> -}
> -
>  static int uk_9pfs_open(struct vfscore_file *file)
>  {
>       struct uk_9pdev *dev = UK_9PFS_MD(file->f_dentry->d_mount)->dev;
> @@ -222,7 +208,6 @@ static int uk_9pfs_close(struct vnode *vn __unused, 
> struct vfscore_file *file)
>       uk_9pfid_put(fd->fid);
>       free(fd);
>       UK_9PFS_ND(file->f_dentry->d_vnode)->nb_open_files--;
> -     uk_9pfs_free_vnode_data_closing(file->f_dentry->d_vnode);
>  
>       return 0;
>  }
> @@ -327,22 +312,22 @@ static int uk_9pfs_remove_generic(struct vnode *dvp, 
> struct vnode *vp)
>  {
>       struct uk_9pdev *dev = UK_9PFS_MD(dvp->v_mount)->dev;
>       struct uk_9pfs_node_data *nd = UK_9PFS_ND(vp);
> -     int rc = 0;
> -
> -     if (!nd->removed && !nd->nb_open_files)
> -             rc = uk_9p_remove(dev, nd->fid);
> -     else
> -             nd->removed = true;
> -
> -     uk_9pfs_free_vnode_data(vp);
>  
> -     return -rc;
> +     return -uk_9p_remove(dev, nd->fid);
>  }
>  
>  static int uk_9pfs_remove(struct vnode *dvp, struct vnode *vp,
>               char *name __unused)
>  {
> -     return uk_9pfs_remove_generic(dvp, vp);
> +     struct uk_9pfs_node_data *nd = UK_9PFS_ND(vp);
> +     int rc = 0;
> +
> +     if (!nd->nb_open_files)
> +             rc = uk_9pfs_remove_generic(dvp, vp);
> +     else
> +             nd->removed = true;
> +
> +     return rc;
>  }
>  
>  static int uk_9pfs_mkdir(struct vnode *dvp, char *name, mode_t mode)
>