[105/104] virtiofsd: Unref old/new inodes with the same mutex lock in lo_rename()
diff mbox series

Message ID 20200117133257.16906-1-philmd@redhat.com
State New
Headers show
Series
  • virtiofs daemon [all]
Related show

Commit Message

Philippe Mathieu-Daudé Jan. 17, 2020, 1:32 p.m. UTC
We can unref both old/new inodes with the same mutex lock.

Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
---
Based-on: <20191212163904.159893-1-dgilbert@redhat.com>
"virtiofs daemon"
https://www.mail-archive.com/qemu-devel@nongnu.org/msg664652.html

 tools/virtiofsd/passthrough_ll.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Comments

Xiao Yang Jan. 19, 2020, 8:35 a.m. UTC | #1
On 2020/1/17 21:32, Philippe Mathieu-Daudé wrote:
> We can unref both old/new inodes with the same mutex lock.
>
> Signed-off-by: Philippe Mathieu-Daudé<philmd@redhat.com>
> ---
> Based-on:<20191212163904.159893-1-dgilbert@redhat.com>
> "virtiofs daemon"
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg664652.html
>
>   tools/virtiofsd/passthrough_ll.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index 57f58aef26..5c717cb5a1 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -1461,8 +1461,10 @@ static void lo_rename(fuse_req_t req, fuse_ino_t parent, const char *name,
>       }
>
>   out:
> -    unref_inode_lolocked(lo, oldinode, 1);
> -    unref_inode_lolocked(lo, newinode, 1);
> +    pthread_mutex_lock(&lo->mutex);
> +    unref_inode(lo, oldinode, 1);
> +    unref_inode(lo, newinode, 1);
> +    pthread_mutex_unlock(&lo->mutex);
Hi,

It seems to avoid calling pthread_mutex_lock and pthread_mutex_unlock twice.
Does the change fix some issues or improve the performance?

Best Regards,
Xiao Yang
>       lo_inode_put(lo,&oldinode);
>       lo_inode_put(lo,&newinode);
>       lo_inode_put(lo,&parent_inode);
Philippe Mathieu-Daudé Jan. 20, 2020, 8:27 a.m. UTC | #2
On 1/19/20 9:35 AM, Xiao Yang wrote:
> On 2020/1/17 21:32, Philippe Mathieu-Daudé wrote:
>> We can unref both old/new inodes with the same mutex lock.
>>
>> Signed-off-by: Philippe Mathieu-Daudé<philmd@redhat.com>
>> ---
>> Based-on:<20191212163904.159893-1-dgilbert@redhat.com>
>> "virtiofs daemon"
>> https://www.mail-archive.com/qemu-devel@nongnu.org/msg664652.html
>>
>>   tools/virtiofsd/passthrough_ll.c | 6 ++++--
>>   1 file changed, 4 insertions(+), 2 deletions(-)
>>
>> diff --git a/tools/virtiofsd/passthrough_ll.c 
>> b/tools/virtiofsd/passthrough_ll.c
>> index 57f58aef26..5c717cb5a1 100644
>> --- a/tools/virtiofsd/passthrough_ll.c
>> +++ b/tools/virtiofsd/passthrough_ll.c
>> @@ -1461,8 +1461,10 @@ static void lo_rename(fuse_req_t req, 
>> fuse_ino_t parent, const char *name,
>>       }
>>
>>   out:
>> -    unref_inode_lolocked(lo, oldinode, 1);
>> -    unref_inode_lolocked(lo, newinode, 1);
>> +    pthread_mutex_lock(&lo->mutex);
>> +    unref_inode(lo, oldinode, 1);
>> +    unref_inode(lo, newinode, 1);
>> +    pthread_mutex_unlock(&lo->mutex);
> Hi,
> 
> It seems to avoid calling pthread_mutex_lock and pthread_mutex_unlock 
> twice.
> Does the change fix some issues or improve the performance?

No issue, simply intend to improve the performance.

> Best Regards,
> Xiao Yang
>>       lo_inode_put(lo,&oldinode);
>>       lo_inode_put(lo,&newinode);
>>       lo_inode_put(lo,&parent_inode);
> 
> 
>
Dr. David Alan Gilbert Jan. 20, 2020, 6:52 p.m. UTC | #3
* Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
> We can unref both old/new inodes with the same mutex lock.
> 
> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> ---
> Based-on: <20191212163904.159893-1-dgilbert@redhat.com>
> "virtiofs daemon"
> https://www.mail-archive.com/qemu-devel@nongnu.org/msg664652.html
> 
>  tools/virtiofsd/passthrough_ll.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> index 57f58aef26..5c717cb5a1 100644
> --- a/tools/virtiofsd/passthrough_ll.c
> +++ b/tools/virtiofsd/passthrough_ll.c
> @@ -1461,8 +1461,10 @@ static void lo_rename(fuse_req_t req, fuse_ino_t parent, const char *name,
>      }
>  
>  out:
> -    unref_inode_lolocked(lo, oldinode, 1);
> -    unref_inode_lolocked(lo, newinode, 1);
> +    pthread_mutex_lock(&lo->mutex);
> +    unref_inode(lo, oldinode, 1);
> +    unref_inode(lo, newinode, 1);
> +    pthread_mutex_unlock(&lo->mutex);

While that would work; I'd rather keep that code simpler and the
same as every other normal operation - we only use the unref_inode
in one other place and that's because we're iterating the hash table
while deleting stuff.

Dave

>      lo_inode_put(lo, &oldinode);
>      lo_inode_put(lo, &newinode);
>      lo_inode_put(lo, &parent_inode);
> -- 
> 2.21.1
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
Philippe Mathieu-Daudé Jan. 20, 2020, 6:55 p.m. UTC | #4
On Mon, Jan 20, 2020 at 7:52 PM Dr. David Alan Gilbert
<dgilbert@redhat.com> wrote:
> * Philippe Mathieu-Daudé (philmd@redhat.com) wrote:
> > We can unref both old/new inodes with the same mutex lock.
> >
> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com>
> > ---
> > Based-on: <20191212163904.159893-1-dgilbert@redhat.com>
> > "virtiofs daemon"
> > https://www.mail-archive.com/qemu-devel@nongnu.org/msg664652.html
> >
> >  tools/virtiofsd/passthrough_ll.c | 6 ++++--
> >  1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
> > index 57f58aef26..5c717cb5a1 100644
> > --- a/tools/virtiofsd/passthrough_ll.c
> > +++ b/tools/virtiofsd/passthrough_ll.c
> > @@ -1461,8 +1461,10 @@ static void lo_rename(fuse_req_t req, fuse_ino_t parent, const char *name,
> >      }
> >
> >  out:
> > -    unref_inode_lolocked(lo, oldinode, 1);
> > -    unref_inode_lolocked(lo, newinode, 1);
> > +    pthread_mutex_lock(&lo->mutex);
> > +    unref_inode(lo, oldinode, 1);
> > +    unref_inode(lo, newinode, 1);
> > +    pthread_mutex_unlock(&lo->mutex);
>
> While that would work; I'd rather keep that code simpler and the
> same as every other normal operation - we only use the unref_inode
> in one other place and that's because we're iterating the hash table
> while deleting stuff.

OK I understand.

> Dave
>
> >      lo_inode_put(lo, &oldinode);
> >      lo_inode_put(lo, &newinode);
> >      lo_inode_put(lo, &parent_inode);
> > --
> > 2.21.1
> >
> --
> Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Patch
diff mbox series

diff --git a/tools/virtiofsd/passthrough_ll.c b/tools/virtiofsd/passthrough_ll.c
index 57f58aef26..5c717cb5a1 100644
--- a/tools/virtiofsd/passthrough_ll.c
+++ b/tools/virtiofsd/passthrough_ll.c
@@ -1461,8 +1461,10 @@  static void lo_rename(fuse_req_t req, fuse_ino_t parent, const char *name,
     }
 
 out:
-    unref_inode_lolocked(lo, oldinode, 1);
-    unref_inode_lolocked(lo, newinode, 1);
+    pthread_mutex_lock(&lo->mutex);
+    unref_inode(lo, oldinode, 1);
+    unref_inode(lo, newinode, 1);
+    pthread_mutex_unlock(&lo->mutex);
     lo_inode_put(lo, &oldinode);
     lo_inode_put(lo, &newinode);
     lo_inode_put(lo, &parent_inode);