[PATCH 0/9] Overhaul btrfs-corrupt-block

[PATCH 0/9] Overhaul btrfs-corrupt-block

[Nikolay Borisov]

btrfs-corrupt-block is a very useful tool albeit very neglected. This series 
aims to give it much needed attention. There is a mix of code-improvements and 
bug fixes. Code improvement mainly consists of factoring our duplicated code 
(Patch 1,3,6) and improving the interface of some options (4,5,8,9). The
recurring topic here is that instead of having to pass 

btrfs-corrupt-block -K <key> -f <field> -<SOME OPTION WHICH CORRUPTS AN ITEM, 
POINTED TO BY K> 

make each corrupting option to take the key as an argument to it. Say we want
to corrupt an item field (-I options) we now do: 
 
 btrfs-corrupt-block -I <key> -r <root> /dev/blah 

 instead of 

 btrfs-corrupt-block -I -K <key> -r <root> /dev/blah 

Some patches also incorporate fixes for bugs (patch 2,7 and 9) I found during 
my testing. Those usability improvements are needed to enable me to produce 
tests for the pending free space tree support in userspace. 


Nikolay Borisov (9):
  btrfs-progs: btrfs-corrupt-block: Factor out specific-root code
  btrfs-progs: btrfs-corrupt-block: Correctly handle -r when passing -I
  btrfs-progs: btrfs-corrupt-block: Factor out key parsing function
  btrfs-progs: btrfs-corrupt-block: Change -I flag parameter format
  btrfs-progs: btrfs-corrupt-block: Convert -K flag argument handling to
    common function
  btrfs-progs: btrfs-corrupt-block: Factor out common "-r" handling code
  btrfs-progs: btrfs-corrupt-block: Add support for handling specific
    root when using -K option
  btrfs-progs: btrfs-corrupt-block: Change format of -d option
  btrfs-progs: btrfs-corrupt-block: Fix -D option

 btrfs-corrupt-block.c | 94 +++++++++++++++++++++++++++++++--------------------
 1 file changed, 57 insertions(+), 37 deletions(-)

-- 
2.7.4

[PATCH 1/9] btrfs-progs: btrfs-corrupt-block: Factor out specific-root code

[Nikolay Borisov]

Some options operate on a specific root so let's extract the code which
deals with this. No functional change.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 btrfs-corrupt-block.c | 37 +++++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 14 deletions(-)

diff --git a/btrfs-corrupt-block.c b/btrfs-corrupt-block.c
index da0ec8c51e5a..ab6ca0a1e90a 100644
--- a/btrfs-corrupt-block.c
+++ b/btrfs-corrupt-block.c
@@ -1080,6 +1080,26 @@ static int find_chunk_offset(struct btrfs_root *root,
 	return ret;
 
 }
+
+static struct btrfs_root *open_root(struct btrfs_fs_info *fs_info,
+				    u64 root_objectid)
+{
+
+	struct btrfs_key root_key;
+	struct btrfs_root *root;
+
+	root_key.objectid = root_objectid;
+	root_key.type = BTRFS_ROOT_ITEM_KEY;
+	root_key.offset = (u64)-1;
+
+	root = btrfs_read_fs_root(fs_info, &root_key);
+	if (IS_ERR(root)) {
+		fprintf(stderr, "Couldn't find root %llu\n", root_objectid);
+		print_usage(1);
+	}
+
+	return root;
+}
 int main(int argc, char **argv)
 {
 	struct cache_tree root_cache;
@@ -1326,20 +1346,9 @@ int main(int argc, char **argv)
 
 		if (!key.objectid)
 			print_usage(1);
-		if (root_objectid) {
-			struct btrfs_key root_key;
-
-			root_key.objectid = root_objectid;
-			root_key.type = BTRFS_ROOT_ITEM_KEY;
-			root_key.offset = (u64)-1;
-
-			target = btrfs_read_fs_root(root->fs_info, &root_key);
-			if (IS_ERR(target)) {
-				fprintf(stderr, "Couldn't find root %llu\n",
-					(unsigned long long)root_objectid);
-				print_usage(1);
-			}
-		}
+		if (root_objectid)
+			target = open_root(root->fs_info, root_objectid);
+
 		ret = delete_item(target, &key);
 		goto out_close;
 	}
-- 
2.7.4

[PATCH 2/9] btrfs-progs: btrfs-corrupt-block: Correctly handle -r when passing -I

[Nikolay Borisov]

The documentation for the -I option (corrupt an item) states:

  An item to corrupt (must also specify the field to corrupt and a root+key for the item)

The code on the other hand doesn't check whether -r is in fact passed,
and even if it is it's not handled at all. This means presently -I
is possible to corrupt items only in the root tree. Fix this by
correctly checking -r is passed and fail otherwise and passing the
correct root to corrupt_btrfs_item.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 btrfs-corrupt-block.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/btrfs-corrupt-block.c b/btrfs-corrupt-block.c
index ab6ca0a1e90a..0018b6c9662d 100644
--- a/btrfs-corrupt-block.c
+++ b/btrfs-corrupt-block.c
@@ -1337,9 +1337,15 @@ int main(int argc, char **argv)
 		goto out_close;
 	}
 	if (corrupt_item) {
+		struct btrfs_root *target;
 		if (!key.objectid)
 			print_usage(1);
-		ret = corrupt_btrfs_item(root, &key, field);
+		if (!root_objectid)
+			print_usage(1);
+
+		target = open_root(root->fs_info, root_objectid);
+
+		ret = corrupt_btrfs_item(target, &key, field);
 	}
 	if (delete) {
 		struct btrfs_root *target = root;
-- 
2.7.4

[PATCH 3/9] btrfs-progs: btrfs-corrupt-block: Factor out key parsing function

[Nikolay Borisov]

Currently passing a key with -K handling is open coded. I intend on
changing the interface a bit to make the program more usable. To aid
in this factor out common code which parses a triplet of the
"u64,u8,u64" format, corresponding to a btrfs key. No functional
changes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 btrfs-corrupt-block.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/btrfs-corrupt-block.c b/btrfs-corrupt-block.c
index 0018b6c9662d..39a611d89d55 100644
--- a/btrfs-corrupt-block.c
+++ b/btrfs-corrupt-block.c
@@ -1081,6 +1081,16 @@ static int find_chunk_offset(struct btrfs_root *root,
 
 }
 
+static void parse_key(u64 *objectid, u8 *type, u64 *offset)
+{
+
+	int ret = sscanf(optarg, "%llu,%hhu,%llu", objectid, type, offset);
+	if (ret != 3) {
+	        fprintf(stderr, "Error parsing key %d\n", errno);
+	        print_usage(1);
+	}
+}
+
 static struct btrfs_root *open_root(struct btrfs_fs_info *fs_info,
 				    u64 root_objectid)
 {
-- 
2.7.4

[PATCH 4/9] btrfs-progs: btrfs-corrupt-block: Change -I flag parameter format

[Nikolay Borisov]

Presently, if we want to corrupt a particular item we need to call
corrupt block like so:

btrfs-corrupt-block -I -K <objectid,type,offset> -r "numeric rootid"

This is problematic because the -K option not only sets the key to the
item that the -I option should corrupt but it also signals that we
want to corrupt the key itself. This way we cannot really use the
program with the following semantics:

"Corrupt only the item which corresponds to this key but leave the
key intact"

This patch aims to fix this by changing the format of the -I flag. So
if one wants to corrupt only an item (and leave the key intact) they
should use:

  btrfs-corrupt-block -r <numeric rootid> -I <objectid,type,offset>

In addition to this problem, -K doesn't really understand the the "-r"
argument, so when using it in conjunction with -I to corrupt an item,
not part of the root tree, it will always produce an error:

  Couldn't find the key to corrupt

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 btrfs-corrupt-block.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/btrfs-corrupt-block.c b/btrfs-corrupt-block.c
index 39a611d89d55..4cc3528df105 100644
--- a/btrfs-corrupt-block.c
+++ b/btrfs-corrupt-block.c
@@ -116,7 +116,7 @@ static void print_usage(int ret)
 	printf("\t-m   The metadata block to corrupt (must also specify -f for the field to corrupt)\n");
 	printf("\t-K   The key to corrupt in the format <num>,<num>,<num> (must also specify -f for the field)\n");
 	printf("\t-f   The field in the item to corrupt\n");
-	printf("\t-I   An item to corrupt (must also specify the field to corrupt and a root+key for the item)\n");
+	printf("\t-I <u64,u8,u64> Corrupt an item corresponding to the passed key triplet (must also specify the field to corrupt and root for the item)\n");
 	printf("\t-D   Corrupt a dir item, must specify key and field\n");
 	printf("\t-d   Delete this item (must specify -K)\n");
 	printf("\t-r   Operate on this root (only works with -d)\n");
@@ -1165,7 +1165,7 @@ int main(int argc, char **argv)
 			{ NULL, 0, NULL, 0 }
 		};
 
-		c = getopt_long(argc, argv, "l:c:b:eEkuUi:f:x:m:K:IDdr:C:",
+		c = getopt_long(argc, argv, "l:c:b:eEkuUi:f:x:m:K:I:Ddr:C:",
 				long_options, NULL);
 		if (c < 0)
 			break;
@@ -1222,6 +1222,7 @@ int main(int argc, char **argv)
 				break;
 			case 'I':
 				corrupt_item = 1;
+				parse_key(&key.objectid, &key.type, &key.offset);
 				break;
 			case 'd':
 				delete = 1;
@@ -1356,6 +1357,7 @@ int main(int argc, char **argv)
 		target = open_root(root->fs_info, root_objectid);
 
 		ret = corrupt_btrfs_item(target, &key, field);
+		goto out_close;
 	}
 	if (delete) {
 		struct btrfs_root *target = root;
-- 
2.7.4

[PATCH 5/9] btrfs-progs: btrfs-corrupt-block: Convert -K flag argument handling to common function

[Nikolay Borisov]

There is now a common function used to parse btrfs keys triplets so
use that one. No functional changes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 btrfs-corrupt-block.c | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/btrfs-corrupt-block.c b/btrfs-corrupt-block.c
index 4cc3528df105..7a5513c455e6 100644
--- a/btrfs-corrupt-block.c
+++ b/btrfs-corrupt-block.c
@@ -114,7 +114,7 @@ static void print_usage(int ret)
 	printf("\t-i   The inode item to corrupt (must also specify the field to corrupt)\n");
 	printf("\t-x   The file extent item to corrupt (must also specify -i for the inode and -f for the field to corrupt)\n");
 	printf("\t-m   The metadata block to corrupt (must also specify -f for the field to corrupt)\n");
-	printf("\t-K   The key to corrupt in the format <num>,<num>,<num> (must also specify -f for the field)\n");
+	printf("\t-K <u64,u8,u64> Corrupt the given key (must also specify -f for the field)\n");
 	printf("\t-f   The field in the item to corrupt\n");
 	printf("\t-I <u64,u8,u64> Corrupt an item corresponding to the passed key triplet (must also specify the field to corrupt and root for the item)\n");
 	printf("\t-D   Corrupt a dir item, must specify key and field\n");
@@ -1129,6 +1129,7 @@ int main(int argc, char **argv)
 	int corrupt_item = 0;
 	int corrupt_di = 0;
 	int delete = 0;
+	int should_corrupt_key = 0;
 	u64 metadata_block = 0;
 	u64 inode = 0;
 	u64 file_extent = (u64)-1;
@@ -1207,15 +1208,8 @@ int main(int argc, char **argv)
 				metadata_block = arg_strtou64(optarg);
 				break;
 			case 'K':
-				ret = sscanf(optarg, "%llu,%u,%llu",
-					     &key.objectid,
-					     (unsigned int *)&key.type,
-					     &key.offset);
-				if (ret != 3) {
-					fprintf(stderr, "error reading key "
-						"%d\n", errno);
-					print_usage(1);
-				}
+				should_corrupt_key = 1;
+				parse_key(&key.objectid, &key.type, &key.offset);
 				break;
 			case 'D':
 				corrupt_di = 1;
@@ -1370,7 +1364,7 @@ int main(int argc, char **argv)
 		ret = delete_item(target, &key);
 		goto out_close;
 	}
-	if (key.objectid || key.offset || key.type) {
+	if (should_corrupt_key) {
 		if (*field == 0)
 			print_usage(1);
 		ret = corrupt_key(root, &key, field);
-- 
2.7.4

[PATCH 6/9] btrfs-progs: btrfs-corrupt-block: Factor out common "-r" handling code

[Nikolay Borisov]

Since more and more of the "corrupt XXX" options are going to support
combination with -r option, let's extract the common code needed for
this. No functional changes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 btrfs-corrupt-block.c | 17 +++++++----------
 1 file changed, 7 insertions(+), 10 deletions(-)

diff --git a/btrfs-corrupt-block.c b/btrfs-corrupt-block.c
index 7a5513c455e6..8bbfe1cdb855 100644
--- a/btrfs-corrupt-block.c
+++ b/btrfs-corrupt-block.c
@@ -1114,7 +1114,7 @@ int main(int argc, char **argv)
 {
 	struct cache_tree root_cache;
 	struct btrfs_key key;
-	struct btrfs_root *root;
+	struct btrfs_root *root, *target_root;
 	char *dev;
 	/* chunk offset can be 0,so change to (u64)-1 */
 	u64 logical = (u64)-1;
@@ -1245,6 +1245,10 @@ int main(int argc, char **argv)
 		fprintf(stderr, "Open ctree failed\n");
 		exit(1);
 	}
+	target_root = root;
+	if (root_objectid)
+		target_root = open_root(root->fs_info, root_objectid);
+
 	if (extent_rec) {
 		struct btrfs_trans_handle *trans;
 
@@ -1342,26 +1346,19 @@ int main(int argc, char **argv)
 		goto out_close;
 	}
 	if (corrupt_item) {
-		struct btrfs_root *target;
 		if (!key.objectid)
 			print_usage(1);
 		if (!root_objectid)
 			print_usage(1);
 
-		target = open_root(root->fs_info, root_objectid);
-
-		ret = corrupt_btrfs_item(target, &key, field);
+		ret = corrupt_btrfs_item(target_root, &key, field);
 		goto out_close;
 	}
 	if (delete) {
-		struct btrfs_root *target = root;
-
 		if (!key.objectid)
 			print_usage(1);
-		if (root_objectid)
-			target = open_root(root->fs_info, root_objectid);
 
-		ret = delete_item(target, &key);
+		ret = delete_item(target_root, &key);
 		goto out_close;
 	}
 	if (should_corrupt_key) {
-- 
2.7.4

[PATCH 7/9] btrfs-progs: btrfs-corrupt-block: Add support for handling specific root when using -K option

[Nikolay Borisov]

Currently the -K option supports corrupting items only in the default
root (which is the root tree). This makes it impossible to test the
free-space recovery (or any other) code for that matter. Fix it by
using the root corresponding to the one passed in -r (if any).

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 btrfs-corrupt-block.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/btrfs-corrupt-block.c b/btrfs-corrupt-block.c
index 8bbfe1cdb855..74928eae839e 100644
--- a/btrfs-corrupt-block.c
+++ b/btrfs-corrupt-block.c
@@ -114,7 +114,7 @@ static void print_usage(int ret)
 	printf("\t-i   The inode item to corrupt (must also specify the field to corrupt)\n");
 	printf("\t-x   The file extent item to corrupt (must also specify -i for the inode and -f for the field to corrupt)\n");
 	printf("\t-m   The metadata block to corrupt (must also specify -f for the field to corrupt)\n");
-	printf("\t-K <u64,u8,u64> Corrupt the given key (must also specify -f for the field)\n");
+	printf("\t-K <u64,u8,u64> Corrupt the given key (must also specify -f for the field and optionally -r for the root)\n");
 	printf("\t-f   The field in the item to corrupt\n");
 	printf("\t-I <u64,u8,u64> Corrupt an item corresponding to the passed key triplet (must also specify the field to corrupt and root for the item)\n");
 	printf("\t-D   Corrupt a dir item, must specify key and field\n");
@@ -449,7 +449,6 @@ static int corrupt_key(struct btrfs_root *root, struct btrfs_key *key,
 	struct btrfs_trans_handle *trans;
 	int ret;
 
-	root = root->fs_info->fs_root;
 	if (corrupt_field == BTRFS_KEY_BAD) {
 		fprintf(stderr, "Invalid field %s\n", field);
 		return -EINVAL;
@@ -1364,7 +1363,8 @@ int main(int argc, char **argv)
 	if (should_corrupt_key) {
 		if (*field == 0)
 			print_usage(1);
-		ret = corrupt_key(root, &key, field);
+
+		ret = corrupt_key(target_root, &key, field);
 		goto out_close;
 	}
 	/*
-- 
2.7.4

[PATCH 8/9] btrfs-progs: btrfs-corrupt-block: Change format of -d option

[Nikolay Borisov]

Currently if we want to delete an item we need to invoke corrupt block
like so:

  btrfs-corrupt-block [-r <numeric id of root>] -K <objectid,type,offset> -d

Instead, this patch converts the format to:

  btrfs-corrupt-block [-r <numeric id of root>] -d <objectid,type,offset>

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 btrfs-corrupt-block.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/btrfs-corrupt-block.c b/btrfs-corrupt-block.c
index 74928eae839e..10a0b2d13b97 100644
--- a/btrfs-corrupt-block.c
+++ b/btrfs-corrupt-block.c
@@ -118,7 +118,7 @@ static void print_usage(int ret)
 	printf("\t-f   The field in the item to corrupt\n");
 	printf("\t-I <u64,u8,u64> Corrupt an item corresponding to the passed key triplet (must also specify the field to corrupt and root for the item)\n");
 	printf("\t-D   Corrupt a dir item, must specify key and field\n");
-	printf("\t-d   Delete this item (must specify -K)\n");
+	printf("\t-d <u64,u8,u64> Delete item corresponding to passed key triplet\n");
 	printf("\t-r   Operate on this root (only works with -d)\n");
 	printf("\t-C   Delete a csum for the specified bytenr.  When used with -b it'll delete that many bytes, otherwise it's just sectorsize\n");
 	exit(ret);
@@ -1165,7 +1165,7 @@ int main(int argc, char **argv)
 			{ NULL, 0, NULL, 0 }
 		};
 
-		c = getopt_long(argc, argv, "l:c:b:eEkuUi:f:x:m:K:I:Ddr:C:",
+		c = getopt_long(argc, argv, "l:c:b:eEkuUi:f:x:m:K:I:Dd:r:C:",
 				long_options, NULL);
 		if (c < 0)
 			break;
@@ -1219,6 +1219,7 @@ int main(int argc, char **argv)
 				break;
 			case 'd':
 				delete = 1;
+				parse_key(&key.objectid, &key.type, &key.offset);
 				break;
 			case 'r':
 				root_objectid = arg_strtou64(optarg);
-- 
2.7.4

[PATCH 9/9] btrfs-progs: btrfs-corrupt-block: Fix -D option

[Nikolay Borisov]

Currently the -D option is essentially defunct since it's the root,
where we are going to corrupt a dir item is always set to the tree
root. Fix this by passing the root from the "-r" option. Aditionally
convert the interface for this option to the new format. So if one
wants to corrupt a dir item in the default fs tree, they should now
invoke:

 btrfs-corrupt-block -r 5 -D <objectid,DIR_ITEM,offset> -f name

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 btrfs-corrupt-block.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/btrfs-corrupt-block.c b/btrfs-corrupt-block.c
index 10a0b2d13b97..37f850b5b239 100644
--- a/btrfs-corrupt-block.c
+++ b/btrfs-corrupt-block.c
@@ -117,7 +117,7 @@ static void print_usage(int ret)
 	printf("\t-K <u64,u8,u64> Corrupt the given key (must also specify -f for the field and optionally -r for the root)\n");
 	printf("\t-f   The field in the item to corrupt\n");
 	printf("\t-I <u64,u8,u64> Corrupt an item corresponding to the passed key triplet (must also specify the field to corrupt and root for the item)\n");
-	printf("\t-D   Corrupt a dir item, must specify key and field\n");
+	printf("\t-D <u64,u8,u64> Corrupt a dir item corresponding to the passed key triplet, must also specify a field\n");
 	printf("\t-d <u64,u8,u64> Delete item corresponding to passed key triplet\n");
 	printf("\t-r   Operate on this root (only works with -d)\n");
 	printf("\t-C   Delete a csum for the specified bytenr.  When used with -b it'll delete that many bytes, otherwise it's just sectorsize\n");
@@ -1165,7 +1165,7 @@ int main(int argc, char **argv)
 			{ NULL, 0, NULL, 0 }
 		};
 
-		c = getopt_long(argc, argv, "l:c:b:eEkuUi:f:x:m:K:I:Dd:r:C:",
+		c = getopt_long(argc, argv, "l:c:b:eEkuUi:f:x:m:K:I:D:d:r:C:",
 				long_options, NULL);
 		if (c < 0)
 			break;
@@ -1212,6 +1212,7 @@ int main(int argc, char **argv)
 				break;
 			case 'D':
 				corrupt_di = 1;
+				parse_key(&key.objectid, &key.type, &key.offset);
 				break;
 			case 'I':
 				corrupt_item = 1;
@@ -1338,7 +1339,7 @@ int main(int argc, char **argv)
 	if (corrupt_di) {
 		if (!key.objectid || *field == 0)
 			print_usage(1);
-		ret = corrupt_dir_item(root, &key, field);
+		ret = corrupt_dir_item(target_root, &key, field);
 		goto out_close;
 	}
 	if (csum_bytenr) {
-- 
2.7.4

Re: [PATCH 0/9] Overhaul btrfs-corrupt-block

[David Sterba]

On Mon, May 14, 2018 at 02:13:25PM +0300, Nikolay Borisov wrote:
> btrfs-corrupt-block is a very useful tool albeit very neglected. This series 
> aims to give it much needed attention. There is a mix of code-improvements and 
> bug fixes. Code improvement mainly consists of factoring our duplicated code 
> (Patch 1,3,6) and improving the interface of some options (4,5,8,9). The
> recurring topic here is that instead of having to pass 
> 
> btrfs-corrupt-block -K <key> -f <field> -<SOME OPTION WHICH CORRUPTS AN ITEM, 
> POINTED TO BY K> 
> 
> make each corrupting option to take the key as an argument to it. Say we want
> to corrupt an item field (-I options) we now do: 
>  
>  btrfs-corrupt-block -I <key> -r <root> /dev/blah 
> 
>  instead of 
> 
>  btrfs-corrupt-block -I -K <key> -r <root> /dev/blah 
> 
> Some patches also incorporate fixes for bugs (patch 2,7 and 9) I found during 
> my testing. Those usability improvements are needed to enable me to produce 
> tests for the pending free space tree support in userspace. 
> 
> 
> Nikolay Borisov (9):
>   btrfs-progs: btrfs-corrupt-block: Factor out specific-root code
>   btrfs-progs: btrfs-corrupt-block: Correctly handle -r when passing -I
>   btrfs-progs: btrfs-corrupt-block: Factor out key parsing function
>   btrfs-progs: btrfs-corrupt-block: Change -I flag parameter format
>   btrfs-progs: btrfs-corrupt-block: Convert -K flag argument handling to
>     common function
>   btrfs-progs: btrfs-corrupt-block: Factor out common "-r" handling code
>   btrfs-progs: btrfs-corrupt-block: Add support for handling specific
>     root when using -K option
>   btrfs-progs: btrfs-corrupt-block: Change format of -d option
>   btrfs-progs: btrfs-corrupt-block: Fix -D option

Applied, thanks.