[Cluster-devel] [PATCH] gfs2: clear journal live bit in gfs2_log_flush

[Cluster-devel] [PATCH] gfs2: clear journal live bit in gfs2_log_flush

[Benjamin Marzinski]

When gfs2 was unmounting filesystems or changing them to read-only it
was clearing the SDF_JOURNAL_LIVE bit before the final log flush.  This
caused a race.  If an inode glock got demoted in the gap between
clearing the bit and the shutdown flush, it would be unable to reserve
log space to clear out the acive items list in inode_go_sync, causing an
error in inode_go_inval because the glock was still dirty.

To solve this, the SDF_JOURNAL_LIVE bit is now cleared inside the
shutdown log flush.  This means that, because of the locking on the log
blocks, either inode_go_sync will be able to reserve space to clean the
glock before the shutdown flush, or the shutdown flush will clean the
glock itself, before inode_go_sync fails to reserve the space. Either
way, the glock will be clean before inode_go_inval.

Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
---
 fs/gfs2/log.c   | 3 +++
 fs/gfs2/super.c | 4 ----
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c
index 536e7a6..0ff028c 100644
--- a/fs/gfs2/log.c
+++ b/fs/gfs2/log.c
@@ -716,6 +716,9 @@ void gfs2_log_flush(struct gfs2_sbd *sdp, struct gfs2_glock *gl,
 	}
 	trace_gfs2_log_flush(sdp, 1);
 
+	if (type == SHUTDOWN_FLUSH)
+		clear_bit(SDF_JOURNAL_LIVE, &sdp->sd_flags);
+
 	sdp->sd_log_flush_head = sdp->sd_log_head;
 	sdp->sd_log_flush_wrapped = 0;
 	tr = sdp->sd_log_tr;
diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
index 894fb01..e55c9b6 100644
--- a/fs/gfs2/super.c
+++ b/fs/gfs2/super.c
@@ -842,10 +842,6 @@ static int gfs2_make_fs_ro(struct gfs2_sbd *sdp)
 	gfs2_quota_sync(sdp->sd_vfs, 0);
 	gfs2_statfs_sync(sdp->sd_vfs, 0);
 
-	down_write(&sdp->sd_log_flush_lock);
-	clear_bit(SDF_JOURNAL_LIVE, &sdp->sd_flags);
-	up_write(&sdp->sd_log_flush_lock);
-
 	gfs2_log_flush(sdp, NULL, SHUTDOWN_FLUSH);
 	wait_event(sdp->sd_reserving_log_wait, atomic_read(&sdp->sd_reserving_log) == 0);
 	gfs2_assert_warn(sdp, atomic_read(&sdp->sd_log_blks_free) == sdp->sd_jdesc->jd_blocks);
-- 
1.8.3.1

[Cluster-devel] [PATCH] gfs2: clear journal live bit in gfs2_log_flush

[Bob Peterson]

----- Original Message -----
> When gfs2 was unmounting filesystems or changing them to read-only it
> was clearing the SDF_JOURNAL_LIVE bit before the final log flush.  This
> caused a race.  If an inode glock got demoted in the gap between
> clearing the bit and the shutdown flush, it would be unable to reserve
> log space to clear out the acive items list in inode_go_sync, causing an
> error in inode_go_inval because the glock was still dirty.
> 
> To solve this, the SDF_JOURNAL_LIVE bit is now cleared inside the
> shutdown log flush.  This means that, because of the locking on the log
> blocks, either inode_go_sync will be able to reserve space to clean the
> glock before the shutdown flush, or the shutdown flush will clean the
> glock itself, before inode_go_sync fails to reserve the space. Either
> way, the glock will be clean before inode_go_inval.
> 
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>  fs/gfs2/log.c   | 3 +++
>  fs/gfs2/super.c | 4 ----
>  2 files changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c
> index 536e7a6..0ff028c 100644
> --- a/fs/gfs2/log.c
> +++ b/fs/gfs2/log.c
> @@ -716,6 +716,9 @@ void gfs2_log_flush(struct gfs2_sbd *sdp, struct
> gfs2_glock *gl,
>  	}
>  	trace_gfs2_log_flush(sdp, 1);
>  
> +	if (type == SHUTDOWN_FLUSH)
> +		clear_bit(SDF_JOURNAL_LIVE, &sdp->sd_flags);
> +
>  	sdp->sd_log_flush_head = sdp->sd_log_head;
>  	sdp->sd_log_flush_wrapped = 0;
>  	tr = sdp->sd_log_tr;
> diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
> index 894fb01..e55c9b6 100644
> --- a/fs/gfs2/super.c
> +++ b/fs/gfs2/super.c
> @@ -842,10 +842,6 @@ static int gfs2_make_fs_ro(struct gfs2_sbd *sdp)
>  	gfs2_quota_sync(sdp->sd_vfs, 0);
>  	gfs2_statfs_sync(sdp->sd_vfs, 0);
>  
> -	down_write(&sdp->sd_log_flush_lock);
> -	clear_bit(SDF_JOURNAL_LIVE, &sdp->sd_flags);
> -	up_write(&sdp->sd_log_flush_lock);
> -
>  	gfs2_log_flush(sdp, NULL, SHUTDOWN_FLUSH);
>  	wait_event(sdp->sd_reserving_log_wait, atomic_read(&sdp->sd_reserving_log)
>  	== 0);
>  	gfs2_assert_warn(sdp, atomic_read(&sdp->sd_log_blks_free) ==
>  	sdp->sd_jdesc->jd_blocks);
> --
> 1.8.3.1
> 
> 
Hi,

Thanks. This is 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=3ceb22a7b7a1e50658cce8c43d942e9f31f654bd

Regards,

Bob Peterson
Red Hat File Systems

[Cluster-devel] [PATCH] gfs2: clear journal live bit in gfs2_log_flush

[Andreas Gruenbacher]

Ben,

the fix is looking good and I can no longer trigger this bug, thanks.

Andreas

[Cluster-devel] [PATCH] gfs2: clear journal live bit in gfs2_log_flush

[Steven Whitehouse]

Hi,

Looks good to me. We should really add the "type" into the log flush 
tracing though. I'd quite like to also be able to differentiate between 
log flushes caused by timeouts, and those caused by glock dropping (and 
possibly other causes too) but I know that is a much more tricky thing 
to do. It would be very handy for performance analysis though,

Steve.

On 09/12/15 03:21, Benjamin Marzinski wrote:
> When gfs2 was unmounting filesystems or changing them to read-only it
> was clearing the SDF_JOURNAL_LIVE bit before the final log flush.  This
> caused a race.  If an inode glock got demoted in the gap between
> clearing the bit and the shutdown flush, it would be unable to reserve
> log space to clear out the acive items list in inode_go_sync, causing an
> error in inode_go_inval because the glock was still dirty.
>
> To solve this, the SDF_JOURNAL_LIVE bit is now cleared inside the
> shutdown log flush.  This means that, because of the locking on the log
> blocks, either inode_go_sync will be able to reserve space to clean the
> glock before the shutdown flush, or the shutdown flush will clean the
> glock itself, before inode_go_sync fails to reserve the space. Either
> way, the glock will be clean before inode_go_inval.
>
> Signed-off-by: Benjamin Marzinski <bmarzins@redhat.com>
> ---
>   fs/gfs2/log.c   | 3 +++
>   fs/gfs2/super.c | 4 ----
>   2 files changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c
> index 536e7a6..0ff028c 100644
> --- a/fs/gfs2/log.c
> +++ b/fs/gfs2/log.c
> @@ -716,6 +716,9 @@ void gfs2_log_flush(struct gfs2_sbd *sdp, struct gfs2_glock *gl,
>   	}
>   	trace_gfs2_log_flush(sdp, 1);
>   
> +	if (type == SHUTDOWN_FLUSH)
> +		clear_bit(SDF_JOURNAL_LIVE, &sdp->sd_flags);
> +
>   	sdp->sd_log_flush_head = sdp->sd_log_head;
>   	sdp->sd_log_flush_wrapped = 0;
>   	tr = sdp->sd_log_tr;
> diff --git a/fs/gfs2/super.c b/fs/gfs2/super.c
> index 894fb01..e55c9b6 100644
> --- a/fs/gfs2/super.c
> +++ b/fs/gfs2/super.c
> @@ -842,10 +842,6 @@ static int gfs2_make_fs_ro(struct gfs2_sbd *sdp)
>   	gfs2_quota_sync(sdp->sd_vfs, 0);
>   	gfs2_statfs_sync(sdp->sd_vfs, 0);
>   
> -	down_write(&sdp->sd_log_flush_lock);
> -	clear_bit(SDF_JOURNAL_LIVE, &sdp->sd_flags);
> -	up_write(&sdp->sd_log_flush_lock);
> -
>   	gfs2_log_flush(sdp, NULL, SHUTDOWN_FLUSH);
>   	wait_event(sdp->sd_reserving_log_wait, atomic_read(&sdp->sd_reserving_log) == 0);
>   	gfs2_assert_warn(sdp, atomic_read(&sdp->sd_log_blks_free) == sdp->sd_jdesc->jd_blocks);