Btrfs devel - Apr 2013 - [PATCH 0/2] do not open the extend inode reference at the beginning

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

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

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

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

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