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

[Steven Whitehouse <swhiteho@redhat.com>]

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