Re: [Minios-devel] [UNIKRAFT VFS PATCH v2 1/4] Fix dentry referecing and file closing

[Costin Lupu <costin.lup@xxxxxxxxx>]

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

On 6/29/19 2:12 PM, Mihai Pogonaru wrote:
> Increase the refcount of the vfscore_file's dentry when opening it.
> This is the OSv behaviour since each vfscore_file has an intrusive_ptr
> ref to its dentry, which increases dentry's refcount on creation and
> decreases dentry's refcount on deletion. We must increase the dentry's
> refcount since we might end up with files pointing to deleted dentries,
> if the actual file/directory the dentry refers gets deleted (so the
> dentry's refcount hits 0 and it gets freed).
> 
> We must also decrease the refcount of the dentry on file closing.
> 
> The statement "Dentry stays forever in the dentry cache. Unless the
> file/directory it refers to gets deleted/renamed" doesn't hold for
> file systems that do not save their structures on permanent mediums
> (like LWIP's sockets abstraction) so the dentry refcount is equal to
> the number of vfscore_file's pointing at it (so the dentry gets
> deleted as soon as the last file is closed).
> 
> Right now the vfscore_file is never freed, excepting the LWIP socket's
> file which is freed inside sock_net_close along with its dentry and
> vnode. This should not be the expected behaviour from a underlying
> file system (ramfs provides a good example of expected behaviour).
> As such, we must free the vfscore_file when closing it and release its
> dentry so the dentry and vnode freeing will be handled by vfs as well.
> 
> This patch also makes fdrop return 0 if the file's refcount hits
> 0 or return 1 otherwise. This is the OSv behaviour and the return
> of an error from fdrop is never checked.
> 
> Signed-off-by: Mihai Pogonaru <pogonarumihai@xxxxxxxxx>
> ---
>  lib/vfscore/file.c     | 18 ++++++++++++++----
>  lib/vfscore/fops.c     |  7 ++++---
>  lib/vfscore/syscalls.c |  3 +++
>  3 files changed, 21 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/vfscore/file.c b/lib/vfscore/file.c
> index e56f16dc..e7627e96 100644
> --- a/lib/vfscore/file.c
> +++ b/lib/vfscore/file.c
> @@ -42,15 +42,25 @@
>  
>  int fdrop(struct vfscore_file *fp)
>  {
> -     int ret = 0;
>       int prev = ukarch_dec(&fp->f_count);
>  
>       if (prev == 0)
>               UK_CRASH("Unbalanced fhold/fdrop");
>  
> -     if (prev == 1)
> -             ret = vfs_close(fp);
> -     return ret;
> +     if (prev == 1) {
> +             /*
> +              * we free the file even in case of an error
> +              * so release the dentry too
> +              */
> +             if (vfs_close(fp) != 0)
> +                     drele(fp->f_dentry);
> +
> +             free(fp);
> +
> +             return 1;
> +     }
> +
> +     return 0;
>  }
>  void fhold(struct vfscore_file *fp)
>  {
> diff --git a/lib/vfscore/fops.c b/lib/vfscore/fops.c
> index 8f5c2e1b..0cef2c12 100644
> --- a/lib/vfscore/fops.c
> +++ b/lib/vfscore/fops.c
> @@ -53,9 +53,10 @@ int vfs_close(struct vfscore_file *fp)
>       if (error)
>               return error;
>  
> -     /* Dentry stays forever in the dentry cache. Unless the
> -      * file/directory it refers to gets deleted/renamed */
> +     /* Release the dentry */
> +     drele(fp->f_dentry);
>       fp->f_dentry = NULL;
> +
>       return 0;
>  }
>  
> @@ -103,7 +104,7 @@ int vfs_write(struct vfscore_file *fp, struct uio *uio, 
> int flags)
>               ioflags |= IO_SYNC;
>  
>       if ((flags & FOF_OFFSET) == 0)
> -             uio->uio_offset = fp->f_offset;
> +             uio->uio_offset = fp->f_offset;
>  
>       error = VOP_WRITE(vp, uio, ioflags);
>       if (!error) {
> diff --git a/lib/vfscore/syscalls.c b/lib/vfscore/syscalls.c
> index 385d90c0..f9666c4c 100644
> --- a/lib/vfscore/syscalls.c
> +++ b/lib/vfscore/syscalls.c
> @@ -204,6 +204,8 @@ sys_open(char *path, int flags, mode_t mode, struct 
> vfscore_file **fpp)
>       }
>       fp->f_flags = flags;
>  
> +     // OSv was using a intrusive_ptr which was increasing the refcount
> +     dref(dp);
>       // change to std::move once dp is a dentry_ref
>       fp->f_dentry = dp;
>       dp = NULL;
> @@ -214,6 +216,7 @@ sys_open(char *path, int flags, mode_t mode, struct 
> vfscore_file **fpp)
>               // Note direct delete of fp instead of fdrop(fp). fp was never
>               // returned so cannot be in use, and because it wasn't opened
>               // it cannot be close()ed.
> +             drele(fp->f_dentry);
>               free(fp);
>               return error;
>       }
> 

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel