Re: [PATCH] ceph: make sure all the files successfully put before unmounting

[Eric Biggers <ebiggers@kernel.org>]

Hi Xiubo,

On Fri, Dec 02, 2022 at 03:04:58PM +0800, Xiubo Li wrote:
> 
> On 02/12/2022 12:19, Eric Biggers wrote:
> > On Fri, Dec 02, 2022 at 09:49:49AM +0800, Xiubo Li wrote:
> > > On 02/12/2022 05:10, Eric Biggers wrote:
> > > > On Thu, Dec 01, 2022 at 11:18:33AM -0800, Eric Biggers wrote:
> > > > > On Thu, Dec 01, 2022 at 02:58:00PM +0800, xiubli@redhat.com wrote:
> > > > > > From: Xiubo Li <xiubli@redhat.com>
> > > > > > 
> > > > > > When close a file it will be deferred to call the fput(), which
> > > > > > will hold the inode's i_count. And when unmounting the mountpoint
> > > > > > the evict_inodes() may skip evicting some inodes.
> > > > > > 
> > > > > > If encrypt is enabled the kernel generate a warning when removing
> > > > > > the encrypt keys when the skipped inodes still hold the keyring:
> > > > > This does not make sense.  Unmounting is only possible once all the files on the
> > > > > filesystem have been closed.
> > > > > 
> > > > Specifically, __fput() puts the reference to the dentry (and thus the inode)
> > > > *before* it puts the reference to the mount.  And an unmount cannot be done
> > > > while the mount still has references.  So there should not be any issue here.
> > > Eric,
> > > 
> > > When I unmounting I can see the following logs, which I added a debug log in
> > > the evcit_inodes():
> > > 
> > > diff --git a/fs/inode.c b/fs/inode.c
> > > index b608528efd3a..f6e69b778d9c 100644
> > > --- a/fs/inode.c
> > > +++ b/fs/inode.c
> > > @@ -716,8 +716,11 @@ void evict_inodes(struct super_block *sb)
> > >   again:
> > >          spin_lock(&sb->s_inode_list_lock);
> > >          list_for_each_entry_safe(inode, next, &sb->s_inodes, i_sb_list) {
> > > -               if (atomic_read(&inode->i_count))
> > > +               if (atomic_read(&inode->i_count)) {
> > > +                       printk("evict_inodes inode %p, i_count = %d, was
> > > skipped!\n",
> > > +                              inode, atomic_read(&inode->i_count));
> > >                          continue;
> > > +               }
> > > 
> > >                  spin_lock(&inode->i_lock);
> > >                  if (inode->i_state & (I_NEW | I_FREEING | I_WILL_FREE)) {
> > > 
> > > The logs:
> > > 
> > > <4>[   95.977395] evict_inodes inode 00000000f90aab7b, i_count = 1, was
> > > skipped!
> > > 
> > > Any reason could cause this ? Since the inode couldn't be evicted in time
> > > and then when removing the master keys it will print this warning.
> > > 
> > It is expected for evict_inodes() to see some inodes with nonzero refcount, but
> > they should only be filesystem internal inodes.  For example, with ext4 this
> > happens with the journal inode.
> > 
> > However, filesystem internal inodes cannot be encrypted, so they are irrelevant
> > here.
> > 
> > I'd guess that CephFS has a bug where it is leaking a reference to a user inode
> > somewhere.
> 
> I also added some debug logs to tracker all the inodes in ceph, and all the
> requests has been finished.
> 
> I will debug it more to see whether it's leaking a reference here.
> 
> Thanks Eric.
> 

Any progress on tracking this down?

- Eric