Re: [PATCH v8 2/5] ceph: add caps perf metric for each session

[Jeff Layton <jlayton@kernel.org>]

On Sat, 2020-02-22 at 09:51 +0800, Xiubo Li wrote:
> On 2020/2/21 20:00, Jeff Layton wrote:
> > On Fri, 2020-02-21 at 02:05 -0500, xiubli@redhat.com wrote:
> > > From: Xiubo Li <xiubli@redhat.com>
> > > 
> > > This will fulfill the cap hit/mis metric stuff per-superblock,
> > > it will count the hit/mis counters based each inode, and if one
> > > inode's 'issued & ~revoking == mask' will mean a hit, or a miss.
> > > 
> > > item          total           miss            hit
> > > -------------------------------------------------
> > > caps          295             107             4119
> > > 
> > > URL: https://tracker.ceph.com/issues/43215
> > > Signed-off-by: Xiubo Li <xiubli@redhat.com>
> > > ---
> > >   fs/ceph/acl.c        |  2 +-
> > >   fs/ceph/caps.c       | 19 +++++++++++++++++++
> > >   fs/ceph/debugfs.c    | 16 ++++++++++++++++
> > >   fs/ceph/dir.c        |  5 +++--
> > >   fs/ceph/inode.c      |  4 ++--
> > >   fs/ceph/mds_client.c | 26 ++++++++++++++++++++++----
> > >   fs/ceph/metric.h     | 19 +++++++++++++++++++
> > >   fs/ceph/super.h      |  8 +++++---
> > >   fs/ceph/xattr.c      |  4 ++--
> > >   9 files changed, 89 insertions(+), 14 deletions(-)
> > > 
> > > diff --git a/fs/ceph/acl.c b/fs/ceph/acl.c
> > > index 26be6520d3fb..e0465741c591 100644
> > > --- a/fs/ceph/acl.c
> > > +++ b/fs/ceph/acl.c
> > > @@ -22,7 +22,7 @@ static inline void ceph_set_cached_acl(struct inode *inode,
> > >   	struct ceph_inode_info *ci = ceph_inode(inode);
> > >   
> > >   	spin_lock(&ci->i_ceph_lock);
> > > -	if (__ceph_caps_issued_mask(ci, CEPH_CAP_XATTR_SHARED, 0))
> > > +	if (__ceph_caps_issued_mask_metric(ci, CEPH_CAP_XATTR_SHARED, 0))
> > >   		set_cached_acl(inode, type, acl);
> > >   	else
> > >   		forget_cached_acl(inode, type);
> > nit: calling __ceph_caps_issued_mask_metric means that you have an extra
> > branch. One to set/forget acl and one to update the counter.
> > 
> > This would be (very slightly) more efficient if you just put the cap
> > hit/miss calls inside the existing if block above. IOW, you could just
> > do:
> > 
> > if (__ceph_caps_issued_mask(ci, CEPH_CAP_XATTR_SHARED, 0)) {
> > 	set_cached_acl(inode, type, acl);
> > 	ceph_update_cap_hit(&fsc->mdsc->metric);
> > } else {
> > 	forget_cached_acl(inode, type);
> > 	ceph_update_cap_mis(&fsc->mdsc->metric);
> > }
> 
> Yeah, this will works well here.
> 
> 
> > > diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c
> > > index ff1714fe03aa..227949c3deb8 100644
> > > --- a/fs/ceph/dir.c
> > > +++ b/fs/ceph/dir.c
> > > @@ -346,8 +346,9 @@ static int ceph_readdir(struct file *file, struct dir_context *ctx)
> > >   	    !ceph_test_mount_opt(fsc, NOASYNCREADDIR) &&
> > >   	    ceph_snap(inode) != CEPH_SNAPDIR &&
> > >   	    __ceph_dir_is_complete_ordered(ci) &&
> > > -	    __ceph_caps_issued_mask(ci, CEPH_CAP_FILE_SHARED, 1)) {
> > > +	    __ceph_caps_issued_mask_metric(ci, CEPH_CAP_FILE_SHARED, 1)) {
> > These could also just be cap_hit/mis calls inside the existing if
> > blocks.
> 
> Yeah, right in the if branch we can be sure that the 
> __ceph_caps_issued_mask() is called. But in the else branch we still 
> need to get the return value from (rc = __ceph_caps_issued_mask()), and 
> only when "rc == 0" cap_mis will need. This could simplify the code here 
> and below.
> 
> This is main reason to add the __ceph_caps_issued_mask_metric() here. 
> And if you do not like this approach I will switch it back :-)
> 

Ok, good point. Let's leave this one as-is.

-- 
Jeff Layton <jlayton@kernel.org>