[Cluster-devel] [PATCH v5 2/3] GFS2: Introduce new gfs2_log_header_v2

[Andrew Price <anprice@redhat.com>]

On 19/01/18 10:53, Steven Whitehouse wrote:
> On 19/01/18 10:47, Andrew Price wrote:
>> On 18/01/18 16:04, Andreas Gruenbacher wrote:
>> <snip>
>>> diff --git a/fs/gfs2/log.c b/fs/gfs2/log.c
>>> index c27cbcebfe88..a2eb13c04591 100644
>>> --- a/fs/gfs2/log.c
>>> +++ b/fs/gfs2/log.c
>>> @@ -14,6 +14,7 @@
>>> ? #include <linux/buffer_head.h>
>>> ? #include <linux/gfs2_ondisk.h>
>>> ? #include <linux/crc32.h>
>>> +#include <linux/crc32c.h>
>>> ? #include <linux/delay.h>
>>> ? #include <linux/kthread.h>
>>> ? #include <linux/freezer.h>
>>> @@ -653,20 +654,25 @@ void gfs2_write_revokes(struct gfs2_sbd *sdp)
>>> ? /**
>>> ?? * write_log_header - Write a journal log header buffer at 
>>> sd_log_flush_head
>>> ?? * @sdp: The GFS2 superblock
>>> + * @jd: journal descriptor of the journal to which we are writing
>>> ?? * @seq: sequence number
>>> ?? * @tail: tail of the log
>>> - * @flags: log header flags
>>> + * @flags: log header flags GFS2_LOG_HEAD_*
>>> ?? * @op_flags: flags to pass to the bio
>>> ?? *
>>> ?? * Returns: the initialized log buffer descriptor
>>> ?? */
>>> ? -void gfs2_write_log_header(struct gfs2_sbd *sdp, u64 seq, u32 tail,
>>> -?????????????? u32 flags, int op_flags)
>>> +void gfs2_write_log_header(struct gfs2_sbd *sdp, struct gfs2_jdesc *jd,
>>> +?????????????? u64 seq, u32 tail, u32 flags, int op_flags)
>>> ? {
>>> ????? struct gfs2_log_header *lh;
>>> -??? u32 hash;
>>> +??? u32 hash, crc;
>>> ????? struct page *page = mempool_alloc(gfs2_page_pool, GFP_NOIO);
>>> +??? struct gfs2_statfs_change_host *l_sc = &sdp->sd_statfs_local;
>>> +??? struct timespec64 tv;
>>> +??? struct super_block *sb = sdp->sd_vfs;
>>> +??? u64 addr;
>>> ? ????? lh = page_address(page);
>>> ????? clear_page(lh);
>>> @@ -680,10 +686,39 @@ void gfs2_write_log_header(struct gfs2_sbd 
>>> *sdp, u64 seq, u32 tail,
>>> ????? 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 = ~crc32(~0, lh, sizeof(*lh));
>>> +??? hash = ~crc32(~0, lh, LH_V1_SIZE);
>>> ????? lh->lh_hash = cpu_to_be32(hash);
>>> ? -??? gfs2_log_write_page(sdp, page);
>>> +??? tv = current_kernel_time64();
>>> +??? 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);
>>> +??? 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
>>> +?????? own journal. The values are left 0 when recovering a journal
>>> +?????? different from our own. */
>>> +??? if (!(flags & GFS2_LOG_HEAD_RECOVERY)) {
>>> +??????? lh->lh_statfs_addr =
>>> + cpu_to_be64(GFS2_I(sdp->sd_sc_inode)->i_no_addr);
>>> +??????? lh->lh_quota_addr =
>>> + cpu_to_be64(GFS2_I(sdp->sd_qc_inode)->i_no_addr);
>>> +
>>> +??????? spin_lock(&sdp->sd_statfs_spin);
>>> +??????? lh->lh_local_total = cpu_to_be64(l_sc->sc_total);
>>> +??????? lh->lh_local_free = cpu_to_be64(l_sc->sc_free);
>>> +??????? lh->lh_local_dinodes = cpu_to_be64(l_sc->sc_dinodes);
>>> +??????? spin_unlock(&sdp->sd_statfs_spin);
>>> +??? }
>>> +
>>> +??? BUILD_BUG_ON(offsetof(struct gfs2_log_header, lh_crc) != 
>>> LH_V1_SIZE);
>>> +
>>> +??? crc = crc32c(~0, (void *)lh + LH_V1_SIZE + 4,
>>> +???????????? sb->s_blocksize - LH_V1_SIZE - 4);
>>> +??? lh->lh_crc = cpu_to_be32(crc);
>>
>> This seems to be at odds with the field comment:
>>
>> > +??? __be32 lh_crc;??????? /* crc32 of whole block with this field 0 */
>>
>> Is it really just CRCing the block from lh_crc + 4 onwards?
>>
>> Andy
>>
> Yes, the comment needs updating to match the code, if we are going to do 
> that. I'm not too concerned either way, but it might be a good plan to 
> make an inline function to calculate the checksum there, just to make 
> the clearer how it is done, and because we are going to need the same 
> code in multiple places eventually,

Thanks for clarifying. I have another query: in the gfs2_jadd case the 
new journal is written via an fd so the lh_addr is unknown when writing 
the log headers it initialises the journal with. Is it ok to leave that 
zero?

Andy