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

[Andrew Price]

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