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

[Xiubo Li <xiubli@redhat.com>]

On 2020/9/10 20:13, Jeff Layton wrote:
> On Thu, 2020-09-10 at 08:00 +0200, Ilya Dryomov wrote:
>> On Thu, Sep 10, 2020 at 2:59 AM Xiubo Li <xiubli@redhat.com> wrote:
>>> 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.
>> 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.
>>
> Good catch, Ilya.
>
> What may be best is to move the increment out of ceph_alloc_inode and
> instead put it in ceph_set_ino_cb. Then the decrement can go back into
> ceph_evict_inode.

Hi Jeff, Ilya

Checked the code, it seems in the ceph_evict_inode() we will also hit 
the same issue .

With the '-f' options when umounting, it will skip the inodes whose 
i_count ref > 0. And then free the fsc/mdsc in ceph. And later the 
iput_final() will call the ceph_evict_inode() and then ceph_free_inode().

Could we just check if !!(sb->s_flags & SB_ACTIVE) is false will we skip 
the counting ?

Thanks


> That will mean that you're only counting hashed inodes, but that's
> mostly what we're concerned with anyway, so I don't see that as a
> problem.