[PATCH v2 0/7] Mkfs: Rework --rootdir to a more generic behavior

[PATCH v2 0/7] Mkfs: Rework --rootdir to a more generic behavior

[Qu Wenruo]

mkfs.btrfs --rootdir provides user a method to generate btrfs with
pre-written content while without the need of root privilege to mount
the fs.

However the code is quite old and doesn't get much review or test.
This makes some strange behavior, from customized chunk allocation
(which uses the reserved 0~1M device space) to variant BUG_ON caused by
ENOSPC or EPERM.

The reworked --rootdir will be based on traditional mkfs, everything is
processed after traditional mkfs, so nothing is customized.

The result will be an equivalent of mkfs, mount, cp, umount.
(If btrfs-progs chunk/extent allocator acts as the same as kernel)

And, add extra explanation for --rootdir, since the old implement
introduced a confusing behavior to limit the filesystem size.

Patch 1 is just a refactor I found during coding.
Patch 2 fixes a bug that will cause BUG_ON if modifying the first DATA
chunk after cleanup_temp_chunks().
Patch 3 is the rework.
Patch 4~5 fixes extra BUG_ON() caused by --rootdir in different error
path.
Patch 6 will add extra explanation for man page.

Qu Wenruo (7):
  btrfs-progs: Refactor find_next_chunk() to get rid of parameter root
    and objectid
  btrfs-progs: Fix one-byte overlap bug in free_block_group_cache
  btrfs-progs: mkfs: Rework rootdir option to avoid custom chunk layout
  btrfs-progs: mkfs: Update allocation info before verbose output
  btrfs-progs: Avoid BUG_ON for chunk allocation when ENOSPC happens
  btrfs-progs: mkfs: Workaround BUG_ON caused by rootdir option
  btrfs-progs: Doc/mkfs: Add extra condition for rootdir option

 Documentation/mkfs.btrfs.asciidoc |  11 ++
 extent-tree.c                     |   5 +-
 mkfs/main.c                       | 281 +++++++++++---------------------------
 volumes.c                         |  32 +++--
 4 files changed, 113 insertions(+), 216 deletions(-)

-- 
2.14.1

[PATCH v2 1/7] btrfs-progs: Refactor find_next_chunk() to get rid of parameter root and objectid

[Qu Wenruo]

Function find_next_chunk() is used to find next chunk start position,
which should only do search on chunk tree and objectid is fixed to
BTRFS_FIRST_CHUNK_TREE_OBJECTID.

So refactor the parameter list to get rid of @root, which should be get
from fs_info->chunk_root, and @objectid, which is fixed to
BTRFS_FIRST_CHUNK_TREE_OBJECTID.

Signed-off-by: Qu Wenruo <quwenruo.btrfs@gmx.com>
---
 volumes.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/volumes.c b/volumes.c
index 2ae2d1bb..2209e5a9 100644
--- a/volumes.c
+++ b/volumes.c
@@ -505,8 +505,9 @@ err:
 	return ret;
 }
 
-static int find_next_chunk(struct btrfs_root *root, u64 objectid, u64 *offset)
+static int find_next_chunk(struct btrfs_fs_info *fs_info, u64 *offset)
 {
+	struct btrfs_root *root = fs_info->chunk_root;
 	struct btrfs_path *path;
 	int ret;
 	struct btrfs_key key;
@@ -517,7 +518,7 @@ static int find_next_chunk(struct btrfs_root *root, u64 objectid, u64 *offset)
 	if (!path)
 		return -ENOMEM;
 
-	key.objectid = objectid;
+	key.objectid = BTRFS_FIRST_CHUNK_TREE_OBJECTID;
 	key.offset = (u64)-1;
 	key.type = BTRFS_CHUNK_ITEM_KEY;
 
@@ -533,7 +534,7 @@ static int find_next_chunk(struct btrfs_root *root, u64 objectid, u64 *offset)
 	} else {
 		btrfs_item_key_to_cpu(path->nodes[0], &found_key,
 				      path->slots[0]);
-		if (found_key.objectid != objectid)
+		if (found_key.objectid != BTRFS_FIRST_CHUNK_TREE_OBJECTID)
 			*offset = 0;
 		else {
 			chunk = btrfs_item_ptr(path->nodes[0], path->slots[0],
@@ -995,8 +996,7 @@ again:
 		}
 		return -ENOSPC;
 	}
-	ret = find_next_chunk(chunk_root, BTRFS_FIRST_CHUNK_TREE_OBJECTID,
-			      &offset);
+	ret = find_next_chunk(info, &offset);
 	if (ret)
 		return ret;
 	key.objectid = BTRFS_FIRST_CHUNK_TREE_OBJECTID;
@@ -1129,9 +1129,7 @@ int btrfs_alloc_data_chunk(struct btrfs_trans_handle *trans,
 	} else {
 		u64 tmp;
 
-		ret = find_next_chunk(chunk_root,
-				      BTRFS_FIRST_CHUNK_TREE_OBJECTID,
-				      &tmp);
+		ret = find_next_chunk(info, &tmp);
 		key.offset = tmp;
 		if (ret)
 			return ret;
-- 
2.14.1

[PATCH v2 2/7] btrfs-progs: Fix one-byte overlap bug in free_block_group_cache

[Qu Wenruo]

free_block_group_cache() calls clear_extent_bits() with wrong end, which
is one byte larger than the correct range.

This will cause the next adjacent cache state be split.
And due to the split, private pointer (which points to block group
cache) will be reset to NULL.

This is very hard to detect as this function only gets called in
cleanup_temp_chunks() which is just before mkfs finishes.
This bug only get exposed when reworking --rootdir option.

Signed-off-by: Qu Wenruo <quwenruo.btrfs@gmx.com>
---
 extent-tree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/extent-tree.c b/extent-tree.c
index eed56886..525a237e 100644
--- a/extent-tree.c
+++ b/extent-tree.c
@@ -3724,7 +3724,7 @@ static int free_block_group_cache(struct btrfs_trans_handle *trans,
 		btrfs_remove_free_space_cache(cache);
 		kfree(cache->free_space_ctl);
 	}
-	clear_extent_bits(&fs_info->block_group_cache, bytenr, bytenr + len,
+	clear_extent_bits(&fs_info->block_group_cache, bytenr, bytenr + len - 1,
 			  (unsigned int)-1);
 	ret = free_space_info(fs_info, flags, len, 0, NULL);
 	if (ret < 0)
-- 
2.14.1

[PATCH v2 3/7] btrfs-progs: mkfs: Rework rootdir option to avoid custom chunk layout

[Qu Wenruo]

mkfs.btrfs --rootdir uses its own custom chunk layout.
This provides the possibility to limit the filesystem to a minimal size.

However this custom chunk allocation has several problems.
The most obvious problem is that it will allocate chunk from device offset
0.
Both kernel and normal mkfs will reserve 0~1M range for each device.

This rework will remove all related custom chunk allocation and size
calculation.
Less code to maintain is always a good thing, especially for minor or
less maintained code.

So all --rootdir operation will result the same result as mkfs.btrfs +
mount + cp. (Same result as mkfs.ext4 -d)

Signed-off-by: Qu Wenruo <quwenruo.btrfs@gmx.com>
---
 mkfs/main.c | 236 +++++++++---------------------------------------------------
 1 file changed, 34 insertions(+), 202 deletions(-)

diff --git a/mkfs/main.c b/mkfs/main.c
index 7592c1fb..0ce1ae26 100644
--- a/mkfs/main.c
+++ b/mkfs/main.c
@@ -991,53 +991,6 @@ fail_no_dir:
 	goto out;
 }
 
-static int create_chunks(struct btrfs_trans_handle *trans,
-			 struct btrfs_root *root, u64 num_of_meta_chunks,
-			 u64 size_of_data,
-			 struct mkfs_allocation *allocation)
-{
-	struct btrfs_fs_info *fs_info = root->fs_info;
-	u64 chunk_start;
-	u64 chunk_size;
-	u64 meta_type = BTRFS_BLOCK_GROUP_METADATA;
-	u64 data_type = BTRFS_BLOCK_GROUP_DATA;
-	u64 minimum_data_chunk_size = SZ_8M;
-	u64 i;
-	int ret;
-
-	for (i = 0; i < num_of_meta_chunks; i++) {
-		ret = btrfs_alloc_chunk(trans, fs_info,
-					&chunk_start, &chunk_size, meta_type);
-		if (ret)
-			return ret;
-		ret = btrfs_make_block_group(trans, fs_info, 0,
-					     meta_type, BTRFS_FIRST_CHUNK_TREE_OBJECTID,
-					     chunk_start, chunk_size);
-		allocation->metadata += chunk_size;
-		if (ret)
-			return ret;
-		set_extent_dirty(&root->fs_info->free_space_cache,
-				 chunk_start, chunk_start + chunk_size - 1);
-	}
-
-	if (size_of_data < minimum_data_chunk_size)
-		size_of_data = minimum_data_chunk_size;
-
-	ret = btrfs_alloc_data_chunk(trans, fs_info,
-				     &chunk_start, size_of_data, data_type, 0);
-	if (ret)
-		return ret;
-	ret = btrfs_make_block_group(trans, fs_info, 0,
-				     data_type, BTRFS_FIRST_CHUNK_TREE_OBJECTID,
-				     chunk_start, size_of_data);
-	allocation->data += size_of_data;
-	if (ret)
-		return ret;
-	set_extent_dirty(&root->fs_info->free_space_cache,
-			 chunk_start, chunk_start + size_of_data - 1);
-	return ret;
-}
-
 static int make_image(const char *source_dir, struct btrfs_root *root)
 {
 	int ret;
@@ -1082,86 +1035,6 @@ out:
 	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;
-	u64 location = 0;
-	char buf[4096];
-	int ret = 0, i;
-	ssize_t written;
-
-	memset(buf, 0, 4096);
-	loop_num = size / 4096;
-	for (i = 0; i < loop_num; i++) {
-		written = pwrite64(out_fd, buf, 4096, location);
-		if (written != 4096)
-			ret = -EIO;
-		location += 4096;
-	}
-	return ret;
-}
-
 static int is_ssd(const char *file)
 {
 	blkid_probe probe;
@@ -1432,9 +1305,6 @@ int main(int argc, char **argv)
 	int force_overwrite = 0;
 	char *source_dir = NULL;
 	int source_dir_set = 0;
-	u64 num_of_meta_chunks = 0;
-	u64 size_of_data = 0;
-	u64 source_dir_size = 0;
 	int dev_cnt = 0;
 	int saved_optind;
 	char fs_uuid[BTRFS_UUID_UNPARSED_SIZE] = { 0 };
@@ -1678,51 +1548,29 @@ int main(int argc, char **argv)
 
 	dev_cnt--;
 
-	if (!source_dir_set) {
-		/*
-		 * open without O_EXCL so that the problem should not
-		 * occur by the following processing.
-		 * (btrfs_register_one_device() fails if O_EXCL is on)
-		 */
-		fd = open(file, O_RDWR);
-		if (fd < 0) {
-			error("unable to open %s: %s", file, strerror(errno));
-			goto error;
-		}
-		ret = btrfs_prepare_device(fd, file, &dev_block_count,
-				block_count,
-				(zero_end ? PREP_DEVICE_ZERO_END : 0) |
-				(discard ? PREP_DEVICE_DISCARD : 0) |
-				(verbose ? PREP_DEVICE_VERBOSE : 0));
-		if (ret) {
-			goto error;
-		}
-		if (block_count && block_count > dev_block_count) {
-			error("%s is smaller than requested size, expected %llu, found %llu",
-					file,
-					(unsigned long long)block_count,
-					(unsigned long long)dev_block_count);
-			goto error;
-		}
-	} else {
-		fd = open(file, O_CREAT | O_RDWR,
-				S_IRUSR | S_IWUSR | S_IRGRP | S_IWGRP | S_IROTH);
-		if (fd < 0) {
-			error("unable to open %s: %s", file, strerror(errno));
-			goto error;
-		}
-
-		source_dir_size = size_sourcedir(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);
-		if (ret) {
-			error("unable to zero the output file");
-			goto error;
-		}
-		/* our "device" is the new image file */
-		dev_block_count = block_count;
+	/*
+	 * open without O_EXCL so that the problem should not
+	 * occur by the following processing.
+	 * (btrfs_register_one_device() fails if O_EXCL is on)
+	 */
+	fd = open(file, O_RDWR);
+	if (fd < 0) {
+		error("unable to open %s: %s", file, strerror(errno));
+		goto error;
+	}
+	ret = btrfs_prepare_device(fd, file, &dev_block_count,
+			block_count,
+			(zero_end ? PREP_DEVICE_ZERO_END : 0) |
+			(discard ? PREP_DEVICE_DISCARD : 0) |
+			(verbose ? PREP_DEVICE_VERBOSE : 0));
+	if (ret)
+		goto error;
+	if (block_count && block_count > dev_block_count) {
+		error("%s is smaller than requested size, expected %llu, found %llu",
+			file,
+			(unsigned long long)block_count,
+			(unsigned long long)dev_block_count);
+		goto error;
 	}
 
 	/* To create the first block group and chunk 0 in make_btrfs */
@@ -1848,13 +1696,11 @@ int main(int argc, char **argv)
 	}
 
 raid_groups:
-	if (!source_dir_set) {
-		ret = create_raid_groups(trans, root, data_profile,
+	ret = create_raid_groups(trans, root, data_profile,
 				 metadata_profile, mixed, &allocation);
-		if (ret) {
-			error("unable to create raid groups: %d", ret);
-			goto out;
-		}
+	if (ret) {
+		error("unable to create raid groups: %d", ret);
+		goto out;
 	}
 
 	ret = create_data_reloc_tree(trans, root);
@@ -1869,34 +1715,20 @@ raid_groups:
 		goto out;
 	}
 
-	if (source_dir_set) {
-		trans = btrfs_start_transaction(root, 1);
-		BUG_ON(IS_ERR(trans));
-		ret = create_chunks(trans, root,
-				    num_of_meta_chunks, size_of_data,
-				    &allocation);
-		if (ret) {
-			error("unable to create chunks: %d", ret);
-			goto out;
-		}
-		ret = btrfs_commit_transaction(trans, root);
-		if (ret) {
-			error("transaction commit failed: %d", ret);
-			goto out;
-		}
+	ret = cleanup_temp_chunks(fs_info, &allocation, data_profile,
+				  metadata_profile, metadata_profile);
+	if (ret < 0) {
+		error("failed to cleanup temporary chunks: %d", ret);
+		goto out;
+	}
 
+	if (source_dir_set) {
 		ret = make_image(source_dir, root);
 		if (ret) {
 			error("error wihle filling filesystem: %d", ret);
 			goto out;
 		}
 	}
-	ret = cleanup_temp_chunks(fs_info, &allocation, data_profile,
-				  metadata_profile, metadata_profile);
-	if (ret < 0) {
-		error("failed to cleanup temporary chunks: %d", ret);
-		goto out;
-	}
 
 	if (verbose) {
 		char features_buf[64];
-- 
2.14.1

[PATCH v2 4/7] btrfs-progs: mkfs: Update allocation info before verbose output

[Qu Wenruo]

Since new --rootdir can allocate chunk, it will modify the chunk
allocation result.

This patch will update allocation info before verbose output to reflect
such info.

Signed-off-by: Qu Wenruo <quwenruo.btrfs@gmx.com>
---
 mkfs/main.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/mkfs/main.c b/mkfs/main.c
index 0ce1ae26..6561ac52 100644
--- a/mkfs/main.c
+++ b/mkfs/main.c
@@ -1276,6 +1276,38 @@ out:
 	return ret;
 }
 
+/*
+ * Just update chunk allocation info, since --rootdir may allocate new
+ * chunks which is not updated in @allocation structure.
+ */
+static void update_chunk_allocation(struct btrfs_fs_info *fs_info,
+				    struct mkfs_allocation *allocation)
+{
+	struct btrfs_block_group_cache *bg_cache;
+	u64 mixed_flag = BTRFS_BLOCK_GROUP_DATA | BTRFS_BLOCK_GROUP_METADATA;
+	u64 search_start = 0;
+
+	allocation->mixed = 0;
+	allocation->data = 0;
+	allocation->metadata = 0;
+	allocation->system = 0;
+	while (1) {
+		bg_cache = btrfs_lookup_first_block_group(fs_info,
+							  search_start);
+		if (!bg_cache)
+			break;
+		if ((bg_cache->flags & mixed_flag) == mixed_flag)
+			allocation->mixed += bg_cache->key.offset;
+		else if (bg_cache->flags & BTRFS_BLOCK_GROUP_DATA)
+			allocation->data += bg_cache->key.offset;
+		else if (bg_cache->flags & BTRFS_BLOCK_GROUP_METADATA)
+			allocation->metadata += bg_cache->key.offset;
+		else
+			allocation->system += bg_cache->key.offset;
+		search_start = bg_cache->key.objectid + bg_cache->key.offset;
+	}
+}
+
 int main(int argc, char **argv)
 {
 	char *file;
@@ -1733,6 +1765,7 @@ raid_groups:
 	if (verbose) {
 		char features_buf[64];
 
+		update_chunk_allocation(fs_info, &allocation);
 		printf("Label:              %s\n", label);
 		printf("UUID:               %s\n", mkfs_cfg.fs_uuid);
 		printf("Node size:          %u\n", nodesize);
-- 
2.14.1

[PATCH v2 5/7] btrfs-progs: Avoid BUG_ON for chunk allocation when ENOSPC happens

[Qu Wenruo]

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.

Signed-off-by: Qu Wenruo <quwenruo.btrfs@gmx.com>
---
 extent-tree.c |  3 ++-
 volumes.c     | 18 ++++++++++++++----
 2 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/extent-tree.c b/extent-tree.c
index 525a237e..055582c3 100644
--- a/extent-tree.c
+++ b/extent-tree.c
@@ -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;
diff --git a/volumes.c b/volumes.c
index 2209e5a9..e1ee27d5 100644
--- a/volumes.c
+++ b/volumes.c
@@ -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;
 }
 
 /*
-- 
2.14.1

[PATCH v2 6/7] btrfs-progs: mkfs: Workaround BUG_ON caused by rootdir option

[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.

Signed-off-by: Qu Wenruo <quwenruo.btrfs@gmx.com>
---
 mkfs/main.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/mkfs/main.c b/mkfs/main.c
index 6561ac52..f78c24ce 100644
--- a/mkfs/main.c
+++ b/mkfs/main.c
@@ -1025,6 +1025,18 @@ static int make_image(const char *source_dir, struct btrfs_root *root)
 		printf("Making image is completed.\n");
 	return 0;
 fail:
+	/*
+	 * XXX:
+	 * To avoid BUG_ON() triggered by uncommitted transaction,
+	 * here we must commit transaction before we have proper
+	 * btrfs_abort_transaction() in btrfs-progs.
+	 *
+	 * Don't worry, the magic number is not valid so the fs can't be
+	 * mounted by kernel even we commit the trans.
+	 * And we don't want to pollute the original error, so we ignore
+	 * the return value from btrfs_commit_transaction().
+	 */
+	btrfs_commit_transaction(trans, root);
 	while (!list_empty(&dir_head.list)) {
 		dir_entry = list_entry(dir_head.list.next,
 				       struct directory_name_entry, list);
-- 
2.14.1

[PATCH v2 7/7] btrfs-progs: Doc/mkfs: Add extra condition for rootdir option

[Qu Wenruo]

Add extra limitation explained for --rootdir option, including:
1) Size limitation
   Now I decide to follow "mkfs.ext4 -d" behavior, so we user is
   responsible to make sure the block device/file is large enough.

2) Read permission
   If user can't read the content, mkfs will just fail.
   So user is also responsible to make sure to have enough privilege.

3) Extra warning about the behavior change
   Since we we don't shrink fs the create file image, add such warning
   in documentation.

Signed-off-by: Qu Wenruo <quwenruo.btrfs@gmx.com>
---
 Documentation/mkfs.btrfs.asciidoc | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/mkfs.btrfs.asciidoc b/Documentation/mkfs.btrfs.asciidoc
index d53d9e26..843e25cd 100644
--- a/Documentation/mkfs.btrfs.asciidoc
+++ b/Documentation/mkfs.btrfs.asciidoc
@@ -106,6 +106,17 @@ Please see the mount option 'discard' for that in `btrfs`(5).
 *-r|--rootdir <rootdir>*::
 Populate the toplevel subvolume with files from 'rootdir'.  This does not
 require root permissions and does not mount the filesystem.
++
+With this option, only one device can be specified.
++
+NOTE: User should make sure the block device/file has large enough space to
+contain the source directory and has enough previllege to read source directory.
+Or mkfs will just fail.
++
+WARNING: Before v4.14 btrfs-progs, *--rootdir* will shrink the filesystem,
+prevent user to make use of the remaining space.
+In v4.14 btrfs-progs, this behavior is changed, and will not shrink the fs.
+The result should be the same as `mkfs`, `mount` and then `cp -r`.
 
 *-O|--features <feature1>[,<feature2>...]*::
 A list of filesystem features turned on at mkfs time. Not all features are
-- 
2.14.1

Re: [PATCH v2 7/7] btrfs-progs: Doc/mkfs: Add extra condition for rootdir option

[David Sterba]

On Mon, Sep 11, 2017 at 03:36:12PM +0900, Qu Wenruo wrote:
> Add extra limitation explained for --rootdir option, including:
> 1) Size limitation
>    Now I decide to follow "mkfs.ext4 -d" behavior, so we user is
>    responsible to make sure the block device/file is large enough.

That's fine and I'm ok for changing that to be the default now. But the
shrinking could be still useful, if I read the usecase that Austin
mentioned in the previous thread correctly.

Say I want to prepare a minimal image but will provide a large file at
the beginning because I don't know what's the resulting size going to
be. In this case, something like

$ mkfs.btrfs --rootdir dir/ --minimize image

would make it possible. I'm not sure if somebody hasn't proposed that
already, but this sounds good to me and I think it's a valid usecase.
This means adding back the patch from V1 and then making it optional.

Re: [PATCH v2 7/7] btrfs-progs: Doc/mkfs: Add extra condition for rootdir option

[Goffredo Baroncelli]

On 09/12/2017 07:03 PM, David Sterba wrote:
> Say I want to prepare a minimal image but will provide a large file 
                                                ^^^^^^^^^^^^^^^^^^^^^^

I think that if the target is a file AND --minimize is passed, it is a reasonable expectation that the file is created "on the fly" and grown up to what needed.  

What I mean is that "--minimize" is passed (and a file is passed), mkfs.btrfs should 
a) create the file if it doesn't exist, and avoid any check about its length
b) truncate the file at the end

unfortunately the checks are in more places, so removing these checks is a quite intrusive change...

> at
> the beginning because I don't know what's the resulting size going to
> be. In this case, something like
> 
> $ mkfs.btrfs --rootdir dir/ --minimize image

BR
G.Baroncelli

-- 
gpg @keyserver.linux.it: Goffredo Baroncelli <kreijackATinwind.it>
Key fingerprint BBF5 1610 0B64 DAC6 5F7D  17B2 0EDA 9B37 8B82 E0B5

Re: [PATCH v2 7/7] btrfs-progs: Doc/mkfs: Add extra condition for rootdir option

[David Sterba]

On Tue, Sep 12, 2017 at 07:50:19PM +0200, Goffredo Baroncelli wrote:
> On 09/12/2017 07:03 PM, David Sterba wrote:
> > Say I want to prepare a minimal image but will provide a large file 
>                                                 ^^^^^^^^^^^^^^^^^^^^^^
> 
> I think that if the target is a file AND --minimize is passed, it is a
> reasonable expectation that the file is created "on the fly" and grown
> up to what needed.  
> 
> What I mean is that "--minimize" is passed (and a file is passed), mkfs.btrfs should 
> a) create the file if it doesn't exist, and avoid any check about its length
> b) truncate the file at the end

I think the farthest-offset write will extend the file size, so we
should not truncate it, in the sense 'make it smaller than it is', but
rather 'align it to the size where the filesystem expects live data".

Eg. if we write a blockgroup header to the beginning of 256MB, but fill
only a few kilobytes, effectively writing to some range, say, [0..32MB].
Then reads past the 32MB (eg. through loop device) would fail.

As long as the file is zero, minimizing will be implicit. My example
with the provided large file was wrong in retrospect. I'll think about
that more.

Re: [PATCH v2 7/7] btrfs-progs: Doc/mkfs: Add extra condition for rootdir option

[Qu Wenruo]

On 2017年09月13日 02:07, David Sterba wrote:
> On Tue, Sep 12, 2017 at 07:50:19PM +0200, Goffredo Baroncelli wrote:
>> On 09/12/2017 07:03 PM, David Sterba wrote:
>>> Say I want to prepare a minimal image but will provide a large file
>>                                                  ^^^^^^^^^^^^^^^^^^^^^^
>>
>> I think that if the target is a file AND --minimize is passed, it is a
>> reasonable expectation that the file is created "on the fly" and grown
>> up to what needed.
>>
>> What I mean is that "--minimize" is passed (and a file is passed), mkfs.btrfs should
>> a) create the file if it doesn't exist, and avoid any check about its length
>> b) truncate the file at the end
> 
> I think the farthest-offset write will extend the file size, so we
> should not truncate it, in the sense 'make it smaller than it is', but
> rather 'align it to the size where the filesystem expects live data".

Unfortunately, that's not the case.

Current implementation must determine its size at the very beginning.

That's to say, if mkfs determines that it's going to use 1G size, then 
current btrfs-progs will be guaranteed that no write over 1G boundary.
(Unless there is some hidden bug)

So if we are realling going to implement "--minimize" option, we are 
doing asymmetric behavior:

1) For fs smaller than file
    Good, we truncate it

2) For fs larger than file
    Bad, we will get ENOSPC error.

One workaround I mentioned in V1 is to make a large enough sparse file 
first and then truncate it.

But such workaround has some problem, especially related to the chunk 
allocator.

For a 1G sparse file as example, we will have about 100M data chunk.
While for 128M space file, we will have about 10M data chunk.
That will cause a lot of space wasted if using large spare file and then 
truncating it than using a small existing file.

Considering the all the variables involved, I choose the KISS method.
Let user to handle it, and mkfs.btrfs --rootdir will just do the copy work.

If user really wants to do a minimal image, the best (AFAIK the only 
correct) method is to implement btrfs-progs balance (offline balanace) 
to compact all these meta and data, then truncate it (offline resize).

The initial --rootdir is doing too many things at the same time in mkfs.
And that's why I want to make it simple.

BTW, just as I stated in cover letter, "mkfs.ext4" -d is doing it easy 
as what I did here, and the complex work is handled by out of e2fsprogs 
utility, genext2fs.



And there may be some use case relying on this truncation, but hey, if 
the need is really high, there should already be a lot of complains 
about --rootdir shrinking fs, and more tested/documented --rootdir 
implementation.

So it's not really worth it to implement a complex code base to handle 
something while there is not much user base.

Thanks,
Qu

> 
> Eg. if we write a blockgroup header to the beginning of 256MB, but fill
> only a few kilobytes, effectively writing to some range, say, [0..32MB].
> Then reads past the 32MB (eg. through loop device) would fail.
> 
> As long as the file is zero, minimizing will be implicit. My example
> with the provided large file was wrong in retrospect. I'll think about
> that more.
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH v2 7/7] btrfs-progs: Doc/mkfs: Add extra condition for rootdir option

[David Sterba]

On Wed, Sep 13, 2017 at 08:32:50AM +0800, Qu Wenruo wrote:
> On 2017年09月13日 02:07, David Sterba wrote:
> > On Tue, Sep 12, 2017 at 07:50:19PM +0200, Goffredo Baroncelli wrote:
> >> On 09/12/2017 07:03 PM, David Sterba wrote:
> >>> Say I want to prepare a minimal image but will provide a large file
> >>                                                  ^^^^^^^^^^^^^^^^^^^^^^
> >>
> >> I think that if the target is a file AND --minimize is passed, it is a
> >> reasonable expectation that the file is created "on the fly" and grown
> >> up to what needed.
> >>
> >> What I mean is that "--minimize" is passed (and a file is passed), mkfs.btrfs should
> >> a) create the file if it doesn't exist, and avoid any check about its length
> >> b) truncate the file at the end
> > 
> > I think the farthest-offset write will extend the file size, so we
> > should not truncate it, in the sense 'make it smaller than it is', but
> > rather 'align it to the size where the filesystem expects live data".
> 
> Unfortunately, that's not the case.
> 
> Current implementation must determine its size at the very beginning.
> 
> That's to say, if mkfs determines that it's going to use 1G size, then 
> current btrfs-progs will be guaranteed that no write over 1G boundary.
> (Unless there is some hidden bug)
> 
> So if we are realling going to implement "--minimize" option, we are 
> doing asymmetric behavior:
> 
> 1) For fs smaller than file
>     Good, we truncate it
> 
> 2) For fs larger than file
>     Bad, we will get ENOSPC error.
> 
> One workaround I mentioned in V1 is to make a large enough sparse file 
> first and then truncate it.
> 
> But such workaround has some problem, especially related to the chunk 
> allocator.
> 
> For a 1G sparse file as example, we will have about 100M data chunk.
> While for 128M space file, we will have about 10M data chunk.
> That will cause a lot of space wasted if using large spare file and then 
> truncating it than using a small existing file.
> 
> Considering the all the variables involved, I choose the KISS method.
> Let user to handle it, and mkfs.btrfs --rootdir will just do the copy work.
> 
> If user really wants to do a minimal image, the best (AFAIK the only 
> correct) method is to implement btrfs-progs balance (offline balanace) 
> to compact all these meta and data, then truncate it (offline resize).
> 
> The initial --rootdir is doing too many things at the same time in mkfs.
> And that's why I want to make it simple.
> 
> BTW, just as I stated in cover letter, "mkfs.ext4" -d is doing it easy 
> as what I did here, and the complex work is handled by out of e2fsprogs 
> utility, genext2fs.
> 
> 
> 
> And there may be some use case relying on this truncation, but hey, if 
> the need is really high, there should already be a lot of complains 
> about --rootdir shrinking fs, and more tested/documented --rootdir 
> implementation.
> 
> So it's not really worth it to implement a complex code base to handle 
> something while there is not much user base.

I think all points have been covered, thanks.

The backward compatibility does not seem to be broken, the difference
between old and new --rootdir is only the size of the resulting
filesystem, otherwise the file size is left untouched and sufficient
size is required.

Minimizing can come as a separate step, either using off-line balance,
or possibly also as an option to mkfs.

I'm going to review & merge this series to devel. Tests and
documentation should be updated to make the usecase clear.

Re: [PATCH v2 7/7] btrfs-progs: Doc/mkfs: Add extra condition for rootdir option

[Qu Wenruo]

On 2017年09月15日 20:56, David Sterba wrote:
> On Wed, Sep 13, 2017 at 08:32:50AM +0800, Qu Wenruo wrote:
>> On 2017年09月13日 02:07, David Sterba wrote:
>>> On Tue, Sep 12, 2017 at 07:50:19PM +0200, Goffredo Baroncelli wrote:
>>>> On 09/12/2017 07:03 PM, David Sterba wrote:
>>>>> Say I want to prepare a minimal image but will provide a large file
>>>>                                                   ^^^^^^^^^^^^^^^^^^^^^^
>>>>
>>>> I think that if the target is a file AND --minimize is passed, it is a
>>>> reasonable expectation that the file is created "on the fly" and grown
>>>> up to what needed.
>>>>
>>>> What I mean is that "--minimize" is passed (and a file is passed), mkfs.btrfs should
>>>> a) create the file if it doesn't exist, and avoid any check about its length
>>>> b) truncate the file at the end
>>>
>>> I think the farthest-offset write will extend the file size, so we
>>> should not truncate it, in the sense 'make it smaller than it is', but
>>> rather 'align it to the size where the filesystem expects live data".
>>
>> Unfortunately, that's not the case.
>>
>> Current implementation must determine its size at the very beginning.
>>
>> That's to say, if mkfs determines that it's going to use 1G size, then
>> current btrfs-progs will be guaranteed that no write over 1G boundary.
>> (Unless there is some hidden bug)
>>
>> So if we are realling going to implement "--minimize" option, we are
>> doing asymmetric behavior:
>>
>> 1) For fs smaller than file
>>      Good, we truncate it
>>
>> 2) For fs larger than file
>>      Bad, we will get ENOSPC error.
>>
>> One workaround I mentioned in V1 is to make a large enough sparse file
>> first and then truncate it.
>>
>> But such workaround has some problem, especially related to the chunk
>> allocator.
>>
>> For a 1G sparse file as example, we will have about 100M data chunk.
>> While for 128M space file, we will have about 10M data chunk.
>> That will cause a lot of space wasted if using large spare file and then
>> truncating it than using a small existing file.
>>
>> Considering the all the variables involved, I choose the KISS method.
>> Let user to handle it, and mkfs.btrfs --rootdir will just do the copy work.
>>
>> If user really wants to do a minimal image, the best (AFAIK the only
>> correct) method is to implement btrfs-progs balance (offline balanace)
>> to compact all these meta and data, then truncate it (offline resize).
>>
>> The initial --rootdir is doing too many things at the same time in mkfs.
>> And that's why I want to make it simple.
>>
>> BTW, just as I stated in cover letter, "mkfs.ext4" -d is doing it easy
>> as what I did here, and the complex work is handled by out of e2fsprogs
>> utility, genext2fs.
>>
>>
>>
>> And there may be some use case relying on this truncation, but hey, if
>> the need is really high, there should already be a lot of complains
>> about --rootdir shrinking fs, and more tested/documented --rootdir
>> implementation.
>>
>> So it's not really worth it to implement a complex code base to handle
>> something while there is not much user base.
> 
> I think all points have been covered, thanks.
> 
> The backward compatibility does not seem to be broken, the difference
> between old and new --rootdir is only the size of the resulting
> filesystem, otherwise the file size is left untouched and sufficient
> size is required.
> 
> Minimizing can come as a separate step, either using off-line balance,
> or possibly also as an option to mkfs.

Glad to hear that.

> 
> I'm going to review & merge this series to devel. Tests and
> documentation should be updated to make the usecase clear.

I'm happy to address any comment, both code and doc/test.

Thanks,
Qu

Re: [PATCH v2 7/7] btrfs-progs: Doc/mkfs: Add extra condition for rootdir option

[David Sterba]

On Fri, Sep 15, 2017 at 09:24:19PM +0800, Qu Wenruo wrote:
> > I'm going to review & merge this series to devel. Tests and
> > documentation should be updated to make the usecase clear.
> 
> I'm happy to address any comment, both code and doc/test.

For the tests I'd like to see:

* with file target, try non-existent file, zero-length file, too-small
  file and large-enough

* for block device target, I think loop device would do, too-small and
  large-enough

* after the test, verify that the files have same size and contents

Re: [PATCH v2 7/7] btrfs-progs: Doc/mkfs: Add extra condition for rootdir option

[Qu Wenruo]

On 2017年09月15日 23:48, David Sterba wrote:
> On Fri, Sep 15, 2017 at 09:24:19PM +0800, Qu Wenruo wrote:
>>> I'm going to review & merge this series to devel. Tests and
>>> documentation should be updated to make the usecase clear.
>>
>> I'm happy to address any comment, both code and doc/test.
> 
> For the tests I'd like to see:
> 
> * with file target, try non-existent file, zero-length file, too-small
>    file and large-enough

No problem.

But it would be better to define the expected behavior first.
I'll show the behavior difference below to see if you're OK with it.

For non-existent file:
New: Fail, no such file at the very beginning
Old: Create new one

Zero-length:
New: Fail, too small file at the very beginning
Old: Expand file and continue mkfs

Too-small:
New: Fail, showing -28 errno half way
Old: Same as zero length.

Large-enough:
New: Success, without shrink
Old: Success, shrink

> 
> * for block device target, I think loop device would do, too-small and
>    large-enough

Same as above too-small and large-enough case.

> 
> * after the test, verify that the files have same size and contents

I'll reuse convert facilities to do that.

And I'll add one extra test case:
No enough privilege to read some file in the directory:
New: Fail gracefully
Old: Fail with BUG_ON.

If you're OK with the behavior difference, I'll create new test cases 
for mkfs --rootdir.

Thanks,
Qu