| Message ID | 20200117133257.16906-1-philmd@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | virtiofs daemon [all] | expand |
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);
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); > > >
* 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
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
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);
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(-)