From mboxrd@z Thu Jan 1 00:00:00 1970 From: Steven Whitehouse Date: Tue, 2 Apr 2019 11:06:34 +0100 Subject: [Cluster-devel] [GFS2 PATCH v3] gfs2: clean_journal improperly set sd_log_flush_head In-Reply-To: <748473503.16067530.1553793025683.JavaMail.zimbra@redhat.com> References: <748473503.16067530.1553793025683.JavaMail.zimbra@redhat.com> Message-ID: List-Id: To: cluster-devel.redhat.com MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit 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 > --- > 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; >