Skip to contentSkip to conversation

Rootdir refactor and small bug fixes

PATCH In review Qu Wenruo 2 participants 9 messages Oct 19, 2017 – Oct 26, 2017

Subsystems (2)
lore
Share & tools
lore

Qu Wenruo

author
#0 lore raw

v2 cover Rootdir refactor and small bug fixes

Sorry for the v2 patchset, just added a new 3-line patch. But since it can screw up bisect, I re-send the whole patchset, to make the new patch just before mkfs return value fix, so bisect will work as it used to do.

First 4 patches are small bug fixes which can be applied even we don't touch the functionality of --rootdir.

The last two patches will refactor --rootdir related functions ,mainly size_sourcedir() and make_image(), to mkfs/rootdir.[ch].
And rename them to btrfs_mkfs_size_dir() and btrfs_mkfs_fill_dir() respectively.
Functionality is not changed at all, so it will still shrink the device or using the first 1M reserved space.

This moved about 700 lines, which reduced about 1/3 of original mkfs.c.

And by moving this ancient code to its own files, I also fixed several small nits exposed by checkpatch script.

This provides a clean environment for later rootdir rework.

changelog:
v2:
Add a new fix, to avoid mkfs return 1. The rest doesn't change. Add reviewed-by tag.

Qu Wenruo (6):
btrfs-progs: Avoid BUG_ON for chunk allocation when ENOSPC happens
btrfs-progs: mkfs: Avoid positive return value from
cleanup_temp_chunks
btrfs-progs: mkfs: Fix overwritten return value for mkfs
btrfs-progs: mkfs: Error out gracefully for --rootdir
btrfs-progs: mkfs: Move image creation of rootdir to its own files
btrfs-progs: mkfs: Move source dir size calculation to its own files

 Makefile       |   4 +-
 extent-tree.c  |   3 +-
 mkfs/main.c    | 713 +------------------------------------------------------
 mkfs/rootdir.c | 735 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 mkfs/rootdir.h |  32 +++
 volumes.c      |  18 +-
 6 files changed, 795 insertions(+), 710 deletions(-)
 create mode 100644 mkfs/rootdir.c
 create mode 100644 mkfs/rootdir.h
Signature · 1 line
2.14.2
author
#1 lore raw

PATCH v2 1/6 btrfs-progs: Avoid BUG_ON for chunk allocation when ENOSPC happens

When passing directory larger than block device using --rootdir parameter, we get the following backtrace:

------
extent-tree.c:2693: btrfs_reserve_extent: BUG_ON ret triggered, value -28 ./mkfs.btrfs(+0x1a05d)[0x557939e6b05d] ./mkfs.btrfs(btrfs_reserve_extent+0xb5a)[0x557939e710c8] ./mkfs.btrfs(+0xb0b6)[0x557939e5c0b6] ./mkfs.btrfs(main+0x15d5)[0x557939e5de04] /usr/lib/libc.so.6(__libc_start_main+0xea)[0x7f83b101af6a] ./mkfs.btrfs(_start+0x2a)[0x557939e5af5a]
------

Nothing special, just BUG_ON() abusing from ancient code.

Fix them by using correct return.

---
 extent-tree.c |  3 ++-
 volumes.c     | 18 ++++++++++++++----
 2 files changed, 16 insertions(+), 5 deletions(-)
extent-tree.c +2 -1
index 525a237e5923..055582c36da6 100644
@@ -2690,7 +2690,8 @@ int btrfs_reserve_extent(struct btrfs_trans_handle *trans,
 			       search_start, search_end, hint_byte, ins,
 			       trans->alloc_exclude_start,
 			       trans->alloc_exclude_nr, data);
-	BUG_ON(ret);
+	if (ret < 0)
+		return ret;
 	clear_extent_dirty(&info->free_space_cache,
 			   ins->objectid, ins->objectid + ins->offset - 1);
 	return ret;
volumes.c +14 -4
index 2209e5a9100b..e1ee27d5f3ce 100644
@@ -1032,11 +1032,13 @@ again:
 			     info->chunk_root->root_key.objectid,
 			     BTRFS_FIRST_CHUNK_TREE_OBJECTID, key.offset,
 			     calc_size, &dev_offset, 0);
-		BUG_ON(ret);
+		if (ret < 0)
+			goto out_chunk_map;
 
 		device->bytes_used += calc_size;
 		ret = btrfs_update_device(trans, device);
-		BUG_ON(ret);
+		if (ret < 0)
+			goto out_chunk_map;
 
 		map->stripes[index].dev = device;
 		map->stripes[index].physical = dev_offset;
@@ -1075,16 +1077,24 @@ again:
 	map->ce.size = *num_bytes;
 
 	ret = insert_cache_extent(&info->mapping_tree.cache_tree, &map->ce);
-	BUG_ON(ret);
+	if (ret < 0)
+		goto out_chunk_map;
 
 	if (type & BTRFS_BLOCK_GROUP_SYSTEM) {
 		ret = btrfs_add_system_chunk(info, &key,
 				    chunk, btrfs_chunk_item_size(num_stripes));
-		BUG_ON(ret);
+		if (ret < 0)
+			goto out_chunk;
 	}
 
 	kfree(chunk);
 	return ret;
+
+out_chunk_map:
+	kfree(map);
+out_chunk:
+	kfree(chunk);
+	return ret;
 }
 
 /*
Signature · 1 line
2.14.2
author
#2 lore raw

PATCH v2 2/6 btrfs-progs: mkfs: Avoid positive return value from cleanup_temp_chunks

In reply to Qu Wenruo

Since we're calling btrfs_search_slot() the return value can be positive.
However we just pass that return value out, causing undefined return value.

This can cause mkfs return 1, which indicates something wrong.

Fix it.

---
 mkfs/main.c | 3 +++
 1 file changed, 3 insertions(+)
mkfs/main.c +3 -0
index 80e6089c37a1..9d53c6632b45 100644
@@ -1350,6 +1350,9 @@ static int cleanup_temp_chunks(struct btrfs_fs_info *fs_info,
 		ret = btrfs_search_slot(trans, root, &key, &path, 0, 0);
 		if (ret < 0)
 			goto out;
+		/* Don't pollute ret for >0 case */
+		if (ret > 0)
+			ret = 0;
 
 		btrfs_item_key_to_cpu(path.nodes[0], &found_key,
 				      path.slots[0]);
Signature · 1 line
2.14.2
author
#3 lore raw

PATCH v2 3/6 btrfs-progs: mkfs: Fix overwritten return value for mkfs

In reply to Qu Wenruo

For mkfs failure, especially --rootdir errors like EPERM/ENOSPC, the out branch will overwrite return value, causing wrong status code.

---
 mkfs/main.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
mkfs/main.c +3 -2
index 9d53c6632b45..60250c011ac3 100644
@@ -1426,6 +1426,7 @@ int main(int argc, char **argv)
 	int zero_end = 1;
 	int fd = -1;
 	int ret;
+	int close_ret;
 	int i;
 	int mixed = 0;
 	int nodesize_forced = 0;
@@ -1944,9 +1945,9 @@ raid_groups:
 	 */
 	fs_info->finalize_on_close = 1;
 out:
-	ret = close_ctree(root);
+	close_ret = close_ctree(root);
 
-	if (!ret) {
+	if (!close_ret) {
 		optind = saved_optind;
 		dev_cnt = argc - optind;
 		while (dev_cnt-- > 0) {
Signature · 1 line
2.14.2
author
#4 lore raw

PATCH v2 4/6 btrfs-progs: mkfs: Error out gracefully for --rootdir

In reply to Qu Wenruo

--rootdir option will start a transaction to fill the fs, however if something goes wrong, from ENOSPC to lack of permission, we won't commit transaction and cause BUG_ON trigger by uncommitted transaction:

------
extent buffer leak: start 29392896 len 16384
extent_io.c:579: free_extent_buffer: BUG_ON eb->flags & EXTENT_DIRTY triggered, value 1
------

The root fix is to introduce btrfs_abort_transaction() in btrfs-progs, however in this particular case, we can workaround it by force committing the transaction.

Since during mkfs, the magic of btrfs is set to an invalid one, without setting fs_info->finalize_on_close() the fs is never able to be mounted. So even we force to commit wrong transaction we won't screw up things worse.

---
 mkfs/main.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)
mkfs/main.c +13 -0
index 60250c011ac3..358a046f1cf2 100644
@@ -1073,6 +1073,19 @@ static int make_image(const char *source_dir, struct btrfs_root *root)
 		printf("Making image is completed.\n");
 	return 0;
 fail:
+	/*
+	 * Since we don't have btrfs_abort_transaction() yet, uncommitted trans
+	 * will trigger a BUG_ON().
+	 *
+	 * However before mkfs is fully finished, the magic number is invalid,
+	 * so even we commit transaction here, the fs still can't be mounted.
+	 *
+	 * To do a graceful error out, here we commit transaction as a
+	 * workaround.
+	 * Since we have already hit some problem, the return value doesn't
+	 * matter now.
+	 */
+	btrfs_commit_transaction(trans, root);
 	while (!list_empty(&dir_head.list)) {
 		dir_entry = list_entry(dir_head.list.next,
 				       struct directory_name_entry, list);
Signature · 1 line
2.14.2
author
#5 lore raw

PATCH v2 5/6 btrfs-progs: mkfs: Move image creation of rootdir to its own files

In reply to Qu Wenruo

In fact, --rootdir option is getting more and more independent from normal mkfs code.

So move image creation function, make_image() and its related code to mkfs/rootdir.[ch], and rename the function to btrfs_mkfs_fill_dir().

---
 Makefile       |   4 +-
 mkfs/main.c    | 652 +------------------------------------------------------
 mkfs/rootdir.c | 672 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 mkfs/rootdir.h |  30 +++
 4 files changed, 706 insertions(+), 652 deletions(-)
 create mode 100644 mkfs/rootdir.c
 create mode 100644 mkfs/rootdir.h
Makefile +2 -2
index d0657aaea0f5..12747547766f 100644
@@ -113,7 +113,7 @@ cmds_objects = cmds-subvolume.o cmds-filesystem.o cmds-device.o cmds-scrub.o \
 	       cmds-restore.o cmds-rescue.o chunk-recover.o super-recover.o \
 	       cmds-property.o cmds-fi-usage.o cmds-inspect-dump-tree.o \
 	       cmds-inspect-dump-super.o cmds-inspect-tree-stats.o cmds-fi-du.o \
-	       mkfs/common.o
+	       mkfs/common.o mkfs/rootdir.o
 libbtrfs_objects = send-stream.o send-utils.o kernel-lib/rbtree.o btrfs-list.o \
 		   kernel-lib/crc32c.o messages.o \
 		   uuid-tree.o utils-lib.o rbtree-utils.o
@@ -123,7 +123,7 @@ libbtrfs_headers = send-stream.h send-utils.h send.h kernel-lib/rbtree.h btrfs-l
 	       extent-cache.h extent_io.h ioctl.h ctree.h btrfsck.h version.h
 convert_objects = convert/main.o convert/common.o convert/source-fs.o \
 		  convert/source-ext2.o convert/source-reiserfs.o
-mkfs_objects = mkfs/main.o mkfs/common.o
+mkfs_objects = mkfs/main.o mkfs/common.o mkfs/rootdir.o
 image_objects = image/main.o
 all_objects = $(objects) $(cmds_objects) $(libbtrfs_objects) $(convert_objects) \
 	      $(mkfs_objects) $(image_objects)
mkfs/main.c +2 -650

692 lines not on this page · raw message

mkfs/rootdir.c new file +672 -0

672 lines not on this page · raw message

mkfs/rootdir.h new file +30 -0
new file mode 100644
index 000000000000..68f88643bf7a
@@ -0,0 +1,30 @@
+/*
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public
+ * License v2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public
+ * License along with this program.
+ */
+
+/*
+ * Defines and functions declarations for users mkfs --rootdir
+ */
+
+#ifndef __BTRFS_MKDIR_ROOTDIR_H__
+#define __BTRFS_MKDIR_ROOTDIR_H__
+struct directory_name_entry {
+	const char *dir_name;
+	const char *path;
+	ino_t inum;
+	struct list_head list;
+};
+
+int btrfs_mkfs_fill_dir(const char *source_dir, struct btrfs_root *root,
+			bool verbose);
+#endif
Signature · 1 line
2.14.2
#6 lore raw

Re: [PATCH v2 5/6] btrfs-progs: mkfs: Move image creation of rootdir to its own files

Makefile cmds_objects = cmds-subvolume.o cmds-filesystem.o cmds-device.o cmds-scrub.o \ · line 116

Quoted 9 lines · Qu Wenruo

index d0657aaea0f5..12747547766f 100644

Makefile +1 -1
@@ -113,7 +113,7 @@ cmds_objects = cmds-subvolume.o cmds-filesystem.o cmds-device.o cmds-scrub.o \
 	       cmds-restore.o cmds-rescue.o chunk-recover.o super-recover.o \
 	       cmds-property.o cmds-fi-usage.o cmds-inspect-dump-tree.o \
 	       cmds-inspect-dump-super.o cmds-inspect-tree-stats.o cmds-fi-du.o \
-	       mkfs/common.o
+	       mkfs/common.o mkfs/rootdir.o

Adding this mkfs file to cmds_objects does not make sense, everything that cmds-* could need should be in mkfs/common.o, so I've removed it.

Quoted 16 lines
Makefile +0 -0
 libbtrfs_objects = send-stream.o send-utils.o kernel-lib/rbtree.o btrfs-list.o \
 		   kernel-lib/crc32c.o messages.o \
 		   uuid-tree.o utils-lib.o rbtree-utils.o
mkfs/rootdir.h new file +10 -0
@@ -0,0 +1,30 @@
+/*
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public
+ * License v2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *

mkfs/rootdir.h line 20

Quoted 10 lines
mkfs/rootdir.h new file +10 -0
+ * You should have received a copy of the GNU General Public
+ * License along with this program.
+ */
+
+/*
+ * Defines and functions declarations for users mkfs --rootdir
+ */
+
+#ifndef __BTRFS_MKDIR_ROOTDIR_H__
+#define __BTRFS_MKDIR_ROOTDIR_H__

MKDIR -> MKFS, fixed

author
#7 lore raw

PATCH v2 6/6 btrfs-progs: mkfs: Move source dir size calculation to its own files

In reply to Qu Wenruo

Also rename the function from size_sourcedir() to mkfs_size_dir().

---
 mkfs/main.c    | 66 ++--------------------------------------------------------
 mkfs/rootdir.c | 63 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 mkfs/rootdir.h |  2 ++
 3 files changed, 67 insertions(+), 64 deletions(-)
mkfs/main.c +2 -64
index 7861e3075d6b..423b35579722 100644
@@ -31,7 +31,6 @@
 #include <uuid/uuid.h>
 #include <ctype.h>
 #include <blkid/blkid.h>
-#include <ftw.h>
 #include "ctree.h"
 #include "disk-io.h"
 #include "volumes.h"
@@ -448,67 +447,6 @@ static int create_chunks(struct btrfs_trans_handle *trans,
 	return ret;
 }
 
-/*
- * This ignores symlinks with unreadable targets and subdirs that can't
- * be read.  It's a best-effort to give a rough estimate of the size of
- * a subdir.  It doesn't guarantee that prepopulating btrfs from this
- * tree won't still run out of space.
- */
-static u64 global_total_size;
-static u64 fs_block_size;
-static int ftw_add_entry_size(const char *fpath, const struct stat *st,
-			      int type)
-{
-	if (type == FTW_F || type == FTW_D)
-		global_total_size += round_up(st->st_size, fs_block_size);
-
-	return 0;
-}
-
-static u64 size_sourcedir(const char *dir_name, u64 sectorsize,
-			  u64 *num_of_meta_chunks_ret, u64 *size_of_data_ret)
-{
-	u64 dir_size = 0;
-	u64 total_size = 0;
-	int ret;
-	u64 default_chunk_size = SZ_8M;
-	u64 allocated_meta_size = SZ_8M;
-	u64 allocated_total_size = 20 * SZ_1M;	/* 20MB */
-	u64 num_of_meta_chunks = 0;
-	u64 num_of_data_chunks = 0;
-	u64 num_of_allocated_meta_chunks =
-			allocated_meta_size / default_chunk_size;
-
-	global_total_size = 0;
-	fs_block_size = sectorsize;
-	ret = ftw(dir_name, ftw_add_entry_size, 10);
-	dir_size = global_total_size;
-	if (ret < 0) {
-		error("ftw subdir walk of %s failed: %s", dir_name,
-			strerror(errno));
-		exit(1);
-	}
-
-	num_of_data_chunks = (dir_size + default_chunk_size - 1) /
-		default_chunk_size;
-
-	num_of_meta_chunks = (dir_size / 2) / default_chunk_size;
-	if (((dir_size / 2) % default_chunk_size) != 0)
-		num_of_meta_chunks++;
-	if (num_of_meta_chunks <= num_of_allocated_meta_chunks)
-		num_of_meta_chunks = 0;
-	else
-		num_of_meta_chunks -= num_of_allocated_meta_chunks;
-
-	total_size = allocated_total_size +
-		     (num_of_data_chunks * default_chunk_size) +
-		     (num_of_meta_chunks * default_chunk_size);
-
-	*num_of_meta_chunks_ret = num_of_meta_chunks;
-	*size_of_data_ret = num_of_data_chunks * default_chunk_size;
-	return total_size;
-}
-
 static int zero_output_file(int out_fd, u64 size)
 {
 	int loop_num;
@@ -1085,8 +1023,8 @@ int main(int argc, char **argv)
 			goto error;
 		}
 
-		source_dir_size = size_sourcedir(source_dir, sectorsize,
-					     &num_of_meta_chunks, &size_of_data);
+		source_dir_size = btrfs_mkfs_size_dir(source_dir, sectorsize,
+					&num_of_meta_chunks, &size_of_data);
 		if(block_count < source_dir_size)
 			block_count = source_dir_size;
 		ret = zero_output_file(fd, block_count);
mkfs/rootdir.c +63 -0

78 lines not on this page · raw message

mkfs/rootdir.h +2 -0
index 68f88643bf7a..a557bd183f7b 100644
@@ -27,4 +27,6 @@ struct directory_name_entry {
 
 int btrfs_mkfs_fill_dir(const char *source_dir, struct btrfs_root *root,
 			bool verbose);
+u64 btrfs_mkfs_size_dir(const char *dir_name, u64 sectorsize,
+			u64 *num_of_meta_chunks_ret, u64 *size_of_data_ret);
 #endif
Signature · 1 line
2.14.2
#8 lore raw
In reply to Qu Wenruo
Quoted 28 lines · Qu Wenruo

Sorry for the v2 patchset, just added a new 3-line patch. But since it can screw up bisect, I re-send the whole patchset, to make the new patch just before mkfs return value fix, so bisect will work as it used to do.

First 4 patches are small bug fixes which can be applied even we don't touch the functionality of --rootdir.

The last two patches will refactor --rootdir related functions ,mainly size_sourcedir() and make_image(), to mkfs/rootdir.[ch].
And rename them to btrfs_mkfs_size_dir() and btrfs_mkfs_fill_dir() respectively.
Functionality is not changed at all, so it will still shrink the device or using the first 1M reserved space.

This moved about 700 lines, which reduced about 1/3 of original mkfs.c.

And by moving this ancient code to its own files, I also fixed several small nits exposed by checkpatch script.

This provides a clean environment for later rootdir rework.

changelog:
v2:
Add a new fix, to avoid mkfs return 1. The rest doesn't change. Add reviewed-by tag.

Quoted 8 lines

Qu Wenruo (6):
btrfs-progs: Avoid BUG_ON for chunk allocation when ENOSPC happens
btrfs-progs: mkfs: Avoid positive return value from
cleanup_temp_chunks
btrfs-progs: mkfs: Fix overwritten return value for mkfs
btrfs-progs: mkfs: Error out gracefully for --rootdir
btrfs-progs: mkfs: Move image creation of rootdir to its own files
btrfs-progs: mkfs: Move source dir size calculation to its own files

All patches applied, thanks.