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

[Xiubo Li <xiubli@redhat.com>]

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@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     | 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@kernel.org>
> ---
>   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.

Thanks

Brs