[PATCH 0/4] JBD2 cleanups

[PATCH 0/4] JBD2 cleanups

[Jan Kara]

Hi!

here are some JBD2 cleanups that have accumulated in my tree when doing
other stuff. I think they are reasonable ones but opinions may differ...

								Honza

[PATCH 1/4] jbd2: Remove some unnecessary arguments of jbd2_journal_write_revoke_records

[Jan Kara]

jbd2_journal_write_revoke_records() takes journal pointer and write_op,
although journal can be obtained from the passed transaction and
write_op is always WRITE_SYNC. Remove these superfluous arguments.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/jbd2/commit.c     |  3 +--
 fs/jbd2/revoke.c     | 33 +++++++++++++++------------------
 include/linux/jbd2.h |  6 ++----
 3 files changed, 18 insertions(+), 24 deletions(-)

diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
index 36345fefa3ff..ae4402d15d46 100644
--- a/fs/jbd2/commit.c
+++ b/fs/jbd2/commit.c
@@ -554,8 +554,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
 		jbd2_journal_abort(journal, err);
 
 	blk_start_plug(&plug);
-	jbd2_journal_write_revoke_records(journal, commit_transaction,
-					  &log_bufs, WRITE_SYNC);
+	jbd2_journal_write_revoke_records(commit_transaction, &log_bufs);
 
 	jbd_debug(3, "JBD2: commit phase 2b\n");
 
diff --git a/fs/jbd2/revoke.c b/fs/jbd2/revoke.c
index 705ae577882b..c839332be56b 100644
--- a/fs/jbd2/revoke.c
+++ b/fs/jbd2/revoke.c
@@ -122,11 +122,11 @@ struct jbd2_revoke_table_s
 
 
 #ifdef __KERNEL__
-static void write_one_revoke_record(journal_t *, transaction_t *,
+static void write_one_revoke_record(transaction_t *,
 				    struct list_head *,
 				    struct buffer_head **, int *,
-				    struct jbd2_revoke_record_s *, int);
-static void flush_descriptor(journal_t *, struct buffer_head *, int, int);
+				    struct jbd2_revoke_record_s *);
+static void flush_descriptor(journal_t *, struct buffer_head *, int);
 #endif
 
 /* Utility functions to maintain the revoke table */
@@ -519,11 +519,10 @@ void jbd2_journal_switch_revoke_table(journal_t *journal)
  * Write revoke records to the journal for all entries in the current
  * revoke hash, deleting the entries as we go.
  */
-void jbd2_journal_write_revoke_records(journal_t *journal,
-				       transaction_t *transaction,
-				       struct list_head *log_bufs,
-				       int write_op)
+void jbd2_journal_write_revoke_records(transaction_t *transaction,
+				       struct list_head *log_bufs)
 {
+	journal_t *journal = transaction->t_journal;
 	struct buffer_head *descriptor;
 	struct jbd2_revoke_record_s *record;
 	struct jbd2_revoke_table_s *revoke;
@@ -544,16 +543,15 @@ void jbd2_journal_write_revoke_records(journal_t *journal,
 		while (!list_empty(hash_list)) {
 			record = (struct jbd2_revoke_record_s *)
 				hash_list->next;
-			write_one_revoke_record(journal, transaction, log_bufs,
-						&descriptor, &offset,
-						record, write_op);
+			write_one_revoke_record(transaction, log_bufs,
+						&descriptor, &offset, record);
 			count++;
 			list_del(&record->hash);
 			kmem_cache_free(jbd2_revoke_record_cache, record);
 		}
 	}
 	if (descriptor)
-		flush_descriptor(journal, descriptor, offset, write_op);
+		flush_descriptor(journal, descriptor, offset);
 	jbd_debug(1, "Wrote %d revoke records\n", count);
 }
 
@@ -562,14 +560,13 @@ void jbd2_journal_write_revoke_records(journal_t *journal,
  * block if the old one is full or if we have not already created one.
  */
 
-static void write_one_revoke_record(journal_t *journal,
-				    transaction_t *transaction,
+static void write_one_revoke_record(transaction_t *transaction,
 				    struct list_head *log_bufs,
 				    struct buffer_head **descriptorp,
 				    int *offsetp,
-				    struct jbd2_revoke_record_s *record,
-				    int write_op)
+				    struct jbd2_revoke_record_s *record)
 {
+	journal_t *journal = transaction->t_journal;
 	int csum_size = 0;
 	struct buffer_head *descriptor;
 	int sz, offset;
@@ -597,7 +594,7 @@ static void write_one_revoke_record(journal_t *journal,
 	/* Make sure we have a descriptor with space left for the record */
 	if (descriptor) {
 		if (offset + sz > journal->j_blocksize - csum_size) {
-			flush_descriptor(journal, descriptor, offset, write_op);
+			flush_descriptor(journal, descriptor, offset);
 			descriptor = NULL;
 		}
 	}
@@ -654,7 +651,7 @@ static void jbd2_revoke_csum_set(journal_t *j, struct buffer_head *bh)
 
 static void flush_descriptor(journal_t *journal,
 			     struct buffer_head *descriptor,
-			     int offset, int write_op)
+			     int offset)
 {
 	jbd2_journal_revoke_header_t *header;
 
@@ -670,7 +667,7 @@ static void flush_descriptor(journal_t *journal,
 	set_buffer_jwrite(descriptor);
 	BUFFER_TRACE(descriptor, "write");
 	set_buffer_dirty(descriptor);
-	write_dirty_buffer(descriptor, write_op);
+	write_dirty_buffer(descriptor, WRITE_SYNC);
 }
 #endif
 
diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
index 65407f6c9120..5716d9554e77 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -1327,10 +1327,8 @@ extern int	   jbd2_journal_init_revoke_caches(void);
 extern void	   jbd2_journal_destroy_revoke(journal_t *);
 extern int	   jbd2_journal_revoke (handle_t *, unsigned long long, struct buffer_head *);
 extern int	   jbd2_journal_cancel_revoke(handle_t *, struct journal_head *);
-extern void	   jbd2_journal_write_revoke_records(journal_t *journal,
-						     transaction_t *transaction,
-						     struct list_head *log_bufs,
-						     int write_op);
+extern void	   jbd2_journal_write_revoke_records(transaction_t *transaction,
+						     struct list_head *log_bufs);
 
 /* Recovery revoke support */
 extern int	jbd2_journal_set_revoke(journal_t *, unsigned long long, tid_t);
-- 
2.6.2

[PATCH 2/4] jbd2: Factor out common descriptor block initialization

[Jan Kara]

Descriptor block header is initialized in several places. Factor out the
common code into jbd2_journal_get_descriptor_buffer().

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/jbd2/commit.c     | 16 +++++-----------
 fs/jbd2/journal.c    |  9 ++++++++-
 fs/jbd2/revoke.c     |  8 ++------
 include/linux/jbd2.h |  2 +-
 4 files changed, 16 insertions(+), 19 deletions(-)

diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
index ae4402d15d46..cf221f3d955a 100644
--- a/fs/jbd2/commit.c
+++ b/fs/jbd2/commit.c
@@ -131,14 +131,12 @@ static int journal_submit_commit_record(journal_t *journal,
 	if (is_journal_aborted(journal))
 		return 0;
 
-	bh = jbd2_journal_get_descriptor_buffer(journal);
+	bh = jbd2_journal_get_descriptor_buffer(commit_transaction,
+						JBD2_COMMIT_BLOCK);
 	if (!bh)
 		return 1;
 
 	tmp = (struct commit_header *)bh->b_data;
-	tmp->h_magic = cpu_to_be32(JBD2_MAGIC_NUMBER);
-	tmp->h_blocktype = cpu_to_be32(JBD2_COMMIT_BLOCK);
-	tmp->h_sequence = cpu_to_be32(commit_transaction->t_tid);
 	tmp->h_commit_sec = cpu_to_be64(now.tv_sec);
 	tmp->h_commit_nsec = cpu_to_be32(now.tv_nsec);
 
@@ -379,7 +377,6 @@ void jbd2_journal_commit_transaction(journal_t *journal)
 	ktime_t start_time;
 	u64 commit_time;
 	char *tagp = NULL;
-	journal_header_t *header;
 	journal_block_tag_t *tag = NULL;
 	int space_left = 0;
 	int first_tag = 0;
@@ -615,7 +612,9 @@ void jbd2_journal_commit_transaction(journal_t *journal)
 
 			jbd_debug(4, "JBD2: get descriptor\n");
 
-			descriptor = jbd2_journal_get_descriptor_buffer(journal);
+			descriptor = jbd2_journal_get_descriptor_buffer(
+							commit_transaction,
+							JBD2_DESCRIPTOR_BLOCK);
 			if (!descriptor) {
 				jbd2_journal_abort(journal, -EIO);
 				continue;
@@ -624,11 +623,6 @@ void jbd2_journal_commit_transaction(journal_t *journal)
 			jbd_debug(4, "JBD2: got buffer %llu (%p)\n",
 				(unsigned long long)descriptor->b_blocknr,
 				descriptor->b_data);
-			header = (journal_header_t *)descriptor->b_data;
-			header->h_magic     = cpu_to_be32(JBD2_MAGIC_NUMBER);
-			header->h_blocktype = cpu_to_be32(JBD2_DESCRIPTOR_BLOCK);
-			header->h_sequence  = cpu_to_be32(commit_transaction->t_tid);
-
 			tagp = &descriptor->b_data[sizeof(journal_header_t)];
 			space_left = descriptor->b_size -
 						sizeof(journal_header_t);
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 81e622681c82..28d05bd9a588 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -805,10 +805,13 @@ int jbd2_journal_bmap(journal_t *journal, unsigned long blocknr,
  * But we don't bother doing that, so there will be coherency problems with
  * mmaps of blockdevs which hold live JBD-controlled filesystems.
  */
-struct buffer_head *jbd2_journal_get_descriptor_buffer(journal_t *journal)
+struct buffer_head *
+jbd2_journal_get_descriptor_buffer(transaction_t *transaction, int type)
 {
+	journal_t *journal = transaction->t_journal;
 	struct buffer_head *bh;
 	unsigned long long blocknr;
+	journal_header_t *header;
 	int err;
 
 	err = jbd2_journal_next_log_block(journal, &blocknr);
@@ -821,6 +824,10 @@ struct buffer_head *jbd2_journal_get_descriptor_buffer(journal_t *journal)
 		return NULL;
 	lock_buffer(bh);
 	memset(bh->b_data, 0, journal->j_blocksize);
+	header = (journal_header_t *)bh->b_data;
+	header->h_magic = cpu_to_be32(JBD2_MAGIC_NUMBER);
+	header->h_blocktype = cpu_to_be32(type);
+	header->h_sequence = cpu_to_be32(transaction->t_tid);
 	set_buffer_uptodate(bh);
 	unlock_buffer(bh);
 	BUFFER_TRACE(bh, "return this buffer");
diff --git a/fs/jbd2/revoke.c b/fs/jbd2/revoke.c
index c839332be56b..d1ebb1d41d17 100644
--- a/fs/jbd2/revoke.c
+++ b/fs/jbd2/revoke.c
@@ -570,7 +570,6 @@ static void write_one_revoke_record(transaction_t *transaction,
 	int csum_size = 0;
 	struct buffer_head *descriptor;
 	int sz, offset;
-	journal_header_t *header;
 
 	/* If we are already aborting, this all becomes a noop.  We
            still need to go round the loop in
@@ -600,13 +599,10 @@ static void write_one_revoke_record(transaction_t *transaction,
 	}
 
 	if (!descriptor) {
-		descriptor = jbd2_journal_get_descriptor_buffer(journal);
+		descriptor = jbd2_journal_get_descriptor_buffer(transaction,
+							JBD2_REVOKE_BLOCK);
 		if (!descriptor)
 			return;
-		header = (journal_header_t *)descriptor->b_data;
-		header->h_magic     = cpu_to_be32(JBD2_MAGIC_NUMBER);
-		header->h_blocktype = cpu_to_be32(JBD2_REVOKE_BLOCK);
-		header->h_sequence  = cpu_to_be32(transaction->t_tid);
 
 		/* Record it so that we can wait for IO completion later */
 		BUFFER_TRACE(descriptor, "file in log_bufs");
diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
index 5716d9554e77..3649cb8d3a41 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -1137,7 +1137,7 @@ static inline void jbd2_unfile_log_bh(struct buffer_head *bh)
 }
 
 /* Log buffer allocation */
-struct buffer_head *jbd2_journal_get_descriptor_buffer(journal_t *journal);
+struct buffer_head *jbd2_journal_get_descriptor_buffer(transaction_t *, int);
 int jbd2_journal_next_log_block(journal_t *, unsigned long long *);
 int jbd2_journal_get_log_tail(journal_t *journal, tid_t *tid,
 			      unsigned long *block);
-- 
2.6.2

[PATCH 3/4] jbd2: Unify revoke and tag block checksum handling

[Jan Kara]

Revoke and tag descriptor blocks are just different kinds of descriptor
blocks and thus have checksum in the same place. Unify computation and
checking of checksums for these.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/jbd2/commit.c     | 18 +-----------------
 fs/jbd2/journal.c    | 15 +++++++++++++++
 fs/jbd2/recovery.c   | 31 +++++--------------------------
 fs/jbd2/revoke.c     | 19 ++-----------------
 include/linux/jbd2.h |  8 ++------
 5 files changed, 25 insertions(+), 66 deletions(-)

diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
index cf221f3d955a..ae832cd44dd8 100644
--- a/fs/jbd2/commit.c
+++ b/fs/jbd2/commit.c
@@ -317,22 +317,6 @@ static void write_tag_block(journal_t *j, journal_block_tag_t *tag,
 		tag->t_blocknr_high = cpu_to_be32((block >> 31) >> 1);
 }
 
-static void jbd2_descr_block_csum_set(journal_t *j,
-				      struct buffer_head *bh)
-{
-	struct jbd2_journal_block_tail *tail;
-	__u32 csum;
-
-	if (!jbd2_journal_has_csum_v2or3(j))
-		return;
-
-	tail = (struct jbd2_journal_block_tail *)(bh->b_data + j->j_blocksize -
-			sizeof(struct jbd2_journal_block_tail));
-	tail->t_checksum = 0;
-	csum = jbd2_chksum(j, j->j_csum_seed, bh->b_data, j->j_blocksize);
-	tail->t_checksum = cpu_to_be32(csum);
-}
-
 static void jbd2_block_tag_csum_set(journal_t *j, journal_block_tag_t *tag,
 				    struct buffer_head *bh, __u32 sequence)
 {
@@ -714,7 +698,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
 
 			tag->t_flags |= cpu_to_be16(JBD2_FLAG_LAST_TAG);
 
-			jbd2_descr_block_csum_set(journal, descriptor);
+			jbd2_descriptor_block_csum_set(journal, descriptor);
 start_journal_io:
 			for (i = 0; i < bufs; i++) {
 				struct buffer_head *bh = wbuf[i];
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index 28d05bd9a588..defa962a8e15 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -834,6 +834,21 @@ jbd2_journal_get_descriptor_buffer(transaction_t *transaction, int type)
 	return bh;
 }
 
+void jbd2_descriptor_block_csum_set(journal_t *j, struct buffer_head *bh)
+{
+	struct jbd2_journal_block_tail *tail;
+	__u32 csum;
+
+	if (!jbd2_journal_has_csum_v2or3(j))
+		return;
+
+	tail = (struct jbd2_journal_block_tail *)(bh->b_data + j->j_blocksize -
+			sizeof(struct jbd2_journal_block_tail));
+	tail->t_checksum = 0;
+	csum = jbd2_chksum(j, j->j_csum_seed, bh->b_data, j->j_blocksize);
+	tail->t_checksum = cpu_to_be32(csum);
+}
+
 /*
  * Return tid of the oldest transaction in the journal and block in the journal
  * where the transaction starts.
diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
index 7f277e49fe88..08a456b96e4e 100644
--- a/fs/jbd2/recovery.c
+++ b/fs/jbd2/recovery.c
@@ -174,8 +174,7 @@ static int jread(struct buffer_head **bhp, journal_t *journal,
 	return 0;
 }
 
-static int jbd2_descr_block_csum_verify(journal_t *j,
-					void *buf)
+static int jbd2_descriptor_block_csum_verify(journal_t *j, void *buf)
 {
 	struct jbd2_journal_block_tail *tail;
 	__be32 provided;
@@ -522,8 +521,8 @@ static int do_one_pass(journal_t *journal,
 				descr_csum_size =
 					sizeof(struct jbd2_journal_block_tail);
 			if (descr_csum_size > 0 &&
-			    !jbd2_descr_block_csum_verify(journal,
-							  bh->b_data)) {
+			    !jbd2_descriptor_block_csum_verify(journal,
+							       bh->b_data)) {
 				printk(KERN_ERR "JBD2: Invalid checksum "
 				       "recovering block %lu in log\n",
 				       next_log_block);
@@ -811,26 +810,6 @@ static int do_one_pass(journal_t *journal,
 	return err;
 }
 
-static int jbd2_revoke_block_csum_verify(journal_t *j,
-					 void *buf)
-{
-	struct jbd2_journal_revoke_tail *tail;
-	__be32 provided;
-	__u32 calculated;
-
-	if (!jbd2_journal_has_csum_v2or3(j))
-		return 1;
-
-	tail = (struct jbd2_journal_revoke_tail *)(buf + j->j_blocksize -
-			sizeof(struct jbd2_journal_revoke_tail));
-	provided = tail->r_checksum;
-	tail->r_checksum = 0;
-	calculated = jbd2_chksum(j, j->j_csum_seed, buf, j->j_blocksize);
-	tail->r_checksum = provided;
-
-	return provided == cpu_to_be32(calculated);
-}
-
 /* Scan a revoke record, marking all blocks mentioned as revoked. */
 
 static int scan_revoke_records(journal_t *journal, struct buffer_head *bh,
@@ -846,11 +825,11 @@ static int scan_revoke_records(journal_t *journal, struct buffer_head *bh,
 	offset = sizeof(jbd2_journal_revoke_header_t);
 	rcount = be32_to_cpu(header->r_count);
 
-	if (!jbd2_revoke_block_csum_verify(journal, header))
+	if (!jbd2_descriptor_block_csum_verify(journal, header))
 		return -EFSBADCRC;
 
 	if (jbd2_journal_has_csum_v2or3(journal))
-		csum_size = sizeof(struct jbd2_journal_revoke_tail);
+		csum_size = sizeof(struct jbd2_journal_block_tail);
 	if (rcount > journal->j_blocksize - csum_size)
 		return -EINVAL;
 	max = rcount;
diff --git a/fs/jbd2/revoke.c b/fs/jbd2/revoke.c
index d1ebb1d41d17..91171dc352cb 100644
--- a/fs/jbd2/revoke.c
+++ b/fs/jbd2/revoke.c
@@ -583,7 +583,7 @@ static void write_one_revoke_record(transaction_t *transaction,
 
 	/* Do we need to leave space at the end for a checksum? */
 	if (jbd2_journal_has_csum_v2or3(journal))
-		csum_size = sizeof(struct jbd2_journal_revoke_tail);
+		csum_size = sizeof(struct jbd2_journal_block_tail);
 
 	if (jbd2_has_feature_64bit(journal))
 		sz = 8;
@@ -623,21 +623,6 @@ static void write_one_revoke_record(transaction_t *transaction,
 	*offsetp = offset;
 }
 
-static void jbd2_revoke_csum_set(journal_t *j, struct buffer_head *bh)
-{
-	struct jbd2_journal_revoke_tail *tail;
-	__u32 csum;
-
-	if (!jbd2_journal_has_csum_v2or3(j))
-		return;
-
-	tail = (struct jbd2_journal_revoke_tail *)(bh->b_data + j->j_blocksize -
-			sizeof(struct jbd2_journal_revoke_tail));
-	tail->r_checksum = 0;
-	csum = jbd2_chksum(j, j->j_csum_seed, bh->b_data, j->j_blocksize);
-	tail->r_checksum = cpu_to_be32(csum);
-}
-
 /*
  * Flush a revoke descriptor out to the journal.  If we are aborting,
  * this is a noop; otherwise we are generating a buffer which needs to
@@ -658,7 +643,7 @@ static void flush_descriptor(journal_t *journal,
 
 	header = (jbd2_journal_revoke_header_t *)descriptor->b_data;
 	header->r_count = cpu_to_be32(offset);
-	jbd2_revoke_csum_set(journal, descriptor);
+	jbd2_descriptor_block_csum_set(journal, descriptor);
 
 	set_buffer_jwrite(descriptor);
 	BUFFER_TRACE(descriptor, "write");
diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
index 3649cb8d3a41..fd1083c46c61 100644
--- a/include/linux/jbd2.h
+++ b/include/linux/jbd2.h
@@ -200,7 +200,7 @@ typedef struct journal_block_tag_s
 	__be32		t_blocknr_high; /* most-significant high 32bits. */
 } journal_block_tag_t;
 
-/* Tail of descriptor block, for checksumming */
+/* Tail of descriptor or revoke block, for checksumming */
 struct jbd2_journal_block_tail {
 	__be32		t_checksum;	/* crc32c(uuid+descr_block) */
 };
@@ -215,11 +215,6 @@ typedef struct jbd2_journal_revoke_header_s
 	__be32		 r_count;	/* Count of bytes used in the block */
 } jbd2_journal_revoke_header_t;
 
-/* Tail of revoke block, for checksumming */
-struct jbd2_journal_revoke_tail {
-	__be32		r_checksum;	/* crc32c(uuid+revoke_block) */
-};

[PATCH 4/4] jbd2: Save some atomic ops in __JI_COMMIT_RUNNING handling

[Jan Kara]

Currently we used atomic bit operations to manipulate
__JI_COMMIT_RUNNING bit. However this is unnecessary as i_flags are
always written and read under j_list_lock. So just change the operations
to standard bit operations.

Signed-off-by: Jan Kara <jack@suse.cz>
---
 fs/jbd2/commit.c  | 12 ++++++------
 fs/jbd2/journal.c |  2 +-
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
index ae832cd44dd8..517f2de784cf 100644
--- a/fs/jbd2/commit.c
+++ b/fs/jbd2/commit.c
@@ -220,7 +220,7 @@ static int journal_submit_data_buffers(journal_t *journal,
 	spin_lock(&journal->j_list_lock);
 	list_for_each_entry(jinode, &commit_transaction->t_inode_list, i_list) {
 		mapping = jinode->i_vfs_inode->i_mapping;
-		set_bit(__JI_COMMIT_RUNNING, &jinode->i_flags);
+		jinode->i_flags |= JI_COMMIT_RUNNING;
 		spin_unlock(&journal->j_list_lock);
 		/*
 		 * submit the inode data buffers. We use writepage
@@ -234,8 +234,8 @@ static int journal_submit_data_buffers(journal_t *journal,
 			ret = err;
 		spin_lock(&journal->j_list_lock);
 		J_ASSERT(jinode->i_transaction == commit_transaction);
-		clear_bit(__JI_COMMIT_RUNNING, &jinode->i_flags);
-		smp_mb__after_atomic();
+		jinode->i_flags &= ~JI_COMMIT_RUNNING;
+		smp_mb();
 		wake_up_bit(&jinode->i_flags, __JI_COMMIT_RUNNING);
 	}
 	spin_unlock(&journal->j_list_lock);
@@ -256,7 +256,7 @@ static int journal_finish_inode_data_buffers(journal_t *journal,
 	/* For locking, see the comment in journal_submit_data_buffers() */
 	spin_lock(&journal->j_list_lock);
 	list_for_each_entry(jinode, &commit_transaction->t_inode_list, i_list) {
-		set_bit(__JI_COMMIT_RUNNING, &jinode->i_flags);
+		jinode->i_flags |= JI_COMMIT_RUNNING;
 		spin_unlock(&journal->j_list_lock);
 		err = filemap_fdatawait(jinode->i_vfs_inode->i_mapping);
 		if (err) {
@@ -272,8 +272,8 @@ static int journal_finish_inode_data_buffers(journal_t *journal,
 				ret = err;
 		}
 		spin_lock(&journal->j_list_lock);
-		clear_bit(__JI_COMMIT_RUNNING, &jinode->i_flags);
-		smp_mb__after_atomic();
+		jinode->i_flags &= ~JI_COMMIT_RUNNING;
+		smp_mb();
 		wake_up_bit(&jinode->i_flags, __JI_COMMIT_RUNNING);
 	}
 
diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
index defa962a8e15..7bf1683e81af 100644
--- a/fs/jbd2/journal.c
+++ b/fs/jbd2/journal.c
@@ -2587,7 +2587,7 @@ void jbd2_journal_release_jbd_inode(journal_t *journal,
 restart:
 	spin_lock(&journal->j_list_lock);
 	/* Is commit writing out inode - we have to wait */
-	if (test_bit(__JI_COMMIT_RUNNING, &jinode->i_flags)) {
+	if (jinode->i_flags & JI_COMMIT_RUNNING) {
 		wait_queue_head_t *wq;
 		DEFINE_WAIT_BIT(wait, &jinode->i_flags, __JI_COMMIT_RUNNING);
 		wq = bit_waitqueue(&jinode->i_flags, __JI_COMMIT_RUNNING);
-- 
2.6.2

Re: [PATCH 3/4] jbd2: Unify revoke and tag block checksum handling

[Darrick J. Wong]

On Thu, Jan 07, 2016 at 03:41:54PM +0100, Jan Kara wrote:
> Revoke and tag descriptor blocks are just different kinds of descriptor
> blocks and thus have checksum in the same place. Unify computation and
> checking of checksums for these.

Looks ok to me,
Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

Is there a corresponding update to e2fsprogs' journal code?

--D

> 
> Signed-off-by: Jan Kara <jack@suse.cz>
> ---
>  fs/jbd2/commit.c     | 18 +-----------------
>  fs/jbd2/journal.c    | 15 +++++++++++++++
>  fs/jbd2/recovery.c   | 31 +++++--------------------------
>  fs/jbd2/revoke.c     | 19 ++-----------------
>  include/linux/jbd2.h |  8 ++------
>  5 files changed, 25 insertions(+), 66 deletions(-)
> 
> diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> index cf221f3d955a..ae832cd44dd8 100644
> --- a/fs/jbd2/commit.c
> +++ b/fs/jbd2/commit.c
> @@ -317,22 +317,6 @@ static void write_tag_block(journal_t *j, journal_block_tag_t *tag,
>  		tag->t_blocknr_high = cpu_to_be32((block >> 31) >> 1);
>  }
>  
> -static void jbd2_descr_block_csum_set(journal_t *j,
> -				      struct buffer_head *bh)
> -{
> -	struct jbd2_journal_block_tail *tail;
> -	__u32 csum;
> -
> -	if (!jbd2_journal_has_csum_v2or3(j))
> -		return;
> -
> -	tail = (struct jbd2_journal_block_tail *)(bh->b_data + j->j_blocksize -
> -			sizeof(struct jbd2_journal_block_tail));
> -	tail->t_checksum = 0;
> -	csum = jbd2_chksum(j, j->j_csum_seed, bh->b_data, j->j_blocksize);
> -	tail->t_checksum = cpu_to_be32(csum);
> -}
> -
>  static void jbd2_block_tag_csum_set(journal_t *j, journal_block_tag_t *tag,
>  				    struct buffer_head *bh, __u32 sequence)
>  {
> @@ -714,7 +698,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
>  
>  			tag->t_flags |= cpu_to_be16(JBD2_FLAG_LAST_TAG);
>  
> -			jbd2_descr_block_csum_set(journal, descriptor);
> +			jbd2_descriptor_block_csum_set(journal, descriptor);
>  start_journal_io:
>  			for (i = 0; i < bufs; i++) {
>  				struct buffer_head *bh = wbuf[i];
> diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> index 28d05bd9a588..defa962a8e15 100644
> --- a/fs/jbd2/journal.c
> +++ b/fs/jbd2/journal.c
> @@ -834,6 +834,21 @@ jbd2_journal_get_descriptor_buffer(transaction_t *transaction, int type)
>  	return bh;
>  }
>  
> +void jbd2_descriptor_block_csum_set(journal_t *j, struct buffer_head *bh)
> +{
> +	struct jbd2_journal_block_tail *tail;
> +	__u32 csum;
> +
> +	if (!jbd2_journal_has_csum_v2or3(j))
> +		return;
> +
> +	tail = (struct jbd2_journal_block_tail *)(bh->b_data + j->j_blocksize -
> +			sizeof(struct jbd2_journal_block_tail));
> +	tail->t_checksum = 0;
> +	csum = jbd2_chksum(j, j->j_csum_seed, bh->b_data, j->j_blocksize);
> +	tail->t_checksum = cpu_to_be32(csum);
> +}
> +
>  /*
>   * Return tid of the oldest transaction in the journal and block in the journal
>   * where the transaction starts.
> diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
> index 7f277e49fe88..08a456b96e4e 100644
> --- a/fs/jbd2/recovery.c
> +++ b/fs/jbd2/recovery.c
> @@ -174,8 +174,7 @@ static int jread(struct buffer_head **bhp, journal_t *journal,
>  	return 0;
>  }
>  
> -static int jbd2_descr_block_csum_verify(journal_t *j,
> -					void *buf)
> +static int jbd2_descriptor_block_csum_verify(journal_t *j, void *buf)
>  {
>  	struct jbd2_journal_block_tail *tail;
>  	__be32 provided;
> @@ -522,8 +521,8 @@ static int do_one_pass(journal_t *journal,
>  				descr_csum_size =
>  					sizeof(struct jbd2_journal_block_tail);
>  			if (descr_csum_size > 0 &&
> -			    !jbd2_descr_block_csum_verify(journal,
> -							  bh->b_data)) {
> +			    !jbd2_descriptor_block_csum_verify(journal,
> +							       bh->b_data)) {
>  				printk(KERN_ERR "JBD2: Invalid checksum "
>  				       "recovering block %lu in log\n",
>  				       next_log_block);
> @@ -811,26 +810,6 @@ static int do_one_pass(journal_t *journal,
>  	return err;
>  }
>  
> -static int jbd2_revoke_block_csum_verify(journal_t *j,
> -					 void *buf)
> -{
> -	struct jbd2_journal_revoke_tail *tail;
> -	__be32 provided;
> -	__u32 calculated;
> -
> -	if (!jbd2_journal_has_csum_v2or3(j))
> -		return 1;
> -
> -	tail = (struct jbd2_journal_revoke_tail *)(buf + j->j_blocksize -
> -			sizeof(struct jbd2_journal_revoke_tail));
> -	provided = tail->r_checksum;
> -	tail->r_checksum = 0;
> -	calculated = jbd2_chksum(j, j->j_csum_seed, buf, j->j_blocksize);
> -	tail->r_checksum = provided;
> -
> -	return provided == cpu_to_be32(calculated);
> -}
> -
>  /* Scan a revoke record, marking all blocks mentioned as revoked. */
>  
>  static int scan_revoke_records(journal_t *journal, struct buffer_head *bh,
> @@ -846,11 +825,11 @@ static int scan_revoke_records(journal_t *journal, struct buffer_head *bh,
>  	offset = sizeof(jbd2_journal_revoke_header_t);
>  	rcount = be32_to_cpu(header->r_count);
>  
> -	if (!jbd2_revoke_block_csum_verify(journal, header))
> +	if (!jbd2_descriptor_block_csum_verify(journal, header))
>  		return -EFSBADCRC;
>  
>  	if (jbd2_journal_has_csum_v2or3(journal))
> -		csum_size = sizeof(struct jbd2_journal_revoke_tail);
> +		csum_size = sizeof(struct jbd2_journal_block_tail);
>  	if (rcount > journal->j_blocksize - csum_size)
>  		return -EINVAL;
>  	max = rcount;
> diff --git a/fs/jbd2/revoke.c b/fs/jbd2/revoke.c
> index d1ebb1d41d17..91171dc352cb 100644
> --- a/fs/jbd2/revoke.c
> +++ b/fs/jbd2/revoke.c
> @@ -583,7 +583,7 @@ static void write_one_revoke_record(transaction_t *transaction,
>  
>  	/* Do we need to leave space at the end for a checksum? */
>  	if (jbd2_journal_has_csum_v2or3(journal))
> -		csum_size = sizeof(struct jbd2_journal_revoke_tail);
> +		csum_size = sizeof(struct jbd2_journal_block_tail);
>  
>  	if (jbd2_has_feature_64bit(journal))
>  		sz = 8;
> @@ -623,21 +623,6 @@ static void write_one_revoke_record(transaction_t *transaction,
>  	*offsetp = offset;
>  }
>  
> -static void jbd2_revoke_csum_set(journal_t *j, struct buffer_head *bh)
> -{
> -	struct jbd2_journal_revoke_tail *tail;
> -	__u32 csum;
> -
> -	if (!jbd2_journal_has_csum_v2or3(j))
> -		return;
> -
> -	tail = (struct jbd2_journal_revoke_tail *)(bh->b_data + j->j_blocksize -
> -			sizeof(struct jbd2_journal_revoke_tail));
> -	tail->r_checksum = 0;
> -	csum = jbd2_chksum(j, j->j_csum_seed, bh->b_data, j->j_blocksize);
> -	tail->r_checksum = cpu_to_be32(csum);
> -}
> -
>  /*
>   * Flush a revoke descriptor out to the journal.  If we are aborting,
>   * this is a noop; otherwise we are generating a buffer which needs to
> @@ -658,7 +643,7 @@ static void flush_descriptor(journal_t *journal,
>  
>  	header = (jbd2_journal_revoke_header_t *)descriptor->b_data;
>  	header->r_count = cpu_to_be32(offset);
> -	jbd2_revoke_csum_set(journal, descriptor);
> +	jbd2_descriptor_block_csum_set(journal, descriptor);
>  
>  	set_buffer_jwrite(descriptor);
>  	BUFFER_TRACE(descriptor, "write");
> diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> index 3649cb8d3a41..fd1083c46c61 100644
> --- a/include/linux/jbd2.h
> +++ b/include/linux/jbd2.h
> @@ -200,7 +200,7 @@ typedef struct journal_block_tag_s
>  	__be32		t_blocknr_high; /* most-significant high 32bits. */
>  } journal_block_tag_t;
>  
> -/* Tail of descriptor block, for checksumming */
> +/* Tail of descriptor or revoke block, for checksumming */
>  struct jbd2_journal_block_tail {
>  	__be32		t_checksum;	/* crc32c(uuid+descr_block) */
>  };
> @@ -215,11 +215,6 @@ typedef struct jbd2_journal_revoke_header_s
>  	__be32		 r_count;	/* Count of bytes used in the block */
>  } jbd2_journal_revoke_header_t;
>  
> -/* Tail of revoke block, for checksumming */
> -struct jbd2_journal_revoke_tail {
> -	__be32		r_checksum;	/* crc32c(uuid+revoke_block) */
> -};
> -
>  /* Definitions for the journal tag flags word: */
>  #define JBD2_FLAG_ESCAPE		1	/* on-disk block is escaped */
>  #define JBD2_FLAG_SAME_UUID	2	/* block has same uuid as previous */
> @@ -1138,6 +1133,7 @@ static inline void jbd2_unfile_log_bh(struct buffer_head *bh)
>  
>  /* Log buffer allocation */
>  struct buffer_head *jbd2_journal_get_descriptor_buffer(transaction_t *, int);
> +void jbd2_descriptor_block_csum_set(journal_t *, struct buffer_head *);
>  int jbd2_journal_next_log_block(journal_t *, unsigned long long *);
>  int jbd2_journal_get_log_tail(journal_t *journal, tid_t *tid,
>  			      unsigned long *block);
> -- 
> 2.6.2
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/4] jbd2: Unify revoke and tag block checksum handling

[Jan Kara]

On Thu 07-01-16 09:16:45, Darrick J. Wong wrote:
> On Thu, Jan 07, 2016 at 03:41:54PM +0100, Jan Kara wrote:
> > Revoke and tag descriptor blocks are just different kinds of descriptor
> > blocks and thus have checksum in the same place. Unify computation and
> > checking of checksums for these.
> 
> Looks ok to me,
> Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>

Thanks for review.

> Is there a corresponding update to e2fsprogs' journal code?

No, I can write one if Ted decides to merge this in JBD2.

								Honza

> > Signed-off-by: Jan Kara <jack@suse.cz>
> > ---
> >  fs/jbd2/commit.c     | 18 +-----------------
> >  fs/jbd2/journal.c    | 15 +++++++++++++++
> >  fs/jbd2/recovery.c   | 31 +++++--------------------------
> >  fs/jbd2/revoke.c     | 19 ++-----------------
> >  include/linux/jbd2.h |  8 ++------
> >  5 files changed, 25 insertions(+), 66 deletions(-)
> > 
> > diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> > index cf221f3d955a..ae832cd44dd8 100644
> > --- a/fs/jbd2/commit.c
> > +++ b/fs/jbd2/commit.c
> > @@ -317,22 +317,6 @@ static void write_tag_block(journal_t *j, journal_block_tag_t *tag,
> >  		tag->t_blocknr_high = cpu_to_be32((block >> 31) >> 1);
> >  }
> >  
> > -static void jbd2_descr_block_csum_set(journal_t *j,
> > -				      struct buffer_head *bh)
> > -{
> > -	struct jbd2_journal_block_tail *tail;
> > -	__u32 csum;
> > -
> > -	if (!jbd2_journal_has_csum_v2or3(j))
> > -		return;
> > -
> > -	tail = (struct jbd2_journal_block_tail *)(bh->b_data + j->j_blocksize -
> > -			sizeof(struct jbd2_journal_block_tail));
> > -	tail->t_checksum = 0;
> > -	csum = jbd2_chksum(j, j->j_csum_seed, bh->b_data, j->j_blocksize);
> > -	tail->t_checksum = cpu_to_be32(csum);
> > -}
> > -
> >  static void jbd2_block_tag_csum_set(journal_t *j, journal_block_tag_t *tag,
> >  				    struct buffer_head *bh, __u32 sequence)
> >  {
> > @@ -714,7 +698,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
> >  
> >  			tag->t_flags |= cpu_to_be16(JBD2_FLAG_LAST_TAG);
> >  
> > -			jbd2_descr_block_csum_set(journal, descriptor);
> > +			jbd2_descriptor_block_csum_set(journal, descriptor);
> >  start_journal_io:
> >  			for (i = 0; i < bufs; i++) {
> >  				struct buffer_head *bh = wbuf[i];
> > diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> > index 28d05bd9a588..defa962a8e15 100644
> > --- a/fs/jbd2/journal.c
> > +++ b/fs/jbd2/journal.c
> > @@ -834,6 +834,21 @@ jbd2_journal_get_descriptor_buffer(transaction_t *transaction, int type)
> >  	return bh;
> >  }
> >  
> > +void jbd2_descriptor_block_csum_set(journal_t *j, struct buffer_head *bh)
> > +{
> > +	struct jbd2_journal_block_tail *tail;
> > +	__u32 csum;
> > +
> > +	if (!jbd2_journal_has_csum_v2or3(j))
> > +		return;
> > +
> > +	tail = (struct jbd2_journal_block_tail *)(bh->b_data + j->j_blocksize -
> > +			sizeof(struct jbd2_journal_block_tail));
> > +	tail->t_checksum = 0;
> > +	csum = jbd2_chksum(j, j->j_csum_seed, bh->b_data, j->j_blocksize);
> > +	tail->t_checksum = cpu_to_be32(csum);
> > +}
> > +
> >  /*
> >   * Return tid of the oldest transaction in the journal and block in the journal
> >   * where the transaction starts.
> > diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
> > index 7f277e49fe88..08a456b96e4e 100644
> > --- a/fs/jbd2/recovery.c
> > +++ b/fs/jbd2/recovery.c
> > @@ -174,8 +174,7 @@ static int jread(struct buffer_head **bhp, journal_t *journal,
> >  	return 0;
> >  }
> >  
> > -static int jbd2_descr_block_csum_verify(journal_t *j,
> > -					void *buf)
> > +static int jbd2_descriptor_block_csum_verify(journal_t *j, void *buf)
> >  {
> >  	struct jbd2_journal_block_tail *tail;
> >  	__be32 provided;
> > @@ -522,8 +521,8 @@ static int do_one_pass(journal_t *journal,
> >  				descr_csum_size =
> >  					sizeof(struct jbd2_journal_block_tail);
> >  			if (descr_csum_size > 0 &&
> > -			    !jbd2_descr_block_csum_verify(journal,
> > -							  bh->b_data)) {
> > +			    !jbd2_descriptor_block_csum_verify(journal,
> > +							       bh->b_data)) {
> >  				printk(KERN_ERR "JBD2: Invalid checksum "
> >  				       "recovering block %lu in log\n",
> >  				       next_log_block);
> > @@ -811,26 +810,6 @@ static int do_one_pass(journal_t *journal,
> >  	return err;
> >  }
> >  
> > -static int jbd2_revoke_block_csum_verify(journal_t *j,
> > -					 void *buf)
> > -{
> > -	struct jbd2_journal_revoke_tail *tail;
> > -	__be32 provided;
> > -	__u32 calculated;
> > -
> > -	if (!jbd2_journal_has_csum_v2or3(j))
> > -		return 1;
> > -
> > -	tail = (struct jbd2_journal_revoke_tail *)(buf + j->j_blocksize -
> > -			sizeof(struct jbd2_journal_revoke_tail));
> > -	provided = tail->r_checksum;
> > -	tail->r_checksum = 0;
> > -	calculated = jbd2_chksum(j, j->j_csum_seed, buf, j->j_blocksize);
> > -	tail->r_checksum = provided;
> > -
> > -	return provided == cpu_to_be32(calculated);
> > -}
> > -
> >  /* Scan a revoke record, marking all blocks mentioned as revoked. */
> >  
> >  static int scan_revoke_records(journal_t *journal, struct buffer_head *bh,
> > @@ -846,11 +825,11 @@ static int scan_revoke_records(journal_t *journal, struct buffer_head *bh,
> >  	offset = sizeof(jbd2_journal_revoke_header_t);
> >  	rcount = be32_to_cpu(header->r_count);
> >  
> > -	if (!jbd2_revoke_block_csum_verify(journal, header))
> > +	if (!jbd2_descriptor_block_csum_verify(journal, header))
> >  		return -EFSBADCRC;
> >  
> >  	if (jbd2_journal_has_csum_v2or3(journal))
> > -		csum_size = sizeof(struct jbd2_journal_revoke_tail);
> > +		csum_size = sizeof(struct jbd2_journal_block_tail);
> >  	if (rcount > journal->j_blocksize - csum_size)
> >  		return -EINVAL;
> >  	max = rcount;
> > diff --git a/fs/jbd2/revoke.c b/fs/jbd2/revoke.c
> > index d1ebb1d41d17..91171dc352cb 100644
> > --- a/fs/jbd2/revoke.c
> > +++ b/fs/jbd2/revoke.c
> > @@ -583,7 +583,7 @@ static void write_one_revoke_record(transaction_t *transaction,
> >  
> >  	/* Do we need to leave space at the end for a checksum? */
> >  	if (jbd2_journal_has_csum_v2or3(journal))
> > -		csum_size = sizeof(struct jbd2_journal_revoke_tail);
> > +		csum_size = sizeof(struct jbd2_journal_block_tail);
> >  
> >  	if (jbd2_has_feature_64bit(journal))
> >  		sz = 8;
> > @@ -623,21 +623,6 @@ static void write_one_revoke_record(transaction_t *transaction,
> >  	*offsetp = offset;
> >  }
> >  
> > -static void jbd2_revoke_csum_set(journal_t *j, struct buffer_head *bh)
> > -{
> > -	struct jbd2_journal_revoke_tail *tail;
> > -	__u32 csum;
> > -
> > -	if (!jbd2_journal_has_csum_v2or3(j))
> > -		return;
> > -
> > -	tail = (struct jbd2_journal_revoke_tail *)(bh->b_data + j->j_blocksize -
> > -			sizeof(struct jbd2_journal_revoke_tail));
> > -	tail->r_checksum = 0;
> > -	csum = jbd2_chksum(j, j->j_csum_seed, bh->b_data, j->j_blocksize);
> > -	tail->r_checksum = cpu_to_be32(csum);
> > -}
> > -
> >  /*
> >   * Flush a revoke descriptor out to the journal.  If we are aborting,
> >   * this is a noop; otherwise we are generating a buffer which needs to
> > @@ -658,7 +643,7 @@ static void flush_descriptor(journal_t *journal,
> >  
> >  	header = (jbd2_journal_revoke_header_t *)descriptor->b_data;
> >  	header->r_count = cpu_to_be32(offset);
> > -	jbd2_revoke_csum_set(journal, descriptor);
> > +	jbd2_descriptor_block_csum_set(journal, descriptor);
> >  
> >  	set_buffer_jwrite(descriptor);
> >  	BUFFER_TRACE(descriptor, "write");
> > diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> > index 3649cb8d3a41..fd1083c46c61 100644
> > --- a/include/linux/jbd2.h
> > +++ b/include/linux/jbd2.h
> > @@ -200,7 +200,7 @@ typedef struct journal_block_tag_s
> >  	__be32		t_blocknr_high; /* most-significant high 32bits. */
> >  } journal_block_tag_t;
> >  
> > -/* Tail of descriptor block, for checksumming */
> > +/* Tail of descriptor or revoke block, for checksumming */
> >  struct jbd2_journal_block_tail {
> >  	__be32		t_checksum;	/* crc32c(uuid+descr_block) */
> >  };
> > @@ -215,11 +215,6 @@ typedef struct jbd2_journal_revoke_header_s
> >  	__be32		 r_count;	/* Count of bytes used in the block */
> >  } jbd2_journal_revoke_header_t;
> >  
> > -/* Tail of revoke block, for checksumming */
> > -struct jbd2_journal_revoke_tail {
> > -	__be32		r_checksum;	/* crc32c(uuid+revoke_block) */
> > -};
> > -
> >  /* Definitions for the journal tag flags word: */
> >  #define JBD2_FLAG_ESCAPE		1	/* on-disk block is escaped */
> >  #define JBD2_FLAG_SAME_UUID	2	/* block has same uuid as previous */
> > @@ -1138,6 +1133,7 @@ static inline void jbd2_unfile_log_bh(struct buffer_head *bh)
> >  
> >  /* Log buffer allocation */
> >  struct buffer_head *jbd2_journal_get_descriptor_buffer(transaction_t *, int);
> > +void jbd2_descriptor_block_csum_set(journal_t *, struct buffer_head *);
> >  int jbd2_journal_next_log_block(journal_t *, unsigned long long *);
> >  int jbd2_journal_get_log_tail(journal_t *journal, tid_t *tid,
> >  			      unsigned long *block);
> > -- 
> > 2.6.2
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH 3/4] jbd2: Unify revoke and tag block checksum handling

[Darrick J. Wong]

On Tue, Jan 12, 2016 at 01:41:01PM +0100, Jan Kara wrote:
> On Thu 07-01-16 09:16:45, Darrick J. Wong wrote:
> > On Thu, Jan 07, 2016 at 03:41:54PM +0100, Jan Kara wrote:
> > > Revoke and tag descriptor blocks are just different kinds of descriptor
> > > blocks and thus have checksum in the same place. Unify computation and
> > > checking of checksums for these.
> > 
> > Looks ok to me,
> > Reviewed-by: Darrick J. Wong <darrick.wong@oracle.com>
> 
> Thanks for review.
> 
> > Is there a corresponding update to e2fsprogs' journal code?
> 
> No, I can write one if Ted decides to merge this in JBD2.

Ok, just checking.  I'll review one if/when I see it. :)

--D

> 
> 								Honza
> 
> > > Signed-off-by: Jan Kara <jack@suse.cz>
> > > ---
> > >  fs/jbd2/commit.c     | 18 +-----------------
> > >  fs/jbd2/journal.c    | 15 +++++++++++++++
> > >  fs/jbd2/recovery.c   | 31 +++++--------------------------
> > >  fs/jbd2/revoke.c     | 19 ++-----------------
> > >  include/linux/jbd2.h |  8 ++------
> > >  5 files changed, 25 insertions(+), 66 deletions(-)
> > > 
> > > diff --git a/fs/jbd2/commit.c b/fs/jbd2/commit.c
> > > index cf221f3d955a..ae832cd44dd8 100644
> > > --- a/fs/jbd2/commit.c
> > > +++ b/fs/jbd2/commit.c
> > > @@ -317,22 +317,6 @@ static void write_tag_block(journal_t *j, journal_block_tag_t *tag,
> > >  		tag->t_blocknr_high = cpu_to_be32((block >> 31) >> 1);
> > >  }
> > >  
> > > -static void jbd2_descr_block_csum_set(journal_t *j,
> > > -				      struct buffer_head *bh)
> > > -{
> > > -	struct jbd2_journal_block_tail *tail;
> > > -	__u32 csum;
> > > -
> > > -	if (!jbd2_journal_has_csum_v2or3(j))
> > > -		return;
> > > -
> > > -	tail = (struct jbd2_journal_block_tail *)(bh->b_data + j->j_blocksize -
> > > -			sizeof(struct jbd2_journal_block_tail));
> > > -	tail->t_checksum = 0;
> > > -	csum = jbd2_chksum(j, j->j_csum_seed, bh->b_data, j->j_blocksize);
> > > -	tail->t_checksum = cpu_to_be32(csum);
> > > -}
> > > -
> > >  static void jbd2_block_tag_csum_set(journal_t *j, journal_block_tag_t *tag,
> > >  				    struct buffer_head *bh, __u32 sequence)
> > >  {
> > > @@ -714,7 +698,7 @@ void jbd2_journal_commit_transaction(journal_t *journal)
> > >  
> > >  			tag->t_flags |= cpu_to_be16(JBD2_FLAG_LAST_TAG);
> > >  
> > > -			jbd2_descr_block_csum_set(journal, descriptor);
> > > +			jbd2_descriptor_block_csum_set(journal, descriptor);
> > >  start_journal_io:
> > >  			for (i = 0; i < bufs; i++) {
> > >  				struct buffer_head *bh = wbuf[i];
> > > diff --git a/fs/jbd2/journal.c b/fs/jbd2/journal.c
> > > index 28d05bd9a588..defa962a8e15 100644
> > > --- a/fs/jbd2/journal.c
> > > +++ b/fs/jbd2/journal.c
> > > @@ -834,6 +834,21 @@ jbd2_journal_get_descriptor_buffer(transaction_t *transaction, int type)
> > >  	return bh;
> > >  }
> > >  
> > > +void jbd2_descriptor_block_csum_set(journal_t *j, struct buffer_head *bh)
> > > +{
> > > +	struct jbd2_journal_block_tail *tail;
> > > +	__u32 csum;
> > > +
> > > +	if (!jbd2_journal_has_csum_v2or3(j))
> > > +		return;
> > > +
> > > +	tail = (struct jbd2_journal_block_tail *)(bh->b_data + j->j_blocksize -
> > > +			sizeof(struct jbd2_journal_block_tail));
> > > +	tail->t_checksum = 0;
> > > +	csum = jbd2_chksum(j, j->j_csum_seed, bh->b_data, j->j_blocksize);
> > > +	tail->t_checksum = cpu_to_be32(csum);
> > > +}
> > > +
> > >  /*
> > >   * Return tid of the oldest transaction in the journal and block in the journal
> > >   * where the transaction starts.
> > > diff --git a/fs/jbd2/recovery.c b/fs/jbd2/recovery.c
> > > index 7f277e49fe88..08a456b96e4e 100644
> > > --- a/fs/jbd2/recovery.c
> > > +++ b/fs/jbd2/recovery.c
> > > @@ -174,8 +174,7 @@ static int jread(struct buffer_head **bhp, journal_t *journal,
> > >  	return 0;
> > >  }
> > >  
> > > -static int jbd2_descr_block_csum_verify(journal_t *j,
> > > -					void *buf)
> > > +static int jbd2_descriptor_block_csum_verify(journal_t *j, void *buf)
> > >  {
> > >  	struct jbd2_journal_block_tail *tail;
> > >  	__be32 provided;
> > > @@ -522,8 +521,8 @@ static int do_one_pass(journal_t *journal,
> > >  				descr_csum_size =
> > >  					sizeof(struct jbd2_journal_block_tail);
> > >  			if (descr_csum_size > 0 &&
> > > -			    !jbd2_descr_block_csum_verify(journal,
> > > -							  bh->b_data)) {
> > > +			    !jbd2_descriptor_block_csum_verify(journal,
> > > +							       bh->b_data)) {
> > >  				printk(KERN_ERR "JBD2: Invalid checksum "
> > >  				       "recovering block %lu in log\n",
> > >  				       next_log_block);
> > > @@ -811,26 +810,6 @@ static int do_one_pass(journal_t *journal,
> > >  	return err;
> > >  }
> > >  
> > > -static int jbd2_revoke_block_csum_verify(journal_t *j,
> > > -					 void *buf)
> > > -{
> > > -	struct jbd2_journal_revoke_tail *tail;
> > > -	__be32 provided;
> > > -	__u32 calculated;
> > > -
> > > -	if (!jbd2_journal_has_csum_v2or3(j))
> > > -		return 1;
> > > -
> > > -	tail = (struct jbd2_journal_revoke_tail *)(buf + j->j_blocksize -
> > > -			sizeof(struct jbd2_journal_revoke_tail));
> > > -	provided = tail->r_checksum;
> > > -	tail->r_checksum = 0;
> > > -	calculated = jbd2_chksum(j, j->j_csum_seed, buf, j->j_blocksize);
> > > -	tail->r_checksum = provided;
> > > -
> > > -	return provided == cpu_to_be32(calculated);
> > > -}
> > > -
> > >  /* Scan a revoke record, marking all blocks mentioned as revoked. */
> > >  
> > >  static int scan_revoke_records(journal_t *journal, struct buffer_head *bh,
> > > @@ -846,11 +825,11 @@ static int scan_revoke_records(journal_t *journal, struct buffer_head *bh,
> > >  	offset = sizeof(jbd2_journal_revoke_header_t);
> > >  	rcount = be32_to_cpu(header->r_count);
> > >  
> > > -	if (!jbd2_revoke_block_csum_verify(journal, header))
> > > +	if (!jbd2_descriptor_block_csum_verify(journal, header))
> > >  		return -EFSBADCRC;
> > >  
> > >  	if (jbd2_journal_has_csum_v2or3(journal))
> > > -		csum_size = sizeof(struct jbd2_journal_revoke_tail);
> > > +		csum_size = sizeof(struct jbd2_journal_block_tail);
> > >  	if (rcount > journal->j_blocksize - csum_size)
> > >  		return -EINVAL;
> > >  	max = rcount;
> > > diff --git a/fs/jbd2/revoke.c b/fs/jbd2/revoke.c
> > > index d1ebb1d41d17..91171dc352cb 100644
> > > --- a/fs/jbd2/revoke.c
> > > +++ b/fs/jbd2/revoke.c
> > > @@ -583,7 +583,7 @@ static void write_one_revoke_record(transaction_t *transaction,
> > >  
> > >  	/* Do we need to leave space at the end for a checksum? */
> > >  	if (jbd2_journal_has_csum_v2or3(journal))
> > > -		csum_size = sizeof(struct jbd2_journal_revoke_tail);
> > > +		csum_size = sizeof(struct jbd2_journal_block_tail);
> > >  
> > >  	if (jbd2_has_feature_64bit(journal))
> > >  		sz = 8;
> > > @@ -623,21 +623,6 @@ static void write_one_revoke_record(transaction_t *transaction,
> > >  	*offsetp = offset;
> > >  }
> > >  
> > > -static void jbd2_revoke_csum_set(journal_t *j, struct buffer_head *bh)
> > > -{
> > > -	struct jbd2_journal_revoke_tail *tail;
> > > -	__u32 csum;
> > > -
> > > -	if (!jbd2_journal_has_csum_v2or3(j))
> > > -		return;
> > > -
> > > -	tail = (struct jbd2_journal_revoke_tail *)(bh->b_data + j->j_blocksize -
> > > -			sizeof(struct jbd2_journal_revoke_tail));
> > > -	tail->r_checksum = 0;
> > > -	csum = jbd2_chksum(j, j->j_csum_seed, bh->b_data, j->j_blocksize);
> > > -	tail->r_checksum = cpu_to_be32(csum);
> > > -}
> > > -
> > >  /*
> > >   * Flush a revoke descriptor out to the journal.  If we are aborting,
> > >   * this is a noop; otherwise we are generating a buffer which needs to
> > > @@ -658,7 +643,7 @@ static void flush_descriptor(journal_t *journal,
> > >  
> > >  	header = (jbd2_journal_revoke_header_t *)descriptor->b_data;
> > >  	header->r_count = cpu_to_be32(offset);
> > > -	jbd2_revoke_csum_set(journal, descriptor);
> > > +	jbd2_descriptor_block_csum_set(journal, descriptor);
> > >  
> > >  	set_buffer_jwrite(descriptor);
> > >  	BUFFER_TRACE(descriptor, "write");
> > > diff --git a/include/linux/jbd2.h b/include/linux/jbd2.h
> > > index 3649cb8d3a41..fd1083c46c61 100644
> > > --- a/include/linux/jbd2.h
> > > +++ b/include/linux/jbd2.h
> > > @@ -200,7 +200,7 @@ typedef struct journal_block_tag_s
> > >  	__be32		t_blocknr_high; /* most-significant high 32bits. */
> > >  } journal_block_tag_t;
> > >  
> > > -/* Tail of descriptor block, for checksumming */
> > > +/* Tail of descriptor or revoke block, for checksumming */
> > >  struct jbd2_journal_block_tail {
> > >  	__be32		t_checksum;	/* crc32c(uuid+descr_block) */
> > >  };
> > > @@ -215,11 +215,6 @@ typedef struct jbd2_journal_revoke_header_s
> > >  	__be32		 r_count;	/* Count of bytes used in the block */
> > >  } jbd2_journal_revoke_header_t;
> > >  
> > > -/* Tail of revoke block, for checksumming */
> > > -struct jbd2_journal_revoke_tail {
> > > -	__be32		r_checksum;	/* crc32c(uuid+revoke_block) */
> > > -};
> > > -
> > >  /* Definitions for the journal tag flags word: */
> > >  #define JBD2_FLAG_ESCAPE		1	/* on-disk block is escaped */
> > >  #define JBD2_FLAG_SAME_UUID	2	/* block has same uuid as previous */
> > > @@ -1138,6 +1133,7 @@ static inline void jbd2_unfile_log_bh(struct buffer_head *bh)
> > >  
> > >  /* Log buffer allocation */
> > >  struct buffer_head *jbd2_journal_get_descriptor_buffer(transaction_t *, int);
> > > +void jbd2_descriptor_block_csum_set(journal_t *, struct buffer_head *);
> > >  int jbd2_journal_next_log_block(journal_t *, unsigned long long *);
> > >  int jbd2_journal_get_log_tail(journal_t *journal, tid_t *tid,
> > >  			      unsigned long *block);
> > > -- 
> > > 2.6.2
> > > 
> > > --
> > > To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> > > the body of a message to majordomo@vger.kernel.org
> > > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> -- 
> Jan Kara <jack@suse.com>
> SUSE Labs, CR

Re: [PATCH 1/4] jbd2: Remove some unnecessary arguments of jbd2_journal_write_revoke_records

[Theodore Ts'o]

On Thu, Jan 07, 2016 at 03:41:52PM +0100, Jan Kara wrote:
> jbd2_journal_write_revoke_records() takes journal pointer and write_op,
> although journal can be obtained from the passed transaction and
> write_op is always WRITE_SYNC. Remove these superfluous arguments.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

Thanks, applied.

						- Ted

Re: [PATCH 2/4] jbd2: Factor out common descriptor block initialization

[Theodore Ts'o]

On Thu, Jan 07, 2016 at 03:41:53PM +0100, Jan Kara wrote:
> Descriptor block header is initialized in several places. Factor out the
> common code into jbd2_journal_get_descriptor_buffer().
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

Thanks, applied.

					- Ted

Re: [PATCH 3/4] jbd2: Unify revoke and tag block checksum handling

[Theodore Ts'o]

On Thu, Jan 07, 2016 at 03:41:54PM +0100, Jan Kara wrote:
> Revoke and tag descriptor blocks are just different kinds of descriptor
> blocks and thus have checksum in the same place. Unify computation and
> checking of checksums for these.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

Thanks, applied.

					- Ted

Re: [PATCH 4/4] jbd2: Save some atomic ops in __JI_COMMIT_RUNNING handling

[Theodore Ts'o]

On Thu, Jan 07, 2016 at 03:41:55PM +0100, Jan Kara wrote:
> Currently we used atomic bit operations to manipulate
> __JI_COMMIT_RUNNING bit. However this is unnecessary as i_flags are
> always written and read under j_list_lock. So just change the operations
> to standard bit operations.
> 
> Signed-off-by: Jan Kara <jack@suse.cz>

Thanks, applied.

					- Ted