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

Message ID 20181119091650.81803-3-alexey.lyashkov@gmail.com
State New
Headers show
Series
  • debugfs fixes
Related show

Commit Message

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

Comments

Theodore Ts'o Nov. 21, 2018, 11:54 p.m. | #1
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
Alexey Lyashkov Nov. 22, 2018, 4:24 a.m. | #2
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
Theodore Ts'o Nov. 22, 2018, 9:23 p.m. | #3
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

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);
 	}