[Cluster-devel] [GFS2 PATCH v3] gfs2: clean_journal improperly set sd_log_flush_head

[Steven Whitehouse <swhiteho@redhat.com>]

Hi,

On 28/03/2019 17:10, Bob Peterson wrote:
> Hi,
>
> Andreas found some problems with the previous version. Here is version 3.
>
> Ross: Can you please test this one with your scenario? Thanks.
>
> Bob Peterson
> ---
>
> This patch fixes regressions in 588bff95c94efc05f9e1a0b19015c9408ed7c0ef.
> Due to that patch, function clean_journal was setting the value of
> sd_log_flush_head, but that's only valid if it is replaying the node's
> own journal. If it's replaying another node's journal, that's completely
> wrong and will lead to multiple problems. This patch tries to clean up
> the mess by passing the value of the logical journal block number into
> gfs2_write_log_header so the function can treat non-owned journals
> generically. For the local journal, the journal extent map is used for
> best performance. For other nodes from other journals, gfs2_extent_map
> is called to figure it out.
>
> This patch also tries to establish more consistency when passing journal
> block parameters by changing several unsigned int types to a consistent
> u32.
>
> Fixes: 588bff95c94e ("GFS2: Reduce code redundancy writing log headers")
>
> Signed-off-by: Bob Peterson <rpeterso@redhat.com>
> ---
>   fs/gfs2/incore.h   |  2 +-
>   fs/gfs2/log.c      | 26 +++++++++++++++++++-------
>   fs/gfs2/log.h      |  3 ++-
>   fs/gfs2/lops.c     |  6 +++---
>   fs/gfs2/lops.h     |  2 +-
>   fs/gfs2/recovery.c |  8 ++++----
>   fs/gfs2/recovery.h |  2 +-
>   7 files changed, 31 insertions(+), 18 deletions(-)
>
> diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h
> index cdf07b408f54..86840a70ee1a 100644
> --- a/fs/gfs2/incore.h
> +++ b/fs/gfs2/incore.h
> @@ -535,7 +535,7 @@ struct gfs2_jdesc {
>   	unsigned long jd_flags;
>   #define JDF_RECOVERY 1
>   	unsigned int jd_jid;
> -	unsigned int jd_blocks;
> +	u32 jd_blocks;
>   	int jd_recover_error;
>   	/* Replay stuff */
>   
> diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c
> index b8830fda51e8..8a5a19a26582 100644
> --- a/fs/gfs2/log.c
> +++ b/fs/gfs2/log.c
> @@ -672,13 +672,15 @@ void gfs2_write_revokes(struct gfs2_sbd *sdp)
>    * @seq: sequence number
>    * @tail: tail of the log
>    * @flags: log header flags GFS2_LOG_HEAD_*
> + * @lblock: value for lh_blkno (block number relative to start of journal)
>    * @op_flags: flags to pass to the bio
>    *
>    * Returns: the initialized log buffer descriptor
>    */
>   
>   void gfs2_write_log_header(struct gfs2_sbd *sdp, struct gfs2_jdesc *jd,
> -			   u64 seq, u32 tail, u32 flags, int op_flags)
> +			   u64 seq, u32 tail, u32 flags, u32 lblock,
> +			   int op_flags)
>   {
>   	struct gfs2_log_header *lh;
>   	u32 hash, crc;
> @@ -686,7 +688,7 @@ void gfs2_write_log_header(struct gfs2_sbd *sdp, struct gfs2_jdesc *jd,
>   	struct gfs2_statfs_change_host *l_sc = &sdp->sd_statfs_local;
>   	struct timespec64 tv;
>   	struct super_block *sb = sdp->sd_vfs;
> -	u64 addr;
> +	u64 dblock;
>   
>   	lh = page_address(page);
>   	clear_page(lh);
> @@ -699,15 +701,25 @@ void gfs2_write_log_header(struct gfs2_sbd *sdp, struct gfs2_jdesc *jd,
>   	lh->lh_sequence = cpu_to_be64(seq);
>   	lh->lh_flags = cpu_to_be32(flags);
>   	lh->lh_tail = cpu_to_be32(tail);
> -	lh->lh_blkno = cpu_to_be32(sdp->sd_log_flush_head);
> +	lh->lh_blkno = cpu_to_be32(lblock);
>   	hash = ~crc32(~0, lh, LH_V1_SIZE);
>   	lh->lh_hash = cpu_to_be32(hash);
>   
>   	ktime_get_coarse_real_ts64(&tv);
>   	lh->lh_nsec = cpu_to_be32(tv.tv_nsec);
>   	lh->lh_sec = cpu_to_be64(tv.tv_sec);
> -	addr = gfs2_log_bmap(sdp);
> -	lh->lh_addr = cpu_to_be64(addr);
> +	if (jd->jd_jid == sdp->sd_lockstruct.ls_jid)
> +		dblock = gfs2_log_bmap(sdp);
> +	else {
> +		u32 extlen;
> +		int new = 0, error;
> +
> +		error = gfs2_extent_map(jd->jd_inode, lblock, &new, &dblock,
> +					&extlen);

We should not be adding new calls to gfs2_extent_map() here since that 
function is obsolete and deprecated. It looks like perhaps we should 
have a parameter to gfs2_log_bmap() to indicate which journal we need to 
map?

Steve.

> +		if (gfs2_assert_withdraw(sdp, error == 0))
> +			return;
> +	}
> +	lh->lh_addr = cpu_to_be64(dblock);
>   	lh->lh_jinode = cpu_to_be64(GFS2_I(jd->jd_inode)->i_no_addr);
>   
>   	/* We may only write local statfs, quota, etc., when writing to our
> @@ -732,7 +744,7 @@ void gfs2_write_log_header(struct gfs2_sbd *sdp, struct gfs2_jdesc *jd,
>   		     sb->s_blocksize - LH_V1_SIZE - 4);
>   	lh->lh_crc = cpu_to_be32(crc);
>   
> -	gfs2_log_write(sdp, page, sb->s_blocksize, 0, addr);
> +	gfs2_log_write(sdp, page, sb->s_blocksize, 0, dblock);
>   	gfs2_log_submit_bio(&sdp->sd_log_bio, REQ_OP_WRITE, op_flags);
>   	log_flush_wait(sdp);
>   }
> @@ -761,7 +773,7 @@ static void log_write_header(struct gfs2_sbd *sdp, u32 flags)
>   	}
>   	sdp->sd_log_idle = (tail == sdp->sd_log_flush_head);
>   	gfs2_write_log_header(sdp, sdp->sd_jdesc, sdp->sd_log_sequence++, tail,
> -			      flags, op_flags);
> +			      flags, sdp->sd_log_flush_head, op_flags);
>   
>   	if (sdp->sd_log_tail != tail)
>   		log_pull_tail(sdp, tail);
> diff --git a/fs/gfs2/log.h b/fs/gfs2/log.h
> index 1bc9bd444b28..1e061657d263 100644
> --- a/fs/gfs2/log.h
> +++ b/fs/gfs2/log.h
> @@ -70,7 +70,8 @@ extern unsigned int gfs2_struct2blk(struct gfs2_sbd *sdp, unsigned int nstruct,
>   extern void gfs2_log_release(struct gfs2_sbd *sdp, unsigned int blks);
>   extern int gfs2_log_reserve(struct gfs2_sbd *sdp, unsigned int blks);
>   extern void gfs2_write_log_header(struct gfs2_sbd *sdp, struct gfs2_jdesc *jd,
> -				  u64 seq, u32 tail, u32 flags, int op_flags);
> +				  u64 seq, u32 tail, u32 flags, u32 lblock,
> +				  int op_flags);
>   extern void gfs2_log_flush(struct gfs2_sbd *sdp, struct gfs2_glock *gl,
>   			   u32 type);
>   extern void gfs2_log_commit(struct gfs2_sbd *sdp, struct gfs2_trans *trans);
> diff --git a/fs/gfs2/lops.c b/fs/gfs2/lops.c
> index 8722c60b11fe..aef21b6a608f 100644
> --- a/fs/gfs2/lops.c
> +++ b/fs/gfs2/lops.c
> @@ -530,7 +530,7 @@ static void buf_lo_before_scan(struct gfs2_jdesc *jd,
>   	jd->jd_replayed_blocks = 0;
>   }
>   
> -static int buf_lo_scan_elements(struct gfs2_jdesc *jd, unsigned int start,
> +static int buf_lo_scan_elements(struct gfs2_jdesc *jd, u32 start,
>   				struct gfs2_log_descriptor *ld, __be64 *ptr,
>   				int pass)
>   {
> @@ -685,7 +685,7 @@ static void revoke_lo_before_scan(struct gfs2_jdesc *jd,
>   	jd->jd_replay_tail = head->lh_tail;
>   }
>   
> -static int revoke_lo_scan_elements(struct gfs2_jdesc *jd, unsigned int start,
> +static int revoke_lo_scan_elements(struct gfs2_jdesc *jd, u32 start,
>   				   struct gfs2_log_descriptor *ld, __be64 *ptr,
>   				   int pass)
>   {
> @@ -767,7 +767,7 @@ static void databuf_lo_before_commit(struct gfs2_sbd *sdp, struct gfs2_trans *tr
>   	gfs2_before_commit(sdp, limit, nbuf, &tr->tr_databuf, 1);
>   }
>   
> -static int databuf_lo_scan_elements(struct gfs2_jdesc *jd, unsigned int start,
> +static int databuf_lo_scan_elements(struct gfs2_jdesc *jd, u32 start,
>   				    struct gfs2_log_descriptor *ld,
>   				    __be64 *ptr, int pass)
>   {
> diff --git a/fs/gfs2/lops.h b/fs/gfs2/lops.h
> index 711c4d89c063..4e81742de7a0 100644
> --- a/fs/gfs2/lops.h
> +++ b/fs/gfs2/lops.h
> @@ -77,7 +77,7 @@ static inline void lops_before_scan(struct gfs2_jdesc *jd,
>   			gfs2_log_ops[x]->lo_before_scan(jd, head, pass);
>   }
>   
> -static inline int lops_scan_elements(struct gfs2_jdesc *jd, unsigned int start,
> +static inline int lops_scan_elements(struct gfs2_jdesc *jd, u32 start,
>   				     struct gfs2_log_descriptor *ld,
>   				     __be64 *ptr,
>   				     unsigned int pass)
> diff --git a/fs/gfs2/recovery.c b/fs/gfs2/recovery.c
> index 2dac43065382..3a6cc5d6acc3 100644
> --- a/fs/gfs2/recovery.c
> +++ b/fs/gfs2/recovery.c
> @@ -316,7 +316,7 @@ int gfs2_find_jhead(struct gfs2_jdesc *jd, struct gfs2_log_header_host *head)
>    * Returns: errno
>    */
>   
> -static int foreach_descriptor(struct gfs2_jdesc *jd, unsigned int start,
> +static int foreach_descriptor(struct gfs2_jdesc *jd, u32 start,
>   			      unsigned int end, int pass)
>   {
>   	struct gfs2_sbd *sdp = GFS2_SB(jd->jd_inode);
> @@ -386,10 +386,10 @@ static void clean_journal(struct gfs2_jdesc *jd,
>   			  struct gfs2_log_header_host *head)
>   {
>   	struct gfs2_sbd *sdp = GFS2_SB(jd->jd_inode);
> +	u32 lblock = head->lh_blkno;
>   
> -	sdp->sd_log_flush_head = head->lh_blkno;
> -	gfs2_replay_incr_blk(jd, &sdp->sd_log_flush_head);
> -	gfs2_write_log_header(sdp, jd, head->lh_sequence + 1, 0,
> +	gfs2_replay_incr_blk(jd, &lblock);
> +	gfs2_write_log_header(sdp, jd, head->lh_sequence + 1, 0, lblock,
>   			      GFS2_LOG_HEAD_UNMOUNT | GFS2_LOG_HEAD_RECOVERY,
>   			      REQ_PREFLUSH | REQ_FUA | REQ_META | REQ_SYNC);
>   }
> diff --git a/fs/gfs2/recovery.h b/fs/gfs2/recovery.h
> index 11d81248be85..5932d4b6f43e 100644
> --- a/fs/gfs2/recovery.h
> +++ b/fs/gfs2/recovery.h
> @@ -14,7 +14,7 @@
>   
>   extern struct workqueue_struct *gfs_recovery_wq;
>   
> -static inline void gfs2_replay_incr_blk(struct gfs2_jdesc *jd, unsigned int *blk)
> +static inline void gfs2_replay_incr_blk(struct gfs2_jdesc *jd, u32 *blk)
>   {
>   	if (++*blk == jd->jd_blocks)
>   	        *blk = 0;
>