ceph: unify i_dirty_item and i_flushing_item handling when auth caps change
diff mbox series

Message ID 20200403144751.23977-1-jlayton@kernel.org
State New
Headers show
Series
  • ceph: unify i_dirty_item and i_flushing_item handling when auth caps change
Related show

Commit Message

Jeff Layton April 3, 2020, 2:47 p.m. UTC
Suggested-by: "Yan, Zheng" <zyan@redhat.com>
Signed-off-by: Jeff Layton <jlayton@kernel.org>
---
 fs/ceph/caps.c | 47 +++++++++++++++++++++++++----------------------
 1 file changed, 25 insertions(+), 22 deletions(-)

Comments

Jeff Layton April 3, 2020, 7:12 p.m. UTC | #1
On Fri, 2020-04-03 at 10:47 -0400, Jeff Layton wrote:
> Suggested-by: "Yan, Zheng" <zyan@redhat.com>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/ceph/caps.c | 47 +++++++++++++++++++++++++----------------------
>  1 file changed, 25 insertions(+), 22 deletions(-)
> 
> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> index eb190e4e203c..b3460d52a305 100644
> --- a/fs/ceph/caps.c
> +++ b/fs/ceph/caps.c
> @@ -3700,6 +3700,27 @@ static bool handle_cap_trunc(struct inode *inode,
>  	return queue_trunc;
>  }
>  
> +/**
> + * transplant_auth_cap - move inode to appropriate lists when auth caps change
> + * @ci: inode to be moved
> + * @session: new auth caps session
> + */
> +static void transplant_auth_ses(struct ceph_inode_info *ci,
> +				struct ceph_mds_session *session)
> +{
> +	lockdep_assert_held(&ci->i_ceph_lock);
> +
> +	if (list_empty(&ci->i_dirty_item) && list_empty(&ci->i_flushing_item))
> +		return;
> +
> +	spin_lock(&session->s_mdsc->cap_dirty_lock);
> +	if (!list_empty(&ci->i_dirty_item))
> +		list_move(&ci->i_dirty_item, &session->s_cap_dirty);
> +	if (!list_empty(&ci->i_flushing_item))
> +		list_move_tail(&ci->i_flushing_item, &session->s_cap_flushing);
> +	spin_unlock(&session->s_mdsc->cap_dirty_lock);
> +}
> +
>  /*
>   * Handle EXPORT from MDS.  Cap is being migrated _from_ this mds to a
>   * different one.  If we are the most recent migration we've seen (as
> @@ -3771,22 +3792,9 @@ static void handle_cap_export(struct inode *inode, struct ceph_mds_caps *ex,
>  			tcap->issue_seq = t_seq - 1;
>  			tcap->issued |= issued;
>  			tcap->implemented |= issued;
> -			if (cap == ci->i_auth_cap)
> +			if (cap == ci->i_auth_cap) {
>  				ci->i_auth_cap = tcap;
> -
> -			if (!list_empty(&ci->i_dirty_item)) {
> -				spin_lock(&mdsc->cap_dirty_lock);
> -				list_move(&ci->i_dirty_item,
> -					  &tcap->session->s_cap_dirty);
> -				spin_unlock(&mdsc->cap_dirty_lock);
> -			}
> -
> -			if (!list_empty(&ci->i_cap_flush_list) &&
> -			    ci->i_auth_cap == tcap) {
> -				spin_lock(&mdsc->cap_dirty_lock);
> -				list_move_tail(&ci->i_flushing_item,
> -					       &tcap->session->s_cap_flushing);
> -				spin_unlock(&mdsc->cap_dirty_lock);
> +				transplant_auth_ses(ci, tcap->session);
>  			}
>  		}
>  		__ceph_remove_cap(cap, false);
> @@ -3798,13 +3806,8 @@ static void handle_cap_export(struct inode *inode, struct ceph_mds_caps *ex,
>  		ceph_add_cap(inode, tsession, t_cap_id, issued, 0,
>  			     t_seq - 1, t_mseq, (u64)-1, flag, &new_cap);
>  
> -		if (!list_empty(&ci->i_cap_flush_list) &&
> -		    ci->i_auth_cap == tcap) {
> -			spin_lock(&mdsc->cap_dirty_lock);
> -			list_move_tail(&ci->i_flushing_item,
> -				       &tcap->session->s_cap_flushing);
> -			spin_unlock(&mdsc->cap_dirty_lock);
> -		}
> +		if (ci->i_auth_cap == tcap)
> +			transplant_auth_ses(ci, tcap->session);
>  
>  		__ceph_remove_cap(cap, false);
>  		goto out_unlock;

If this looks ok, I think I'll fold it into "ceph: convert
mdsc->cap_dirty to a per-session list".
Yan, Zheng April 6, 2020, 2:55 p.m. UTC | #2
On Fri, Apr 3, 2020 at 10:47 PM Jeff Layton <jlayton@kernel.org> wrote:
>
> Suggested-by: "Yan, Zheng" <zyan@redhat.com>
> Signed-off-by: Jeff Layton <jlayton@kernel.org>
> ---
>  fs/ceph/caps.c | 47 +++++++++++++++++++++++++----------------------
>  1 file changed, 25 insertions(+), 22 deletions(-)
>
> diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> index eb190e4e203c..b3460d52a305 100644
> --- a/fs/ceph/caps.c
> +++ b/fs/ceph/caps.c
> @@ -3700,6 +3700,27 @@ static bool handle_cap_trunc(struct inode *inode,
>         return queue_trunc;
>  }
>
> +/**
> + * transplant_auth_cap - move inode to appropriate lists when auth caps change
> + * @ci: inode to be moved
> + * @session: new auth caps session
> + */
> +static void transplant_auth_ses(struct ceph_inode_info *ci,
> +                               struct ceph_mds_session *session)
> +{
> +       lockdep_assert_held(&ci->i_ceph_lock);
> +
> +       if (list_empty(&ci->i_dirty_item) && list_empty(&ci->i_flushing_item))
> +               return;
> +
> +       spin_lock(&session->s_mdsc->cap_dirty_lock);
> +       if (!list_empty(&ci->i_dirty_item))
> +               list_move(&ci->i_dirty_item, &session->s_cap_dirty);
> +       if (!list_empty(&ci->i_flushing_item))
> +               list_move_tail(&ci->i_flushing_item, &session->s_cap_flushing);
> +       spin_unlock(&session->s_mdsc->cap_dirty_lock);
> +}
> +
>  /*
>   * Handle EXPORT from MDS.  Cap is being migrated _from_ this mds to a
>   * different one.  If we are the most recent migration we've seen (as
> @@ -3771,22 +3792,9 @@ static void handle_cap_export(struct inode *inode, struct ceph_mds_caps *ex,
>                         tcap->issue_seq = t_seq - 1;
>                         tcap->issued |= issued;
>                         tcap->implemented |= issued;
> -                       if (cap == ci->i_auth_cap)
> +                       if (cap == ci->i_auth_cap) {
>                                 ci->i_auth_cap = tcap;
> -
> -                       if (!list_empty(&ci->i_dirty_item)) {
> -                               spin_lock(&mdsc->cap_dirty_lock);
> -                               list_move(&ci->i_dirty_item,
> -                                         &tcap->session->s_cap_dirty);
> -                               spin_unlock(&mdsc->cap_dirty_lock);
> -                       }
> -
> -                       if (!list_empty(&ci->i_cap_flush_list) &&
> -                           ci->i_auth_cap == tcap) {
> -                               spin_lock(&mdsc->cap_dirty_lock);
> -                               list_move_tail(&ci->i_flushing_item,
> -                                              &tcap->session->s_cap_flushing);
> -                               spin_unlock(&mdsc->cap_dirty_lock);
> +                               transplant_auth_ses(ci, tcap->session);
>                         }
>                 }
>                 __ceph_remove_cap(cap, false);
> @@ -3798,13 +3806,8 @@ static void handle_cap_export(struct inode *inode, struct ceph_mds_caps *ex,
>                 ceph_add_cap(inode, tsession, t_cap_id, issued, 0,
>                              t_seq - 1, t_mseq, (u64)-1, flag, &new_cap);
>
> -               if (!list_empty(&ci->i_cap_flush_list) &&
> -                   ci->i_auth_cap == tcap) {
> -                       spin_lock(&mdsc->cap_dirty_lock);
> -                       list_move_tail(&ci->i_flushing_item,
> -                                      &tcap->session->s_cap_flushing);
> -                       spin_unlock(&mdsc->cap_dirty_lock);
> -               }
> +               if (ci->i_auth_cap == tcap)
> +                       transplant_auth_ses(ci, tcap->session);
>

why not call transplant_auth_ses() inside ceph_add_cap() (replace code
added by "ceph: convert mdsc->cap_dirty to a per-session list")

>                 __ceph_remove_cap(cap, false);
>                 goto out_unlock;
> --
> 2.25.1
>
Jeff Layton April 6, 2020, 7:51 p.m. UTC | #3
On Mon, 2020-04-06 at 22:55 +0800, Yan, Zheng wrote:
> On Fri, Apr 3, 2020 at 10:47 PM Jeff Layton <jlayton@kernel.org> wrote:
> > Suggested-by: "Yan, Zheng" <zyan@redhat.com>
> > Signed-off-by: Jeff Layton <jlayton@kernel.org>
> > ---
> >  fs/ceph/caps.c | 47 +++++++++++++++++++++++++----------------------
> >  1 file changed, 25 insertions(+), 22 deletions(-)
> > 
> > diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
> > index eb190e4e203c..b3460d52a305 100644
> > --- a/fs/ceph/caps.c
> > +++ b/fs/ceph/caps.c
> > @@ -3700,6 +3700,27 @@ static bool handle_cap_trunc(struct inode *inode,
> >         return queue_trunc;
> >  }
> > 
> > +/**
> > + * transplant_auth_cap - move inode to appropriate lists when auth caps change
> > + * @ci: inode to be moved
> > + * @session: new auth caps session
> > + */
> > +static void transplant_auth_ses(struct ceph_inode_info *ci,
> > +                               struct ceph_mds_session *session)
> > +{
> > +       lockdep_assert_held(&ci->i_ceph_lock);
> > +
> > +       if (list_empty(&ci->i_dirty_item) && list_empty(&ci->i_flushing_item))
> > +               return;
> > +
> > +       spin_lock(&session->s_mdsc->cap_dirty_lock);
> > +       if (!list_empty(&ci->i_dirty_item))
> > +               list_move(&ci->i_dirty_item, &session->s_cap_dirty);
> > +       if (!list_empty(&ci->i_flushing_item))
> > +               list_move_tail(&ci->i_flushing_item, &session->s_cap_flushing);
> > +       spin_unlock(&session->s_mdsc->cap_dirty_lock);
> > +}
> > +
> >  /*
> >   * Handle EXPORT from MDS.  Cap is being migrated _from_ this mds to a
> >   * different one.  If we are the most recent migration we've seen (as
> > @@ -3771,22 +3792,9 @@ static void handle_cap_export(struct inode *inode, struct ceph_mds_caps *ex,
> >                         tcap->issue_seq = t_seq - 1;
> >                         tcap->issued |= issued;
> >                         tcap->implemented |= issued;
> > -                       if (cap == ci->i_auth_cap)
> > +                       if (cap == ci->i_auth_cap) {
> >                                 ci->i_auth_cap = tcap;
> > -
> > -                       if (!list_empty(&ci->i_dirty_item)) {
> > -                               spin_lock(&mdsc->cap_dirty_lock);
> > -                               list_move(&ci->i_dirty_item,
> > -                                         &tcap->session->s_cap_dirty);
> > -                               spin_unlock(&mdsc->cap_dirty_lock);
> > -                       }
> > -
> > -                       if (!list_empty(&ci->i_cap_flush_list) &&
> > -                           ci->i_auth_cap == tcap) {
> > -                               spin_lock(&mdsc->cap_dirty_lock);
> > -                               list_move_tail(&ci->i_flushing_item,
> > -                                              &tcap->session->s_cap_flushing);
> > -                               spin_unlock(&mdsc->cap_dirty_lock);
> > +                               transplant_auth_ses(ci, tcap->session);
> >                         }
> >                 }
> >                 __ceph_remove_cap(cap, false);
> > @@ -3798,13 +3806,8 @@ static void handle_cap_export(struct inode *inode, struct ceph_mds_caps *ex,
> >                 ceph_add_cap(inode, tsession, t_cap_id, issued, 0,
> >                              t_seq - 1, t_mseq, (u64)-1, flag, &new_cap);
> > 
> > -               if (!list_empty(&ci->i_cap_flush_list) &&
> > -                   ci->i_auth_cap == tcap) {
> > -                       spin_lock(&mdsc->cap_dirty_lock);
> > -                       list_move_tail(&ci->i_flushing_item,
> > -                                      &tcap->session->s_cap_flushing);
> > -                       spin_unlock(&mdsc->cap_dirty_lock);
> > -               }
> > +               if (ci->i_auth_cap == tcap)
> > +                       transplant_auth_ses(ci, tcap->session);
> > 
> 
> why not call transplant_auth_ses() inside ceph_add_cap() (replace code
> added by "ceph: convert mdsc->cap_dirty to a per-session list")
> 

Good point. I just sent a v3 version that squashes down all of the
changes since v2, including this one.

> >                 __ceph_remove_cap(cap, false);
> >                 goto out_unlock;
> > --
> > 2.25.1
> >

Patch
diff mbox series

diff --git a/fs/ceph/caps.c b/fs/ceph/caps.c
index eb190e4e203c..b3460d52a305 100644
--- a/fs/ceph/caps.c
+++ b/fs/ceph/caps.c
@@ -3700,6 +3700,27 @@  static bool handle_cap_trunc(struct inode *inode,
 	return queue_trunc;
 }
 
+/**
+ * transplant_auth_cap - move inode to appropriate lists when auth caps change
+ * @ci: inode to be moved
+ * @session: new auth caps session
+ */
+static void transplant_auth_ses(struct ceph_inode_info *ci,
+				struct ceph_mds_session *session)
+{
+	lockdep_assert_held(&ci->i_ceph_lock);
+
+	if (list_empty(&ci->i_dirty_item) && list_empty(&ci->i_flushing_item))
+		return;
+
+	spin_lock(&session->s_mdsc->cap_dirty_lock);
+	if (!list_empty(&ci->i_dirty_item))
+		list_move(&ci->i_dirty_item, &session->s_cap_dirty);
+	if (!list_empty(&ci->i_flushing_item))
+		list_move_tail(&ci->i_flushing_item, &session->s_cap_flushing);
+	spin_unlock(&session->s_mdsc->cap_dirty_lock);
+}
+
 /*
  * Handle EXPORT from MDS.  Cap is being migrated _from_ this mds to a
  * different one.  If we are the most recent migration we've seen (as
@@ -3771,22 +3792,9 @@  static void handle_cap_export(struct inode *inode, struct ceph_mds_caps *ex,
 			tcap->issue_seq = t_seq - 1;
 			tcap->issued |= issued;
 			tcap->implemented |= issued;
-			if (cap == ci->i_auth_cap)
+			if (cap == ci->i_auth_cap) {
 				ci->i_auth_cap = tcap;
-
-			if (!list_empty(&ci->i_dirty_item)) {
-				spin_lock(&mdsc->cap_dirty_lock);
-				list_move(&ci->i_dirty_item,
-					  &tcap->session->s_cap_dirty);
-				spin_unlock(&mdsc->cap_dirty_lock);
-			}
-
-			if (!list_empty(&ci->i_cap_flush_list) &&
-			    ci->i_auth_cap == tcap) {
-				spin_lock(&mdsc->cap_dirty_lock);
-				list_move_tail(&ci->i_flushing_item,
-					       &tcap->session->s_cap_flushing);
-				spin_unlock(&mdsc->cap_dirty_lock);
+				transplant_auth_ses(ci, tcap->session);
 			}
 		}
 		__ceph_remove_cap(cap, false);
@@ -3798,13 +3806,8 @@  static void handle_cap_export(struct inode *inode, struct ceph_mds_caps *ex,
 		ceph_add_cap(inode, tsession, t_cap_id, issued, 0,
 			     t_seq - 1, t_mseq, (u64)-1, flag, &new_cap);
 
-		if (!list_empty(&ci->i_cap_flush_list) &&
-		    ci->i_auth_cap == tcap) {
-			spin_lock(&mdsc->cap_dirty_lock);
-			list_move_tail(&ci->i_flushing_item,
-				       &tcap->session->s_cap_flushing);
-			spin_unlock(&mdsc->cap_dirty_lock);
-		}
+		if (ci->i_auth_cap == tcap)
+			transplant_auth_ses(ci, tcap->session);
 
 		__ceph_remove_cap(cap, false);
 		goto out_unlock;