Re: [PATCH v5 0/2] ceph: metrics for opened files, pinned caps and opened inodes

[Xiubo Li <xiubli@redhat.com>]

On 2020/9/10 4:34, Ilya Dryomov wrote:
> On Thu, Sep 3, 2020 at 4:22 PM Xiubo Li <xiubli@redhat.com> wrote:
>> On 2020/9/3 22:18, Jeff Layton wrote:
>>> On Thu, 2020-09-03 at 09:01 -0400, xiubli@redhat.com wrote:
>>>> From: Xiubo Li <xiubli@redhat.com>
>>>>
>>>> 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.

Thanks Ilya.


> Thanks,
>
>                  Ilya
>