On Tue, Aug 6, 2013 at 9:37 PM, Eric Sandeen <sandeen@xxxxxxxxxx> 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@xxxxxxxxx> >> --- >> 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
