| Message ID | 516691F0.4080204@cn.fujitsu.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, April 11, 2013 at 12:35 (+0200), Miao Xie wrote: > Now, we set incompat flag EXTEND_IREF when we actually need insert a > extend inode reference, not when making a fs. But some users may hope > that the fs still can be mounted on the old kernel, and don't hope we > insert any extend inode references. So we introduce noextiref mount > option to close this function. That's a much better approach compared to setting the flag on mkfs, I agree. > Signed-off-by: Miao Xie <miaox@cn.fujitsu.com> > Cc: Mark Fasheh <mfasheh@suse.de> > --- > fs/btrfs/ctree.h | 1 + > fs/btrfs/disk-io.c | 9 +++++++++ > fs/btrfs/inode-item.c | 2 +- > fs/btrfs/super.c | 41 ++++++++++++++++++++++++++++++++++++++++- > 4 files changed, 51 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h > index a883e47..db88963 100644 > --- a/fs/btrfs/ctree.h > +++ b/fs/btrfs/ctree.h > @@ -1911,6 +1911,7 @@ struct btrfs_ioctl_defrag_range_args { > #define BTRFS_MOUNT_CHECK_INTEGRITY (1 << 20) > #define BTRFS_MOUNT_CHECK_INTEGRITY_INCLUDING_EXTENT_DATA (1 << 21) > #define BTRFS_MOUNT_PANIC_ON_FATAL_ERROR (1 << 22) > +#define BTRFS_MOUNT_NOEXTIREF (1 << 23) > > #define btrfs_clear_opt(o, opt) ((o) &= ~BTRFS_MOUNT_##opt) > #define btrfs_set_opt(o, opt) ((o) |= BTRFS_MOUNT_##opt) > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index ab8ef37..ee00448 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -2269,6 +2269,15 @@ int open_ctree(struct super_block *sb, > goto fail_alloc; > } > > + if ((btrfs_super_incompat_flags(disk_super) & > + BTRFS_FEATURE_INCOMPAT_EXTENDED_IREF) && > + btrfs_test_opt(tree_root, NOEXTIREF)) { > + printk(KERN_ERR "BTRFS: couldn't mount because the extend iref" > + "can not be close.\n"); > + err = -EINVAL; > + goto fail_alloc; > + } > + > if (btrfs_super_leafsize(disk_super) != > btrfs_super_nodesize(disk_super)) { > printk(KERN_ERR "BTRFS: couldn't mount because metadata " > diff --git a/fs/btrfs/inode-item.c b/fs/btrfs/inode-item.c > index f07eb45..7c4f880 100644 > --- a/fs/btrfs/inode-item.c > +++ b/fs/btrfs/inode-item.c > @@ -442,7 +442,7 @@ int btrfs_insert_inode_ref(struct btrfs_trans_handle *trans, > out: > btrfs_free_path(path); > > - if (ret == -EMLINK) { > + if (ret == -EMLINK && !btrfs_test_opt(root, NOEXTIREF)) { > /* > * We ran out of space in the ref array. Need to add an > * extended ref. > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c > index 0f03569..fd375b3 100644 > --- a/fs/btrfs/super.c > +++ b/fs/btrfs/super.c > @@ -315,7 +315,7 @@ enum { > Opt_nodatacow, Opt_max_inline, Opt_alloc_start, Opt_nobarrier, Opt_ssd, > Opt_nossd, Opt_ssd_spread, Opt_thread_pool, Opt_noacl, Opt_compress, > Opt_compress_type, Opt_compress_force, Opt_compress_force_type, > - Opt_notreelog, Opt_ratio, Opt_flushoncommit, Opt_discard, > + Opt_notreelog, Opt_noextiref, Opt_ratio, Opt_flushoncommit, Opt_discard, > Opt_space_cache, Opt_clear_cache, Opt_user_subvol_rm_allowed, > Opt_enospc_debug, Opt_subvolrootid, Opt_defrag, Opt_inode_cache, > Opt_no_space_cache, Opt_recovery, Opt_skip_balance, > @@ -344,6 +344,7 @@ static match_table_t tokens = { > {Opt_nossd, "nossd"}, > {Opt_noacl, "noacl"}, > {Opt_notreelog, "notreelog"}, > + {Opt_noextiref, "noextiref"}, > {Opt_flushoncommit, "flushoncommit"}, > {Opt_ratio, "metadata_ratio=%d"}, > {Opt_discard, "discard"}, > @@ -535,6 +536,10 @@ int btrfs_parse_options(struct btrfs_root *root, char *options) > printk(KERN_INFO "btrfs: disabling tree log\n"); > btrfs_set_opt(info->mount_opt, NOTREELOG); > break; > + case Opt_noextiref: > + printk(KERN_INFO "btrfs: disabling extend inode ref\n"); > + btrfs_set_opt(info->mount_opt, NOEXTIREF); > + break; > case Opt_flushoncommit: > printk(KERN_INFO "btrfs: turning on flush-on-commit\n"); > btrfs_set_opt(info->mount_opt, FLUSHONCOMMIT); > @@ -1202,6 +1207,35 @@ static void btrfs_resize_thread_pool(struct btrfs_fs_info *fs_info, > new_pool_size); > } > > +static int btrfs_close_extend_iref(struct btrfs_fs_info *fs_info, > + unsigned long old_opts) The name irritated me, it's more like "unset" instead of "close", isn't it? > +{ > + struct btrfs_trans_handle *trans; > + int ret; > + > + if (btrfs_raw_test_opt(old_opts, NOEXTIREF) || > + !btrfs_raw_test_opt(fs_info->mount_opt, NOEXTIREF)) > + return 0; > + > + trans = btrfs_attach_transaction(fs_info->tree_root); > + if (IS_ERR(trans)) { > + if (PTR_ERR(trans) != -ENOENT) > + return PTR_ERR(trans); > + } else { > + ret = btrfs_commit_transaction(trans, fs_info->tree_root); > + if (ret) > + return ret; > + } Huh? I don't see why we need to commit the transaction here. Can you please explain? Thanks, -Jan > + > + if (btrfs_super_incompat_flags(fs_info->super_copy) & > + BTRFS_FEATURE_INCOMPAT_EXTENDED_IREF) { > + printk(KERN_ERR "BTRFS: could not close extend iref.\n"); > + return -EINVAL; > + } > + > + return 0; > +} > + > static inline void btrfs_remount_prepare(struct btrfs_fs_info *fs_info) > { > set_bit(BTRFS_FS_STATE_REMOUNTING, &fs_info->fs_state); > @@ -1259,6 +1293,11 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data) > } > > btrfs_remount_begin(fs_info, old_opts, *flags); > + > + ret = btrfs_close_extend_iref(fs_info, old_opts); > + if (ret) > + goto restore; > + > btrfs_resize_thread_pool(fs_info, > fs_info->thread_pool_size, old_thread_pool_size); > > -- 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 thu, 11 Apr 2013 16:29:48 +0200, Jan Schmidt wrote: > On Thu, April 11, 2013 at 12:35 (+0200), Miao Xie wrote: >> Now, we set incompat flag EXTEND_IREF when we actually need insert a >> extend inode reference, not when making a fs. But some users may hope >> that the fs still can be mounted on the old kernel, and don't hope we >> insert any extend inode references. So we introduce noextiref mount >> option to close this function. > > That's a much better approach compared to setting the flag on mkfs, I agree. > >> Signed-off-by: Miao Xie <miaox@cn.fujitsu.com> >> Cc: Mark Fasheh <mfasheh@suse.de> >> --- >> fs/btrfs/ctree.h | 1 + >> fs/btrfs/disk-io.c | 9 +++++++++ >> fs/btrfs/inode-item.c | 2 +- >> fs/btrfs/super.c | 41 ++++++++++++++++++++++++++++++++++++++++- >> 4 files changed, 51 insertions(+), 2 deletions(-) >> >> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h >> index a883e47..db88963 100644 >> --- a/fs/btrfs/ctree.h >> +++ b/fs/btrfs/ctree.h >> @@ -1911,6 +1911,7 @@ struct btrfs_ioctl_defrag_range_args { >> #define BTRFS_MOUNT_CHECK_INTEGRITY (1 << 20) >> #define BTRFS_MOUNT_CHECK_INTEGRITY_INCLUDING_EXTENT_DATA (1 << 21) >> #define BTRFS_MOUNT_PANIC_ON_FATAL_ERROR (1 << 22) >> +#define BTRFS_MOUNT_NOEXTIREF (1 << 23) >> >> #define btrfs_clear_opt(o, opt) ((o) &= ~BTRFS_MOUNT_##opt) >> #define btrfs_set_opt(o, opt) ((o) |= BTRFS_MOUNT_##opt) >> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c >> index ab8ef37..ee00448 100644 >> --- a/fs/btrfs/disk-io.c >> +++ b/fs/btrfs/disk-io.c >> @@ -2269,6 +2269,15 @@ int open_ctree(struct super_block *sb, >> goto fail_alloc; >> } >> >> + if ((btrfs_super_incompat_flags(disk_super) & >> + BTRFS_FEATURE_INCOMPAT_EXTENDED_IREF) && >> + btrfs_test_opt(tree_root, NOEXTIREF)) { >> + printk(KERN_ERR "BTRFS: couldn't mount because the extend iref" >> + "can not be close.\n"); >> + err = -EINVAL; >> + goto fail_alloc; >> + } >> + >> if (btrfs_super_leafsize(disk_super) != >> btrfs_super_nodesize(disk_super)) { >> printk(KERN_ERR "BTRFS: couldn't mount because metadata " >> diff --git a/fs/btrfs/inode-item.c b/fs/btrfs/inode-item.c >> index f07eb45..7c4f880 100644 >> --- a/fs/btrfs/inode-item.c >> +++ b/fs/btrfs/inode-item.c >> @@ -442,7 +442,7 @@ int btrfs_insert_inode_ref(struct btrfs_trans_handle *trans, >> out: >> btrfs_free_path(path); >> >> - if (ret == -EMLINK) { >> + if (ret == -EMLINK && !btrfs_test_opt(root, NOEXTIREF)) { >> /* >> * We ran out of space in the ref array. Need to add an >> * extended ref. >> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c >> index 0f03569..fd375b3 100644 >> --- a/fs/btrfs/super.c >> +++ b/fs/btrfs/super.c >> @@ -315,7 +315,7 @@ enum { >> Opt_nodatacow, Opt_max_inline, Opt_alloc_start, Opt_nobarrier, Opt_ssd, >> Opt_nossd, Opt_ssd_spread, Opt_thread_pool, Opt_noacl, Opt_compress, >> Opt_compress_type, Opt_compress_force, Opt_compress_force_type, >> - Opt_notreelog, Opt_ratio, Opt_flushoncommit, Opt_discard, >> + Opt_notreelog, Opt_noextiref, Opt_ratio, Opt_flushoncommit, Opt_discard, >> Opt_space_cache, Opt_clear_cache, Opt_user_subvol_rm_allowed, >> Opt_enospc_debug, Opt_subvolrootid, Opt_defrag, Opt_inode_cache, >> Opt_no_space_cache, Opt_recovery, Opt_skip_balance, >> @@ -344,6 +344,7 @@ static match_table_t tokens = { >> {Opt_nossd, "nossd"}, >> {Opt_noacl, "noacl"}, >> {Opt_notreelog, "notreelog"}, >> + {Opt_noextiref, "noextiref"}, >> {Opt_flushoncommit, "flushoncommit"}, >> {Opt_ratio, "metadata_ratio=%d"}, >> {Opt_discard, "discard"}, >> @@ -535,6 +536,10 @@ int btrfs_parse_options(struct btrfs_root *root, char *options) >> printk(KERN_INFO "btrfs: disabling tree log\n"); >> btrfs_set_opt(info->mount_opt, NOTREELOG); >> break; >> + case Opt_noextiref: >> + printk(KERN_INFO "btrfs: disabling extend inode ref\n"); >> + btrfs_set_opt(info->mount_opt, NOEXTIREF); >> + break; >> case Opt_flushoncommit: >> printk(KERN_INFO "btrfs: turning on flush-on-commit\n"); >> btrfs_set_opt(info->mount_opt, FLUSHONCOMMIT); >> @@ -1202,6 +1207,35 @@ static void btrfs_resize_thread_pool(struct btrfs_fs_info *fs_info, >> new_pool_size); >> } >> >> +static int btrfs_close_extend_iref(struct btrfs_fs_info *fs_info, >> + unsigned long old_opts) > > The name irritated me, it's more like "unset" instead of "close", isn't it? Maybe "btrfs_set_no_extend_iref()" is better, the other developers might think we will clear BTRFS_FEATURE_INCOMPAT_EXTENDED_IREF. > >> +{ >> + struct btrfs_trans_handle *trans; >> + int ret; >> + >> + if (btrfs_raw_test_opt(old_opts, NOEXTIREF) || >> + !btrfs_raw_test_opt(fs_info->mount_opt, NOEXTIREF)) >> + return 0; >> + >> + trans = btrfs_attach_transaction(fs_info->tree_root); >> + if (IS_ERR(trans)) { >> + if (PTR_ERR(trans) != -ENOENT) >> + return PTR_ERR(trans); >> + } else { >> + ret = btrfs_commit_transaction(trans, fs_info->tree_root); >> + if (ret) >> + return ret; >> + } > > Huh? I don't see why we need to commit the transaction here. Can you please explain? We need avoid the case that we check incompat flag is set or not between the extended iref insertion and incompat flag set. Task1 Task2 start_transaction() insert extended iref set NOEXTIREF check incompat flag set incompat flag checking incompat flag after transaction commit can make sure our check happens after the flag is set. Thanks Miao > > Thanks, > -Jan > >> + >> + if (btrfs_super_incompat_flags(fs_info->super_copy) & >> + BTRFS_FEATURE_INCOMPAT_EXTENDED_IREF) { >> + printk(KERN_ERR "BTRFS: could not close extend iref.\n"); >> + return -EINVAL; >> + } >> + >> + return 0; >> +} >> + >> static inline void btrfs_remount_prepare(struct btrfs_fs_info *fs_info) >> { >> set_bit(BTRFS_FS_STATE_REMOUNTING, &fs_info->fs_state); >> @@ -1259,6 +1293,11 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data) >> } >> >> btrfs_remount_begin(fs_info, old_opts, *flags); >> + >> + ret = btrfs_close_extend_iref(fs_info, old_opts); >> + if (ret) >> + goto restore; >> + >> btrfs_resize_thread_pool(fs_info, >> fs_info->thread_pool_size, old_thread_pool_size); >> >> > -- > 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 > -- 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, April 12, 2013 at 06:13 (+0200), Miao Xie wrote: > On thu, 11 Apr 2013 16:29:48 +0200, Jan Schmidt wrote: >> On Thu, April 11, 2013 at 12:35 (+0200), Miao Xie wrote: >>> Now, we set incompat flag EXTEND_IREF when we actually need insert a >>> extend inode reference, not when making a fs. But some users may hope >>> that the fs still can be mounted on the old kernel, and don't hope we >>> insert any extend inode references. So we introduce noextiref mount >>> option to close this function. >> >> That's a much better approach compared to setting the flag on mkfs, I agree. >> >>> Signed-off-by: Miao Xie <miaox@cn.fujitsu.com> >>> Cc: Mark Fasheh <mfasheh@suse.de> >>> --- >>> fs/btrfs/ctree.h | 1 + >>> fs/btrfs/disk-io.c | 9 +++++++++ >>> fs/btrfs/inode-item.c | 2 +- >>> fs/btrfs/super.c | 41 ++++++++++++++++++++++++++++++++++++++++- >>> 4 files changed, 51 insertions(+), 2 deletions(-) >>> >>> diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h >>> index a883e47..db88963 100644 >>> --- a/fs/btrfs/ctree.h >>> +++ b/fs/btrfs/ctree.h >>> @@ -1911,6 +1911,7 @@ struct btrfs_ioctl_defrag_range_args { >>> #define BTRFS_MOUNT_CHECK_INTEGRITY (1 << 20) >>> #define BTRFS_MOUNT_CHECK_INTEGRITY_INCLUDING_EXTENT_DATA (1 << 21) >>> #define BTRFS_MOUNT_PANIC_ON_FATAL_ERROR (1 << 22) >>> +#define BTRFS_MOUNT_NOEXTIREF (1 << 23) >>> >>> #define btrfs_clear_opt(o, opt) ((o) &= ~BTRFS_MOUNT_##opt) >>> #define btrfs_set_opt(o, opt) ((o) |= BTRFS_MOUNT_##opt) >>> diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c >>> index ab8ef37..ee00448 100644 >>> --- a/fs/btrfs/disk-io.c >>> +++ b/fs/btrfs/disk-io.c >>> @@ -2269,6 +2269,15 @@ int open_ctree(struct super_block *sb, >>> goto fail_alloc; >>> } >>> >>> + if ((btrfs_super_incompat_flags(disk_super) & >>> + BTRFS_FEATURE_INCOMPAT_EXTENDED_IREF) && >>> + btrfs_test_opt(tree_root, NOEXTIREF)) { >>> + printk(KERN_ERR "BTRFS: couldn't mount because the extend iref" >>> + "can not be close.\n"); >>> + err = -EINVAL; >>> + goto fail_alloc; >>> + } >>> + >>> if (btrfs_super_leafsize(disk_super) != >>> btrfs_super_nodesize(disk_super)) { >>> printk(KERN_ERR "BTRFS: couldn't mount because metadata " >>> diff --git a/fs/btrfs/inode-item.c b/fs/btrfs/inode-item.c >>> index f07eb45..7c4f880 100644 >>> --- a/fs/btrfs/inode-item.c >>> +++ b/fs/btrfs/inode-item.c >>> @@ -442,7 +442,7 @@ int btrfs_insert_inode_ref(struct btrfs_trans_handle *trans, >>> out: >>> btrfs_free_path(path); >>> >>> - if (ret == -EMLINK) { >>> + if (ret == -EMLINK && !btrfs_test_opt(root, NOEXTIREF)) { >>> /* >>> * We ran out of space in the ref array. Need to add an >>> * extended ref. >>> diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c >>> index 0f03569..fd375b3 100644 >>> --- a/fs/btrfs/super.c >>> +++ b/fs/btrfs/super.c >>> @@ -315,7 +315,7 @@ enum { >>> Opt_nodatacow, Opt_max_inline, Opt_alloc_start, Opt_nobarrier, Opt_ssd, >>> Opt_nossd, Opt_ssd_spread, Opt_thread_pool, Opt_noacl, Opt_compress, >>> Opt_compress_type, Opt_compress_force, Opt_compress_force_type, >>> - Opt_notreelog, Opt_ratio, Opt_flushoncommit, Opt_discard, >>> + Opt_notreelog, Opt_noextiref, Opt_ratio, Opt_flushoncommit, Opt_discard, >>> Opt_space_cache, Opt_clear_cache, Opt_user_subvol_rm_allowed, >>> Opt_enospc_debug, Opt_subvolrootid, Opt_defrag, Opt_inode_cache, >>> Opt_no_space_cache, Opt_recovery, Opt_skip_balance, >>> @@ -344,6 +344,7 @@ static match_table_t tokens = { >>> {Opt_nossd, "nossd"}, >>> {Opt_noacl, "noacl"}, >>> {Opt_notreelog, "notreelog"}, >>> + {Opt_noextiref, "noextiref"}, >>> {Opt_flushoncommit, "flushoncommit"}, >>> {Opt_ratio, "metadata_ratio=%d"}, >>> {Opt_discard, "discard"}, >>> @@ -535,6 +536,10 @@ int btrfs_parse_options(struct btrfs_root *root, char *options) >>> printk(KERN_INFO "btrfs: disabling tree log\n"); >>> btrfs_set_opt(info->mount_opt, NOTREELOG); >>> break; >>> + case Opt_noextiref: >>> + printk(KERN_INFO "btrfs: disabling extend inode ref\n"); >>> + btrfs_set_opt(info->mount_opt, NOEXTIREF); >>> + break; >>> case Opt_flushoncommit: >>> printk(KERN_INFO "btrfs: turning on flush-on-commit\n"); >>> btrfs_set_opt(info->mount_opt, FLUSHONCOMMIT); >>> @@ -1202,6 +1207,35 @@ static void btrfs_resize_thread_pool(struct btrfs_fs_info *fs_info, >>> new_pool_size); >>> } >>> >>> +static int btrfs_close_extend_iref(struct btrfs_fs_info *fs_info, >>> + unsigned long old_opts) >> >> The name irritated me, it's more like "unset" instead of "close", isn't it? > > Maybe "btrfs_set_no_extend_iref()" is better, the other developers might think > we will clear BTRFS_FEATURE_INCOMPAT_EXTENDED_IREF. I think we should use the exact name of the mount option, so btrfs_set_noextiref is probably least ambiguous. Or even btrfs_set_mntflag_noextiref. >> >>> +{ >>> + struct btrfs_trans_handle *trans; >>> + int ret; >>> + >>> + if (btrfs_raw_test_opt(old_opts, NOEXTIREF) || >>> + !btrfs_raw_test_opt(fs_info->mount_opt, NOEXTIREF)) >>> + return 0; >>> + >>> + trans = btrfs_attach_transaction(fs_info->tree_root); >>> + if (IS_ERR(trans)) { >>> + if (PTR_ERR(trans) != -ENOENT) >>> + return PTR_ERR(trans); >>> + } else { >>> + ret = btrfs_commit_transaction(trans, fs_info->tree_root); >>> + if (ret) >>> + return ret; >>> + } >> >> Huh? I don't see why we need to commit the transaction here. Can you please explain? > > We need avoid the case that we check incompat flag is set or not between the > extended iref insertion and incompat flag set. > Task1 Task2 > start_transaction() > insert extended iref > set NOEXTIREF > check incompat flag > set incompat flag > > checking incompat flag after transaction commit can make sure our check happens > after the flag is set. Understood. However, in my understanding of transaction.c, btrfs_join_transaction, btrfs_attach_transaction and btrfs_commit_transaction are special and need justification. If you only need the transaction for synchronization purposes, which seems to be the case here, btrfs_start_transaction and btrfs_end_transaction are the right choice. Thanks, -Jan > Thanks > Miao > >> >> Thanks, >> -Jan >> >>> + >>> + if (btrfs_super_incompat_flags(fs_info->super_copy) & >>> + BTRFS_FEATURE_INCOMPAT_EXTENDED_IREF) { >>> + printk(KERN_ERR "BTRFS: could not close extend iref.\n"); >>> + return -EINVAL; >>> + } >>> + >>> + return 0; >>> +} >>> + >>> static inline void btrfs_remount_prepare(struct btrfs_fs_info *fs_info) >>> { >>> set_bit(BTRFS_FS_STATE_REMOUNTING, &fs_info->fs_state); >>> @@ -1259,6 +1293,11 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data) >>> } >>> >>> btrfs_remount_begin(fs_info, old_opts, *flags); >>> + >>> + ret = btrfs_close_extend_iref(fs_info, old_opts); >>> + if (ret) >>> + goto restore; >>> + >>> btrfs_resize_thread_pool(fs_info, >>> fs_info->thread_pool_size, old_thread_pool_size); >>> >>> >> -- >> 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 >> > -- 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 4/11/13 5:35 AM, Miao Xie wrote: > Now, we set incompat flag EXTEND_IREF when we actually need insert a > extend inode reference, not when making a fs. But some users may hope > that the fs still can be mounted on the old kernel, and don't hope we > insert any extend inode references. So we introduce noextiref mount > option to close this function. I'd really rather not have yet another work-around mount option. Wouldn't it be better to say: if you don't want extended irefs, turn that feature off on the filesystem itself, either at mkfs time or via btrfstune after the fact. So rather than opportunistically turning it on, and then adding another patch to disable that behavior with a mount option, why not just teach btrfstune to turn off the flag, if it's not desired by the admin? It'd save a mount option, and 50 or so lines of new kernel code, and make things simpler overall, IMHO. Thanks, -Eric > Signed-off-by: Miao Xie <miaox@cn.fujitsu.com> > Cc: Mark Fasheh <mfasheh@suse.de> > --- > fs/btrfs/ctree.h | 1 + > fs/btrfs/disk-io.c | 9 +++++++++ > fs/btrfs/inode-item.c | 2 +- > fs/btrfs/super.c | 41 ++++++++++++++++++++++++++++++++++++++++- > 4 files changed, 51 insertions(+), 2 deletions(-) > > diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h > index a883e47..db88963 100644 > --- a/fs/btrfs/ctree.h > +++ b/fs/btrfs/ctree.h > @@ -1911,6 +1911,7 @@ struct btrfs_ioctl_defrag_range_args { > #define BTRFS_MOUNT_CHECK_INTEGRITY (1 << 20) > #define BTRFS_MOUNT_CHECK_INTEGRITY_INCLUDING_EXTENT_DATA (1 << 21) > #define BTRFS_MOUNT_PANIC_ON_FATAL_ERROR (1 << 22) > +#define BTRFS_MOUNT_NOEXTIREF (1 << 23) > > #define btrfs_clear_opt(o, opt) ((o) &= ~BTRFS_MOUNT_##opt) > #define btrfs_set_opt(o, opt) ((o) |= BTRFS_MOUNT_##opt) > diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c > index ab8ef37..ee00448 100644 > --- a/fs/btrfs/disk-io.c > +++ b/fs/btrfs/disk-io.c > @@ -2269,6 +2269,15 @@ int open_ctree(struct super_block *sb, > goto fail_alloc; > } > > + if ((btrfs_super_incompat_flags(disk_super) & > + BTRFS_FEATURE_INCOMPAT_EXTENDED_IREF) && > + btrfs_test_opt(tree_root, NOEXTIREF)) { > + printk(KERN_ERR "BTRFS: couldn't mount because the extend iref" > + "can not be close.\n"); > + err = -EINVAL; > + goto fail_alloc; > + } > + > if (btrfs_super_leafsize(disk_super) != > btrfs_super_nodesize(disk_super)) { > printk(KERN_ERR "BTRFS: couldn't mount because metadata " > diff --git a/fs/btrfs/inode-item.c b/fs/btrfs/inode-item.c > index f07eb45..7c4f880 100644 > --- a/fs/btrfs/inode-item.c > +++ b/fs/btrfs/inode-item.c > @@ -442,7 +442,7 @@ int btrfs_insert_inode_ref(struct btrfs_trans_handle *trans, > out: > btrfs_free_path(path); > > - if (ret == -EMLINK) { > + if (ret == -EMLINK && !btrfs_test_opt(root, NOEXTIREF)) { > /* > * We ran out of space in the ref array. Need to add an > * extended ref. > diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c > index 0f03569..fd375b3 100644 > --- a/fs/btrfs/super.c > +++ b/fs/btrfs/super.c > @@ -315,7 +315,7 @@ enum { > Opt_nodatacow, Opt_max_inline, Opt_alloc_start, Opt_nobarrier, Opt_ssd, > Opt_nossd, Opt_ssd_spread, Opt_thread_pool, Opt_noacl, Opt_compress, > Opt_compress_type, Opt_compress_force, Opt_compress_force_type, > - Opt_notreelog, Opt_ratio, Opt_flushoncommit, Opt_discard, > + Opt_notreelog, Opt_noextiref, Opt_ratio, Opt_flushoncommit, Opt_discard, > Opt_space_cache, Opt_clear_cache, Opt_user_subvol_rm_allowed, > Opt_enospc_debug, Opt_subvolrootid, Opt_defrag, Opt_inode_cache, > Opt_no_space_cache, Opt_recovery, Opt_skip_balance, > @@ -344,6 +344,7 @@ static match_table_t tokens = { > {Opt_nossd, "nossd"}, > {Opt_noacl, "noacl"}, > {Opt_notreelog, "notreelog"}, > + {Opt_noextiref, "noextiref"}, > {Opt_flushoncommit, "flushoncommit"}, > {Opt_ratio, "metadata_ratio=%d"}, > {Opt_discard, "discard"}, > @@ -535,6 +536,10 @@ int btrfs_parse_options(struct btrfs_root *root, char *options) > printk(KERN_INFO "btrfs: disabling tree log\n"); > btrfs_set_opt(info->mount_opt, NOTREELOG); > break; > + case Opt_noextiref: > + printk(KERN_INFO "btrfs: disabling extend inode ref\n"); > + btrfs_set_opt(info->mount_opt, NOEXTIREF); > + break; > case Opt_flushoncommit: > printk(KERN_INFO "btrfs: turning on flush-on-commit\n"); > btrfs_set_opt(info->mount_opt, FLUSHONCOMMIT); > @@ -1202,6 +1207,35 @@ static void btrfs_resize_thread_pool(struct btrfs_fs_info *fs_info, > new_pool_size); > } > > +static int btrfs_close_extend_iref(struct btrfs_fs_info *fs_info, > + unsigned long old_opts) > +{ > + struct btrfs_trans_handle *trans; > + int ret; > + > + if (btrfs_raw_test_opt(old_opts, NOEXTIREF) || > + !btrfs_raw_test_opt(fs_info->mount_opt, NOEXTIREF)) > + return 0; > + > + trans = btrfs_attach_transaction(fs_info->tree_root); > + if (IS_ERR(trans)) { > + if (PTR_ERR(trans) != -ENOENT) > + return PTR_ERR(trans); > + } else { > + ret = btrfs_commit_transaction(trans, fs_info->tree_root); > + if (ret) > + return ret; > + } > + > + if (btrfs_super_incompat_flags(fs_info->super_copy) & > + BTRFS_FEATURE_INCOMPAT_EXTENDED_IREF) { > + printk(KERN_ERR "BTRFS: could not close extend iref.\n"); > + return -EINVAL; > + } > + > + return 0; > +} > + > static inline void btrfs_remount_prepare(struct btrfs_fs_info *fs_info) > { > set_bit(BTRFS_FS_STATE_REMOUNTING, &fs_info->fs_state); > @@ -1259,6 +1293,11 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data) > } > > btrfs_remount_begin(fs_info, old_opts, *flags); > + > + ret = btrfs_close_extend_iref(fs_info, old_opts); > + if (ret) > + goto restore; > + > btrfs_resize_thread_pool(fs_info, > fs_info->thread_pool_size, old_thread_pool_size); > > -- 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, 12 Apr 2013 09:02:34 +0200, Jan Schmidt wrote: >>>> +static int btrfs_close_extend_iref(struct btrfs_fs_info *fs_info, >>>> + unsigned long old_opts) >>> >>> The name irritated me, it's more like "unset" instead of "close", isn't it? >> >> Maybe "btrfs_set_no_extend_iref()" is better, the other developers might think >> we will clear BTRFS_FEATURE_INCOMPAT_EXTENDED_IREF. > > I think we should use the exact name of the mount option, so > btrfs_set_noextiref is probably least ambiguous. Or even > btrfs_set_mntflag_noextiref. Much better than mine. >>> >>>> +{ >>>> + struct btrfs_trans_handle *trans; >>>> + int ret; >>>> + >>>> + if (btrfs_raw_test_opt(old_opts, NOEXTIREF) || >>>> + !btrfs_raw_test_opt(fs_info->mount_opt, NOEXTIREF)) >>>> + return 0; >>>> + >>>> + trans = btrfs_attach_transaction(fs_info->tree_root); >>>> + if (IS_ERR(trans)) { >>>> + if (PTR_ERR(trans) != -ENOENT) >>>> + return PTR_ERR(trans); >>>> + } else { >>>> + ret = btrfs_commit_transaction(trans, fs_info->tree_root); >>>> + if (ret) >>>> + return ret; >>>> + } >>> >>> Huh? I don't see why we need to commit the transaction here. Can you please explain? >> >> We need avoid the case that we check incompat flag is set or not between the >> extended iref insertion and incompat flag set. >> Task1 Task2 >> start_transaction() >> insert extended iref >> set NOEXTIREF >> check incompat flag >> set incompat flag >> >> checking incompat flag after transaction commit can make sure our check happens >> after the flag is set. > > Understood. > > However, in my understanding of transaction.c, btrfs_join_transaction, > btrfs_attach_transaction and btrfs_commit_transaction are special and need > justification. If you only need the transaction for synchronization purposes, > which seems to be the case here, btrfs_start_transaction and > btrfs_end_transaction are the right choice. btrfs_end_transaction() does not wait for/force the other tasks to end their transaction, so it is not right here. Thanks Miao > > Thanks, > -Jan > >> Thanks >> Miao >> >>> >>> Thanks, >>> -Jan >>> >>>> + >>>> + if (btrfs_super_incompat_flags(fs_info->super_copy) & >>>> + BTRFS_FEATURE_INCOMPAT_EXTENDED_IREF) { >>>> + printk(KERN_ERR "BTRFS: could not close extend iref.\n"); >>>> + return -EINVAL; >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> static inline void btrfs_remount_prepare(struct btrfs_fs_info *fs_info) >>>> { >>>> set_bit(BTRFS_FS_STATE_REMOUNTING, &fs_info->fs_state); >>>> @@ -1259,6 +1293,11 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data) >>>> } >>>> >>>> btrfs_remount_begin(fs_info, old_opts, *flags); >>>> + >>>> + ret = btrfs_close_extend_iref(fs_info, old_opts); >>>> + if (ret) >>>> + goto restore; >>>> + >>>> btrfs_resize_thread_pool(fs_info, >>>> fs_info->thread_pool_size, old_thread_pool_size); >>>> >>>> >>> -- >>> 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 >>> >> > -- 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 Mon, April 15, 2013 at 04:58 (+0200), Miao Xie wrote: > On Fri, 12 Apr 2013 09:02:34 +0200, Jan Schmidt wrote: >>>>> +static int btrfs_close_extend_iref(struct btrfs_fs_info *fs_info, >>>>> + unsigned long old_opts) >>>> >>>> The name irritated me, it's more like "unset" instead of "close", isn't it? >>> >>> Maybe "btrfs_set_no_extend_iref()" is better, the other developers might think >>> we will clear BTRFS_FEATURE_INCOMPAT_EXTENDED_IREF. >> >> I think we should use the exact name of the mount option, so >> btrfs_set_noextiref is probably least ambiguous. Or even >> btrfs_set_mntflag_noextiref. > > Much better than mine. > >>>> >>>>> +{ >>>>> + struct btrfs_trans_handle *trans; >>>>> + int ret; >>>>> + >>>>> + if (btrfs_raw_test_opt(old_opts, NOEXTIREF) || >>>>> + !btrfs_raw_test_opt(fs_info->mount_opt, NOEXTIREF)) >>>>> + return 0; >>>>> + >>>>> + trans = btrfs_attach_transaction(fs_info->tree_root); >>>>> + if (IS_ERR(trans)) { >>>>> + if (PTR_ERR(trans) != -ENOENT) >>>>> + return PTR_ERR(trans); >>>>> + } else { >>>>> + ret = btrfs_commit_transaction(trans, fs_info->tree_root); >>>>> + if (ret) >>>>> + return ret; >>>>> + } >>>> >>>> Huh? I don't see why we need to commit the transaction here. Can you please explain? >>> >>> We need avoid the case that we check incompat flag is set or not between the >>> extended iref insertion and incompat flag set. >>> Task1 Task2 >>> start_transaction() >>> insert extended iref >>> set NOEXTIREF >>> check incompat flag >>> set incompat flag >>> >>> checking incompat flag after transaction commit can make sure our check happens >>> after the flag is set. >> >> Understood. >> >> However, in my understanding of transaction.c, btrfs_join_transaction, >> btrfs_attach_transaction and btrfs_commit_transaction are special and need >> justification. If you only need the transaction for synchronization purposes, >> which seems to be the case here, btrfs_start_transaction and >> btrfs_end_transaction are the right choice. > > btrfs_end_transaction() does not wait for/force the other tasks to end their > transaction, so it is not right here. Now I see what you're actually synchronizing, thanks. I still don't see why your're using attach instead of join, but that's probably just a minor thing. However, ... > > Thanks > Miao > >> >> Thanks, >> -Jan >> >>> Thanks >>> Miao >>> >>>> >>>> Thanks, >>>> -Jan >>>> >>>>> + >>>>> + if (btrfs_super_incompat_flags(fs_info->super_copy) & >>>>> + BTRFS_FEATURE_INCOMPAT_EXTENDED_IREF) { >>>>> + printk(KERN_ERR "BTRFS: could not close extend iref.\n"); >>>>> + return -EINVAL; >>>>> + } >>>>> + >>>>> + return 0; >>>>> +} >>>>> + >>>>> static inline void btrfs_remount_prepare(struct btrfs_fs_info *fs_info) >>>>> { >>>>> set_bit(BTRFS_FS_STATE_REMOUNTING, &fs_info->fs_state); >>>>> @@ -1259,6 +1293,11 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data) >>>>> } >>>>> >>>>> btrfs_remount_begin(fs_info, old_opts, *flags); >>>>> + >>>>> + ret = btrfs_close_extend_iref(fs_info, old_opts); >>>>> + if (ret) >>>>> + goto restore; >>>>> + ... btrfs_remount_prepare is called even before btrfs_parse_options (which subsequently can return early with -EINVAL). So, it really shouldn't so a transaction commit in my opinion. Later, at least in the read-only case, btrfs_commit_super is doing a commit anyway - so perhaps you can find a way of not introducing a double commit just for this mount flag. Last but not least, Eric has made a good point, too. I'm undecided if a new mount option would in fact be better compared to btrfstune. -Jan >>>>> btrfs_resize_thread_pool(fs_info, >>>>> fs_info->thread_pool_size, old_thread_pool_size); >>>>> >>>>> >>>> -- >>>> 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 >>>> >>> >> > > -- > 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 > -- 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
Jan Schmidt ??: > On Mon, April 15, 2013 at 04:58 (+0200), Miao Xie wrote: >> On Fri, 12 Apr 2013 09:02:34 +0200, Jan Schmidt wrote: >>>>>> +static int btrfs_close_extend_iref(struct btrfs_fs_info *fs_info, >>>>>> + unsigned long old_opts) >>>>> The name irritated me, it's more like "unset" instead of "close", isn't it? >>>> Maybe "btrfs_set_no_extend_iref()" is better, the other developers might think >>>> we will clear BTRFS_FEATURE_INCOMPAT_EXTENDED_IREF. >>> I think we should use the exact name of the mount option, so >>> btrfs_set_noextiref is probably least ambiguous. Or even >>> btrfs_set_mntflag_noextiref. >> Much better than mine. >> >>>>>> +{ >>>>>> + struct btrfs_trans_handle *trans; >>>>>> + int ret; >>>>>> + >>>>>> + if (btrfs_raw_test_opt(old_opts, NOEXTIREF) || >>>>>> + !btrfs_raw_test_opt(fs_info->mount_opt, NOEXTIREF)) >>>>>> + return 0; >>>>>> + >>>>>> + trans = btrfs_attach_transaction(fs_info->tree_root); >>>>>> + if (IS_ERR(trans)) { >>>>>> + if (PTR_ERR(trans) != -ENOENT) >>>>>> + return PTR_ERR(trans); >>>>>> + } else { >>>>>> + ret = btrfs_commit_transaction(trans, fs_info->tree_root); >>>>>> + if (ret) >>>>>> + return ret; >>>>>> + } >>>>> Huh? I don't see why we need to commit the transaction here. Can you please explain? >>>> We need avoid the case that we check incompat flag is set or not between the >>>> extended iref insertion and incompat flag set. >>>> Task1 Task2 >>>> start_transaction() >>>> insert extended iref >>>> set NOEXTIREF >>>> check incompat flag >>>> set incompat flag >>>> >>>> checking incompat flag after transaction commit can make sure our check happens >>>> after the flag is set. >>> Understood. >>> >>> However, in my understanding of transaction.c, btrfs_join_transaction, >>> btrfs_attach_transaction and btrfs_commit_transaction are special and need >>> justification. If you only need the transaction for synchronization purposes, >>> which seems to be the case here, btrfs_start_transaction and >>> btrfs_end_transaction are the right choice. >> btrfs_end_transaction() does not wait for/force the other tasks to end their >> transaction, so it is not right here. > > Now I see what you're actually synchronizing, thanks. I still don't see why > your're using attach instead of join, but that's probably just a minor thing. > > However, ... Hello Jan. miao is out for LSF.... However, btrfs_attach_transaction() catch the running transaction which is used when we want to commit the transaction, but we don't want to start a new one. Maybe this will help you.... Thanks, Wang > >> Thanks >> Miao >> >>> Thanks, >>> -Jan >>> >>>> Thanks >>>> Miao >>>> >>>>> Thanks, >>>>> -Jan >>>>> >>>>>> + >>>>>> + if (btrfs_super_incompat_flags(fs_info->super_copy) & >>>>>> + BTRFS_FEATURE_INCOMPAT_EXTENDED_IREF) { >>>>>> + printk(KERN_ERR "BTRFS: could not close extend iref.\n"); >>>>>> + return -EINVAL; >>>>>> + } >>>>>> + >>>>>> + return 0; >>>>>> +} >>>>>> + >>>>>> static inline void btrfs_remount_prepare(struct btrfs_fs_info *fs_info) >>>>>> { >>>>>> set_bit(BTRFS_FS_STATE_REMOUNTING, &fs_info->fs_state); >>>>>> @@ -1259,6 +1293,11 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data) >>>>>> } >>>>>> >>>>>> btrfs_remount_begin(fs_info, old_opts, *flags); >>>>>> + >>>>>> + ret = btrfs_close_extend_iref(fs_info, old_opts); >>>>>> + if (ret) >>>>>> + goto restore; >>>>>> + > > ... btrfs_remount_prepare is called even before btrfs_parse_options (which > subsequently can return early with -EINVAL). So, it really shouldn't so a > transaction commit in my opinion. Later, at least in the read-only case, > btrfs_commit_super is doing a commit anyway - so perhaps you can find a way of > not introducing a double commit just for this mount flag. > > Last but not least, Eric has made a good point, too. I'm undecided if a new > mount option would in fact be better compared to btrfstune. > > -Jan > >>>>>> btrfs_resize_thread_pool(fs_info, >>>>>> fs_info->thread_pool_size, old_thread_pool_size); >>>>>> >>>>>> >>>>> -- >>>>> 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 >>>>> >> -- >> 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 >> > -- > 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 > > -- 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, Apr 12, 2013 at 12:01:19PM -0500, Eric Sandeen wrote: > On 4/11/13 5:35 AM, Miao Xie wrote: > > Now, we set incompat flag EXTEND_IREF when we actually need insert a > > extend inode reference, not when making a fs. But some users may hope > > that the fs still can be mounted on the old kernel, and don't hope we > > insert any extend inode references. So we introduce noextiref mount > > option to close this function. > > I'd really rather not have yet another work-around mount option. > > Wouldn't it be better to say: if you don't want extended irefs, turn > that feature off on the filesystem itself, either at mkfs time or via > btrfstune after the fact. > > So rather than opportunistically turning it on, and then adding another > patch to disable that behavior with a mount option, why not just teach > btrfstune to turn off the flag, if it's not desired by the admin? > > It'd save a mount option, and 50 or so lines of new kernel code, > and make things simpler overall, IMHO. I don't like this so much either - in particular I don't like the fs to silently turn on incompat flags based on some heuristic or random number that the user won't likely know (in this case, the number of bytes that would fit in a standard btrfs hard link). Tunefs can already turn the flag on, I don't see why we don't add the reverse functionality. Also making it optional on mkfs time is easy enough... --Mark -- Mark Fasheh -- 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, Apr 12, 2013 at 12:01:19PM -0500, Eric Sandeen wrote: > On 4/11/13 5:35 AM, Miao Xie wrote: > > Now, we set incompat flag EXTEND_IREF when we actually need insert a > > extend inode reference, not when making a fs. But some users may hope > > that the fs still can be mounted on the old kernel, and don't hope we > > insert any extend inode references. So we introduce noextiref mount > > option to close this function. > > I'd really rather not have yet another work-around mount option. > > Wouldn't it be better to say: if you don't want extended irefs, turn > that feature off on the filesystem itself, either at mkfs time or via > btrfstune after the fact. I agree with this, and hope the inconsistency around extref is only temporary so the mount options is not required in the long term. The code reverting extref set by default in mkfs is in integration branch. The preferred solution is the -O option where we can put all the fs features in one go at mkfs time. 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 Mon, 15 Apr 2013 19:20:51 +0200, David Sterba wrote: > On Fri, Apr 12, 2013 at 12:01:19PM -0500, Eric Sandeen wrote: >> On 4/11/13 5:35 AM, Miao Xie wrote: >>> Now, we set incompat flag EXTEND_IREF when we actually need insert a >>> extend inode reference, not when making a fs. But some users may hope >>> that the fs still can be mounted on the old kernel, and don't hope we >>> insert any extend inode references. So we introduce noextiref mount >>> option to close this function. >> >> I'd really rather not have yet another work-around mount option. >> >> Wouldn't it be better to say: if you don't want extended irefs, turn >> that feature off on the filesystem itself, either at mkfs time or via >> btrfstune after the fact. > > I agree with this, and hope the inconsistency around extref is only > temporary so the mount options is not required in the long term. > > The code reverting extref set by default in mkfs is in integration > branch. > > The preferred solution is the -O option where we can put all the fs > features in one go at mkfs time. All right, let's add a option for mkfs, and throw away this patchset. Thanks. Miao -- 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
diff --git a/fs/btrfs/ctree.h b/fs/btrfs/ctree.h index a883e47..db88963 100644 --- a/fs/btrfs/ctree.h +++ b/fs/btrfs/ctree.h @@ -1911,6 +1911,7 @@ struct btrfs_ioctl_defrag_range_args { #define BTRFS_MOUNT_CHECK_INTEGRITY (1 << 20) #define BTRFS_MOUNT_CHECK_INTEGRITY_INCLUDING_EXTENT_DATA (1 << 21) #define BTRFS_MOUNT_PANIC_ON_FATAL_ERROR (1 << 22) +#define BTRFS_MOUNT_NOEXTIREF (1 << 23) #define btrfs_clear_opt(o, opt) ((o) &= ~BTRFS_MOUNT_##opt) #define btrfs_set_opt(o, opt) ((o) |= BTRFS_MOUNT_##opt) diff --git a/fs/btrfs/disk-io.c b/fs/btrfs/disk-io.c index ab8ef37..ee00448 100644 --- a/fs/btrfs/disk-io.c +++ b/fs/btrfs/disk-io.c @@ -2269,6 +2269,15 @@ int open_ctree(struct super_block *sb, goto fail_alloc; } + if ((btrfs_super_incompat_flags(disk_super) & + BTRFS_FEATURE_INCOMPAT_EXTENDED_IREF) && + btrfs_test_opt(tree_root, NOEXTIREF)) { + printk(KERN_ERR "BTRFS: couldn't mount because the extend iref" + "can not be close.\n"); + err = -EINVAL; + goto fail_alloc; + } + if (btrfs_super_leafsize(disk_super) != btrfs_super_nodesize(disk_super)) { printk(KERN_ERR "BTRFS: couldn't mount because metadata " diff --git a/fs/btrfs/inode-item.c b/fs/btrfs/inode-item.c index f07eb45..7c4f880 100644 --- a/fs/btrfs/inode-item.c +++ b/fs/btrfs/inode-item.c @@ -442,7 +442,7 @@ int btrfs_insert_inode_ref(struct btrfs_trans_handle *trans, out: btrfs_free_path(path); - if (ret == -EMLINK) { + if (ret == -EMLINK && !btrfs_test_opt(root, NOEXTIREF)) { /* * We ran out of space in the ref array. Need to add an * extended ref. diff --git a/fs/btrfs/super.c b/fs/btrfs/super.c index 0f03569..fd375b3 100644 --- a/fs/btrfs/super.c +++ b/fs/btrfs/super.c @@ -315,7 +315,7 @@ enum { Opt_nodatacow, Opt_max_inline, Opt_alloc_start, Opt_nobarrier, Opt_ssd, Opt_nossd, Opt_ssd_spread, Opt_thread_pool, Opt_noacl, Opt_compress, Opt_compress_type, Opt_compress_force, Opt_compress_force_type, - Opt_notreelog, Opt_ratio, Opt_flushoncommit, Opt_discard, + Opt_notreelog, Opt_noextiref, Opt_ratio, Opt_flushoncommit, Opt_discard, Opt_space_cache, Opt_clear_cache, Opt_user_subvol_rm_allowed, Opt_enospc_debug, Opt_subvolrootid, Opt_defrag, Opt_inode_cache, Opt_no_space_cache, Opt_recovery, Opt_skip_balance, @@ -344,6 +344,7 @@ static match_table_t tokens = { {Opt_nossd, "nossd"}, {Opt_noacl, "noacl"}, {Opt_notreelog, "notreelog"}, + {Opt_noextiref, "noextiref"}, {Opt_flushoncommit, "flushoncommit"}, {Opt_ratio, "metadata_ratio=%d"}, {Opt_discard, "discard"}, @@ -535,6 +536,10 @@ int btrfs_parse_options(struct btrfs_root *root, char *options) printk(KERN_INFO "btrfs: disabling tree log\n"); btrfs_set_opt(info->mount_opt, NOTREELOG); break; + case Opt_noextiref: + printk(KERN_INFO "btrfs: disabling extend inode ref\n"); + btrfs_set_opt(info->mount_opt, NOEXTIREF); + break; case Opt_flushoncommit: printk(KERN_INFO "btrfs: turning on flush-on-commit\n"); btrfs_set_opt(info->mount_opt, FLUSHONCOMMIT); @@ -1202,6 +1207,35 @@ static void btrfs_resize_thread_pool(struct btrfs_fs_info *fs_info, new_pool_size); } +static int btrfs_close_extend_iref(struct btrfs_fs_info *fs_info, + unsigned long old_opts) +{ + struct btrfs_trans_handle *trans; + int ret; + + if (btrfs_raw_test_opt(old_opts, NOEXTIREF) || + !btrfs_raw_test_opt(fs_info->mount_opt, NOEXTIREF)) + return 0; + + trans = btrfs_attach_transaction(fs_info->tree_root); + if (IS_ERR(trans)) { + if (PTR_ERR(trans) != -ENOENT) + return PTR_ERR(trans); + } else { + ret = btrfs_commit_transaction(trans, fs_info->tree_root); + if (ret) + return ret; + } + + if (btrfs_super_incompat_flags(fs_info->super_copy) & + BTRFS_FEATURE_INCOMPAT_EXTENDED_IREF) { + printk(KERN_ERR "BTRFS: could not close extend iref.\n"); + return -EINVAL; + } + + return 0; +} + static inline void btrfs_remount_prepare(struct btrfs_fs_info *fs_info) { set_bit(BTRFS_FS_STATE_REMOUNTING, &fs_info->fs_state); @@ -1259,6 +1293,11 @@ static int btrfs_remount(struct super_block *sb, int *flags, char *data) } btrfs_remount_begin(fs_info, old_opts, *flags); + + ret = btrfs_close_extend_iref(fs_info, old_opts); + if (ret) + goto restore; + btrfs_resize_thread_pool(fs_info, fs_info->thread_pool_size, old_thread_pool_size);
Now, we set incompat flag EXTEND_IREF when we actually need insert a extend inode reference, not when making a fs. But some users may hope that the fs still can be mounted on the old kernel, and don't hope we insert any extend inode references. So we introduce noextiref mount option to close this function. Signed-off-by: Miao Xie <miaox@cn.fujitsu.com> Cc: Mark Fasheh <mfasheh@suse.de> --- fs/btrfs/ctree.h | 1 + fs/btrfs/disk-io.c | 9 +++++++++ fs/btrfs/inode-item.c | 2 +- fs/btrfs/super.c | 41 ++++++++++++++++++++++++++++++++++++++++- 4 files changed, 51 insertions(+), 2 deletions(-)