| Message ID | 1583739430-4928-3-git-send-email-xiubli@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ceph: add perf metrics support | expand |
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;
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,
On 2020/3/9 19:51, 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? A negative error code could also from the case that if have & need == need, but it may fail with ret = -EAGAIN. Or maybe could we just move this to : if ((have & need) == need){ hit() } else { miss() } Thanks BRs >> + 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;
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
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@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. > 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. Thanks!
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@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. >> > 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!
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); + 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; diff --git a/fs/ceph/debugfs.c b/fs/ceph/debugfs.c index 15975ba..c83e52b 100644 --- a/fs/ceph/debugfs.c +++ b/fs/ceph/debugfs.c @@ -128,6 +128,7 @@ static int metric_show(struct seq_file *s, void *p) { struct ceph_fs_client *fsc = s->private; struct ceph_mds_client *mdsc = fsc->mdsc; + int i, nr_caps = 0; seq_printf(s, "item total miss hit\n"); seq_printf(s, "-------------------------------------------------\n"); @@ -137,6 +138,21 @@ static int metric_show(struct seq_file *s, void *p) percpu_counter_sum(&mdsc->metric.d_lease_mis), percpu_counter_sum(&mdsc->metric.d_lease_hit)); + mutex_lock(&mdsc->mutex); + for (i = 0; i < mdsc->max_sessions; i++) { + struct ceph_mds_session *s; + + s = __ceph_lookup_mds_session(mdsc, i); + if (!s) + continue; + nr_caps += s->s_nr_caps; + ceph_put_mds_session(s); + } + mutex_unlock(&mdsc->mutex); + seq_printf(s, "%-14s%-16d%-16lld%lld\n", "caps", nr_caps, + percpu_counter_sum(&mdsc->metric.i_caps_mis), + percpu_counter_sum(&mdsc->metric.i_caps_hit)); + return 0; } diff --git a/fs/ceph/dir.c b/fs/ceph/dir.c index 8097a86..10d528a 100644 --- a/fs/ceph/dir.c +++ b/fs/ceph/dir.c @@ -349,8 +349,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)) { int shared_gen = atomic_read(&ci->i_shared_gen); + spin_unlock(&ci->i_ceph_lock); err = __dcache_readdir(file, ctx, shared_gen); if (err != -EAGAIN) @@ -767,7 +768,7 @@ static struct dentry *ceph_lookup(struct inode *dir, struct dentry *dentry, !is_root_ceph_dentry(dir, dentry) && ceph_test_mount_opt(fsc, DCACHE) && __ceph_dir_is_complete(ci) && - (__ceph_caps_issued_mask(ci, CEPH_CAP_FILE_SHARED, 1))) { + __ceph_caps_issued_mask_metric(ci, CEPH_CAP_FILE_SHARED, 1)) { __ceph_touch_fmode(ci, mdsc, CEPH_FILE_MODE_RD); spin_unlock(&ci->i_ceph_lock); dout(" dir %p complete, -ENOENT\n", dir); diff --git a/fs/ceph/inode.c b/fs/ceph/inode.c index ee40ba7..b5a30ca6 100644 --- a/fs/ceph/inode.c +++ b/fs/ceph/inode.c @@ -2284,8 +2284,8 @@ int __ceph_do_getattr(struct inode *inode, struct page *locked_page, dout("do_getattr inode %p mask %s mode 0%o\n", inode, ceph_cap_string(mask), inode->i_mode); - if (!force && ceph_caps_issued_mask(ceph_inode(inode), mask, 1)) - return 0; + if (!force && ceph_caps_issued_mask_metric(ceph_inode(inode), mask, 1)) + return 0; mode = (mask & CEPH_STAT_RSTAT) ? USE_AUTH_MDS : USE_ANY_MDS; req = ceph_mdsc_create_request(mdsc, CEPH_MDS_OP_GETATTR, mode); diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c index 0e2557b..ba54fd2 100644 --- a/fs/ceph/mds_client.c +++ b/fs/ceph/mds_client.c @@ -4332,13 +4332,29 @@ static int ceph_mdsc_metric_init(struct ceph_client_metric *metric) ret = percpu_counter_init(&metric->d_lease_hit, 0, GFP_KERNEL); if (ret) return ret; + ret = percpu_counter_init(&metric->d_lease_mis, 0, GFP_KERNEL); - if (ret) { - percpu_counter_destroy(&metric->d_lease_hit); - return ret; - } + if (ret) + goto err_d_lease_mis; + + ret = percpu_counter_init(&metric->i_caps_hit, 0, GFP_KERNEL); + if (ret) + goto err_i_caps_hit; + + ret = percpu_counter_init(&metric->i_caps_mis, 0, GFP_KERNEL); + if (ret) + goto err_i_caps_mis; return 0; + +err_i_caps_mis: + percpu_counter_destroy(&metric->i_caps_hit); +err_i_caps_hit: + percpu_counter_destroy(&metric->d_lease_mis); +err_d_lease_mis: + percpu_counter_destroy(&metric->d_lease_hit); + + return ret; } int ceph_mdsc_init(struct ceph_fs_client *fsc) @@ -4678,6 +4694,8 @@ void ceph_mdsc_destroy(struct ceph_fs_client *fsc) ceph_mdsc_stop(mdsc); + percpu_counter_destroy(&mdsc->metric.i_caps_mis); + percpu_counter_destroy(&mdsc->metric.i_caps_hit); percpu_counter_destroy(&mdsc->metric.d_lease_mis); percpu_counter_destroy(&mdsc->metric.d_lease_hit); diff --git a/fs/ceph/metric.h b/fs/ceph/metric.h index 998fe2a..f620f72 100644 --- a/fs/ceph/metric.h +++ b/fs/ceph/metric.h @@ -7,5 +7,18 @@ 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) +{ + percpu_counter_inc(&m->i_caps_hit); +} + +static inline void ceph_update_cap_mis(struct ceph_client_metric *m) +{ + 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 5c73cf1..47cfd89 100644 --- a/fs/ceph/super.h +++ b/fs/ceph/super.h @@ -645,6 +645,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); @@ -657,12 +659,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 7b8a070..71ee34d 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)