From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steven Whitehouse Date: Wed, 07 Nov 2012 10:27:20 +0000 Subject: [Cluster-devel] [PATCH] GFS2: Test bufdata with buffer locked and gfs2_log_lock held In-Reply-To: <20121107063806.GO5182@ether.msp.redhat.com> References: <20121107063806.GO5182@ether.msp.redhat.com> Message-ID: <1352284040.2729.4.camel@menhir> List-Id: To: cluster-devel.redhat.com MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit 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 > --- > 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) >