On Thu, Sep 10, 2020 at 2:59 AM Xiubo Li <xiubli@xxxxxxxxxx> wrote:
>
> On 2020/9/10 4:34, Ilya Dryomov wrote:
> > On Thu, Sep 3, 2020 at 4:22 PM Xiubo Li <xiubli@xxxxxxxxxx> wrote:
> >> On 2020/9/3 22:18, Jeff Layton wrote:
> >>> On Thu, 2020-09-03 at 09:01 -0400, xiubli@xxxxxxxxxx wrote:
> >>>> From: Xiubo Li <xiubli@xxxxxxxxxx>
> >>>>
> >>>> Changed in V5:
> >>>> - Remove mdsc parsing helpers except the ceph_sb_to_mdsc()
> >>>> - Remove the is_opened member.
> >>>>
> >>>> Changed in V4:
> >>>> - A small fix about the total_inodes.
> >>>>
> >>>> Changed in V3:
> >>>> - Resend for V2 just forgot one patch, which is adding some helpers
> >>>> support to simplify the code.
> >>>>
> >>>> Changed in V2:
> >>>> - Add number of inodes that have opened files.
> >>>> - Remove the dir metrics and fold into files.
> >>>>
> >>>>
> >>>>
> >>>> Xiubo Li (2):
> >>>> ceph: add ceph_sb_to_mdsc helper support to parse the mdsc
> >>>> ceph: metrics for opened files, pinned caps and opened inodes
> >>>>
> >>>> fs/ceph/caps.c | 41 +++++++++++++++++++++++++++++++++++++----
> >>>> fs/ceph/debugfs.c | 11 +++++++++++
> >>>> fs/ceph/dir.c | 20 +++++++-------------
> >>>> fs/ceph/file.c | 13 ++++++-------
> >>>> fs/ceph/inode.c | 11 ++++++++---
> >>>> fs/ceph/locks.c | 2 +-
> >>>> fs/ceph/metric.c | 14 ++++++++++++++
> >>>> fs/ceph/metric.h | 7 +++++++
> >>>> fs/ceph/quota.c | 10 +++++-----
> >>>> fs/ceph/snap.c | 2 +-
> >>>> fs/ceph/super.h | 6 ++++++
> >>>> 11 files changed, 103 insertions(+), 34 deletions(-)
> >>>>
> >>> Looks good. I went ahead and merge this into testing.
> >>>
> >>> Small merge conflict in quota.c, which I guess is probably due to not
> >>> basing this on testing branch. I also dropped what looks like an
> >>> unrelated hunk in the second patch.
> >>>
> >>> In the future, if you can be sure that patches you post apply cleanly to
> >>> testing branch then that would make things easier.
> >> Okay, will do it.
> > Hi Xiubo,
> >
> > There is a problem with lifetimes here. mdsc isn't guaranteed to exist
> > when ->free_inode() is called. This can lead to crashes on a NULL mdsc
> > in ceph_free_inode() in case of e.g. "umount -f". I know it was Jeff's
> > suggestion to move the decrement of total_inodes into ceph_free_inode(),
> > but it doesn't look like it can be easily deferred past ->evict_inode().
>
> Okay, I will take a look.
Given that it's just a counter which we don't care about if the
mount is going away, some form of "if (mdsc)" check might do, but
need to make sure that it covers possible races, if any.
Thanks,
Ilya
