On 2020/2/21 20:00, Jeff Layton wrote:
On Fri, 2020-02-21 at 02:05 -0500, 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 | 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 :-)
@@ -7,5 +7,24 @@ struct ceph_client_metric { atomic64_t total_dentries; struct percpu_counter d_lease_hit; struct percpu_counter d_lease_mis; + + struct percpu_counter i_caps_hit; + struct percpu_counter i_caps_mis; }; + +static inline void ceph_update_cap_hit(struct ceph_client_metric *m) +{ + if (!m) + return; +When are these ever NULL?
Will delete it. Thanks BRs
+ percpu_counter_inc(&m->i_caps_hit); +} + +static inline void ceph_update_cap_mis(struct ceph_client_metric *m) +{ + if (!m) + return; + + percpu_counter_inc(&m->i_caps_mis); +} #endif /* _FS_CEPH_MDS_METRIC_H */ diff --git a/fs/ceph/super.h b/fs/ceph/super.h index ebcf7612eac9..4b269dc845bb 100644 --- a/fs/ceph/super.h +++ b/fs/ceph/super.h @@ -639,6 +639,8 @@ static inline bool __ceph_is_any_real_caps(struct ceph_inode_info *ci)extern int __ceph_caps_issued(struct ceph_inode_info *ci, int *implemented);extern int __ceph_caps_issued_mask(struct ceph_inode_info *ci, int mask, int t); +extern int __ceph_caps_issued_mask_metric(struct ceph_inode_info *ci, int mask, + int t); extern int __ceph_caps_issued_other(struct ceph_inode_info *ci, struct ceph_cap *cap);@@ -651,12 +653,12 @@ static inline int ceph_caps_issued(struct ceph_inode_info *ci)return issued; }-static inline int ceph_caps_issued_mask(struct ceph_inode_info *ci, int mask,- int touch) +static inline int ceph_caps_issued_mask_metric(struct ceph_inode_info *ci, + int mask, int touch) { int r; spin_lock(&ci->i_ceph_lock); - r = __ceph_caps_issued_mask(ci, mask, touch); + r = __ceph_caps_issued_mask_metric(ci, mask, touch); spin_unlock(&ci->i_ceph_lock); return r; } diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c index 7b8a070a782d..71ee34d160c3 100644 --- a/fs/ceph/xattr.c +++ b/fs/ceph/xattr.c @@ -856,7 +856,7 @@ ssize_t __ceph_getxattr(struct inode *inode, const char *name, void *value,if (ci->i_xattrs.version == 0 ||!((req_mask & CEPH_CAP_XATTR_SHARED) || - __ceph_caps_issued_mask(ci, CEPH_CAP_XATTR_SHARED, 1))) { + __ceph_caps_issued_mask_metric(ci, CEPH_CAP_XATTR_SHARED, 1))) { spin_unlock(&ci->i_ceph_lock);/* security module gets xattr while filling trace */@@ -914,7 +914,7 @@ ssize_t ceph_listxattr(struct dentry *dentry, char *names, size_t size) ci->i_xattrs.version, ci->i_xattrs.index_version);if (ci->i_xattrs.version == 0 ||- !__ceph_caps_issued_mask(ci, CEPH_CAP_XATTR_SHARED, 1)) { + !__ceph_caps_issued_mask_metric(ci, CEPH_CAP_XATTR_SHARED, 1)) { spin_unlock(&ci->i_ceph_lock); err = ceph_do_getattr(inode, CEPH_STAT_CAP_XATTR, true); if (err)
