Re: [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