[PATCH v3 00/14] Mkfs: Rework --rootdir to a more generic behavior

[PATCH v3 00/14] Mkfs: Rework --rootdir to a more generic behavior

[Qu Wenruo]

The patchset can be fetched from github:
https://github.com/adam900710/btrfs-progs/tree/mkfs_rootdir_rework

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)

Also, documentation and test cases (especially test cases) are also
enhanced.
Documentation has extra explanation for the behavior change, and test
cases are following convert tests, testing all possible feature and
nodesize combinations.

Patch 1~6 change the rootdir behavior.
Patch 7 enhances the documentation.
Patch 8~14 enhances the mkfs test cases, with one bug fix exposed by new
test cases.

Changelog:
  v2:
    Follows the "mkfs.ext4 -d" behavior, which will not create the file
    for non-existent destination, nor shrink the fs.
    Add extra explanation for the behavior change.

  v3:
    Make convert tests facility more flex to handle the test pattern of
    "--rootdir".
    Introduce new mkfs test cases to both check file content and error
    handlers.
    Slightly enhance the documentation explanation of --rootdir.
    Since the main change is test case, patch 1~5 are not modified.

Qu Wenruo (14):
  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
  btrfs-progs: tests/common: Split user xattr into its own branch for
    generate_dataset
  btrfs-progs: tests/common: Introduce optional parameter to specify
    destination directory for generate_dataset
  btrfs-progs: tests/common: Make checksum, permission and acl check
    path independent
  btrfs-progs: tests/mkfs: Add basic test case for rootdir parameter
  btrfs-progs: tests/common: Detect ungraceful failure case
  btrfs-progs: mkfs: Fix overwritten return value for mkfs
  btrfs-progs: tests/mkfs: Check error handler for rootdir parameter

 Documentation/mkfs.btrfs.asciidoc                  |  13 +
 extent-tree.c                                      |   5 +-
 mkfs/main.c                                        | 286 ++++++---------------
 tests/common                                       |  17 +-
 tests/common.convert                               |  93 ++++++-
 tests/mkfs-tests/010-basic-rootdir/test.sh         |  78 ++++++
 .../mkfs-tests/011-rootdir-fail-condition/test.sh  |  82 ++++++
 volumes.c                                          |  32 ++-
 8 files changed, 377 insertions(+), 229 deletions(-)
 create mode 100755 tests/mkfs-tests/010-basic-rootdir/test.sh
 create mode 100755 tests/mkfs-tests/011-rootdir-fail-condition/test.sh

-- 
2.14.1

[PATCH v3 01/14] 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 v3 02/14] 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 v3 03/14] 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 v3 04/14] 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 v3 05/14] 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 v3 06/14] 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 v3 07/14] 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 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 | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/Documentation/mkfs.btrfs.asciidoc b/Documentation/mkfs.btrfs.asciidoc
index d53d9e26..645a2881 100644
--- a/Documentation/mkfs.btrfs.asciidoc
+++ b/Documentation/mkfs.btrfs.asciidoc
@@ -106,6 +106,19 @@ 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`. +
+Also, if destination file/block device does not exist, *--rootdir* will not
+create the image file, to make it follow the normal mkfs behavior.
 
 *-O|--features <feature1>[,<feature2>...]*::
 A list of filesystem features turned on at mkfs time. Not all features are
-- 
2.14.1

Re: [PATCH v3 07/14] btrfs-progs: Doc/mkfs: Add extra condition for rootdir option

[Anand Jain]

> +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`. +

  Hmm well. Shrink to fit exactly to the size of the given 
files-and-directory is indeed a nice feature. Which would help to create 
a golden-image btrfs seed device. Its not popular as of now, but at some 
point it may in the cloud environment.

  Replacing this feature instead of creating a new option is not a good 
idea indeed. I missed something ?

Thanks, Anand

> +Also, if destination file/block device does not exist, *--rootdir* will not
> +create the image file, to make it follow the normal mkfs behavior.

Re: [PATCH v3 07/14] btrfs-progs: Doc/mkfs: Add extra condition for rootdir option

[Qu Wenruo]

As I already stated in an other thread, if you want to shrink, do it in another command line tool.
Do one thing and do it simple. (Although Btrfs itself is already out of the UNIX way)

It may be offline shrink/balance. But not to further complexing the --rootdir option now.

And you also said that, the shrink feature is not a popular feature *NOW*, then I don't think it's worthy to implment it *NOW* either.
Implement future feature in the future please.

And further more, even following the existing shrink behavior, you still need to truncate the file all by yourself.
Which is no better than creating a good sized file and then mkfs on it.

Thanks,
Qu

Sent: Friday, September 22, 2017 at 5:24 PM
From: "Anand Jain" <anand.jain@oracle.com>
To: "Qu Wenruo" <quwenruo.btrfs@gmx.com>, linux-btrfs@vger.kernel.org
Cc: dsterba@suse.cz
Subject: Re: [PATCH v3 07/14] btrfs-progs: Doc/mkfs: Add extra condition for rootdir option

> +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`. +

Hmm well. Shrink to fit exactly to the size of the given
files-and-directory is indeed a nice feature. Which would help to create
a golden-image btrfs seed device. Its not popular as of now, but at some
point it may in the cloud environment.

Replacing this feature instead of creating a new option is not a good
idea indeed. I missed something ?

Thanks, Anand

> +Also, if destination file/block device does not exist, *--rootdir* will not
> +create the image file, to make it follow the normal mkfs behavior.


--
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 v3 07/14] btrfs-progs: Doc/mkfs: Add extra condition for rootdir option

[Austin S. Hemmelgarn]

On 2017-09-22 06:39, Qu Wenruo wrote:
> As I already stated in an other thread, if you want to shrink, do it in another command line tool.
> Do one thing and do it simple. (Although Btrfs itself is already out of the UNIX way)
Unless I'm reading the code wrong, the shrinking isn't happening in a 
second pass, so this _is_ doing one thing, and it appears to be doing it 
as simply as possible (although arguably not correctly because of the 
1MB reserved area being used).
> 
> It may be offline shrink/balance. But not to further complexing the --rootdir option now. >
> And you also said that, the shrink feature is not a popular feature *NOW*, then I don't think it's worthy to implment it *NOW* either.
> Implement future feature in the future please.
I'm not sure about you, but I could have sworn that he meant seed 
devices weren't a popular feature right now, not that the shrinking is. 
As a general rule, the whole option of pre-loading a filesystem with 
data as you're creating it is not a popular feature, because most 
sysadmins are much more willing to trust adding data after the 
filesystem is created.

Personally, given the existence of seed devices, I would absolutely 
expect there to be a quick and easy way to generate a minimalistic image 
using a single command (because realistic usage of seed devices implies 
minimalistic images).  I agree that it shouldn't be the default 
behavior, but I don't think it needs to be removed completely.  The main 
issues here are that it wasn't documented well (like many other things 
in BTRFS), and it didn't generate a filesystem that was properly 
compliant with the on-disk format (because it used space in the 1MB 
reserved area at the beginning of the FS).  Fixing those issues in no 
way requires removing the feature.
> 
> And further more, even following the existing shrink behavior, you still need to truncate the file all by yourself.
> Which is no better than creating a good sized file and then mkfs on it.
Only if you pre-create the file.  If the file doesn't exist, it gets 
created at the appropriate size.  That's part of why the chunk 
allocations are screwed up and stuff gets put in the first 1MB, it 
generates the FS on-the-fly and writes it out as it's generating it.
> 
> Thanks,
> Qu
> 
> Sent: Friday, September 22, 2017 at 5:24 PM
> From: "Anand Jain" <anand.jain@oracle.com>
> To: "Qu Wenruo" <quwenruo.btrfs@gmx.com>, linux-btrfs@vger.kernel.org
> Cc: dsterba@suse.cz
> Subject: Re: [PATCH v3 07/14] btrfs-progs: Doc/mkfs: Add extra condition for rootdir option
> 
>> +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`. +
> 
> Hmm well. Shrink to fit exactly to the size of the given
> files-and-directory is indeed a nice feature. Which would help to create
> a golden-image btrfs seed device. Its not popular as of now, but at some
> point it may in the cloud environment.
> 
> Replacing this feature instead of creating a new option is not a good
> idea indeed. I missed something ?
> 
> Thanks, Anand
> 
>> +Also, if destination file/block device does not exist, *--rootdir* will not
>> +create the image file, to make it follow the normal mkfs behavior.

Re: [PATCH v3 07/14] btrfs-progs: Doc/mkfs: Add extra condition for rootdir option

[Qu Wenruo]

On 2017年09月22日 19:38, Austin S. Hemmelgarn wrote:
> On 2017-09-22 06:39, Qu Wenruo wrote:
>> As I already stated in an other thread, if you want to shrink, do it 
>> in another command line tool.
>> Do one thing and do it simple. (Although Btrfs itself is already out 
>> of the UNIX way)
> Unless I'm reading the code wrong, the shrinking isn't happening in a 
> second pass, so this _is_ doing one thing, and it appears to be doing it 
> as simply as possible (although arguably not correctly because of the 
> 1MB reserved area being used).

If you're referring to my V1 implementation of shrink, that's doing 
*one* thing.

But the original shrinking code? Nope, or we won't have the custom chunk 
allocator at all.


What I really mean is, if one wants to shrink, at least don't couple the 
shrink code into "mkfs.btrfs".

Do shrink in its own tool/subcommand, not in a really unrelated tool.

>>
>> It may be offline shrink/balance. But not to further complexing the 
>> --rootdir option now. >
>> And you also said that, the shrink feature is not a popular feature 
>> *NOW*, then I don't think it's worthy to implment it *NOW* either.
>> Implement future feature in the future please.
> I'm not sure about you, but I could have sworn that he meant seed 
> devices weren't a popular feature right now,

Oh, sorry for my misunderstanding.

> not that the shrinking is. 
> As a general rule, the whole option of pre-loading a filesystem with 
> data as you're creating it is not a popular feature, because most 
> sysadmins are much more willing to trust adding data after the 
> filesystem is created.
> 
> Personally, given the existence of seed devices, I would absolutely 
> expect there to be a quick and easy way to generate a minimalistic image 
> using a single command (because realistic usage of seed devices implies 
> minimalistic images).  I agree that it shouldn't be the default 
> behavior, but I don't think it needs to be removed completely.

Just like I said in cover letter, even for ext*, it's provided by 
genext2fs, not mke2fs.

I totally understand end-user really want a do-it-all solution.
But from developers' view, the old UNIX way is better to maintain code 
clean and easy to read.


In fact, you can even create your script to do the old behavior if you 
don't care that the result may not fully take use of the space, just by:

1) Calculate the size of the whole directory
    "du" command can do it easily, and it does things better than us! For
    years!

2) Multiple the value according to the meta/data profile
    Take care of small files, which will be inlined.
    And don't forget size for data checksum.
    (BTW, there is no way to change the behavor of inlined data and data
     checksum for mkfs. unlike btrfs-convert)


3) Create a file with size calculated by step 2)

4) Execute "mkfs.btrfs -d <dir> <created file>"

>  The main 
> issues here are that it wasn't documented well (like many other things 
> in BTRFS), and it didn't generate a filesystem that was properly 
> compliant with the on-disk format (because it used space in the 1MB 
> reserved area at the beginning of the FS).  Fixing those issues in no 
> way requires removing the feature.

Yes, 1MB can be fixed easily (although not properly). But the whole 
customized chunk allocator is the real problem.
The almost dead code is always bug-prone. Replace it with updated 
generic chunk allocator is the way to avoid later whac-a-mole, and 
should be done asap.

>>
>> And further more, even following the existing shrink behavior, you 
>> still need to truncate the file all by yourself.
>> Which is no better than creating a good sized file and then mkfs on it.
> Only if you pre-create the file.  If the file doesn't exist, it gets 
> created at the appropriate size.  That's part of why the chunk 
> allocations are screwed up and stuff gets put in the first 1MB, it 
> generates the FS on-the-fly and writes it out as it's generating it.

Nope, even you created the file in advance, it will still occupy the 
first 1M.

BTW, you can get back the size calculation for shrink, but you will soon 
find that it's just the start of a new nightmare.
Because there is no easy way to calculate the real metadata usage.

The result (and the old calculator) will be no better than guessing it.
(Well, just multiply the dir size by 2 will never go wrong)


Thanks,
Qu

>>
>> Thanks,
>> Qu
>>
>> Sent: Friday, September 22, 2017 at 5:24 PM
>> From: "Anand Jain" <anand.jain@oracle.com>
>> To: "Qu Wenruo" <quwenruo.btrfs@gmx.com>, linux-btrfs@vger.kernel.org
>> Cc: dsterba@suse.cz
>> Subject: Re: [PATCH v3 07/14] btrfs-progs: Doc/mkfs: Add extra 
>> condition for rootdir option
>>
>>> +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`. +
>>
>> Hmm well. Shrink to fit exactly to the size of the given
>> files-and-directory is indeed a nice feature. Which would help to create
>> a golden-image btrfs seed device. Its not popular as of now, but at some
>> point it may in the cloud environment.
>>
>> Replacing this feature instead of creating a new option is not a good
>> idea indeed. I missed something ?
>>
>> Thanks, Anand
>>
>>> +Also, if destination file/block device does not exist, *--rootdir* 
>>> will not
>>> +create the image file, to make it follow the normal mkfs behavior.
> 
> -- 
> 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 v3 07/14] btrfs-progs: Doc/mkfs: Add extra condition for rootdir option

[Austin S. Hemmelgarn]

On 2017-09-22 08:32, Qu Wenruo wrote:
> On 2017年09月22日 19:38, Austin S. Hemmelgarn wrote:
>> On 2017-09-22 06:39, Qu Wenruo wrote:
>>> As I already stated in an other thread, if you want to shrink, do it 
>>> in another command line tool.
>>> Do one thing and do it simple. (Although Btrfs itself is already out 
>>> of the UNIX way)
>> Unless I'm reading the code wrong, the shrinking isn't happening in a 
>> second pass, so this _is_ doing one thing, and it appears to be doing 
>> it as simply as possible (although arguably not correctly because of 
>> the 1MB reserved area being used).
> 
> If you're referring to my V1 implementation of shrink, that's doing 
> *one* thing.
> 
> But the original shrinking code? Nope, or we won't have the custom chunk 
> allocator at all.
> 
> What I really mean is, if one wants to shrink, at least don't couple the 
> shrink code into "mkfs.btrfs".
> 
> Do shrink in its own tool/subcommand, not in a really unrelated tool.
There are two cases for shrinking a filesystem:
1. You're resizing it to move to a smaller disk (or speed up copying to 
another disk).
2. You're generating a filesystem image that needs to be as small as 
possible.

Case 1 is obviously unrelated to creating a filesystem.  Case 2 however 
is kind of integral to the creation of the filesystem image itself by 
definition, especially for a CoW filesystem because it's not possible to 
shrink to the absolute smallest size due to the GlobalReserve and other 
things.

Similarly, there are two primary use cases for pre-loading the 
filesystem with data:
1. Avoiding a copy when reprovisioning storage on a system.  For 
example, splitting a directory out to a new filesystem, you could use 
the -r option to avoid having to copy the data after mounting the 
filesystem.
2. Creating base images for systems.

The first case shouldn't need the shrinking functionality, but the 
second is a very common use case together with the second usage for 
shrinking a filesystem.
> 
>>>
>>> It may be offline shrink/balance. But not to further complexing the 
>>> --rootdir option now. >
>>> And you also said that, the shrink feature is not a popular feature 
>>> *NOW*, then I don't think it's worthy to implment it *NOW* either.
>>> Implement future feature in the future please.
>> I'm not sure about you, but I could have sworn that he meant seed 
>> devices weren't a popular feature right now,
> 
> Oh, sorry for my misunderstanding.
> 
>> not that the shrinking is. As a general rule, the whole option of 
>> pre-loading a filesystem with data as you're creating it is not a 
>> popular feature, because most sysadmins are much more willing to trust 
>> adding data after the filesystem is created.
>>
>> Personally, given the existence of seed devices, I would absolutely 
>> expect there to be a quick and easy way to generate a minimalistic 
>> image using a single command (because realistic usage of seed devices 
>> implies minimalistic images).  I agree that it shouldn't be the 
>> default behavior, but I don't think it needs to be removed completely.
> 
> Just like I said in cover letter, even for ext*, it's provided by 
> genext2fs, not mke2fs.
Then maybe this should get split out into a separate tool instead of 
just removing it completely?  There is obviously at least some interest 
in this functionality.
> 
> I totally understand end-user really want a do-it-all solution.
> But from developers' view, the old UNIX way is better to maintain code 
> clean and easy to read.
What the code is doing should have near zero impact on readability.  If 
it did, then the BTRFS code in general is already way beyond most people.
> 
> 
> In fact, you can even create your script to do the old behavior if you 
> don't care that the result may not fully take use of the space, just by:
> 
> 1) Calculate the size of the whole directory
>     "du" command can do it easily, and it does things better than us! For
>     years!
Um, no it actually doesn't do things better in all cases.  it doesn't 
account for extended attributes, or metadata usage, or any number of 
other things that factor into how much space a file or directory will 
take up on BTRFS.  It's good enough for finding what's using most of 
your space, but it's not reliable for determining how much space you 
need to store that data (especially once you throw in in-line compression).
> 
> 2) Multiple the value according to the meta/data profile
>     Take care of small files, which will be inlined.
>     And don't forget size for data checksum.
>     (BTW, there is no way to change the behavor of inlined data and data
>      checksum for mkfs. unlike btrfs-convert)
This is where the issue lies.  It's not possible for a person to 
calculate this with reasonable accuracy, and you arguably can't even do 
it for certain programmatically without some serious work.
> 
> 3) Create a file with size calculated by step 2)
> 
> 4) Execute "mkfs.btrfs -d <dir> <created file>"
> 
>>   The main issues here are that it wasn't documented well (like many 
>> other things in BTRFS), and it didn't generate a filesystem that was 
>> properly compliant with the on-disk format (because it used space in 
>> the 1MB reserved area at the beginning of the FS).  Fixing those 
>> issues in no way requires removing the feature.
> 
> Yes, 1MB can be fixed easily (although not properly). But the whole 
> customized chunk allocator is the real problem.
> The almost dead code is always bug-prone. Replace it with updated 
> generic chunk allocator is the way to avoid later whac-a-mole, and 
> should be done asap.
Agreed, but that doesn't preclude having the option to keep the 
generated image to the minimum size.
> 
>>>
>>> And further more, even following the existing shrink behavior, you 
>>> still need to truncate the file all by yourself.
>>> Which is no better than creating a good sized file and then mkfs on it.
>> Only if you pre-create the file.  If the file doesn't exist, it gets 
>> created at the appropriate size.  That's part of why the chunk 
>> allocations are screwed up and stuff gets put in the first 1MB, it 
>> generates the FS on-the-fly and writes it out as it's generating it.
> 
> Nope, even you created the file in advance, it will still occupy the 
> first 1M.
Because it doesn't assume that the file is there to begin with.  It's 
not trying O_CREAT and falling back to some different code if that 
fails.  The code assumes that the file won't be there, and handles 
things accordingly albeit incorrectly (it should seek past the first 
1MB, write the initial SB, and then start chunk allocation).  IOW, the 
code takes a shortcut in that it doesn't check for the file, and the 
rest is written to account for that by assuming there wasn't a file. 
The lack of truncation just means it doesn't try to trim things down by 
itself if the file is already there (it assumes that you knew what you 
were doing).

Put differently, I'm fairly certain that the current -r option removes 
the total size check unless the target is a device (although it may 
remove the check there too and just fail when it tries to write past the 
end of the device), and will thus extend existing files to the required 
size to hold the data.
> 
> BTW, you can get back the size calculation for shrink, but you will soon 
> find that it's just the start of a new nightmare.
> Because there is no easy way to calculate the real metadata usage.
> 
> The result (and the old calculator) will be no better than guessing it.
> (Well, just multiply the dir size by 2 will never go wrong)
No, it can go wrong depending on what you count as part of the size.
> 
> 
> Thanks,
> Qu
> 
>>>
>>> Thanks,
>>> Qu
>>>
>>> Sent: Friday, September 22, 2017 at 5:24 PM
>>> From: "Anand Jain" <anand.jain@oracle.com>
>>> To: "Qu Wenruo" <quwenruo.btrfs@gmx.com>, linux-btrfs@vger.kernel.org
>>> Cc: dsterba@suse.cz
>>> Subject: Re: [PATCH v3 07/14] btrfs-progs: Doc/mkfs: Add extra 
>>> condition for rootdir option
>>>
>>>> +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`. +
>>>
>>> Hmm well. Shrink to fit exactly to the size of the given
>>> files-and-directory is indeed a nice feature. Which would help to create
>>> a golden-image btrfs seed device. Its not popular as of now, but at some
>>> point it may in the cloud environment.
>>>
>>> Replacing this feature instead of creating a new option is not a good
>>> idea indeed. I missed something ?
>>>
>>> Thanks, Anand
>>>
>>>> +Also, if destination file/block device does not exist, *--rootdir* 
>>>> will not
>>>> +create the image file, to make it follow the normal mkfs behavior.
>>
>> -- 
>> 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 v3 07/14] btrfs-progs: Doc/mkfs: Add extra condition for rootdir option

[Qu Wenruo]

On 2017年09月22日 21:33, Austin S. Hemmelgarn wrote:
> On 2017-09-22 08:32, Qu Wenruo wrote:
>> On 2017年09月22日 19:38, Austin S. Hemmelgarn wrote:
>>> On 2017-09-22 06:39, Qu Wenruo wrote:
>>>> As I already stated in an other thread, if you want to shrink, do it 
>>>> in another command line tool.
>>>> Do one thing and do it simple. (Although Btrfs itself is already out 
>>>> of the UNIX way)
>>> Unless I'm reading the code wrong, the shrinking isn't happening in a 
>>> second pass, so this _is_ doing one thing, and it appears to be doing 
>>> it as simply as possible (although arguably not correctly because of 
>>> the 1MB reserved area being used).
>>
>> If you're referring to my V1 implementation of shrink, that's doing 
>> *one* thing.
>>
>> But the original shrinking code? Nope, or we won't have the custom 
>> chunk allocator at all.
>>
>> What I really mean is, if one wants to shrink, at least don't couple 
>> the shrink code into "mkfs.btrfs".
>>
>> Do shrink in its own tool/subcommand, not in a really unrelated tool.
> There are two cases for shrinking a filesystem:
> 1. You're resizing it to move to a smaller disk (or speed up copying to 
> another disk).
> 2. You're generating a filesystem image that needs to be as small as 
> possible.

I would argue there is no meaning of creating *smallest* image. (Of 
course it exists)

We could put tons of code to implement, and more (or less) test cases to 
verify it.

But the demand doesn't validate the effort.

All my points are clear for this patchset:
I know I removed one function, and my reason is:
1) No or little usage
    And it's anti intuition.
2) Dead code (not tested nor well documented)
3) Possible workaround

I can add several extra reasons as I stated before, but number of 
reasons won't validate anything anyway.

Building software is always trading one thing for another.
I understand there may be some need for this function, but it doesn't 
validate the cost.

And I think the fact that until recently a mail reported about the 
shrinking behavior has already backed up my point.

Thanks,
Qu

> 
> Case 1 is obviously unrelated to creating a filesystem.  Case 2 however 
> is kind of integral to the creation of the filesystem image itself by 
> definition, especially for a CoW filesystem because it's not possible to 
> shrink to the absolute smallest size due to the GlobalReserve and other 
> things.
> 
> Similarly, there are two primary use cases for pre-loading the 
> filesystem with data:
> 1. Avoiding a copy when reprovisioning storage on a system.  For 
> example, splitting a directory out to a new filesystem, you could use 
> the -r option to avoid having to copy the data after mounting the 
> filesystem.
> 2. Creating base images for systems.
> 
> The first case shouldn't need the shrinking functionality, but the 
> second is a very common use case together with the second usage for 
> shrinking a filesystem.
>>
>>>>
>>>> It may be offline shrink/balance. But not to further complexing the 
>>>> --rootdir option now. >
>>>> And you also said that, the shrink feature is not a popular feature 
>>>> *NOW*, then I don't think it's worthy to implment it *NOW* either.
>>>> Implement future feature in the future please.
>>> I'm not sure about you, but I could have sworn that he meant seed 
>>> devices weren't a popular feature right now,
>>
>> Oh, sorry for my misunderstanding.
>>
>>> not that the shrinking is. As a general rule, the whole option of 
>>> pre-loading a filesystem with data as you're creating it is not a 
>>> popular feature, because most sysadmins are much more willing to 
>>> trust adding data after the filesystem is created.
>>>
>>> Personally, given the existence of seed devices, I would absolutely 
>>> expect there to be a quick and easy way to generate a minimalistic 
>>> image using a single command (because realistic usage of seed devices 
>>> implies minimalistic images).  I agree that it shouldn't be the 
>>> default behavior, but I don't think it needs to be removed completely.
>>
>> Just like I said in cover letter, even for ext*, it's provided by 
>> genext2fs, not mke2fs.
> Then maybe this should get split out into a separate tool instead of 
> just removing it completely?  There is obviously at least some interest 
> in this functionality.
>>
>> I totally understand end-user really want a do-it-all solution.
>> But from developers' view, the old UNIX way is better to maintain code 
>> clean and easy to read.
> What the code is doing should have near zero impact on readability.  If 
> it did, then the BTRFS code in general is already way beyond most people.
>>
>>
>> In fact, you can even create your script to do the old behavior if you 
>> don't care that the result may not fully take use of the space, just by:
>>
>> 1) Calculate the size of the whole directory
>>     "du" command can do it easily, and it does things better than us! For
>>     years!
> Um, no it actually doesn't do things better in all cases.  it doesn't 
> account for extended attributes, or metadata usage, or any number of 
> other things that factor into how much space a file or directory will 
> take up on BTRFS.  It's good enough for finding what's using most of 
> your space, but it's not reliable for determining how much space you 
> need to store that data (especially once you throw in in-line compression).
>>
>> 2) Multiple the value according to the meta/data profile
>>     Take care of small files, which will be inlined.
>>     And don't forget size for data checksum.
>>     (BTW, there is no way to change the behavor of inlined data and data
>>      checksum for mkfs. unlike btrfs-convert)
> This is where the issue lies.  It's not possible for a person to 
> calculate this with reasonable accuracy, and you arguably can't even do 
> it for certain programmatically without some serious work.
>>
>> 3) Create a file with size calculated by step 2)
>>
>> 4) Execute "mkfs.btrfs -d <dir> <created file>"
>>
>>>   The main issues here are that it wasn't documented well (like many 
>>> other things in BTRFS), and it didn't generate a filesystem that was 
>>> properly compliant with the on-disk format (because it used space in 
>>> the 1MB reserved area at the beginning of the FS).  Fixing those 
>>> issues in no way requires removing the feature.
>>
>> Yes, 1MB can be fixed easily (although not properly). But the whole 
>> customized chunk allocator is the real problem.
>> The almost dead code is always bug-prone. Replace it with updated 
>> generic chunk allocator is the way to avoid later whac-a-mole, and 
>> should be done asap.
> Agreed, but that doesn't preclude having the option to keep the 
> generated image to the minimum size.
>>
>>>>
>>>> And further more, even following the existing shrink behavior, you 
>>>> still need to truncate the file all by yourself.
>>>> Which is no better than creating a good sized file and then mkfs on it.
>>> Only if you pre-create the file.  If the file doesn't exist, it gets 
>>> created at the appropriate size.  That's part of why the chunk 
>>> allocations are screwed up and stuff gets put in the first 1MB, it 
>>> generates the FS on-the-fly and writes it out as it's generating it.
>>
>> Nope, even you created the file in advance, it will still occupy the 
>> first 1M.
> Because it doesn't assume that the file is there to begin with.  It's 
> not trying O_CREAT and falling back to some different code if that 
> fails.  The code assumes that the file won't be there, and handles 
> things accordingly albeit incorrectly (it should seek past the first 
> 1MB, write the initial SB, and then start chunk allocation).  IOW, the 
> code takes a shortcut in that it doesn't check for the file, and the 
> rest is written to account for that by assuming there wasn't a file. The 
> lack of truncation just means it doesn't try to trim things down by 
> itself if the file is already there (it assumes that you knew what you 
> were doing).
> 
> Put differently, I'm fairly certain that the current -r option removes 
> the total size check unless the target is a device (although it may 
> remove the check there too and just fail when it tries to write past the 
> end of the device), and will thus extend existing files to the required 
> size to hold the data.
>>
>> BTW, you can get back the size calculation for shrink, but you will 
>> soon find that it's just the start of a new nightmare.
>> Because there is no easy way to calculate the real metadata usage.
>>
>> The result (and the old calculator) will be no better than guessing it.
>> (Well, just multiply the dir size by 2 will never go wrong)
> No, it can go wrong depending on what you count as part of the size.
>>
>>
>> Thanks,
>> Qu
>>
>>>>
>>>> Thanks,
>>>> Qu
>>>>
>>>> Sent: Friday, September 22, 2017 at 5:24 PM
>>>> From: "Anand Jain" <anand.jain@oracle.com>
>>>> To: "Qu Wenruo" <quwenruo.btrfs@gmx.com>, linux-btrfs@vger.kernel.org
>>>> Cc: dsterba@suse.cz
>>>> Subject: Re: [PATCH v3 07/14] btrfs-progs: Doc/mkfs: Add extra 
>>>> condition for rootdir option
>>>>
>>>>> +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`. +
>>>>
>>>> Hmm well. Shrink to fit exactly to the size of the given
>>>> files-and-directory is indeed a nice feature. Which would help to 
>>>> create
>>>> a golden-image btrfs seed device. Its not popular as of now, but at 
>>>> some
>>>> point it may in the cloud environment.
>>>>
>>>> Replacing this feature instead of creating a new option is not a good
>>>> idea indeed. I missed something ?
>>>>
>>>> Thanks, Anand
>>>>
>>>>> +Also, if destination file/block device does not exist, *--rootdir* 
>>>>> will not
>>>>> +create the image file, to make it follow the normal mkfs behavior.
>>>
>>> -- 
>>> 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 v3 07/14] btrfs-progs: Doc/mkfs: Add extra condition for rootdir option

[Anand Jain]

> All my points are clear for this patchset:
> I know I removed one function, and my reason is:
> 1) No or little usage
>     And it's anti intuition.
> 2) Dead code (not tested nor well documented)
> 3) Possible workaround
> 
> I can add several extra reasons as I stated before, but number of 
> reasons won't validate anything anyway.

  End user convenience wins over the developer's technical difficulties.

  Pls don't remove this feature, it needs fix such as #2 (above) to 
improve on #1 (above) as in your list.

Thanks, Anand

Re: [PATCH v3 07/14] btrfs-progs: Doc/mkfs: Add extra condition for rootdir option

[Goffredo Baroncelli]

On 09/24/2017 12:10 PM, Anand Jain wrote:
> 
> 
>> All my points are clear for this patchset:
>> I know I removed one function, and my reason is:
>> 1) No or little usage
>>     And it's anti intuition.
>> 2) Dead code (not tested nor well documented)
>> 3) Possible workaround
>>
>> I can add several extra reasons as I stated before, but number of reasons won't validate anything anyway.
> 
>  End user convenience wins over the developer's technical difficulties.

Sorry, but I have to agree with Qu; there is no a big demand (other than Austin) for this functionality; even you stated that "...at some point it may..."; until now the UI is quite unfriendly (we should use a big enough device, and then cut it by hand on the basis of the output of mkfs.btrfs)...

I fear that this is another feature which increase the complexity of btrfs (and tools) with little or none usage....

The work of Qu is a nice cleanup; I hope that this will be the direction which BTRFS will takes: removing of "strange/unused" features improving the reliability of the others.

BR
G.Baroncelli



> 
>  Pls don't remove this feature, it needs fix such as #2 (above) to improve on #1 (above) as in your list.
> 
> Thanks, Anand
> -- 
> 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
> 


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

Re: [PATCH v3 07/14] btrfs-progs: Doc/mkfs: Add extra condition for rootdir option

[Austin S. Hemmelgarn]

On 2017-09-24 10:08, Goffredo Baroncelli wrote:
> On 09/24/2017 12:10 PM, Anand Jain wrote:
>>
>>
>>> All my points are clear for this patchset:
>>> I know I removed one function, and my reason is:
>>> 1) No or little usage
>>>      And it's anti intuition.
>>> 2) Dead code (not tested nor well documented)
>>> 3) Possible workaround
>>>
>>> I can add several extra reasons as I stated before, but number of reasons won't validate anything anyway.
>>
>>   End user convenience wins over the developer's technical difficulties.
> 
> Sorry, but I have to agree with Qu; there is no a big demand (other than Austin) for this functionality; even you stated that "...at some point it may..."; until now the UI is quite unfriendly (we should use a big enough device, and then cut it by hand on the basis of the output of mkfs.btrfs)...
I will comment that I agree that it should not be the default.  It's not 
intuitive for most people, and as you say it's a niche feature.  Just 
removing it completely though with the above argument sounds very much 
like trying to meet quotas for coding.
> 
> I fear that this is another feature which increase the complexity of btrfs (and tools) with little or none usage....
On average?  It only increases the complexity of mkfs once there's a fix 
for the (theoretically trivial to fix) issue with it ignoring the fact 
that the first 1MB of space is supposed to be untouched by BTRFS.
> 
> The work of Qu is a nice cleanup; I hope that this will be the direction which BTRFS will takes: removing of "strange/unused" features improving the reliability of the others.
The two are not inherently interdependent, and that argument doesn't 
exactly hold up to scrutiny considering that mkfs is already perfectly 
reliable even with this feature, and it does not compromise the 
reliability of other features (again, once you fix the usage of the 
reserved area at the beginning of the image).

Re: [PATCH v3 07/14] btrfs-progs: Doc/mkfs: Add extra condition for rootdir option

[David Sterba]

On Mon, Sep 25, 2017 at 07:15:30AM -0400, Austin S. Hemmelgarn wrote:
> On 2017-09-24 10:08, Goffredo Baroncelli wrote:
> > On 09/24/2017 12:10 PM, Anand Jain wrote:

A lot of points in this thread, let me address them here.

I don't want to remove --rootdir functionality, the fix that's being
proposed removes the minimalization -- feature that was not well known,
but I understand it's useful (and already used).

I'd like to fix that in another way, eg. as an option to mkfs or a
separate tool.

I'm not worried about adding more code or code complexity. If we do it
right it's not a problem in the long run. But people for some reason
like to delete other people's code or functionality.

Regarding guessing number of users, this is always hard. So if there's
one vocal enough to let us know about the usecase, it's IMHO good time
to explore the it, code-wise and documentation-wise, and fix it or
improve.

So, what next. I'd like to get rid of the custom chunk allocator, namely
because of the known 1MB area misuse and duplication.

Implementing the minimalization might need some preparatory work and I
don't have a realistic ETA now. Ideally we'd fix both problems in one
version, as I'd rather avoid "temporary" solution to drop the
minimalization with the promise to put it back later.

Re: [PATCH v3 07/14] btrfs-progs: Doc/mkfs: Add extra condition for rootdir option

[Qu Wenruo]

On 2017年09月28日 00:20, David Sterba wrote:
> On Mon, Sep 25, 2017 at 07:15:30AM -0400, Austin S. Hemmelgarn wrote:
>> On 2017-09-24 10:08, Goffredo Baroncelli wrote:
>>> On 09/24/2017 12:10 PM, Anand Jain wrote:
> 
> A lot of points in this thread, let me address them here.
> 
> I don't want to remove --rootdir functionality, the fix that's being
> proposed removes the minimalization -- feature that was not well known,
> but I understand it's useful (and already used).
> 
> I'd like to fix that in another way, eg. as an option to mkfs or a
> separate tool.
> 
> I'm not worried about adding more code or code complexity. If we do it
> right it's not a problem in the long run. But people for some reason
> like to delete other people's code or functionality.
> 
> Regarding guessing number of users, this is always hard. So if there's
> one vocal enough to let us know about the usecase, it's IMHO good time
> to explore the it, code-wise and documentation-wise, and fix it or
> improve.
> 
> So, what next. I'd like to get rid of the custom chunk allocator, namely
> because of the known 1MB area misuse and duplication.
> 
> Implementing the minimalization might need some preparatory work and I
> don't have a realistic ETA now.

Well, if using over-reserve-then-shrink method, it could be done, 
without much hassle.

However ETA maybe delayed to middle of Oct, as I'm going to enjoy my 
holiday during 1st Oct to 7th Oct.

Thanks,
Qu

> Ideally we'd fix both problems in one
> version, as I'd rather avoid "temporary" solution to drop the
> minimalization with the promise to put it back later.
> 

Re: [PATCH v3 07/14] btrfs-progs: Doc/mkfs: Add extra condition for rootdir option

[Austin S. Hemmelgarn]

On 2017-09-27 20:00, Qu Wenruo wrote:
> 
> 
> On 2017年09月28日 00:20, David Sterba wrote:
>> On Mon, Sep 25, 2017 at 07:15:30AM -0400, Austin S. Hemmelgarn wrote:
>>> On 2017-09-24 10:08, Goffredo Baroncelli wrote:
>>>> On 09/24/2017 12:10 PM, Anand Jain wrote:
>>
>> A lot of points in this thread, let me address them here.
>>
>> I don't want to remove --rootdir functionality, the fix that's being
>> proposed removes the minimalization -- feature that was not well known,
>> but I understand it's useful (and already used).
>>
>> I'd like to fix that in another way, eg. as an option to mkfs or a
>> separate tool.
>>
>> I'm not worried about adding more code or code complexity. If we do it
>> right it's not a problem in the long run. But people for some reason
>> like to delete other people's code or functionality.
>>
>> Regarding guessing number of users, this is always hard. So if there's
>> one vocal enough to let us know about the usecase, it's IMHO good time
>> to explore the it, code-wise and documentation-wise, and fix it or
>> improve.
>>
>> So, what next. I'd like to get rid of the custom chunk allocator, namely
>> because of the known 1MB area misuse and duplication.
>>
>> Implementing the minimalization might need some preparatory work and I
>> don't have a realistic ETA now.
> 
> Well, if using over-reserve-then-shrink method, it could be done, 
> without much hassle.
It _should_ be possible to compute the exact size required though.  As 
outlined in one of my other replies, an absolute minimal filesystem 
should in theory consist of the following space usage:
1. 1MB (for the reserved space at the beginning).
2. However many superblocks it should have given the size.
3. The total amount of file data and extended attribute data to be 
included, rounding up for block size
4. The exact amount of metadata space needed to represent the tree from 
3, also rounding up for block size.
5. The exact amount of system chunk space needed to handle 3 and 4, plus 
enough room to allocate at least one more chunk of each type (to 
ultimately allow for resizing the filesystem if desired).
6. Exactly enough reserved metadata space to resize the FS 
(alternatively, the standard space for the GlobalReserve, which should 
be enough and then some).

Computing this is non-trivial for a human, and even with a computer 
requires knowledge of how the chunk allocator is implemented.  mkfs 
should have that knowledge though, so in theory it is reasonably well 
positioned to just compute this, allocate that much space, and then 
generate the filesystem.  The complaints I have about trying to do this 
manually in two passes are that:
1. Computing the above by hand is not easy.
2. Computing it by hand and then running mkfs on the resultant file 
doesn't work most of the time.

As a result, I would think this could easily be handled in the following 
steps:
1. Scan the contents of the --rootdir option to determine total size needed.
2. Compute the required allocation and allocate it.
3. Generate the filesystem image.

This has an inherent TOCTOU race, but it's one that a lot of archiving 
tools (tar being a prime example) already have, and it's not likely that 
people will hit it since you're not likely to be using a changing 
directory for --rootdir.
> 
> However ETA maybe delayed to middle of Oct, as I'm going to enjoy my 
> holiday during 1st Oct to 7th Oct.
Entirely understandable, enjoy your time off!

Re: [PATCH v3 07/14] btrfs-progs: Doc/mkfs: Add extra condition for rootdir option

[Goffredo Baroncelli]

On 09/28/2017 02:00 AM, Qu Wenruo wrote:
> 
> 
> On 2017年09月28日 00:20, David Sterba wrote:
>> On Mon, Sep 25, 2017 at 07:15:30AM -0400, Austin S. Hemmelgarn wrote:
>>> On 2017-09-24 10:08, Goffredo Baroncelli wrote:
>>>> On 09/24/2017 12:10 PM, Anand Jain wrote:
>>
>> A lot of points in this thread, let me address them here.
>>
>> I don't want to remove --rootdir functionality, the fix that's being
>> proposed removes the minimalization -- feature that was not well known,
>> but I understand it's useful (and already used).
>>
>> I'd like to fix that in another way, eg. as an option to mkfs or a
>> separate tool.
>>
>> I'm not worried about adding more code or code complexity. If we do it
>> right it's not a problem in the long run. But people for some reason
>> like to delete other people's code or functionality.
>>
>> Regarding guessing number of users, this is always hard. So if there's
>> one vocal enough to let us know about the usecase, it's IMHO good time
>> to explore the it, code-wise and documentation-wise, and fix it or
>> improve.
>>
>> So, what next. I'd like to get rid of the custom chunk allocator, namely
>> because of the known 1MB area misuse and duplication.
>>
>> Implementing the minimalization might need some preparatory work and I
>> don't have a realistic ETA now.
> 
> Well, if using over-reserve-then-shrink method, it could be done, without much hassle.

What about doing it on a file instead of a device ? As sparse file, it would be less expensive to enlarge then shrink. I think that who need to build a filesystem with "shrink", doesn't need to create it on a real block device
> 
> However ETA maybe delayed to middle of Oct, as I'm going to enjoy my holiday during 1st Oct to 7th Oct.
> 
> Thanks,
> Qu
> 
>> Ideally we'd fix both problems in one
>> version, as I'd rather avoid "temporary" solution to drop the
>> minimalization with the promise to put it back later.
>>
> -- 
> 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
> 


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

Re: [PATCH v3 07/14] btrfs-progs: Doc/mkfs: Add extra condition for rootdir option

[Qu Wenruo]

On 2017年09月30日 00:57, Goffredo Baroncelli wrote:
> On 09/28/2017 02:00 AM, Qu Wenruo wrote:
>>
>>
>> On 2017年09月28日 00:20, David Sterba wrote:
>>> On Mon, Sep 25, 2017 at 07:15:30AM -0400, Austin S. Hemmelgarn wrote:
>>>> On 2017-09-24 10:08, Goffredo Baroncelli wrote:
>>>>> On 09/24/2017 12:10 PM, Anand Jain wrote:
>>>
>>> A lot of points in this thread, let me address them here.
>>>
>>> I don't want to remove --rootdir functionality, the fix that's being
>>> proposed removes the minimalization -- feature that was not well known,
>>> but I understand it's useful (and already used).
>>>
>>> I'd like to fix that in another way, eg. as an option to mkfs or a
>>> separate tool.
>>>
>>> I'm not worried about adding more code or code complexity. If we do it
>>> right it's not a problem in the long run. But people for some reason
>>> like to delete other people's code or functionality.
>>>
>>> Regarding guessing number of users, this is always hard. So if there's
>>> one vocal enough to let us know about the usecase, it's IMHO good time
>>> to explore the it, code-wise and documentation-wise, and fix it or
>>> improve.
>>>
>>> So, what next. I'd like to get rid of the custom chunk allocator, namely
>>> because of the known 1MB area misuse and duplication.
>>>
>>> Implementing the minimalization might need some preparatory work and I
>>> don't have a realistic ETA now.
>>
>> Well, if using over-reserve-then-shrink method, it could be done, without much hassle.
> 
> What about doing it on a file instead of a device ? As sparse file, it would be less expensive to enlarge then shrink. I think that who need to build a filesystem with "shrink", doesn't need to create it on a real block device

For device, nothing different, just v3 patchset will handle it.

For file, sparse file of course.

Thanks,
Qu

>>
>> However ETA maybe delayed to middle of Oct, as I'm going to enjoy my holiday during 1st Oct to 7th Oct.
>>
>> Thanks,
>> Qu
>>
>>> Ideally we'd fix both problems in one
>>> version, as I'd rather avoid "temporary" solution to drop the
>>> minimalization with the promise to put it back later.
>>>
>> -- 
>> 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 v3 07/14] btrfs-progs: Doc/mkfs: Add extra condition for rootdir option

[Austin S. Hemmelgarn]

On 2017-09-29 23:33, Qu Wenruo wrote:
> 
> 
> On 2017年09月30日 00:57, Goffredo Baroncelli wrote:
>> On 09/28/2017 02:00 AM, Qu Wenruo wrote:
>>>
>>>
>>> On 2017年09月28日 00:20, David Sterba wrote:
>>>> On Mon, Sep 25, 2017 at 07:15:30AM -0400, Austin S. Hemmelgarn wrote:
>>>>> On 2017-09-24 10:08, Goffredo Baroncelli wrote:
>>>>>> On 09/24/2017 12:10 PM, Anand Jain wrote:
>>>>
>>>> A lot of points in this thread, let me address them here.
>>>>
>>>> I don't want to remove --rootdir functionality, the fix that's being
>>>> proposed removes the minimalization -- feature that was not well known,
>>>> but I understand it's useful (and already used).
>>>>
>>>> I'd like to fix that in another way, eg. as an option to mkfs or a
>>>> separate tool.
>>>>
>>>> I'm not worried about adding more code or code complexity. If we do it
>>>> right it's not a problem in the long run. But people for some reason
>>>> like to delete other people's code or functionality.
>>>>
>>>> Regarding guessing number of users, this is always hard. So if there's
>>>> one vocal enough to let us know about the usecase, it's IMHO good time
>>>> to explore the it, code-wise and documentation-wise, and fix it or
>>>> improve.
>>>>
>>>> So, what next. I'd like to get rid of the custom chunk allocator, 
>>>> namely
>>>> because of the known 1MB area misuse and duplication.
>>>>
>>>> Implementing the minimalization might need some preparatory work and I
>>>> don't have a realistic ETA now.
>>>
>>> Well, if using over-reserve-then-shrink method, it could be done, 
>>> without much hassle.
>>
>> What about doing it on a file instead of a device ? As sparse file, it 
>> would be less expensive to enlarge then shrink. I think that who need 
>> to build a filesystem with "shrink", doesn't need to create it on a 
>> real block device
> 
> For device, nothing different, just v3 patchset will handle it.
Agreed on this point.
> 
> For file, sparse file of course.
But not on this one.  Unless the image gets properly compacted, a sparse 
file will only help when you're just storing the image on a filesystem. 
It doesn't really help at all for when you need to put it on a physical 
device afterwards (except possibly if you're using thin provisioning). 
The reason I (at least) am complaining about losing this feature isn't 
about having space efficiency when storing image files on a filesystem, 
it's about time efficiency in transferring the image either to a device 
after generation (if you're going to multiple devices, it makes more 
sense to just generate an image file instead of writing directly to the 
first device in most cases), or to another system where it will 
ultimately be written to a device.

As a more specific example, look at how installation of Linux 
(preferably Raspbian) is typically done on a Raspberry Pi.  You have a 
base SD-card image that is designed to fit on a 1G SD card, and the root 
filesystem then gets resized automatically on first boot to fill the 
card.  You still need to copy that whole 1G of data to a _slow_ SD card 
before you can do anything with it, because you can't assume the card is 
empty and do sparse writes (and very few tools used for writing out disk 
images can do sparse writes).  You also have to copy all that data to 
download the image, and while they're distributed compressed, 
compressing runs of empty space still takes space.  If that instead used 
image generation similar to the current behavior of the --rootdir 
option, a reasonable amount of time would be saved in most steps, simply 
because the image was smaller (the 1G image has 300MB or more of empty 
space in it).

As another example, when dealing with seed devices, it makes just as 
much if not more sense if you're using LVM to store the data to create 
an LV that is exactly as large as needed for the seed device, and no 
bigger.  Using sparse files similarly doesn't work for that unless 
you're using thin provisioning and don't mind LVM complaining all the 
time about over-provisioning your space.

Re: [PATCH v3 07/14] btrfs-progs: Doc/mkfs: Add extra condition for rootdir option

[Goffredo Baroncelli]

On 10/02/2017 01:47 PM, Austin S. Hemmelgarn wrote:
>>>
>>> What about doing it on a file instead of a device ? As sparse file, it would be less expensive to enlarge then shrink. I think that who need to build a filesystem with "shrink", doesn't need to create it on a real block device
>>
>> For device, nothing different, just v3 patchset will handle it.
> Agreed on this point.
>>
>> For file, sparse file of course.
> But not on this one.  Unless the image gets properly compacted, a sparse file will only help when you're just storing the image on a filesystem.

I think that you have misunderstood my proposal... My suggestion is to create the image using a file, and after image creation compact it, and then cut it at the end. So you don't have to care about the space needing during the process (before the shrinking).

Today mkfs.btrfs fails to create an image on an empty file

ghigo@venice:/tmp$ mkdir test
ghigo@venice:/tmp$ mkdir test/test1
ghigo@venice:/tmp$ touch test/test1/file
ghigo@venice:/tmp$ mkfs.btrfs --root test disk.img
btrfs-progs v4.12
See http://btrfs.wiki.kernel.org for more information.

ERROR: failed to check size for disk.img: No such file or directory
ghigo@venice:/tmp$ touch disk.img
ghigo@venice:/tmp$ mkfs.btrfs --root test disk.img
btrfs-progs v4.12
See http://btrfs.wiki.kernel.org for more information.

ERROR: 'disk.img' is too small to make a usable filesystem
ERROR: minimum size for each btrfs device is 41943040

you have to create a big enough

ghigo@venice:/tmp$ truncate -s 10G disk.img 
ghigo@venice:/tmp$ mkfs.btrfs --root test disk.img
btrfs-progs v4.12
See http://btrfs.wiki.kernel.org for more information.
[...]


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

Re: [PATCH v3 07/14] btrfs-progs: Doc/mkfs: Add extra condition for rootdir option

[Austin S. Hemmelgarn]

On 2017-09-22 11:07, Qu Wenruo wrote:
> 
> 
> On 2017年09月22日 21:33, Austin S. Hemmelgarn wrote:
>> On 2017-09-22 08:32, Qu Wenruo wrote:
>>> On 2017年09月22日 19:38, Austin S. Hemmelgarn wrote:
>>>> On 2017-09-22 06:39, Qu Wenruo wrote:
>>>>> As I already stated in an other thread, if you want to shrink, do 
>>>>> it in another command line tool.
>>>>> Do one thing and do it simple. (Although Btrfs itself is already 
>>>>> out of the UNIX way)
>>>> Unless I'm reading the code wrong, the shrinking isn't happening in 
>>>> a second pass, so this _is_ doing one thing, and it appears to be 
>>>> doing it as simply as possible (although arguably not correctly 
>>>> because of the 1MB reserved area being used).
>>>
>>> If you're referring to my V1 implementation of shrink, that's doing 
>>> *one* thing.
>>>
>>> But the original shrinking code? Nope, or we won't have the custom 
>>> chunk allocator at all.
>>>
>>> What I really mean is, if one wants to shrink, at least don't couple 
>>> the shrink code into "mkfs.btrfs".
>>>
>>> Do shrink in its own tool/subcommand, not in a really unrelated tool.
>> There are two cases for shrinking a filesystem:
>> 1. You're resizing it to move to a smaller disk (or speed up copying 
>> to another disk).
>> 2. You're generating a filesystem image that needs to be as small as 
>> possible.
> 
> I would argue there is no meaning of creating *smallest* image. (Of 
> course it exists).
There is an exact meaning given the on-disk layout.  It's an image whose 
size is equal to the sum of:
1. 1MB (for the reserved space at the beginning).
2. However many superblocks it should have given the size.
3. The total amount of file data and extended attribute data to be 
included, rounding up for block size
4. The exact amount of metadata space needed to represent the tree from 
3, also rounding up for block size.
5. The exact amount of system chunk space needed to handle 3 and 4, plus 
enough room to allocate at least one more chunk of each type (to 
ultimately allow for resizing the filesystem if desired).
6. Exactly enough reserved metadata space to resize the FS.
> 
> We could put tons of code to implement, and more (or less) test cases to 
> verify it.
> 
> But the demand doesn't validate the effort.
And how much effort has been put into ripping this out completely 
together with the other fixes?  How much more would it have been to just 
move it to another option and fix the reserved area usage?
> 
> All my points are clear for this patchset:
> I know I removed one function, and my reason is:
> 1) No or little usage
>     And it's anti intuition.
So split it to a separate tool (mkimage maybe?), and fix mkfs to behave 
sensibly.  I absolutely agree on the fact that it's non-intuitive.  It 
should either be it's own option (with a dependency on -r being passed 
of course), or a separate tool if you're so worried about mkfs being too 
complex.

As to usage, given the current data, there is no proof that I'm the only 
one using it, but there is also no proof that anybody other than me is 
using it, which means that you can't reasonably base an argument on 
actual usage of this option, since you can't prove anything about usage. 
  All you know is that you have one person who uses it, and one who was 
confused by it (but appears to want to use it in a different way).  It's 
a niche use case though, and when dealing with something like this, 
there is a threshold of usage below which you won't see much in the way 
of discussion of the option on the list, since only a reasonably small 
percentage of BTRFS users are actually subscribed.

> 2) Dead code (not tested nor well documented)
<rant>It _IS NOT_ dead code.  It is absolutely reachable from code 
external to itself.  It's potentially unused code, but that is not the 
same thing as dead code.</rant>

That aside, I can fix up the documentation, and I've actually tested it 
reasonably thoroughly (I use it every month or so when I update stuff I 
have using seed devices, and it also gets used by my testing 
infrastructure when generating images pre-loaded with files for tests to 
save time).  I'll agree it hasn't been rigorously tested, but it does 
appear to work as (not) advertised, even when used in odd ways.

> 3) Possible workaround
There are three options for workarounds, and both of them are sub-par to 
this even aside from the reduced simplicity it offers to userspace:
1. Resize after mkfs.  This is impractical both because there is no 
offline resize (having to mount the FS RW prior to use as a seed device 
means that you don't have a guaranteed reproducible image, which is a 
pretty common request for container usage there days), and it will end 
up with wasted space (the smallest possible filesystem created through a 
resize is consistently larger (by more than 1MB) than what the -r option 
to mkfs generates).
2. Use a binary search to determine the smallest size to a reasonable 
margin.  This is impractical simply because it takes too long, and again 
can't reliably get the smallest possible image.
3. Attempt to compute the smallest possible image without using a binary 
search, pre-create the file, and then call mkfs.  This is non-trivial 
without knowledge of the internal workings of mkfs, and is liable to 
break when something changes in mkfs (unless you want to consider the 
block-level layout generated by the --rootdir option to be part of the 
ABI and something that shouldn't change, but that is something you would 
need to discuss with the other developers).

IOW, this is like saying that duct tape is a workaround for not having 
super glue.  It will technically work, but not anywhere near as well.
> 
> I can add several extra reasons as I stated before, but number of 
> reasons won't validate anything anyway.
> 
> Building software is always trading one thing for another.
> I understand there may be some need for this function, but it doesn't 
> validate the cost.
> 
> And I think the fact that until recently a mail reported about the 
> shrinking behavior has already backed up my point.
The only information it gives is that until now nobody who tried that 
option either cared enough to complain about it, or needed it to behave 
any other way.

IOW, as stated above, given the current data, there is no proof that I'm 
the only one using it, but there is also no proof that anybody other 
than me is using it, which means that you can't reasonably base your 
argument on actual usage of this option.
> 
> Thanks,
> Qu
> 
>>
>> Case 1 is obviously unrelated to creating a filesystem.  Case 2 
>> however is kind of integral to the creation of the filesystem image 
>> itself by definition, especially for a CoW filesystem because it's not 
>> possible to shrink to the absolute smallest size due to the 
>> GlobalReserve and other things.
>>
>> Similarly, there are two primary use cases for pre-loading the 
>> filesystem with data:
>> 1. Avoiding a copy when reprovisioning storage on a system.  For 
>> example, splitting a directory out to a new filesystem, you could use 
>> the -r option to avoid having to copy the data after mounting the 
>> filesystem.
>> 2. Creating base images for systems.
>>
>> The first case shouldn't need the shrinking functionality, but the 
>> second is a very common use case together with the second usage for 
>> shrinking a filesystem.
>>>
>>>>>
>>>>> It may be offline shrink/balance. But not to further complexing the 
>>>>> --rootdir option now. >
>>>>> And you also said that, the shrink feature is not a popular feature 
>>>>> *NOW*, then I don't think it's worthy to implment it *NOW* either.
>>>>> Implement future feature in the future please.
>>>> I'm not sure about you, but I could have sworn that he meant seed 
>>>> devices weren't a popular feature right now,
>>>
>>> Oh, sorry for my misunderstanding.
>>>
>>>> not that the shrinking is. As a general rule, the whole option of 
>>>> pre-loading a filesystem with data as you're creating it is not a 
>>>> popular feature, because most sysadmins are much more willing to 
>>>> trust adding data after the filesystem is created.
>>>>
>>>> Personally, given the existence of seed devices, I would absolutely 
>>>> expect there to be a quick and easy way to generate a minimalistic 
>>>> image using a single command (because realistic usage of seed 
>>>> devices implies minimalistic images).  I agree that it shouldn't be 
>>>> the default behavior, but I don't think it needs to be removed 
>>>> completely.
>>>
>>> Just like I said in cover letter, even for ext*, it's provided by 
>>> genext2fs, not mke2fs.
>> Then maybe this should get split out into a separate tool instead of 
>> just removing it completely?  There is obviously at least some 
>> interest in this functionality.
>>>
>>> I totally understand end-user really want a do-it-all solution.
>>> But from developers' view, the old UNIX way is better to maintain 
>>> code clean and easy to read.
>> What the code is doing should have near zero impact on readability.  
>> If it did, then the BTRFS code in general is already way beyond most 
>> people.
>>>
>>>
>>> In fact, you can even create your script to do the old behavior if 
>>> you don't care that the result may not fully take use of the space, 
>>> just by:
>>>
>>> 1) Calculate the size of the whole directory
>>>     "du" command can do it easily, and it does things better than us! 
>>> For
>>>     years!
>> Um, no it actually doesn't do things better in all cases.  it doesn't 
>> account for extended attributes, or metadata usage, or any number of 
>> other things that factor into how much space a file or directory will 
>> take up on BTRFS.  It's good enough for finding what's using most of 
>> your space, but it's not reliable for determining how much space you 
>> need to store that data (especially once you throw in in-line 
>> compression).
>>>
>>> 2) Multiple the value according to the meta/data profile
>>>     Take care of small files, which will be inlined.
>>>     And don't forget size for data checksum.
>>>     (BTW, there is no way to change the behavor of inlined data and data
>>>      checksum for mkfs. unlike btrfs-convert)
>> This is where the issue lies.  It's not possible for a person to 
>> calculate this with reasonable accuracy, and you arguably can't even 
>> do it for certain programmatically without some serious work.
>>>
>>> 3) Create a file with size calculated by step 2)
>>>
>>> 4) Execute "mkfs.btrfs -d <dir> <created file>"
>>>
>>>>   The main issues here are that it wasn't documented well (like many 
>>>> other things in BTRFS), and it didn't generate a filesystem that was 
>>>> properly compliant with the on-disk format (because it used space in 
>>>> the 1MB reserved area at the beginning of the FS).  Fixing those 
>>>> issues in no way requires removing the feature.
>>>
>>> Yes, 1MB can be fixed easily (although not properly). But the whole 
>>> customized chunk allocator is the real problem.
>>> The almost dead code is always bug-prone. Replace it with updated 
>>> generic chunk allocator is the way to avoid later whac-a-mole, and 
>>> should be done asap.
>> Agreed, but that doesn't preclude having the option to keep the 
>> generated image to the minimum size.
>>>
>>>>>
>>>>> And further more, even following the existing shrink behavior, you 
>>>>> still need to truncate the file all by yourself.
>>>>> Which is no better than creating a good sized file and then mkfs on 
>>>>> it.
>>>> Only if you pre-create the file.  If the file doesn't exist, it gets 
>>>> created at the appropriate size.  That's part of why the chunk 
>>>> allocations are screwed up and stuff gets put in the first 1MB, it 
>>>> generates the FS on-the-fly and writes it out as it's generating it.
>>>
>>> Nope, even you created the file in advance, it will still occupy the 
>>> first 1M.
>> Because it doesn't assume that the file is there to begin with.  It's 
>> not trying O_CREAT and falling back to some different code if that 
>> fails.  The code assumes that the file won't be there, and handles 
>> things accordingly albeit incorrectly (it should seek past the first 
>> 1MB, write the initial SB, and then start chunk allocation).  IOW, the 
>> code takes a shortcut in that it doesn't check for the file, and the 
>> rest is written to account for that by assuming there wasn't a file. 
>> The lack of truncation just means it doesn't try to trim things down 
>> by itself if the file is already there (it assumes that you knew what 
>> you were doing).
>>
>> Put differently, I'm fairly certain that the current -r option removes 
>> the total size check unless the target is a device (although it may 
>> remove the check there too and just fail when it tries to write past 
>> the end of the device), and will thus extend existing files to the 
>> required size to hold the data.
>>>
>>> BTW, you can get back the size calculation for shrink, but you will 
>>> soon find that it's just the start of a new nightmare.
>>> Because there is no easy way to calculate the real metadata usage.
>>>
>>> The result (and the old calculator) will be no better than guessing it.
>>> (Well, just multiply the dir size by 2 will never go wrong)
>> No, it can go wrong depending on what you count as part of the size.
>>>
>>>
>>> Thanks,
>>> Qu
>>>
>>>>>
>>>>> Thanks,
>>>>> Qu
>>>>>
>>>>> Sent: Friday, September 22, 2017 at 5:24 PM
>>>>> From: "Anand Jain" <anand.jain@oracle.com>
>>>>> To: "Qu Wenruo" <quwenruo.btrfs@gmx.com>, linux-btrfs@vger.kernel.org
>>>>> Cc: dsterba@suse.cz
>>>>> Subject: Re: [PATCH v3 07/14] btrfs-progs: Doc/mkfs: Add extra 
>>>>> condition for rootdir option
>>>>>
>>>>>> +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`. +
>>>>>
>>>>> Hmm well. Shrink to fit exactly to the size of the given
>>>>> files-and-directory is indeed a nice feature. Which would help to 
>>>>> create
>>>>> a golden-image btrfs seed device. Its not popular as of now, but at 
>>>>> some
>>>>> point it may in the cloud environment.
>>>>>
>>>>> Replacing this feature instead of creating a new option is not a good
>>>>> idea indeed. I missed something ?
>>>>>
>>>>> Thanks, Anand
>>>>>
>>>>>> +Also, if destination file/block device does not exist, 
>>>>>> *--rootdir* will not
>>>>>> +create the image file, to make it follow the normal mkfs behavior.
>>>>
>>>> -- 
>>>> 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
>>

[PATCH v3 08/14] btrfs-progs: tests/common: Split user xattr into its own branch for generate_dataset

[Qu Wenruo]

generate_dataset() combines file acl and file xattr into "acls" type,
since both of them are implemented by using file xattr.

However sometimes we don't want user file attr under certain case, for
example to populate files on tmpfs, which doesn't support user file
xattr.

So this patch split original "acls" type into "user_xattr" and "acls",
make it easier for us to use it on tmpfs.

Signed-off-by: Qu Wenruo <quwenruo.btrfs@gmx.com>
---
 tests/common         | 6 +++++-
 tests/common.convert | 2 +-
 2 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/tests/common b/tests/common
index 08a25918..9aebe3e7 100644
--- a/tests/common
+++ b/tests/common
@@ -498,10 +498,14 @@ generate_dataset() {
 			for num in $(seq 1 "$DATASET_SIZE"); do
 				run_check $SUDO_HELPER touch "$dirpath/$dataset_type.$num"
 				run_check $SUDO_HELPER setfacl -m "u:root:x" "$dirpath/$dataset_type.$num"
+			done
+			;;
+		user_xattr)
+			for num in $(seq 1 "$DATASET_SIZE"); do
+				run_check $SUDO_HELPER touch "$dirpath/$dataset_type.$num"
 				run_check $SUDO_HELPER setfattr -n user.foo -v "bar$num" "$dirpath/$dataset_type.$num"
 			done
 			;;
-
 		fifo)
 			for num in $(seq 1 "$DATASET_SIZE"); do
 				run_check $SUDO_HELPER mkfifo "$dirpath/$dataset_type.$num"
diff --git a/tests/common.convert b/tests/common.convert
index 1be804cf..45174b7e 100644
--- a/tests/common.convert
+++ b/tests/common.convert
@@ -36,7 +36,7 @@ run_check_mount_convert_dev()
 
 populate_fs() {
 
-        for dataset_type in 'small' 'hardlink' 'fast_symlink' 'brokenlink' 'perm' 'sparse' 'acls' 'fifo' 'slow_symlink'; do
+        for dataset_type in 'small' 'hardlink' 'fast_symlink' 'brokenlink' 'perm' 'sparse' 'acls' 'user_xattr' 'fifo' 'slow_symlink'; do
 		generate_dataset "$dataset_type"
 	done
 }
-- 
2.14.1

[PATCH v3 09/14] btrfs-progs: tests/common: Introduce optional parameter to specify destination directory for generate_dataset

[Qu Wenruo]

Normally generate_dataset() will create data into
$TEST_MNT/$dataset_type.

This is OK since most tests are doing their operation in $TEST_MNT.
However this is not the case for "mkfs --rootdir" test.

This patch will adds an optional parameter for generate_dataset() to
specify the destination directory, for later "mkfs --rootdir" test
cases.

Signed-off-by: Qu Wenruo <quwenruo.btrfs@gmx.com>
---
 tests/common | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tests/common b/tests/common
index 9aebe3e7..242ded1d 100644
--- a/tests/common
+++ b/tests/common
@@ -440,7 +440,11 @@ DATASET_SIZE=50
 generate_dataset() {
 
 	dataset_type="$1"
-	dirpath=$TEST_MNT/$dataset_type
+	if [ -z "$2" ]; then
+		dirpath="$TEST_MNT/$dataset_type"
+	else
+		dirpath="$2/$dataset_type"
+	fi
 	run_check $SUDO_HELPER mkdir -p "$dirpath"
 
 	case "$dataset_type" in
-- 
2.14.1

[PATCH v3 10/14] btrfs-progs: tests/common: Make checksum, permission and acl check path independent

[Qu Wenruo]

convert_test_gen_checksums(), convert_test_perm() and convert_test_acl()
all uses absolute path, which is good enough for convert test.

However for "mkfs --rootdir" test, we want all above function to use
relative path, making the output path independent.

This patch modified all these functions by:
1) Adding new optional parameter to specify destination directory
   Callers and corresponding checkers also get this new optional parameter
2) Changing directory before generate files list/csum file
   And return to old pwd after work is done.

Signed-off-by: Qu Wenruo <quwenruo.btrfs@gmx.com>
---
 tests/common.convert | 91 +++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 83 insertions(+), 8 deletions(-)

diff --git a/tests/common.convert b/tests/common.convert
index 45174b7e..8a36cba3 100644
--- a/tests/common.convert
+++ b/tests/common.convert
@@ -93,45 +93,92 @@ convert_test_prep_fs() {
 
 # generate md5 checksums of files on $TEST_MNT
 # $1: path where the checksums will be stored
+# $2: (optional) destination directory if we're not using $TEST_MNT
 convert_test_gen_checksums() {
+	local dir_path
+	local csum_file
+	local saved_pwd
+
 	_assert_path "$1"
+	csum_file="$1"
+	if [ -z "$2" ]; then
+		dir_path="$TEST_MNT"
+	else
+		dir_path="$2"
+	fi
 
-	run_check $SUDO_HELPER dd if=/dev/zero of="$TEST_MNT/test" "bs=$nodesize" \
+	run_check $SUDO_HELPER dd if=/dev/zero of="$dir_path/test" "bs=$nodesize" \
 		count=1 >/dev/null 2>&1
-	run_check_stdout $SUDO_HELPER find "$TEST_MNT" -type f ! -name 'image' -exec md5sum {} \+ > "$1"
+
+	# We change directory into destination, so generated md5sum file won't
+	# include absolute path, making the result path independent.
+	saved_pwd="$(pwd)"
+	run_check cd "$dir_path"
+	run_check_stdout $SUDO_HELPER find . -type f ! -name 'image' -exec md5sum {} \+ \
+		> "$csum_file"
+	run_check cd "$saved_pwd"
 }
+
 # list $TEST_MNT data set file permissions.
 # $1: path where the permissions will be stored
+# $2: (optional) destination directory if we're not using $TEST_MNT
 convert_test_perm() {
 	local PERMTMP
+	local saved_pwd
+	local dir_path
 
 	_assert_path "$1"
 	PERMTMP="$1"
+	if [ -z "$2" ]; then
+		dir_path="$TEST_MNT"
+	else
+		dir_path="$2"
+	fi
 	FILES_LIST=$(mktemp --tmpdir btrfs-progs-convert.fileslistXXXXXX)
 
-	run_check $SUDO_HELPER dd if=/dev/zero of="$TEST_MNT/test" "bs=$nodesize" \
+	run_check $SUDO_HELPER dd if=/dev/zero of="$dir_path/test" "bs=$nodesize" \
 		count=1 >/dev/null 2>&1
-	run_check_stdout $SUDO_HELPER find "$TEST_MNT" -type f ! -name 'image' -fprint "$FILES_LIST"
+
+	# Same as convert_test_gen_checksums(), make output path independent
+	saved_pwd="$(pwd)"
+	run_check cd "$dir_path"
+	run_check_stdout $SUDO_HELPER find . -type f ! -name 'image' -fprint "$FILES_LIST"
 	# Fix directory entries order
 	sort "$FILES_LIST" -o "$FILES_LIST"
 	for file in `cat "$FILES_LIST"` ;do
 		run_check_stdout $SUDO_HELPER getfacl --absolute-names "$file" >> "$PERMTMP"
 	done
+	run_check cd "$saved_pwd"
 	rm -- "$FILES_LIST"
 }
+
 # list acls of files on $TEST_MNT
 # $1: path where the acls will be stored
+# $2: (optional) destination directory if we're not using $TEST_MNT
 convert_test_acl() {
 	local ACLSTMP
+	local dir_path
+	local saved_pwd
+
+	_assert_path "$1"
 	ACLTMP="$1"
+	if [ -z "$2" ]; then
+		dir_path="$TEST_MNT"
+	else
+		dir_path="$2"
+	fi
 	FILES_LIST=$(mktemp --tmpdir btrfs-progs-convert.fileslistXXXXXX)
 
-	run_check_stdout $SUDO_HELPER find "$TEST_MNT/acls" -type f -fprint "$FILES_LIST"
+	# Make find result and later getfattr output path independent
+	saved_pwd="$(pwd)"
+	run_check cd "$dir_path"
+	run_check_stdout $SUDO_HELPER find "./acls" -type f -fprint "$FILES_LIST"
 	# Fix directory entries order
 	sort "$FILES_LIST" -o "$FILES_LIST"
 	for file in `cat "$FILES_LIST"`;do
 		run_check_stdout $SUDO_HELPER getfattr --absolute-names -d "$file" >> "$ACLTMP"
 	done
+	run_check cd "$saved_pwd"
 	rm -- "$FILES_LIST"
 }
 
@@ -149,11 +196,18 @@ convert_test_do_convert() {
 convert_test_post_check_permissions() {
 	local EXT_PERMTMP
 	local BTRFS_PERMTMP
+	local dir_path
+	local saved_pwd
 
 	_assert_path "$1"
 	EXT_PERMTMP="$1"
+	if [ -z "$2" ]; then
+		dir_path="$TEST_MNT"
+	else
+		dir_path="$2"
+	fi
 	BTRFS_PERMTMP=$(mktemp --tmpdir btrfs-progs-convert.permXXXXXX)
-	convert_test_perm "$BTRFS_PERMTMP"
+	convert_test_perm "$BTRFS_PERMTMP" "$dir_path"
 
 	btrfs_perm=`md5sum "$BTRFS_PERMTMP" | cut -f1 -d' '`
 	ext_perm=`md5sum "$EXT_PERMTMP" | cut -f1 -d' '`
@@ -162,7 +216,7 @@ convert_test_post_check_permissions() {
 	then
 		btrfs_perm_file=`md5sum "$BTRFS_PERMTMP" | cut -f2 -d' '`
 		ext_perm_file=`md5sum "$EXT_PERMTMP" | cut -f2 -d' '`
-		_fail "file permission failed. Mismatched BTRFS:$btrfs_perm_file:$btrfs_perm EXT:$ext_perm_file:$ext_perm"
+		_fail "file permission failed. Mismatched AFTER:$btrfs_perm_file:$btrfs_perm BEFORE:$ext_perm_file:$ext_perm"
 	fi
 
 	rm -- "$BTRFS_PERMTMP"
@@ -172,11 +226,17 @@ convert_test_post_check_permissions() {
 convert_test_post_check_acl() {
 	local EXT_ACLTMP
 	local BTRFS_ACLTMP
+	local dir_path
 
 	_assert_path "$1"
 	EXT_ACLTMP="$1"
+	if [ -z "$2" ]; then
+		dir_path="$TEST_MNT"
+	else
+		dir_path="$2"
+	fi
 	BTRFS_ACLTMP=$(mktemp --tmpdir btrfs-progs-convert.aclsXXXXXXX)
-	convert_test_acl "$BTRFS_ACLTMP"
+	convert_test_acl "$BTRFS_ACLTMP" "$dir_path"
 
 	btrfs_acl=`md5sum "$BTRFS_ACLTMP" | cut -f1 -d' '`
 	ext_acl=`md5sum "$EXT_ACLTMP" | cut -f1 -d' '`
@@ -193,9 +253,24 @@ convert_test_post_check_acl() {
 
 # post conversion checks, verify md5sums
 convert_test_post_check_checksums() {
+	local dir_path
+	local csum_file
+
 	_assert_path "$1"
+	csum_file="$1"
+
+	if [ -z "$2" ]; then
+		dir_path="$TEST_MNT"
+	else
+		dir_path="$2"
+	fi
+
+	# csum file is generated using relative path, change directory
+	saved_pwd="$(pwd)"
+	run_check cd $dir_path
 	run_check_stdout $SUDO_HELPER md5sum -c "$1" |
 		grep -q 'FAILED' && _fail "file validation failed"
+	run_check cd $saved_pwd
 }
 
 # post conversion checks, all three in one call, on an unmounted image
-- 
2.14.1

[PATCH v3 11/14] btrfs-progs: tests/mkfs: Add basic test case for rootdir parameter

[Qu Wenruo]

Add a new test case to check if "--rootdir" option of mkfs.btrfs can
handle the file content, perrmission and xattr correctly.

The new test case reuses the convert facility, and looks just like
convert-tests/001-ext2-basic.

Signed-off-by: Qu Wenruo <quwenruo.btrfs@gmx.com>
---
 tests/mkfs-tests/010-basic-rootdir/test.sh | 78 ++++++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)
 create mode 100755 tests/mkfs-tests/010-basic-rootdir/test.sh

diff --git a/tests/mkfs-tests/010-basic-rootdir/test.sh b/tests/mkfs-tests/010-basic-rootdir/test.sh
new file mode 100755
index 00000000..6ff3e1af
--- /dev/null
+++ b/tests/mkfs-tests/010-basic-rootdir/test.sh
@@ -0,0 +1,78 @@
+#!/bin/bash
+# Check basic operations for "mkfs.btrfs --rootdir", including:
+# 1)	Checksum, permission and acl
+#	Should be consistent with source directory
+# 2)	Failure condition
+# 2.1)  Non-existent file/block as destination
+# 2.2)	Too small destination file
+# 2.3)  No privilege to read source directory
+#	All failure condition should fail, but without segfault/backtrace
+
+source "$TOP/tests/common"
+source "$TOP/tests/common.convert"
+
+setup_root_helper
+prepare_test_dev 512M
+check_prereq mkfs.btrfs
+check_global_prereq dd
+check_global_prereq sed
+
+# Since our test dir will be in /tmp, which is nowadays tmpfs for most
+# distributions, and tmpfs xattr doesn't support user xattr, here we
+# use a special populate_fs() which won't create user xattr.
+#
+# Don't worry, both acl and user xattr is implemented by xattr in btrfs,
+# so "acls" should cover the case.
+populate_tmpfs() {
+	_assert_path "$1"
+
+        for dataset_type in 'small' 'hardlink' 'fast_symlink' 'brokenlink' 'perm' 'sparse' 'acls' 'fifo' 'slow_symlink'; do
+		generate_dataset "$dataset_type" "$1"
+	done
+}
+
+# Basic content checker for difference nodesize/features
+content_test() {
+	local features
+	local nodesize
+	local src_dir
+	local csum_tmp 
+	local perm_tmp 
+	local acl_tmp
+
+	features="$1"
+	nodesize="$2"
+	src_dir=$(mktemp --tmpdir --directory btrfs-progs-mkfs-rootdir.XXXXXXX)
+
+	echo "    [TEST/mkfs_rootdir]   nodesize=$nodesize" "${features:-defaults}"
+	echo "creating test dir at $src_dir" >> "$RESULTS"
+
+	populate_tmpfs "$src_dir"
+	csum_tmp=$(mktemp --tmpdir btrfs-progs-convert.XXXXXXXXXX)
+	perm_tmp=$(mktemp --tmpdir btrfs-progs-convert.permXXXXXX)
+	acl_tmp=$(mktemp --tmpdir btrfs-progs-convert.aclsXXXXXXX)
+	convert_test_gen_checksums "$csum_tmp" "$src_dir"
+	convert_test_perm "$perm_tmp" "$src_dir"
+	convert_test_acl "$acl_tmp" "$src_dir"
+
+	run_check "$TOP/mkfs.btrfs" ${1:+-O "$1"} ${2:+-n "$2"} \
+		"--rootdir" "$src_dir" "$TEST_DEV"
+	run_check "$TOP/btrfs" check "$TEST_DEV"
+	run_check_mount_test_dev
+	convert_test_post_check_checksums "$csum_tmp"
+	convert_test_post_check_permissions "$perm_tmp"
+	convert_test_post_check_acl "$acl_tmp"
+	run_check_umount_test_dev
+
+	rm -- "$csum_tmp"
+	rm -- "$perm_tmp"
+	rm -- "$acl_tmp"
+}
+
+for feature in '' 'extref' 'skinny-metadata' 'no-holes'; do
+	content_test "$feature" 4096
+	content_test "$feature" 8192
+	content_test "$feature" 16384
+	content_test "$feature" 32768
+	content_test "$feature" 65536
+done
-- 
2.14.1

[PATCH v3.1 11/14] btrfs-progs: tests/mkfs: Add basic test case for rootdir parameter

[Qu Wenruo]

Add a new test case to check if "--rootdir" option of mkfs.btrfs can
handle the file content, perrmission and xattr correctly.

The new test case reuses the convert facility, and looks just like
convert-tests/001-ext2-basic.

Signed-off-by: Qu Wenruo <quwenruo.btrfs@gmx.com>
---
changelog:
v3.1:
    Cleanup the temporary rootdir directory.
---
 tests/mkfs-tests/010-basic-rootdir/test.sh | 79 ++++++++++++++++++++++++++++++
 1 file changed, 79 insertions(+)
 create mode 100755 tests/mkfs-tests/010-basic-rootdir/test.sh

diff --git a/tests/mkfs-tests/010-basic-rootdir/test.sh b/tests/mkfs-tests/010-basic-rootdir/test.sh
new file mode 100755
index 00000000..49f9b860
--- /dev/null
+++ b/tests/mkfs-tests/010-basic-rootdir/test.sh
@@ -0,0 +1,79 @@
+#!/bin/bash
+# Check basic operations for "mkfs.btrfs --rootdir", including:
+# 1)	Checksum, permission and acl
+#	Should be consistent with source directory
+# 2)	Failure condition
+# 2.1)  Non-existent file/block as destination
+# 2.2)	Too small destination file
+# 2.3)  No privilege to read source directory
+#	All failure condition should fail, but without segfault/backtrace
+
+source "$TOP/tests/common"
+source "$TOP/tests/common.convert"
+
+setup_root_helper
+prepare_test_dev 512M
+check_prereq mkfs.btrfs
+check_global_prereq dd
+check_global_prereq sed
+
+# Since our test dir will be in /tmp, which is nowadays tmpfs for most
+# distributions, and tmpfs xattr doesn't support user xattr, here we
+# use a special populate_fs() which won't create user xattr.
+#
+# Don't worry, both acl and user xattr is implemented by xattr in btrfs,
+# so "acls" should cover the case.
+populate_tmpfs() {
+	_assert_path "$1"
+
+        for dataset_type in 'small' 'hardlink' 'fast_symlink' 'brokenlink' 'perm' 'sparse' 'acls' 'fifo' 'slow_symlink'; do
+		generate_dataset "$dataset_type" "$1"
+	done
+}
+
+# Basic content checker for difference nodesize/features
+content_test() {
+	local features
+	local nodesize
+	local src_dir
+	local csum_tmp 
+	local perm_tmp 
+	local acl_tmp
+
+	features="$1"
+	nodesize="$2"
+	src_dir=$(mktemp --tmpdir --directory btrfs-progs-mkfs-rootdir.XXXXXXX)
+
+	echo "    [TEST/mkfs_rootdir]   nodesize=$nodesize" "${features:-defaults}"
+	echo "creating test dir at $src_dir" >> "$RESULTS"
+
+	populate_tmpfs "$src_dir"
+	csum_tmp=$(mktemp --tmpdir btrfs-progs-convert.XXXXXXXXXX)
+	perm_tmp=$(mktemp --tmpdir btrfs-progs-convert.permXXXXXX)
+	acl_tmp=$(mktemp --tmpdir btrfs-progs-convert.aclsXXXXXXX)
+	convert_test_gen_checksums "$csum_tmp" "$src_dir"
+	convert_test_perm "$perm_tmp" "$src_dir"
+	convert_test_acl "$acl_tmp" "$src_dir"
+
+	run_check "$TOP/mkfs.btrfs" ${1:+-O "$1"} ${2:+-n "$2"} \
+		"--rootdir" "$src_dir" "$TEST_DEV"
+	run_check "$TOP/btrfs" check "$TEST_DEV"
+	run_check_mount_test_dev
+	convert_test_post_check_checksums "$csum_tmp"
+	convert_test_post_check_permissions "$perm_tmp"
+	convert_test_post_check_acl "$acl_tmp"
+	run_check_umount_test_dev
+
+	rm -- "$csum_tmp"
+	rm -- "$perm_tmp"
+	rm -- "$acl_tmp"
+	run_check $SUDO_HELPER rm -rf -- "$src_dir"
+}
+
+for feature in '' 'extref' 'skinny-metadata' 'no-holes'; do
+	content_test "$feature" 4096
+	content_test "$feature" 8192
+	content_test "$feature" 16384
+	content_test "$feature" 32768
+	content_test "$feature" 65536
+done
-- 
2.14.1

[PATCH v3 12/14] btrfs-progs: tests/common: Detect ungraceful failure case

[Qu Wenruo]

run_mustfail() checks the return value but doesn't check if the failure
is graceful.

Add such ungraceful failure check, for incoming "mkfs.btrfs --rootdir"
test cases, as old "--rootdir" never fail gracefully.

Signed-off-by: Qu Wenruo <quwenruo.btrfs@gmx.com>
---
 tests/common | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tests/common b/tests/common
index 242ded1d..b49d9ff3 100644
--- a/tests/common
+++ b/tests/common
@@ -227,6 +227,11 @@ run_mustfail()
 		$INSTRUMENT "$@" >> "$RESULTS" 2>&1
 	fi
 	if [ $? != 0 ]; then
+		if [ $? -gt 128 ]; then
+			"failed (ungracefully): $@" >> "$RESULTS"
+			_fail "unexpected ungraceful failure: $msg"
+			return 1
+		fi
 		echo "failed (expected): $@" >> "$RESULTS"
 		return 0
 	else
-- 
2.14.1

[PATCH v3 13/14] btrfs-progs: mkfs: Fix overwritten return value for mkfs

[Qu Wenruo]

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

Fix it so it can pass incoming test cases.

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

diff --git a/mkfs/main.c b/mkfs/main.c
index f78c24ce..caa5c2e2 100644
--- a/mkfs/main.c
+++ b/mkfs/main.c
@@ -1339,6 +1339,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;
@@ -1814,9 +1815,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) {
-- 
2.14.1

[PATCH v3 14/14] btrfs-progs: tests/mkfs: Check error handler for rootdir parameter

[Qu Wenruo]

Error handlers for the following case must fail gracefully:
1) No permission to read content of rootdir
2) Too small destination file
3) Non-existent destination file
4) Zero sized destination file

Signed-off-by: Qu Wenruo <quwenruo.btrfs@gmx.com>
---
 .../mkfs-tests/011-rootdir-fail-condition/test.sh  | 82 ++++++++++++++++++++++
 1 file changed, 82 insertions(+)
 create mode 100755 tests/mkfs-tests/011-rootdir-fail-condition/test.sh

diff --git a/tests/mkfs-tests/011-rootdir-fail-condition/test.sh b/tests/mkfs-tests/011-rootdir-fail-condition/test.sh
new file mode 100755
index 00000000..efdb4e9f
--- /dev/null
+++ b/tests/mkfs-tests/011-rootdir-fail-condition/test.sh
@@ -0,0 +1,82 @@
+#!/bin/bash
+#
+# Check if "mkfs.btrfs --rootdir" exit gracefully for different error cases
+
+source "$TOP/tests/common"
+source "$TOP/tests/common.convert"
+
+prepare_test_dev 128M 
+check_prereq mkfs.btrfs
+check_global_prereq fallocate
+
+no_permission() {
+	local dir_path
+	local bad_file 
+
+	dir_path=$(mktemp --tmpdir --directory btrfs-progs-mkfs-rootdir.XXXXXX)
+	bad_file="$dir_path/no_read_permission"
+
+	echo "    [TEST/mkfs_rootdir]   error handler for -EPERM"
+	echo "creating test dir at $src_dir" >> "$RESULTS"
+
+	run_check fallocate -l 4k "$bad_file"
+	run_check chmod 000 "$bad_file"
+
+	run_mustfail "no permission to read" "$TOP/mkfs.btrfs" \
+		--rootdir "$dir_path" "$TEST_DEV"
+	rm -rf -- "$dir_path"
+}
+
+too_small_dest() {
+	local dir_path
+	local bad_file 
+
+	dir_path=$(mktemp --tmpdir --directory btrfs-progs-mkfs-rootdir.XXXXXX)
+	bad_file="$dir_path/no_space"
+
+	echo "    [TEST/mkfs_rootdir]   error handler for -ENOSPC"
+	echo "creating test dir at $src_dir" >> "$RESULTS"
+
+	run_check fallocate -l 256M "$bad_file"
+
+	run_mustfail "too small destination file" "$TOP/mkfs.btrfs" \
+		--rootdir "$dir_path" "$TEST_DEV"
+	rm -rf -- "$dir_path"
+}
+
+non_existent_dest() {
+	local dir_path
+	local dest_file
+
+	dir_path=$(mktemp --tmpdir --directory btrfs-progs-mkfs-rootdir.XXXXXX)
+	dest_file=$(mktemp --tmpdir btrfs-progs-mkfs-dest-file.XXXX)
+	run_check rm -- "$dest_file"
+
+	echo "    [TEST/mkfs_rootdir]   error handler for non-existent destination file"
+	echo "creating test dir at $src_dir" >> "$RESULTS"
+
+	run_mustfail "non-existent destination file" "$TOP/mkfs.btrfs" \
+		--rootdir "$dir_path" "$dest_file"
+	rm -rf -- "$dir_path"
+}
+
+zero_sized_dest() {
+	local dir_path
+	local dest_file
+
+	dir_path=$(mktemp --tmpdir --directory btrfs-progs-mkfs-rootdir.XXXXXX)
+	dest_file=$(mktemp --tmpdir btrfs-progs-mkfs-dest-file.XXXX)
+
+	echo "    [TEST/mkfs_rootdir]   error handler for zero-sized destination file"
+	echo "creating test dir at $src_dir" >> "$RESULTS"
+
+	run_mustfail "zero-sized destination file" "$TOP/mkfs.btrfs" \
+		--rootdir "$dir_path" "$dest_file"
+	rm -rf -- "$dir_path"
+	rm -- "$dest_file"
+}
+
+no_permission
+too_small_dest
+non_existent_dest
+zero_sized_dest
-- 
2.14.1