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

[Costin Lupu <costin.lup@xxxxxxxxx>]

Hi Mihai,

As a general rule, we are pedantic when reviewing patches for the kernel
because they might have side-effects and each changes should be properly
and extensively explained. That being said, most of my comments for this
series are about the presentation/format of these patches (about how
they are organized, about comments, etc.).

Please see inline.

On 6/26/19 4:54 AM, Mihai Pogonaru wrote:
> Increase the refcount of the dentry when opening a file
> Decrease the refcount of the dentry and free the file on closing
> 
> Make fdrop return 0 if the file's refcount hit 0 or 1 otherwise.
> This is the OSv behaviour and the return of an error from fdrop
> is never checked.

From what I understand, this patch fixes a proper handling of dentries
refcounts when it comes to vfscore file abstraction. This should be the
main idea in the commit message, the details you stated here come after
that. In conclusion, this commit message has to be a bit more detailed
given that the changes are a bit tricky and difficult to understand at
the beginning.

> 
> 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 */

We had a strong statement here about not deleting dentries and it's
overwritten by this patch. I don't think that at the time when this was
introduced, the authors thought about sockets. Therefore we should
outline how this change is related to sockets, we have to provide this
explanation in the commit message.

> +     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