On 19 January 2018 at 13:19, Andrew Price <[email protected]> wrote: > 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,
I'll do that and add the necessary padding at the end as well. > 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? Right now, no. You can easily find out the physical blocks with the FS_IOC_FIEMAP ioctl though, will that work? Thanks, Andreas
