[PATCH 0/3] debugfs fixes

[PATCH 0/3] debugfs fixes

[alexey.lyashkov]

From: Alexey Lyashkov <alexey.lyashkov@gmail.com>

This patchset provide a various fixes for debugfs tool.
All fixes related to the logdump command which able to handle large FS
and journal checksums version 1. It fixes a panic with journal super block
flags print.


Alexey Lyashkov (3):
  block number output fix.
  Fix panic with journal superblock flags printing.
  add support to check a journal checksum v1 while journal dump.

 debugfs/logdump.c | 103 +++++++++++++++++++++++++++++++++++-----------
 e2fsck/jfs_user.h |   5 +++
 2 files changed, 84 insertions(+), 24 deletions(-)

-- 
2.18.0

[PATCH 1/3] block number output fix.

[alexey.lyashkov]

[-- Attachment #1: Type: text/plain, Size: 44 bytes --]

This is a multi-part message in MIME format.

[-- Attachment #2: Type: text/plain, Size: 186 bytes --]


debugfs lack a support journal tag v3 and block numbers over 2^32
fix it.
---
 debugfs/logdump.c | 30 ++++++++++++++++++++----------
 1 file changed, 20 insertions(+), 10 deletions(-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0001-block-number-output-fix.patch --]
[-- Type: text/x-patch; name="0001-block-number-output-fix.patch", Size: 3592 bytes --]

diff --git a/debugfs/logdump.c b/debugfs/logdump.c
index e286ae83..84108a6e 100644
--- a/debugfs/logdump.c
+++ b/debugfs/logdump.c
@@ -58,11 +58,11 @@ static void dump_descriptor_block(FILE *, struct journal_source *,
 				  unsigned int *, int, tid_t);
 
 static void dump_revoke_block(FILE *, char *, journal_superblock_t *,
-				  unsigned int, int, tid_t);
+				  blk64_t, int, tid_t);
 
 static void dump_metadata_block(FILE *, struct journal_source *,
 				journal_superblock_t*,
-				unsigned int, unsigned int, unsigned int,
+				unsigned int, blk64_t, unsigned int,
 				int, tid_t);
 
 static void do_hexdump (FILE *, char *, int);
@@ -496,6 +496,16 @@ static inline size_t journal_super_tag_bytes(journal_superblock_t *jsb)
 	return sz - sizeof(__u32);
 }
 
+static blk64_t tag_blocknr(journal_superblock_t *jsb,
+			      journal_block_tag_t *tag)
+{
+	blk64_t block = be32_to_cpu(tag->t_blocknr);
+
+	if (JSB_HAS_INCOMPAT_FEATURE(jsb, JFS_FEATURE_INCOMPAT_64BIT))
+		block |= (u64)be32_to_cpu(tag->t_blocknr_high) << 32;
+	return block;
+}
+
 static void dump_descriptor_block(FILE *out_file,
 				  struct journal_source *source,
 				  char *buf,
@@ -507,7 +517,7 @@ static void dump_descriptor_block(FILE *out_file,
 	char			*tagp;
 	journal_block_tag_t	*tag;
 	unsigned int		blocknr;
-	__u32			tag_block;
+	blk64_t			tag_block;
 	__u32			tag_flags;
 
 	tag_size = journal_super_tag_bytes(jsb);
@@ -537,7 +547,7 @@ static void dump_descriptor_block(FILE *out_file,
 		if (offset > blocksize - csum_size)
 			break;
 
-		tag_block = be32_to_cpu(tag->t_blocknr);
+		tag_block = tag_blocknr(jsb, tag);
 		tag_flags = be16_to_cpu(tag->t_flags);
 
 		if (!(tag_flags & JFS_FLAG_SAME_UUID))
@@ -558,7 +568,7 @@ static void dump_descriptor_block(FILE *out_file,
 
 static void dump_revoke_block(FILE *out_file, char *buf,
 			      journal_superblock_t *jsb EXT2FS_ATTR((unused)),
-			      unsigned int blocknr,
+			      blk64_t blocknr,
 			      int blocksize EXT2FS_ATTR((unused)),
 			      tid_t transaction)
 {
@@ -569,9 +579,9 @@ static void dump_revoke_block(FILE *out_file, char *buf,
 
 	if (dump_all)
 		fprintf(out_file, "Dumping revoke block, sequence %u, at "
-			"block %u:\n", transaction, blocknr);
+			"block %llu:\n", transaction, blocknr);
 
-	if (be32_to_cpu(jsb->s_feature_incompat) & JFS_FEATURE_INCOMPAT_64BIT)
+	if (JSB_HAS_INCOMPAT_FEATURE(jsb, JFS_FEATURE_INCOMPAT_64BIT))
 		tag_size = sizeof(__u64);
 
 	header = (journal_revoke_header_t *) buf;
@@ -591,7 +601,7 @@ static void dump_revoke_block(FILE *out_file, char *buf,
 			if (dump_all)
 				fprintf(out_file, "\n");
 			else
-				fprintf(out_file," at block %u, sequence %u\n",
+				fprintf(out_file," at block %llu, sequence %u\n",
 					blocknr, transaction);
 		}
 		offset += tag_size;
@@ -617,7 +627,7 @@ static void show_indirect(FILE *out_file, const char *name, __u32 where)
 static void dump_metadata_block(FILE *out_file, struct journal_source *source,
 				journal_superblock_t *jsb EXT2FS_ATTR((unused)),
 				unsigned int log_blocknr,
-				unsigned int fs_blocknr,
+				blk64_t fs_blocknr,
 				unsigned int log_tag_flags,
 				int blocksize,
 				tid_t transaction)
@@ -631,7 +641,7 @@ static void dump_metadata_block(FILE *out_file, struct journal_source *source,
 	      || (fs_blocknr == bitmap_to_dump)))
 		return;
 
-	fprintf(out_file, "  FS block %u logged at ", fs_blocknr);
+	fprintf(out_file, "  FS block %llu logged at ", fs_blocknr);
 	if (!dump_all)
 		fprintf(out_file, "sequence %u, ", transaction);
 	fprintf(out_file, "journal block %u (flags 0x%x)\n", log_blocknr,

Re: [PATCH 1/3] block number output fix.

[Theodore Y. Ts'o]

On Mon, Nov 19, 2018 at 12:16:48PM +0300, alexey.lyashkov@gmail.com wrote:
> 
> debugfs lack a support journal tag v3 and block numbers over 2^32
> fix it.

This file is missing a Signed-off-by trailer.

Also please use a subject prefix of "debugfs: " for these patches,
since they are related to debugfs.  I also prefer not having a
trailing period, and use a more descriptive summary so that it's more
understandable what the commit will do when using "git log --oneline".

So, something like:

debugfs: fix logdump to support 64-bit block numbers with v3 journal tags

Thanks,

					- Ted

[PATCH 2/3] Fix panic with journal superblock flags printing.

[alexey.lyashkov]

[-- Attachment #1: Type: text/plain, Size: 44 bytes --]

This is a multi-part message in MIME format.

[-- Attachment #2: Type: text/plain, Size: 142 bytes --]


Signed-off-by: Alexey Lyashkov <alexey.lyashkov@gmail.com>
---
 debugfs/logdump.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0002-Fix-panic-with-journal-superblock-flags-printing.patch --]
[-- Type: text/x-patch; name="0002-Fix-panic-with-journal-superblock-flags-printing.patch", Size: 427 bytes --]

diff --git a/debugfs/logdump.c b/debugfs/logdump.c
index 84108a6e..c88f6f9c 100644
--- a/debugfs/logdump.c
+++ b/debugfs/logdump.c
@@ -388,9 +388,9 @@ static void dump_journal(char *cmdname, FILE *out_file,
 	if (retval)
 		return;
 
-	if (dump_super) {
+	if (dump_all || dump_super) {
 		e2p_list_journal_super(out_file, jsb_buffer,
-				       current_fs->blocksize, 0);
+				       1024, 0);
 		fputc('\n', out_file);
 	}
 

Re: [PATCH 2/3] Fix panic with journal superblock flags printing.

[Theodore Y. Ts'o]

The summary really isn't right; "panic" is something that kernels do,
not userspace programs.

> -	if (dump_super) {
> +	if (dump_all || dump_super) {
>  		e2p_list_journal_super(out_file, jsb_buffer,
> -				       current_fs->blocksize, 0);
> +				       1024, 0);

This makes no sense to me.  Why is hard-coding the expected blocksize
to be 1024 a good thing to do here?

					- Ted

Re: [PATCH 2/3] Fix panic with journal superblock flags printing.

[Alexey Lyashkov]

1024 is jbd superblock size, and this constant widely used over e2fsprogs code.
from this particular case it’s clear bug as jsb_buffer defined as.
>>
        char                    jsb_buffer[1024];
>>


> 22 нояб. 2018 г., в 2:54, Theodore Y. Ts'o <tytso@mit.edu> написал(а):
> 
> The summary really isn't right; "panic" is something that kernels do,
> not userspace programs.
> 
>> -	if (dump_super) {
>> +	if (dump_all || dump_super) {
>> 		e2p_list_journal_super(out_file, jsb_buffer,
>> -				       current_fs->blocksize, 0);
>> +				       1024, 0);
> 
> This makes no sense to me.  Why is hard-coding the expected blocksize
> to be 1024 a good thing to do here?
> 
> 					- Ted

Re: [PATCH 2/3] Fix panic with journal superblock flags printing.

[Theodore Y. Ts'o]

On Thu, Nov 22, 2018 at 07:24:17AM +0300, Alexey Lyashkov wrote:
> 1024 is jbd superblock size, and this constant widely used over e2fsprogs code.
> from this particular case it’s clear bug as jsb_buffer defined as.
> >>
>         char                    jsb_buffer[1024];

The jbd superblock size is 1024 bytes --- just as the ext4 superblock
is only 1024 bytes.  It may be *stored* in a 4k block, and the size of
the super block has nothingm to do with the size of the file system
block size.

It gets used here:

	if (exp_block_size != (int) ntohl(jsb->s_blocksize))
		fprintf(f, "Journal block size:       %u\n",
			(unsigned int)ntohl(jsb->s_blocksize));

The normal case is when the journal superblock size is the same as the
file system block size, and in that case, there's no reason to print
the journal block size.  It *can* happen where the journal block size
can be different from the file system block size.  The most likely
case is the one where an external journal is in use, and the external
journal is shared between two file systems, one which uses (say) a 4k
block size, and the other uses (say) a 1k block size.  The data
structures support this mode, but what we don't have is e2fsck support
for handling a journal replay where the journal needs to be replayed
to two different file systems --- especially if one of the block
device is temporarily unavailable.

So that's why this is there, and in fact it's intentional that it's
done this way.

Cheers,

						- Ted

[PATCH 3/3] add support to check a journal checksum v1 while journal dump.

[alexey.lyashkov]

[-- Attachment #1: Type: text/plain, Size: 44 bytes --]

This is a multi-part message in MIME format.

[-- Attachment #2: Type: text/plain, Size: 358 bytes --]


journal checksum v1 cover an all blocks in journal descriptor.
This checksum stored into commit block and check with commit
block reading.

Signed-off-by: Alexey Lyashkov <alexey.lyashkov@gmail.com>
---
 debugfs/logdump.c | 69 ++++++++++++++++++++++++++++++++++++++---------
 e2fsck/jfs_user.h |  5 ++++
 2 files changed, 62 insertions(+), 12 deletions(-)


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #3: 0003-add-support-to-check-a-journal-checksum-v1-while-jou.patch --]
[-- Type: text/x-patch; name="0003-add-support-to-check-a-journal-checksum-v1-while-jou.patch", Size: 5596 bytes --]

diff --git a/debugfs/logdump.c b/debugfs/logdump.c
index c88f6f9c..3dd04789 100644
--- a/debugfs/logdump.c
+++ b/debugfs/logdump.c
@@ -55,7 +55,7 @@ static void dump_journal(char *, FILE *, struct journal_source *);
 
 static void dump_descriptor_block(FILE *, struct journal_source *,
 				  char *, journal_superblock_t *,
-				  unsigned int *, int, tid_t);
+				  unsigned int *, int, tid_t, __u32 *);
 
 static void dump_revoke_block(FILE *, char *, journal_superblock_t *,
 				  blk64_t, int, tid_t);
@@ -63,10 +63,14 @@ static void dump_revoke_block(FILE *, char *, journal_superblock_t *,
 static void dump_metadata_block(FILE *, struct journal_source *,
 				journal_superblock_t*,
 				unsigned int, blk64_t, unsigned int,
-				int, tid_t);
+				int, tid_t, __u32 *);
 
 static void do_hexdump (FILE *, char *, int);
 
+static void jbd2_check_commit_cksum(FILE *, struct journal_source *,
+				  journal_superblock_t *,
+				  unsigned int , int , __u32);
+
 #define WRAP(jsb, blocknr)					\
 	if (blocknr >= be32_to_cpu((jsb)->s_maxlen))		\
 		blocknr -= (be32_to_cpu((jsb)->s_maxlen) -	\
@@ -353,6 +357,7 @@ static void dump_journal(char *cmdname, FILE *out_file,
 	journal_header_t	*header;
 	tid_t			transaction;
 	unsigned int		blocknr = 0;
+	__u32			crc32_sum = ~0;
 
 	/* First, check to see if there's an ext2 superblock header */
 	retval = read_journal_block(cmdname, source, 0, buf, 2048);
@@ -453,12 +458,16 @@ static void dump_journal(char *cmdname, FILE *out_file,
 		case JFS_DESCRIPTOR_BLOCK:
 			dump_descriptor_block(out_file, source, buf, jsb,
 					      &blocknr, blocksize,
-					      transaction);
+					      transaction, &crc32_sum);
 			continue;
 
 		case JFS_COMMIT_BLOCK:
+			jbd2_check_commit_cksum(out_file, source,
+						jsb, blocknr, blocksize,
+						crc32_sum);
 			transaction++;
 			blocknr++;
+			crc32_sum = ~0;
 			WRAP(jsb, blocknr);
 			continue;
 
@@ -511,7 +520,7 @@ static void dump_descriptor_block(FILE *out_file,
 				  char *buf,
 				  journal_superblock_t *jsb,
 				  unsigned int *blockp, int blocksize,
-				  tid_t transaction)
+				  tid_t transaction, __u32 *crc32_sum)
 {
 	int			offset, tag_size, csum_size = 0;
 	char			*tagp;
@@ -535,6 +544,9 @@ static void dump_descriptor_block(FILE *out_file,
 	++blocknr;
 	WRAP(jsb, blocknr);
 
+	if (JSB_HAS_COMPAT_FEATURE(jsb, JFS_FEATURE_COMPAT_CHECKSUM))
+		*crc32_sum = ext2fs_crc32_be(*crc32_sum, buf, blocksize);
+
 	do {
 		/* Work out the location of the current tag, and skip to
 		 * the next one... */
@@ -555,7 +567,7 @@ static void dump_descriptor_block(FILE *out_file,
 
 		dump_metadata_block(out_file, source, jsb,
 				    blocknr, tag_block, tag_flags, blocksize,
-				    transaction);
+				    transaction, crc32_sum);
 
 		++blocknr;
 		WRAP(jsb, blocknr);
@@ -630,7 +642,7 @@ static void dump_metadata_block(FILE *out_file, struct journal_source *source,
 				blk64_t fs_blocknr,
 				unsigned int log_tag_flags,
 				int blocksize,
-				tid_t transaction)
+				tid_t transaction, __u32 *crc32_sum)
 {
 	int		retval;
 	char 		buf[8192];
@@ -659,18 +671,21 @@ static void dump_metadata_block(FILE *out_file, struct journal_source *source,
 	 * structure symbolically.
 	 */
 
-	if (!(dump_contents && dump_all)
-	    && fs_blocknr != block_to_dump
-	    && fs_blocknr != bitmap_to_dump
-	    && fs_blocknr != inode_block_to_dump)
-		return;
-
 	retval = read_journal_block("logdump", source,
 				    ((ext2_loff_t) log_blocknr) * blocksize,
 				    buf, blocksize);
 	if (retval)
 		return;
 
+	if (JSB_HAS_COMPAT_FEATURE(jsb, JFS_FEATURE_COMPAT_CHECKSUM))
+		*crc32_sum = ext2fs_crc32_be(*crc32_sum, buf, blocksize);
+
+	if (!(dump_contents && dump_all)
+	    && fs_blocknr != block_to_dump
+	    && fs_blocknr != bitmap_to_dump
+	    && fs_blocknr != inode_block_to_dump)
+		return;
+
 	if (fs_blocknr == bitmap_to_dump) {
 		struct ext2_super_block *super;
 		int offset;
@@ -751,3 +766,33 @@ static void do_hexdump (FILE *out_file, char *buf, int blocksize)
 	}
 }
 
+static void jbd2_check_commit_cksum(FILE *out_file, struct journal_source *source,
+				  journal_superblock_t *jsb,
+				  unsigned int blocknr, int blocksize,
+				  __u32 crc32_sum)
+{
+	char 		buf[8192];
+	struct commit_header *h = (void *)&buf[0];
+	int retval;
+
+	if (!dump_all)
+		return;
+
+	retval = read_journal_block("logdump", source,
+				    ((ext2_loff_t) blocknr) * blocksize,
+				    buf, blocksize);
+	if (retval)
+		return;
+
+	if (JSB_HAS_COMPAT_FEATURE(jsb, JFS_FEATURE_COMPAT_CHECKSUM)) {
+		if (h->h_chksum_type != JBD2_CRC32_CHKSUM)
+			fprintf(out_file, "Unknow commit checksum %u\n",
+				h->h_chksum);
+		if (h->h_chksum_size != JBD2_CRC32_CHKSUM_SIZE)
+			fprintf(out_file, "Unknown checksum size %u\n",
+				h->h_chksum_size);
+		if (h->h_chksum[0] != cpu_to_be32(crc32_sum))
+			fprintf(out_file, "Wrong checksum %u <> %u\n",
+				h->h_chksum[0], cpu_to_be32(crc32_sum));
+	}
+}
diff --git a/e2fsck/jfs_user.h b/e2fsck/jfs_user.h
index a1c6951c..6e42a2a6 100644
--- a/e2fsck/jfs_user.h
+++ b/e2fsck/jfs_user.h
@@ -204,6 +204,11 @@ void wait_on_buffer(struct buffer_head *bh);
 #define JSB_HAS_INCOMPAT_FEATURE(jsb, mask)				\
 	((jsb)->s_header.h_blocktype == ext2fs_cpu_to_be32(JFS_SUPERBLOCK_V2) &&	\
 	 ((jsb)->s_feature_incompat & ext2fs_cpu_to_be32((mask))))
+
+#define JSB_HAS_COMPAT_FEATURE(jsb, mask)				\
+	((jsb)->s_header.h_blocktype == ext2fs_cpu_to_be32(JFS_SUPERBLOCK_V2) &&	\
+	 ((jsb)->s_feature_compat & ext2fs_cpu_to_be32((mask))))
+
 #else  /* !DEBUGFS */
 
 extern e2fsck_t e2fsck_global_ctx;  /* Try your very best not to use this! */

Re: [PATCH 3/3] add support to check a journal checksum v1 while journal dump.

[Theodore Y. Ts'o]

On Mon, Nov 19, 2018 at 12:16:50PM +0300, alexey.lyashkov@gmail.com wrote:
> 
> journal checksum v1 cover an all blocks in journal descriptor.
> This checksum stored into commit block and check with commit
> block reading.

I'm really curious --- why do you care about journal_checksum v1 at
all?  It rarely used, and the journal_checksum v3 is much superior.

      	 	       	     	     		      - Ted
						      

Re: [PATCH 3/3] add support to check a journal checksum v1 while journal dump.

[Alexey Lyashkov]

it’s typical option for Sonexion storages.

journal checksum is enough to cover a corruptions for external journal, 
but provide a less overhead than full metadata checksums.
In our’s workload - any checksums is source of huge latency, so minimal option had enabled.
from other sides it option can enabled without disk format change.

I was have plan to add v2/v3 checksums to patch series but images need to prepared for it.

In this particular usage it was used to find a bug in jbd2 code, with back porting commit
de92c8caf16ca84926fa31b7a5590c0fb9c0d5ca
    jbd2: speedup jbd2_journal_get_[write|undo]_access()

to rhel7 code.

that commits introduce a race window while frozen buffer had modified in parallel thread.
this race likely to be fixed by
ee57aba159a5c329dc78c181a3ae0549e59f0925
    jbd2: simplify code flow in do_get_write_access()

which described as cleanup.


> 22 нояб. 2018 г., в 3:07, Theodore Y. Ts'o <tytso@mit.edu> написал(а):
> 
> On Mon, Nov 19, 2018 at 12:16:50PM +0300, alexey.lyashkov@gmail.com wrote:
>> 
>> journal checksum v1 cover an all blocks in journal descriptor.
>> This checksum stored into commit block and check with commit
>> block reading.
> 
> I'm really curious --- why do you care about journal_checksum v1 at
> all?  It rarely used, and the journal_checksum v3 is much superior.
> 
>      	 	       	     	     		      - Ted
>