[PATCH 1/2] btrfs-progs: btrfs-convert: Prevent accounting blocks beyond end of device

[PATCH 1/2] btrfs-progs: btrfs-convert: Prevent accounting blocks beyond end of device

[Chandan Rajendra]

When looping across data block bitmap, __ext2_add_one_block() may add
blocks which do not exist on the underlying disk. This commit prevents
this from happening by checking the block index against the maximum
block count that was present in the ext4 filesystem instance that is
being converted.

Signed-off-by: Chandan Rajendra <chandan@linux.vnet.ibm.com>
---
 convert/main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/convert/main.c b/convert/main.c
index 4b4cea4..1148a36 100644
--- a/convert/main.c
+++ b/convert/main.c
@@ -1525,6 +1525,9 @@ static int __ext2_add_one_block(ext2_filsys fs, char *bitmap,
 	offset /= EXT2FS_CLUSTER_RATIO(fs);
 	offset += group_nr * EXT2_CLUSTERS_PER_GROUP(fs->super);
 	for (i = 0; i < EXT2_CLUSTERS_PER_GROUP(fs->super); i++) {
+		if ((i + offset) >= ext2fs_blocks_count(fs->super))
+			break;
+
 		if (ext2fs_test_bit(i, bitmap)) {
 			u64 start;
 
-- 
2.5.5

[PATCH 2/2] btrfs-convert: Fix migrate_super_block() to work with 64k sectorsize

[Chandan Rajendra]

migrate_super_block() uses sectorsize to refer to the size of the
superblock. Hence on 64k sectorsize filesystems, it ends up computing
checksum beyond the super block length (i.e.
BTRFS_SUPER_INFO_SIZE). This commit fixes the bug by using
BTRFS_SUPER_INFO_SIZE instead of sectorsize of the underlying
filesystem.

Signed-off-by: Chandan Rajendra <chandan@linux.vnet.ibm.com>
---
 convert/main.c | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/convert/main.c b/convert/main.c
index 1148a36..fd6f77b 100644
--- a/convert/main.c
+++ b/convert/main.c
@@ -1360,7 +1360,7 @@ err:
 /*
  * Migrate super block to its default position and zero 0 ~ 16k
  */
-static int migrate_super_block(int fd, u64 old_bytenr, u32 sectorsize)
+static int migrate_super_block(int fd, u64 old_bytenr)
 {
 	int ret;
 	struct extent_buffer *buf;
@@ -1368,13 +1368,13 @@ static int migrate_super_block(int fd, u64 old_bytenr, u32 sectorsize)
 	u32 len;
 	u32 bytenr;
 
-	buf = malloc(sizeof(*buf) + sectorsize);
+	buf = malloc(sizeof(*buf) + BTRFS_SUPER_INFO_SIZE);
 	if (!buf)
 		return -ENOMEM;
 
-	buf->len = sectorsize;
-	ret = pread(fd, buf->data, sectorsize, old_bytenr);
-	if (ret != sectorsize)
+	buf->len = BTRFS_SUPER_INFO_SIZE;
+	ret = pread(fd, buf->data, BTRFS_SUPER_INFO_SIZE, old_bytenr);
+	if (ret != BTRFS_SUPER_INFO_SIZE)
 		goto fail;
 
 	super = (struct btrfs_super_block *)buf->data;
@@ -1382,19 +1382,20 @@ static int migrate_super_block(int fd, u64 old_bytenr, u32 sectorsize)
 	btrfs_set_super_bytenr(super, BTRFS_SUPER_INFO_OFFSET);
 
 	csum_tree_block_size(buf, BTRFS_CRC32_SIZE, 0);
-	ret = pwrite(fd, buf->data, sectorsize, BTRFS_SUPER_INFO_OFFSET);
-	if (ret != sectorsize)
+	ret = pwrite(fd, buf->data, BTRFS_SUPER_INFO_SIZE,
+		BTRFS_SUPER_INFO_OFFSET);
+	if (ret != BTRFS_SUPER_INFO_SIZE)
 		goto fail;
 
 	ret = fsync(fd);
 	if (ret)
 		goto fail;
 
-	memset(buf->data, 0, sectorsize);
+	memset(buf->data, 0, BTRFS_SUPER_INFO_SIZE);
 	for (bytenr = 0; bytenr < BTRFS_SUPER_INFO_OFFSET; ) {
 		len = BTRFS_SUPER_INFO_OFFSET - bytenr;
-		if (len > sectorsize)
-			len = sectorsize;
+		if (len > BTRFS_SUPER_INFO_SIZE)
+			len = BTRFS_SUPER_INFO_SIZE;
 		ret = pwrite(fd, buf->data, len, bytenr);
 		if (ret != len) {
 			fprintf(stderr, "unable to zero fill device\n");
@@ -2519,7 +2520,7 @@ static int do_convert(const char *devname, int datacsum, int packing,
 	 * If this step succeed, we get a mountable btrfs. Otherwise
 	 * the source fs is left unchanged.
 	 */
-	ret = migrate_super_block(fd, mkfs_cfg.super_bytenr, blocksize);
+	ret = migrate_super_block(fd, mkfs_cfg.super_bytenr);
 	if (ret) {
 		error("unable to migrate super block: %d", ret);
 		goto fail;
-- 
2.5.5

Re: [PATCH 2/2] btrfs-convert: Fix migrate_super_block() to work with 64k sectorsize

[Qu Wenruo]

At 12/08/2016 09:56 PM, Chandan Rajendra wrote:
> migrate_super_block() uses sectorsize to refer to the size of the
> superblock. Hence on 64k sectorsize filesystems, it ends up computing
> checksum beyond the super block length (i.e.
> BTRFS_SUPER_INFO_SIZE). This commit fixes the bug by using
> BTRFS_SUPER_INFO_SIZE instead of sectorsize of the underlying
> filesystem.
>
> Signed-off-by: Chandan Rajendra <chandan@linux.vnet.ibm.com>

Reviewed-by: Qu Wenruo <quwenruo@cn.fujitsu.com>

BTW would you please enhance the convert tests?
Current convert tests only uses 4K as block size.
So adding 64K blocksize would definitely improve the tests.

Thanks,
Qu

> ---
>  convert/main.c | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)
>
> diff --git a/convert/main.c b/convert/main.c
> index 1148a36..fd6f77b 100644
> --- a/convert/main.c
> +++ b/convert/main.c
> @@ -1360,7 +1360,7 @@ err:
>  /*
>   * Migrate super block to its default position and zero 0 ~ 16k
>   */
> -static int migrate_super_block(int fd, u64 old_bytenr, u32 sectorsize)
> +static int migrate_super_block(int fd, u64 old_bytenr)
>  {
>  	int ret;
>  	struct extent_buffer *buf;
> @@ -1368,13 +1368,13 @@ static int migrate_super_block(int fd, u64 old_bytenr, u32 sectorsize)
>  	u32 len;
>  	u32 bytenr;
>
> -	buf = malloc(sizeof(*buf) + sectorsize);
> +	buf = malloc(sizeof(*buf) + BTRFS_SUPER_INFO_SIZE);
>  	if (!buf)
>  		return -ENOMEM;
>
> -	buf->len = sectorsize;
> -	ret = pread(fd, buf->data, sectorsize, old_bytenr);
> -	if (ret != sectorsize)
> +	buf->len = BTRFS_SUPER_INFO_SIZE;
> +	ret = pread(fd, buf->data, BTRFS_SUPER_INFO_SIZE, old_bytenr);
> +	if (ret != BTRFS_SUPER_INFO_SIZE)
>  		goto fail;
>
>  	super = (struct btrfs_super_block *)buf->data;
> @@ -1382,19 +1382,20 @@ static int migrate_super_block(int fd, u64 old_bytenr, u32 sectorsize)
>  	btrfs_set_super_bytenr(super, BTRFS_SUPER_INFO_OFFSET);
>
>  	csum_tree_block_size(buf, BTRFS_CRC32_SIZE, 0);
> -	ret = pwrite(fd, buf->data, sectorsize, BTRFS_SUPER_INFO_OFFSET);
> -	if (ret != sectorsize)
> +	ret = pwrite(fd, buf->data, BTRFS_SUPER_INFO_SIZE,
> +		BTRFS_SUPER_INFO_OFFSET);
> +	if (ret != BTRFS_SUPER_INFO_SIZE)
>  		goto fail;
>
>  	ret = fsync(fd);
>  	if (ret)
>  		goto fail;
>
> -	memset(buf->data, 0, sectorsize);
> +	memset(buf->data, 0, BTRFS_SUPER_INFO_SIZE);
>  	for (bytenr = 0; bytenr < BTRFS_SUPER_INFO_OFFSET; ) {
>  		len = BTRFS_SUPER_INFO_OFFSET - bytenr;
> -		if (len > sectorsize)
> -			len = sectorsize;
> +		if (len > BTRFS_SUPER_INFO_SIZE)
> +			len = BTRFS_SUPER_INFO_SIZE;
>  		ret = pwrite(fd, buf->data, len, bytenr);
>  		if (ret != len) {
>  			fprintf(stderr, "unable to zero fill device\n");
> @@ -2519,7 +2520,7 @@ static int do_convert(const char *devname, int datacsum, int packing,
>  	 * If this step succeed, we get a mountable btrfs. Otherwise
>  	 * the source fs is left unchanged.
>  	 */
> -	ret = migrate_super_block(fd, mkfs_cfg.super_bytenr, blocksize);
> +	ret = migrate_super_block(fd, mkfs_cfg.super_bytenr);
>  	if (ret) {
>  		error("unable to migrate super block: %d", ret);
>  		goto fail;
>

Re: [PATCH 2/2] btrfs-convert: Fix migrate_super_block() to work with 64k sectorsize

[Chandan Rajendra]

On Friday, December 09, 2016 09:09:29 AM Qu Wenruo wrote:
> 
> At 12/08/2016 09:56 PM, Chandan Rajendra wrote:
> > migrate_super_block() uses sectorsize to refer to the size of the
> > superblock. Hence on 64k sectorsize filesystems, it ends up computing
> > checksum beyond the super block length (i.e.
> > BTRFS_SUPER_INFO_SIZE). This commit fixes the bug by using
> > BTRFS_SUPER_INFO_SIZE instead of sectorsize of the underlying
> > filesystem.
> >
> > Signed-off-by: Chandan Rajendra <chandan@linux.vnet.ibm.com>
> 
> Reviewed-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
> 
> BTW would you please enhance the convert tests?
> Current convert tests only uses 4K as block size.
> So adding 64K blocksize would definitely improve the tests.
>

Thanks for the hint. I just executed btrfs/012 with 64k blocksize hardcoded
and found that 'btrfs rollback' failed. I will fix rollback first and then
work on getting btrfs/012 to support 64k blocksize.

-- 
chandan

Re: [PATCH 2/2] btrfs-convert: Fix migrate_super_block() to work with 64k sectorsize

[David Sterba]

On Fri, Dec 09, 2016 at 09:09:29AM +0800, Qu Wenruo wrote:
> 
> 
> At 12/08/2016 09:56 PM, Chandan Rajendra wrote:
> > migrate_super_block() uses sectorsize to refer to the size of the
> > superblock. Hence on 64k sectorsize filesystems, it ends up computing
> > checksum beyond the super block length (i.e.
> > BTRFS_SUPER_INFO_SIZE). This commit fixes the bug by using
> > BTRFS_SUPER_INFO_SIZE instead of sectorsize of the underlying
> > filesystem.
> >
> > Signed-off-by: Chandan Rajendra <chandan@linux.vnet.ibm.com>
> 
> Reviewed-by: Qu Wenruo <quwenruo@cn.fujitsu.com>

Applied, thanks.

Re: [PATCH 1/2] btrfs-progs: btrfs-convert: Prevent accounting blocks beyond end of device

[Qu Wenruo]

Hi Chandan,

Thanks for the patch.

At 12/08/2016 09:56 PM, Chandan Rajendra wrote:
> When looping across data block bitmap, __ext2_add_one_block() may add
> blocks which do not exist on the underlying disk. This commit prevents
> this from happening by checking the block index against the maximum
> block count that was present in the ext4 filesystem instance that is
> being converted.

The patch looks good to me.

Reviewed-by: Qu Wenruo <quwenruo@cn.fujitsu.com>

Just curious about if such image can pass e2fsck.
And would you please upload a minimal image as btrfs-progs test case?

Thanks,
Qu

>
> Signed-off-by: Chandan Rajendra <chandan@linux.vnet.ibm.com>
> ---
>  convert/main.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/convert/main.c b/convert/main.c
> index 4b4cea4..1148a36 100644
> --- a/convert/main.c
> +++ b/convert/main.c
> @@ -1525,6 +1525,9 @@ static int __ext2_add_one_block(ext2_filsys fs, char *bitmap,
>  	offset /= EXT2FS_CLUSTER_RATIO(fs);
>  	offset += group_nr * EXT2_CLUSTERS_PER_GROUP(fs->super);
>  	for (i = 0; i < EXT2_CLUSTERS_PER_GROUP(fs->super); i++) {
> +		if ((i + offset) >= ext2fs_blocks_count(fs->super))
> +			break;
> +
>  		if (ext2fs_test_bit(i, bitmap)) {
>  			u64 start;
>
>

Re: [PATCH 1/2] btrfs-progs: btrfs-convert: Prevent accounting blocks beyond end of device

[Chandan Rajendra]

On Friday, December 09, 2016 09:03:57 AM Qu Wenruo wrote:
> Hi Chandan,
> 
> Thanks for the patch.
> 
> At 12/08/2016 09:56 PM, Chandan Rajendra wrote:
> > When looping across data block bitmap, __ext2_add_one_block() may add
> > blocks which do not exist on the underlying disk. This commit prevents
> > this from happening by checking the block index against the maximum
> > block count that was present in the ext4 filesystem instance that is
> > being converted.
> 
> The patch looks good to me.
> 
> Reviewed-by: Qu Wenruo <quwenruo@cn.fujitsu.com>
> 
> Just curious about if such image can pass e2fsck.
> And would you please upload a minimal image as btrfs-progs test case?
> 

Hi Qu,

Such an ext4 filesystem can be consistently created on ppc64 with 64k as the
blocksize of the filesystem. Also, the filesystem thus created passes e2fsck.

-- 
chandan

Re: [PATCH 1/2] btrfs-progs: btrfs-convert: Prevent accounting blocks beyond end of device

[David Sterba]

On Fri, Dec 09, 2016 at 09:03:57AM +0800, Qu Wenruo wrote:
> Hi Chandan,
> 
> Thanks for the patch.
> 
> At 12/08/2016 09:56 PM, Chandan Rajendra wrote:
> > When looping across data block bitmap, __ext2_add_one_block() may add
> > blocks which do not exist on the underlying disk. This commit prevents
> > this from happening by checking the block index against the maximum
> > block count that was present in the ext4 filesystem instance that is
> > being converted.
> 
> The patch looks good to me.
> 
> Reviewed-by: Qu Wenruo <quwenruo@cn.fujitsu.com>

Applied, thanks.

Re: [PATCH 1/2] btrfs-progs: btrfs-convert: Prevent accounting blocks beyond end of device

[Qu Wenruo]

Hi Chandan,

Thanks for the patch.

At 12/08/2016 09:56 PM, Chandan Rajendra wrote:
> When looping across data block bitmap, __ext2_add_one_block() may add
> blocks which do not exist on the underlying disk. This commit prevents
> this from happening by checking the block index against the maximum
> block count that was present in the ext4 filesystem instance that is
> being converted.

The patch looks good to me.

Reviewed-by: Qu Wenruo <quwenruo@cn.fujitsu.com>

Just curious about if such image can pass e2fsck.
And would you please upload a minimal image as btrfs-progs test case?

Thanks,
Qu

>
> Signed-off-by: Chandan Rajendra <chandan@linux.vnet.ibm.com>
> ---
>  convert/main.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/convert/main.c b/convert/main.c
> index 4b4cea4..1148a36 100644
> --- a/convert/main.c
> +++ b/convert/main.c
> @@ -1525,6 +1525,9 @@ static int __ext2_add_one_block(ext2_filsys fs, char *bitmap,
>  	offset /= EXT2FS_CLUSTER_RATIO(fs);
>  	offset += group_nr * EXT2_CLUSTERS_PER_GROUP(fs->super);
>  	for (i = 0; i < EXT2_CLUSTERS_PER_GROUP(fs->super); i++) {
> +		if ((i + offset) >= ext2fs_blocks_count(fs->super))
> +			break;
> +
>  		if (ext2fs_test_bit(i, bitmap)) {
>  			u64 start;
>
>