[PATCH] e2fsprogs: journal code small cleanup and fixes

[PATCH] e2fsprogs: journal code small cleanup and fixes

[Azat Khuzhin]

Hi there, here is a patch set with various cleanups in e2fsprogs code for 
working journal device and fix tune2fs to update UUID for journal dev properly.
(we need to copy UUID into jsb for this)

First first two are cleanups, and the last one is UUID fix:
[PATCH 1/3] journal: use consts instead of 1024 and add helper for
[PATCH 2/3] tune2fs: remove_journal_device(): use the correct block
[PATCH 3/3] tune2fs: update journal super block when changing UUID

[PATCH 1/3] journal: use consts instead of 1024 and add helper for journal with 1k blocksize

[Azat Khuzhin]

Use EXT2_MIN_BLOCK_SIZE/JFS_MIN_JOURNAL_BLOCKS instead of hardcoded 1024
when it is okay, and also add a helper ext2fs_journal_sb_start() that
will return start of journal sb with special case for fs with 1k block
size.

Signed-off-by: Azat Khuzhin <a3at.mail@gmail.com>
---
 e2fsck/journal.c       |  5 ++---
 lib/ext2fs/ext2fs.h    |  1 +
 lib/ext2fs/mkjournal.c | 28 ++++++++++++++++------------
 misc/tune2fs.c         |  6 +++---
 4 files changed, 22 insertions(+), 18 deletions(-)

diff --git a/e2fsck/journal.c b/e2fsck/journal.c
index a7b1150..5780be0 100644
--- a/e2fsck/journal.c
+++ b/e2fsck/journal.c
@@ -443,8 +443,7 @@ static errcode_t e2fsck_get_journal(e2fsck_t ctx, journal_t **ret_journal)
 	if (ext_journal) {
 		blk64_t maxlen;
 
-		if (ctx->fs->blocksize == 1024)
-			start = 1;
+		start = ext2fs_journal_sb_start(ctx->fs->blocksize) - 1;
 		bh = getblk(dev_journal, start, ctx->fs->blocksize);
 		if (!bh) {
 			retval = EXT2_ET_NO_MEMORY;
@@ -455,7 +454,7 @@ static errcode_t e2fsck_get_journal(e2fsck_t ctx, journal_t **ret_journal)
 			brelse(bh);
 			goto errout;
 		}
-		memcpy(&jsuper, start ? bh->b_data :  bh->b_data + 1024,
+		memcpy(&jsuper, start ? bh->b_data :  bh->b_data + EXT2_MIN_BLOCK_SIZE,
 		       sizeof(jsuper));
 		brelse(bh);
 #ifdef WORDS_BIGENDIAN
diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h
index 599c972..a759741 100644
--- a/lib/ext2fs/ext2fs.h
+++ b/lib/ext2fs/ext2fs.h
@@ -1498,6 +1498,7 @@ extern errcode_t ext2fs_add_journal_inode(ext2_filsys fs, blk_t num_blocks,
 extern errcode_t ext2fs_add_journal_inode2(ext2_filsys fs, blk_t num_blocks,
 					   blk64_t goal, int flags);
 extern int ext2fs_default_journal_size(__u64 num_blocks);
+extern int ext2fs_journal_sb_start(int blocksize);
 
 /* openfs.c */
 extern errcode_t ext2fs_open(const char *name, int flags, int superblock,
diff --git a/lib/ext2fs/mkjournal.c b/lib/ext2fs/mkjournal.c
index 884d9c0..068eed7 100644
--- a/lib/ext2fs/mkjournal.c
+++ b/lib/ext2fs/mkjournal.c
@@ -49,7 +49,7 @@ errcode_t ext2fs_create_journal_superblock(ext2_filsys fs,
 	errcode_t		retval;
 	journal_superblock_t	*jsb;
 
-	if (num_blocks < 1024)
+	if (num_blocks < JFS_MIN_JOURNAL_BLOCKS)
 		return EXT2_ET_JOURNAL_TOO_SMALL;
 
 	if ((retval = ext2fs_get_mem(fs->blocksize, &jsb)))
@@ -75,10 +75,7 @@ errcode_t ext2fs_create_journal_superblock(ext2_filsys fs,
 	if (fs->super->s_feature_incompat &
 	    EXT3_FEATURE_INCOMPAT_JOURNAL_DEV) {
 		jsb->s_nr_users = 0;
-		if (fs->blocksize == 1024)
-			jsb->s_first = htonl(3);
-		else
-			jsb->s_first = htonl(2);
+		jsb->s_first = ext2fs_journal_sb_start(fs->blocksize) + 1;
 	}
 
 	*ret_jsb = (char *) jsb;
@@ -430,6 +427,13 @@ int ext2fs_default_journal_size(__u64 num_blocks)
 	return 32768;
 }
 
+int ext2fs_journal_sb_start(int blocksize)
+{
+	if (blocksize == EXT2_MIN_BLOCK_SIZE)
+		return 2;
+	return 1;
+}
+
 /*
  * This function adds a journal device to a filesystem
  */
@@ -437,7 +441,7 @@ errcode_t ext2fs_add_journal_device(ext2_filsys fs, ext2_filsys journal_dev)
 {
 	struct stat	st;
 	errcode_t	retval;
-	char		buf[1024];
+	char		buf[SUPERBLOCK_SIZE];
 	journal_superblock_t	*jsb;
 	int		start;
 	__u32		i, nr_users;
@@ -450,10 +454,9 @@ errcode_t ext2fs_add_journal_device(ext2_filsys fs, ext2_filsys journal_dev)
 		return EXT2_ET_JOURNAL_NOT_BLOCK; /* Must be a block device */
 
 	/* Get the journal superblock */
-	start = 1;
-	if (journal_dev->blocksize == 1024)
-		start++;
-	if ((retval = io_channel_read_blk64(journal_dev->io, start, -1024,
+	start = ext2fs_journal_sb_start(journal_dev->blocksize);
+	if ((retval = io_channel_read_blk64(journal_dev->io, start,
+					    -SUPERBLOCK_SIZE,
 					    buf)))
 		return retval;
 
@@ -479,7 +482,8 @@ errcode_t ext2fs_add_journal_device(ext2_filsys fs, ext2_filsys journal_dev)
 	}
 
 	/* Writeback the journal superblock */
-	if ((retval = io_channel_write_blk64(journal_dev->io, start, -1024, buf)))
+	if ((retval = io_channel_write_blk64(journal_dev->io, start,
+					    -SUPERBLOCK_SIZE, buf)))
 		return retval;
 
 	fs->super->s_journal_inum = 0;
@@ -632,7 +636,7 @@ main(int argc, char **argv)
 		exit(1);
 	}
 
-	retval = ext2fs_add_journal_inode(fs, 1024, 0);
+	retval = ext2fs_add_journal_inode(fs, JFS_MIN_JOURNAL_BLOCKS, 0);
 	if (retval) {
 		com_err(argv[0], retval, "while adding journal to %s",
 			device_name);
diff --git a/misc/tune2fs.c b/misc/tune2fs.c
index d95cc3d..0e7caf2 100644
--- a/misc/tune2fs.c
+++ b/misc/tune2fs.c
@@ -187,7 +187,7 @@ static int remove_journal_device(ext2_filsys fs)
 {
 	char		*journal_path;
 	ext2_filsys	jfs;
-	char		buf[1024];
+	char		buf[SUPERBLOCK_SIZE];
 	journal_superblock_t	*jsb;
 	int		i, nr_users;
 	errcode_t	retval;
@@ -230,7 +230,7 @@ static int remove_journal_device(ext2_filsys fs)
 	}
 
 	/* Get the journal superblock */
-	if ((retval = io_channel_read_blk64(jfs->io, 1, -1024, buf))) {
+	if ((retval = io_channel_read_blk64(jfs->io, 1, -SUPERBLOCK_SIZE, buf))) {
 		com_err(program_name, retval, "%s",
 			_("while reading journal superblock"));
 		goto no_valid_journal;
@@ -261,7 +261,7 @@ static int remove_journal_device(ext2_filsys fs)
 	jsb->s_nr_users = htonl(nr_users);
 
 	/* Write back the journal superblock */
-	if ((retval = io_channel_write_blk64(jfs->io, 1, -1024, buf))) {
+	if ((retval = io_channel_write_blk64(jfs->io, 1, -SUPERBLOCK_SIZE, buf))) {
 		com_err(program_name, retval,
 			"while writing journal superblock.");
 		goto no_valid_journal;
-- 
2.0.0

Re: [PATCH 1/3] journal: use consts instead of 1024 and add helper for journal with 1k blocksize

[Andreas Dilger]

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

On Jul 8, 2014, at 2:41 PM, Azat Khuzhin <a3at.mail@gmail.com> wrote:
> Use EXT2_MIN_BLOCK_SIZE/JFS_MIN_JOURNAL_BLOCKS instead of hardcoded 1024
> when it is okay, and also add a helper ext2fs_journal_sb_start() that
> will return start of journal sb with special case for fs with 1k block
> size.

Seems like a good idea, but an issue below.

> diff --git a/lib/ext2fs/mkjournal.c b/lib/ext2fs/mkjournal.c
> index 884d9c0..068eed7 100644
> --- a/lib/ext2fs/mkjournal.c
> +++ b/lib/ext2fs/mkjournal.c
> @@ -75,10 +75,7 @@ errcode_t ext2fs_create_journal_superblock(ext2_filsys fs,
> 	if (fs->super->s_feature_incompat &
> 	    EXT3_FEATURE_INCOMPAT_JOURNAL_DEV) {
> 		jsb->s_nr_users = 0;
> -		if (fs->blocksize == 1024)
> -			jsb->s_first = htonl(3);
> -		else
> -			jsb->s_first = htonl(2);
> +		jsb->s_first = ext2fs_journal_sb_start(fs->blocksize) + 1;

This looks like it is missing the htonl() conversion, and will break the on-disk format?

The JBD code stores all data on-disk in big-endian to ensure that it is converted properly
on the most common little-endian systems, and will therefore also work on big-endian systems.

Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 1/3] journal: use consts instead of 1024 and add helper for journal with 1k blocksize

[Azat Khuzhin]

On Tue, Jul 08, 2014 at 05:30:10PM -0600, Andreas Dilger wrote:
> On Jul 8, 2014, at 2:41 PM, Azat Khuzhin <a3at.mail@gmail.com> wrote:
> > Use EXT2_MIN_BLOCK_SIZE/JFS_MIN_JOURNAL_BLOCKS instead of hardcoded 1024
> > when it is okay, and also add a helper ext2fs_journal_sb_start() that
> > will return start of journal sb with special case for fs with 1k block
> > size.
> 
> Seems like a good idea, but an issue below.
> 
> > diff --git a/lib/ext2fs/mkjournal.c b/lib/ext2fs/mkjournal.c
> > index 884d9c0..068eed7 100644
> > --- a/lib/ext2fs/mkjournal.c
> > +++ b/lib/ext2fs/mkjournal.c
> > @@ -75,10 +75,7 @@ errcode_t ext2fs_create_journal_superblock(ext2_filsys fs,
> > 	if (fs->super->s_feature_incompat &
> > 	    EXT3_FEATURE_INCOMPAT_JOURNAL_DEV) {
> > 		jsb->s_nr_users = 0;
> > -		if (fs->blocksize == 1024)
> > -			jsb->s_first = htonl(3);
> > -		else
> > -			jsb->s_first = htonl(2);
> > +		jsb->s_first = ext2fs_journal_sb_start(fs->blocksize) + 1;
> 
> This looks like it is missing the htonl() conversion, and will break the on-disk format?

Yeah, good catch, thanks!
I will resend this patch with htonl().

Cheers, Azat.

> 
> The JBD code stores all data on-disk in big-endian to ensure that it is converted properly
> on the most common little-endian systems, and will therefore also work on big-endian systems.
> 
> Cheers, Andreas
> 
> 
> 
> 
> 



-- 
Respectfully
Azat Khuzhin

[PATCH 1/3 v2] journal: use consts instead of 1024 and add helper for journal with 1k blocksize

[Azat Khuzhin]

Use EXT2_MIN_BLOCK_SIZE/JFS_MIN_JOURNAL_BLOCKS instead of hardcoded 1024
when it is okay, and also add a helper ext2fs_journal_sb_start() that
will return start of journal sb with special case for fs with 1k block
size.

Signed-off-by: Azat Khuzhin <a3at.mail@gmail.com>
---
v2: mixxed htonl() (thanks to Andreas Dilger)

 e2fsck/journal.c       |  5 ++---
 lib/ext2fs/ext2fs.h    |  1 +
 lib/ext2fs/mkjournal.c | 28 ++++++++++++++++------------
 misc/tune2fs.c         |  6 +++---
 4 files changed, 22 insertions(+), 18 deletions(-)

diff --git a/e2fsck/journal.c b/e2fsck/journal.c
index a7b1150..5780be0 100644
--- a/e2fsck/journal.c
+++ b/e2fsck/journal.c
@@ -443,8 +443,7 @@ static errcode_t e2fsck_get_journal(e2fsck_t ctx, journal_t **ret_journal)
 	if (ext_journal) {
 		blk64_t maxlen;
 
-		if (ctx->fs->blocksize == 1024)
-			start = 1;
+		start = ext2fs_journal_sb_start(ctx->fs->blocksize) - 1;
 		bh = getblk(dev_journal, start, ctx->fs->blocksize);
 		if (!bh) {
 			retval = EXT2_ET_NO_MEMORY;
@@ -455,7 +454,7 @@ static errcode_t e2fsck_get_journal(e2fsck_t ctx, journal_t **ret_journal)
 			brelse(bh);
 			goto errout;
 		}
-		memcpy(&jsuper, start ? bh->b_data :  bh->b_data + 1024,
+		memcpy(&jsuper, start ? bh->b_data :  bh->b_data + EXT2_MIN_BLOCK_SIZE,
 		       sizeof(jsuper));
 		brelse(bh);
 #ifdef WORDS_BIGENDIAN
diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h
index 599c972..a759741 100644
--- a/lib/ext2fs/ext2fs.h
+++ b/lib/ext2fs/ext2fs.h
@@ -1498,6 +1498,7 @@ extern errcode_t ext2fs_add_journal_inode(ext2_filsys fs, blk_t num_blocks,
 extern errcode_t ext2fs_add_journal_inode2(ext2_filsys fs, blk_t num_blocks,
 					   blk64_t goal, int flags);
 extern int ext2fs_default_journal_size(__u64 num_blocks);
+extern int ext2fs_journal_sb_start(int blocksize);
 
 /* openfs.c */
 extern errcode_t ext2fs_open(const char *name, int flags, int superblock,
diff --git a/lib/ext2fs/mkjournal.c b/lib/ext2fs/mkjournal.c
index 884d9c0..0ad674d 100644
--- a/lib/ext2fs/mkjournal.c
+++ b/lib/ext2fs/mkjournal.c
@@ -49,7 +49,7 @@ errcode_t ext2fs_create_journal_superblock(ext2_filsys fs,
 	errcode_t		retval;
 	journal_superblock_t	*jsb;
 
-	if (num_blocks < 1024)
+	if (num_blocks < JFS_MIN_JOURNAL_BLOCKS)
 		return EXT2_ET_JOURNAL_TOO_SMALL;
 
 	if ((retval = ext2fs_get_mem(fs->blocksize, &jsb)))
@@ -75,10 +75,7 @@ errcode_t ext2fs_create_journal_superblock(ext2_filsys fs,
 	if (fs->super->s_feature_incompat &
 	    EXT3_FEATURE_INCOMPAT_JOURNAL_DEV) {
 		jsb->s_nr_users = 0;
-		if (fs->blocksize == 1024)
-			jsb->s_first = htonl(3);
-		else
-			jsb->s_first = htonl(2);
+		jsb->s_first = htonl(ext2fs_journal_sb_start(fs->blocksize) + 1);
 	}
 
 	*ret_jsb = (char *) jsb;
@@ -430,6 +427,13 @@ int ext2fs_default_journal_size(__u64 num_blocks)
 	return 32768;
 }
 
+int ext2fs_journal_sb_start(int blocksize)
+{
+	if (blocksize == EXT2_MIN_BLOCK_SIZE)
+		return 2;
+	return 1;
+}
+
 /*
  * This function adds a journal device to a filesystem
  */
@@ -437,7 +441,7 @@ errcode_t ext2fs_add_journal_device(ext2_filsys fs, ext2_filsys journal_dev)
 {
 	struct stat	st;
 	errcode_t	retval;
-	char		buf[1024];
+	char		buf[SUPERBLOCK_SIZE];
 	journal_superblock_t	*jsb;
 	int		start;
 	__u32		i, nr_users;
@@ -450,10 +454,9 @@ errcode_t ext2fs_add_journal_device(ext2_filsys fs, ext2_filsys journal_dev)
 		return EXT2_ET_JOURNAL_NOT_BLOCK; /* Must be a block device */
 
 	/* Get the journal superblock */
-	start = 1;
-	if (journal_dev->blocksize == 1024)
-		start++;
-	if ((retval = io_channel_read_blk64(journal_dev->io, start, -1024,
+	start = ext2fs_journal_sb_start(journal_dev->blocksize);
+	if ((retval = io_channel_read_blk64(journal_dev->io, start,
+					    -SUPERBLOCK_SIZE,
 					    buf)))
 		return retval;
 
@@ -479,7 +482,8 @@ errcode_t ext2fs_add_journal_device(ext2_filsys fs, ext2_filsys journal_dev)
 	}
 
 	/* Writeback the journal superblock */
-	if ((retval = io_channel_write_blk64(journal_dev->io, start, -1024, buf)))
+	if ((retval = io_channel_write_blk64(journal_dev->io, start,
+					    -SUPERBLOCK_SIZE, buf)))
 		return retval;
 
 	fs->super->s_journal_inum = 0;
@@ -632,7 +636,7 @@ main(int argc, char **argv)
 		exit(1);
 	}
 
-	retval = ext2fs_add_journal_inode(fs, 1024, 0);
+	retval = ext2fs_add_journal_inode(fs, JFS_MIN_JOURNAL_BLOCKS, 0);
 	if (retval) {
 		com_err(argv[0], retval, "while adding journal to %s",
 			device_name);
diff --git a/misc/tune2fs.c b/misc/tune2fs.c
index d95cc3d..0e7caf2 100644
--- a/misc/tune2fs.c
+++ b/misc/tune2fs.c
@@ -187,7 +187,7 @@ static int remove_journal_device(ext2_filsys fs)
 {
 	char		*journal_path;
 	ext2_filsys	jfs;
-	char		buf[1024];
+	char		buf[SUPERBLOCK_SIZE];
 	journal_superblock_t	*jsb;
 	int		i, nr_users;
 	errcode_t	retval;
@@ -230,7 +230,7 @@ static int remove_journal_device(ext2_filsys fs)
 	}
 
 	/* Get the journal superblock */
-	if ((retval = io_channel_read_blk64(jfs->io, 1, -1024, buf))) {
+	if ((retval = io_channel_read_blk64(jfs->io, 1, -SUPERBLOCK_SIZE, buf))) {
 		com_err(program_name, retval, "%s",
 			_("while reading journal superblock"));
 		goto no_valid_journal;
@@ -261,7 +261,7 @@ static int remove_journal_device(ext2_filsys fs)
 	jsb->s_nr_users = htonl(nr_users);
 
 	/* Write back the journal superblock */
-	if ((retval = io_channel_write_blk64(jfs->io, 1, -1024, buf))) {
+	if ((retval = io_channel_write_blk64(jfs->io, 1, -SUPERBLOCK_SIZE, buf))) {
 		com_err(program_name, retval,
 			"while writing journal superblock.");
 		goto no_valid_journal;
-- 
2.0.0

Re: [PATCH 1/3 v2] journal: use consts instead of 1024 and add helper for journal with 1k blocksize

[Andreas Dilger]

The patch looks good, and you can add:

Reviewed-by: Andreas Dilger <adilger@dilger.ca>

One thing that could probably be fixed if this patch is refreshed,
or in a precursor patch is to rename SUPERBLOCK_SIZE to be
EXT2_SUPERBLOCK_SIZE since this avoids potential name clashes. 

Cheers, Andreas

> On Jul 8, 2014, at 21:56, Azat Khuzhin <a3at.mail@gmail.com> wrote:
> 
> Use EXT2_MIN_BLOCK_SIZE/JFS_MIN_JOURNAL_BLOCKS instead of hardcoded 1024
> when it is okay, and also add a helper ext2fs_journal_sb_start() that
> will return start of journal sb with special case for fs with 1k block
> size.
> 
> Signed-off-by: Azat Khuzhin <a3at.mail@gmail.com>
> ---
> v2: mixxed htonl() (thanks to Andreas Dilger)
> 
> e2fsck/journal.c       |  5 ++---
> lib/ext2fs/ext2fs.h    |  1 +
> lib/ext2fs/mkjournal.c | 28 ++++++++++++++++------------
> misc/tune2fs.c         |  6 +++---
> 4 files changed, 22 insertions(+), 18 deletions(-)
> 
> diff --git a/e2fsck/journal.c b/e2fsck/journal.c
> index a7b1150..5780be0 100644
> --- a/e2fsck/journal.c
> +++ b/e2fsck/journal.c
> @@ -443,8 +443,7 @@ static errcode_t e2fsck_get_journal(e2fsck_t ctx, journal_t **ret_journal)
>    if (ext_journal) {
>        blk64_t maxlen;
> 
> -        if (ctx->fs->blocksize == 1024)
> -            start = 1;
> +        start = ext2fs_journal_sb_start(ctx->fs->blocksize) - 1;
>        bh = getblk(dev_journal, start, ctx->fs->blocksize);
>        if (!bh) {
>            retval = EXT2_ET_NO_MEMORY;
> @@ -455,7 +454,7 @@ static errcode_t e2fsck_get_journal(e2fsck_t ctx, journal_t **ret_journal)
>            brelse(bh);
>            goto errout;
>        }
> -        memcpy(&jsuper, start ? bh->b_data :  bh->b_data + 1024,
> +        memcpy(&jsuper, start ? bh->b_data :  bh->b_data + EXT2_MIN_BLOCK_SIZE,
>               sizeof(jsuper));
>        brelse(bh);
> #ifdef WORDS_BIGENDIAN
> diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h
> index 599c972..a759741 100644
> --- a/lib/ext2fs/ext2fs.h
> +++ b/lib/ext2fs/ext2fs.h
> @@ -1498,6 +1498,7 @@ extern errcode_t ext2fs_add_journal_inode(ext2_filsys fs, blk_t num_blocks,
> extern errcode_t ext2fs_add_journal_inode2(ext2_filsys fs, blk_t num_blocks,
>                       blk64_t goal, int flags);
> extern int ext2fs_default_journal_size(__u64 num_blocks);
> +extern int ext2fs_journal_sb_start(int blocksize);
> 
> /* openfs.c */
> extern errcode_t ext2fs_open(const char *name, int flags, int superblock,
> diff --git a/lib/ext2fs/mkjournal.c b/lib/ext2fs/mkjournal.c
> index 884d9c0..0ad674d 100644
> --- a/lib/ext2fs/mkjournal.c
> +++ b/lib/ext2fs/mkjournal.c
> @@ -49,7 +49,7 @@ errcode_t ext2fs_create_journal_superblock(ext2_filsys fs,
>    errcode_t        retval;
>    journal_superblock_t    *jsb;
> 
> -    if (num_blocks < 1024)
> +    if (num_blocks < JFS_MIN_JOURNAL_BLOCKS)
>        return EXT2_ET_JOURNAL_TOO_SMALL;
> 
>    if ((retval = ext2fs_get_mem(fs->blocksize, &jsb)))
> @@ -75,10 +75,7 @@ errcode_t ext2fs_create_journal_superblock(ext2_filsys fs,
>    if (fs->super->s_feature_incompat &
>        EXT3_FEATURE_INCOMPAT_JOURNAL_DEV) {
>        jsb->s_nr_users = 0;
> -        if (fs->blocksize == 1024)
> -            jsb->s_first = htonl(3);
> -        else
> -            jsb->s_first = htonl(2);
> +        jsb->s_first = htonl(ext2fs_journal_sb_start(fs->blocksize) + 1);
>    }
> 
>    *ret_jsb = (char *) jsb;
> @@ -430,6 +427,13 @@ int ext2fs_default_journal_size(__u64 num_blocks)
>    return 32768;
> }
> 
> +int ext2fs_journal_sb_start(int blocksize)
> +{
> +    if (blocksize == EXT2_MIN_BLOCK_SIZE)
> +        return 2;
> +    return 1;
> +}
> +
> /*
>  * This function adds a journal device to a filesystem
>  */
> @@ -437,7 +441,7 @@ errcode_t ext2fs_add_journal_device(ext2_filsys fs, ext2_filsys journal_dev)
> {
>    struct stat    st;
>    errcode_t    retval;
> -    char        buf[1024];
> +    char        buf[SUPERBLOCK_SIZE];
>    journal_superblock_t    *jsb;
>    int        start;
>    __u32        i, nr_users;
> @@ -450,10 +454,9 @@ errcode_t ext2fs_add_journal_device(ext2_filsys fs, ext2_filsys journal_dev)
>        return EXT2_ET_JOURNAL_NOT_BLOCK; /* Must be a block device */
> 
>    /* Get the journal superblock */
> -    start = 1;
> -    if (journal_dev->blocksize == 1024)
> -        start++;
> -    if ((retval = io_channel_read_blk64(journal_dev->io, start, -1024,
> +    start = ext2fs_journal_sb_start(journal_dev->blocksize);
> +    if ((retval = io_channel_read_blk64(journal_dev->io, start,
> +                        -SUPERBLOCK_SIZE,
>                        buf)))
>        return retval;
> 
> @@ -479,7 +482,8 @@ errcode_t ext2fs_add_journal_device(ext2_filsys fs, ext2_filsys journal_dev)
>    }
> 
>    /* Writeback the journal superblock */
> -    if ((retval = io_channel_write_blk64(journal_dev->io, start, -1024, buf)))
> +    if ((retval = io_channel_write_blk64(journal_dev->io, start,
> +                        -SUPERBLOCK_SIZE, buf)))
>        return retval;
> 
>    fs->super->s_journal_inum = 0;
> @@ -632,7 +636,7 @@ main(int argc, char **argv)
>        exit(1);
>    }
> 
> -    retval = ext2fs_add_journal_inode(fs, 1024, 0);
> +    retval = ext2fs_add_journal_inode(fs, JFS_MIN_JOURNAL_BLOCKS, 0);
>    if (retval) {
>        com_err(argv[0], retval, "while adding journal to %s",
>            device_name);
> diff --git a/misc/tune2fs.c b/misc/tune2fs.c
> index d95cc3d..0e7caf2 100644
> --- a/misc/tune2fs.c
> +++ b/misc/tune2fs.c
> @@ -187,7 +187,7 @@ static int remove_journal_device(ext2_filsys fs)
> {
>    char        *journal_path;
>    ext2_filsys    jfs;
> -    char        buf[1024];
> +    char        buf[SUPERBLOCK_SIZE];
>    journal_superblock_t    *jsb;
>    int        i, nr_users;
>    errcode_t    retval;
> @@ -230,7 +230,7 @@ static int remove_journal_device(ext2_filsys fs)
>    }
> 
>    /* Get the journal superblock */
> -    if ((retval = io_channel_read_blk64(jfs->io, 1, -1024, buf))) {
> +    if ((retval = io_channel_read_blk64(jfs->io, 1, -SUPERBLOCK_SIZE, buf))) {
>        com_err(program_name, retval, "%s",
>            _("while reading journal superblock"));
>        goto no_valid_journal;
> @@ -261,7 +261,7 @@ static int remove_journal_device(ext2_filsys fs)
>    jsb->s_nr_users = htonl(nr_users);
> 
>    /* Write back the journal superblock */
> -    if ((retval = io_channel_write_blk64(jfs->io, 1, -1024, buf))) {
> +    if ((retval = io_channel_write_blk64(jfs->io, 1, -SUPERBLOCK_SIZE, buf))) {
>        com_err(program_name, retval,
>            "while writing journal superblock.");
>        goto no_valid_journal;
> -- 
> 2.0.0
> 

Re: [PATCH 1/3 v2] journal: use consts instead of 1024 and add helper for journal with 1k blocksize

[Azat Khuzhin]

On Tue, Jul 08, 2014 at 10:46:56PM -0600, Andreas Dilger wrote:
> The patch looks good, and you can add:
> 
> Reviewed-by: Andreas Dilger <adilger@dilger.ca>
> 
> One thing that could probably be fixed if this patch is refreshed,
> or in a precursor patch is to rename SUPERBLOCK_SIZE to be
> EXT2_SUPERBLOCK_SIZE since this avoids potential name clashes. 

I don't have any objections, but this change could break the
header-compatibily, no? (Since lib/ext2fs/ext2fs.h is public available).

Cheers, Azat.

> 
> Cheers, Andreas
> 
> > On Jul 8, 2014, at 21:56, Azat Khuzhin <a3at.mail@gmail.com> wrote:
> > 
> > Use EXT2_MIN_BLOCK_SIZE/JFS_MIN_JOURNAL_BLOCKS instead of hardcoded 1024
> > when it is okay, and also add a helper ext2fs_journal_sb_start() that
> > will return start of journal sb with special case for fs with 1k block
> > size.
> > 
> > Signed-off-by: Azat Khuzhin <a3at.mail@gmail.com>
> > ---
> > v2: mixxed htonl() (thanks to Andreas Dilger)
> > 
> > e2fsck/journal.c       |  5 ++---
> > lib/ext2fs/ext2fs.h    |  1 +
> > lib/ext2fs/mkjournal.c | 28 ++++++++++++++++------------
> > misc/tune2fs.c         |  6 +++---
> > 4 files changed, 22 insertions(+), 18 deletions(-)
> > 
> > diff --git a/e2fsck/journal.c b/e2fsck/journal.c
> > index a7b1150..5780be0 100644
> > --- a/e2fsck/journal.c
> > +++ b/e2fsck/journal.c
> > @@ -443,8 +443,7 @@ static errcode_t e2fsck_get_journal(e2fsck_t ctx, journal_t **ret_journal)
> >    if (ext_journal) {
> >        blk64_t maxlen;
> > 
> > -        if (ctx->fs->blocksize == 1024)
> > -            start = 1;
> > +        start = ext2fs_journal_sb_start(ctx->fs->blocksize) - 1;
> >        bh = getblk(dev_journal, start, ctx->fs->blocksize);
> >        if (!bh) {
> >            retval = EXT2_ET_NO_MEMORY;
> > @@ -455,7 +454,7 @@ static errcode_t e2fsck_get_journal(e2fsck_t ctx, journal_t **ret_journal)
> >            brelse(bh);
> >            goto errout;
> >        }
> > -        memcpy(&jsuper, start ? bh->b_data :  bh->b_data + 1024,
> > +        memcpy(&jsuper, start ? bh->b_data :  bh->b_data + EXT2_MIN_BLOCK_SIZE,
> >               sizeof(jsuper));
> >        brelse(bh);
> > #ifdef WORDS_BIGENDIAN
> > diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h
> > index 599c972..a759741 100644
> > --- a/lib/ext2fs/ext2fs.h
> > +++ b/lib/ext2fs/ext2fs.h
> > @@ -1498,6 +1498,7 @@ extern errcode_t ext2fs_add_journal_inode(ext2_filsys fs, blk_t num_blocks,
> > extern errcode_t ext2fs_add_journal_inode2(ext2_filsys fs, blk_t num_blocks,
> >                       blk64_t goal, int flags);
> > extern int ext2fs_default_journal_size(__u64 num_blocks);
> > +extern int ext2fs_journal_sb_start(int blocksize);
> > 
> > /* openfs.c */
> > extern errcode_t ext2fs_open(const char *name, int flags, int superblock,
> > diff --git a/lib/ext2fs/mkjournal.c b/lib/ext2fs/mkjournal.c
> > index 884d9c0..0ad674d 100644
> > --- a/lib/ext2fs/mkjournal.c
> > +++ b/lib/ext2fs/mkjournal.c
> > @@ -49,7 +49,7 @@ errcode_t ext2fs_create_journal_superblock(ext2_filsys fs,
> >    errcode_t        retval;
> >    journal_superblock_t    *jsb;
> > 
> > -    if (num_blocks < 1024)
> > +    if (num_blocks < JFS_MIN_JOURNAL_BLOCKS)
> >        return EXT2_ET_JOURNAL_TOO_SMALL;
> > 
> >    if ((retval = ext2fs_get_mem(fs->blocksize, &jsb)))
> > @@ -75,10 +75,7 @@ errcode_t ext2fs_create_journal_superblock(ext2_filsys fs,
> >    if (fs->super->s_feature_incompat &
> >        EXT3_FEATURE_INCOMPAT_JOURNAL_DEV) {
> >        jsb->s_nr_users = 0;
> > -        if (fs->blocksize == 1024)
> > -            jsb->s_first = htonl(3);
> > -        else
> > -            jsb->s_first = htonl(2);
> > +        jsb->s_first = htonl(ext2fs_journal_sb_start(fs->blocksize) + 1);
> >    }
> > 
> >    *ret_jsb = (char *) jsb;
> > @@ -430,6 +427,13 @@ int ext2fs_default_journal_size(__u64 num_blocks)
> >    return 32768;
> > }
> > 
> > +int ext2fs_journal_sb_start(int blocksize)
> > +{
> > +    if (blocksize == EXT2_MIN_BLOCK_SIZE)
> > +        return 2;
> > +    return 1;
> > +}
> > +
> > /*
> >  * This function adds a journal device to a filesystem
> >  */
> > @@ -437,7 +441,7 @@ errcode_t ext2fs_add_journal_device(ext2_filsys fs, ext2_filsys journal_dev)
> > {
> >    struct stat    st;
> >    errcode_t    retval;
> > -    char        buf[1024];
> > +    char        buf[SUPERBLOCK_SIZE];
> >    journal_superblock_t    *jsb;
> >    int        start;
> >    __u32        i, nr_users;
> > @@ -450,10 +454,9 @@ errcode_t ext2fs_add_journal_device(ext2_filsys fs, ext2_filsys journal_dev)
> >        return EXT2_ET_JOURNAL_NOT_BLOCK; /* Must be a block device */
> > 
> >    /* Get the journal superblock */
> > -    start = 1;
> > -    if (journal_dev->blocksize == 1024)
> > -        start++;
> > -    if ((retval = io_channel_read_blk64(journal_dev->io, start, -1024,
> > +    start = ext2fs_journal_sb_start(journal_dev->blocksize);
> > +    if ((retval = io_channel_read_blk64(journal_dev->io, start,
> > +                        -SUPERBLOCK_SIZE,
> >                        buf)))
> >        return retval;
> > 
> > @@ -479,7 +482,8 @@ errcode_t ext2fs_add_journal_device(ext2_filsys fs, ext2_filsys journal_dev)
> >    }
> > 
> >    /* Writeback the journal superblock */
> > -    if ((retval = io_channel_write_blk64(journal_dev->io, start, -1024, buf)))
> > +    if ((retval = io_channel_write_blk64(journal_dev->io, start,
> > +                        -SUPERBLOCK_SIZE, buf)))
> >        return retval;
> > 
> >    fs->super->s_journal_inum = 0;
> > @@ -632,7 +636,7 @@ main(int argc, char **argv)
> >        exit(1);
> >    }
> > 
> > -    retval = ext2fs_add_journal_inode(fs, 1024, 0);
> > +    retval = ext2fs_add_journal_inode(fs, JFS_MIN_JOURNAL_BLOCKS, 0);
> >    if (retval) {
> >        com_err(argv[0], retval, "while adding journal to %s",
> >            device_name);
> > diff --git a/misc/tune2fs.c b/misc/tune2fs.c
> > index d95cc3d..0e7caf2 100644
> > --- a/misc/tune2fs.c
> > +++ b/misc/tune2fs.c
> > @@ -187,7 +187,7 @@ static int remove_journal_device(ext2_filsys fs)
> > {
> >    char        *journal_path;
> >    ext2_filsys    jfs;
> > -    char        buf[1024];
> > +    char        buf[SUPERBLOCK_SIZE];
> >    journal_superblock_t    *jsb;
> >    int        i, nr_users;
> >    errcode_t    retval;
> > @@ -230,7 +230,7 @@ static int remove_journal_device(ext2_filsys fs)
> >    }
> > 
> >    /* Get the journal superblock */
> > -    if ((retval = io_channel_read_blk64(jfs->io, 1, -1024, buf))) {
> > +    if ((retval = io_channel_read_blk64(jfs->io, 1, -SUPERBLOCK_SIZE, buf))) {
> >        com_err(program_name, retval, "%s",
> >            _("while reading journal superblock"));
> >        goto no_valid_journal;
> > @@ -261,7 +261,7 @@ static int remove_journal_device(ext2_filsys fs)
> >    jsb->s_nr_users = htonl(nr_users);
> > 
> >    /* Write back the journal superblock */
> > -    if ((retval = io_channel_write_blk64(jfs->io, 1, -1024, buf))) {
> > +    if ((retval = io_channel_write_blk64(jfs->io, 1, -SUPERBLOCK_SIZE, buf))) {
> >        com_err(program_name, retval,
> >            "while writing journal superblock.");
> >        goto no_valid_journal;
> > -- 
> > 2.0.0
> > 

-- 
Respectfully
Azat Khuzhin

Re: [PATCH 1/3 v2] journal: use consts instead of 1024 and add helper for journal with 1k blocksize

[Darrick J. Wong]

On Wed, Jul 09, 2014 at 08:54:35AM +0400, Azat Khuzhin wrote:
> On Tue, Jul 08, 2014 at 10:46:56PM -0600, Andreas Dilger wrote:
> > The patch looks good, and you can add:
> > 
> > Reviewed-by: Andreas Dilger <adilger@dilger.ca>
> > 
> > One thing that could probably be fixed if this patch is refreshed,
> > or in a precursor patch is to rename SUPERBLOCK_SIZE to be
> > EXT2_SUPERBLOCK_SIZE since this avoids potential name clashes. 
> 
> I don't have any objections, but this change could break the
> header-compatibily, no? (Since lib/ext2fs/ext2fs.h is public available).

Perhaps something like this?

#define EXT2_SUPERBLOCK_SIZE	1024
#define SUPERBLOCK_SIZE		EXT2_SUPERBLOCK_SIZE

...and we can deprecate the old cpp symbol the next major release?

If anybody prepends EXT2_ to that symbol, please be sure to rename
SUPERBLOCK_OFFSET too.

> Cheers, Azat.
> 
> > 
> > Cheers, Andreas
> > 
> > > On Jul 8, 2014, at 21:56, Azat Khuzhin <a3at.mail@gmail.com> wrote:
> > > 
> > > Use EXT2_MIN_BLOCK_SIZE/JFS_MIN_JOURNAL_BLOCKS instead of hardcoded 1024
> > > when it is okay, and also add a helper ext2fs_journal_sb_start() that
> > > will return start of journal sb with special case for fs with 1k block
> > > size.
> > > 
> > > Signed-off-by: Azat Khuzhin <a3at.mail@gmail.com>
> > > ---
> > > v2: mixxed htonl() (thanks to Andreas Dilger)
> > > 
> > > e2fsck/journal.c       |  5 ++---
> > > lib/ext2fs/ext2fs.h    |  1 +
> > > lib/ext2fs/mkjournal.c | 28 ++++++++++++++++------------
> > > misc/tune2fs.c         |  6 +++---
> > > 4 files changed, 22 insertions(+), 18 deletions(-)
> > > 
> > > diff --git a/e2fsck/journal.c b/e2fsck/journal.c
> > > index a7b1150..5780be0 100644
> > > --- a/e2fsck/journal.c
> > > +++ b/e2fsck/journal.c
> > > @@ -443,8 +443,7 @@ static errcode_t e2fsck_get_journal(e2fsck_t ctx, journal_t **ret_journal)
> > >    if (ext_journal) {
> > >        blk64_t maxlen;
> > > 
> > > -        if (ctx->fs->blocksize == 1024)
> > > -            start = 1;
> > > +        start = ext2fs_journal_sb_start(ctx->fs->blocksize) - 1;
> > >        bh = getblk(dev_journal, start, ctx->fs->blocksize);
> > >        if (!bh) {
> > >            retval = EXT2_ET_NO_MEMORY;
> > > @@ -455,7 +454,7 @@ static errcode_t e2fsck_get_journal(e2fsck_t ctx, journal_t **ret_journal)
> > >            brelse(bh);
> > >            goto errout;
> > >        }
> > > -        memcpy(&jsuper, start ? bh->b_data :  bh->b_data + 1024,
> > > +        memcpy(&jsuper, start ? bh->b_data :  bh->b_data + EXT2_MIN_BLOCK_SIZE,

I ... think this 1024 looks more like an offset; did you mean SUPERBLOCK_OFFSET
here?

--D

> > >               sizeof(jsuper));
> > >        brelse(bh);
> > > #ifdef WORDS_BIGENDIAN
> > > diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h
> > > index 599c972..a759741 100644
> > > --- a/lib/ext2fs/ext2fs.h
> > > +++ b/lib/ext2fs/ext2fs.h
> > > @@ -1498,6 +1498,7 @@ extern errcode_t ext2fs_add_journal_inode(ext2_filsys fs, blk_t num_blocks,
> > > extern errcode_t ext2fs_add_journal_inode2(ext2_filsys fs, blk_t num_blocks,
> > >                       blk64_t goal, int flags);
> > > extern int ext2fs_default_journal_size(__u64 num_blocks);
> > > +extern int ext2fs_journal_sb_start(int blocksize);
> > > 
> > > /* openfs.c */
> > > extern errcode_t ext2fs_open(const char *name, int flags, int superblock,
> > > diff --git a/lib/ext2fs/mkjournal.c b/lib/ext2fs/mkjournal.c
> > > index 884d9c0..0ad674d 100644
> > > --- a/lib/ext2fs/mkjournal.c
> > > +++ b/lib/ext2fs/mkjournal.c
> > > @@ -49,7 +49,7 @@ errcode_t ext2fs_create_journal_superblock(ext2_filsys fs,
> > >    errcode_t        retval;
> > >    journal_superblock_t    *jsb;
> > > 
> > > -    if (num_blocks < 1024)
> > > +    if (num_blocks < JFS_MIN_JOURNAL_BLOCKS)
> > >        return EXT2_ET_JOURNAL_TOO_SMALL;
> > > 
> > >    if ((retval = ext2fs_get_mem(fs->blocksize, &jsb)))
> > > @@ -75,10 +75,7 @@ errcode_t ext2fs_create_journal_superblock(ext2_filsys fs,
> > >    if (fs->super->s_feature_incompat &
> > >        EXT3_FEATURE_INCOMPAT_JOURNAL_DEV) {
> > >        jsb->s_nr_users = 0;
> > > -        if (fs->blocksize == 1024)
> > > -            jsb->s_first = htonl(3);
> > > -        else
> > > -            jsb->s_first = htonl(2);
> > > +        jsb->s_first = htonl(ext2fs_journal_sb_start(fs->blocksize) + 1);
> > >    }
> > > 
> > >    *ret_jsb = (char *) jsb;
> > > @@ -430,6 +427,13 @@ int ext2fs_default_journal_size(__u64 num_blocks)
> > >    return 32768;
> > > }
> > > 
> > > +int ext2fs_journal_sb_start(int blocksize)
> > > +{
> > > +    if (blocksize == EXT2_MIN_BLOCK_SIZE)
> > > +        return 2;
> > > +    return 1;
> > > +}
> > > +
> > > /*
> > >  * This function adds a journal device to a filesystem
> > >  */
> > > @@ -437,7 +441,7 @@ errcode_t ext2fs_add_journal_device(ext2_filsys fs, ext2_filsys journal_dev)
> > > {
> > >    struct stat    st;
> > >    errcode_t    retval;
> > > -    char        buf[1024];
> > > +    char        buf[SUPERBLOCK_SIZE];
> > >    journal_superblock_t    *jsb;
> > >    int        start;
> > >    __u32        i, nr_users;
> > > @@ -450,10 +454,9 @@ errcode_t ext2fs_add_journal_device(ext2_filsys fs, ext2_filsys journal_dev)
> > >        return EXT2_ET_JOURNAL_NOT_BLOCK; /* Must be a block device */
> > > 
> > >    /* Get the journal superblock */
> > > -    start = 1;
> > > -    if (journal_dev->blocksize == 1024)
> > > -        start++;
> > > -    if ((retval = io_channel_read_blk64(journal_dev->io, start, -1024,
> > > +    start = ext2fs_journal_sb_start(journal_dev->blocksize);
> > > +    if ((retval = io_channel_read_blk64(journal_dev->io, start,
> > > +                        -SUPERBLOCK_SIZE,
> > >                        buf)))
> > >        return retval;
> > > 
> > > @@ -479,7 +482,8 @@ errcode_t ext2fs_add_journal_device(ext2_filsys fs, ext2_filsys journal_dev)
> > >    }
> > > 
> > >    /* Writeback the journal superblock */
> > > -    if ((retval = io_channel_write_blk64(journal_dev->io, start, -1024, buf)))
> > > +    if ((retval = io_channel_write_blk64(journal_dev->io, start,
> > > +                        -SUPERBLOCK_SIZE, buf)))
> > >        return retval;
> > > 
> > >    fs->super->s_journal_inum = 0;
> > > @@ -632,7 +636,7 @@ main(int argc, char **argv)
> > >        exit(1);
> > >    }
> > > 
> > > -    retval = ext2fs_add_journal_inode(fs, 1024, 0);
> > > +    retval = ext2fs_add_journal_inode(fs, JFS_MIN_JOURNAL_BLOCKS, 0);
> > >    if (retval) {
> > >        com_err(argv[0], retval, "while adding journal to %s",
> > >            device_name);
> > > diff --git a/misc/tune2fs.c b/misc/tune2fs.c
> > > index d95cc3d..0e7caf2 100644
> > > --- a/misc/tune2fs.c
> > > +++ b/misc/tune2fs.c
> > > @@ -187,7 +187,7 @@ static int remove_journal_device(ext2_filsys fs)
> > > {
> > >    char        *journal_path;
> > >    ext2_filsys    jfs;
> > > -    char        buf[1024];
> > > +    char        buf[SUPERBLOCK_SIZE];
> > >    journal_superblock_t    *jsb;
> > >    int        i, nr_users;
> > >    errcode_t    retval;
> > > @@ -230,7 +230,7 @@ static int remove_journal_device(ext2_filsys fs)
> > >    }
> > > 
> > >    /* Get the journal superblock */
> > > -    if ((retval = io_channel_read_blk64(jfs->io, 1, -1024, buf))) {
> > > +    if ((retval = io_channel_read_blk64(jfs->io, 1, -SUPERBLOCK_SIZE, buf))) {
> > >        com_err(program_name, retval, "%s",
> > >            _("while reading journal superblock"));
> > >        goto no_valid_journal;
> > > @@ -261,7 +261,7 @@ static int remove_journal_device(ext2_filsys fs)
> > >    jsb->s_nr_users = htonl(nr_users);
> > > 
> > >    /* Write back the journal superblock */
> > > -    if ((retval = io_channel_write_blk64(jfs->io, 1, -1024, buf))) {
> > > +    if ((retval = io_channel_write_blk64(jfs->io, 1, -SUPERBLOCK_SIZE, buf))) {
> > >        com_err(program_name, retval,
> > >            "while writing journal superblock.");
> > >        goto no_valid_journal;
> > > -- 
> > > 2.0.0
> > > 
> 
> -- 
> Respectfully
> Azat Khuzhin
> --
> 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 1/3 v2] journal: use consts instead of 1024 and add helper for journal with 1k blocksize

[Azat Khuzhin]

On Wed, Jul 09, 2014 at 11:57:42AM -0700, Darrick J. Wong wrote:
> On Wed, Jul 09, 2014 at 08:54:35AM +0400, Azat Khuzhin wrote:
> > On Tue, Jul 08, 2014 at 10:46:56PM -0600, Andreas Dilger wrote:
> > > The patch looks good, and you can add:
> > > 
> > > Reviewed-by: Andreas Dilger <adilger@dilger.ca>
> > > 
> > > One thing that could probably be fixed if this patch is refreshed,
> > > or in a precursor patch is to rename SUPERBLOCK_SIZE to be
> > > EXT2_SUPERBLOCK_SIZE since this avoids potential name clashes. 
> > 
> > I don't have any objections, but this change could break the
> > header-compatibily, no? (Since lib/ext2fs/ext2fs.h is public available).
> 
> Perhaps something like this?
> 
> #define EXT2_SUPERBLOCK_SIZE	1024
> #define SUPERBLOCK_SIZE		EXT2_SUPERBLOCK_SIZE

Hi Darrick, this should works, thanks.
I think that this will be the separate patch.

> 
> ...and we can deprecate the old cpp symbol the next major release?
> 
> If anybody prepends EXT2_ to that symbol, please be sure to rename
> SUPERBLOCK_OFFSET too.
> 
> > Cheers, Azat.
> > 
> > > 
> > > Cheers, Andreas
> > > 
> > > > On Jul 8, 2014, at 21:56, Azat Khuzhin <a3at.mail@gmail.com> wrote:
> > > > 
> > > > Use EXT2_MIN_BLOCK_SIZE/JFS_MIN_JOURNAL_BLOCKS instead of hardcoded 1024
> > > > when it is okay, and also add a helper ext2fs_journal_sb_start() that
> > > > will return start of journal sb with special case for fs with 1k block
> > > > size.
> > > > 
> > > > Signed-off-by: Azat Khuzhin <a3at.mail@gmail.com>
> > > > ---
> > > > v2: mixxed htonl() (thanks to Andreas Dilger)
> > > > 
> > > > e2fsck/journal.c       |  5 ++---
> > > > lib/ext2fs/ext2fs.h    |  1 +
> > > > lib/ext2fs/mkjournal.c | 28 ++++++++++++++++------------
> > > > misc/tune2fs.c         |  6 +++---
> > > > 4 files changed, 22 insertions(+), 18 deletions(-)
> > > > 
> > > > diff --git a/e2fsck/journal.c b/e2fsck/journal.c
> > > > index a7b1150..5780be0 100644
> > > > --- a/e2fsck/journal.c
> > > > +++ b/e2fsck/journal.c
> > > > @@ -443,8 +443,7 @@ static errcode_t e2fsck_get_journal(e2fsck_t ctx, journal_t **ret_journal)
> > > >    if (ext_journal) {
> > > >        blk64_t maxlen;
> > > > 
> > > > -        if (ctx->fs->blocksize == 1024)
> > > > -            start = 1;
> > > > +        start = ext2fs_journal_sb_start(ctx->fs->blocksize) - 1;
> > > >        bh = getblk(dev_journal, start, ctx->fs->blocksize);
> > > >        if (!bh) {
> > > >            retval = EXT2_ET_NO_MEMORY;
> > > > @@ -455,7 +454,7 @@ static errcode_t e2fsck_get_journal(e2fsck_t ctx, journal_t **ret_journal)
> > > >            brelse(bh);
> > > >            goto errout;
> > > >        }
> > > > -        memcpy(&jsuper, start ? bh->b_data :  bh->b_data + 1024,
> > > > +        memcpy(&jsuper, start ? bh->b_data :  bh->b_data + EXT2_MIN_BLOCK_SIZE,
> 
> I ... think this 1024 looks more like an offset; did you mean SUPERBLOCK_OFFSET
> here?

Yeah, good catch, thanks.
I will resend updated patch shortly.

> 
> --D
> 
> > > >               sizeof(jsuper));
> > > >        brelse(bh);
> > > > #ifdef WORDS_BIGENDIAN
> > > > diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h
> > > > index 599c972..a759741 100644
> > > > --- a/lib/ext2fs/ext2fs.h
> > > > +++ b/lib/ext2fs/ext2fs.h
> > > > @@ -1498,6 +1498,7 @@ extern errcode_t ext2fs_add_journal_inode(ext2_filsys fs, blk_t num_blocks,
> > > > extern errcode_t ext2fs_add_journal_inode2(ext2_filsys fs, blk_t num_blocks,
> > > >                       blk64_t goal, int flags);
> > > > extern int ext2fs_default_journal_size(__u64 num_blocks);
> > > > +extern int ext2fs_journal_sb_start(int blocksize);
> > > > 
> > > > /* openfs.c */
> > > > extern errcode_t ext2fs_open(const char *name, int flags, int superblock,
> > > > diff --git a/lib/ext2fs/mkjournal.c b/lib/ext2fs/mkjournal.c
> > > > index 884d9c0..0ad674d 100644
> > > > --- a/lib/ext2fs/mkjournal.c
> > > > +++ b/lib/ext2fs/mkjournal.c
> > > > @@ -49,7 +49,7 @@ errcode_t ext2fs_create_journal_superblock(ext2_filsys fs,
> > > >    errcode_t        retval;
> > > >    journal_superblock_t    *jsb;
> > > > 
> > > > -    if (num_blocks < 1024)
> > > > +    if (num_blocks < JFS_MIN_JOURNAL_BLOCKS)
> > > >        return EXT2_ET_JOURNAL_TOO_SMALL;
> > > > 
> > > >    if ((retval = ext2fs_get_mem(fs->blocksize, &jsb)))
> > > > @@ -75,10 +75,7 @@ errcode_t ext2fs_create_journal_superblock(ext2_filsys fs,
> > > >    if (fs->super->s_feature_incompat &
> > > >        EXT3_FEATURE_INCOMPAT_JOURNAL_DEV) {
> > > >        jsb->s_nr_users = 0;
> > > > -        if (fs->blocksize == 1024)
> > > > -            jsb->s_first = htonl(3);
> > > > -        else
> > > > -            jsb->s_first = htonl(2);
> > > > +        jsb->s_first = htonl(ext2fs_journal_sb_start(fs->blocksize) + 1);
> > > >    }
> > > > 
> > > >    *ret_jsb = (char *) jsb;
> > > > @@ -430,6 +427,13 @@ int ext2fs_default_journal_size(__u64 num_blocks)
> > > >    return 32768;
> > > > }
> > > > 
> > > > +int ext2fs_journal_sb_start(int blocksize)
> > > > +{
> > > > +    if (blocksize == EXT2_MIN_BLOCK_SIZE)
> > > > +        return 2;
> > > > +    return 1;
> > > > +}
> > > > +
> > > > /*
> > > >  * This function adds a journal device to a filesystem
> > > >  */
> > > > @@ -437,7 +441,7 @@ errcode_t ext2fs_add_journal_device(ext2_filsys fs, ext2_filsys journal_dev)
> > > > {
> > > >    struct stat    st;
> > > >    errcode_t    retval;
> > > > -    char        buf[1024];
> > > > +    char        buf[SUPERBLOCK_SIZE];
> > > >    journal_superblock_t    *jsb;
> > > >    int        start;
> > > >    __u32        i, nr_users;
> > > > @@ -450,10 +454,9 @@ errcode_t ext2fs_add_journal_device(ext2_filsys fs, ext2_filsys journal_dev)
> > > >        return EXT2_ET_JOURNAL_NOT_BLOCK; /* Must be a block device */
> > > > 
> > > >    /* Get the journal superblock */
> > > > -    start = 1;
> > > > -    if (journal_dev->blocksize == 1024)
> > > > -        start++;
> > > > -    if ((retval = io_channel_read_blk64(journal_dev->io, start, -1024,
> > > > +    start = ext2fs_journal_sb_start(journal_dev->blocksize);
> > > > +    if ((retval = io_channel_read_blk64(journal_dev->io, start,
> > > > +                        -SUPERBLOCK_SIZE,
> > > >                        buf)))
> > > >        return retval;
> > > > 
> > > > @@ -479,7 +482,8 @@ errcode_t ext2fs_add_journal_device(ext2_filsys fs, ext2_filsys journal_dev)
> > > >    }
> > > > 
> > > >    /* Writeback the journal superblock */
> > > > -    if ((retval = io_channel_write_blk64(journal_dev->io, start, -1024, buf)))
> > > > +    if ((retval = io_channel_write_blk64(journal_dev->io, start,
> > > > +                        -SUPERBLOCK_SIZE, buf)))
> > > >        return retval;
> > > > 
> > > >    fs->super->s_journal_inum = 0;
> > > > @@ -632,7 +636,7 @@ main(int argc, char **argv)
> > > >        exit(1);
> > > >    }
> > > > 
> > > > -    retval = ext2fs_add_journal_inode(fs, 1024, 0);
> > > > +    retval = ext2fs_add_journal_inode(fs, JFS_MIN_JOURNAL_BLOCKS, 0);
> > > >    if (retval) {
> > > >        com_err(argv[0], retval, "while adding journal to %s",
> > > >            device_name);
> > > > diff --git a/misc/tune2fs.c b/misc/tune2fs.c
> > > > index d95cc3d..0e7caf2 100644
> > > > --- a/misc/tune2fs.c
> > > > +++ b/misc/tune2fs.c
> > > > @@ -187,7 +187,7 @@ static int remove_journal_device(ext2_filsys fs)
> > > > {
> > > >    char        *journal_path;
> > > >    ext2_filsys    jfs;
> > > > -    char        buf[1024];
> > > > +    char        buf[SUPERBLOCK_SIZE];
> > > >    journal_superblock_t    *jsb;
> > > >    int        i, nr_users;
> > > >    errcode_t    retval;
> > > > @@ -230,7 +230,7 @@ static int remove_journal_device(ext2_filsys fs)
> > > >    }
> > > > 
> > > >    /* Get the journal superblock */
> > > > -    if ((retval = io_channel_read_blk64(jfs->io, 1, -1024, buf))) {
> > > > +    if ((retval = io_channel_read_blk64(jfs->io, 1, -SUPERBLOCK_SIZE, buf))) {
> > > >        com_err(program_name, retval, "%s",
> > > >            _("while reading journal superblock"));
> > > >        goto no_valid_journal;
> > > > @@ -261,7 +261,7 @@ static int remove_journal_device(ext2_filsys fs)
> > > >    jsb->s_nr_users = htonl(nr_users);
> > > > 
> > > >    /* Write back the journal superblock */
> > > > -    if ((retval = io_channel_write_blk64(jfs->io, 1, -1024, buf))) {
> > > > +    if ((retval = io_channel_write_blk64(jfs->io, 1, -SUPERBLOCK_SIZE, buf))) {
> > > >        com_err(program_name, retval,
> > > >            "while writing journal superblock.");
> > > >        goto no_valid_journal;
> > > > -- 
> > > > 2.0.0
> > > > 
> > 
> > -- 
> > Respectfully
> > Azat Khuzhin
> > --
> > 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

-- 
Respectfully
Azat Khuzhin

[PATCH 1/3 v3] journal: use consts instead of 1024 and add helper for journal with 1k blocksize

[Azat Khuzhin]

Use EXT2_MIN_BLOCK_SIZE/JFS_MIN_JOURNAL_BLOCKS instead of hardcoded 1024
when it is okay, and also add a helper ext2fs_journal_sb_start() that
will return start of journal sb with special case for fs with 1k block
size.

Signed-off-by: Azat Khuzhin <a3at.mail@gmail.com>
---
v2: missed htonl() (thanks to Andreas Dilger)
v3: use SUPERBLOCK_OFFSET instead of EXT2_MIN_BLOCK_SIZE (thanks to Darrick J.
Wong)

 e2fsck/journal.c       |  5 ++---
 lib/ext2fs/ext2fs.h    |  1 +
 lib/ext2fs/mkjournal.c | 28 ++++++++++++++++------------
 misc/tune2fs.c         |  6 +++---
 4 files changed, 22 insertions(+), 18 deletions(-)

diff --git a/e2fsck/journal.c b/e2fsck/journal.c
index a7b1150..6df0165 100644
--- a/e2fsck/journal.c
+++ b/e2fsck/journal.c
@@ -443,8 +443,7 @@ static errcode_t e2fsck_get_journal(e2fsck_t ctx, journal_t **ret_journal)
 	if (ext_journal) {
 		blk64_t maxlen;
 
-		if (ctx->fs->blocksize == 1024)
-			start = 1;
+		start = ext2fs_journal_sb_start(ctx->fs->blocksize) - 1;
 		bh = getblk(dev_journal, start, ctx->fs->blocksize);
 		if (!bh) {
 			retval = EXT2_ET_NO_MEMORY;
@@ -455,7 +454,7 @@ static errcode_t e2fsck_get_journal(e2fsck_t ctx, journal_t **ret_journal)
 			brelse(bh);
 			goto errout;
 		}
-		memcpy(&jsuper, start ? bh->b_data :  bh->b_data + 1024,
+		memcpy(&jsuper, start ? bh->b_data :  bh->b_data + SUPERBLOCK_OFFSET,
 		       sizeof(jsuper));
 		brelse(bh);
 #ifdef WORDS_BIGENDIAN
diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h
index 599c972..a759741 100644
--- a/lib/ext2fs/ext2fs.h
+++ b/lib/ext2fs/ext2fs.h
@@ -1498,6 +1498,7 @@ extern errcode_t ext2fs_add_journal_inode(ext2_filsys fs, blk_t num_blocks,
 extern errcode_t ext2fs_add_journal_inode2(ext2_filsys fs, blk_t num_blocks,
 					   blk64_t goal, int flags);
 extern int ext2fs_default_journal_size(__u64 num_blocks);
+extern int ext2fs_journal_sb_start(int blocksize);
 
 /* openfs.c */
 extern errcode_t ext2fs_open(const char *name, int flags, int superblock,
diff --git a/lib/ext2fs/mkjournal.c b/lib/ext2fs/mkjournal.c
index 884d9c0..0ad674d 100644
--- a/lib/ext2fs/mkjournal.c
+++ b/lib/ext2fs/mkjournal.c
@@ -49,7 +49,7 @@ errcode_t ext2fs_create_journal_superblock(ext2_filsys fs,
 	errcode_t		retval;
 	journal_superblock_t	*jsb;
 
-	if (num_blocks < 1024)
+	if (num_blocks < JFS_MIN_JOURNAL_BLOCKS)
 		return EXT2_ET_JOURNAL_TOO_SMALL;
 
 	if ((retval = ext2fs_get_mem(fs->blocksize, &jsb)))
@@ -75,10 +75,7 @@ errcode_t ext2fs_create_journal_superblock(ext2_filsys fs,
 	if (fs->super->s_feature_incompat &
 	    EXT3_FEATURE_INCOMPAT_JOURNAL_DEV) {
 		jsb->s_nr_users = 0;
-		if (fs->blocksize == 1024)
-			jsb->s_first = htonl(3);
-		else
-			jsb->s_first = htonl(2);
+		jsb->s_first = htonl(ext2fs_journal_sb_start(fs->blocksize) + 1);
 	}
 
 	*ret_jsb = (char *) jsb;
@@ -430,6 +427,13 @@ int ext2fs_default_journal_size(__u64 num_blocks)
 	return 32768;
 }
 
+int ext2fs_journal_sb_start(int blocksize)
+{
+	if (blocksize == EXT2_MIN_BLOCK_SIZE)
+		return 2;
+	return 1;
+}
+
 /*
  * This function adds a journal device to a filesystem
  */
@@ -437,7 +441,7 @@ errcode_t ext2fs_add_journal_device(ext2_filsys fs, ext2_filsys journal_dev)
 {
 	struct stat	st;
 	errcode_t	retval;
-	char		buf[1024];
+	char		buf[SUPERBLOCK_SIZE];
 	journal_superblock_t	*jsb;
 	int		start;
 	__u32		i, nr_users;
@@ -450,10 +454,9 @@ errcode_t ext2fs_add_journal_device(ext2_filsys fs, ext2_filsys journal_dev)
 		return EXT2_ET_JOURNAL_NOT_BLOCK; /* Must be a block device */
 
 	/* Get the journal superblock */
-	start = 1;
-	if (journal_dev->blocksize == 1024)
-		start++;
-	if ((retval = io_channel_read_blk64(journal_dev->io, start, -1024,
+	start = ext2fs_journal_sb_start(journal_dev->blocksize);
+	if ((retval = io_channel_read_blk64(journal_dev->io, start,
+					    -SUPERBLOCK_SIZE,
 					    buf)))
 		return retval;
 
@@ -479,7 +482,8 @@ errcode_t ext2fs_add_journal_device(ext2_filsys fs, ext2_filsys journal_dev)
 	}
 
 	/* Writeback the journal superblock */
-	if ((retval = io_channel_write_blk64(journal_dev->io, start, -1024, buf)))
+	if ((retval = io_channel_write_blk64(journal_dev->io, start,
+					    -SUPERBLOCK_SIZE, buf)))
 		return retval;
 
 	fs->super->s_journal_inum = 0;
@@ -632,7 +636,7 @@ main(int argc, char **argv)
 		exit(1);
 	}
 
-	retval = ext2fs_add_journal_inode(fs, 1024, 0);
+	retval = ext2fs_add_journal_inode(fs, JFS_MIN_JOURNAL_BLOCKS, 0);
 	if (retval) {
 		com_err(argv[0], retval, "while adding journal to %s",
 			device_name);
diff --git a/misc/tune2fs.c b/misc/tune2fs.c
index d95cc3d..0e7caf2 100644
--- a/misc/tune2fs.c
+++ b/misc/tune2fs.c
@@ -187,7 +187,7 @@ static int remove_journal_device(ext2_filsys fs)
 {
 	char		*journal_path;
 	ext2_filsys	jfs;
-	char		buf[1024];
+	char		buf[SUPERBLOCK_SIZE];
 	journal_superblock_t	*jsb;
 	int		i, nr_users;
 	errcode_t	retval;
@@ -230,7 +230,7 @@ static int remove_journal_device(ext2_filsys fs)
 	}
 
 	/* Get the journal superblock */
-	if ((retval = io_channel_read_blk64(jfs->io, 1, -1024, buf))) {
+	if ((retval = io_channel_read_blk64(jfs->io, 1, -SUPERBLOCK_SIZE, buf))) {
 		com_err(program_name, retval, "%s",
 			_("while reading journal superblock"));
 		goto no_valid_journal;
@@ -261,7 +261,7 @@ static int remove_journal_device(ext2_filsys fs)
 	jsb->s_nr_users = htonl(nr_users);
 
 	/* Write back the journal superblock */
-	if ((retval = io_channel_write_blk64(jfs->io, 1, -1024, buf))) {
+	if ((retval = io_channel_write_blk64(jfs->io, 1, -SUPERBLOCK_SIZE, buf))) {
 		com_err(program_name, retval,
 			"while writing journal superblock.");
 		goto no_valid_journal;
-- 
2.0.0

Re: [PATCH 1/3 v3] journal: use consts instead of 1024 and add helper for journal with 1k blocksize

[Andreas Dilger]

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


On Jul 9, 2014, at 1:39 PM, Azat Khuzhin <a3at.mail@gmail.com> wrote:

> Use EXT2_MIN_BLOCK_SIZE/JFS_MIN_JOURNAL_BLOCKS instead of hardcoded 1024
> when it is okay, and also add a helper ext2fs_journal_sb_start() that
> will return start of journal sb with special case for fs with 1k block
> size.
> 
> Signed-off-by: Azat Khuzhin <a3at.mail@gmail.com>

Reviewed-by: Andreas Dilger <adilger@dilger.ca>

> ---
> v2: missed htonl() (thanks to Andreas Dilger)
> v3: use SUPERBLOCK_OFFSET instead of EXT2_MIN_BLOCK_SIZE (thanks to Darrick J.
> Wong)
> 
> e2fsck/journal.c       |  5 ++---
> lib/ext2fs/ext2fs.h    |  1 +
> lib/ext2fs/mkjournal.c | 28 ++++++++++++++++------------
> misc/tune2fs.c         |  6 +++---
> 4 files changed, 22 insertions(+), 18 deletions(-)
> 
> diff --git a/e2fsck/journal.c b/e2fsck/journal.c
> index a7b1150..6df0165 100644
> --- a/e2fsck/journal.c
> +++ b/e2fsck/journal.c
> @@ -443,8 +443,7 @@ static errcode_t e2fsck_get_journal(e2fsck_t ctx, journal_t **ret_journal)
> 	if (ext_journal) {
> 		blk64_t maxlen;
> 
> -		if (ctx->fs->blocksize == 1024)
> -			start = 1;
> +		start = ext2fs_journal_sb_start(ctx->fs->blocksize) - 1;
> 		bh = getblk(dev_journal, start, ctx->fs->blocksize);
> 		if (!bh) {
> 			retval = EXT2_ET_NO_MEMORY;
> @@ -455,7 +454,7 @@ static errcode_t e2fsck_get_journal(e2fsck_t ctx, journal_t **ret_journal)
> 			brelse(bh);
> 			goto errout;
> 		}
> -		memcpy(&jsuper, start ? bh->b_data :  bh->b_data + 1024,
> +		memcpy(&jsuper, start ? bh->b_data :  bh->b_data + SUPERBLOCK_OFFSET,
> 		       sizeof(jsuper));
> 		brelse(bh);
> #ifdef WORDS_BIGENDIAN
> diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h
> index 599c972..a759741 100644
> --- a/lib/ext2fs/ext2fs.h
> +++ b/lib/ext2fs/ext2fs.h
> @@ -1498,6 +1498,7 @@ extern errcode_t ext2fs_add_journal_inode(ext2_filsys fs, blk_t num_blocks,
> extern errcode_t ext2fs_add_journal_inode2(ext2_filsys fs, blk_t num_blocks,
> 					   blk64_t goal, int flags);
> extern int ext2fs_default_journal_size(__u64 num_blocks);
> +extern int ext2fs_journal_sb_start(int blocksize);
> 
> /* openfs.c */
> extern errcode_t ext2fs_open(const char *name, int flags, int superblock,
> diff --git a/lib/ext2fs/mkjournal.c b/lib/ext2fs/mkjournal.c
> index 884d9c0..0ad674d 100644
> --- a/lib/ext2fs/mkjournal.c
> +++ b/lib/ext2fs/mkjournal.c
> @@ -49,7 +49,7 @@ errcode_t ext2fs_create_journal_superblock(ext2_filsys fs,
> 	errcode_t		retval;
> 	journal_superblock_t	*jsb;
> 
> -	if (num_blocks < 1024)
> +	if (num_blocks < JFS_MIN_JOURNAL_BLOCKS)
> 		return EXT2_ET_JOURNAL_TOO_SMALL;
> 
> 	if ((retval = ext2fs_get_mem(fs->blocksize, &jsb)))
> @@ -75,10 +75,7 @@ errcode_t ext2fs_create_journal_superblock(ext2_filsys fs,
> 	if (fs->super->s_feature_incompat &
> 	    EXT3_FEATURE_INCOMPAT_JOURNAL_DEV) {
> 		jsb->s_nr_users = 0;
> -		if (fs->blocksize == 1024)
> -			jsb->s_first = htonl(3);
> -		else
> -			jsb->s_first = htonl(2);
> +		jsb->s_first = htonl(ext2fs_journal_sb_start(fs->blocksize) + 1);
> 	}
> 
> 	*ret_jsb = (char *) jsb;
> @@ -430,6 +427,13 @@ int ext2fs_default_journal_size(__u64 num_blocks)
> 	return 32768;
> }
> 
> +int ext2fs_journal_sb_start(int blocksize)
> +{
> +	if (blocksize == EXT2_MIN_BLOCK_SIZE)
> +		return 2;
> +	return 1;
> +}
> +
> /*
>  * This function adds a journal device to a filesystem
>  */
> @@ -437,7 +441,7 @@ errcode_t ext2fs_add_journal_device(ext2_filsys fs, ext2_filsys journal_dev)
> {
> 	struct stat	st;
> 	errcode_t	retval;
> -	char		buf[1024];
> +	char		buf[SUPERBLOCK_SIZE];
> 	journal_superblock_t	*jsb;
> 	int		start;
> 	__u32		i, nr_users;
> @@ -450,10 +454,9 @@ errcode_t ext2fs_add_journal_device(ext2_filsys fs, ext2_filsys journal_dev)
> 		return EXT2_ET_JOURNAL_NOT_BLOCK; /* Must be a block device */
> 
> 	/* Get the journal superblock */
> -	start = 1;
> -	if (journal_dev->blocksize == 1024)
> -		start++;
> -	if ((retval = io_channel_read_blk64(journal_dev->io, start, -1024,
> +	start = ext2fs_journal_sb_start(journal_dev->blocksize);
> +	if ((retval = io_channel_read_blk64(journal_dev->io, start,
> +					    -SUPERBLOCK_SIZE,
> 					    buf)))
> 		return retval;
> 
> @@ -479,7 +482,8 @@ errcode_t ext2fs_add_journal_device(ext2_filsys fs, ext2_filsys journal_dev)
> 	}
> 
> 	/* Writeback the journal superblock */
> -	if ((retval = io_channel_write_blk64(journal_dev->io, start, -1024, buf)))
> +	if ((retval = io_channel_write_blk64(journal_dev->io, start,
> +					    -SUPERBLOCK_SIZE, buf)))
> 		return retval;
> 
> 	fs->super->s_journal_inum = 0;
> @@ -632,7 +636,7 @@ main(int argc, char **argv)
> 		exit(1);
> 	}
> 
> -	retval = ext2fs_add_journal_inode(fs, 1024, 0);
> +	retval = ext2fs_add_journal_inode(fs, JFS_MIN_JOURNAL_BLOCKS, 0);
> 	if (retval) {
> 		com_err(argv[0], retval, "while adding journal to %s",
> 			device_name);
> diff --git a/misc/tune2fs.c b/misc/tune2fs.c
> index d95cc3d..0e7caf2 100644
> --- a/misc/tune2fs.c
> +++ b/misc/tune2fs.c
> @@ -187,7 +187,7 @@ static int remove_journal_device(ext2_filsys fs)
> {
> 	char		*journal_path;
> 	ext2_filsys	jfs;
> -	char		buf[1024];
> +	char		buf[SUPERBLOCK_SIZE];
> 	journal_superblock_t	*jsb;
> 	int		i, nr_users;
> 	errcode_t	retval;
> @@ -230,7 +230,7 @@ static int remove_journal_device(ext2_filsys fs)
> 	}
> 
> 	/* Get the journal superblock */
> -	if ((retval = io_channel_read_blk64(jfs->io, 1, -1024, buf))) {
> +	if ((retval = io_channel_read_blk64(jfs->io, 1, -SUPERBLOCK_SIZE, buf))) {
> 		com_err(program_name, retval, "%s",
> 			_("while reading journal superblock"));
> 		goto no_valid_journal;
> @@ -261,7 +261,7 @@ static int remove_journal_device(ext2_filsys fs)
> 	jsb->s_nr_users = htonl(nr_users);
> 
> 	/* Write back the journal superblock */
> -	if ((retval = io_channel_write_blk64(jfs->io, 1, -1024, buf))) {
> +	if ((retval = io_channel_write_blk64(jfs->io, 1, -SUPERBLOCK_SIZE, buf))) {
> 		com_err(program_name, retval,
> 			"while writing journal superblock.");
> 		goto no_valid_journal;
> -- 
> 2.0.0
> 


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/3] tune2fs: remove_journal_device(): use the correct block to find jsb

[Azat Khuzhin]

Signed-off-by: Azat Khuzhin <a3at.mail@gmail.com>
---
 misc/tune2fs.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/misc/tune2fs.c b/misc/tune2fs.c
index 0e7caf2..811cb4d 100644
--- a/misc/tune2fs.c
+++ b/misc/tune2fs.c
@@ -193,6 +193,7 @@ static int remove_journal_device(ext2_filsys fs)
 	errcode_t	retval;
 	int		commit_remove_journal = 0;
 	io_manager	io_ptr;
+	int start;
 
 	if (f_flag)
 		commit_remove_journal = 1; /* force removal even if error */
@@ -229,8 +230,10 @@ static int remove_journal_device(ext2_filsys fs)
 		goto no_valid_journal;
 	}
 
+	start = ext2fs_journal_sb_start(fs->blocksize);
 	/* Get the journal superblock */
-	if ((retval = io_channel_read_blk64(jfs->io, 1, -SUPERBLOCK_SIZE, buf))) {
+	if ((retval = io_channel_read_blk64(jfs->io, start,
+	    -SUPERBLOCK_SIZE, buf))) {
 		com_err(program_name, retval, "%s",
 			_("while reading journal superblock"));
 		goto no_valid_journal;
@@ -261,7 +264,8 @@ static int remove_journal_device(ext2_filsys fs)
 	jsb->s_nr_users = htonl(nr_users);
 
 	/* Write back the journal superblock */
-	if ((retval = io_channel_write_blk64(jfs->io, 1, -SUPERBLOCK_SIZE, buf))) {
+	if ((retval = io_channel_write_blk64(jfs->io, start,
+	    -SUPERBLOCK_SIZE, buf))) {
 		com_err(program_name, retval,
 			"while writing journal superblock.");
 		goto no_valid_journal;
-- 
2.0.0

Re: [PATCH 2/3] tune2fs: remove_journal_device(): use the correct block to find jsb

[Andreas Dilger]

Looks good. You can add:

Reviewed-by: Andreas Dilger <adilger@dilger.ca>

Cheers, Andreas

> On Jul 8, 2014, at 14:41, Azat Khuzhin <a3at.mail@gmail.com> wrote:
> 
> Signed-off-by: Azat Khuzhin <a3at.mail@gmail.com>
> ---
> misc/tune2fs.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/misc/tune2fs.c b/misc/tune2fs.c
> index 0e7caf2..811cb4d 100644
> --- a/misc/tune2fs.c
> +++ b/misc/tune2fs.c
> @@ -193,6 +193,7 @@ static int remove_journal_device(ext2_filsys fs)
>    errcode_t    retval;
>    int        commit_remove_journal = 0;
>    io_manager    io_ptr;
> +    int start;
> 
>    if (f_flag)
>        commit_remove_journal = 1; /* force removal even if error */
> @@ -229,8 +230,10 @@ static int remove_journal_device(ext2_filsys fs)
>        goto no_valid_journal;
>    }
> 
> +    start = ext2fs_journal_sb_start(fs->blocksize);
>    /* Get the journal superblock */
> -    if ((retval = io_channel_read_blk64(jfs->io, 1, -SUPERBLOCK_SIZE, buf))) {
> +    if ((retval = io_channel_read_blk64(jfs->io, start,
> +        -SUPERBLOCK_SIZE, buf))) {
>        com_err(program_name, retval, "%s",
>            _("while reading journal superblock"));
>        goto no_valid_journal;
> @@ -261,7 +264,8 @@ static int remove_journal_device(ext2_filsys fs)
>    jsb->s_nr_users = htonl(nr_users);
> 
>    /* Write back the journal superblock */
> -    if ((retval = io_channel_write_blk64(jfs->io, 1, -SUPERBLOCK_SIZE, buf))) {
> +    if ((retval = io_channel_write_blk64(jfs->io, start,
> +        -SUPERBLOCK_SIZE, buf))) {
>        com_err(program_name, retval,
>            "while writing journal superblock.");
>        goto no_valid_journal;
> -- 
> 2.0.0
> 
> --
> 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

[PATCH 3/3] tune2fs: update journal super block when changing UUID for fs.

[Azat Khuzhin]

Using -U option you can change the UUID for fs, however it will not work
for journal device, since it have a copy of this UUID inside jsb (i.e.
journal super block). So copy UUID on change into that block.

Here is the initial thread:
http://comments.gmane.org/gmane.comp.file-systems.ext4/44532

You can reproduce this by executing following commands:
$ fallocate -l100M /tmp/dev
$ fallocate -l100M /tmp/journal
$ sudo /sbin/losetup /dev/loop1 /tmp/dev
$ sudo /sbin/losetup /dev/loop0 /tmp/journal
$ mke2fs -O journal_dev /tmp/journal
$ tune2fs -U da1f2ed0-60f6-aaaa-92fd-738701418523 /tmp/journal
$ sudo mke2fs -t ext4 -J device=/dev/loop0 /dev/loop1
$ dumpe2fs -h /tmp/dev | fgrep UUID
dumpe2fs 1.43-WIP (18-May-2014)
Filesystem UUID:          8a776be9-12eb-411f-8e88-b873575ecfb6
Journal UUID:             e3d02151-e776-4865-af25-aecb7291e8e5
$ sudo e2fsck /dev/vdc
e2fsck 1.43-WIP (18-May-2014)
External journal does not support this filesystem

/dev/loop1: ********** WARNING: Filesystem still has errors **********

Reported-by: Chin Tzung Cheng <chintzung@gmail.com>
Signed-off-by: Azat Khuzhin <a3at.mail@gmail.com>
---
 misc/tune2fs.c | 74 ++++++++++++++++++++++++++++++++++++++++++++--------------
 1 file changed, 56 insertions(+), 18 deletions(-)

diff --git a/misc/tune2fs.c b/misc/tune2fs.c
index 811cb4d..898a1b3 100644
--- a/misc/tune2fs.c
+++ b/misc/tune2fs.c
@@ -180,6 +180,38 @@ static __u32 clear_ok_features[3] = {
 		EXT4_FEATURE_RO_COMPAT_METADATA_CSUM
 };
 
+/**
+ * Try to get journal super block if any
+ */
+static int get_journal_sb(ext2_filsys jfs, char buf[SUPERBLOCK_SIZE])
+{
+	int retval;
+	int start;
+	journal_superblock_t *jsb;
+
+	if (!(jfs->super->s_feature_incompat &
+	    EXT3_FEATURE_INCOMPAT_JOURNAL_DEV)) {
+		return EXT2_ET_UNSUPP_FEATURE;
+	}
+
+	/* Get the journal superblock */
+	if ((retval = io_channel_read_blk64(jfs->io,
+	    ext2fs_journal_sb_start(jfs->blocksize), -SUPERBLOCK_SIZE, buf))) {
+		com_err(program_name, retval, "%s",
+		_("while reading journal superblock"));
+		return retval;
+	}
+
+	jsb = (journal_superblock_t *) buf;
+	if ((jsb->s_header.h_magic != (unsigned)ntohl(JFS_MAGIC_NUMBER)) ||
+	    (jsb->s_header.h_blocktype != (unsigned)ntohl(JFS_SUPERBLOCK_V2))) {
+		fputs(_("Journal superblock not found!\n"), stderr);
+		return EXT2_ET_BAD_MAGIC;
+	}
+
+	return 0;
+}
+
 /*
  * Remove an external journal from the filesystem
  */
@@ -223,29 +255,15 @@ static int remove_journal_device(ext2_filsys fs)
 			_("while trying to open external journal"));
 		goto no_valid_journal;
 	}
-	if (!(jfs->super->s_feature_incompat &
-	      EXT3_FEATURE_INCOMPAT_JOURNAL_DEV)) {
-		fprintf(stderr, _("%s is not a journal device.\n"),
-			journal_path);
-		goto no_valid_journal;
-	}
 
-	start = ext2fs_journal_sb_start(fs->blocksize);
-	/* Get the journal superblock */
-	if ((retval = io_channel_read_blk64(jfs->io, start,
-	    -SUPERBLOCK_SIZE, buf))) {
-		com_err(program_name, retval, "%s",
-			_("while reading journal superblock"));
+	if ((retval = get_journal_sb(jfs, buf))) {
+		if (retval == EXT2_ET_UNSUPP_FEATURE)
+			fprintf(stderr, _("%s is not a journal device.\n"),
+				journal_path);
 		goto no_valid_journal;
 	}
 
 	jsb = (journal_superblock_t *) buf;
-	if ((jsb->s_header.h_magic != (unsigned)ntohl(JFS_MAGIC_NUMBER)) ||
-	    (jsb->s_header.h_blocktype != (unsigned)ntohl(JFS_SUPERBLOCK_V2))) {
-		fputs(_("Journal superblock not found!\n"), stderr);
-		goto no_valid_journal;
-	}
-
 	/* Find the filesystem UUID */
 	nr_users = ntohl(jsb->s_nr_users);
 	for (i = 0; i < nr_users; i++) {
@@ -2695,6 +2713,7 @@ retry_open:
 	if (U_flag) {
 		int set_csum = 0;
 		dgrp_t i;
+		char buf[SUPERBLOCK_SIZE];
 
 		if (ext2fs_has_group_desc_csum(fs)) {
 			/*
@@ -2740,6 +2759,25 @@ retry_open:
 				ext2fs_group_desc_csum_set(fs, i);
 			fs->flags &= ~EXT2_FLAG_SUPER_ONLY;
 		}
+
+		/* If this is a journal dev, we need to copy UUID into jsb */
+		if (!(rc = get_journal_sb(fs, buf))) {
+			journal_superblock_t *jsb;
+
+			jsb = (journal_superblock_t *) buf;
+			fputs(_("Need to update journal superblock.\n"), stdout);
+			memcpy(jsb->s_uuid, sb->s_uuid, sizeof(sb->s_uuid));
+
+			/* Writeback the journal superblock */
+			if ((rc = io_channel_write_blk64(fs->io,
+				ext2fs_journal_sb_start(fs->blocksize),
+					-SUPERBLOCK_SIZE, buf)))
+				goto closefs;
+		} else if (rc != EXT2_ET_UNSUPP_FEATURE)
+			goto closefs;
+		else
+			rc = 0; /** Reset rc to avoid ext2fs_mmp_stop() */
+
 		ext2fs_mark_super_dirty(fs);
 		if (EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
 				EXT4_FEATURE_RO_COMPAT_METADATA_CSUM))
-- 
2.0.0

Re: [PATCH 3/3] tune2fs: update journal super block when changing UUID for fs.

[Andreas Dilger]

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


On Jul 8, 2014, at 2:41 PM, Azat Khuzhin <a3at.mail@gmail.com> wrote:

> Using -U option you can change the UUID for fs, however it will not work
> for journal device, since it have a copy of this UUID inside jsb (i.e.
> journal super block). So copy UUID on change into that block.
> 
> Here is the initial thread:
> http://comments.gmane.org/gmane.comp.file-systems.ext4/44532
> 
> You can reproduce this by executing following commands:
> $ fallocate -l100M /tmp/dev
> $ fallocate -l100M /tmp/journal
> $ sudo /sbin/losetup /dev/loop1 /tmp/dev
> $ sudo /sbin/losetup /dev/loop0 /tmp/journal
> $ mke2fs -O journal_dev /tmp/journal
> $ tune2fs -U da1f2ed0-60f6-aaaa-92fd-738701418523 /tmp/journal
> $ sudo mke2fs -t ext4 -J device=/dev/loop0 /dev/loop1
> $ dumpe2fs -h /tmp/dev | fgrep UUID
> dumpe2fs 1.43-WIP (18-May-2014)
> Filesystem UUID:          8a776be9-12eb-411f-8e88-b873575ecfb6
> Journal UUID:             e3d02151-e776-4865-af25-aecb7291e8e5
> $ sudo e2fsck /dev/vdc
> e2fsck 1.43-WIP (18-May-2014)
> External journal does not support this filesystem
> 
> /dev/loop1: ********** WARNING: Filesystem still has errors **********
> 
> Reported-by: Chin Tzung Cheng <chintzung@gmail.com>
> Signed-off-by: Azat Khuzhin <a3at.mail@gmail.com>

It may be that the reverse problem also exists - if the UUID is changed
on the filesystem, it probably is not updated in the journal "users" list?
I'm not sure either way, but it is worthwhile to check.

Reviewed-by: Andreas Dilger <adilger@dilger.ca>

> ---
> misc/tune2fs.c | 74 ++++++++++++++++++++++++++++++++++++++++++++--------------
> 1 file changed, 56 insertions(+), 18 deletions(-)
> 
> diff --git a/misc/tune2fs.c b/misc/tune2fs.c
> index 811cb4d..898a1b3 100644
> --- a/misc/tune2fs.c
> +++ b/misc/tune2fs.c
> @@ -180,6 +180,38 @@ static __u32 clear_ok_features[3] = {
> 		EXT4_FEATURE_RO_COMPAT_METADATA_CSUM
> };
> 
> +/**
> + * Try to get journal super block if any
> + */
> +static int get_journal_sb(ext2_filsys jfs, char buf[SUPERBLOCK_SIZE])
> +{
> +	int retval;
> +	int start;
> +	journal_superblock_t *jsb;
> +
> +	if (!(jfs->super->s_feature_incompat &
> +	    EXT3_FEATURE_INCOMPAT_JOURNAL_DEV)) {
> +		return EXT2_ET_UNSUPP_FEATURE;
> +	}
> +
> +	/* Get the journal superblock */
> +	if ((retval = io_channel_read_blk64(jfs->io,
> +	    ext2fs_journal_sb_start(jfs->blocksize), -SUPERBLOCK_SIZE, buf))) {
> +		com_err(program_name, retval, "%s",
> +		_("while reading journal superblock"));
> +		return retval;
> +	}
> +
> +	jsb = (journal_superblock_t *) buf;
> +	if ((jsb->s_header.h_magic != (unsigned)ntohl(JFS_MAGIC_NUMBER)) ||
> +	    (jsb->s_header.h_blocktype != (unsigned)ntohl(JFS_SUPERBLOCK_V2))) {
> +		fputs(_("Journal superblock not found!\n"), stderr);
> +		return EXT2_ET_BAD_MAGIC;
> +	}
> +
> +	return 0;
> +}
> +
> /*
>  * Remove an external journal from the filesystem
>  */
> @@ -223,29 +255,15 @@ static int remove_journal_device(ext2_filsys fs)
> 			_("while trying to open external journal"));
> 		goto no_valid_journal;
> 	}
> -	if (!(jfs->super->s_feature_incompat &
> -	      EXT3_FEATURE_INCOMPAT_JOURNAL_DEV)) {
> -		fprintf(stderr, _("%s is not a journal device.\n"),
> -			journal_path);
> -		goto no_valid_journal;
> -	}
> 
> -	start = ext2fs_journal_sb_start(fs->blocksize);
> -	/* Get the journal superblock */
> -	if ((retval = io_channel_read_blk64(jfs->io, start,
> -	    -SUPERBLOCK_SIZE, buf))) {
> -		com_err(program_name, retval, "%s",
> -			_("while reading journal superblock"));
> +	if ((retval = get_journal_sb(jfs, buf))) {
> +		if (retval == EXT2_ET_UNSUPP_FEATURE)
> +			fprintf(stderr, _("%s is not a journal device.\n"),
> +				journal_path);
> 		goto no_valid_journal;
> 	}
> 
> 	jsb = (journal_superblock_t *) buf;
> -	if ((jsb->s_header.h_magic != (unsigned)ntohl(JFS_MAGIC_NUMBER)) ||
> -	    (jsb->s_header.h_blocktype != (unsigned)ntohl(JFS_SUPERBLOCK_V2))) {
> -		fputs(_("Journal superblock not found!\n"), stderr);
> -		goto no_valid_journal;
> -	}
> -
> 	/* Find the filesystem UUID */
> 	nr_users = ntohl(jsb->s_nr_users);
> 	for (i = 0; i < nr_users; i++) {
> @@ -2695,6 +2713,7 @@ retry_open:
> 	if (U_flag) {
> 		int set_csum = 0;
> 		dgrp_t i;
> +		char buf[SUPERBLOCK_SIZE];
> 
> 		if (ext2fs_has_group_desc_csum(fs)) {
> 			/*
> @@ -2740,6 +2759,25 @@ retry_open:
> 				ext2fs_group_desc_csum_set(fs, i);
> 			fs->flags &= ~EXT2_FLAG_SUPER_ONLY;
> 		}
> +
> +		/* If this is a journal dev, we need to copy UUID into jsb */
> +		if (!(rc = get_journal_sb(fs, buf))) {
> +			journal_superblock_t *jsb;
> +
> +			jsb = (journal_superblock_t *) buf;
> +			fputs(_("Need to update journal superblock.\n"), stdout);
> +			memcpy(jsb->s_uuid, sb->s_uuid, sizeof(sb->s_uuid));
> +
> +			/* Writeback the journal superblock */
> +			if ((rc = io_channel_write_blk64(fs->io,
> +				ext2fs_journal_sb_start(fs->blocksize),
> +					-SUPERBLOCK_SIZE, buf)))
> +				goto closefs;
> +		} else if (rc != EXT2_ET_UNSUPP_FEATURE)
> +			goto closefs;
> +		else
> +			rc = 0; /** Reset rc to avoid ext2fs_mmp_stop() */
> +
> 		ext2fs_mark_super_dirty(fs);
> 		if (EXT2_HAS_RO_COMPAT_FEATURE(fs->super,
> 				EXT4_FEATURE_RO_COMPAT_METADATA_CSUM))
> -- 
> 2.0.0
> 
> --
> 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


Cheers, Andreas






[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 3/3] tune2fs: update journal super block when changing UUID for fs.

[Azat Khuzhin]

On Wed, Jul 09, 2014 at 02:38:19PM -0600, Andreas Dilger wrote:
> 
> On Jul 8, 2014, at 2:41 PM, Azat Khuzhin <a3at.mail@gmail.com> wrote:
> 
> > Using -U option you can change the UUID for fs, however it will not work
> > for journal device, since it have a copy of this UUID inside jsb (i.e.
> > journal super block). So copy UUID on change into that block.
> > 
> > Here is the initial thread:
> > http://comments.gmane.org/gmane.comp.file-systems.ext4/44532
> > 
> > You can reproduce this by executing following commands:
> > $ fallocate -l100M /tmp/dev
> > $ fallocate -l100M /tmp/journal
> > $ sudo /sbin/losetup /dev/loop1 /tmp/dev
> > $ sudo /sbin/losetup /dev/loop0 /tmp/journal
> > $ mke2fs -O journal_dev /tmp/journal
> > $ tune2fs -U da1f2ed0-60f6-aaaa-92fd-738701418523 /tmp/journal
> > $ sudo mke2fs -t ext4 -J device=/dev/loop0 /dev/loop1
> > $ dumpe2fs -h /tmp/dev | fgrep UUID
> > dumpe2fs 1.43-WIP (18-May-2014)
> > Filesystem UUID:          8a776be9-12eb-411f-8e88-b873575ecfb6
> > Journal UUID:             e3d02151-e776-4865-af25-aecb7291e8e5
> > $ sudo e2fsck /dev/vdc
> > e2fsck 1.43-WIP (18-May-2014)
> > External journal does not support this filesystem
> > 
> > /dev/loop1: ********** WARNING: Filesystem still has errors **********
> > 
> > Reported-by: Chin Tzung Cheng <chintzung@gmail.com>
> > Signed-off-by: Azat Khuzhin <a3at.mail@gmail.com>
> 
> It may be that the reverse problem also exists - if the UUID is changed
> on the filesystem, it probably is not updated in the journal "users" list?
> I'm not sure either way, but it is worthwhile to check.

You are right, it will not update the 'Journal users' field, there is no
code for U_flag in tune2fs for this (I also test it).
But to make patch clean I need more time, I will send it later.

> 
> Reviewed-by: Andreas Dilger <adilger@dilger.ca>

Thanks!


-- 
Respectfully
Azat Khuzhin