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

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

[Benjamin Marzinski]

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.

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;
-- 
1.8.3.1

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

[Bob Peterson]

----- Original Message -----
> 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.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>

Hi,

ACK
Makes sense to me.

Regards,

Bob Peterson
Red Hat File Systems

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

[Steven Whitehouse]

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?

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;

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

[Benjamin Marzinski]

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;

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

[Steven Whitehouse]

Hi,

On 27/04/15 18:31, Benjamin Marzinski wrote:
> 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
That makes sense to me - thanks,

Steve.

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

[Bob Peterson]

----- Original Message -----
> 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.
> 
> 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;
> --
> 1.8.3.1

Hi,

Now applied to the for-next branch of the linux-gfs2 tree:
https://git.kernel.org/cgit/linux/kernel/git/gfs2/linux-gfs2.git/commit/fs/gfs2?h=for-next&id=086cc672e1cb600b9c17688a4aa44560db858c03

Regards,

Bob Peterson
Red Hat File Systems