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

[Xiubo Li <xiubli@xxxxxxxxxx>]

On 2020/3/10 2:22, Jeff Layton wrote:
> On Mon, 2020-03-09 at 20:36 +0800, Xiubo Li wrote:
> > On 2020/3/9 20:05, Jeff Layton wrote:
> > > On Mon, 2020-03-09 at 07:51 -0400, Jeff Layton wrote:
> > > > On Mon, 2020-03-09 at 03:37 -0400, xiubli@xxxxxxxxxx wrote:
> > > > > From: Xiubo Li <xiubli@xxxxxxxxxx>
> > > > >
> > > > > 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@xxxxxxxxxx>
> > > > > ---
> > > > >    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     | 13 +++++++++++++
> > > > >    fs/ceph/super.h      |  8 +++++---
> > > > >    fs/ceph/xattr.c      |  4 ++--
> > > > >    9 files changed, 83 insertions(+), 14 deletions(-)
> > > > >
> > > > > diff --git a/fs/ceph/acl.c b/fs/ceph/acl.c
> > > > > index 26be652..e046574 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);
> > > > > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> > > > > index 342a32c..efaeb67 100644
> > > > > --- a/fs/ceph/caps.c
> > > > > +++ b/fs/ceph/caps.c
> > > > > @@ -912,6 +912,20 @@ int __ceph_caps_issued_mask(struct ceph_inode_info *ci, int mask, int touch)
> > > > >    	return 0;
> > > > >    }
> > > > >    
> > > > > +int __ceph_caps_issued_mask_metric(struct ceph_inode_info *ci, int mask,
> > > > > +				   int touch)
> > > > > +{
> > > > > +	struct ceph_fs_client *fsc = ceph_sb_to_client(ci->vfs_inode.i_sb);
> > > > > +	int r;
> > > > > +
> > > > > +	r = __ceph_caps_issued_mask(ci, mask, touch);
> > > > > +	if (r)
> > > > > +		ceph_update_cap_hit(&fsc->mdsc->metric);
> > > > > +	else
> > > > > +		ceph_update_cap_mis(&fsc->mdsc->metric);
> > > > > +	return r;
> > > > > +}
> > > > > +
> > > > >    /*
> > > > >     * Return true if mask caps are currently being revoked by an MDS.
> > > > >     */
> > > > > @@ -2680,6 +2694,11 @@ static int try_get_cap_refs(struct inode *inode, int need, int want,
> > > > >    	if (snap_rwsem_locked)
> > > > >    		up_read(&mdsc->snap_rwsem);
> > > > >    
> > > > > +	if (!ret)
> > > > > +		ceph_update_cap_mis(&mdsc->metric);
> > > > Should a negative error code here also mean a miss?
> > > >
> > > > > +	else if (ret == 1)
> > > > > +		ceph_update_cap_hit(&mdsc->metric);
> > > > > +
> > > > >    	dout("get_cap_refs %p ret %d got %s\n", inode,
> > > > >    	     ret, ceph_cap_string(*got));
> > > > >    	return ret;
> > > Here's what I'd propose on top. If this looks ok, then I can just fold
> > > this patch into yours before merging. No need to resend just for this.
> > >
> > > ----------------8<----------------
> > >
> > > [PATCH] SQUASH: count negative error code as miss in try_get_cap_refs
> > >
> > > Signed-off-by: Jeff Layton <jlayton@xxxxxxxxxx>
> > > ---
> > >    fs/ceph/caps.c | 4 ++--
> > >    1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> > > index efaeb67d584c..3be928782b45 100644
> > > --- a/fs/ceph/caps.c
> > > +++ b/fs/ceph/caps.c
> > > @@ -2694,9 +2694,9 @@ static int try_get_cap_refs(struct inode *inode,
> > > int need, int want,
> > >    	if (snap_rwsem_locked)
> > >    		up_read(&mdsc->snap_rwsem);
> > >    
> > > -	if (!ret)
> > > +	if (ret <= 0)
> > >    		ceph_update_cap_mis(&mdsc->metric);
> > > -	else if (ret == 1)
> > > +	else
> > >    		ceph_update_cap_hit(&mdsc->metric);
> > >    
> > >    	dout("get_cap_refs %p ret %d got %s\n", inode,
> > For the try_get_cap_refs(), maybe this is the correct approach to count
> > hit/miss as the function comments.
> I decided to just merge your patches as-is. Given that those are error
> conditions, and some of them may occur before we ever check the caps, I
> think we should just opt to not count those cases.
>
> I did clean up the changelogs a bit, so please have a look and let me
> know if they look ok to you.

Cool, it looks nice to me.

Thanks Jeff.

BRs


> Thanks!