Btrfs devel - Aug 2013 - [PATCH RFC] Btrfs: add support for persistent mount options

This change allows for most mount options to be persisted in
the filesystem, and be applied when the filesystem is mounted.
If the same options are specified at mount time, the persisted
values for those options are ignored.

The only options not supported are: subvol, subvolid, subvolrootid,
device and thread_pool. This limitation is due to how this feature
is implemented: basically there''s an optional value (of type
struct btrfs_dir_item) in the tree of tree roots used to store the
list of options in the same format as they are passed to btrfs_mount().
This means any mount option that takes effect before the tree of tree
roots is setup is not supported.

To set these options, the user space tool btrfstune was modified
to persist the list of options into an unmounted filesystem''s
tree of tree roots.

NOTE: Like the corresponding btrfs-progs patch, this is a WIP with
the goal o gathering feedback.

Signed-off-by: Filipe David Borba Manana <fdmanana@gmail.com>
---
 fs/btrfs/ctree.h   |   11 +++++++-
 fs/btrfs/disk-io.c |   72 +++++++++++++++++++++++++++++++++++++++++++++++++++-
 fs/btrfs/super.c   |   46 +++++++++++++++++++++++++++++++--
 3 files changed, 125 insertions(+), 4 deletions(-)

diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
index cbb1263..24363df 100644
--- a/fs/btrfs/ctree.h
+++ b/fs/btrfs/ctree.h
@@ -121,6 +121,14 @@ struct btrfs_ordered_sum;
  */
 #define BTRFS_FREE_INO_OBJECTID -12ULL
 
+/*
+ * Item that stores permanent mount options. These options
+ * have effect if they are not specified as well at mount
+ * time (that is, if a permanent option is also specified at
+ * mount time, the later wins).
+ */
+#define BTRFS_PERSISTENT_OPTIONS_OBJECTID -13ULL
+
 /* dummy objectid represents multiple objectids */
 #define BTRFS_MULTIPLE_OBJECTIDS -255ULL
 
@@ -3739,7 +3747,8 @@ void btrfs_exit_sysfs(void);
 ssize_t btrfs_listxattr(struct dentry *dentry, char *buffer, size_t size);
 
 /* super.c */
-int btrfs_parse_options(struct btrfs_root *root, char *options);
+int btrfs_parse_options(struct btrfs_root *root, char *options,
+			int parsing_persistent, int **options_parsed);
 int btrfs_sync_fs(struct super_block *sb, int wait);
 
 #ifdef CONFIG_PRINTK
diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
index 254cdc8..eeabdd4 100644
--- a/fs/btrfs/disk-io.c
+++ b/fs/btrfs/disk-io.c
@@ -2077,6 +2077,53 @@ static void del_fs_roots(struct btrfs_fs_info *fs_info)
 	}
 }
 
+static char *get_persistent_options(struct btrfs_root *tree_root)
+{
+	int ret;
+	struct btrfs_key key;
+	struct btrfs_path *path;
+	struct extent_buffer *leaf;
+	struct btrfs_dir_item *di;
+	u32 name_len, data_len;
+	char *options = NULL;
+
+	path = btrfs_alloc_path();
+	if (!path)
+		return ERR_PTR(-ENOMEM);
+
+	key.objectid = BTRFS_PERSISTENT_OPTIONS_OBJECTID;
+	key.type = 0;
+	key.offset = 0;
+
+	ret = btrfs_search_slot(NULL, tree_root, &key, path, 0, 0);
+	if (ret < 0)
+		goto out;
+	if (ret > 0) {
+		ret = 0;
+		goto out;
+	}
+
+	leaf = path->nodes[0];
+	di = btrfs_item_ptr(leaf, path->slots[0], struct btrfs_dir_item);
+	name_len = btrfs_dir_name_len(leaf, di);
+	data_len = btrfs_dir_data_len(leaf, di);
+	options = kmalloc(data_len + 1, GFP_NOFS);
+	if (!options) {
+		ret = -ENOMEM;
+		goto out;
+	}
+	read_extent_buffer(leaf, options,
+			   (unsigned long)((char *)(di + 1) + name_len),
+			   data_len);
+	options[data_len] = ''\0'';
+
+out:
+	btrfs_free_path(path);
+	if (ret)
+		return ERR_PTR(ret);
+	return options;
+}
+
 int open_ctree(struct super_block *sb,
 	       struct btrfs_fs_devices *fs_devices,
 	       char *options)
@@ -2103,6 +2150,8 @@ int open_ctree(struct super_block *sb,
 	int err = -EINVAL;
 	int num_backups_tried = 0;
 	int backup_index = 0;
+	int *mnt_options = NULL;
+	char *persist_options = NULL;
 
 	tree_root = fs_info->tree_root = btrfs_alloc_root(fs_info);
 	chunk_root = fs_info->chunk_root = btrfs_alloc_root(fs_info);
@@ -2372,7 +2421,7 @@ int open_ctree(struct super_block *sb,
 	 */
 	fs_info->compress_type = BTRFS_COMPRESS_ZLIB;
 
-	ret = btrfs_parse_options(tree_root, options);
+	ret = btrfs_parse_options(tree_root, options, 0, &mnt_options);
 	if (ret) {
 		err = ret;
 		goto fail_alloc;
@@ -2656,6 +2705,26 @@ retry_root_backup:
 	btrfs_set_root_node(&tree_root->root_item, tree_root->node);
 	tree_root->commit_root = btrfs_root_node(tree_root);
 
+	persist_options = get_persistent_options(tree_root);
+	if (IS_ERR(persist_options)) {
+		ret = PTR_ERR(persist_options);
+		goto fail_tree_roots;
+	} else if (persist_options) {
+		ret = btrfs_parse_options(tree_root, persist_options,
+					  1, &mnt_options);
+		kfree(mnt_options);
+		mnt_options = NULL;
+		if (ret) {
+			err = ret;
+			goto fail_tree_roots;
+		}
+		if (tree_root->fs_info->compress_type == BTRFS_COMPRESS_LZO) {
+			features = btrfs_super_incompat_flags(disk_super);
+			features |= BTRFS_FEATURE_INCOMPAT_COMPRESS_LZO;
+			btrfs_set_super_incompat_flags(disk_super, features);
+		}
+	}
+
 	location.objectid = BTRFS_EXTENT_TREE_OBJECTID;
 	location.type = BTRFS_ROOT_ITEM_KEY;
 	location.offset = 0;
@@ -2904,6 +2973,7 @@ fail_block_groups:
 	btrfs_free_block_groups(fs_info);
 
 fail_tree_roots:
+	kfree(mnt_options);
 	free_root_pointers(fs_info, 1);
 	invalidate_inode_pages2(fs_info->btree_inode->i_mapping);
 
diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
index 2cc5b80..ced0a85 100644
--- a/fs/btrfs/super.c
+++ b/fs/btrfs/super.c
@@ -369,7 +369,8 @@ static match_table_t tokens = {
  * reading in a new superblock is parsed here.
  * XXX JDM: This needs to be cleaned up for remount.
  */
-int btrfs_parse_options(struct btrfs_root *root, char *options)
+int btrfs_parse_options(struct btrfs_root *root, char *options,
+			int parsing_persistent, int **options_parsed)
 {
 	struct btrfs_fs_info *info = root->fs_info;
 	substring_t args[MAX_OPT_ARGS];
@@ -379,11 +380,21 @@ int btrfs_parse_options(struct btrfs_root *root, char
*options)
 	int ret = 0;
 	char *compress_type;
 	bool compress_force = false;
+	int *parsed = NULL;
 
 	cache_gen = btrfs_super_cache_generation(root->fs_info->super_copy);
 	if (cache_gen)
 		btrfs_set_opt(info->mount_opt, SPACE_CACHE);
 
+	if (!parsing_persistent && options_parsed) {
+		parsed = kzalloc(sizeof(int) * ARRAY_SIZE(tokens), GFP_NOFS);
+		if (!parsed)
+			return -ENOMEM;
+		*options_parsed = parsed;
+	} else if (parsing_persistent && options_parsed) {
+		parsed = *options_parsed;
+	}
+
 	if (!options)
 		goto out;
 
@@ -403,6 +414,37 @@ int btrfs_parse_options(struct btrfs_root *root, char
*options)
 			continue;
 
 		token = match_token(p, tokens, args);
+
+		if (parsing_persistent && parsed) {
+			/*
+			 * A persistent option value is ignored if a value for
+			 * that option was given at mount time.
+			 */
+
+			if (parsed[token])
+				continue;
+			if (token == Opt_no_space_cache &&
+			    parsed[Opt_space_cache])
+				continue;
+			if (token == Opt_space_cache &&
+			    parsed[Opt_no_space_cache])
+				continue;
+
+			if (token == Opt_subvol)
+				printk(KERN_WARNING "btrfs: subvol not supported as a persistent
option");
+			else if (token == Opt_subvolid)
+				printk(KERN_WARNING "btrfs: subvolid not supported as a persistent
option");
+			else if (token == Opt_subvolrootid)
+				printk(KERN_WARNING "btrfs: subvolrootid not supported as a persistent
option");
+			else if (token == Opt_device)
+				printk(KERN_WARNING "btrfs: device not supported as a persistent
option");
+			else if (token == Opt_thread_pool)
+				printk(KERN_WARNING "btrfs: thread_pool not supported as a persistent
option");
+		}
+
+		if (!parsing_persistent && parsed)
+			parsed[token] = 1;
+
 		switch (token) {
 		case Opt_degraded:
 			printk(KERN_INFO "btrfs: allowing degraded mounts\n");
@@ -1279,7 +1321,7 @@ static int btrfs_remount(struct super_block *sb, int
*flags, char *data)
 
 	btrfs_remount_prepare(fs_info);
 
-	ret = btrfs_parse_options(root, data);
+	ret = btrfs_parse_options(root, data, 0, NULL);
 	if (ret) {
 		ret = -EINVAL;
 		goto restore;
-- 
1.7.9.5

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

On 8/6/13 1:27 PM, Filipe David Borba Manana wrote:
> This change allows for most mount options to be persisted in
> the filesystem, and be applied when the filesystem is mounted.
> If the same options are specified at mount time, the persisted
> values for those options are ignored.
> 
> The only options not supported are: subvol, subvolid, subvolrootid,
> device and thread_pool. This limitation is due to how this feature
> is implemented: basically there''s an optional value (of type
> struct btrfs_dir_item) in the tree of tree roots used to store the
> list of options in the same format as they are passed to btrfs_mount().
> This means any mount option that takes effect before the tree of tree
> roots is setup is not supported.
> 
> To set these options, the user space tool btrfstune was modified
> to persist the list of options into an unmounted filesystem''s
> tree of tree roots.
So, it does this thing, ok - but why?
What is seen as the administrative advantage of this new mechanism?

Just to play devil''s advocate, and to add a bit of history:

On any production system, the filesystems will be mounted via fstab,
which has the advantages of being widely known, well understood, and
100% expected - as well as being transparent, unsurprising, and seamless.

For history: ext4 did this too.  And now it''s in a situation where
it''s
got mount options coming at it from both the superblock and from
the commandline (or fstab), and sometimes they conflict; it also tries
to report mount options in /proc/mounts, but has grown hairy code
to decide which ones to print and which ones to not print (if it''s
a "default" option, don''t print it in /proc/mounts, but
what''s default,
code-default or fs-default?)  And it''s really kind of an ugly mess.

Further, mounting 2 filesystems w/ no options in fstab or on the
commandline, and getting different behavior due to hidden (sorry,
persistent) options in the fs itself is surprising, and surprise
is rarely good.

So this patch adds 100+ lines of new code, to implement this idea, but:
what is the advantage?  Unless there is a compelling administrative
use case, I''d vote against it.  Lines of code that don''t exist
don''t
have bugs.  ;)

-Eric
> NOTE: Like the corresponding btrfs-progs patch, this is a WIP with
> the goal o gathering feedback.
> 
> Signed-off-by: Filipe David Borba Manana <fdmanana@gmail.com>
> ---
>  fs/btrfs/ctree.h   |   11 +++++++-
>  fs/btrfs/disk-io.c |   72
+++++++++++++++++++++++++++++++++++++++++++++++++++-
>  fs/btrfs/super.c   |   46 +++++++++++++++++++++++++++++++--
>  3 files changed, 125 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
> index cbb1263..24363df 100644
> --- a/fs/btrfs/ctree.h
> +++ b/fs/btrfs/ctree.h
> @@ -121,6 +121,14 @@ struct btrfs_ordered_sum;
>   */
>  #define BTRFS_FREE_INO_OBJECTID -12ULL
>  
> +/*
> + * Item that stores permanent mount options. These options
> + * have effect if they are not specified as well at mount
> + * time (that is, if a permanent option is also specified at
> + * mount time, the later wins).
> + */
> +#define BTRFS_PERSISTENT_OPTIONS_OBJECTID -13ULL
> +
>  /* dummy objectid represents multiple objectids */
>  #define BTRFS_MULTIPLE_OBJECTIDS -255ULL
>  
> @@ -3739,7 +3747,8 @@ void btrfs_exit_sysfs(void);
>  ssize_t btrfs_listxattr(struct dentry *dentry, char *buffer, size_t size);
>  
>  /* super.c */
> -int btrfs_parse_options(struct btrfs_root *root, char *options);
> +int btrfs_parse_options(struct btrfs_root *root, char *options,
> +			int parsing_persistent, int **options_parsed);
>  int btrfs_sync_fs(struct super_block *sb, int wait);
>  
>  #ifdef CONFIG_PRINTK
> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
> index 254cdc8..eeabdd4 100644
> --- a/fs/btrfs/disk-io.c
> +++ b/fs/btrfs/disk-io.c
> @@ -2077,6 +2077,53 @@ static void del_fs_roots(struct btrfs_fs_info
*fs_info)
>  	}
>  }
>  
> +static char *get_persistent_options(struct btrfs_root *tree_root)
> +{
> +	int ret;
> +	struct btrfs_key key;
> +	struct btrfs_path *path;
> +	struct extent_buffer *leaf;
> +	struct btrfs_dir_item *di;
> +	u32 name_len, data_len;
> +	char *options = NULL;
> +
> +	path = btrfs_alloc_path();
> +	if (!path)
> +		return ERR_PTR(-ENOMEM);
> +
> +	key.objectid = BTRFS_PERSISTENT_OPTIONS_OBJECTID;
> +	key.type = 0;
> +	key.offset = 0;
> +
> +	ret = btrfs_search_slot(NULL, tree_root, &key, path, 0, 0);
> +	if (ret < 0)
> +		goto out;
> +	if (ret > 0) {
> +		ret = 0;
> +		goto out;
> +	}
> +
> +	leaf = path->nodes[0];
> +	di = btrfs_item_ptr(leaf, path->slots[0], struct btrfs_dir_item);
> +	name_len = btrfs_dir_name_len(leaf, di);
> +	data_len = btrfs_dir_data_len(leaf, di);
> +	options = kmalloc(data_len + 1, GFP_NOFS);
> +	if (!options) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +	read_extent_buffer(leaf, options,
> +			   (unsigned long)((char *)(di + 1) + name_len),
> +			   data_len);
> +	options[data_len] = ''\0'';
> +
> +out:
> +	btrfs_free_path(path);
> +	if (ret)
> +		return ERR_PTR(ret);
> +	return options;
> +}
> +
>  int open_ctree(struct super_block *sb,
>  	       struct btrfs_fs_devices *fs_devices,
>  	       char *options)
> @@ -2103,6 +2150,8 @@ int open_ctree(struct super_block *sb,
>  	int err = -EINVAL;
>  	int num_backups_tried = 0;
>  	int backup_index = 0;
> +	int *mnt_options = NULL;
> +	char *persist_options = NULL;
>  
>  	tree_root = fs_info->tree_root = btrfs_alloc_root(fs_info);
>  	chunk_root = fs_info->chunk_root = btrfs_alloc_root(fs_info);
> @@ -2372,7 +2421,7 @@ int open_ctree(struct super_block *sb,
>  	 */
>  	fs_info->compress_type = BTRFS_COMPRESS_ZLIB;
>  
> -	ret = btrfs_parse_options(tree_root, options);
> +	ret = btrfs_parse_options(tree_root, options, 0, &mnt_options);
>  	if (ret) {
>  		err = ret;
>  		goto fail_alloc;
> @@ -2656,6 +2705,26 @@ retry_root_backup:
>  	btrfs_set_root_node(&tree_root->root_item, tree_root->node);
>  	tree_root->commit_root = btrfs_root_node(tree_root);
>  
> +	persist_options = get_persistent_options(tree_root);
> +	if (IS_ERR(persist_options)) {
> +		ret = PTR_ERR(persist_options);
> +		goto fail_tree_roots;
> +	} else if (persist_options) {
> +		ret = btrfs_parse_options(tree_root, persist_options,
> +					  1, &mnt_options);
> +		kfree(mnt_options);
> +		mnt_options = NULL;
> +		if (ret) {
> +			err = ret;
> +			goto fail_tree_roots;
> +		}
> +		if (tree_root->fs_info->compress_type == BTRFS_COMPRESS_LZO) {
> +			features = btrfs_super_incompat_flags(disk_super);
> +			features |= BTRFS_FEATURE_INCOMPAT_COMPRESS_LZO;
> +			btrfs_set_super_incompat_flags(disk_super, features);
> +		}
> +	}
> +
>  	location.objectid = BTRFS_EXTENT_TREE_OBJECTID;
>  	location.type = BTRFS_ROOT_ITEM_KEY;
>  	location.offset = 0;
> @@ -2904,6 +2973,7 @@ fail_block_groups:
>  	btrfs_free_block_groups(fs_info);
>  
>  fail_tree_roots:
> +	kfree(mnt_options);
>  	free_root_pointers(fs_info, 1);
>  	invalidate_inode_pages2(fs_info->btree_inode->i_mapping);
>  
> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
> index 2cc5b80..ced0a85 100644
> --- a/fs/btrfs/super.c
> +++ b/fs/btrfs/super.c
> @@ -369,7 +369,8 @@ static match_table_t tokens = {
>   * reading in a new superblock is parsed here.
>   * XXX JDM: This needs to be cleaned up for remount.
>   */
> -int btrfs_parse_options(struct btrfs_root *root, char *options)
> +int btrfs_parse_options(struct btrfs_root *root, char *options,
> +			int parsing_persistent, int **options_parsed)
>  {
>  	struct btrfs_fs_info *info = root->fs_info;
>  	substring_t args[MAX_OPT_ARGS];
> @@ -379,11 +380,21 @@ int btrfs_parse_options(struct btrfs_root *root, char
*options)
>  	int ret = 0;
>  	char *compress_type;
>  	bool compress_force = false;
> +	int *parsed = NULL;
>  
>  	cache_gen =
btrfs_super_cache_generation(root->fs_info->super_copy);
>  	if (cache_gen)
>  		btrfs_set_opt(info->mount_opt, SPACE_CACHE);
>  
> +	if (!parsing_persistent && options_parsed) {
> +		parsed = kzalloc(sizeof(int) * ARRAY_SIZE(tokens), GFP_NOFS);
> +		if (!parsed)
> +			return -ENOMEM;
> +		*options_parsed = parsed;
> +	} else if (parsing_persistent && options_parsed) {
> +		parsed = *options_parsed;
> +	}
> +
>  	if (!options)
>  		goto out;
>  
> @@ -403,6 +414,37 @@ int btrfs_parse_options(struct btrfs_root *root, char
*options)
>  			continue;
>  
>  		token = match_token(p, tokens, args);
> +
> +		if (parsing_persistent && parsed) {
> +			/*
> +			 * A persistent option value is ignored if a value for
> +			 * that option was given at mount time.
> +			 */
> +
> +			if (parsed[token])
> +				continue;
> +			if (token == Opt_no_space_cache &&
> +			    parsed[Opt_space_cache])
> +				continue;
> +			if (token == Opt_space_cache &&
> +			    parsed[Opt_no_space_cache])
> +				continue;
> +
> +			if (token == Opt_subvol)
> +				printk(KERN_WARNING "btrfs: subvol not supported as a persistent
option");
> +			else if (token == Opt_subvolid)
> +				printk(KERN_WARNING "btrfs: subvolid not supported as a
persistent option");
> +			else if (token == Opt_subvolrootid)
> +				printk(KERN_WARNING "btrfs: subvolrootid not supported as a
persistent option");
> +			else if (token == Opt_device)
> +				printk(KERN_WARNING "btrfs: device not supported as a persistent
option");
> +			else if (token == Opt_thread_pool)
> +				printk(KERN_WARNING "btrfs: thread_pool not supported as a
persistent option");
> +		}
> +
> +		if (!parsing_persistent && parsed)
> +			parsed[token] = 1;
> +
>  		switch (token) {
>  		case Opt_degraded:
>  			printk(KERN_INFO "btrfs: allowing degraded mounts\n");
> @@ -1279,7 +1321,7 @@ static int btrfs_remount(struct super_block *sb, int
*flags, char *data)
>  
>  	btrfs_remount_prepare(fs_info);
>  
> -	ret = btrfs_parse_options(root, data);
> +	ret = btrfs_parse_options(root, data, 0, NULL);
>  	if (ret) {
>  		ret = -EINVAL;
>  		goto restore;
> 
--
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

On Tue, Aug 6, 2013 at 9:37 PM, Eric Sandeen <sandeen@redhat.com>
wrote:
> On 8/6/13 1:27 PM, Filipe David Borba Manana wrote:
>> This change allows for most mount options to be persisted in
>> the filesystem, and be applied when the filesystem is mounted.
>> If the same options are specified at mount time, the persisted
>> values for those options are ignored.
>>
>> The only options not supported are: subvol, subvolid, subvolrootid,
>> device and thread_pool. This limitation is due to how this feature
>> is implemented: basically there''s an optional value (of type
>> struct btrfs_dir_item) in the tree of tree roots used to store the
>> list of options in the same format as they are passed to btrfs_mount().
>> This means any mount option that takes effect before the tree of tree
>> roots is setup is not supported.
>>
>> To set these options, the user space tool btrfstune was modified
>> to persist the list of options into an unmounted filesystem''s
>> tree of tree roots.
>
> So, it does this thing, ok - but why?
> What is seen as the administrative advantage of this new mechanism?
>
> Just to play devil''s advocate, and to add a bit of history:
>
> On any production system, the filesystems will be mounted via fstab,
> which has the advantages of being widely known, well understood, and
> 100% expected - as well as being transparent, unsurprising, and seamless.
>
> For history: ext4 did this too.  And now it''s in a situation where
it''s
> got mount options coming at it from both the superblock and from
> the commandline (or fstab), and sometimes they conflict; it also tries
> to report mount options in /proc/mounts, but has grown hairy code
> to decide which ones to print and which ones to not print (if it''s
> a "default" option, don''t print it in /proc/mounts, but
what''s default,
> code-default or fs-default?)  And it''s really kind of an ugly
mess.
>
> Further, mounting 2 filesystems w/ no options in fstab or on the
> commandline, and getting different behavior due to hidden (sorry,
> persistent) options in the fs itself is surprising, and surprise
> is rarely good.
>
> So this patch adds 100+ lines of new code, to implement this idea, but:
> what is the advantage?  Unless there is a compelling administrative
> use case, I''d vote against it.  Lines of code that don''t
exist don''t
> have bugs.  ;)
There was a recent good example (imho at least) mentioned by Xavier
Gnata some time ago:

http://comments.gmane.org/gmane.comp.file-systems.btrfs/26011

cheers

>
> -Eric
>
>> NOTE: Like the corresponding btrfs-progs patch, this is a WIP with
>> the goal o gathering feedback.
>>
>> Signed-off-by: Filipe David Borba Manana <fdmanana@gmail.com>
>> ---
>>  fs/btrfs/ctree.h   |   11 +++++++-
>>  fs/btrfs/disk-io.c |   72
+++++++++++++++++++++++++++++++++++++++++++++++++++-
>>  fs/btrfs/super.c   |   46 +++++++++++++++++++++++++++++++--
>>  3 files changed, 125 insertions(+), 4 deletions(-)
>>
>> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h
>> index cbb1263..24363df 100644
>> --- a/fs/btrfs/ctree.h
>> +++ b/fs/btrfs/ctree.h
>> @@ -121,6 +121,14 @@ struct btrfs_ordered_sum;
>>   */
>>  #define BTRFS_FREE_INO_OBJECTID -12ULL
>>
>> +/*
>> + * Item that stores permanent mount options. These options
>> + * have effect if they are not specified as well at mount
>> + * time (that is, if a permanent option is also specified at
>> + * mount time, the later wins).
>> + */
>> +#define BTRFS_PERSISTENT_OPTIONS_OBJECTID -13ULL
>> +
>>  /* dummy objectid represents multiple objectids */
>>  #define BTRFS_MULTIPLE_OBJECTIDS -255ULL
>>
>> @@ -3739,7 +3747,8 @@ void btrfs_exit_sysfs(void);
>>  ssize_t btrfs_listxattr(struct dentry *dentry, char *buffer, size_t
size);
>>
>>  /* super.c */
>> -int btrfs_parse_options(struct btrfs_root *root, char *options);
>> +int btrfs_parse_options(struct btrfs_root *root, char *options,
>> +                     int parsing_persistent, int **options_parsed);
>>  int btrfs_sync_fs(struct super_block *sb, int wait);
>>
>>  #ifdef CONFIG_PRINTK
>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c
>> index 254cdc8..eeabdd4 100644
>> --- a/fs/btrfs/disk-io.c
>> +++ b/fs/btrfs/disk-io.c
>> @@ -2077,6 +2077,53 @@ static void del_fs_roots(struct btrfs_fs_info
*fs_info)
>>       }
>>  }
>>
>> +static char *get_persistent_options(struct btrfs_root *tree_root)
>> +{
>> +     int ret;
>> +     struct btrfs_key key;
>> +     struct btrfs_path *path;
>> +     struct extent_buffer *leaf;
>> +     struct btrfs_dir_item *di;
>> +     u32 name_len, data_len;
>> +     char *options = NULL;
>> +
>> +     path = btrfs_alloc_path();
>> +     if (!path)
>> +             return ERR_PTR(-ENOMEM);
>> +
>> +     key.objectid = BTRFS_PERSISTENT_OPTIONS_OBJECTID;
>> +     key.type = 0;
>> +     key.offset = 0;
>> +
>> +     ret = btrfs_search_slot(NULL, tree_root, &key, path, 0, 0);
>> +     if (ret < 0)
>> +             goto out;
>> +     if (ret > 0) {
>> +             ret = 0;
>> +             goto out;
>> +     }
>> +
>> +     leaf = path->nodes[0];
>> +     di = btrfs_item_ptr(leaf, path->slots[0], struct
btrfs_dir_item);
>> +     name_len = btrfs_dir_name_len(leaf, di);
>> +     data_len = btrfs_dir_data_len(leaf, di);
>> +     options = kmalloc(data_len + 1, GFP_NOFS);
>> +     if (!options) {
>> +             ret = -ENOMEM;
>> +             goto out;
>> +     }
>> +     read_extent_buffer(leaf, options,
>> +                        (unsigned long)((char *)(di + 1) + name_len),
>> +                        data_len);
>> +     options[data_len] = ''\0'';
>> +
>> +out:
>> +     btrfs_free_path(path);
>> +     if (ret)
>> +             return ERR_PTR(ret);
>> +     return options;
>> +}
>> +
>>  int open_ctree(struct super_block *sb,
>>              struct btrfs_fs_devices *fs_devices,
>>              char *options)
>> @@ -2103,6 +2150,8 @@ int open_ctree(struct super_block *sb,
>>       int err = -EINVAL;
>>       int num_backups_tried = 0;
>>       int backup_index = 0;
>> +     int *mnt_options = NULL;
>> +     char *persist_options = NULL;
>>
>>       tree_root = fs_info->tree_root = btrfs_alloc_root(fs_info);
>>       chunk_root = fs_info->chunk_root = btrfs_alloc_root(fs_info);
>> @@ -2372,7 +2421,7 @@ int open_ctree(struct super_block *sb,
>>        */
>>       fs_info->compress_type = BTRFS_COMPRESS_ZLIB;
>>
>> -     ret = btrfs_parse_options(tree_root, options);
>> +     ret = btrfs_parse_options(tree_root, options, 0,
&mnt_options);
>>       if (ret) {
>>               err = ret;
>>               goto fail_alloc;
>> @@ -2656,6 +2705,26 @@ retry_root_backup:
>>       btrfs_set_root_node(&tree_root->root_item,
tree_root->node);
>>       tree_root->commit_root = btrfs_root_node(tree_root);
>>
>> +     persist_options = get_persistent_options(tree_root);
>> +     if (IS_ERR(persist_options)) {
>> +             ret = PTR_ERR(persist_options);
>> +             goto fail_tree_roots;
>> +     } else if (persist_options) {
>> +             ret = btrfs_parse_options(tree_root, persist_options,
>> +                                       1, &mnt_options);
>> +             kfree(mnt_options);
>> +             mnt_options = NULL;
>> +             if (ret) {
>> +                     err = ret;
>> +                     goto fail_tree_roots;
>> +             }
>> +             if (tree_root->fs_info->compress_type ==
BTRFS_COMPRESS_LZO) {
>> +                     features =
btrfs_super_incompat_flags(disk_super);
>> +                     features |= BTRFS_FEATURE_INCOMPAT_COMPRESS_LZO;
>> +                     btrfs_set_super_incompat_flags(disk_super,
features);
>> +             }
>> +     }
>> +
>>       location.objectid = BTRFS_EXTENT_TREE_OBJECTID;
>>       location.type = BTRFS_ROOT_ITEM_KEY;
>>       location.offset = 0;
>> @@ -2904,6 +2973,7 @@ fail_block_groups:
>>       btrfs_free_block_groups(fs_info);
>>
>>  fail_tree_roots:
>> +     kfree(mnt_options);
>>       free_root_pointers(fs_info, 1);
>>       invalidate_inode_pages2(fs_info->btree_inode->i_mapping);
>>
>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c
>> index 2cc5b80..ced0a85 100644
>> --- a/fs/btrfs/super.c
>> +++ b/fs/btrfs/super.c
>> @@ -369,7 +369,8 @@ static match_table_t tokens = {
>>   * reading in a new superblock is parsed here.
>>   * XXX JDM: This needs to be cleaned up for remount.
>>   */
>> -int btrfs_parse_options(struct btrfs_root *root, char *options)
>> +int btrfs_parse_options(struct btrfs_root *root, char *options,
>> +                     int parsing_persistent, int **options_parsed)
>>  {
>>       struct btrfs_fs_info *info = root->fs_info;
>>       substring_t args[MAX_OPT_ARGS];
>> @@ -379,11 +380,21 @@ int btrfs_parse_options(struct btrfs_root *root,
char *options)
>>       int ret = 0;
>>       char *compress_type;
>>       bool compress_force = false;
>> +     int *parsed = NULL;
>>
>>       cache_gen =
btrfs_super_cache_generation(root->fs_info->super_copy);
>>       if (cache_gen)
>>               btrfs_set_opt(info->mount_opt, SPACE_CACHE);
>>
>> +     if (!parsing_persistent && options_parsed) {
>> +             parsed = kzalloc(sizeof(int) * ARRAY_SIZE(tokens),
GFP_NOFS);
>> +             if (!parsed)
>> +                     return -ENOMEM;
>> +             *options_parsed = parsed;
>> +     } else if (parsing_persistent && options_parsed) {
>> +             parsed = *options_parsed;
>> +     }
>> +
>>       if (!options)
>>               goto out;
>>
>> @@ -403,6 +414,37 @@ int btrfs_parse_options(struct btrfs_root *root,
char *options)
>>                       continue;
>>
>>               token = match_token(p, tokens, args);
>> +
>> +             if (parsing_persistent && parsed) {
>> +                     /*
>> +                      * A persistent option value is ignored if a
value for
>> +                      * that option was given at mount time.
>> +                      */
>> +
>> +                     if (parsed[token])
>> +                             continue;
>> +                     if (token == Opt_no_space_cache &&
>> +                         parsed[Opt_space_cache])
>> +                             continue;
>> +                     if (token == Opt_space_cache &&
>> +                         parsed[Opt_no_space_cache])
>> +                             continue;
>> +
>> +                     if (token == Opt_subvol)
>> +                             printk(KERN_WARNING "btrfs: subvol
not supported as a persistent option");
>> +                     else if (token == Opt_subvolid)
>> +                             printk(KERN_WARNING "btrfs: subvolid
not supported as a persistent option");
>> +                     else if (token == Opt_subvolrootid)
>> +                             printk(KERN_WARNING "btrfs:
subvolrootid not supported as a persistent option");
>> +                     else if (token == Opt_device)
>> +                             printk(KERN_WARNING "btrfs: device
not supported as a persistent option");
>> +                     else if (token == Opt_thread_pool)
>> +                             printk(KERN_WARNING "btrfs:
thread_pool not supported as a persistent option");
>> +             }
>> +
>> +             if (!parsing_persistent && parsed)
>> +                     parsed[token] = 1;
>> +
>>               switch (token) {
>>               case Opt_degraded:
>>                       printk(KERN_INFO "btrfs: allowing degraded
mounts\n");
>> @@ -1279,7 +1321,7 @@ static int btrfs_remount(struct super_block *sb,
int *flags, char *data)
>>
>>       btrfs_remount_prepare(fs_info);
>>
>> -     ret = btrfs_parse_options(root, data);
>> +     ret = btrfs_parse_options(root, data, 0, NULL);
>>       if (ret) {
>>               ret = -EINVAL;
>>               goto restore;
>>
>


-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That''s why all progress depends on unreasonable men."
--
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

On 8/6/13 3:45 PM, Filipe David Manana wrote:
> On Tue, Aug 6, 2013 at 9:37 PM, Eric Sandeen <sandeen@redhat.com>
wrote:
>> On 8/6/13 1:27 PM, Filipe David Borba Manana wrote:
>>> This change allows for most mount options to be persisted in
>>> the filesystem, and be applied when the filesystem is mounted.
>>> If the same options are specified at mount time, the persisted
>>> values for those options are ignored.
>>>
>>> The only options not supported are: subvol, subvolid, subvolrootid,
>>> device and thread_pool. This limitation is due to how this feature
>>> is implemented: basically there''s an optional value (of
type
>>> struct btrfs_dir_item) in the tree of tree roots used to store the
>>> list of options in the same format as they are passed to
btrfs_mount().
>>> This means any mount option that takes effect before the tree of
tree
>>> roots is setup is not supported.
>>>
>>> To set these options, the user space tool btrfstune was modified
>>> to persist the list of options into an unmounted
filesystem''s
>>> tree of tree roots.
>>
>> So, it does this thing, ok - but why?
>> What is seen as the administrative advantage of this new mechanism?
>>
>> Just to play devil''s advocate, and to add a bit of history:
>>
>> On any production system, the filesystems will be mounted via fstab,
>> which has the advantages of being widely known, well understood, and
>> 100% expected - as well as being transparent, unsurprising, and
seamless.
>>
>> For history: ext4 did this too.  And now it''s in a situation
where it''s
>> got mount options coming at it from both the superblock and from
>> the commandline (or fstab), and sometimes they conflict; it also tries
>> to report mount options in /proc/mounts, but has grown hairy code
>> to decide which ones to print and which ones to not print (if
it''s
>> a "default" option, don''t print it in /proc/mounts,
but what''s default,
>> code-default or fs-default?)  And it''s really kind of an ugly
mess.
>>
>> Further, mounting 2 filesystems w/ no options in fstab or on the
>> commandline, and getting different behavior due to hidden (sorry,
>> persistent) options in the fs itself is surprising, and surprise
>> is rarely good.
>>
>> So this patch adds 100+ lines of new code, to implement this idea, but:
>> what is the advantage?  Unless there is a compelling administrative
>> use case, I''d vote against it.  Lines of code that
don''t exist don''t
>> have bugs.  ;)
> 
> There was a recent good example (imho at least) mentioned by Xavier
> Gnata some time ago:
> 
> http://comments.gmane.org/gmane.comp.file-systems.btrfs/26011
> 
> cheers
Hm, I see.  I forgot about hotplugging in my "most systems mount
via fstab" assertion.  :)

I was thinking (and Josef just suggested too) that making a
dir flag, saying "everything under this dir gets compressed" might
make
more sense for that scenario than adding a whole slew of
on-disk-persistent-mount-option code.

Because really, the motivation sounds like it''s primarily for
significant
on-disk format changes controlled by mount options.  I understand that
motivation more than being able to persist something like "noatime."

-Eric

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

Eric Sandeen posted on Tue, 06 Aug 2013 15:37:30 -0500 as excerpted:
> On 8/6/13 1:27 PM, Filipe David Borba Manana wrote:
>> This change allows for most mount options to be persisted in the
>> filesystem, and be applied when the filesystem is mounted.
>> If the same options are specified at mount time, the persisted values
>> for those options are ignored.
>> 
>> The only options not supported are: subvol, subvolid, subvolrootid,
>> device and thread_pool. This limitation is due to how this feature is
>> implemented: basically there''s an optional value (of type
struct
>> btrfs_dir_item) in the tree of tree roots used to store the list of
>> options in the same format as they are passed to btrfs_mount().
>> This means any mount option that takes effect before the tree of tree
>> roots is setup is not supported.
>> 
>> To set these options, the user space tool btrfstune was modified to
>> persist the list of options into an unmounted filesystem''s
tree of tree
>> roots.
> 
> So, it does this thing, ok - but why?
> What is seen as the administrative advantage of this new mechanism?
> 
> Just to play devil''s advocate, and to add a bit of history:
> 
> On any production system, the filesystems will be mounted via fstab,
> which has the advantages of being widely known, well understood, and
> 100% expected - as well as being transparent, unsurprising, and
> seamless.
> 
> For history: ext4 did this too.  And now it''s in a situation where
it''s
> got mount options coming at it from both the superblock and from the
> commandline (or fstab), and sometimes they conflict; it also tries to
> report mount options in /proc/mounts, but has grown hairy code to decide
> which ones to print and which ones to not print (if it''s a
"default"
> option, don''t print it in /proc/mounts, but what''s
default, code-default
> or fs-default?)  And it''s really kind of an ugly mess.
> 
> Further, mounting 2 filesystems w/ no options in fstab or on the
> commandline, and getting different behavior due to hidden (sorry,
> persistent) options in the fs itself is surprising, and surprise is
> rarely good.
> 
> So this patch adds 100+ lines of new code, to implement this idea, but:
> what is the advantage?  Unless there is a compelling administrative use
> case, I''d vote against it.  Lines of code that don''t
exist don''t have
> bugs.  ;)
As an admin, there''s some options I want always applied as site policy.
Here, that includes compress=lzo, autodefrag and noatime.  And with all 
the capacities btrfs has what with raid and the like, particularly if 
someone''s needing to use device= (which won''t go in the
persistent
options for what should be pretty obvious reasons) a bunch of times, that 
fstab line can get quite long indeed![1]

Just like the kernel has a config option for a built-in commandline that 
takes some of the pressure off the actually passed commandline for 
options that are pretty much always going to be used so it''s easier to 
administer because only the possibly dynamic options or those going 
against the builtin commandline defaults need passed, a filesystem as 
complex and multi-optioned as btrfs is, really NEEDS some way to persist 
the options that are effectively site policy default, so the admin 
doesn''t have to worry about them any longer.

FWIW, I had assumed persistent mount options were planned as a given and 
simply hadn''t been implemented yet.  Because to me it''s a
no-brainer.
After all, people don''t have to use the feature if they don''t
like it.
And it sure saves a headache when what might otherwise be a dozen 
parameters passed in can be cut in half or better, leaving only the ones 
that are going to differ depending on circumstances to worry about.  I 
know that from experience with the kernel builtin commandline option!

---
[1] I ran initr*less root-on-md-raid for many years.  That involved 
passing a complicated set of md1=/dev/sda3,/dev/sdb3,/dev/sdc3,/dev/sdd3 
type options to the kernel, along with more usual options I was passing, 
and I was SO happy when the kernel got that built-in-commandline option 
and I could put the default set in there, such that I only had to worry 
about passing that parameter for the backup boot option, thus along with 
several other passed options I was able to put in the builtin, shrinking 
the actual passed kernel commandline dramatically, so only the stuff that 
wasn''t the default needed passed for a particular boot option.  It
would
sure be nice to be able to do the same thing, but at the filesystem 
level, here!

-- 
Duncan - List replies preferred.   No HTML msgs.
"Every nonfree program has a lord, a master --
and if you use the program, he is your master."  Richard Stallman

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

On 8/6/13 8:20 PM, Duncan wrote:
> Eric Sandeen posted on Tue, 06 Aug 2013 15:37:30 -0500 as excerpted:
> 
>> On 8/6/13 1:27 PM, Filipe David Borba Manana wrote:
>>> This change allows for most mount options to be persisted in the
>>> filesystem, and be applied when the filesystem is mounted.
>>> If the same options are specified at mount time, the persisted
values
>>> for those options are ignored.
>>>
>>> The only options not supported are: subvol, subvolid, subvolrootid,
>>> device and thread_pool. This limitation is due to how this feature
is
>>> implemented: basically there''s an optional value (of type
struct
>>> btrfs_dir_item) in the tree of tree roots used to store the list of
>>> options in the same format as they are passed to btrfs_mount().
>>> This means any mount option that takes effect before the tree of
tree
>>> roots is setup is not supported.
>>>
>>> To set these options, the user space tool btrfstune was modified to
>>> persist the list of options into an unmounted filesystem''s
tree of tree
>>> roots.
>>
>> So, it does this thing, ok - but why?
>> What is seen as the administrative advantage of this new mechanism?
>>
>> Just to play devil''s advocate, and to add a bit of history:
>>
>> On any production system, the filesystems will be mounted via fstab,
>> which has the advantages of being widely known, well understood, and
>> 100% expected - as well as being transparent, unsurprising, and
>> seamless.
>>
>> For history: ext4 did this too.  And now it''s in a situation
where it''s
>> got mount options coming at it from both the superblock and from the
>> commandline (or fstab), and sometimes they conflict; it also tries to
>> report mount options in /proc/mounts, but has grown hairy code to
decide
>> which ones to print and which ones to not print (if it''s a
"default"
>> option, don''t print it in /proc/mounts, but what''s
default, code-default
>> or fs-default?)  And it''s really kind of an ugly mess.
>>
>> Further, mounting 2 filesystems w/ no options in fstab or on the
>> commandline, and getting different behavior due to hidden (sorry,
>> persistent) options in the fs itself is surprising, and surprise is
>> rarely good.
>>
>> So this patch adds 100+ lines of new code, to implement this idea, but:
>> what is the advantage?  Unless there is a compelling administrative use
>> case, I''d vote against it.  Lines of code that don''t
exist don''t have
>> bugs.  ;)
> 
> As an admin, there''s some options I want always applied as site
policy.
> Here, that includes compress=lzo, autodefrag and noatime.  And with all 
But as an admin, you can add that to fstab, right?
> the capacities btrfs has what with raid and the like, particularly if 
> someone''s needing to use device= (which won''t go in the
persistent
> options for what should be pretty obvious reasons) a bunch of times, that 
> fstab line can get quite long indeed![1]
> 
> Just like the kernel has a config option for a built-in commandline that 
> takes some of the pressure off the actually passed commandline for 
> options that are pretty much always going to be used so it''s
easier to
> administer because only the possibly dynamic options or those going 
> against the builtin commandline defaults need passed, a filesystem as 
> complex and multi-optioned as btrfs is, really NEEDS some way to persist 
> the options that are effectively site policy default, so the admin 
> doesn''t have to worry about them any longer.
Again, fstab is perfectly sufficient, simply for site policy.

It seems that your main argument here (no pun intended) is that the sheer
length of the options string becomes unwieldy.  i.e. "it''s too
long
for fstab, so it must be moved into the filesystem."

"too long" is a bit subjective, unless util-linux
has an actual string limit.  If not, I guess it''s mostly aesthetics.
> FWIW, I had assumed persistent mount options were planned as a given and 
> simply hadn''t been implemented yet.  Because to me it''s a
no-brainer.
> After all, people don''t have to use the feature if they
don''t like it.
no, but we still have to maintain it ;)

So just speaking for myself, 100+ new lines of code forever after, vs.
"I find my fstab to be unattractive" is an obvious choice.
> And it sure saves a headache when what might otherwise be a dozen 
> parameters passed in can be cut in half or better, leaving only the ones 
> that are going to differ depending on circumstances to worry about.  I 
> know that from experience with the kernel builtin commandline option!
But you still have to set them all.  Is it less headache to use btrfstune
vs edit fstab?  I''m not really convinced.

I guess the argument that it''s easier to notice specific differences
against
a site-wide (hidden) set of options, vs. a bunch of long option strings
resonates somewhat.

*shrug* well, I did my ghost-of-christmas-future bit; it''s just my
$0.02.

Thanks,
-Eric
> ---
> [1] I ran initr*less root-on-md-raid for many years.
isn''t that kind of doing it wrong, though?  init*fs solves the problem
you complain about.  (There''s probably a reason for it that
I''m unaware
of, though.)
>  That involved 
> passing a complicated set of md1=/dev/sda3,/dev/sdb3,/dev/sdc3,/dev/sdd3 
> type options to the kernel, along with more usual options I was passing, 
> and I was SO happy when the kernel got that built-in-commandline option 
> and I could put the default set in there, such that I only had to worry 
> about passing that parameter for the backup boot option, thus along with 
> several other passed options I was able to put in the builtin, shrinking 
> the actual passed kernel commandline dramatically, so only the stuff that 
> wasn''t the default needed passed for a particular boot option.  It
would
> sure be nice to be able to do the same thing, but at the filesystem 
> level, here!
> 
--
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

On 8/6/13 1:27 PM, Filipe David Borba Manana wrote:
> This change allows for most mount options to be persisted in
> the filesystem, and be applied when the filesystem is mounted.
> If the same options are specified at mount time, the persisted
> values for those options are ignored.
I thought the plan was to make commandline mount options
override persistent ones, but that doesn''t seem to be the case,
at least in this example:

# ./btrfstune -o compress,discard,ssd /dev/sdb1
New persistent options:      compress,discard,ssd
# mount -o nossd /dev/sdb1 /mnt/test
# dmesg | tail
[  995.657233] btrfs: use ssd allocation scheme
[  995.661501] btrfs: disk space caching is enabled

and /proc/mounts is similarly confused, showing both options:

# grep sdb1 /proc/mounts
/dev/sdb1 /mnt/test btrfs
rw,seclabel,relatime,compress=zlib,nossd,ssd,discard,space_cache 0 0

(This is the trail of woe I was talking about from ext4-land) ;)
> The only options not supported are: subvol, subvolid, subvolrootid,
> device and thread_pool. This limitation is due to how this feature
> is implemented: basically there''s an optional value (of type
> struct btrfs_dir_item) in the tree of tree roots used to store the
> list of options in the same format as they are passed to btrfs_mount().
> This means any mount option that takes effect before the tree of tree
> roots is setup is not supported.
Just FWIW, vfs-level mount options are also not supported; i.e.
"noatime"
or "ro" won''t work either, because the code is already past
the VFS
option parsing:

# ./btrfstune -o compress,discard,ro /dev/sdb1
Current persistent options:  compress,discard
New persistent options:      compress,discard,ro
# mount /dev/sdb1 /mnt/test
mount: wrong fs type, bad option, bad superblock on /dev/sdb1, ...
# dmesg | tail
[  817.681417] btrfs: unrecognized mount option ''ro''
[  817.694689] btrfs: open_ctree failed
> To set these options, the user space tool btrfstune was modified
> to persist the list of options into an unmounted filesystem''s
> tree of tree roots.
If this goes forward, you''d want an easy way to display
them, not just set them, I suppose.

Thanks,
-Eric
 
> NOTE: Like the corresponding btrfs-progs patch, this is a WIP with
> the goal o gathering feedback.
> 
> Signed-off-by: Filipe David Borba Manana <fdmanana@gmail.com>

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

On Tue, Aug 6, 2013 at 10:05 PM, Eric Sandeen <sandeen@redhat.com>
wrote:
> On 8/6/13 3:45 PM, Filipe David Manana wrote:
>> On Tue, Aug 6, 2013 at 9:37 PM, Eric Sandeen <sandeen@redhat.com>
wrote:
>>> On 8/6/13 1:27 PM, Filipe David Borba Manana wrote:
>>>> This change allows for most mount options to be persisted in
>>>> the filesystem, and be applied when the filesystem is mounted.
>>>> If the same options are specified at mount time, the persisted
>>>> values for those options are ignored.
>>>>
>>>> The only options not supported are: subvol, subvolid,
subvolrootid,
>>>> device and thread_pool. This limitation is due to how this
feature
>>>> is implemented: basically there''s an optional value
(of type
>>>> struct btrfs_dir_item) in the tree of tree roots used to store
the
>>>> list of options in the same format as they are passed to
btrfs_mount().
>>>> This means any mount option that takes effect before the tree
of tree
>>>> roots is setup is not supported.
>>>>
>>>> To set these options, the user space tool btrfstune was
modified
>>>> to persist the list of options into an unmounted
filesystem''s
>>>> tree of tree roots.
>>>
>>> So, it does this thing, ok - but why?
>>> What is seen as the administrative advantage of this new mechanism?
>>>
>>> Just to play devil''s advocate, and to add a bit of
history:
>>>
>>> On any production system, the filesystems will be mounted via
fstab,
>>> which has the advantages of being widely known, well understood,
and
>>> 100% expected - as well as being transparent, unsurprising, and
seamless.
>>>
>>> For history: ext4 did this too.  And now it''s in a
situation where it''s
>>> got mount options coming at it from both the superblock and from
>>> the commandline (or fstab), and sometimes they conflict; it also
tries
>>> to report mount options in /proc/mounts, but has grown hairy code
>>> to decide which ones to print and which ones to not print (if
it''s
>>> a "default" option, don''t print it in
/proc/mounts, but what''s default,
>>> code-default or fs-default?)  And it''s really kind of an
ugly mess.
>>>
>>> Further, mounting 2 filesystems w/ no options in fstab or on the
>>> commandline, and getting different behavior due to hidden (sorry,
>>> persistent) options in the fs itself is surprising, and surprise
>>> is rarely good.
>>>
>>> So this patch adds 100+ lines of new code, to implement this idea,
but:
>>> what is the advantage?  Unless there is a compelling administrative
>>> use case, I''d vote against it.  Lines of code that
don''t exist don''t
>>> have bugs.  ;)
>>
>> There was a recent good example (imho at least) mentioned by Xavier
>> Gnata some time ago:
>>
>> http://comments.gmane.org/gmane.comp.file-systems.btrfs/26011
>>
>> cheers
>
> Hm, I see.  I forgot about hotplugging in my "most systems mount
> via fstab" assertion.  :)
>
> I was thinking (and Josef just suggested too) that making a
> dir flag, saying "everything under this dir gets compressed"
might make
> more sense for that scenario than adding a whole slew of
> on-disk-persistent-mount-option code.
I like that idea too. Thanks for the suggestion. A quick experiment
allowed for that approach in under 20 lines, will test it a bit more.
>
> Because really, the motivation sounds like it''s primarily for
significant
> on-disk format changes controlled by mount options.  I understand that
> motivation more than being able to persist something like
"noatime."
>
> -Eric
>


-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That''s why all progress depends on unreasonable men."
--
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

On Wed, Aug 7, 2013 at 4:04 AM, Eric Sandeen <sandeen@redhat.com>
wrote:
> On 8/6/13 1:27 PM, Filipe David Borba Manana wrote:
>> This change allows for most mount options to be persisted in
>> the filesystem, and be applied when the filesystem is mounted.
>> If the same options are specified at mount time, the persisted
>> values for those options are ignored.
>
> I thought the plan was to make commandline mount options
> override persistent ones, but that doesn''t seem to be the case,
> at least in this example:
Yes, that was the idea. However I didn''t try all possible combinations
of mount and persistent parameters.
>
> # ./btrfstune -o compress,discard,ssd /dev/sdb1
> New persistent options:      compress,discard,ssd
> # mount -o nossd /dev/sdb1 /mnt/test
> # dmesg | tail
> [  995.657233] btrfs: use ssd allocation scheme
> [  995.661501] btrfs: disk space caching is enabled
Yes. Misses some checks like the ones I added for the space_cache /
no_space_cache combinations:

+ if (token == Opt_no_space_cache &&
+    parsed[Opt_space_cache])
+ continue;
+ if (token == Opt_space_cache &&
+    parsed[Opt_no_space_cache])
+ continue;
>
> and /proc/mounts is similarly confused, showing both options:
>
> # grep sdb1 /proc/mounts
> /dev/sdb1 /mnt/test btrfs
rw,seclabel,relatime,compress=zlib,nossd,ssd,discard,space_cache 0 0
>
> (This is the trail of woe I was talking about from ext4-land) ;)
>
>> The only options not supported are: subvol, subvolid, subvolrootid,
>> device and thread_pool. This limitation is due to how this feature
>> is implemented: basically there''s an optional value (of type
>> struct btrfs_dir_item) in the tree of tree roots used to store the
>> list of options in the same format as they are passed to btrfs_mount().
>> This means any mount option that takes effect before the tree of tree
>> roots is setup is not supported.
>
> Just FWIW, vfs-level mount options are also not supported; i.e.
"noatime"
> or "ro" won''t work either, because the code is already
past the VFS
> option parsing:
>
> # ./btrfstune -o compress,discard,ro /dev/sdb1
> Current persistent options:  compress,discard
> New persistent options:      compress,discard,ro
> # mount /dev/sdb1 /mnt/test
> mount: wrong fs type, bad option, bad superblock on /dev/sdb1, ...
> # dmesg | tail
> [  817.681417] btrfs: unrecognized mount option ''ro''
> [  817.694689] btrfs: open_ctree failed
Yes, that would be a next step to work on after getting community
feedback about the approach (from a user point of view and the
technical details / implementation).

Thanks for trying it and for your feedback Eric.
>
>> To set these options, the user space tool btrfstune was modified
>> to persist the list of options into an unmounted filesystem''s
>> tree of tree roots.
>
> If this goes forward, you''d want an easy way to display
> them, not just set them, I suppose.
>
> Thanks,
> -Eric
>
>> NOTE: Like the corresponding btrfs-progs patch, this is a WIP with
>> the goal o gathering feedback.
>>
>> Signed-off-by: Filipe David Borba Manana <fdmanana@gmail.com>
>
>


-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That''s why all progress depends on unreasonable men."
--
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

On Tue, Aug 06, 2013 at 07:27:20PM +0100, Filipe David Borba Manana
wrote:
> This change allows for most mount options to be persisted in
> the filesystem, and be applied when the filesystem is mounted.
> If the same options are specified at mount time, the persisted
> values for those options are ignored.
> 
> The only options not supported are: subvol, subvolid, subvolrootid,
> device and thread_pool. This limitation is due to how this feature
> is implemented: basically there''s an optional value (of type
> struct btrfs_dir_item) in the tree of tree roots used to store the
> list of options in the same format as they are passed to btrfs_mount().
> This means any mount option that takes effect before the tree of tree
> roots is setup is not supported.
> 
> To set these options, the user space tool btrfstune was modified
> to persist the list of options into an unmounted filesystem''s
> tree of tree roots.
We’ve seen this proposed in the past, IIRC this thread contains most of what
has been discussed:
http://thread.gmane.org/gmane.comp.file-systems.btrfs/19757

Implementing just whole-filesystem mount options is not convering all usecases,
more broad approach like per-subvolume or per-file settings is desired. There
are always settings that aren’t known now but would be useful in the future, so
we need a flexible infrastructure to maintain the properties.

There was a proposed implementation for set/get properties:
http://thread.gmane.org/gmane.comp.file-systems.btrfs/18287

From the implementation side, I very much want to abandon the separate
btrfstune utility. Currently it’s a bandaid because there’s nothing better atm.

Designing and merging the properties feature takes time, but we want to tune
simple things now. The wiki project mentions ‘tune2fs’ as an example, but the
project details are not always accurate about how to do the things, it’s more
like ideas what to do. If you’re going to work on that, please claim the
project on the wiki, and possibly write more details abou the design.

david
--
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

On Tue, Aug 06, 2013 at 04:05:50PM -0500, Eric Sandeen
wrote:
> I was thinking (and Josef just suggested too) that making a
> dir flag, saying "everything under this dir gets compressed"
might make
> more sense for that scenario than adding a whole slew of
> on-disk-persistent-mount-option code.
We have this dir flag stored in the attributes, chattr +c dir/, but this
cannot be tuned further - no way to specify the compression algorithm
(or for example the target ratio as compression hint), or we can''t say
"never compress files in this dir (even if mounted with compression)"
etc. 

david
--
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

On Wed, Aug 7, 2013 at 11:40 AM, David Sterba <dsterba@suse.cz>
wrote:
> On Tue, Aug 06, 2013 at 07:27:20PM +0100, Filipe David Borba Manana wrote:
>> This change allows for most mount options to be persisted in
>> the filesystem, and be applied when the filesystem is mounted.
>> If the same options are specified at mount time, the persisted
>> values for those options are ignored.
>>
>> The only options not supported are: subvol, subvolid, subvolrootid,
>> device and thread_pool. This limitation is due to how this feature
>> is implemented: basically there''s an optional value (of type
>> struct btrfs_dir_item) in the tree of tree roots used to store the
>> list of options in the same format as they are passed to btrfs_mount().
>> This means any mount option that takes effect before the tree of tree
>> roots is setup is not supported.
>>
>> To set these options, the user space tool btrfstune was modified
>> to persist the list of options into an unmounted filesystem''s
>> tree of tree roots.
>
> We’ve seen this proposed in the past, IIRC this thread contains most of
what
> has been discussed:
> http://thread.gmane.org/gmane.comp.file-systems.btrfs/19757
Thanks, I missed to find that before.
The implementation is very different from the one I proposed.
>
> Implementing just whole-filesystem mount options is not convering all
usecases,
> more broad approach like per-subvolume or per-file settings is desired.
There
> are always settings that aren’t known now but would be useful in the
future, so
> we need a flexible infrastructure to maintain the properties.
>
> There was a proposed implementation for set/get properties:
> http://thread.gmane.org/gmane.comp.file-systems.btrfs/18287
Will take a detailed look at it.
Thanks for pointing it out David.
Why was it never picked?
>
> From the implementation side, I very much want to abandon the separate
> btrfstune utility. Currently it’s a bandaid because there’s nothing better
atm.
>
> Designing and merging the properties feature takes time, but we want to
tune
> simple things now. The wiki project mentions ‘tune2fs’ as an example, but
the
> project details are not always accurate about how to do the things, it’s
more
> like ideas what to do. If you’re going to work on that, please claim the
> project on the wiki, and possibly write more details abou the design.
I will.
>
> david


-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That''s why all progress depends on unreasonable men."
--
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

On Wed, Aug 7, 2013 at 11:48 AM, David Sterba <dsterba@suse.cz>
wrote:
> On Tue, Aug 06, 2013 at 04:05:50PM -0500, Eric Sandeen wrote:
>> I was thinking (and Josef just suggested too) that making a
>> dir flag, saying "everything under this dir gets compressed"
might make
>> more sense for that scenario than adding a whole slew of
>> on-disk-persistent-mount-option code.
>
> We have this dir flag stored in the attributes, chattr +c dir/, but this
> cannot be tuned further - no way to specify the compression algorithm
> (or for example the target ratio as compression hint), or we can''t
say
> "never compress files in this dir (even if mounted with
compression)"
> etc.
What Eric/Josef suggested, I think I''ve implemented it in the following
RFC:

https://patchwork.kernel.org/patch/2840235/

The only thing it doesn''t permit, is to disallow compression for
individual files or files placed under specific directories.
Any idea how would this be specified? (not via chattr I guess)

>
> david


-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That''s why all progress depends on unreasonable men."
--
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

Am Dienstag, 6. August 2013, 16:05:50 schrieb Eric
Sandeen:
> On 8/6/13 3:45 PM, Filipe David Manana wrote:
> > On Tue, Aug 6, 2013 at 9:37 PM, Eric Sandeen
<sandeen@redhat.com> wrote:
> >> On 8/6/13 1:27 PM, Filipe David Borba Manana wrote:
> >>> This change allows for most mount options to be persisted in
> >>> the filesystem, and be applied when the filesystem is mounted.
> >>> If the same options are specified at mount time, the persisted
> >>> values for those options are ignored.
> >>> 
> >>> The only options not supported are: subvol, subvolid,
subvolrootid,
> >>> device and thread_pool. This limitation is due to how this
feature
> >>> is implemented: basically there''s an optional value
(of type
> >>> struct btrfs_dir_item) in the tree of tree roots used to store
the
> >>> list of options in the same format as they are passed to
btrfs_mount().
> >>> This means any mount option that takes effect before the tree
of tree
> >>> roots is setup is not supported.
> >>> 
> >>> To set these options, the user space tool btrfstune was
modified
> >>> to persist the list of options into an unmounted
filesystem''s
> >>> tree of tree roots.
> >> 
> >> So, it does this thing, ok - but why?
> >> What is seen as the administrative advantage of this new
mechanism?
> >> 
> >> Just to play devil''s advocate, and to add a bit of
history:
> >> 
> >> On any production system, the filesystems will be mounted via
fstab,
> >> which has the advantages of being widely known, well understood,
and
> >> 100% expected - as well as being transparent, unsurprising, and
seamless.
> >> 
> >> For history: ext4 did this too.  And now it''s in a
situation where it''s
> >> got mount options coming at it from both the superblock and from
> >> the commandline (or fstab), and sometimes they conflict; it also
tries
> >> to report mount options in /proc/mounts, but has grown hairy code
> >> to decide which ones to print and which ones to not print (if
it''s
> >> a "default" option, don''t print it in
/proc/mounts, but what''s default,
> >> code-default or fs-default?)  And it''s really kind of an
ugly mess.
> >> 
> >> Further, mounting 2 filesystems w/ no options in fstab or on the
> >> commandline, and getting different behavior due to hidden (sorry,
> >> persistent) options in the fs itself is surprising, and surprise
> >> is rarely good.
> >> 
> >> So this patch adds 100+ lines of new code, to implement this idea,
but:
> >> what is the advantage?  Unless there is a compelling
administrative
> >> use case, I''d vote against it.  Lines of code that
don''t exist don''t
> >> have bugs.  ;)
> > 
> > There was a recent good example (imho at least) mentioned by Xavier
> > Gnata some time ago:
> > 
> > http://comments.gmane.org/gmane.comp.file-systems.btrfs/26011
> > 
> > cheers
> 
> Hm, I see.  I forgot about hotplugging in my "most systems mount
> via fstab" assertion.  :)
> 
> I was thinking (and Josef just suggested too) that making a
> dir flag, saying "everything under this dir gets compressed"
might make
> more sense for that scenario than adding a whole slew of
> on-disk-persistent-mount-option code.
> 
> Because really, the motivation sounds like it''s primarily for
significant
> on-disk format changes controlled by mount options.  I understand that
> motivation more than being able to persist something like
"noatime."
For a hotplug-able SSD having noatime stored persistently IMHO makes a lot of 
sense as well.

I won´t be surprised that at some time, extern SSDs or extern $successor-of-
SSD will be replacing extern harddisks.

-- 
Martin ''Helios'' Steigerwald - http://www.Lichtvoll.de
GPG: 03B0 0D6C 0040 0710 4AFA  B82F 991B EAAC A599 84C7
--
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

On Wed, Aug 07, 2013 at 03:46:20PM +0200, Martin Steigerwald
wrote:
> > Because really, the motivation sounds like it''s primarily for
significant
> > on-disk format changes controlled by mount options.  I understand that
> > motivation more than being able to persist something like
"noatime."
> 
> For a hotplug-able SSD having noatime stored persistently IMHO makes a lot
of
> sense as well.
I agree, and we can let btrfs understand noatime (or ro) even if they get
processed by vfs layer.
--
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

On Wed, Aug 07, 2013 at 12:33:09PM +0100, Filipe David Manana
wrote:
> Thanks, I missed to find that before.
> The implementation is very different from the one I proposed.
That''s one of the fundaental questions how to store the information:
inside existing structures, via xattrs, under new tree items. Each one
has pros and cons.
> > Designing and merging the properties feature takes time, but we want
to tune
> > simple things now. The wiki project mentions ‘tune2fs’ as an example,
but the
> > project details are not always accurate about how to do the things,
it’s more
> > like ideas what to do. If you’re going to work on that, please claim
the
> > project on the wiki, and possibly write more details abou the design.
> 
> I will.
The project is titled as persistent mount options, are you willing to
take the more general "per-object properties" task? IMHO
there''s not
much difference, the UI should be the same, just that it implements
per-fs or per-subvolume properties like mount options. The rest of the
object properties has to be collected and agreed on. I''m sure
there''s
community knowledge of what''s desired, so it''s a matter of
writing it
down and bikeshe^Wagreement on the naming syntax.

david
--
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

On Fri, Aug 9, 2013 at 1:01 AM, David Sterba <dsterba@suse.cz>
wrote:
> On Wed, Aug 07, 2013 at 12:33:09PM +0100, Filipe David Manana wrote:
>> Thanks, I missed to find that before.
>> The implementation is very different from the one I proposed.
>
> That''s one of the fundaental questions how to store the
information:
> inside existing structures, via xattrs, under new tree items. Each one
> has pros and cons.
>
>> > Designing and merging the properties feature takes time, but we
want to tune
>> > simple things now. The wiki project mentions ‘tune2fs’ as an
example, but the
>> > project details are not always accurate about how to do the
things, it’s more
>> > like ideas what to do. If you’re going to work on that, please
claim the
>> > project on the wiki, and possibly write more details abou the
design.
>>
>> I will.
>
> The project is titled as persistent mount options, are you willing to
> take the more general "per-object properties" task? IMHO
there''s not
> much difference, the UI should be the same, just that it implements
> per-fs or per-subvolume properties like mount options. The rest of the
> object properties has to be collected and agreed on. I''m sure
there''s
> community knowledge of what''s desired, so it''s a matter
of writing it
> down and bikeshe^Wagreement on the naming syntax.
Yes, I will.
I''ll get back to this soon and update the wiki page.

thanks
>
> david


-- 
Filipe David Manana,

"Reasonable men adapt themselves to the world.
 Unreasonable men adapt the world to themselves.
 That''s why all progress depends on unreasonable men."
--
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