[PATCH] Btrfs: avoid losing data raid profile when deleting a device

[PATCH] Btrfs: avoid losing data raid profile when deleting a device

[Liu Bo]

We've avoided data losing raid profile when doing balance, but it
turns out that deleting a device could also result in the same
problem.

This fixes the problem by creating an empty data chunk before
relocating the data chunk.

Metadata/System chunk are supposed to have non-zero bytes all the time
so their raid profile is persistent.

Reported-by: James Alandt <James.Alandt@wdc.com>
Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
 fs/btrfs/volumes.c | 87 ++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 68 insertions(+), 19 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 4a72c45..3f48bcd 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -144,6 +144,8 @@ static int __btrfs_map_block(struct btrfs_fs_info *fs_info,
 			     u64 logical, u64 *length,
 			     struct btrfs_bio **bbio_ret,
 			     int mirror_num, int need_raid_map);
+static int btrfs_may_alloc_data_chunk(struct btrfs_fs_info *fs_info,
+				      u64 chunk_offset);
 
 DEFINE_MUTEX(uuid_mutex);
 static LIST_HEAD(fs_uuids);
@@ -3476,7 +3478,6 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
 	u32 count_meta = 0;
 	u32 count_sys = 0;
 	int chunk_reserved = 0;
-	u64 bytes_used = 0;
 
 	/* step one make some room on all the devices */
 	devices = &fs_info->fs_devices->devices;
@@ -3635,28 +3636,22 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
 			goto loop;
 		}
 
-		ASSERT(fs_info->data_sinfo);
-		spin_lock(&fs_info->data_sinfo->lock);
-		bytes_used = fs_info->data_sinfo->bytes_used;
-		spin_unlock(&fs_info->data_sinfo->lock);
-
-		if ((chunk_type & BTRFS_BLOCK_GROUP_DATA) &&
-		    !chunk_reserved && !bytes_used) {
-			trans = btrfs_start_transaction(chunk_root, 0);
-			if (IS_ERR(trans)) {
-				mutex_unlock(&fs_info->delete_unused_bgs_mutex);
-				ret = PTR_ERR(trans);
-				goto error;
-			}
-
-			ret = btrfs_force_chunk_alloc(trans, fs_info,
-						      BTRFS_BLOCK_GROUP_DATA);
-			btrfs_end_transaction(trans);
+		if (!chunk_reserved) {
+			/*
+			 * We may be relocating the only data chunk we have,
+			 * which could potentially end up with losing data's
+			 * raid profile, so lets allocate an empty one in
+			 * advance.
+			 */
+			ret = btrfs_may_alloc_data_chunk(fs_info,
+							 found_key.offset);
 			if (ret < 0) {
 				mutex_unlock(&fs_info->delete_unused_bgs_mutex);
+				ret = PTR_ERR(trans);
 				goto error;
+			} else if (ret == 1) {
+				chunk_reserved = 1;
 			}
-			chunk_reserved = 1;
 		}
 
 		ret = btrfs_relocate_chunk(fs_info, found_key.offset);
@@ -4327,6 +4322,48 @@ int btrfs_check_uuid_tree(struct btrfs_fs_info *fs_info)
 }
 
 /*
+ * return 1 : allocate a data chunk successfully,
+ * return <0: errors during allocating a data chunk,
+ * return 0 : no need to allocate a data chunk.
+ */
+static int btrfs_may_alloc_data_chunk(struct btrfs_fs_info *fs_info,
+				      u64 chunk_offset)
+{
+	struct btrfs_block_group_cache *cache;
+	u64 bytes_used;
+	u64 chunk_type;
+
+	cache = btrfs_lookup_block_group(fs_info, chunk_offset);
+	ASSERT(cache);
+	chunk_type = cache->flags;
+	btrfs_put_block_group(cache);
+
+	if (chunk_type & BTRFS_BLOCK_GROUP_DATA) {
+		spin_lock(&fs_info->data_sinfo->lock);
+		bytes_used = fs_info->data_sinfo->bytes_used;
+		spin_unlock(&fs_info->data_sinfo->lock);
+
+		if (!bytes_used) {
+			struct btrfs_trans_handle *trans;
+			int ret;
+
+			trans =	btrfs_join_transaction(fs_info->tree_root);
+			if (IS_ERR(trans))
+				return PTR_ERR(trans);
+
+			ret = btrfs_force_chunk_alloc(trans, fs_info,
+						      BTRFS_BLOCK_GROUP_DATA);
+			btrfs_end_transaction(trans);
+			if (ret < 0)
+				return ret;
+
+			return 1;
+		}
+	}
+	return 0;
+}
+
+/*
  * shrinking a device means finding all of the device extents past
  * the new size, and then following the back refs to the chunks.
  * The chunk relocation code actually frees the device extent
@@ -4419,6 +4456,18 @@ int btrfs_shrink_device(struct btrfs_device *device, u64 new_size)
 		chunk_offset = btrfs_dev_extent_chunk_offset(l, dev_extent);
 		btrfs_release_path(path);
 
+		/*
+		 * We may be relocating the only data chunk we have,
+		 * which could potentially end up with losing data's
+		 * raid profile, so lets allocate an empty one in
+		 * advance.
+		 */
+		ret = btrfs_may_alloc_data_chunk(fs_info, chunk_offset);
+		if (ret < 0) {
+			mutex_unlock(&fs_info->delete_unused_bgs_mutex);
+			goto done;
+		}
+
 		ret = btrfs_relocate_chunk(fs_info, chunk_offset);
 		mutex_unlock(&fs_info->delete_unused_bgs_mutex);
 		if (ret && ret != -ENOSPC)
-- 
2.9.4

Re: [PATCH] Btrfs: avoid losing data raid profile when deleting a device

[Nikolay Borisov]

On  9.10.2017 21:01, Liu Bo wrote:
> We've avoided data losing raid profile when doing balance, but it
> turns out that deleting a device could also result in the same
> problem.
> 
> This fixes the problem by creating an empty data chunk before
> relocating the data chunk.
> 
> Metadata/System chunk are supposed to have non-zero bytes all the time
> so their raid profile is persistent.

This patch introduces new warning:

fs/btrfs/volumes.c:3523:29: note: ‘trans’ was declared here
  struct btrfs_trans_handle *trans;


> 
> Reported-by: James Alandt <James.Alandt@wdc.com>
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> ---
>  fs/btrfs/volumes.c | 87 ++++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 68 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 4a72c45..3f48bcd 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -144,6 +144,8 @@ static int __btrfs_map_block(struct btrfs_fs_info *fs_info,
>  			     u64 logical, u64 *length,
>  			     struct btrfs_bio **bbio_ret,
>  			     int mirror_num, int need_raid_map);
> +static int btrfs_may_alloc_data_chunk(struct btrfs_fs_info *fs_info,
> +				      u64 chunk_offset);

Also there is no need to have this forward declaration, the function can
just as well be put right before __btrfs_balance. Let's try and keep
changes minimal.

>  
>  DEFINE_MUTEX(uuid_mutex);
>  static LIST_HEAD(fs_uuids);
> @@ -3476,7 +3478,6 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
>  	u32 count_meta = 0;
>  	u32 count_sys = 0;
>  	int chunk_reserved = 0;
> -	u64 bytes_used = 0;
>  
>  	/* step one make some room on all the devices */
>  	devices = &fs_info->fs_devices->devices;
> @@ -3635,28 +3636,22 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
>  			goto loop;
>  		}
>  
> -		ASSERT(fs_info->data_sinfo);
> -		spin_lock(&fs_info->data_sinfo->lock);
> -		bytes_used = fs_info->data_sinfo->bytes_used;
> -		spin_unlock(&fs_info->data_sinfo->lock);
> -
> -		if ((chunk_type & BTRFS_BLOCK_GROUP_DATA) &&
> -		    !chunk_reserved && !bytes_used) {
> -			trans = btrfs_start_transaction(chunk_root, 0);
> -			if (IS_ERR(trans)) {
> -				mutex_unlock(&fs_info->delete_unused_bgs_mutex);
> -				ret = PTR_ERR(trans);
> -				goto error;
> -			}
> -
> -			ret = btrfs_force_chunk_alloc(trans, fs_info,
> -						      BTRFS_BLOCK_GROUP_DATA);
> -			btrfs_end_transaction(trans);
> +		if (!chunk_reserved) {
> +			/*
> +			 * We may be relocating the only data chunk we have,
> +			 * which could potentially end up with losing data's
> +			 * raid profile, so lets allocate an empty one in
> +			 * advance.
> +			 */
> +			ret = btrfs_may_alloc_data_chunk(fs_info,
> +							 found_key.offset);
>  			if (ret < 0) {
>  				mutex_unlock(&fs_info->delete_unused_bgs_mutex);
> +				ret = PTR_ERR(trans);
>  				goto error;
> +			} else if (ret == 1) {
> +				chunk_reserved = 1;
>  			}
> -			chunk_reserved = 1;
>  		}
>  
>  		ret = btrfs_relocate_chunk(fs_info, found_key.offset);
> @@ -4327,6 +4322,48 @@ int btrfs_check_uuid_tree(struct btrfs_fs_info *fs_info)
>  }
>  
>  /*
> + * return 1 : allocate a data chunk successfully,
> + * return <0: errors during allocating a data chunk,
> + * return 0 : no need to allocate a data chunk.
> + */
> +static int btrfs_may_alloc_data_chunk(struct btrfs_fs_info *fs_info,
> +				      u64 chunk_offset)
> +{
> +	struct btrfs_block_group_cache *cache;
> +	u64 bytes_used;
> +	u64 chunk_type;
> +
> +	cache = btrfs_lookup_block_group(fs_info, chunk_offset);
> +	ASSERT(cache);
> +	chunk_type = cache->flags;
> +	btrfs_put_block_group(cache);
> +
> +	if (chunk_type & BTRFS_BLOCK_GROUP_DATA) {
> +		spin_lock(&fs_info->data_sinfo->lock);
> +		bytes_used = fs_info->data_sinfo->bytes_used;
> +		spin_unlock(&fs_info->data_sinfo->lock);
> +
> +		if (!bytes_used) {
> +			struct btrfs_trans_handle *trans;
> +			int ret;
> +
> +			trans =	btrfs_join_transaction(fs_info->tree_root);
> +			if (IS_ERR(trans))
> +				return PTR_ERR(trans);
> +
> +			ret = btrfs_force_chunk_alloc(trans, fs_info,
> +						      BTRFS_BLOCK_GROUP_DATA);
> +			btrfs_end_transaction(trans);
> +			if (ret < 0)
> +				return ret;
> +
> +			return 1;
> +		}
> +	}
> +	return 0;
> +}
> +
> +/*
>   * shrinking a device means finding all of the device extents past
>   * the new size, and then following the back refs to the chunks.
>   * The chunk relocation code actually frees the device extent
> @@ -4419,6 +4456,18 @@ int btrfs_shrink_device(struct btrfs_device *device, u64 new_size)
>  		chunk_offset = btrfs_dev_extent_chunk_offset(l, dev_extent);
>  		btrfs_release_path(path);
>  
> +		/*
> +		 * We may be relocating the only data chunk we have,
> +		 * which could potentially end up with losing data's
> +		 * raid profile, so lets allocate an empty one in
> +		 * advance.
> +		 */
> +		ret = btrfs_may_alloc_data_chunk(fs_info, chunk_offset);
> +		if (ret < 0) {
> +			mutex_unlock(&fs_info->delete_unused_bgs_mutex);
> +			goto done;
> +		}
> +
>  		ret = btrfs_relocate_chunk(fs_info, chunk_offset);
>  		mutex_unlock(&fs_info->delete_unused_bgs_mutex);
>  		if (ret && ret != -ENOSPC)
> 

Re: [PATCH] Btrfs: avoid losing data raid profile when deleting a device

[Liu Bo]

On Tue, Oct 10, 2017 at 09:57:46AM +0300, Nikolay Borisov wrote:
> 
> 
> On  9.10.2017 21:01, Liu Bo wrote:
> > We've avoided data losing raid profile when doing balance, but it
> > turns out that deleting a device could also result in the same
> > problem.
> > 
> > This fixes the problem by creating an empty data chunk before
> > relocating the data chunk.
> > 
> > Metadata/System chunk are supposed to have non-zero bytes all the time
> > so their raid profile is persistent.
> 
> This patch introduces new warning:
> 
> fs/btrfs/volumes.c:3523:29: note: ‘trans’ was declared here
>   struct btrfs_trans_handle *trans;
>

Not sure how I missed this, thanks for pointing it out.

> 
> > 
> > Reported-by: James Alandt <James.Alandt@wdc.com>
> > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> > ---
> >  fs/btrfs/volumes.c | 87 ++++++++++++++++++++++++++++++++++++++++++------------
> >  1 file changed, 68 insertions(+), 19 deletions(-)
> > 
> > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > index 4a72c45..3f48bcd 100644
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -144,6 +144,8 @@ static int __btrfs_map_block(struct btrfs_fs_info *fs_info,
> >  			     u64 logical, u64 *length,
> >  			     struct btrfs_bio **bbio_ret,
> >  			     int mirror_num, int need_raid_map);
> > +static int btrfs_may_alloc_data_chunk(struct btrfs_fs_info *fs_info,
> > +				      u64 chunk_offset);
> 
> Also there is no need to have this forward declaration, the function can
> just as well be put right before __btrfs_balance. Let's try and keep
> changes minimal.
>

OK.

> >  
> >  DEFINE_MUTEX(uuid_mutex);
> >  static LIST_HEAD(fs_uuids);
> > @@ -3476,7 +3478,6 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
> >  	u32 count_meta = 0;
> >  	u32 count_sys = 0;
> >  	int chunk_reserved = 0;
> > -	u64 bytes_used = 0;
> >  
> >  	/* step one make some room on all the devices */
> >  	devices = &fs_info->fs_devices->devices;
> > @@ -3635,28 +3636,22 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
> >  			goto loop;
> >  		}
> >  
> > -		ASSERT(fs_info->data_sinfo);
> > -		spin_lock(&fs_info->data_sinfo->lock);
> > -		bytes_used = fs_info->data_sinfo->bytes_used;
> > -		spin_unlock(&fs_info->data_sinfo->lock);
> > -
> > -		if ((chunk_type & BTRFS_BLOCK_GROUP_DATA) &&
> > -		    !chunk_reserved && !bytes_used) {
> > -			trans = btrfs_start_transaction(chunk_root, 0);
> > -			if (IS_ERR(trans)) {
> > -				mutex_unlock(&fs_info->delete_unused_bgs_mutex);
> > -				ret = PTR_ERR(trans);
> > -				goto error;
> > -			}
> > -
> > -			ret = btrfs_force_chunk_alloc(trans, fs_info,
> > -						      BTRFS_BLOCK_GROUP_DATA);
> > -			btrfs_end_transaction(trans);
> > +		if (!chunk_reserved) {
> > +			/*
> > +			 * We may be relocating the only data chunk we have,
> > +			 * which could potentially end up with losing data's
> > +			 * raid profile, so lets allocate an empty one in
> > +			 * advance.
> > +			 */
> > +			ret = btrfs_may_alloc_data_chunk(fs_info,
> > +							 found_key.offset);
> >  			if (ret < 0) {
> >  				mutex_unlock(&fs_info->delete_unused_bgs_mutex);
> > +				ret = PTR_ERR(trans);

I'll remove this ret = PTR_ERR(trans);

-liubo

> >  				goto error;
> > +			} else if (ret == 1) {
> > +				chunk_reserved = 1;
> >  			}
> > -			chunk_reserved = 1;
> >  		}
> >  
> >  		ret = btrfs_relocate_chunk(fs_info, found_key.offset);
> > @@ -4327,6 +4322,48 @@ int btrfs_check_uuid_tree(struct btrfs_fs_info *fs_info)
> >  }
> >  
> >  /*
> > + * return 1 : allocate a data chunk successfully,
> > + * return <0: errors during allocating a data chunk,
> > + * return 0 : no need to allocate a data chunk.
> > + */
> > +static int btrfs_may_alloc_data_chunk(struct btrfs_fs_info *fs_info,
> > +				      u64 chunk_offset)
> > +{
> > +	struct btrfs_block_group_cache *cache;
> > +	u64 bytes_used;
> > +	u64 chunk_type;
> > +
> > +	cache = btrfs_lookup_block_group(fs_info, chunk_offset);
> > +	ASSERT(cache);
> > +	chunk_type = cache->flags;
> > +	btrfs_put_block_group(cache);
> > +
> > +	if (chunk_type & BTRFS_BLOCK_GROUP_DATA) {
> > +		spin_lock(&fs_info->data_sinfo->lock);
> > +		bytes_used = fs_info->data_sinfo->bytes_used;
> > +		spin_unlock(&fs_info->data_sinfo->lock);
> > +
> > +		if (!bytes_used) {
> > +			struct btrfs_trans_handle *trans;
> > +			int ret;
> > +
> > +			trans =	btrfs_join_transaction(fs_info->tree_root);
> > +			if (IS_ERR(trans))
> > +				return PTR_ERR(trans);
> > +
> > +			ret = btrfs_force_chunk_alloc(trans, fs_info,
> > +						      BTRFS_BLOCK_GROUP_DATA);
> > +			btrfs_end_transaction(trans);
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			return 1;
> > +		}
> > +	}
> > +	return 0;
> > +}
> > +
> > +/*
> >   * shrinking a device means finding all of the device extents past
> >   * the new size, and then following the back refs to the chunks.
> >   * The chunk relocation code actually frees the device extent
> > @@ -4419,6 +4456,18 @@ int btrfs_shrink_device(struct btrfs_device *device, u64 new_size)
> >  		chunk_offset = btrfs_dev_extent_chunk_offset(l, dev_extent);
> >  		btrfs_release_path(path);
> >  
> > +		/*
> > +		 * We may be relocating the only data chunk we have,
> > +		 * which could potentially end up with losing data's
> > +		 * raid profile, so lets allocate an empty one in
> > +		 * advance.
> > +		 */
> > +		ret = btrfs_may_alloc_data_chunk(fs_info, chunk_offset);
> > +		if (ret < 0) {
> > +			mutex_unlock(&fs_info->delete_unused_bgs_mutex);
> > +			goto done;
> > +		}
> > +
> >  		ret = btrfs_relocate_chunk(fs_info, chunk_offset);
> >  		mutex_unlock(&fs_info->delete_unused_bgs_mutex);
> >  		if (ret && ret != -ENOSPC)
> > 
> --
> 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

[PATCH v2] Btrfs: avoid losing data raid profile when deleting a device

[Liu Bo]

We've avoided data losing raid profile when doing balance, but it
turns out that deleting a device could also result in the same
problem.

This fixes the problem by creating an empty data chunk before
relocating the data chunk.

Metadata/System chunk are supposed to have non-zero bytes all the time
so their raid profile is persistent.

Reported-by: James Alandt <James.Alandt@wdc.com>
Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---

v2: - return the correct error.
    - move helper ahead of __btrfs_balance().

 fs/btrfs/volumes.c | 84 ++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 65 insertions(+), 19 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 4a72c45..a74396d 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -3018,6 +3018,48 @@ static int btrfs_relocate_sys_chunks(struct btrfs_fs_info *fs_info)
 	return ret;
 }
 
+/*
+ * return 1 : allocate a data chunk successfully,
+ * return <0: errors during allocating a data chunk,
+ * return 0 : no need to allocate a data chunk.
+ */
+static int btrfs_may_alloc_data_chunk(struct btrfs_fs_info *fs_info,
+				      u64 chunk_offset)
+{
+	struct btrfs_block_group_cache *cache;
+	u64 bytes_used;
+	u64 chunk_type;
+
+	cache = btrfs_lookup_block_group(fs_info, chunk_offset);
+	ASSERT(cache);
+	chunk_type = cache->flags;
+	btrfs_put_block_group(cache);
+
+	if (chunk_type & BTRFS_BLOCK_GROUP_DATA) {
+		spin_lock(&fs_info->data_sinfo->lock);
+		bytes_used = fs_info->data_sinfo->bytes_used;
+		spin_unlock(&fs_info->data_sinfo->lock);
+
+		if (!bytes_used) {
+			struct btrfs_trans_handle *trans;
+			int ret;
+
+			trans =	btrfs_join_transaction(fs_info->tree_root);
+			if (IS_ERR(trans))
+				return PTR_ERR(trans);
+
+			ret = btrfs_force_chunk_alloc(trans, fs_info,
+						      BTRFS_BLOCK_GROUP_DATA);
+			btrfs_end_transaction(trans);
+			if (ret < 0)
+				return ret;
+
+			return 1;
+		}
+	}
+	return 0;
+}
+
 static int insert_balance_item(struct btrfs_fs_info *fs_info,
 			       struct btrfs_balance_control *bctl)
 {
@@ -3476,7 +3518,6 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
 	u32 count_meta = 0;
 	u32 count_sys = 0;
 	int chunk_reserved = 0;
-	u64 bytes_used = 0;
 
 	/* step one make some room on all the devices */
 	devices = &fs_info->fs_devices->devices;
@@ -3635,28 +3676,21 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
 			goto loop;
 		}
 
-		ASSERT(fs_info->data_sinfo);
-		spin_lock(&fs_info->data_sinfo->lock);
-		bytes_used = fs_info->data_sinfo->bytes_used;
-		spin_unlock(&fs_info->data_sinfo->lock);
-
-		if ((chunk_type & BTRFS_BLOCK_GROUP_DATA) &&
-		    !chunk_reserved && !bytes_used) {
-			trans = btrfs_start_transaction(chunk_root, 0);
-			if (IS_ERR(trans)) {
-				mutex_unlock(&fs_info->delete_unused_bgs_mutex);
-				ret = PTR_ERR(trans);
-				goto error;
-			}
-
-			ret = btrfs_force_chunk_alloc(trans, fs_info,
-						      BTRFS_BLOCK_GROUP_DATA);
-			btrfs_end_transaction(trans);
+		if (!chunk_reserved) {
+			/*
+			 * We may be relocating the only data chunk we have,
+			 * which could potentially end up with losing data's
+			 * raid profile, so lets allocate an empty one in
+			 * advance.
+			 */
+			ret = btrfs_may_alloc_data_chunk(fs_info,
+							 found_key.offset);
 			if (ret < 0) {
 				mutex_unlock(&fs_info->delete_unused_bgs_mutex);
 				goto error;
+			} else if (ret == 1) {
+				chunk_reserved = 1;
 			}
-			chunk_reserved = 1;
 		}
 
 		ret = btrfs_relocate_chunk(fs_info, found_key.offset);
@@ -4419,6 +4453,18 @@ int btrfs_shrink_device(struct btrfs_device *device, u64 new_size)
 		chunk_offset = btrfs_dev_extent_chunk_offset(l, dev_extent);
 		btrfs_release_path(path);
 
+		/*
+		 * We may be relocating the only data chunk we have,
+		 * which could potentially end up with losing data's
+		 * raid profile, so lets allocate an empty one in
+		 * advance.
+		 */
+		ret = btrfs_may_alloc_data_chunk(fs_info, chunk_offset);
+		if (ret < 0) {
+			mutex_unlock(&fs_info->delete_unused_bgs_mutex);
+			goto done;
+		}
+
 		ret = btrfs_relocate_chunk(fs_info, chunk_offset);
 		mutex_unlock(&fs_info->delete_unused_bgs_mutex);
 		if (ret && ret != -ENOSPC)
-- 
2.9.4

Re: [PATCH v2] Btrfs: avoid losing data raid profile when deleting a device

[Nikolay Borisov]

On 10.10.2017 20:53, Liu Bo wrote:
> We've avoided data losing raid profile when doing balance, but it
> turns out that deleting a device could also result in the same
> problem
> 
> This fixes the problem by creating an empty data chunk before
> relocating the data chunk.

Why is this needed - copy the metadata of the to-be-relocated chunk into
the newly created empty chunk? I don't entirely understand that code but
doesn't this seem a bit like a hack in order to stash some information?
Perhaps you could elaborate the logic a bit more in the changelog?

> 
> Metadata/System chunk are supposed to have non-zero bytes all the time
> so their raid profile is persistent.

I think this changelog is a bit scarce on detail as to the culprit of
the problem. Could you perhaps put a sentence or two what the underlying
logic which deletes the raid profile if a chunk is empty ?

> 
> Reported-by: James Alandt <James.Alandt@wdc.com>
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> ---
> 
> v2: - return the correct error.
>     - move helper ahead of __btrfs_balance().
> 
>  fs/btrfs/volumes.c | 84 ++++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 65 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 4a72c45..a74396d 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -3018,6 +3018,48 @@ static int btrfs_relocate_sys_chunks(struct btrfs_fs_info *fs_info)
>  	return ret;
>  }
>  
> +/*
> + * return 1 : allocate a data chunk successfully,
> + * return <0: errors during allocating a data chunk,
> + * return 0 : no need to allocate a data chunk.
> + */
> +static int btrfs_may_alloc_data_chunk(struct btrfs_fs_info *fs_info,
> +				      u64 chunk_offset)
> +{
> +	struct btrfs_block_group_cache *cache;
> +	u64 bytes_used;
> +	u64 chunk_type;
> +
> +	cache = btrfs_lookup_block_group(fs_info, chunk_offset);
> +	ASSERT(cache);
> +	chunk_type = cache->flags;
> +	btrfs_put_block_group(cache);
> +
> +	if (chunk_type & BTRFS_BLOCK_GROUP_DATA) {
> +		spin_lock(&fs_info->data_sinfo->lock);
> +		bytes_used = fs_info->data_sinfo->bytes_used;
> +		spin_unlock(&fs_info->data_sinfo->lock);
> +
> +		if (!bytes_used) {
> +			struct btrfs_trans_handle *trans;
> +			int ret;
> +
> +			trans =	btrfs_join_transaction(fs_info->tree_root);
> +			if (IS_ERR(trans))
> +				return PTR_ERR(trans);
> +
> +			ret = btrfs_force_chunk_alloc(trans, fs_info,
> +						      BTRFS_BLOCK_GROUP_DATA);
> +			btrfs_end_transaction(trans);
> +			if (ret < 0)
> +				return ret;
> +
> +			return 1;
> +		}
> +	}
> +	return 0;
> +}
> +
>  static int insert_balance_item(struct btrfs_fs_info *fs_info,
>  			       struct btrfs_balance_control *bctl)
>  {
> @@ -3476,7 +3518,6 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
>  	u32 count_meta = 0;
>  	u32 count_sys = 0;
>  	int chunk_reserved = 0;
> -	u64 bytes_used = 0;
>  
>  	/* step one make some room on all the devices */
>  	devices = &fs_info->fs_devices->devices;
> @@ -3635,28 +3676,21 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
>  			goto loop;
>  		}
>  
> -		ASSERT(fs_info->data_sinfo);
> -		spin_lock(&fs_info->data_sinfo->lock);
> -		bytes_used = fs_info->data_sinfo->bytes_used;
> -		spin_unlock(&fs_info->data_sinfo->lock);
> -
> -		if ((chunk_type & BTRFS_BLOCK_GROUP_DATA) &&
> -		    !chunk_reserved && !bytes_used) {
> -			trans = btrfs_start_transaction(chunk_root, 0);
> -			if (IS_ERR(trans)) {
> -				mutex_unlock(&fs_info->delete_unused_bgs_mutex);
> -				ret = PTR_ERR(trans);
> -				goto error;
> -			}
> -
> -			ret = btrfs_force_chunk_alloc(trans, fs_info,
> -						      BTRFS_BLOCK_GROUP_DATA);
> -			btrfs_end_transaction(trans);
> +		if (!chunk_reserved) {
> +			/*
> +			 * We may be relocating the only data chunk we have,
> +			 * which could potentially end up with losing data's
> +			 * raid profile, so lets allocate an empty one in
> +			 * advance.
> +			 */
> +			ret = btrfs_may_alloc_data_chunk(fs_info,
> +							 found_key.offset);
>  			if (ret < 0) {
>  				mutex_unlock(&fs_info->delete_unused_bgs_mutex);
>  				goto error;
> +			} else if (ret == 1) {
> +				chunk_reserved = 1;
>  			}
> -			chunk_reserved = 1;
>  		}
>  
>  		ret = btrfs_relocate_chunk(fs_info, found_key.offset);
> @@ -4419,6 +4453,18 @@ int btrfs_shrink_device(struct btrfs_device *device, u64 new_size)
>  		chunk_offset = btrfs_dev_extent_chunk_offset(l, dev_extent);
>  		btrfs_release_path(path);
>  
> +		/*
> +		 * We may be relocating the only data chunk we have,
> +		 * which could potentially end up with losing data's
> +		 * raid profile, so lets allocate an empty one in
> +		 * advance.
> +		 */
> +		ret = btrfs_may_alloc_data_chunk(fs_info, chunk_offset);
> +		if (ret < 0) {
> +			mutex_unlock(&fs_info->delete_unused_bgs_mutex);
> +			goto done;
> +		}
> +
>  		ret = btrfs_relocate_chunk(fs_info, chunk_offset);
>  		mutex_unlock(&fs_info->delete_unused_bgs_mutex);
>  		if (ret && ret != -ENOSPC)
> 

Re: [PATCH v2] Btrfs: avoid losing data raid profile when deleting a device

[Liu Bo]

On Wed, Oct 11, 2017 at 10:38:50AM +0300, Nikolay Borisov wrote:
> 
> 
> On 10.10.2017 20:53, Liu Bo wrote:
> > We've avoided data losing raid profile when doing balance, but it
> > turns out that deleting a device could also result in the same
> > problem
> > 
> > This fixes the problem by creating an empty data chunk before
> > relocating the data chunk.
> 
> Why is this needed - copy the metadata of the to-be-relocated chunk into
> the newly created empty chunk? I don't entirely understand that code but
> doesn't this seem a bit like a hack in order to stash some information?
> Perhaps you could elaborate the logic a bit more in the changelog?
> 
> > 
> > Metadata/System chunk are supposed to have non-zero bytes all the time
> > so their raid profile is persistent.
> 
> I think this changelog is a bit scarce on detail as to the culprit of
> the problem. Could you perhaps put a sentence or two what the underlying
> logic which deletes the raid profile if a chunk is empty ?
>

Fair enough.

The problem is as same as what commit 2c9fe8355258 ("btrfs: Fix
lost-data-profile caused by balance bg") had fixed.

Similar to doing balance, deleting a device can also move all chunks
on this disk to other available disks, after 'move' successfully,
it'll remove those chunks.

If our last data chunk is empty and part of it happens to be on this
disk, then there is no data chunk in this btrfs after deleting the
device successfully, any following write will try to create a new data
chunk which ends up with a single data chunk because the only
available data raid profile is 'single'.

thanks,
-liubo

> > 
> > Reported-by: James Alandt <James.Alandt@wdc.com>
> > Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> > ---
> > 
> > v2: - return the correct error.
> >     - move helper ahead of __btrfs_balance().
> > 
> >  fs/btrfs/volumes.c | 84 ++++++++++++++++++++++++++++++++++++++++++------------
> >  1 file changed, 65 insertions(+), 19 deletions(-)
> > 
> > diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> > index 4a72c45..a74396d 100644
> > --- a/fs/btrfs/volumes.c
> > +++ b/fs/btrfs/volumes.c
> > @@ -3018,6 +3018,48 @@ static int btrfs_relocate_sys_chunks(struct btrfs_fs_info *fs_info)
> >  	return ret;
> >  }
> >  
> > +/*
> > + * return 1 : allocate a data chunk successfully,
> > + * return <0: errors during allocating a data chunk,
> > + * return 0 : no need to allocate a data chunk.
> > + */
> > +static int btrfs_may_alloc_data_chunk(struct btrfs_fs_info *fs_info,
> > +				      u64 chunk_offset)
> > +{
> > +	struct btrfs_block_group_cache *cache;
> > +	u64 bytes_used;
> > +	u64 chunk_type;
> > +
> > +	cache = btrfs_lookup_block_group(fs_info, chunk_offset);
> > +	ASSERT(cache);
> > +	chunk_type = cache->flags;
> > +	btrfs_put_block_group(cache);
> > +
> > +	if (chunk_type & BTRFS_BLOCK_GROUP_DATA) {
> > +		spin_lock(&fs_info->data_sinfo->lock);
> > +		bytes_used = fs_info->data_sinfo->bytes_used;
> > +		spin_unlock(&fs_info->data_sinfo->lock);
> > +
> > +		if (!bytes_used) {
> > +			struct btrfs_trans_handle *trans;
> > +			int ret;
> > +
> > +			trans =	btrfs_join_transaction(fs_info->tree_root);
> > +			if (IS_ERR(trans))
> > +				return PTR_ERR(trans);
> > +
> > +			ret = btrfs_force_chunk_alloc(trans, fs_info,
> > +						      BTRFS_BLOCK_GROUP_DATA);
> > +			btrfs_end_transaction(trans);
> > +			if (ret < 0)
> > +				return ret;
> > +
> > +			return 1;
> > +		}
> > +	}
> > +	return 0;
> > +}
> > +
> >  static int insert_balance_item(struct btrfs_fs_info *fs_info,
> >  			       struct btrfs_balance_control *bctl)
> >  {
> > @@ -3476,7 +3518,6 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
> >  	u32 count_meta = 0;
> >  	u32 count_sys = 0;
> >  	int chunk_reserved = 0;
> > -	u64 bytes_used = 0;
> >  
> >  	/* step one make some room on all the devices */
> >  	devices = &fs_info->fs_devices->devices;
> > @@ -3635,28 +3676,21 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
> >  			goto loop;
> >  		}
> >  
> > -		ASSERT(fs_info->data_sinfo);
> > -		spin_lock(&fs_info->data_sinfo->lock);
> > -		bytes_used = fs_info->data_sinfo->bytes_used;
> > -		spin_unlock(&fs_info->data_sinfo->lock);
> > -
> > -		if ((chunk_type & BTRFS_BLOCK_GROUP_DATA) &&
> > -		    !chunk_reserved && !bytes_used) {
> > -			trans = btrfs_start_transaction(chunk_root, 0);
> > -			if (IS_ERR(trans)) {
> > -				mutex_unlock(&fs_info->delete_unused_bgs_mutex);
> > -				ret = PTR_ERR(trans);
> > -				goto error;
> > -			}
> > -
> > -			ret = btrfs_force_chunk_alloc(trans, fs_info,
> > -						      BTRFS_BLOCK_GROUP_DATA);
> > -			btrfs_end_transaction(trans);
> > +		if (!chunk_reserved) {
> > +			/*
> > +			 * We may be relocating the only data chunk we have,
> > +			 * which could potentially end up with losing data's
> > +			 * raid profile, so lets allocate an empty one in
> > +			 * advance.
> > +			 */
> > +			ret = btrfs_may_alloc_data_chunk(fs_info,
> > +							 found_key.offset);
> >  			if (ret < 0) {
> >  				mutex_unlock(&fs_info->delete_unused_bgs_mutex);
> >  				goto error;
> > +			} else if (ret == 1) {
> > +				chunk_reserved = 1;
> >  			}
> > -			chunk_reserved = 1;
> >  		}
> >  
> >  		ret = btrfs_relocate_chunk(fs_info, found_key.offset);
> > @@ -4419,6 +4453,18 @@ int btrfs_shrink_device(struct btrfs_device *device, u64 new_size)
> >  		chunk_offset = btrfs_dev_extent_chunk_offset(l, dev_extent);
> >  		btrfs_release_path(path);
> >  
> > +		/*
> > +		 * We may be relocating the only data chunk we have,
> > +		 * which could potentially end up with losing data's
> > +		 * raid profile, so lets allocate an empty one in
> > +		 * advance.
> > +		 */
> > +		ret = btrfs_may_alloc_data_chunk(fs_info, chunk_offset);
> > +		if (ret < 0) {
> > +			mutex_unlock(&fs_info->delete_unused_bgs_mutex);
> > +			goto done;
> > +		}
> > +
> >  		ret = btrfs_relocate_chunk(fs_info, chunk_offset);
> >  		mutex_unlock(&fs_info->delete_unused_bgs_mutex);
> >  		if (ret && ret != -ENOSPC)
> > 

Re: [PATCH v2] Btrfs: avoid losing data raid profile when deleting a device

[Anand Jain]

On 10/14/2017 04:51 AM, Liu Bo wrote:
> On Wed, Oct 11, 2017 at 10:38:50AM +0300, Nikolay Borisov wrote:
>>
>>
>> On 10.10.2017 20:53, Liu Bo wrote:
>>> We've avoided data losing raid profile when doing balance, but it
>>> turns out that deleting a device could also result in the same
>>> problem
>>>
>>> This fixes the problem by creating an empty data chunk before
>>> relocating the data chunk.
>>
>> Why is this needed - copy the metadata of the to-be-relocated chunk into
>> the newly created empty chunk? I don't entirely understand that code but
>> doesn't this seem a bit like a hack in order to stash some information?
>> Perhaps you could elaborate the logic a bit more in the changelog?
>>
>>>
>>> Metadata/System chunk are supposed to have non-zero bytes all the time
>>> so their raid profile is persistent.
>>
>> I think this changelog is a bit scarce on detail as to the culprit of
>> the problem. Could you perhaps put a sentence or two what the underlying
>> logic which deletes the raid profile if a chunk is empty ?
>>
> 
> Fair enough.
> 
> The problem is as same as what commit 2c9fe8355258 ("btrfs: Fix
> lost-data-profile caused by balance bg") had fixed.
> 
> Similar to doing balance, deleting a device can also move all chunks
> on this disk to other available disks, after 'move' successfully,
> it'll remove those chunks.
> 
> If our last data chunk is empty and part of it happens to be on this
> disk, then there is no data chunk in this btrfs after deleting the
> device successfully, any following write will try to create a new data
> chunk which ends up with a single data chunk because the only
> available data raid profile is 'single'.

  So you are referring to a raid1 group profile which contains 3 or more
  devices otherwise single group file is what it will fit ? Is there
  reproducer ?

Thanks, Anand


> thanks,
> -liubo
> 
>>>
>>> Reported-by: James Alandt <James.Alandt@wdc.com>
>>> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
>>> ---
>>>
>>> v2: - return the correct error.
>>>      - move helper ahead of __btrfs_balance().
>>>
>>>   fs/btrfs/volumes.c | 84 ++++++++++++++++++++++++++++++++++++++++++------------
>>>   1 file changed, 65 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>> index 4a72c45..a74396d 100644
>>> --- a/fs/btrfs/volumes.c
>>> +++ b/fs/btrfs/volumes.c
>>> @@ -3018,6 +3018,48 @@ static int btrfs_relocate_sys_chunks(struct btrfs_fs_info *fs_info)
>>>   	return ret;
>>>   }
>>>   
>>> +/*
>>> + * return 1 : allocate a data chunk successfully,
>>> + * return <0: errors during allocating a data chunk,
>>> + * return 0 : no need to allocate a data chunk.
>>> + */
>>> +static int btrfs_may_alloc_data_chunk(struct btrfs_fs_info *fs_info,
>>> +				      u64 chunk_offset)
>>> +{
>>> +	struct btrfs_block_group_cache *cache;
>>> +	u64 bytes_used;
>>> +	u64 chunk_type;
>>> +
>>> +	cache = btrfs_lookup_block_group(fs_info, chunk_offset);
>>> +	ASSERT(cache);
>>> +	chunk_type = cache->flags;
>>> +	btrfs_put_block_group(cache);
>>> +
>>> +	if (chunk_type & BTRFS_BLOCK_GROUP_DATA) {
>>> +		spin_lock(&fs_info->data_sinfo->lock);
>>> +		bytes_used = fs_info->data_sinfo->bytes_used;
>>> +		spin_unlock(&fs_info->data_sinfo->lock);
>>> +
>>> +		if (!bytes_used) {
>>> +			struct btrfs_trans_handle *trans;
>>> +			int ret;
>>> +
>>> +			trans =	btrfs_join_transaction(fs_info->tree_root);
>>> +			if (IS_ERR(trans))
>>> +				return PTR_ERR(trans);
>>> +
>>> +			ret = btrfs_force_chunk_alloc(trans, fs_info,
>>> +						      BTRFS_BLOCK_GROUP_DATA);
>>> +			btrfs_end_transaction(trans);
>>> +			if (ret < 0)
>>> +				return ret;
>>> +
>>> +			return 1;
>>> +		}
>>> +	}
>>> +	return 0;
>>> +}
>>> +
>>>   static int insert_balance_item(struct btrfs_fs_info *fs_info,
>>>   			       struct btrfs_balance_control *bctl)
>>>   {
>>> @@ -3476,7 +3518,6 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
>>>   	u32 count_meta = 0;
>>>   	u32 count_sys = 0;
>>>   	int chunk_reserved = 0;
>>> -	u64 bytes_used = 0;
>>>   
>>>   	/* step one make some room on all the devices */
>>>   	devices = &fs_info->fs_devices->devices;
>>> @@ -3635,28 +3676,21 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
>>>   			goto loop;
>>>   		}
>>>   
>>> -		ASSERT(fs_info->data_sinfo);
>>> -		spin_lock(&fs_info->data_sinfo->lock);
>>> -		bytes_used = fs_info->data_sinfo->bytes_used;
>>> -		spin_unlock(&fs_info->data_sinfo->lock);
>>> -
>>> -		if ((chunk_type & BTRFS_BLOCK_GROUP_DATA) &&
>>> -		    !chunk_reserved && !bytes_used) {
>>> -			trans = btrfs_start_transaction(chunk_root, 0);
>>> -			if (IS_ERR(trans)) {
>>> -				mutex_unlock(&fs_info->delete_unused_bgs_mutex);
>>> -				ret = PTR_ERR(trans);
>>> -				goto error;
>>> -			}
>>> -
>>> -			ret = btrfs_force_chunk_alloc(trans, fs_info,
>>> -						      BTRFS_BLOCK_GROUP_DATA);
>>> -			btrfs_end_transaction(trans);
>>> +		if (!chunk_reserved) {
>>> +			/*
>>> +			 * We may be relocating the only data chunk we have,
>>> +			 * which could potentially end up with losing data's
>>> +			 * raid profile, so lets allocate an empty one in
>>> +			 * advance.
>>> +			 */
>>> +			ret = btrfs_may_alloc_data_chunk(fs_info,
>>> +							 found_key.offset);
>>>   			if (ret < 0) {
>>>   				mutex_unlock(&fs_info->delete_unused_bgs_mutex);
>>>   				goto error;
>>> +			} else if (ret == 1) {
>>> +				chunk_reserved = 1;
>>>   			}
>>> -			chunk_reserved = 1;
>>>   		}
>>>   
>>>   		ret = btrfs_relocate_chunk(fs_info, found_key.offset);
>>> @@ -4419,6 +4453,18 @@ int btrfs_shrink_device(struct btrfs_device *device, u64 new_size)
>>>   		chunk_offset = btrfs_dev_extent_chunk_offset(l, dev_extent);
>>>   		btrfs_release_path(path);
>>>   
>>> +		/*
>>> +		 * We may be relocating the only data chunk we have,
>>> +		 * which could potentially end up with losing data's
>>> +		 * raid profile, so lets allocate an empty one in
>>> +		 * advance.
>>> +		 */
>>> +		ret = btrfs_may_alloc_data_chunk(fs_info, chunk_offset);
>>> +		if (ret < 0) {
>>> +			mutex_unlock(&fs_info->delete_unused_bgs_mutex);
>>> +			goto done;
>>> +		}
>>> +
>>>   		ret = btrfs_relocate_chunk(fs_info, chunk_offset);
>>>   		mutex_unlock(&fs_info->delete_unused_bgs_mutex);
>>>   		if (ret && ret != -ENOSPC)
>>>
> --
> 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 v2] Btrfs: avoid losing data raid profile when deleting a device

[Nikolay Borisov]

On 13.10.2017 23:51, Liu Bo wrote:
> On Wed, Oct 11, 2017 at 10:38:50AM +0300, Nikolay Borisov wrote:
>>
>>
>> On 10.10.2017 20:53, Liu Bo wrote:
>>> We've avoided data losing raid profile when doing balance, but it
>>> turns out that deleting a device could also result in the same
>>> problem
>>>
>>> This fixes the problem by creating an empty data chunk before
>>> relocating the data chunk.
>>
>> Why is this needed - copy the metadata of the to-be-relocated chunk into
>> the newly created empty chunk? I don't entirely understand that code but
>> doesn't this seem a bit like a hack in order to stash some information?
>> Perhaps you could elaborate the logic a bit more in the changelog?
>>
>>>
>>> Metadata/System chunk are supposed to have non-zero bytes all the time
>>> so their raid profile is persistent.
>>
>> I think this changelog is a bit scarce on detail as to the culprit of
>> the problem. Could you perhaps put a sentence or two what the underlying
>> logic which deletes the raid profile if a chunk is empty ?
>>
> 
> Fair enough.
> 
> The problem is as same as what commit 2c9fe8355258 ("btrfs: Fix
> lost-data-profile caused by balance bg") had fixed.
> 
> Similar to doing balance, deleting a device can also move all chunks
> on this disk to other available disks, after 'move' successfully,
> it'll remove those chunks.
> 
> If our last data chunk is empty and part of it happens to be on this
          ^
"Last data chunk" of which disk - the "to-be-removed" one or the disk to
which chunks have been relocated?

> disk, then there is no data chunk in this btrfs after deleting the
   ^
Which disk are you referring to - the one being removed?
> device successfully, any following write will try to create a new data
> chunk which ends up with a single data chunk because the only
> available data raid profile is 'single'.
> 
> thanks,
> -liubo
> 
>>>
>>> Reported-by: James Alandt <James.Alandt@wdc.com>
>>> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
>>> ---
>>>
>>> v2: - return the correct error.
>>>     - move helper ahead of __btrfs_balance().
>>>
>>>  fs/btrfs/volumes.c | 84 ++++++++++++++++++++++++++++++++++++++++++------------
>>>  1 file changed, 65 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>> index 4a72c45..a74396d 100644
>>> --- a/fs/btrfs/volumes.c
>>> +++ b/fs/btrfs/volumes.c
>>> @@ -3018,6 +3018,48 @@ static int btrfs_relocate_sys_chunks(struct btrfs_fs_info *fs_info)
>>>  	return ret;
>>>  }
>>>  
>>> +/*
>>> + * return 1 : allocate a data chunk successfully,
>>> + * return <0: errors during allocating a data chunk,
>>> + * return 0 : no need to allocate a data chunk.
>>> + */
>>> +static int btrfs_may_alloc_data_chunk(struct btrfs_fs_info *fs_info,
>>> +				      u64 chunk_offset)
>>> +{
>>> +	struct btrfs_block_group_cache *cache;
>>> +	u64 bytes_used;
>>> +	u64 chunk_type;
>>> +
>>> +	cache = btrfs_lookup_block_group(fs_info, chunk_offset);
>>> +	ASSERT(cache);
>>> +	chunk_type = cache->flags;
>>> +	btrfs_put_block_group(cache);
>>> +
>>> +	if (chunk_type & BTRFS_BLOCK_GROUP_DATA) {
>>> +		spin_lock(&fs_info->data_sinfo->lock);
>>> +		bytes_used = fs_info->data_sinfo->bytes_used;
>>> +		spin_unlock(&fs_info->data_sinfo->lock);
>>> +
>>> +		if (!bytes_used) {
>>> +			struct btrfs_trans_handle *trans;
>>> +			int ret;
>>> +
>>> +			trans =	btrfs_join_transaction(fs_info->tree_root);
>>> +			if (IS_ERR(trans))
>>> +				return PTR_ERR(trans);
>>> +
>>> +			ret = btrfs_force_chunk_alloc(trans, fs_info,
>>> +						      BTRFS_BLOCK_GROUP_DATA);
>>> +			btrfs_end_transaction(trans);
>>> +			if (ret < 0)
>>> +				return ret;
>>> +
>>> +			return 1;
>>> +		}
>>> +	}
>>> +	return 0;
>>> +}
>>> +
>>>  static int insert_balance_item(struct btrfs_fs_info *fs_info,
>>>  			       struct btrfs_balance_control *bctl)
>>>  {
>>> @@ -3476,7 +3518,6 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
>>>  	u32 count_meta = 0;
>>>  	u32 count_sys = 0;
>>>  	int chunk_reserved = 0;
>>> -	u64 bytes_used = 0;
>>>  
>>>  	/* step one make some room on all the devices */
>>>  	devices = &fs_info->fs_devices->devices;
>>> @@ -3635,28 +3676,21 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
>>>  			goto loop;
>>>  		}
>>>  
>>> -		ASSERT(fs_info->data_sinfo);
>>> -		spin_lock(&fs_info->data_sinfo->lock);
>>> -		bytes_used = fs_info->data_sinfo->bytes_used;
>>> -		spin_unlock(&fs_info->data_sinfo->lock);
>>> -
>>> -		if ((chunk_type & BTRFS_BLOCK_GROUP_DATA) &&
>>> -		    !chunk_reserved && !bytes_used) {
>>> -			trans = btrfs_start_transaction(chunk_root, 0);
>>> -			if (IS_ERR(trans)) {
>>> -				mutex_unlock(&fs_info->delete_unused_bgs_mutex);
>>> -				ret = PTR_ERR(trans);
>>> -				goto error;
>>> -			}
>>> -
>>> -			ret = btrfs_force_chunk_alloc(trans, fs_info,
>>> -						      BTRFS_BLOCK_GROUP_DATA);
>>> -			btrfs_end_transaction(trans);
>>> +		if (!chunk_reserved) {
>>> +			/*
>>> +			 * We may be relocating the only data chunk we have,
>>> +			 * which could potentially end up with losing data's
>>> +			 * raid profile, so lets allocate an empty one in
>>> +			 * advance.
>>> +			 */
>>> +			ret = btrfs_may_alloc_data_chunk(fs_info,
>>> +							 found_key.offset);
>>>  			if (ret < 0) {
>>>  				mutex_unlock(&fs_info->delete_unused_bgs_mutex);
>>>  				goto error;
>>> +			} else if (ret == 1) {
>>> +				chunk_reserved = 1;
>>>  			}
>>> -			chunk_reserved = 1;
>>>  		}
>>>  
>>>  		ret = btrfs_relocate_chunk(fs_info, found_key.offset);
>>> @@ -4419,6 +4453,18 @@ int btrfs_shrink_device(struct btrfs_device *device, u64 new_size)
>>>  		chunk_offset = btrfs_dev_extent_chunk_offset(l, dev_extent);
>>>  		btrfs_release_path(path);
>>>  
>>> +		/*
>>> +		 * We may be relocating the only data chunk we have,
>>> +		 * which could potentially end up with losing data's
>>> +		 * raid profile, so lets allocate an empty one in
>>> +		 * advance.
>>> +		 */
>>> +		ret = btrfs_may_alloc_data_chunk(fs_info, chunk_offset);
>>> +		if (ret < 0) {
>>> +			mutex_unlock(&fs_info->delete_unused_bgs_mutex);
>>> +			goto done;
>>> +		}
>>> +
>>>  		ret = btrfs_relocate_chunk(fs_info, chunk_offset);
>>>  		mutex_unlock(&fs_info->delete_unused_bgs_mutex);
>>>  		if (ret && ret != -ENOSPC)
>>>
> 

Re: [PATCH v2] Btrfs: avoid losing data raid profile when deleting a device

[Liu Bo]

On Mon, Oct 16, 2017 at 12:22:44PM +0800, Anand Jain wrote:
> 
> 
> On 10/14/2017 04:51 AM, Liu Bo wrote:
> >On Wed, Oct 11, 2017 at 10:38:50AM +0300, Nikolay Borisov wrote:
> >>
> >>
> >>On 10.10.2017 20:53, Liu Bo wrote:
> >>>We've avoided data losing raid profile when doing balance, but it
> >>>turns out that deleting a device could also result in the same
> >>>problem
> >>>
> >>>This fixes the problem by creating an empty data chunk before
> >>>relocating the data chunk.
> >>
> >>Why is this needed - copy the metadata of the to-be-relocated chunk into
> >>the newly created empty chunk? I don't entirely understand that code but
> >>doesn't this seem a bit like a hack in order to stash some information?
> >>Perhaps you could elaborate the logic a bit more in the changelog?
> >>
> >>>
> >>>Metadata/System chunk are supposed to have non-zero bytes all the time
> >>>so their raid profile is persistent.
> >>
> >>I think this changelog is a bit scarce on detail as to the culprit of
> >>the problem. Could you perhaps put a sentence or two what the underlying
> >>logic which deletes the raid profile if a chunk is empty ?
> >>
> >
> >Fair enough.
> >
> >The problem is as same as what commit 2c9fe8355258 ("btrfs: Fix
> >lost-data-profile caused by balance bg") had fixed.
> >
> >Similar to doing balance, deleting a device can also move all chunks
> >on this disk to other available disks, after 'move' successfully,
> >it'll remove those chunks.
> >
> >If our last data chunk is empty and part of it happens to be on this
> >disk, then there is no data chunk in this btrfs after deleting the
> >device successfully, any following write will try to create a new data
> >chunk which ends up with a single data chunk because the only
> >available data raid profile is 'single'.
> 
>  So you are referring to a raid1 group profile which contains 3 or more
>  devices otherwise single group file is what it will fit ? Is there
>  reproducer ?

[PATCH] Fstest: btrfs/151: test if device delete ends up with losing raid profile

Thanks,

-liubo
> 
> Thanks, Anand
> 
> 
> >thanks,
> >-liubo
> >
> >>>
> >>>Reported-by: James Alandt <James.Alandt@wdc.com>
> >>>Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> >>>---
> >>>
> >>>v2: - return the correct error.
> >>>     - move helper ahead of __btrfs_balance().
> >>>
> >>>  fs/btrfs/volumes.c | 84 ++++++++++++++++++++++++++++++++++++++++++------------
> >>>  1 file changed, 65 insertions(+), 19 deletions(-)
> >>>
> >>>diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> >>>index 4a72c45..a74396d 100644
> >>>--- a/fs/btrfs/volumes.c
> >>>+++ b/fs/btrfs/volumes.c
> >>>@@ -3018,6 +3018,48 @@ static int btrfs_relocate_sys_chunks(struct btrfs_fs_info *fs_info)
> >>>  	return ret;
> >>>  }
> >>>+/*
> >>>+ * return 1 : allocate a data chunk successfully,
> >>>+ * return <0: errors during allocating a data chunk,
> >>>+ * return 0 : no need to allocate a data chunk.
> >>>+ */
> >>>+static int btrfs_may_alloc_data_chunk(struct btrfs_fs_info *fs_info,
> >>>+				      u64 chunk_offset)
> >>>+{
> >>>+	struct btrfs_block_group_cache *cache;
> >>>+	u64 bytes_used;
> >>>+	u64 chunk_type;
> >>>+
> >>>+	cache = btrfs_lookup_block_group(fs_info, chunk_offset);
> >>>+	ASSERT(cache);
> >>>+	chunk_type = cache->flags;
> >>>+	btrfs_put_block_group(cache);
> >>>+
> >>>+	if (chunk_type & BTRFS_BLOCK_GROUP_DATA) {
> >>>+		spin_lock(&fs_info->data_sinfo->lock);
> >>>+		bytes_used = fs_info->data_sinfo->bytes_used;
> >>>+		spin_unlock(&fs_info->data_sinfo->lock);
> >>>+
> >>>+		if (!bytes_used) {
> >>>+			struct btrfs_trans_handle *trans;
> >>>+			int ret;
> >>>+
> >>>+			trans =	btrfs_join_transaction(fs_info->tree_root);
> >>>+			if (IS_ERR(trans))
> >>>+				return PTR_ERR(trans);
> >>>+
> >>>+			ret = btrfs_force_chunk_alloc(trans, fs_info,
> >>>+						      BTRFS_BLOCK_GROUP_DATA);
> >>>+			btrfs_end_transaction(trans);
> >>>+			if (ret < 0)
> >>>+				return ret;
> >>>+
> >>>+			return 1;
> >>>+		}
> >>>+	}
> >>>+	return 0;
> >>>+}
> >>>+
> >>>  static int insert_balance_item(struct btrfs_fs_info *fs_info,
> >>>  			       struct btrfs_balance_control *bctl)
> >>>  {
> >>>@@ -3476,7 +3518,6 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
> >>>  	u32 count_meta = 0;
> >>>  	u32 count_sys = 0;
> >>>  	int chunk_reserved = 0;
> >>>-	u64 bytes_used = 0;
> >>>  	/* step one make some room on all the devices */
> >>>  	devices = &fs_info->fs_devices->devices;
> >>>@@ -3635,28 +3676,21 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
> >>>  			goto loop;
> >>>  		}
> >>>-		ASSERT(fs_info->data_sinfo);
> >>>-		spin_lock(&fs_info->data_sinfo->lock);
> >>>-		bytes_used = fs_info->data_sinfo->bytes_used;
> >>>-		spin_unlock(&fs_info->data_sinfo->lock);
> >>>-
> >>>-		if ((chunk_type & BTRFS_BLOCK_GROUP_DATA) &&
> >>>-		    !chunk_reserved && !bytes_used) {
> >>>-			trans = btrfs_start_transaction(chunk_root, 0);
> >>>-			if (IS_ERR(trans)) {
> >>>-				mutex_unlock(&fs_info->delete_unused_bgs_mutex);
> >>>-				ret = PTR_ERR(trans);
> >>>-				goto error;
> >>>-			}
> >>>-
> >>>-			ret = btrfs_force_chunk_alloc(trans, fs_info,
> >>>-						      BTRFS_BLOCK_GROUP_DATA);
> >>>-			btrfs_end_transaction(trans);
> >>>+		if (!chunk_reserved) {
> >>>+			/*
> >>>+			 * We may be relocating the only data chunk we have,
> >>>+			 * which could potentially end up with losing data's
> >>>+			 * raid profile, so lets allocate an empty one in
> >>>+			 * advance.
> >>>+			 */
> >>>+			ret = btrfs_may_alloc_data_chunk(fs_info,
> >>>+							 found_key.offset);
> >>>  			if (ret < 0) {
> >>>  				mutex_unlock(&fs_info->delete_unused_bgs_mutex);
> >>>  				goto error;
> >>>+			} else if (ret == 1) {
> >>>+				chunk_reserved = 1;
> >>>  			}
> >>>-			chunk_reserved = 1;
> >>>  		}
> >>>  		ret = btrfs_relocate_chunk(fs_info, found_key.offset);
> >>>@@ -4419,6 +4453,18 @@ int btrfs_shrink_device(struct btrfs_device *device, u64 new_size)
> >>>  		chunk_offset = btrfs_dev_extent_chunk_offset(l, dev_extent);
> >>>  		btrfs_release_path(path);
> >>>+		/*
> >>>+		 * We may be relocating the only data chunk we have,
> >>>+		 * which could potentially end up with losing data's
> >>>+		 * raid profile, so lets allocate an empty one in
> >>>+		 * advance.
> >>>+		 */
> >>>+		ret = btrfs_may_alloc_data_chunk(fs_info, chunk_offset);
> >>>+		if (ret < 0) {
> >>>+			mutex_unlock(&fs_info->delete_unused_bgs_mutex);
> >>>+			goto done;
> >>>+		}
> >>>+
> >>>  		ret = btrfs_relocate_chunk(fs_info, chunk_offset);
> >>>  		mutex_unlock(&fs_info->delete_unused_bgs_mutex);
> >>>  		if (ret && ret != -ENOSPC)
> >>>
> >--
> >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 v2] Btrfs: avoid losing data raid profile when deleting a device

[Liu Bo]

On Mon, Oct 16, 2017 at 11:53:08AM +0300, Nikolay Borisov wrote:
> 
> 
> On 13.10.2017 23:51, Liu Bo wrote:
> > On Wed, Oct 11, 2017 at 10:38:50AM +0300, Nikolay Borisov wrote:
> >>
> >>
> >> On 10.10.2017 20:53, Liu Bo wrote:
> >>> We've avoided data losing raid profile when doing balance, but it
> >>> turns out that deleting a device could also result in the same
> >>> problem
> >>>
> >>> This fixes the problem by creating an empty data chunk before
> >>> relocating the data chunk.
> >>
> >> Why is this needed - copy the metadata of the to-be-relocated chunk into
> >> the newly created empty chunk? I don't entirely understand that code but
> >> doesn't this seem a bit like a hack in order to stash some information?
> >> Perhaps you could elaborate the logic a bit more in the changelog?
> >>
> >>>
> >>> Metadata/System chunk are supposed to have non-zero bytes all the time
> >>> so their raid profile is persistent.
> >>
> >> I think this changelog is a bit scarce on detail as to the culprit of
> >> the problem. Could you perhaps put a sentence or two what the underlying
> >> logic which deletes the raid profile if a chunk is empty ?
> >>
> > 
> > Fair enough.
> > 
> > The problem is as same as what commit 2c9fe8355258 ("btrfs: Fix
> > lost-data-profile caused by balance bg") had fixed.
> > 
> > Similar to doing balance, deleting a device can also move all chunks
> > on this disk to other available disks, after 'move' successfully,
> > it'll remove those chunks.
> > 
> > If our last data chunk is empty and part of it happens to be on this
>           ^
> "Last data chunk" of which disk - the "to-be-removed" one or the disk to
> which chunks have been relocated?
>

It refers to the 'to-be-removed' disk.

> > disk, then there is no data chunk in this btrfs after deleting the
>    ^
> Which disk are you referring to - the one being removed?

Yes, the 'to-be-removed' disk.

Thanks,

-liubo

> > device successfully, any following write will try to create a new data
> > chunk which ends up with a single data chunk because the only
> > available data raid profile is 'single'.
> > 
> > thanks,
> > -liubo
> > 
> >>>
> >>> Reported-by: James Alandt <James.Alandt@wdc.com>
> >>> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> >>> ---
> >>>
> >>> v2: - return the correct error.
> >>>     - move helper ahead of __btrfs_balance().
> >>>
> >>>  fs/btrfs/volumes.c | 84 ++++++++++++++++++++++++++++++++++++++++++------------
> >>>  1 file changed, 65 insertions(+), 19 deletions(-)
> >>>
> >>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> >>> index 4a72c45..a74396d 100644
> >>> --- a/fs/btrfs/volumes.c
> >>> +++ b/fs/btrfs/volumes.c
> >>> @@ -3018,6 +3018,48 @@ static int btrfs_relocate_sys_chunks(struct btrfs_fs_info *fs_info)
> >>>  	return ret;
> >>>  }
> >>>  
> >>> +/*
> >>> + * return 1 : allocate a data chunk successfully,
> >>> + * return <0: errors during allocating a data chunk,
> >>> + * return 0 : no need to allocate a data chunk.
> >>> + */
> >>> +static int btrfs_may_alloc_data_chunk(struct btrfs_fs_info *fs_info,
> >>> +				      u64 chunk_offset)
> >>> +{
> >>> +	struct btrfs_block_group_cache *cache;
> >>> +	u64 bytes_used;
> >>> +	u64 chunk_type;
> >>> +
> >>> +	cache = btrfs_lookup_block_group(fs_info, chunk_offset);
> >>> +	ASSERT(cache);
> >>> +	chunk_type = cache->flags;
> >>> +	btrfs_put_block_group(cache);
> >>> +
> >>> +	if (chunk_type & BTRFS_BLOCK_GROUP_DATA) {
> >>> +		spin_lock(&fs_info->data_sinfo->lock);
> >>> +		bytes_used = fs_info->data_sinfo->bytes_used;
> >>> +		spin_unlock(&fs_info->data_sinfo->lock);
> >>> +
> >>> +		if (!bytes_used) {
> >>> +			struct btrfs_trans_handle *trans;
> >>> +			int ret;
> >>> +
> >>> +			trans =	btrfs_join_transaction(fs_info->tree_root);
> >>> +			if (IS_ERR(trans))
> >>> +				return PTR_ERR(trans);
> >>> +
> >>> +			ret = btrfs_force_chunk_alloc(trans, fs_info,
> >>> +						      BTRFS_BLOCK_GROUP_DATA);
> >>> +			btrfs_end_transaction(trans);
> >>> +			if (ret < 0)
> >>> +				return ret;
> >>> +
> >>> +			return 1;
> >>> +		}
> >>> +	}
> >>> +	return 0;
> >>> +}
> >>> +
> >>>  static int insert_balance_item(struct btrfs_fs_info *fs_info,
> >>>  			       struct btrfs_balance_control *bctl)
> >>>  {
> >>> @@ -3476,7 +3518,6 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
> >>>  	u32 count_meta = 0;
> >>>  	u32 count_sys = 0;
> >>>  	int chunk_reserved = 0;
> >>> -	u64 bytes_used = 0;
> >>>  
> >>>  	/* step one make some room on all the devices */
> >>>  	devices = &fs_info->fs_devices->devices;
> >>> @@ -3635,28 +3676,21 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
> >>>  			goto loop;
> >>>  		}
> >>>  
> >>> -		ASSERT(fs_info->data_sinfo);
> >>> -		spin_lock(&fs_info->data_sinfo->lock);
> >>> -		bytes_used = fs_info->data_sinfo->bytes_used;
> >>> -		spin_unlock(&fs_info->data_sinfo->lock);
> >>> -
> >>> -		if ((chunk_type & BTRFS_BLOCK_GROUP_DATA) &&
> >>> -		    !chunk_reserved && !bytes_used) {
> >>> -			trans = btrfs_start_transaction(chunk_root, 0);
> >>> -			if (IS_ERR(trans)) {
> >>> -				mutex_unlock(&fs_info->delete_unused_bgs_mutex);
> >>> -				ret = PTR_ERR(trans);
> >>> -				goto error;
> >>> -			}
> >>> -
> >>> -			ret = btrfs_force_chunk_alloc(trans, fs_info,
> >>> -						      BTRFS_BLOCK_GROUP_DATA);
> >>> -			btrfs_end_transaction(trans);
> >>> +		if (!chunk_reserved) {
> >>> +			/*
> >>> +			 * We may be relocating the only data chunk we have,
> >>> +			 * which could potentially end up with losing data's
> >>> +			 * raid profile, so lets allocate an empty one in
> >>> +			 * advance.
> >>> +			 */
> >>> +			ret = btrfs_may_alloc_data_chunk(fs_info,
> >>> +							 found_key.offset);
> >>>  			if (ret < 0) {
> >>>  				mutex_unlock(&fs_info->delete_unused_bgs_mutex);
> >>>  				goto error;
> >>> +			} else if (ret == 1) {
> >>> +				chunk_reserved = 1;
> >>>  			}
> >>> -			chunk_reserved = 1;
> >>>  		}
> >>>  
> >>>  		ret = btrfs_relocate_chunk(fs_info, found_key.offset);
> >>> @@ -4419,6 +4453,18 @@ int btrfs_shrink_device(struct btrfs_device *device, u64 new_size)
> >>>  		chunk_offset = btrfs_dev_extent_chunk_offset(l, dev_extent);
> >>>  		btrfs_release_path(path);
> >>>  
> >>> +		/*
> >>> +		 * We may be relocating the only data chunk we have,
> >>> +		 * which could potentially end up with losing data's
> >>> +		 * raid profile, so lets allocate an empty one in
> >>> +		 * advance.
> >>> +		 */
> >>> +		ret = btrfs_may_alloc_data_chunk(fs_info, chunk_offset);
> >>> +		if (ret < 0) {
> >>> +			mutex_unlock(&fs_info->delete_unused_bgs_mutex);
> >>> +			goto done;
> >>> +		}
> >>> +
> >>>  		ret = btrfs_relocate_chunk(fs_info, chunk_offset);
> >>>  		mutex_unlock(&fs_info->delete_unused_bgs_mutex);
> >>>  		if (ret && ret != -ENOSPC)
> >>>
> > 

[PATCH v3] Btrfs: avoid losing data raid profile when deleting a device

[Liu Bo]

We've avoided data losing raid profile when doing balance, but it
turns out that deleting a device could also result in the same
problem.

Say we have 3 disks, and they're created with '-d raid1' profile.

- We have chunk P (the only data chunk on the empty btrfs).

- Suppose that chunk P's two raid1 copies reside in disk A and disk B.

- Now, 'btrfs device remove disk B'
         btrfs_rm_device()
	   -> btrfs_shrink_device()
	      -> btrfs_relocate_chunk() #relocate any chunk on disk B
	      	 			 to other places.

- Chunk P will be removed and a new chunk will be created to hold
  those data, but as chunk P is the only one holding raid1 profile,
  after it goes away, the new chunk will be created as single profile
  which is our default profile.

This fixes the problem by creating an empty data chunk before
relocating the data chunk.

Metadata/System chunk are supposed to have non-zero bytes all the time
so their raid profile is preserved.

Reported-by: James Alandt <James.Alandt@wdc.com>
Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
v3: More details about how losing data profile happens.

v2: - return the correct error.
    - move helper ahead of __btrfs_balance().

 fs/btrfs/volumes.c | 84 ++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 65 insertions(+), 19 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 4a72c45..a74396d 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -3018,6 +3018,48 @@ static int btrfs_relocate_sys_chunks(struct btrfs_fs_info *fs_info)
 	return ret;
 }
 
+/*
+ * return 1 : allocate a data chunk successfully,
+ * return <0: errors during allocating a data chunk,
+ * return 0 : no need to allocate a data chunk.
+ */
+static int btrfs_may_alloc_data_chunk(struct btrfs_fs_info *fs_info,
+				      u64 chunk_offset)
+{
+	struct btrfs_block_group_cache *cache;
+	u64 bytes_used;
+	u64 chunk_type;
+
+	cache = btrfs_lookup_block_group(fs_info, chunk_offset);
+	ASSERT(cache);
+	chunk_type = cache->flags;
+	btrfs_put_block_group(cache);
+
+	if (chunk_type & BTRFS_BLOCK_GROUP_DATA) {
+		spin_lock(&fs_info->data_sinfo->lock);
+		bytes_used = fs_info->data_sinfo->bytes_used;
+		spin_unlock(&fs_info->data_sinfo->lock);
+
+		if (!bytes_used) {
+			struct btrfs_trans_handle *trans;
+			int ret;
+
+			trans =	btrfs_join_transaction(fs_info->tree_root);
+			if (IS_ERR(trans))
+				return PTR_ERR(trans);
+
+			ret = btrfs_force_chunk_alloc(trans, fs_info,
+						      BTRFS_BLOCK_GROUP_DATA);
+			btrfs_end_transaction(trans);
+			if (ret < 0)
+				return ret;
+
+			return 1;
+		}
+	}
+	return 0;
+}
+
 static int insert_balance_item(struct btrfs_fs_info *fs_info,
 			       struct btrfs_balance_control *bctl)
 {
@@ -3476,7 +3518,6 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
 	u32 count_meta = 0;
 	u32 count_sys = 0;
 	int chunk_reserved = 0;
-	u64 bytes_used = 0;
 
 	/* step one make some room on all the devices */
 	devices = &fs_info->fs_devices->devices;
@@ -3635,28 +3676,21 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
 			goto loop;
 		}
 
-		ASSERT(fs_info->data_sinfo);
-		spin_lock(&fs_info->data_sinfo->lock);
-		bytes_used = fs_info->data_sinfo->bytes_used;
-		spin_unlock(&fs_info->data_sinfo->lock);
-
-		if ((chunk_type & BTRFS_BLOCK_GROUP_DATA) &&
-		    !chunk_reserved && !bytes_used) {
-			trans = btrfs_start_transaction(chunk_root, 0);
-			if (IS_ERR(trans)) {
-				mutex_unlock(&fs_info->delete_unused_bgs_mutex);
-				ret = PTR_ERR(trans);
-				goto error;
-			}
-
-			ret = btrfs_force_chunk_alloc(trans, fs_info,
-						      BTRFS_BLOCK_GROUP_DATA);
-			btrfs_end_transaction(trans);
+		if (!chunk_reserved) {
+			/*
+			 * We may be relocating the only data chunk we have,
+			 * which could potentially end up with losing data's
+			 * raid profile, so lets allocate an empty one in
+			 * advance.
+			 */
+			ret = btrfs_may_alloc_data_chunk(fs_info,
+							 found_key.offset);
 			if (ret < 0) {
 				mutex_unlock(&fs_info->delete_unused_bgs_mutex);
 				goto error;
+			} else if (ret == 1) {
+				chunk_reserved = 1;
 			}
-			chunk_reserved = 1;
 		}
 
 		ret = btrfs_relocate_chunk(fs_info, found_key.offset);
@@ -4419,6 +4453,18 @@ int btrfs_shrink_device(struct btrfs_device *device, u64 new_size)
 		chunk_offset = btrfs_dev_extent_chunk_offset(l, dev_extent);
 		btrfs_release_path(path);
 
+		/*
+		 * We may be relocating the only data chunk we have,
+		 * which could potentially end up with losing data's
+		 * raid profile, so lets allocate an empty one in
+		 * advance.
+		 */
+		ret = btrfs_may_alloc_data_chunk(fs_info, chunk_offset);
+		if (ret < 0) {
+			mutex_unlock(&fs_info->delete_unused_bgs_mutex);
+			goto done;
+		}
+
 		ret = btrfs_relocate_chunk(fs_info, chunk_offset);
 		mutex_unlock(&fs_info->delete_unused_bgs_mutex);
 		if (ret && ret != -ENOSPC)
-- 
2.9.4

Re: [PATCH v3] Btrfs: avoid losing data raid profile when deleting a device

[David Sterba]

On Wed, Nov 15, 2017 at 04:28:11PM -0700, Liu Bo wrote:
> We've avoided data losing raid profile when doing balance, but it
> turns out that deleting a device could also result in the same
> problem.
> 
> Say we have 3 disks, and they're created with '-d raid1' profile.
> 
> - We have chunk P (the only data chunk on the empty btrfs).
> 
> - Suppose that chunk P's two raid1 copies reside in disk A and disk B.
> 
> - Now, 'btrfs device remove disk B'
>          btrfs_rm_device()
> 	   -> btrfs_shrink_device()
> 	      -> btrfs_relocate_chunk() #relocate any chunk on disk B
> 	      	 			 to other places.
> 
> - Chunk P will be removed and a new chunk will be created to hold
>   those data, but as chunk P is the only one holding raid1 profile,
>   after it goes away, the new chunk will be created as single profile
>   which is our default profile.
> 
> This fixes the problem by creating an empty data chunk before
> relocating the data chunk.
> 
> Metadata/System chunk are supposed to have non-zero bytes all the time
> so their raid profile is preserved.
> 
> Reported-by: James Alandt <James.Alandt@wdc.com>
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>

Added to 4.16 queue, thanks.

Re: [PATCH] Btrfs: avoid losing data raid profile when deleting a device

[Liu Bo]

Please ignore this, I put the wrong ML.

Thanks,

-liubo

On Mon, Oct 09, 2017 at 11:38:09AM -0600, Liu Bo wrote:
> We've avoided data losing raid profile when doing balance, but it
> turns out that deleting a device could also result in the same
> problem.
> 
> This fixes the problem by creating an empty data chunk before
> relocating the data chunk.
> 
> Metadata/System chunk are supposed to have non-zero bytes all the time
> so their raid profile is persistent.
> 
> Reported-by: James Alandt <James.Alandt@wdc.com>
> Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
> ---
>  fs/btrfs/volumes.c | 87 ++++++++++++++++++++++++++++++++++++++++++------------
>  1 file changed, 68 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 4a72c45..3f48bcd 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -144,6 +144,8 @@ static int __btrfs_map_block(struct btrfs_fs_info *fs_info,
>  			     u64 logical, u64 *length,
>  			     struct btrfs_bio **bbio_ret,
>  			     int mirror_num, int need_raid_map);
> +static int btrfs_may_alloc_data_chunk(struct btrfs_fs_info *fs_info,
> +				      u64 chunk_offset);
>  
>  DEFINE_MUTEX(uuid_mutex);
>  static LIST_HEAD(fs_uuids);
> @@ -3476,7 +3478,6 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
>  	u32 count_meta = 0;
>  	u32 count_sys = 0;
>  	int chunk_reserved = 0;
> -	u64 bytes_used = 0;
>  
>  	/* step one make some room on all the devices */
>  	devices = &fs_info->fs_devices->devices;
> @@ -3635,28 +3636,22 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
>  			goto loop;
>  		}
>  
> -		ASSERT(fs_info->data_sinfo);
> -		spin_lock(&fs_info->data_sinfo->lock);
> -		bytes_used = fs_info->data_sinfo->bytes_used;
> -		spin_unlock(&fs_info->data_sinfo->lock);
> -
> -		if ((chunk_type & BTRFS_BLOCK_GROUP_DATA) &&
> -		    !chunk_reserved && !bytes_used) {
> -			trans = btrfs_start_transaction(chunk_root, 0);
> -			if (IS_ERR(trans)) {
> -				mutex_unlock(&fs_info->delete_unused_bgs_mutex);
> -				ret = PTR_ERR(trans);
> -				goto error;
> -			}
> -
> -			ret = btrfs_force_chunk_alloc(trans, fs_info,
> -						      BTRFS_BLOCK_GROUP_DATA);
> -			btrfs_end_transaction(trans);
> +		if (!chunk_reserved) {
> +			/*
> +			 * We may be relocating the only data chunk we have,
> +			 * which could potentially end up with losing data's
> +			 * raid profile, so lets allocate an empty one in
> +			 * advance.
> +			 */
> +			ret = btrfs_may_alloc_data_chunk(fs_info,
> +							 found_key.offset);
>  			if (ret < 0) {
>  				mutex_unlock(&fs_info->delete_unused_bgs_mutex);
> +				ret = PTR_ERR(trans);
>  				goto error;
> +			} else if (ret == 1) {
> +				chunk_reserved = 1;
>  			}
> -			chunk_reserved = 1;
>  		}
>  
>  		ret = btrfs_relocate_chunk(fs_info, found_key.offset);
> @@ -4327,6 +4322,48 @@ int btrfs_check_uuid_tree(struct btrfs_fs_info *fs_info)
>  }
>  
>  /*
> + * return 1 : allocate a data chunk successfully,
> + * return <0: errors during allocating a data chunk,
> + * return 0 : no need to allocate a data chunk.
> + */
> +static int btrfs_may_alloc_data_chunk(struct btrfs_fs_info *fs_info,
> +				      u64 chunk_offset)
> +{
> +	struct btrfs_block_group_cache *cache;
> +	u64 bytes_used;
> +	u64 chunk_type;
> +
> +	cache = btrfs_lookup_block_group(fs_info, chunk_offset);
> +	ASSERT(cache);
> +	chunk_type = cache->flags;
> +	btrfs_put_block_group(cache);
> +
> +	if (chunk_type & BTRFS_BLOCK_GROUP_DATA) {
> +		spin_lock(&fs_info->data_sinfo->lock);
> +		bytes_used = fs_info->data_sinfo->bytes_used;
> +		spin_unlock(&fs_info->data_sinfo->lock);
> +
> +		if (!bytes_used) {
> +			struct btrfs_trans_handle *trans;
> +			int ret;
> +
> +			trans =	btrfs_join_transaction(fs_info->tree_root);
> +			if (IS_ERR(trans))
> +				return PTR_ERR(trans);
> +
> +			ret = btrfs_force_chunk_alloc(trans, fs_info,
> +						      BTRFS_BLOCK_GROUP_DATA);
> +			btrfs_end_transaction(trans);
> +			if (ret < 0)
> +				return ret;
> +
> +			return 1;
> +		}
> +	}
> +	return 0;
> +}
> +
> +/*
>   * shrinking a device means finding all of the device extents past
>   * the new size, and then following the back refs to the chunks.
>   * The chunk relocation code actually frees the device extent
> @@ -4419,6 +4456,18 @@ int btrfs_shrink_device(struct btrfs_device *device, u64 new_size)
>  		chunk_offset = btrfs_dev_extent_chunk_offset(l, dev_extent);
>  		btrfs_release_path(path);
>  
> +		/*
> +		 * We may be relocating the only data chunk we have,
> +		 * which could potentially end up with losing data's
> +		 * raid profile, so lets allocate an empty one in
> +		 * advance.
> +		 */
> +		ret = btrfs_may_alloc_data_chunk(fs_info, chunk_offset);
> +		if (ret < 0) {
> +			mutex_unlock(&fs_info->delete_unused_bgs_mutex);
> +			goto done;
> +		}
> +
>  		ret = btrfs_relocate_chunk(fs_info, chunk_offset);
>  		mutex_unlock(&fs_info->delete_unused_bgs_mutex);
>  		if (ret && ret != -ENOSPC)
> -- 
> 2.9.4
> 

[PATCH] Btrfs: avoid losing data raid profile when deleting a device

[Liu Bo]

We've avoided data losing raid profile when doing balance, but it
turns out that deleting a device could also result in the same
problem.

This fixes the problem by creating an empty data chunk before
relocating the data chunk.

Metadata/System chunk are supposed to have non-zero bytes all the time
so their raid profile is persistent.

Reported-by: James Alandt <James.Alandt@wdc.com>
Signed-off-by: Liu Bo <bo.li.liu@oracle.com>
---
 fs/btrfs/volumes.c | 87 ++++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 68 insertions(+), 19 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 4a72c45..3f48bcd 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -144,6 +144,8 @@ static int __btrfs_map_block(struct btrfs_fs_info *fs_info,
 			     u64 logical, u64 *length,
 			     struct btrfs_bio **bbio_ret,
 			     int mirror_num, int need_raid_map);
+static int btrfs_may_alloc_data_chunk(struct btrfs_fs_info *fs_info,
+				      u64 chunk_offset);
 
 DEFINE_MUTEX(uuid_mutex);
 static LIST_HEAD(fs_uuids);
@@ -3476,7 +3478,6 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
 	u32 count_meta = 0;
 	u32 count_sys = 0;
 	int chunk_reserved = 0;
-	u64 bytes_used = 0;
 
 	/* step one make some room on all the devices */
 	devices = &fs_info->fs_devices->devices;
@@ -3635,28 +3636,22 @@ static int __btrfs_balance(struct btrfs_fs_info *fs_info)
 			goto loop;
 		}
 
-		ASSERT(fs_info->data_sinfo);
-		spin_lock(&fs_info->data_sinfo->lock);
-		bytes_used = fs_info->data_sinfo->bytes_used;
-		spin_unlock(&fs_info->data_sinfo->lock);
-
-		if ((chunk_type & BTRFS_BLOCK_GROUP_DATA) &&
-		    !chunk_reserved && !bytes_used) {
-			trans = btrfs_start_transaction(chunk_root, 0);
-			if (IS_ERR(trans)) {
-				mutex_unlock(&fs_info->delete_unused_bgs_mutex);
-				ret = PTR_ERR(trans);
-				goto error;
-			}
-
-			ret = btrfs_force_chunk_alloc(trans, fs_info,
-						      BTRFS_BLOCK_GROUP_DATA);
-			btrfs_end_transaction(trans);
+		if (!chunk_reserved) {
+			/*
+			 * We may be relocating the only data chunk we have,
+			 * which could potentially end up with losing data's
+			 * raid profile, so lets allocate an empty one in
+			 * advance.
+			 */
+			ret = btrfs_may_alloc_data_chunk(fs_info,
+							 found_key.offset);
 			if (ret < 0) {
 				mutex_unlock(&fs_info->delete_unused_bgs_mutex);
+				ret = PTR_ERR(trans);
 				goto error;
+			} else if (ret == 1) {
+				chunk_reserved = 1;
 			}
-			chunk_reserved = 1;
 		}
 
 		ret = btrfs_relocate_chunk(fs_info, found_key.offset);
@@ -4327,6 +4322,48 @@ int btrfs_check_uuid_tree(struct btrfs_fs_info *fs_info)
 }
 
 /*
+ * return 1 : allocate a data chunk successfully,
+ * return <0: errors during allocating a data chunk,
+ * return 0 : no need to allocate a data chunk.
+ */
+static int btrfs_may_alloc_data_chunk(struct btrfs_fs_info *fs_info,
+				      u64 chunk_offset)
+{
+	struct btrfs_block_group_cache *cache;
+	u64 bytes_used;
+	u64 chunk_type;
+
+	cache = btrfs_lookup_block_group(fs_info, chunk_offset);
+	ASSERT(cache);
+	chunk_type = cache->flags;
+	btrfs_put_block_group(cache);
+
+	if (chunk_type & BTRFS_BLOCK_GROUP_DATA) {
+		spin_lock(&fs_info->data_sinfo->lock);
+		bytes_used = fs_info->data_sinfo->bytes_used;
+		spin_unlock(&fs_info->data_sinfo->lock);
+
+		if (!bytes_used) {
+			struct btrfs_trans_handle *trans;
+			int ret;
+
+			trans =	btrfs_join_transaction(fs_info->tree_root);
+			if (IS_ERR(trans))
+				return PTR_ERR(trans);
+
+			ret = btrfs_force_chunk_alloc(trans, fs_info,
+						      BTRFS_BLOCK_GROUP_DATA);
+			btrfs_end_transaction(trans);
+			if (ret < 0)
+				return ret;
+
+			return 1;
+		}
+	}
+	return 0;
+}
+
+/*
  * shrinking a device means finding all of the device extents past
  * the new size, and then following the back refs to the chunks.
  * The chunk relocation code actually frees the device extent
@@ -4419,6 +4456,18 @@ int btrfs_shrink_device(struct btrfs_device *device, u64 new_size)
 		chunk_offset = btrfs_dev_extent_chunk_offset(l, dev_extent);
 		btrfs_release_path(path);
 
+		/*
+		 * We may be relocating the only data chunk we have,
+		 * which could potentially end up with losing data's
+		 * raid profile, so lets allocate an empty one in
+		 * advance.
+		 */
+		ret = btrfs_may_alloc_data_chunk(fs_info, chunk_offset);
+		if (ret < 0) {
+			mutex_unlock(&fs_info->delete_unused_bgs_mutex);
+			goto done;
+		}
+
 		ret = btrfs_relocate_chunk(fs_info, chunk_offset);
 		mutex_unlock(&fs_info->delete_unused_bgs_mutex);
 		if (ret && ret != -ENOSPC)
-- 
2.9.4