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

[Jeff Layton <jlayton@kernel.org>]

On Wed, 2021-08-18 at 16:06 +0800, xiubli@redhat.com wrote:
> From: Xiubo Li <xiubli@redhat.com>
> 
> 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@redhat.com>
> ---
>  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).

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

>  {
>  	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);

-- 
Jeff Layton <jlayton@kernel.org>