Re: [Minios-devel] [UNIKRAFT PATCH v3 14/23] lib/vfscore: keep vnode locked during drele call

[Yuri Volchkov <yuri.volchkov@xxxxxxxxx>]

Hi Sharan,

let's skip this patch at all for now. This is a part of a bigger fight,
which I unfortunately postponed for now. That is also a reason why
vn_del_name has this comment

        /* TODO: Re-enable this check when preemption and/or smp is
         * here */
        /* UK_ASSERT(uk_mutex_is_locked(&vp->v_lock)); */

The problem here is that OSv is using recursive locking, which we do not
support. I was leaning towards fixing locking scheme in a way that we do
not need nested locks, rather then implementing them. But that would
require a bit of efforts.

Since this the initial import, and sort of preparations towards a proper
vfscore, I'd say this fight is out of the scope of the series. And this
patch actually does not belong to it.

BR, Yuri.

Sharan Santhanam <sharan.santhanam@xxxxxxxxx> writes:

> Hello Yuri,
>
> Please find the comment inline
>
> Thanks & Regards
> Sharan
>
> On 2/7/19 2:58 PM, Yuri Volchkov wrote:
>> Signed-off-by: Yuri Volchkov <yuri.volchkov@xxxxxxxxx>
>> ---
>>   lib/vfscore/lookup.c   | 2 +-
>>   lib/vfscore/syscalls.c | 2 +-
>>   2 files changed, 2 insertions(+), 2 deletions(-)
>> 
>> diff --git a/lib/vfscore/lookup.c b/lib/vfscore/lookup.c
>> index 1e2c4005..cbc02b52 100644
>> --- a/lib/vfscore/lookup.c
>> +++ b/lib/vfscore/lookup.c
>> @@ -181,8 +181,8 @@ namei(const char *path, struct dentry **dpp)
>>                              /* Find a vnode in this directory. */
>>                              error = VOP_LOOKUP(dvp, name, &vp);
>>                              if (error) {
>> -                                    vn_unlock(dvp);
>>                                      drele(ddp);
>> +                                    vn_unlock(dvp);
> Why are we changing order of unlocking and releasing the dentry on the 
> lookup and not when the entry allocation fails in the statement below 
> it. A comment explaining the reason should help
>>                                      return error;
>>                              }
>>   
>> diff --git a/lib/vfscore/syscalls.c b/lib/vfscore/syscalls.c
>> index fa0498e7..33004960 100644
>> --- a/lib/vfscore/syscalls.c
>> +++ b/lib/vfscore/syscalls.c
>> @@ -132,8 +132,8 @@ sys_open(char *path, int flags, mode_t mode, struct 
>> vfscore_file **fpp)
>>                      mode &= ~S_IFMT;
>>                      mode |= S_IFREG;
>>                      error = VOP_CREATE(ddp->d_vnode, filename, mode);
>> -                    vn_unlock(ddp->d_vnode);
>>                      drele(ddp);
>> +                    vn_unlock(ddp->d_vnode);
> If the VOP_CREATE has to maintain this order releasing the parent 
> directory entry, shouldn't VOP_REMOVE operation also follow a similar 
> pattern?
>
>>   
>>                      if (error)
>>                              return error;
>> 

-- 
Yuri Volchkov
Software Specialist

NEC Europe Ltd
Kurfürsten-Anlage 36
D-69115 Heidelberg

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