[Cluster-devel] [PATCH] GFS2: mark the journal idle to fix ro mounts

[Benjamin Marzinski <bmarzins@redhat.com>]

On Mon, Apr 27, 2015 at 11:01:42AM +0100, Steven Whitehouse wrote:
> Hi,
> 
> On 24/04/15 16:13, Benjamin Marzinski wrote:
> >When gfs2 was mounted read-only and then unmounted, it was writing a
> >header block to the journal in the syncing gfs2_log_flush() call from
> >kill_sb(). This is because the journal was not being marked as idle
> >until the first log header was written out, and on a read-only mount
> >there never was a log header written out. Since the journal was not
> >marked idle, gfs2_log_flush() was writing out a header lock to make
> >sure it was empty during the sync.  Not only did this cause IO to a
> >read-only filesystem, but the journalling isn't completely initialized
> >on read-only mounts, and so gfs2 was writing out the wrong sequence
> >number in the log header.
> >
> >Now, the journal is marked idle on mount, and gfs2_log_flush() won't
> >write out anything until there starts being transactions to flush.
> Does that mean that we should be doing more to initialize the log in the r/o
> mount case? It should know enough to recover the journals in the case that
> it is the first mounter, so did this perhaps only apply to subsequent
> mounters of the filesystem?

gfs2 currently has enough information to do recovery.  Both
gfs2_recover_func() and gfs2_make_fs_rw() call gfs2_find_jhead() to
get information about the head of the journal and the sequence
numbers.

gfs2_make_fs_rw() saves this information with these lines

        /*  Initialize some head of the log stuff  */
        sdp->sd_log_sequence = head.lh_sequence + 1;
        gfs2_log_pointers_init(sdp, head.lh_blkno);

This is what's not getting called on the read-only mounts that was
causing the fsck error. But the read only mounts should never be writing
anything in gfs2_log_flush(), which is where these values are used. So
we could make sure these values are always initialized, but the fact
that they weren't was what allowed us to catch this bug (and it would
still be a bug to write that header, even if it didn't mess with fsck).

-Ben

> 
> Still it is a good catch!
> 
> Acked-by: Steven Whitehouse <swhiteho@redhat.com>
> 
> Steve.
> 
> >Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> >---
> >  fs/gfs2/ops_fstype.c | 1 +
> >  1 file changed, 1 insertion(+)
> >
> >diff --git a/fs/gfs2/ops_fstype.c b/fs/gfs2/ops_fstype.c
> >index 8633ad3..fd984f6 100644
> >--- a/fs/gfs2/ops_fstype.c
> >+++ b/fs/gfs2/ops_fstype.c
> >@@ -757,6 +757,7 @@ static int init_journal(struct gfs2_sbd *sdp, int undo)
> >  		}
> >  	}
> >+	sdp->sd_log_idle = 1;
> >  	set_bit(SDF_JOURNAL_CHECKED, &sdp->sd_flags);
> >  	gfs2_glock_dq_uninit(&ji_gh);
> >  	jindex = 0;