diff mbox series

[v4] btrfs-progs: add --subvol option to mkfs.btrfs

Message ID 20240807151707.2828988-1-maharmstone@fb.com (mailing list archive)
State New, archived
Headers show
Series [v4] btrfs-progs: add --subvol option to mkfs.btrfs | expand

Commit Message

Mark Harmstone Aug. 7, 2024, 3:16 p.m. UTC
This patch adds a --subvol option, which tells mkfs.btrfs to create the
specified directories as subvolumes.

Given a populated directory img, the command

$ mkfs.btrfs --rootdir img --subvol img/usr --subvol img/home --subvol img/home/username /dev/loop0

will create subvolumes usr and home within the FS root, and subvolume
username within the home subvolume. It will fail if any of the
directories do not yet exist.

Signed-off-by: Mark Harmstone <maharmstone@fb.com>
---

Changelog:

Patch 2:
* Rebased against upstream changes
* Rewrote so that directory sizes are correct within transactions
* Changed --subvol so that it is relative to cwd rather than rootdir, so
that in future we might allow out-of-tree subvols

Patch 3:
* Changed btrfs_mkfs_fill_dir so it doesn't start a transaction itself
* Moved subvol creation and linking into traverse_directory
* Removed depth calculation code, no longer needed

Patch 4:
* Rebased against upstream changes

 mkfs/main.c                                 | 155 ++++++++++++++++++--
 mkfs/rootdir.c                              | 132 +++++++++++++----
 mkfs/rootdir.h                              |   9 +-
 tests/mkfs-tests/036-rootdir-subvol/test.sh |  33 +++++
 4 files changed, 287 insertions(+), 42 deletions(-)
 create mode 100755 tests/mkfs-tests/036-rootdir-subvol/test.sh

Comments

Qu Wenruo Aug. 7, 2024, 10:37 p.m. UTC | #1
在 2024/8/8 00:46, Mark Harmstone 写道:
> This patch adds a --subvol option, which tells mkfs.btrfs to create the
> specified directories as subvolumes.
>
> Given a populated directory img, the command
>
> $ mkfs.btrfs --rootdir img --subvol img/usr --subvol img/home --subvol img/home/username /dev/loop0
>
> will create subvolumes usr and home within the FS root, and subvolume
> username within the home subvolume. It will fail if any of the
> directories do not yet exist.
>
> Signed-off-by: Mark Harmstone <maharmstone@fb.com>

This version looks much better.

Reviewed-by: Qu Wenruo <wqu@suse.com>

Just some small code style related comments.

[...]
> @@ -1272,6 +1291,77 @@ int BOX_MAIN(mkfs)(int argc, char **argv)
>   		ret = 1;
>   		goto error;
>   	}
> +	if (!list_empty(&subvols) && source_dir == NULL) {
> +		error("the option --subvol must be used with --rootdir");
> +		ret = 1;
> +		goto error;
> +	}
> +
> +	if (source_dir) {
> +		char *canonical = realpath(source_dir, NULL);
> +
> +		if (!canonical) {
> +			error("could not get canonical path to %s", source_dir);
> +			ret = 1;
> +			goto error;
> +		}
> +
> +		free(source_dir);
> +		source_dir = canonical;
> +	}
> +
> +	if (!list_empty(&subvols)) {

We can skip the list_empty() check, as the later list_for_each_entry()
can handle empty list pretty well.

This will save us one indent.

[...]
> @@ -1821,24 +1911,42 @@ raid_groups:
>   		error_msg(ERROR_MSG_START_TRANS, "%m");
>   		goto out;
>   	}
> -	ret = btrfs_rebuild_uuid_tree(fs_info);
> -	if (ret < 0)
> -		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) {
>   		pr_verbose(LOG_DEFAULT, "Rootdir from:       %s\n", source_dir);
> -		ret = btrfs_mkfs_fill_dir(source_dir, root);
> +
> +		trans = btrfs_start_transaction(root, 1);
> +		if (IS_ERR(trans)) {
> +			errno = -PTR_ERR(trans);
> +			error_msg(ERROR_MSG_START_TRANS, "%m");
> +			goto out;
> +		}
> +
> +		ret = btrfs_mkfs_fill_dir(trans, source_dir, root,
> +					  &subvols);
>   		if (ret) {
>   			error("error while filling filesystem: %d", ret);
> +			btrfs_abort_transaction(trans, ret);
>   			goto out;
>   		}
> +
> +		ret = btrfs_commit_transaction(trans, root);
> +		if (ret) {
> +			errno = -ret;
> +			error_msg(ERROR_MSG_COMMIT_TRANS, "%m");
> +			goto out;
> +		}
> +
> +		if (!list_empty(&subvols)) {

Same here.

[...]
> @@ -511,6 +590,20 @@ static int ftw_add_inode(const char *full_path, const struct stat *st,
>   	while (current_path.level > ftwbuf->level)
>   		rootdir_path_pop(&current_path);
>
> +	if (!list_empty(g_subvols) && S_ISDIR(st->st_mode)) {

The same here.

> +		struct rootdir_subvol *s;
> +
> +		list_for_each_entry(s, g_subvols, list) {
> +			if (!strcmp(full_path, s->full_path)) {
> +				return ftw_add_subvol(full_path, st, typeflag,
> +						      ftwbuf, s);

Another optimization can be, to remove @s from the g_subvols list.
So that the next search will spend a little less time to do the search.

Thanks,
Qu
Mark Harmstone Aug. 8, 2024, 5:04 p.m. UTC | #2
Thanks Qu.

On 7/8/24 23:37, Qu Wenruo wrote:
>> @@ -1272,6 +1291,77 @@ int BOX_MAIN(mkfs)(int argc, char **argv)
>>           ret = 1;
>>           goto error;
>>       }
>> +    if (!list_empty(&subvols) && source_dir == NULL) {
>> +        error("the option --subvol must be used with --rootdir");
>> +        ret = 1;
>> +        goto error;
>> +    }
>> +
>> +    if (source_dir) {
>> +        char *canonical = realpath(source_dir, NULL);
>> +
>> +        if (!canonical) {
>> +            error("could not get canonical path to %s", source_dir);
>> +            ret = 1;
>> +            goto error;
>> +        }
>> +
>> +        free(source_dir);
>> +        source_dir = canonical;
>> +    }
>> +
>> +    if (!list_empty(&subvols)) {
> 
> We can skip the list_empty() check, as the later list_for_each_entry()
> can handle empty list pretty well.

I did this to avoid a strlen call if there's no subvols, but that's not 
very important.

>> +        struct rootdir_subvol *s;
>> +
>> +        list_for_each_entry(s, g_subvols, list) {
>> +            if (!strcmp(full_path, s->full_path)) {
>> +                return ftw_add_subvol(full_path, st, typeflag,
>> +                              ftwbuf, s);
> 
> Another optimization can be, to remove @s from the g_subvols list.
> So that the next search will spend a little less time to do the search.

Unfortunately you can't do that, as it means that the pointer will be 
missed in the mkfs_main cleanup.

Mark
Qu Wenruo Aug. 8, 2024, 10:10 p.m. UTC | #3
在 2024/8/9 02:34, Mark Harmstone 写道:
> Thanks Qu.
> 
> On 7/8/24 23:37, Qu Wenruo wrote:
>>> @@ -1272,6 +1291,77 @@ int BOX_MAIN(mkfs)(int argc, char **argv)
>>>            ret = 1;
>>>            goto error;
>>>        }
>>> +    if (!list_empty(&subvols) && source_dir == NULL) {
>>> +        error("the option --subvol must be used with --rootdir");
>>> +        ret = 1;
>>> +        goto error;
>>> +    }
>>> +
>>> +    if (source_dir) {
>>> +        char *canonical = realpath(source_dir, NULL);
>>> +
>>> +        if (!canonical) {
>>> +            error("could not get canonical path to %s", source_dir);
>>> +            ret = 1;
>>> +            goto error;
>>> +        }
>>> +
>>> +        free(source_dir);
>>> +        source_dir = canonical;
>>> +    }
>>> +
>>> +    if (!list_empty(&subvols)) {
>>
>> We can skip the list_empty() check, as the later list_for_each_entry()
>> can handle empty list pretty well.
> 
> I did this to avoid a strlen call if there's no subvols, but that's not
> very important.
> 
>>> +        struct rootdir_subvol *s;
>>> +
>>> +        list_for_each_entry(s, g_subvols, list) {
>>> +            if (!strcmp(full_path, s->full_path)) {
>>> +                return ftw_add_subvol(full_path, st, typeflag,
>>> +                              ftwbuf, s);
>>
>> Another optimization can be, to remove @s from the g_subvols list.
>> So that the next search will spend a little less time to do the search.
> 
> Unfortunately you can't do that, as it means that the pointer will be
> missed in the mkfs_main cleanup.

Why? You removed it from the list and free the memory.

And at the final cleanup, there will be no entry left.

Thanks,
Qu
> 
> Mark
>
Mark Harmstone Aug. 9, 2024, 9:09 a.m. UTC | #4
On 8/8/24 23:10, Qu Wenruo wrote:
> 
> Why? You removed it from the list and free the memory.
> 
> And at the final cleanup, there will be no entry left.

Okay - if you can commit patch v5 to the repo, I'll send through a patch 
for this.

Thanks

Mark
diff mbox series

Patch

diff --git a/mkfs/main.c b/mkfs/main.c
index b24b148d..9e0e5fc9 100644
--- a/mkfs/main.c
+++ b/mkfs/main.c
@@ -440,6 +440,7 @@  static const char * const mkfs_usage[] = {
 	"Creation:",
 	OPTLINE("-b|--byte-count SIZE", "set size of each device to SIZE (filesystem size is sum of all device sizes)"),
 	OPTLINE("-r|--rootdir DIR", "copy files from DIR to the image root directory"),
+	OPTLINE("-u|--subvol SUBDIR", "create SUBDIR as subvolume rather than normal directory"),
 	OPTLINE("--shrink", "(with --rootdir) shrink the filled filesystem to minimal size"),
 	OPTLINE("-K|--nodiscard", "do not perform whole device TRIM"),
 	OPTLINE("-f|--force", "force overwrite of existing filesystem"),
@@ -1055,6 +1056,7 @@  int BOX_MAIN(mkfs)(int argc, char **argv)
 	char *label = NULL;
 	int nr_global_roots = sysconf(_SC_NPROCESSORS_ONLN);
 	char *source_dir = NULL;
+	LIST_HEAD(subvols);
 
 	cpu_detect_flags();
 	hash_init_accel();
@@ -1085,6 +1087,7 @@  int BOX_MAIN(mkfs)(int argc, char **argv)
 			{ "data", required_argument, NULL, 'd' },
 			{ "version", no_argument, NULL, 'V' },
 			{ "rootdir", required_argument, NULL, 'r' },
+			{ "subvol", required_argument, NULL, 'u' },
 			{ "nodiscard", no_argument, NULL, 'K' },
 			{ "features", required_argument, NULL, 'O' },
 			{ "runtime-features", required_argument, NULL, 'R' },
@@ -1102,7 +1105,7 @@  int BOX_MAIN(mkfs)(int argc, char **argv)
 			{ NULL, 0, NULL, 0}
 		};
 
-		c = getopt_long(argc, argv, "A:b:fl:n:s:m:d:L:R:O:r:U:VvMKq",
+		c = getopt_long(argc, argv, "A:b:fl:n:s:m:d:L:R:O:r:U:VvMKqu:",
 				long_options, NULL);
 		if (c < 0)
 			break;
@@ -1208,6 +1211,22 @@  int BOX_MAIN(mkfs)(int argc, char **argv)
 				free(source_dir);
 				source_dir = strdup(optarg);
 				break;
+			case 'u': {
+				struct rootdir_subvol *s;
+
+				s = malloc(sizeof(struct rootdir_subvol));
+				if (!s) {
+					error("out of memory");
+					ret = 1;
+					goto error;
+				}
+
+				s->dir = strdup(optarg);
+				s->full_path = NULL;
+
+				list_add_tail(&s->list, &subvols);
+				break;
+				}
 			case 'U':
 				strncpy_null(fs_uuid, optarg, BTRFS_UUID_UNPARSED_SIZE);
 				break;
@@ -1272,6 +1291,77 @@  int BOX_MAIN(mkfs)(int argc, char **argv)
 		ret = 1;
 		goto error;
 	}
+	if (!list_empty(&subvols) && source_dir == NULL) {
+		error("the option --subvol must be used with --rootdir");
+		ret = 1;
+		goto error;
+	}
+
+	if (source_dir) {
+		char *canonical = realpath(source_dir, NULL);
+
+		if (!canonical) {
+			error("could not get canonical path to %s", source_dir);
+			ret = 1;
+			goto error;
+		}
+
+		free(source_dir);
+		source_dir = canonical;
+	}
+
+	if (!list_empty(&subvols)) {
+		size_t source_dir_len = strlen(source_dir);
+		struct rootdir_subvol *s;
+
+		list_for_each_entry(s, &subvols, list) {
+			char *path;
+			struct rootdir_subvol *s2;
+
+			if (!path_exists(s->dir)) {
+				error("subvol %s does not exist",
+				      s->dir);
+				ret = 1;
+				goto error;
+			}
+
+			if (!path_is_dir(s->dir)) {
+				error("subvol %s is not a directory", s->dir);
+				ret = 1;
+				goto error;
+			}
+
+			path = realpath(s->dir, NULL);
+
+			if (!path) {
+				error("could not get canonical path to %s",
+				      s->dir);
+				ret = 1;
+				goto error;
+			}
+
+			s->full_path = path;
+
+			if (strlen(path) < source_dir_len + 1 ||
+			    memcmp(path, source_dir, source_dir_len) ||
+			    path[source_dir_len] != '/') {
+				error("subvol %s is not a child of %s",
+				      s->dir, source_dir);
+				ret = 1;
+				goto error;
+			}
+
+			for (s2 = list_first_entry(&subvols, struct rootdir_subvol, list);
+			     s2 != s; s2 = list_next_entry(s2, list)) {
+				if (!strcmp(s2->full_path, path)) {
+					error("subvol %s specified more than once",
+					      s->dir);
+					ret = 1;
+					goto error;
+				}
+			}
+		}
+	}
 
 	if (*fs_uuid) {
 		uuid_t dummy_uuid;
@@ -1821,24 +1911,42 @@  raid_groups:
 		error_msg(ERROR_MSG_START_TRANS, "%m");
 		goto out;
 	}
-	ret = btrfs_rebuild_uuid_tree(fs_info);
-	if (ret < 0)
-		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) {
 		pr_verbose(LOG_DEFAULT, "Rootdir from:       %s\n", source_dir);
-		ret = btrfs_mkfs_fill_dir(source_dir, root);
+
+		trans = btrfs_start_transaction(root, 1);
+		if (IS_ERR(trans)) {
+			errno = -PTR_ERR(trans);
+			error_msg(ERROR_MSG_START_TRANS, "%m");
+			goto out;
+		}
+
+		ret = btrfs_mkfs_fill_dir(trans, source_dir, root,
+					  &subvols);
 		if (ret) {
 			error("error while filling filesystem: %d", ret);
+			btrfs_abort_transaction(trans, ret);
 			goto out;
 		}
+
+		ret = btrfs_commit_transaction(trans, root);
+		if (ret) {
+			errno = -ret;
+			error_msg(ERROR_MSG_COMMIT_TRANS, "%m");
+			goto out;
+		}
+
+		if (!list_empty(&subvols)) {
+			struct rootdir_subvol *s;
+
+			list_for_each_entry(s, &subvols, list) {
+				pr_verbose(LOG_DEFAULT,
+					   "  Subvol from:      %s\n",
+					   s->full_path);
+			}
+		}
+
 		if (shrink_rootdir) {
 			pr_verbose(LOG_DEFAULT, "  Shrink:           yes\n");
 			ret = btrfs_mkfs_shrink_fs(fs_info, &shrink_size,
@@ -1853,6 +1961,17 @@  raid_groups:
 		}
 	}
 
+	ret = btrfs_rebuild_uuid_tree(fs_info);
+	if (ret < 0)
+		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 (features.runtime_flags & BTRFS_FEATURE_RUNTIME_QUOTA ||
 	    features.incompat_flags & BTRFS_FEATURE_INCOMPAT_SIMPLE_QUOTA) {
 		ret = setup_quota_root(fs_info);
@@ -1946,6 +2065,18 @@  error:
 	free(label);
 	free(source_dir);
 
+	while (!list_empty(&subvols)) {
+		struct rootdir_subvol *head = list_entry(subvols.next,
+					      struct rootdir_subvol,
+					      list);
+
+		free(head->dir);
+		free(head->full_path);
+
+		list_del(&head->list);
+		free(head);
+	}
+
 	return !!ret;
 
 success:
diff --git a/mkfs/rootdir.c b/mkfs/rootdir.c
index 05787dc3..24c57fed 100644
--- a/mkfs/rootdir.c
+++ b/mkfs/rootdir.c
@@ -40,6 +40,8 @@ 
 #include "common/messages.h"
 #include "common/utils.h"
 #include "common/extent-tree-utils.h"
+#include "common/root-tree-utils.h"
+#include "common/path-utils.h"
 #include "mkfs/rootdir.h"
 
 static u32 fs_block_size;
@@ -68,6 +70,7 @@  static u64 ftw_data_size;
 struct inode_entry {
 	/* The inode number inside btrfs. */
 	u64 ino;
+	struct btrfs_root *root;
 	struct list_head list;
 };
 
@@ -91,6 +94,8 @@  static struct rootdir_path current_path = {
 };
 
 static struct btrfs_trans_handle *g_trans = NULL;
+static struct list_head *g_subvols;
+static u64 next_subvol_id = BTRFS_FIRST_FREE_OBJECTID;
 
 static inline struct inode_entry *rootdir_path_last(struct rootdir_path *path)
 {
@@ -111,13 +116,15 @@  static void rootdir_path_pop(struct rootdir_path *path)
 	free(last);
 }
 
-static int rootdir_path_push(struct rootdir_path *path, u64 ino)
+static int rootdir_path_push(struct rootdir_path *path, struct btrfs_root *root,
+			     u64 ino)
 {
 	struct inode_entry *new;
 
 	new = malloc(sizeof(*new));
 	if (!new)
 		return -ENOMEM;
+	new->root = root;
 	new->ino = ino;
 	list_add_tail(&new->list, &path->inode_list);
 	path->level++;
@@ -409,11 +416,80 @@  static u8 ftype_to_btrfs_type(mode_t ftype)
 	return BTRFS_FT_UNKNOWN;
 }
 
+static int ftw_add_subvol(const char *full_path, const struct stat *st,
+			  int typeflag, struct FTW *ftwbuf,
+			  struct rootdir_subvol *s)
+{
+	int ret;
+	struct btrfs_key key;
+	struct btrfs_root *new_root;
+	struct inode_entry *parent;
+	struct btrfs_inode_item inode_item = { 0 };
+	u64 subvol_id, ino;
+
+	subvol_id = next_subvol_id++;
+
+	ret = btrfs_make_subvolume(g_trans, subvol_id);
+	if (ret < 0) {
+		error("failed to create subvolume: %d", ret);
+		return ret;
+	}
+
+	key.objectid = subvol_id;
+	key.type = BTRFS_ROOT_ITEM_KEY;
+	key.offset = (u64)-1;
+
+	new_root = btrfs_read_fs_root(g_trans->fs_info, &key);
+	if (IS_ERR(new_root)) {
+		error("unable to fs read root: %lu", PTR_ERR(new_root));
+		return -PTR_ERR(new_root);
+	}
+
+	parent = rootdir_path_last(&current_path);
+
+	ret = btrfs_link_subvolume(g_trans, parent->root, parent->ino,
+				   path_basename(s->full_path),
+				   strlen(path_basename(s->full_path)), new_root);
+	if (ret) {
+		error("unable to link subvolume %s", path_basename(s->full_path));
+		return ret;
+	}
+
+	ino = btrfs_root_dirid(&new_root->root_item);
+
+	ret = add_xattr_item(g_trans, new_root, ino, full_path);
+	if (ret < 0) {
+		errno = -ret;
+		error("failed to add xattr item for the top level inode in subvol %llu: %m",
+		      subvol_id);
+		return ret;
+	}
+	stat_to_inode_item(&inode_item, st);
+
+	btrfs_set_stack_inode_nlink(&inode_item, 1);
+	ret = update_inode_item(g_trans, new_root, &inode_item, ino);
+	if (ret < 0) {
+		errno = -ret;
+		error("failed to update root dir for root %llu: %m", subvol_id);
+		return ret;
+	}
+
+	ret = rootdir_path_push(&current_path, new_root, ino);
+	if (ret < 0) {
+		errno = -ret;
+		error("failed to allocate new entry for subvol %llu ('%s'): %m",
+		      subvol_id, full_path);
+		return ret;
+	}
+
+	return 0;
+}
+
 static int ftw_add_inode(const char *full_path, const struct stat *st,
 			 int typeflag, struct FTW *ftwbuf)
 {
 	struct btrfs_fs_info *fs_info = g_trans->fs_info;
-	struct btrfs_root *root = fs_info->fs_root;
+	struct btrfs_root *root;
 	struct btrfs_inode_item inode_item = { 0 };
 	struct inode_entry *parent;
 	u64 ino;
@@ -436,7 +512,10 @@  static int ftw_add_inode(const char *full_path, const struct stat *st,
 
 	/* The rootdir itself. */
 	if (unlikely(ftwbuf->level == 0)) {
-		u64 root_ino = btrfs_root_dirid(&root->root_item);
+		u64 root_ino;
+
+		root = fs_info->fs_root;
+		root_ino = btrfs_root_dirid(&root->root_item);
 
 		UASSERT(S_ISDIR(st->st_mode));
 		UASSERT(current_path.level == 0);
@@ -462,7 +541,7 @@  static int ftw_add_inode(const char *full_path, const struct stat *st,
 		}
 
 		/* Push (and initialize) the rootdir directory into the stack. */
-		ret = rootdir_path_push(&current_path,
+		ret = rootdir_path_push(&current_path, root,
 					btrfs_root_dirid(&root->root_item));
 		if (ret < 0) {
 			errno = -ret;
@@ -511,6 +590,20 @@  static int ftw_add_inode(const char *full_path, const struct stat *st,
 	while (current_path.level > ftwbuf->level)
 		rootdir_path_pop(&current_path);
 
+	if (!list_empty(g_subvols) && S_ISDIR(st->st_mode)) {
+		struct rootdir_subvol *s;
+
+		list_for_each_entry(s, g_subvols, list) {
+			if (!strcmp(full_path, s->full_path)) {
+				return ftw_add_subvol(full_path, st, typeflag,
+						      ftwbuf, s);
+			}
+		}
+	}
+
+	parent = rootdir_path_last(&current_path);
+	root = parent->root;
+
 	ret = btrfs_find_free_objectid(g_trans, root,
 				       BTRFS_FIRST_FREE_OBJECTID, &ino);
 	if (ret < 0) {
@@ -529,7 +622,6 @@  static int ftw_add_inode(const char *full_path, const struct stat *st,
 		return ret;
 	}
 
-	parent = rootdir_path_last(&current_path);
 	ret = btrfs_add_link(g_trans, root, ino, parent->ino,
 			     full_path + ftwbuf->base,
 			     strlen(full_path) - ftwbuf->base,
@@ -556,7 +648,7 @@  static int ftw_add_inode(const char *full_path, const struct stat *st,
 		return ret;
 	}
 	if (S_ISDIR(st->st_mode)) {
-		ret = rootdir_path_push(&current_path, ino);
+		ret = rootdir_path_push(&current_path, root, ino);
 		if (ret < 0) {
 			errno = -ret;
 			error("failed to allocate new entry for inode %llu ('%s'): %m",
@@ -597,49 +689,31 @@  static int ftw_add_inode(const char *full_path, const struct stat *st,
 	return 0;
 };
 
-int btrfs_mkfs_fill_dir(const char *source_dir, struct btrfs_root *root)
+int btrfs_mkfs_fill_dir(struct btrfs_trans_handle *trans, const char *source_dir,
+			struct btrfs_root *root, struct list_head *subvols)
 {
 	int ret;
-	struct btrfs_trans_handle *trans;
 	struct stat root_st;
 
 	ret = lstat(source_dir, &root_st);
 	if (ret) {
 		error("unable to lstat %s: %m", source_dir);
-		ret = -errno;
-		goto out;
-	}
-
-	trans = btrfs_start_transaction(root, 1);
-	if (IS_ERR(trans)) {
-		ret = PTR_ERR(trans);
-		errno = -ret;
-		error_msg(ERROR_MSG_START_TRANS, "%m");
-		goto fail;
+		return -errno;
 	}
 
 	g_trans = trans;
+	g_subvols = subvols;
 	INIT_LIST_HEAD(&current_path.inode_list);
 
 	ret = nftw(source_dir, ftw_add_inode, 32, FTW_PHYS);
 	if (ret) {
 		error("unable to traverse directory %s: %d", source_dir, ret);
-		goto fail;
-	}
-	ret = btrfs_commit_transaction(trans, root);
-	if (ret) {
-		errno = -ret;
-		error_msg(ERROR_MSG_COMMIT_TRANS, "%m");
-		goto out;
+		return ret;
 	}
 	while (current_path.level > 0)
 		rootdir_path_pop(&current_path);
 
 	return 0;
-fail:
-	btrfs_abort_transaction(trans, ret);
-out:
-	return ret;
 }
 
 static int ftw_add_entry_size(const char *fpath, const struct stat *st,
diff --git a/mkfs/rootdir.h b/mkfs/rootdir.h
index 4233431a..128e9e09 100644
--- a/mkfs/rootdir.h
+++ b/mkfs/rootdir.h
@@ -28,7 +28,14 @@ 
 struct btrfs_fs_info;
 struct btrfs_root;
 
-int btrfs_mkfs_fill_dir(const char *source_dir, struct btrfs_root *root);
+struct rootdir_subvol {
+	struct list_head list;
+	char *dir;
+	char *full_path;
+};
+
+int btrfs_mkfs_fill_dir(struct btrfs_trans_handle *trans, const char *source_dir,
+			struct btrfs_root *root, struct list_head *subvols);
 u64 btrfs_mkfs_size_dir(const char *dir_name, u32 sectorsize, u64 min_dev_size,
 			u64 meta_profile, u64 data_profile);
 int btrfs_mkfs_shrink_fs(struct btrfs_fs_info *fs_info, u64 *new_size_ret,
diff --git a/tests/mkfs-tests/036-rootdir-subvol/test.sh b/tests/mkfs-tests/036-rootdir-subvol/test.sh
new file mode 100755
index 00000000..ccd6893f
--- /dev/null
+++ b/tests/mkfs-tests/036-rootdir-subvol/test.sh
@@ -0,0 +1,33 @@ 
+#!/bin/bash
+# smoke test for mkfs.btrfs --subvol option
+
+source "$TEST_TOP/common" || exit
+
+check_prereq mkfs.btrfs
+check_prereq btrfs
+
+setup_root_helper
+prepare_test_dev
+
+tmp=$(_mktemp_dir mkfs-rootdir)
+
+touch $tmp/foo
+mkdir $tmp/dir
+mkdir $tmp/dir/subvol
+touch $tmp/dir/subvol/bar
+
+run_check_mkfs_test_dev --rootdir "$tmp" --subvol "$tmp/dir/subvol"
+run_check $SUDO_HELPER "$TOP/btrfs" check "$TEST_DEV"
+
+run_check_mount_test_dev
+run_check_stdout $SUDO_HELPER "$TOP/btrfs" subvolume list "$TEST_MNT" | \
+	cut -d\  -f9 > "$tmp/output"
+run_check_umount_test_dev
+
+result=$(cat "$tmp/output")
+
+if [ "$result" != "dir/subvol" ]; then
+	_fail "dir/subvol not in subvolume list"
+fi
+
+rm -rf -- "$tmp"