[PATCH 0/2] do not open the extend inode reference at the beginning

[PATCH 0/2] do not open the extend inode reference at the beginning

[Miao Xie]

In most cases, we do not insert so many inode references, so it is
better that we don't set incompat flag -- EXTEND_IREF -- when we
make a fs. Otherwise we can not mount the fs on the old kernel
though there is no extend iref in fact.

And some users may not hope to inserting the extend inode reference
because of the incompatible problem. In this case, we introduce a
mount option named noextiref.

Note, if the extend inode reference function is enabled, we will
fail to mount a fs with this option because there might be some
extend irefs in the fs, we should not close this function.

This patchset is against:
[PATCH 1/2] Btrfs: fix unblocked autodefraggers when remount
[PATCH 2/2] Btrfs: use a lock to protect incompat/compat flag of the super block

Miao Xie (2):
  Btrfs: set the INCOMPAT_EXTENDED_IREF when the extended iref is inserted
  Btrfs: introduce noextiref mount option

 fs/btrfs/ctree.h      |  1 +
 fs/btrfs/disk-io.c    |  9 +++++++++
 fs/btrfs/inode-item.c | 20 ++++++++++----------
 fs/btrfs/super.c      | 41 ++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 60 insertions(+), 11 deletions(-)

-- 
1.8.0.1

[PATCH 1/2] Btrfs: set the INCOMPAT_EXTENDED_IREF when the extended iref is inserted

[Miao Xie]

We needn't set the INCOMAT_EXTENDED_IREF when making a new fs, just
do it after we insert a  extended iref successfully. Otherwise, we
can not mount the fs in which there is no extended iref in fact on
the old kernel, it is not so flexible for the users.

Signed-off-by: Miao Xie <miaox@cn.fujitsu.com>
Cc: Mark Fasheh <mfasheh@suse.de>
---
 fs/btrfs/inode-item.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/fs/btrfs/inode-item.c b/fs/btrfs/inode-item.c
index 48b8fda..f07eb45 100644
--- a/fs/btrfs/inode-item.c
+++ b/fs/btrfs/inode-item.c
@@ -443,15 +443,15 @@ out:
 	btrfs_free_path(path);
 
 	if (ret == -EMLINK) {
-		struct btrfs_super_block *disk_super = root->fs_info->super_copy;
-		/* We ran out of space in the ref array. Need to
-		 * add an extended ref. */
-		if (btrfs_super_incompat_flags(disk_super)
-		    & BTRFS_FEATURE_INCOMPAT_EXTENDED_IREF)
-			ret = btrfs_insert_inode_extref(trans, root, name,
-							name_len,
-							inode_objectid,
-							ref_objectid, index);
+		/*
+		 * We ran out of space in the ref array. Need to add an
+		 * extended ref.
+		 */
+		ret = btrfs_insert_inode_extref(trans, root, name, name_len,
+						inode_objectid, ref_objectid,
+						index);
+		if (!ret)
+			btrfs_set_fs_incompat(root->fs_info, EXTENDED_IREF);
 	}
 
 	return ret;
-- 
1.8.0.1

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

[Miao Xie]

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

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);
 
-- 
1.8.0.1

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

[Jan Schmidt]

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

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

[Miao Xie]

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
> 

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

[Jan Schmidt]

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

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

[Eric Sandeen]

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

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

[Miao Xie]

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

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

[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, ...

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

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

[Wang Shilong]

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

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

[Mark Fasheh]

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

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

[David Sterba]

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

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

[Miao Xie]

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