From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steven Whitehouse Date: Tue, 12 Jun 2007 17:30:39 +0100 Subject: [Cluster-devel] Re: [PATCH] GFS2: bz 283162 - GFS2: Journaled file write/unstuff bug In-Reply-To: <466EC8C4.4030908@redhat.com> References: <466EC8C4.4030908@redhat.com> Message-ID: <1181665839.25918.319.camel@quoit> 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 GFS2 -nmw git tree. Thanks, Steve. On Tue, 2007-06-12 at 11:24 -0500, Robert Peterson wrote: > This patch is for bugzilla bug 283162, which uncovered a number of > bugs pertaining to writing to files that have the journaled bit on. > These bugs happen most often when writing to the meta_fs because > the files are always journaled. So operations like gfs2_grow were > particularly vulnerable, although many of the problems could be > recreated with normal files after setting the journaled bit on. > The problems fixed are: > > -GFS2 wasn't ever writing unstuffed journaled data blocks to their > in-place location on disk. Now it does. > > -If you unmounted too quickly after doing IO to a journaled file, > GFS2 was crashing because you would discard a buffer whose bufdata > was still on the active items list. GFS2 now deals with this > gracefully. > > -GFS2 was losing track of the bufdata for journaled data blocks, > and it wasn't getting freed, causing an error when you tried to > unmount the module. GFS2 now frees all the bufdata structures. > > -There was a memory corruption occurring because GFS2 wrote > twice as many log entries for journaled buffers. > > -It was occasionally trying to write journal headers in buffers > that weren't currently mapped. > > Signed-off-by: Bob Peterson > -- > > fs/gfs2/log.c | 15 ++++++++++++++- > fs/gfs2/lops.c | 4 +++- > fs/gfs2/ops_address.c | 26 +++++++++++++++++++++++++- > 3 files changed, 42 insertions(+), 3 deletions(-) > > diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c > index 1fb846f..fbdc0dc 100644 > --- a/fs/gfs2/log.c > +++ b/fs/gfs2/log.c > @@ -83,6 +83,11 @@ static void gfs2_ail1_start_one(struct gfs2_sbd *sdp, struct gfs2_ail *ai) > > gfs2_assert(sdp, bd->bd_ail == ai); > > + if (!bh){ > + list_move(&bd->bd_ail_st_list, &ai->ai_ail2_list); > + continue; > + } > + > if (!buffer_busy(bh)) { > if (!buffer_uptodate(bh)) { > gfs2_log_unlock(sdp); > @@ -125,6 +130,11 @@ static int gfs2_ail1_empty_one(struct gfs2_sbd *sdp, struct gfs2_ail *ai, int fl > bd_ail_st_list) { > bh = bd->bd_bh; > > + if (!bh){ > + list_move(&bd->bd_ail_st_list, &ai->ai_ail2_list); > + continue; > + } > + > gfs2_assert(sdp, bd->bd_ail == ai); > > if (buffer_busy(bh)) { > @@ -227,7 +237,10 @@ static void gfs2_ail2_empty_one(struct gfs2_sbd *sdp, struct gfs2_ail *ai) > list_del(&bd->bd_ail_st_list); > list_del(&bd->bd_ail_gl_list); > atomic_dec(&bd->bd_gl->gl_ail_count); > - brelse(bd->bd_bh); > + if (bd->bd_bh) > + brelse(bd->bd_bh); > + else > + kmem_cache_free(gfs2_bufdata_cachep, bd); > } > } > > diff --git a/fs/gfs2/lops.c b/fs/gfs2/lops.c > index 3e971f2..2730071 100644 > --- a/fs/gfs2/lops.c > +++ b/fs/gfs2/lops.c > @@ -607,7 +607,8 @@ static void databuf_lo_before_commit(struct gfs2_sbd *sdp) > if (unlikely(magic != 0)) > set_buffer_escaped(bh1); > gfs2_log_lock(sdp); > - if (n++ > num) > + n += 2; > + if (n >= num) > break; > } else if (!bh1) { > total_dbuf--; > @@ -624,6 +625,7 @@ static void databuf_lo_before_commit(struct gfs2_sbd *sdp) > } > gfs2_log_unlock(sdp); > if (bh) { > + set_buffer_mapped(bh); > set_buffer_dirty(bh); > ll_rw_block(WRITE, 1, &bh); > bh = NULL; > diff --git a/fs/gfs2/ops_address.c b/fs/gfs2/ops_address.c > index ac56595..9ab35a9 100644 > --- a/fs/gfs2/ops_address.c > +++ b/fs/gfs2/ops_address.c > @@ -137,7 +137,9 @@ static int gfs2_writepage(struct page *page, struct writeback_control *wbc) > return 0; /* don't care */ > } > > - if (sdp->sd_args.ar_data == GFS2_DATA_ORDERED || gfs2_is_jdata(ip)) { > + if ((sdp->sd_args.ar_data == GFS2_DATA_ORDERED || gfs2_is_jdata(ip)) && > + PageChecked(page)) { > + ClearPageChecked(page); > error = gfs2_trans_begin(sdp, RES_DINODE + 1, 0); > if (error) > goto out_ignore; > @@ -574,6 +576,23 @@ fail_nounlock: > } > > /** > + * gfs2_set_page_dirty - Page dirtying function > + * @page: The page to dirty > + * > + * Returns: 1 if it dirtyed the page, or 0 otherwise > + */ > + > +static int gfs2_set_page_dirty(struct page *page) > +{ > + struct gfs2_inode *ip = GFS2_I(page->mapping->host); > + struct gfs2_sbd *sdp = GFS2_SB(page->mapping->host); > + > + if (sdp->sd_args.ar_data == GFS2_DATA_ORDERED || gfs2_is_jdata(ip)) > + SetPageChecked(page); > + return __set_page_dirty_buffers(page); > +} > + > +/** > * gfs2_bmap - Block map function > * @mapping: Address space info > * @lblock: The block to map > @@ -609,6 +628,8 @@ static void discard_buffer(struct gfs2_sbd *sdp, struct buffer_head *bh) > if (bd) { > bd->bd_bh = NULL; > bh->b_private = NULL; > + if (!bd->bd_ail && list_empty(&bd->bd_le.le_list)) > + kmem_cache_free(gfs2_bufdata_cachep, bd); > } > gfs2_log_unlock(sdp); > > @@ -629,6 +650,8 @@ static void gfs2_invalidatepage(struct page *page, unsigned long offset) > unsigned int curr_off = 0; > > BUG_ON(!PageLocked(page)); > + if (offset == 0) > + ClearPageChecked(page); > if (!page_has_buffers(page)) > return; > > @@ -841,6 +864,7 @@ const struct address_space_operations gfs2_file_aops = { > .sync_page = block_sync_page, > .prepare_write = gfs2_prepare_write, > .commit_write = gfs2_commit_write, > + .set_page_dirty = gfs2_set_page_dirty, > .bmap = gfs2_bmap, > .invalidatepage = gfs2_invalidatepage, > .releasepage = gfs2_releasepage,