Re: [PATCH 3/3] ceph: don't WARN if we're iterate removing the session caps

[Xiubo Li <xiubli@xxxxxxxxxx>]

On 8/23/21 9:59 PM, Jeff Layton wrote:
> On Wed, 2021-08-18 at 16:06 +0800, xiubli@xxxxxxxxxx wrote:
> > From: Xiubo Li <xiubli@xxxxxxxxxx>
> >
> > For example in case force umounting it will remove all the session
> > caps one by one even it's dirty cap.
> >
> > URL: https://tracker.ceph.com/issues/52295
> > Signed-off-by: Xiubo Li <xiubli@xxxxxxxxxx>
> > ---
> >   fs/ceph/caps.c       | 15 ++++++++-------
> >   fs/ceph/mds_client.c |  4 ++--
> >   fs/ceph/super.h      |  3 ++-
> >   3 files changed, 12 insertions(+), 10 deletions(-)
> >
> > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> > index 7def99fbdca6..1ed9b9d57dd3 100644
> > --- a/fs/ceph/caps.c
> > +++ b/fs/ceph/caps.c
> > @@ -1101,7 +1101,7 @@ int ceph_is_any_caps(struct inode *inode)
> >    * caller should hold i_ceph_lock.
> >    * caller will not hold session s_mutex if called from destroy_inode.
> >    */
> > -void __ceph_remove_cap(struct ceph_cap *cap, bool queue_release)
> > +void __ceph_remove_cap(struct ceph_cap *cap, bool queue_release, bool warn)
> I think it'd be better to refactor __ceph_remove_cap so that it has a
> wrapper function that does the WARN_ON_ONCE call under the right
> conditions.
>
> Maybe add a ceph_remove_cap() that does:
>
> 	WARN_ON_ONCE(ci && ci->i_auth_cap == cap &&
> 		     !list_empty(&ci->i_dirty_item) &&
> 		     !mdsc->fsc->blocklisted);
>
> ...and then calls __ceph_remove_cap(). Then you could have the ones that
> set "warn" to false call __ceph_remove_cap() and the others would call
> ceph_remove_cap(). That's at least a little less ugly (and more
> efficient).

Sure, make sense.

> Alternately, I guess you could try to test what state the session is in
> and only warn if it's not being force-unmounted?

I will try this later.

Thanks


> >   {
> >   	struct ceph_mds_session *session = cap->session;
> >   	struct ceph_inode_info *ci = cap->ci;
> > @@ -1121,7 +1121,7 @@ void __ceph_remove_cap(struct ceph_cap *cap, bool queue_release)
> >   	/* remove from inode's cap rbtree, and clear auth cap */
> >   	rb_erase(&cap->ci_node, &ci->i_caps);
> >   	if (ci->i_auth_cap == cap) {
> > -		WARN_ON_ONCE(!list_empty(&ci->i_dirty_item) &&
> > +		WARN_ON_ONCE(warn && !list_empty(&ci->i_dirty_item) &&
> >   			     !mdsc->fsc->blocklisted);
> >   		ci->i_auth_cap = NULL;
> >   	}
> > @@ -1304,7 +1304,7 @@ void __ceph_remove_caps(struct ceph_inode_info *ci)
> >   	while (p) {
> >   		struct ceph_cap *cap = rb_entry(p, struct ceph_cap, ci_node);
> >   		p = rb_next(p);
> > -		__ceph_remove_cap(cap, true);
> > +		__ceph_remove_cap(cap, true, true);
> >   	}
> >   	spin_unlock(&ci->i_ceph_lock);
> >   }
> > @@ -3815,7 +3815,7 @@ static void handle_cap_export(struct inode *inode, struct ceph_mds_caps *ex,
> >   		goto out_unlock;
> >   
> >   	if (target < 0) {
> > -		__ceph_remove_cap(cap, false);
> > +		__ceph_remove_cap(cap, false, true);
> >   		goto out_unlock;
> >   	}
> >   
> > @@ -3850,7 +3850,7 @@ static void handle_cap_export(struct inode *inode, struct ceph_mds_caps *ex,
> >   				change_auth_cap_ses(ci, tcap->session);
> >   			}
> >   		}
> > -		__ceph_remove_cap(cap, false);
> > +		__ceph_remove_cap(cap, false, true);
> >   		goto out_unlock;
> >   	} else if (tsession) {
> >   		/* add placeholder for the export tagert */
> > @@ -3867,7 +3867,7 @@ static void handle_cap_export(struct inode *inode, struct ceph_mds_caps *ex,
> >   			spin_unlock(&mdsc->cap_dirty_lock);
> >   		}
> >   
> > -		__ceph_remove_cap(cap, false);
> > +		__ceph_remove_cap(cap, false, true);
> >   		goto out_unlock;
> >   	}
> >   
> > @@ -3978,7 +3978,8 @@ static void handle_cap_import(struct ceph_mds_client *mdsc,
> >   					ocap->mseq, mds, le32_to_cpu(ph->seq),
> >   					le32_to_cpu(ph->mseq));
> >   		}
> > -		__ceph_remove_cap(ocap, (ph->flags & CEPH_CAP_FLAG_RELEASE));
> > +		__ceph_remove_cap(ocap, (ph->flags & CEPH_CAP_FLAG_RELEASE),
> > +				  true);
> >   	}
> >   
> >   	*old_issued = issued;
> > diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> > index 0302af53e079..d99ec2618585 100644
> > --- a/fs/ceph/mds_client.c
> > +++ b/fs/ceph/mds_client.c
> > @@ -1636,7 +1636,7 @@ static int remove_session_caps_cb(struct inode *inode, struct ceph_cap *cap,
> >   	dout("removing cap %p, ci is %p, inode is %p\n",
> >   	     cap, ci, &ci->vfs_inode);
> >   	spin_lock(&ci->i_ceph_lock);
> > -	__ceph_remove_cap(cap, false);
> > +	__ceph_remove_cap(cap, false, false);
> >   	if (!ci->i_auth_cap) {
> >   		struct ceph_cap_flush *cf;
> >   
> > @@ -2008,7 +2008,7 @@ static int trim_caps_cb(struct inode *inode, struct ceph_cap *cap, void *arg)
> >   
> >   	if (oissued) {
> >   		/* we aren't the only cap.. just remove us */
> > -		__ceph_remove_cap(cap, true);
> > +		__ceph_remove_cap(cap, true, true);
> >   		(*remaining)--;
> >   	} else {
> >   		struct dentry *dentry;
> > diff --git a/fs/ceph/super.h b/fs/ceph/super.h
> > index 51ec17d12b26..106ddfd1ce92 100644
> > --- a/fs/ceph/super.h
> > +++ b/fs/ceph/super.h
> > @@ -1142,7 +1142,8 @@ extern void ceph_add_cap(struct inode *inode,
> >   			 unsigned issued, unsigned wanted,
> >   			 unsigned cap, unsigned seq, u64 realmino, int flags,
> >   			 struct ceph_cap **new_cap);
> > -extern void __ceph_remove_cap(struct ceph_cap *cap, bool queue_release);
> > +extern void __ceph_remove_cap(struct ceph_cap *cap, bool queue_release,
> > +			      bool warn);
> >   extern void __ceph_remove_caps(struct ceph_inode_info *ci);
> >   extern void ceph_put_cap(struct ceph_mds_client *mdsc,
> >   			 struct ceph_cap *cap);