Skip to contentSkip to conversation

Small fixes for btrfs-progs

PATCH Superseded Filipe David Borba Manana 4 participants 16 messages Jun 30, 2013 – Aug 8, 2013 Btrfs file system

lore
Share & tools
lore

Filipe David Borba Manana

author
#0 lore raw

cover Small fixes for btrfs-progs

1) add missing write check for mkfs
2) add kstrdup() return value check
3) remove unused code in btrfs_scan_one_device()

Filipe David Borba Manana (3):
Btrfs-progs: add missing write check for mkfs
Btrfs-progs: add kstrdup() return value check
Btrfs-progs: remove unused code in btrfs_scan_one_device()

 utils.c   |    1 +
 volumes.c |    7 +++++--
 2 files changed, 6 insertions(+), 2 deletions(-)
Signature · 1 line
1.7.9.5
author
#1 lore raw

PATCH 1/3 Btrfs-progs: add missing write check for mkfs

Assert that the write of the device tree root succeeds. This verification is currently done for all other tree roots, however it was missing for the device tree root.

Would this tree root write fail, but all others succeed, it would lead to a corrupted/incomplete btrfs filesystem.

Signed-off-by Filipe David Borba Manana <[email protected]>
---
 utils.c |    1 +
 1 file changed, 1 insertion(+)
utils.c +1 -0
index 7b4cd74..43d93f1 100644
@@ -381,6 +381,7 @@ int make_btrfs(int fd, const char *device, const char *label,
 	btrfs_set_header_nritems(buf, nritems);
 	csum_tree_block_size(buf, BTRFS_CRC32_SIZE, 0);
 	ret = pwrite(fd, buf->data, leafsize, blocks[4]);
+	BUG_ON(ret != leafsize);
 
 	/* create the FS root */
 	memset(buf->data+sizeof(struct btrfs_header), 0,
Signature · 1 line
1.7.9.5
#2 lore raw

Re: [PATCH 1/3] Btrfs-progs: add missing write check for mkfs

Quoted 17 lines · Filipe David Borba Manana

Assert that the write of the device tree root succeeds. This verification is currently done for all other tree roots, however it was missing for the device tree root.

Would this tree root write fail, but all others succeed, it would lead to a corrupted/incomplete btrfs filesystem.

Signed-off-by Filipe David Borba Manana <[email protected]>
---
 utils.c |    1 +
 1 file changed, 1 insertion(+)
utils.c +0 -0
index 7b4cd74..43d93f1 100644

utils.c int make_btrfs(int fd, const char *device, const char *label, · line 384

Quoted 4 lines
utils.c +1 -0
@@ -381,6 +381,7 @@ int make_btrfs(int fd, const char *device, const char *label,
 	btrfs_set_header_nritems(buf, nritems);
 	csum_tree_block_size(buf, BTRFS_CRC32_SIZE, 0);
 	ret = pwrite(fd, buf->data, leafsize, blocks[4]);
+	BUG_ON(ret != leafsize);

Please fix also the 2nd instance where the return value check is missing

 344         btrfs_set_header_bytenr(buf, blocks[3]);
 345         btrfs_set_header_owner(buf, BTRFS_CHUNK_TREE_OBJECTID);
 346         btrfs_set_header_nritems(buf, nritems);
 347         csum_tree_block_size(buf, BTRFS_CRC32_SIZE, 0);
 348         ret = pwrite(fd, buf->data, leafsize, blocks[3]);

349
350 /* create the device tree */
351 memset(buf->data+sizeof(struct btrfs_header), 0,
352 leafsize-sizeof(struct btrfs_header));

utils.c int make_btrfs(int fd, const char *device, const char *label, · line 387

Quoted 3 lines
utils.c +0 -0
 	/* create the FS root */
 	memset(buf->data+sizeof(struct btrfs_header), 0,

Also, replacing the BUG_ON with something else would be nice of course.

thanks,
david

author
#3 lore raw

Re: [PATCH 1/3] Btrfs-progs: add missing write check for mkfs

Quoted 13 lines · David Sterba
Quoted 12 lines · Filipe David Borba Manana

Assert that the write of the device tree root succeeds. This verification is currently done for all other tree roots, however it was missing for the device tree root.

Would this tree root write fail, but all others succeed, it would lead to a corrupted/incomplete btrfs filesystem.

Signed-off-by Filipe David Borba Manana <[email protected]>
---
 utils.c |    1 +
 1 file changed, 1 insertion(+)
Quoted 12 lines

utils.c int make_btrfs(int fd, const char *device, const char *label, · line 384

Quoted 9 lines
utils.c +1 -0
index 7b4cd74..43d93f1 100644
@@ -381,6 +381,7 @@ int make_btrfs(int fd, const char *device, const char *label,
      btrfs_set_header_nritems(buf, nritems);
      csum_tree_block_size(buf, BTRFS_CRC32_SIZE, 0);
      ret = pwrite(fd, buf->data, leafsize, blocks[4]);
+     BUG_ON(ret != leafsize);

Please fix also the 2nd instance where the return value check is missing

Good catch, thanks. Adding it now.

Quoted 12 lines
 344         btrfs_set_header_bytenr(buf, blocks[3]);
 345         btrfs_set_header_owner(buf, BTRFS_CHUNK_TREE_OBJECTID);
 346         btrfs_set_header_nritems(buf, nritems);
 347         csum_tree_block_size(buf, BTRFS_CRC32_SIZE, 0);
 348         ret = pwrite(fd, buf->data, leafsize, blocks[3]);

349
350 /* create the device tree */
351 memset(buf->data+sizeof(struct btrfs_header), 0,
352 leafsize-sizeof(struct btrfs_header));

Quoted 1 line
Quoted 4 lines
Quoted 2 lines
      /* create the FS root */
      memset(buf->data+sizeof(struct btrfs_header), 0,

Also, replacing the BUG_ON with something else would be nice of course.

If you don't mind, I would prefer to do that as a different patch, since the BUG_ON() use is what is currently done everywhere in this function at least.

thanks

Quoted 3 lines

thanks,
david

--
Filipe David Manana,

"Reasonable men adapt themselves to the world.
Unreasonable men adapt the world to themselves. That's why all progress depends on unreasonable men."

#4 lore raw

Re: [PATCH 1/3] Btrfs-progs: add missing write check for mkfs

Quoted 5 lines · Filipe David Manana
Quoted 1 line

Also, replacing the BUG_ON with something else would be nice of course.

If you don't mind, I would prefer to do that as a different patch, since the BUG_ON() use is what is currently done everywhere in this function at least.

Yes, separate patch.

thanks,
david

author
#5 lore raw

PATCH 2/3 Btrfs-progs: add kstrdup() return value check

In reply to Filipe David Borba Manana

When allocating a btrfs_device structure, device_list_add() in volumes.c was not checking if the call to duplicate the label string succeeded or not.

Signed-off-by Filipe David Borba Manana <[email protected]>
---
 volumes.c |    5 +++++
 1 file changed, 5 insertions(+)
volumes.c +5 -0
index d6f81f8..ea1d401 100644
@@ -125,6 +125,11 @@ static int device_list_add(const char *path,
 			return -ENOMEM;
 		}
 		device->label = kstrdup(disk_super->label, GFP_NOFS);
+		if (!device->label) {
+			kfree(device->name);
+			kfree(device);
+			return -ENOMEM;
+		}
 		device->total_devs = btrfs_super_num_devices(disk_super);
 		device->super_bytes_used = btrfs_super_bytes_used(disk_super);
 		device->total_bytes =
Signature · 1 line
1.7.9.5
author
#6 lore raw

PATCH 3/3 Btrfs-progs: remove unused code

In reply to Filipe David Borba Manana

The uuid_unparse() call in btrfs_scan_one_device() was a no-op.

Signed-off-by Filipe David Borba Manana <[email protected]>
---
 volumes.c |    2 --
 1 file changed, 2 deletions(-)
volumes.c +0 -2
index ea1d401..9d5e97e 100644
@@ -223,7 +223,6 @@ int btrfs_scan_one_device(int fd, const char *path,
 	char *buf;
 	int ret;
 	u64 devid;
-	char uuidbuf[37];
 
 	buf = malloc(4096);
 	if (!buf) {

@@ -241,7 +240,6 @@ int btrfs_scan_one_device(int fd, const char *path,
 		*total_devs = 1;
 	else
 		*total_devs = btrfs_super_num_devices(disk_super);
-	uuid_unparse(disk_super->fsid, uuidbuf);
 
 	ret = device_list_add(path, disk_super, devid, fs_devices_ret);
Signature · 1 line
1.7.9.5
Reviewed-by on v1 3/3
#7 lore raw

Re: [PATCH 3/3] Btrfs-progs: remove unused code

good catch.

Reviewed-by Anand Jain <[email protected]>
Quoted 30 lines · Filipe David Borba Manana

The uuid_unparse() call in btrfs_scan_one_device() was a no-op.

Signed-off-by Filipe David Borba Manana <[email protected]>
---
  volumes.c |    2 --
  1 file changed, 2 deletions(-)
volumes.c +0 -2
index ea1d401..9d5e97e 100644
@@ -223,7 +223,6 @@ int btrfs_scan_one_device(int fd, const char *path,
  	char *buf;
  	int ret;
  	u64 devid;
-	char uuidbuf[37];
 
  	buf = malloc(4096);
  	if (!buf) {

@@ -241,7 +240,6 @@ int btrfs_scan_one_device(int fd, const char *path,
  		*total_devs = 1;
  	else
  		*total_devs = btrfs_super_num_devices(disk_super);
-	uuid_unparse(disk_super->fsid, uuidbuf);
 
  	ret = device_list_add(path, disk_super, devid, fs_devices_ret);
author
#8 lore raw

PATCH v2 1/3 Btrfs-progs: add missing write check for mkfs

In reply to Filipe David Borba Manana

Assert that the writes of the device and chunk tree roots succeed. This verification is currently done for all other tree roots, however it was missing for those 2 trees.

Would these tree root writes fail, but all others succeed, it would lead to a corrupted/incomplete btrfs filesystem, or, more likely some weird failure later on in mkfs.btrfs inside open_ctree().

V2: Added check for the chunk tree root write.

Signed-off-by Filipe David Borba Manana <[email protected]>
---
 utils.c |    2 ++
 1 file changed, 2 insertions(+)
utils.c +2 -0
index 7b4cd74..702a0b1 100644
@@ -346,6 +346,7 @@ int make_btrfs(int fd, const char *device, const char *label,
 	btrfs_set_header_nritems(buf, nritems);
 	csum_tree_block_size(buf, BTRFS_CRC32_SIZE, 0);
 	ret = pwrite(fd, buf->data, leafsize, blocks[3]);
+	BUG_ON(ret != leafsize);
 
 	/* create the device tree */
 	memset(buf->data+sizeof(struct btrfs_header), 0,

@@ -381,6 +382,7 @@ int make_btrfs(int fd, const char *device, const char *label,
 	btrfs_set_header_nritems(buf, nritems);
 	csum_tree_block_size(buf, BTRFS_CRC32_SIZE, 0);
 	ret = pwrite(fd, buf->data, leafsize, blocks[4]);
+	BUG_ON(ret != leafsize);
 
 	/* create the FS root */
 	memset(buf->data+sizeof(struct btrfs_header), 0,
Signature · 1 line
1.7.9.5
author
#9 lore raw

PATCH v2 3/3 Btrfs-progs: remove unused code

In reply to Filipe David Borba Manana

V2: removed unused statement in
extent-cache.c:tree_insert() too.

Signed-off-by Filipe David Borba Manana <[email protected]>
---
 extent-cache.c |    1 -
 volumes.c      |    2 --
 2 files changed, 3 deletions(-)
extent-cache.c +0 -1
index 3dd6434..a8bab59 100644
@@ -44,7 +44,6 @@ static struct rb_node *tree_insert(struct rb_root *root, u64 offset,
 			return parent;
 	}
 
-	entry = rb_entry(parent, struct cache_extent, rb_node);
 	rb_link_node(node, parent, p);
 	rb_insert_color(node, root);
 	return NULL;
volumes.c +0 -2
index ea1d401..9d5e97e 100644
@@ -223,7 +223,6 @@ int btrfs_scan_one_device(int fd, const char *path,
 	char *buf;
 	int ret;
 	u64 devid;
-	char uuidbuf[37];
 
 	buf = malloc(4096);
 	if (!buf) {

@@ -241,7 +240,6 @@ int btrfs_scan_one_device(int fd, const char *path,
 		*total_devs = 1;
 	else
 		*total_devs = btrfs_super_num_devices(disk_super);
-	uuid_unparse(disk_super->fsid, uuidbuf);
 
 	ret = device_list_add(path, disk_super, devid, fs_devices_ret);
Signature · 1 line
1.7.9.5
author
#10 lore raw

v2 cover Small fixes for btrfs-progs

In reply to Filipe David Borba Manana

1) add missing write checks for mkfs
2) add kstrdup() return value check
3) remove unused code
4) make_btrfs() return error code on write failure
5) check for errors in btrfs_add_block_group()

V2: added patches 4 and 5.

Filipe David Borba Manana (5):
Btrfs-progs: add missing write check for mkfs
Btrfs-progs: add kstrdup() return value check
Btrfs-progs: remove unused code
Btrfs-progs: return error on write failure in make_btrfs()
Btrfs-progs: don't ignore errors in btrfs_add_block_group()

 btrfs-convert.c |    3 ++-
 extent-cache.c  |    1 -
 extent-tree.c   |   12 +++++++-----
 mkfs.c          |    2 +-
 utils.c         |   34 ++++++++++++++++++++++++++++------
 volumes.c       |    7 +++++--
 6 files changed, 43 insertions(+), 16 deletions(-)
Signature · 1 line
1.7.9.5
author
#11 lore raw

PATCH v2 4/5 Btrfs-progs: return error on write failure in make_btrfs()

Instead of aborting with a BUG_ON() statement, return a negated errno code. Also updated mkfs and convert tools to print a nicer error message when make_btrfs() returns an error.

Signed-off-by Filipe David Borba Manana <[email protected]>
---
 btrfs-convert.c |    3 ++-
 mkfs.c          |    2 +-
 utils.c         |   36 ++++++++++++++++++++++++++++--------
 3 files changed, 31 insertions(+), 10 deletions(-)
btrfs-convert.c +2 -1
index 399856f..0e66eb9 100644
@@ -2323,7 +2323,8 @@ int do_convert(const char *devname, int datacsum, int packing, int noxattr)
 			 blocks, total_bytes, blocksize, blocksize,
 			 blocksize, blocksize);
 	if (ret) {
-		fprintf(stderr, "unable to create initial ctree\n");
+		fprintf(stderr, "unable to create initial ctree: %s\n",
+			strerror(-ret));
 		goto fail;
 	}
 	/* create a system chunk that maps the whole device */
mkfs.c +1 -1
index 7ff60e5..348bcaa 100644
@@ -1446,7 +1446,7 @@ int main(int ac, char **av)
 			 nodesize, leafsize,
 			 sectorsize, stripesize);
 	if (ret) {
-		fprintf(stderr, "error during mkfs %d\n", ret);
+		fprintf(stderr, "error during mkfs: %s\n", strerror(-ret));
 		exit(1);
 	}
utils.c +28 -8
index 702a0b1..f43816b 100644
@@ -215,7 +215,10 @@ int make_btrfs(int fd, const char *device, const char *label,
 
 	csum_tree_block_size(buf, BTRFS_CRC32_SIZE, 0);
 	ret = pwrite(fd, buf->data, leafsize, blocks[1]);
-	BUG_ON(ret != leafsize);
+	if (ret < 0)
+		return -errno;
+	else if (ret != leafsize)
+		return -EIO;
 
 	/* create the items for the extent tree */
 	memset(buf->data+sizeof(struct btrfs_header), 0,

@@ -262,7 +265,10 @@ int make_btrfs(int fd, const char *device, const char *label,
 	btrfs_set_header_nritems(buf, nritems);
 	csum_tree_block_size(buf, BTRFS_CRC32_SIZE, 0);
 	ret = pwrite(fd, buf->data, leafsize, blocks[2]);
-	BUG_ON(ret != leafsize);
+	if (ret < 0)
+		return -errno;
+	else if (ret != leafsize)
+		return -EIO;
 
 	/* create the chunk tree */
 	memset(buf->data+sizeof(struct btrfs_header), 0,

@@ -346,7 +352,10 @@ int make_btrfs(int fd, const char *device, const char *label,
 	btrfs_set_header_nritems(buf, nritems);
 	csum_tree_block_size(buf, BTRFS_CRC32_SIZE, 0);
 	ret = pwrite(fd, buf->data, leafsize, blocks[3]);
-	BUG_ON(ret != leafsize);
+	if (ret < 0)
+		return -errno;
+	else if (ret != leafsize)
+		return -EIO;
 
 	/* create the device tree */
 	memset(buf->data+sizeof(struct btrfs_header), 0,

@@ -382,7 +391,10 @@ int make_btrfs(int fd, const char *device, const char *label,
 	btrfs_set_header_nritems(buf, nritems);
 	csum_tree_block_size(buf, BTRFS_CRC32_SIZE, 0);
 	ret = pwrite(fd, buf->data, leafsize, blocks[4]);
-	BUG_ON(ret != leafsize);
+	if (ret < 0)
+		return -errno;
+	else if (ret != leafsize)
+		return -EIO;
 
 	/* create the FS root */
 	memset(buf->data+sizeof(struct btrfs_header), 0,

@@ -392,7 +404,10 @@ int make_btrfs(int fd, const char *device, const char *label,
 	btrfs_set_header_nritems(buf, 0);
 	csum_tree_block_size(buf, BTRFS_CRC32_SIZE, 0);
 	ret = pwrite(fd, buf->data, leafsize, blocks[5]);
-	BUG_ON(ret != leafsize);
+	if (ret < 0)
+		return -errno;
+	else if (ret != leafsize)
+		return -EIO;
 
 	/* finally create the csum root */
 	memset(buf->data+sizeof(struct btrfs_header), 0,

@@ -402,7 +417,10 @@ int make_btrfs(int fd, const char *device, const char *label,
 	btrfs_set_header_nritems(buf, 0);
 	csum_tree_block_size(buf, BTRFS_CRC32_SIZE, 0);
 	ret = pwrite(fd, buf->data, leafsize, blocks[6]);
-	BUG_ON(ret != leafsize);
+	if (ret < 0)
+		return -errno;
+	else if (ret != leafsize)
+		return -EIO;
 
 	/* and write out the super block */
 	BUG_ON(sizeof(super) > sectorsize);

@@ -411,8 +429,10 @@ int make_btrfs(int fd, const char *device, const char *label,
 	buf->len = sectorsize;
 	csum_tree_block_size(buf, BTRFS_CRC32_SIZE, 0);
 	ret = pwrite(fd, buf->data, sectorsize, blocks[0]);
-	BUG_ON(ret != sectorsize);
-
+	if (ret < 0)
+		return -errno;
+	else if (ret != leafsize)
+		return -EIO;
 
 	free(buf);
 	return 0;
Signature · 1 line
1.7.9.5
#12 lore raw

Re: [PATCH 4/5] Btrfs-progs: return error on write failure in make_btrfs()

Quoted 6 lines · Filipe David Borba Manana

Instead of aborting with a BUG_ON() statement, return a negated errno code. Also updated mkfs and convert tools to print a nicer error message when make_btrfs() returns an error.

Signed-off-by Filipe David Borba Manana <[email protected]>

[...]

utils.c int make_btrfs(int fd, const char *device, const char *label, · line 435

Quoted 7 lines
utils.c +4 -2
 	ret = pwrite(fd, buf->data, sectorsize, blocks[0]);
-	BUG_ON(ret != sectorsize);
-
+	if (ret < 0)
+		return -errno;
+	else if (ret != leafsize)
+		return -EIO;

mkfs.btrfs with leafsize != sectorsize fails. I've sent a patch for it.

author
#13 lore raw

Re: [PATCH 4/5] Btrfs-progs: return error on write failure in make_btrfs()

Quoted 16 lines · Stefan Behrens
Quoted 6 lines · Filipe David Borba Manana

Instead of aborting with a BUG_ON() statement, return a negated errno code. Also updated mkfs and convert tools to print a nicer error message when make_btrfs() returns an error.

Signed-off-by Filipe David Borba Manana <[email protected]>

[...]

utils.c int make_btrfs(int fd, const char *device, const char *label, · line 435

Quoted 7 lines
      ret = pwrite(fd, buf->data, sectorsize, blocks[0]);
utils.c +4 -2
-     BUG_ON(ret != sectorsize);
-
+     if (ret < 0)
+             return -errno;
+     else if (ret != leafsize)
+             return -EIO;
Quoted 1 line

mkfs.btrfs with leafsize != sectorsize fails. I've sent a patch for it.

Copy paste error :(
Thanks for catching and fixing it.

Quoted 1 line
Signature · 5 lines
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That's why all progress depends on unreasonable men."
author
#14 lore raw

PATCH v2 5/5 Btrfs-progs: don't ignore errors in btrfs_add_block_group()

In reply to Filipe David Borba Manana

This function was not checking if the calls to set_extent_bits() and set_state_private() actually succeeded or not.

Signed-off-by Filipe David Borba Manana <[email protected]>
---
 extent-tree.c |   12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)
extent-tree.c +7 -5
index b0cfe0a..d260c14 100644
@@ -3355,12 +3355,14 @@ btrfs_add_block_group(struct btrfs_fs_info *fs_info, u64 bytes_used, u64 type,
 	BUG_ON(ret);
 
 	bit = block_group_state_bits(type);
-	set_extent_bits(block_group_cache, chunk_offset,
-			chunk_offset + size - 1,
-			bit | EXTENT_LOCKED, GFP_NOFS);
+	ret = set_extent_bits(block_group_cache, chunk_offset,
+			      chunk_offset + size - 1,
+			      bit | EXTENT_LOCKED, GFP_NOFS);
+	BUG_ON(ret);
 
-	set_state_private(block_group_cache, chunk_offset,
-			  (unsigned long)cache);
+	ret = set_state_private(block_group_cache, chunk_offset,
+				(unsigned long)cache);
+	BUG_ON(ret);
 	set_avail_alloc_bits(fs_info, type);
 
 	return cache;
Signature · 1 line
1.7.9.5
author
#15 lore raw

PATCH v2 5/5 Btrfs-progs: don't ignore errors in extent-tree.c

In reply to Filipe David Borba Manana

Several function return values were being completely ignored.

V2: Added more error checking all over extent-tree.c

Signed-off-by Filipe David Borba Manana <[email protected]>
---
 extent-tree.c |   20 +++++++++++++-------
 1 file changed, 13 insertions(+), 7 deletions(-)
extent-tree.c +13 -7
index 6490d8a..6458ce4 100644
@@ -2110,6 +2110,7 @@ static int finish_current_insert(struct btrfs_trans_handle *trans,
 						extent_op->flags,
 						&extent_op->key,
 						extent_op->level, &key);
+			BUG_ON(ret);
 		} else {
 			BUG_ON(1);
 		}

@@ -2756,7 +2757,7 @@ static int alloc_reserved_tree_block(struct btrfs_trans_handle *trans,
 
 	ret = update_block_group(trans, root, ins->objectid, root->leafsize,
 				 1, 0);
-	return 0;
+	return ret;
 }
 
 static int alloc_tree_block(struct btrfs_trans_handle *trans,

@@ -3345,12 +3346,14 @@ btrfs_add_block_group(struct btrfs_fs_info *fs_info, u64 bytes_used, u64 type,
 	BUG_ON(ret);
 
 	bit = block_group_state_bits(type);
-	set_extent_bits(block_group_cache, chunk_offset,
-			chunk_offset + size - 1,
-			bit | EXTENT_LOCKED, GFP_NOFS);
+	ret = set_extent_bits(block_group_cache, chunk_offset,
+			      chunk_offset + size - 1,
+			      bit | EXTENT_LOCKED, GFP_NOFS);
+	BUG_ON(ret);
 
-	set_state_private(block_group_cache, chunk_offset,
-			  (unsigned long)cache);
+	ret = set_state_private(block_group_cache, chunk_offset,
+				(unsigned long)cache);
+	BUG_ON(ret);
 	set_avail_alloc_bits(fs_info, type);
 
 	return cache;

@@ -3372,8 +3375,11 @@ int btrfs_make_block_group(struct btrfs_trans_handle *trans,
 				sizeof(cache->item));
 	BUG_ON(ret);
 
-	finish_current_insert(trans, extent_root);
+	ret = finish_current_insert(trans, extent_root);
+	BUG_ON(ret);
 	ret = del_pending_extents(trans, extent_root);
+	BUG_ON(ret);
+
 	return 0;
 }
Signature · 1 line
1.7.9.5