[Cluster-devel] [PATCH] GFS2: Test bufdata with buffer locked and gfs2_log_lock held

[Cluster-devel] [PATCH] GFS2: Test bufdata with buffer locked and gfs2_log_lock held

[Benjamin Marzinski]

In gfs2_trans_add_bh(), gfs2 was testing if a there was a bd attached to the
buffer without having the gfs2_log_lock held. It was then assuming it would
stay attached for the rest of the function. However, without either the log
lock being held of the buffer locked, __gfs2_ail_flush() could detach bd at any
time.  This patch moves the locking before the test.  If there isn't a bd
already attached, gfs2 can safely allocate one and attach it before locking.
There is no way that the newly allocated bd could be on the ail list,
and thus no way for __gfs2_ail_flush() to detach it.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 fs/gfs2/lops.c  |   14 ++------------
 fs/gfs2/trans.c |    8 ++++++++
 2 files changed, 10 insertions(+), 12 deletions(-)

Index: gfs2-3.0-nmw/fs/gfs2/lops.c
===================================================================
--- gfs2-3.0-nmw.orig/fs/gfs2/lops.c
+++ gfs2-3.0-nmw/fs/gfs2/lops.c
@@ -393,12 +393,10 @@ static void buf_lo_add(struct gfs2_sbd *
 	struct gfs2_meta_header *mh;
 	struct gfs2_trans *tr;
 
-	lock_buffer(bd->bd_bh);
-	gfs2_log_lock(sdp);
 	tr = current->journal_info;
 	tr->tr_touched = 1;
 	if (!list_empty(&bd->bd_list))
-		goto out;
+		return;
 	set_bit(GLF_LFLUSH, &bd->bd_gl->gl_flags);
 	set_bit(GLF_DIRTY, &bd->bd_gl->gl_flags);
 	mh = (struct gfs2_meta_header *)bd->bd_bh->b_data;
@@ -414,9 +412,6 @@ static void buf_lo_add(struct gfs2_sbd *
 	sdp->sd_log_num_buf++;
 	list_add(&bd->bd_list, &sdp->sd_log_le_buf);
 	tr->tr_num_buf_new++;
-out:
-	gfs2_log_unlock(sdp);
-	unlock_buffer(bd->bd_bh);
 }
 
 static void gfs2_check_magic(struct buffer_head *bh)
@@ -775,12 +770,10 @@ static void databuf_lo_add(struct gfs2_s
 	struct address_space *mapping = bd->bd_bh->b_page->mapping;
 	struct gfs2_inode *ip = GFS2_I(mapping->host);
 
-	lock_buffer(bd->bd_bh);
-	gfs2_log_lock(sdp);
 	if (tr)
 		tr->tr_touched = 1;
 	if (!list_empty(&bd->bd_list))
-		goto out;
+		return;
 	set_bit(GLF_LFLUSH, &bd->bd_gl->gl_flags);
 	set_bit(GLF_DIRTY, &bd->bd_gl->gl_flags);
 	if (gfs2_is_jdata(ip)) {
@@ -791,9 +784,6 @@ static void databuf_lo_add(struct gfs2_s
 	} else {
 		list_add_tail(&bd->bd_list, &sdp->sd_log_le_ordered);
 	}
-out:
-	gfs2_log_unlock(sdp);
-	unlock_buffer(bd->bd_bh);
 }
 
 /**
Index: gfs2-3.0-nmw/fs/gfs2/trans.c
===================================================================
--- gfs2-3.0-nmw.orig/fs/gfs2/trans.c
+++ gfs2-3.0-nmw/fs/gfs2/trans.c
@@ -155,14 +155,22 @@ void gfs2_trans_add_bh(struct gfs2_glock
 	struct gfs2_sbd *sdp = gl->gl_sbd;
 	struct gfs2_bufdata *bd;
 
+	lock_buffer(bh);
+	gfs2_log_lock(sdp);
 	bd = bh->b_private;
 	if (bd)
 		gfs2_assert(sdp, bd->bd_gl == gl);
 	else {
+		gfs2_log_unlock(sdp);
+		unlock_buffer(bh);
 		gfs2_attach_bufdata(gl, bh, meta);
 		bd = bh->b_private;
+		lock_buffer(bh);
+		gfs2_log_lock(sdp);
 	}
 	lops_add(sdp, bd);
+	gfs2_log_unlock(sdp);
+	unlock_buffer(bh);
 }
 
 void gfs2_trans_add_revoke(struct gfs2_sbd *sdp, struct gfs2_bufdata *bd)

[Cluster-devel] [PATCH] GFS2: Test bufdata with buffer locked and gfs2_log_lock held

[Steven Whitehouse]

Hi,

Now in the -nmw tree - thanks for fixing this. Also I'm sorting out a
-fixes tree too, so that I can send the collected bug fixes to Linus
ahead of the next merge window,

Steve.

On Wed, 2012-11-07 at 00:38 -0600, Benjamin Marzinski wrote:
> In gfs2_trans_add_bh(), gfs2 was testing if a there was a bd attached to the
> buffer without having the gfs2_log_lock held. It was then assuming it would
> stay attached for the rest of the function. However, without either the log
> lock being held of the buffer locked, __gfs2_ail_flush() could detach bd at any
> time.  This patch moves the locking before the test.  If there isn't a bd
> already attached, gfs2 can safely allocate one and attach it before locking.
> There is no way that the newly allocated bd could be on the ail list,
> and thus no way for __gfs2_ail_flush() to detach it.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  fs/gfs2/lops.c  |   14 ++------------
>  fs/gfs2/trans.c |    8 ++++++++
>  2 files changed, 10 insertions(+), 12 deletions(-)
> 
> Index: gfs2-3.0-nmw/fs/gfs2/lops.c
> ===================================================================
> --- gfs2-3.0-nmw.orig/fs/gfs2/lops.c
> +++ gfs2-3.0-nmw/fs/gfs2/lops.c
> @@ -393,12 +393,10 @@ static void buf_lo_add(struct gfs2_sbd *
>  	struct gfs2_meta_header *mh;
>  	struct gfs2_trans *tr;
>  
> -	lock_buffer(bd->bd_bh);
> -	gfs2_log_lock(sdp);
>  	tr = current->journal_info;
>  	tr->tr_touched = 1;
>  	if (!list_empty(&bd->bd_list))
> -		goto out;
> +		return;
>  	set_bit(GLF_LFLUSH, &bd->bd_gl->gl_flags);
>  	set_bit(GLF_DIRTY, &bd->bd_gl->gl_flags);
>  	mh = (struct gfs2_meta_header *)bd->bd_bh->b_data;
> @@ -414,9 +412,6 @@ static void buf_lo_add(struct gfs2_sbd *
>  	sdp->sd_log_num_buf++;
>  	list_add(&bd->bd_list, &sdp->sd_log_le_buf);
>  	tr->tr_num_buf_new++;
> -out:
> -	gfs2_log_unlock(sdp);
> -	unlock_buffer(bd->bd_bh);
>  }
>  
>  static void gfs2_check_magic(struct buffer_head *bh)
> @@ -775,12 +770,10 @@ static void databuf_lo_add(struct gfs2_s
>  	struct address_space *mapping = bd->bd_bh->b_page->mapping;
>  	struct gfs2_inode *ip = GFS2_I(mapping->host);
>  
> -	lock_buffer(bd->bd_bh);
> -	gfs2_log_lock(sdp);
>  	if (tr)
>  		tr->tr_touched = 1;
>  	if (!list_empty(&bd->bd_list))
> -		goto out;
> +		return;
>  	set_bit(GLF_LFLUSH, &bd->bd_gl->gl_flags);
>  	set_bit(GLF_DIRTY, &bd->bd_gl->gl_flags);
>  	if (gfs2_is_jdata(ip)) {
> @@ -791,9 +784,6 @@ static void databuf_lo_add(struct gfs2_s
>  	} else {
>  		list_add_tail(&bd->bd_list, &sdp->sd_log_le_ordered);
>  	}
> -out:
> -	gfs2_log_unlock(sdp);
> -	unlock_buffer(bd->bd_bh);
>  }
>  
>  /**
> Index: gfs2-3.0-nmw/fs/gfs2/trans.c
> ===================================================================
> --- gfs2-3.0-nmw.orig/fs/gfs2/trans.c
> +++ gfs2-3.0-nmw/fs/gfs2/trans.c
> @@ -155,14 +155,22 @@ void gfs2_trans_add_bh(struct gfs2_glock
>  	struct gfs2_sbd *sdp = gl->gl_sbd;
>  	struct gfs2_bufdata *bd;
>  
> +	lock_buffer(bh);
> +	gfs2_log_lock(sdp);
>  	bd = bh->b_private;
>  	if (bd)
>  		gfs2_assert(sdp, bd->bd_gl == gl);
>  	else {
> +		gfs2_log_unlock(sdp);
> +		unlock_buffer(bh);
>  		gfs2_attach_bufdata(gl, bh, meta);
>  		bd = bh->b_private;
> +		lock_buffer(bh);
> +		gfs2_log_lock(sdp);
>  	}
>  	lops_add(sdp, bd);
> +	gfs2_log_unlock(sdp);
> +	unlock_buffer(bh);
>  }
>  
>  void gfs2_trans_add_revoke(struct gfs2_sbd *sdp, struct gfs2_bufdata *bd)
>