Re: [PATCH 2/2] Btrfs: introduce noextiref mount option

[Jan Schmidt <list.btrfs@jan-o-sch.net>]

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