[Cluster-devel] [GFS2 PATCH][v2] GFS2: Reduce code redundancy writing log headers
Steven Whitehouse
swhiteho at redhat.com
Thu Dec 21 10:31:29 UTC 2017
Hi,
On 20/12/17 17:23, Bob Peterson wrote:
> Hi,
>
> This is a new improved version of the patch I posted prematurely yesterday.
> I've found and fixed the bug I encountered, plus simplified things a lot.
>
> Bob Peterson
> ---
>
> Before this patch, there was a lot of code redundancy between functions
> log_write_header (which uses bio) and clean_journal (which uses
> buffer_head). This patch reduces the redundancy to simplify the code
> and make log header writing more consistent. We want more consistency
> and reduced redundancy because we plan to add a bunch of new fields
> to improve performance (by eliminating the local statfs and quota files)
> improve metadata integrity (by adding new crcs and such) and for better
> debugging (by adding new fields to track when and where metadata was
> pushed through the journals.) We don't want to duplicate setting these
> new fields, nor allow for human error in the process.
>
> This reduction in code redundancy is accomplished by introducing a new
> helper function, gfs2_write_log_header which uses bio rather than bh.
> That simplifies recovery function clean_journal() to use the new helper
> function and iomap rather than redundancy and block_map (and eventually
> we can maybe remove block_map). It also reduces our dependency on
> buffer_heads.
>
> Signed-off-by: Bob Peterson <rpeterso at redhat.com>
Yes, that looks like a very worthwhile improvement and a nice clean up,
Steve.
> ---
> fs/gfs2/log.c | 46 +++++++++++++++++++++++++++++-------------
> fs/gfs2/log.h | 2 ++
> fs/gfs2/recovery.c | 59 +++++++++---------------------------------------------
> 3 files changed, 43 insertions(+), 64 deletions(-)
>
> diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c
> index f72c44231406..27e97d3de1e0 100644
> --- a/fs/gfs2/log.c
> +++ b/fs/gfs2/log.c
> @@ -648,49 +648,67 @@ void gfs2_write_revokes(struct gfs2_sbd *sdp)
> }
>
> /**
> - * log_write_header - Get and initialize a journal header buffer
> + * write_log_header - Write a journal log header buffer at sd_log_flush_head
> * @sdp: The GFS2 superblock
> + * @seq: sequence number
> + * @tail: tail of the log
> + * @flags: log header flags
> + * @op_flags: flags to pass to the bio
> *
> * Returns: the initialized log buffer descriptor
> */
>
> -static void log_write_header(struct gfs2_sbd *sdp, u32 flags)
> +void gfs2_write_log_header(struct gfs2_sbd *sdp, u64 seq, u32 tail,
> + u32 flags, int op_flags)
> {
> struct gfs2_log_header *lh;
> - unsigned int tail;
> u32 hash;
> - int op_flags = REQ_PREFLUSH | REQ_FUA | REQ_META | REQ_SYNC;
> struct page *page = mempool_alloc(gfs2_page_pool, GFP_NOIO);
> - enum gfs2_freeze_state state = atomic_read(&sdp->sd_freeze_state);
> +
> lh = page_address(page);
> clear_page(lh);
>
> - gfs2_assert_withdraw(sdp, (state != SFS_FROZEN));
> -
> - tail = current_tail(sdp);
> -
> lh->lh_header.mh_magic = cpu_to_be32(GFS2_MAGIC);
> lh->lh_header.mh_type = cpu_to_be32(GFS2_METATYPE_LH);
> lh->lh_header.__pad0 = cpu_to_be64(0);
> lh->lh_header.mh_format = cpu_to_be32(GFS2_FORMAT_LH);
> lh->lh_header.mh_jid = cpu_to_be32(sdp->sd_jdesc->jd_jid);
> - lh->lh_sequence = cpu_to_be64(sdp->sd_log_sequence++);
> + 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);
> hash = gfs2_disk_hash(page_address(page), sizeof(struct gfs2_log_header));
> lh->lh_hash = cpu_to_be32(hash);
>
> + gfs2_log_write_page(sdp, page);
> + gfs2_log_flush_bio(sdp, REQ_OP_WRITE, op_flags);
> + log_flush_wait(sdp);
> +}
> +
> +/**
> + * log_write_header - Get and initialize a journal header buffer
> + * @sdp: The GFS2 superblock
> + *
> + * Returns: the initialized log buffer descriptor
> + */
> +
> +static void log_write_header(struct gfs2_sbd *sdp, u32 flags)
> +{
> + unsigned int tail;
> + int op_flags = REQ_PREFLUSH | REQ_FUA | REQ_META | REQ_SYNC;
> + enum gfs2_freeze_state state = atomic_read(&sdp->sd_freeze_state);
> +
> + gfs2_assert_withdraw(sdp, (state != SFS_FROZEN));
> + tail = current_tail(sdp);
> +
> if (test_bit(SDF_NOBARRIERS, &sdp->sd_flags)) {
> gfs2_ordered_wait(sdp);
> log_flush_wait(sdp);
> op_flags = REQ_SYNC | REQ_META | REQ_PRIO;
> }
> -
> sdp->sd_log_idle = (tail == sdp->sd_log_flush_head);
> - gfs2_log_write_page(sdp, page);
> - gfs2_log_flush_bio(sdp, REQ_OP_WRITE, op_flags);
> - log_flush_wait(sdp);
> + gfs2_write_log_header(sdp, sdp->sd_log_sequence++, tail, flags,
> + 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 9499a6049212..619de9a1ff4f 100644
> --- a/fs/gfs2/log.h
> +++ b/fs/gfs2/log.h
> @@ -71,6 +71,8 @@ enum gfs2_flush_type {
> SHUTDOWN_FLUSH,
> FREEZE_FLUSH
> };
> +extern void gfs2_write_log_header(struct gfs2_sbd *sdp, u64 seq, u32 tail,
> + u32 flags, int op_flags);
> extern void gfs2_log_flush(struct gfs2_sbd *sdp, struct gfs2_glock *gl,
> enum gfs2_flush_type type);
> extern void gfs2_log_commit(struct gfs2_sbd *sdp, struct gfs2_trans *trans);
> diff --git a/fs/gfs2/recovery.c b/fs/gfs2/recovery.c
> index 9395a3db1a60..5d3431219425 100644
> --- a/fs/gfs2/recovery.c
> +++ b/fs/gfs2/recovery.c
> @@ -20,6 +20,7 @@
> #include "bmap.h"
> #include "glock.h"
> #include "glops.h"
> +#include "log.h"
> #include "lops.h"
> #include "meta_io.h"
> #include "recovery.h"
> @@ -370,62 +371,22 @@ static int foreach_descriptor(struct gfs2_jdesc *jd, unsigned int start,
>
> /**
> * clean_journal - mark a dirty journal as being clean
> - * @sdp: the filesystem
> * @jd: the journal
> - * @gl: the journal's glock
> * @head: the head journal to start from
> *
> * Returns: errno
> */
>
> -static int clean_journal(struct gfs2_jdesc *jd, struct gfs2_log_header_host *head)
> +static void clean_journal(struct gfs2_jdesc *jd,
> + struct gfs2_log_header_host *head)
> {
> - struct gfs2_inode *ip = GFS2_I(jd->jd_inode);
> struct gfs2_sbd *sdp = GFS2_SB(jd->jd_inode);
> - unsigned int lblock;
> - struct gfs2_log_header *lh;
> - u32 hash;
> - struct buffer_head *bh;
> - int error;
> - struct buffer_head bh_map = { .b_state = 0, .b_blocknr = 0 };
> -
> - lblock = head->lh_blkno;
> - gfs2_replay_incr_blk(jd, &lblock);
> - bh_map.b_size = 1 << ip->i_inode.i_blkbits;
> - error = gfs2_block_map(&ip->i_inode, lblock, &bh_map, 0);
> - if (error)
> - return error;
> - if (!bh_map.b_blocknr) {
> - gfs2_consist_inode(ip);
> - return -EIO;
> - }
> -
> - bh = sb_getblk(sdp->sd_vfs, bh_map.b_blocknr);
> - lock_buffer(bh);
> - memset(bh->b_data, 0, bh->b_size);
> - set_buffer_uptodate(bh);
> - clear_buffer_dirty(bh);
> - unlock_buffer(bh);
> -
> - lh = (struct gfs2_log_header *)bh->b_data;
> - memset(lh, 0, sizeof(struct gfs2_log_header));
> - lh->lh_header.mh_magic = cpu_to_be32(GFS2_MAGIC);
> - lh->lh_header.mh_type = cpu_to_be32(GFS2_METATYPE_LH);
> - lh->lh_header.__pad0 = cpu_to_be64(0);
> - lh->lh_header.mh_format = cpu_to_be32(GFS2_FORMAT_LH);
> - lh->lh_header.mh_jid = cpu_to_be32(sdp->sd_jdesc->jd_jid);
> - lh->lh_sequence = cpu_to_be64(head->lh_sequence + 1);
> - lh->lh_flags = cpu_to_be32(GFS2_LOG_HEAD_UNMOUNT);
> - lh->lh_blkno = cpu_to_be32(lblock);
> - hash = gfs2_disk_hash((const char *)lh, sizeof(struct gfs2_log_header));
> - lh->lh_hash = cpu_to_be32(hash);
> -
> - set_buffer_dirty(bh);
> - if (sync_dirty_buffer(bh))
> - gfs2_io_error_bh(sdp, bh);
> - brelse(bh);
>
> - return error;
> + sdp->sd_log_flush_head = head->lh_blkno;
> + gfs2_replay_incr_blk(jd, &sdp->sd_log_flush_head);
> + gfs2_write_log_header(sdp, head->lh_sequence + 1, 0,
> + GFS2_LOG_HEAD_UNMOUNT, REQ_PREFLUSH |
> + REQ_FUA | REQ_META | REQ_SYNC);
> }
>
>
> @@ -552,9 +513,7 @@ void gfs2_recover_func(struct work_struct *work)
> goto fail_gunlock_thaw;
> }
>
> - error = clean_journal(jd, &head);
> - if (error)
> - goto fail_gunlock_thaw;
> + clean_journal(jd, &head);
>
> gfs2_glock_dq_uninit(&thaw_gh);
> t = DIV_ROUND_UP(jiffies - t, HZ);
>
More information about the Cluster-devel
mailing list