[PATCH RESEND 1/2] btrfs: fix parent in memory total_devices after seed delete

[PATCH RESEND 1/2] btrfs: fix parent in memory total_devices after seed delete

[Anand Jain]

In case of deleting the seed device the %cur_devices (seed)
and the %fs_devices (parent) are different. Now, as the parent
fs_devices::total_devices also maintains the total number of devices
including the seed device, so decrement its in-memory value for the
successful seed delete. We are already updating its corresponding on-disk
btrfs_super_block::number_devices value.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/volumes.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 5bd6f3a40f9c..6b807b166ca3 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -2027,6 +2027,9 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 
 	cur_devices->num_devices--;
 	cur_devices->total_devices--;
+	/* Update total_devices for the parent fs_devices if its seed */
+	if (cur_devices != fs_devices)
+		fs_devices->total_devices--;
 
 	if (test_bit(BTRFS_DEV_STATE_MISSING, &device->dev_state))
 		cur_devices->missing_devices--;
-- 
2.15.0

[PATCH 2/2] btrfs: fix missing superblock update in the device delete commit transaction

[Anand Jain]

When a device is deleted, the btrfs_super_block::number_devices is
reduced by 1, but we do that after the commit transaction, so this
change did not made it to the disk and waits for the next commit
transaction whenever it happens.

This can be easily demonstrated using the following test case where I
use the btrfs device ready cli to read the disk and report.

  mkfs.btrfs -fq -dsingle -msingle $dev1 $dev2
  mount $dev1 /btrfs
  btrfs dev del $dev2 /btrfs
  btrfs dev ready $dev1; echo RESULT=$? <-- 1
   Without this patch RESULT returns 1, indicating not ready!

  Testing with a seed device:

  mkfs.btrfs -fq $dev1
  btrfstune -S1 $dev1
  mount $dev1 /btrfs
  btrfs dev add -f $dev2 /btrfs
  umount /btrfs
  mount $dev2 /btrfs
  btrfs dev del $dev1 /btrfs
  btrfs dev ready $dev2; echo RESULT=$? <-- 1

  Fix this by bringing in the num_devices update with in the
  current commit transaction.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 fs/btrfs/volumes.c | 38 ++++++++++++++++++--------------------
 1 file changed, 18 insertions(+), 20 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 6b807b166ca3..18cd73703951 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1823,46 +1823,32 @@ static void update_dev_time(const char *path_name)
 }
 
 static int btrfs_rm_dev_item(struct btrfs_fs_info *fs_info,
-			     struct btrfs_device *device)
+			     struct btrfs_device *device,
+			     struct btrfs_trans_handle *trans)
 {
 	struct btrfs_root *root = fs_info->chunk_root;
 	int ret;
 	struct btrfs_path *path;
 	struct btrfs_key key;
-	struct btrfs_trans_handle *trans;
 
 	path = btrfs_alloc_path();
 	if (!path)
 		return -ENOMEM;
 
-	trans = btrfs_start_transaction(root, 0);
-	if (IS_ERR(trans)) {
-		btrfs_free_path(path);
-		return PTR_ERR(trans);
-	}
 	key.objectid = BTRFS_DEV_ITEMS_OBJECTID;
 	key.type = BTRFS_DEV_ITEM_KEY;
 	key.offset = device->devid;
 
 	ret = btrfs_search_slot(trans, root, &key, path, -1, 1);
-	if (ret) {
-		if (ret > 0)
-			ret = -ENOENT;
-		btrfs_abort_transaction(trans, ret);
-		btrfs_end_transaction(trans);
+	if (ret > 0) {
+		ret = -ENOENT;
 		goto out;
 	}
 
 	ret = btrfs_del_item(trans, root, path);
-	if (ret) {
-		btrfs_abort_transaction(trans, ret);
-		btrfs_end_transaction(trans);
-	}
 
 out:
 	btrfs_free_path(path);
-	if (!ret)
-		ret = btrfs_commit_transaction(trans);
 	return ret;
 }
 
@@ -1946,7 +1932,9 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 		u64 devid)
 {
 	struct btrfs_device *device;
+	struct btrfs_trans_handle *trans;
 	struct btrfs_fs_devices *cur_devices;
+	struct btrfs_root *root = fs_info->dev_root;
 	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
 	u64 num_devices;
 	int ret = 0;
@@ -1994,14 +1982,23 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 	if (ret)
 		goto error_undo;
 
+	trans = btrfs_start_transaction(root, 0);
+	if (IS_ERR(trans)) {
+		ret = PTR_ERR(trans);
+		goto error_undo;
+	}
+
 	/*
 	 * TODO: the superblock still includes this device in its num_devices
 	 * counter although write_all_supers() is not locked out. This
 	 * could give a filesystem state which requires a degraded mount.
 	 */
-	ret = btrfs_rm_dev_item(fs_info, device);
-	if (ret)
+	ret = btrfs_rm_dev_item(fs_info, device, trans);
+	if (ret) {
+		btrfs_abort_transaction(trans, ret);
+		btrfs_end_transaction(trans);
 		goto error_undo;
+	}
 
 	clear_bit(BTRFS_DEV_STATE_IN_FS_METADATA, &device->dev_state);
 	btrfs_scrub_cancel_dev(fs_info, device);
@@ -2070,6 +2067,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 		free_fs_devices(cur_devices);
 	}
 
+	ret = btrfs_commit_transaction(trans);
 out:
 	mutex_unlock(&uuid_mutex);
 	return ret;
-- 
2.15.0

Re: [PATCH 2/2] btrfs: fix missing superblock update in the device delete commit transaction

[Nikolay Borisov]

On  3.07.2018 08:12, Anand Jain wrote:
> When a device is deleted, the btrfs_super_block::number_devices is
> reduced by 1, but we do that after the commit transaction, so this
> change did not made it to the disk and waits for the next commit
> transaction whenever it happens.
> 
> This can be easily demonstrated using the following test case where I
> use the btrfs device ready cli to read the disk and report.
> 
>   mkfs.btrfs -fq -dsingle -msingle $dev1 $dev2
>   mount $dev1 /btrfs
>   btrfs dev del $dev2 /btrfs
>   btrfs dev ready $dev1; echo RESULT=$? <-- 1
>    Without this patch RESULT returns 1, indicating not ready!
> 
>   Testing with a seed device:
> 
>   mkfs.btrfs -fq $dev1
>   btrfstune -S1 $dev1
>   mount $dev1 /btrfs
>   btrfs dev add -f $dev2 /btrfs
>   umount /btrfs
>   mount $dev2 /btrfs
>   btrfs dev del $dev1 /btrfs
>   btrfs dev ready $dev2; echo RESULT=$? <-- 1

Turn those into fully fledged xfstests

> 
>   Fix this by bringing in the num_devices update with in the
>   current commit transaction.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/volumes.c | 38 ++++++++++++++++++--------------------
>  1 file changed, 18 insertions(+), 20 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 6b807b166ca3..18cd73703951 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1823,46 +1823,32 @@ static void update_dev_time(const char *path_name)
>  }
>  
>  static int btrfs_rm_dev_item(struct btrfs_fs_info *fs_info,
> -			     struct btrfs_device *device)
> +			     struct btrfs_device *device,
> +			     struct btrfs_trans_handle *trans)
>  {

While doing this, refactor the function to take transaction as the first
argument and remove the fs_info arg. fs_info in turn can be referenced
from the transaction handle
>  	struct btrfs_root *root = fs_info->chunk_root;
>  	int ret;
>  	struct btrfs_path *path;
>  	struct btrfs_key key;
> -	struct btrfs_trans_handle *trans;
>  
>  	path = btrfs_alloc_path();
>  	if (!path)
>  		return -ENOMEM;
>  
> -	trans = btrfs_start_transaction(root, 0);
> -	if (IS_ERR(trans)) {
> -		btrfs_free_path(path);
> -		return PTR_ERR(trans);
> -	}
>  	key.objectid = BTRFS_DEV_ITEMS_OBJECTID;
>  	key.type = BTRFS_DEV_ITEM_KEY;
>  	key.offset = device->devid;
>  
>  	ret = btrfs_search_slot(trans, root, &key, path, -1, 1);
> -	if (ret) {
> -		if (ret > 0)
> -			ret = -ENOENT;
> -		btrfs_abort_transaction(trans, ret);
> -		btrfs_end_transaction(trans);
> +	if (ret > 0) {
> +		ret = -ENOENT;
>  		goto out;
>  	}
>  
>  	ret = btrfs_del_item(trans, root, path);
> -	if (ret) {
> -		btrfs_abort_transaction(trans, ret);
> -		btrfs_end_transaction(trans);
> -	}
>  
>  out:
>  	btrfs_free_path(path);
> -	if (!ret)
> -		ret = btrfs_commit_transaction(trans);
>  	return ret;
>  }
>  
> @@ -1946,7 +1932,9 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
>  		u64 devid)
>  {
>  	struct btrfs_device *device;
> +	struct btrfs_trans_handle *trans;
>  	struct btrfs_fs_devices *cur_devices;
> +	struct btrfs_root *root = fs_info->dev_root;
>  	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
>  	u64 num_devices;
>  	int ret = 0;
> @@ -1994,14 +1982,23 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
>  	if (ret)
>  		goto error_undo;
>  
> +	trans = btrfs_start_transaction(root, 0);
> +	if (IS_ERR(trans)) {
> +		ret = PTR_ERR(trans);
> +		goto error_undo;
> +	}
> +
>  	/*
>  	 * TODO: the superblock still includes this device in its num_devices
>  	 * counter although write_all_supers() is not locked out. This
>  	 * could give a filesystem state which requires a degraded mount.
>  	 */
Isn't introducing the transaction fixing this TODO item so it can be
removed?
> -	ret = btrfs_rm_dev_item(fs_info, device);
> -	if (ret)
> +	ret = btrfs_rm_dev_item(fs_info, device, trans);
> +	if (ret) {
> +		btrfs_abort_transaction(trans, ret);
> +		btrfs_end_transaction(trans);
>  		goto error_undo;
> +	}
>  
>  	clear_bit(BTRFS_DEV_STATE_IN_FS_METADATA, &device->dev_state);
>  	btrfs_scrub_cancel_dev(fs_info, device);
> @@ -2070,6 +2067,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
>  		free_fs_devices(cur_devices);
>  	}
>  
> +	ret = btrfs_commit_transaction(trans);
>  out:
>  	mutex_unlock(&uuid_mutex);
>  	return ret;
> 

Re: [PATCH 2/2] btrfs: fix missing superblock update in the device delete commit transaction

[Anand Jain]

On 07/03/2018 01:56 PM, Nikolay Borisov wrote:
> 
> 
> On  3.07.2018 08:12, Anand Jain wrote:
>> When a device is deleted, the btrfs_super_block::number_devices is
>> reduced by 1, but we do that after the commit transaction, so this
>> change did not made it to the disk and waits for the next commit
>> transaction whenever it happens.
>>
>> This can be easily demonstrated using the following test case where I
>> use the btrfs device ready cli to read the disk and report.
>>
>>    mkfs.btrfs -fq -dsingle -msingle $dev1 $dev2
>>    mount $dev1 /btrfs
>>    btrfs dev del $dev2 /btrfs
>>    btrfs dev ready $dev1; echo RESULT=$? <-- 1
>>     Without this patch RESULT returns 1, indicating not ready!
>>
>>    Testing with a seed device:
>>
>>    mkfs.btrfs -fq $dev1
>>    btrfstune -S1 $dev1
>>    mount $dev1 /btrfs
>>    btrfs dev add -f $dev2 /btrfs
>>    umount /btrfs
>>    mount $dev2 /btrfs
>>    btrfs dev del $dev1 /btrfs
>>    btrfs dev ready $dev2; echo RESULT=$? <-- 1
> 
> Turn those into fully fledged xfstests

  Thanks for the review. Though that popped-up in my mind I
  didn't do it as because its one off bug test. Anyway as you
  think its a good idea I sent one in the ML.

>>
>>    Fix this by bringing in the num_devices update with in the
>>    current commit transaction.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/volumes.c | 38 ++++++++++++++++++--------------------
>>   1 file changed, 18 insertions(+), 20 deletions(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 6b807b166ca3..18cd73703951 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -1823,46 +1823,32 @@ static void update_dev_time(const char *path_name)
>>   }
>>   
>>   static int btrfs_rm_dev_item(struct btrfs_fs_info *fs_info,
>> -			     struct btrfs_device *device)
>> +			     struct btrfs_device *device,
>> +			     struct btrfs_trans_handle *trans)
>>   {
> 
> While doing this, refactor the function to take transaction as the first
> argument and remove the fs_info arg. fs_info in turn can be referenced
> from the transaction handle

  Right will do.

>>   	struct btrfs_root *root = fs_info->chunk_root;
>>   	int ret;
>>   	struct btrfs_path *path;
>>   	struct btrfs_key key;
>> -	struct btrfs_trans_handle *trans;
>>   
>>   	path = btrfs_alloc_path();
>>   	if (!path)
>>   		return -ENOMEM;
>>   
>> -	trans = btrfs_start_transaction(root, 0);
>> -	if (IS_ERR(trans)) {
>> -		btrfs_free_path(path);
>> -		return PTR_ERR(trans);
>> -	}
>>   	key.objectid = BTRFS_DEV_ITEMS_OBJECTID;
>>   	key.type = BTRFS_DEV_ITEM_KEY;
>>   	key.offset = device->devid;
>>   
>>   	ret = btrfs_search_slot(trans, root, &key, path, -1, 1);
>> -	if (ret) {
>> -		if (ret > 0)
>> -			ret = -ENOENT;
>> -		btrfs_abort_transaction(trans, ret);
>> -		btrfs_end_transaction(trans);
>> +	if (ret > 0) {
>> +		ret = -ENOENT;
>>   		goto out;
>>   	}
>>   
>>   	ret = btrfs_del_item(trans, root, path);
>> -	if (ret) {
>> -		btrfs_abort_transaction(trans, ret);
>> -		btrfs_end_transaction(trans);
>> -	}
>>   
>>   out:
>>   	btrfs_free_path(path);
>> -	if (!ret)
>> -		ret = btrfs_commit_transaction(trans);
>>   	return ret;
>>   }
>>   
>> @@ -1946,7 +1932,9 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
>>   		u64 devid)
>>   {
>>   	struct btrfs_device *device;
>> +	struct btrfs_trans_handle *trans;
>>   	struct btrfs_fs_devices *cur_devices;
>> +	struct btrfs_root *root = fs_info->dev_root;
>>   	struct btrfs_fs_devices *fs_devices = fs_info->fs_devices;
>>   	u64 num_devices;
>>   	int ret = 0;
>> @@ -1994,14 +1982,23 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
>>   	if (ret)
>>   		goto error_undo;
>>   
>> +	trans = btrfs_start_transaction(root, 0);
>> +	if (IS_ERR(trans)) {
>> +		ret = PTR_ERR(trans);
>> +		goto error_undo;
>> +	}
>> +
>>   	/*
>>   	 * TODO: the superblock still includes this device in its num_devices
>>   	 * counter although write_all_supers() is not locked out. This
>>   	 * could give a filesystem state which requires a degraded mount.
>>   	 */
> Isn't introducing the transaction fixing this TODO item so it can be
> removed?

  I think comment is talking about the power-loss situation,
  this comment can be removed.

Thanks, Anand

>> -	ret = btrfs_rm_dev_item(fs_info, device);
>> -	if (ret)
>> +	ret = btrfs_rm_dev_item(fs_info, device, trans);
>> +	if (ret) {
>> +		btrfs_abort_transaction(trans, ret);
>> +		btrfs_end_transaction(trans);
>>   		goto error_undo;
>> +	}
>>   
>>   	clear_bit(BTRFS_DEV_STATE_IN_FS_METADATA, &device->dev_state);
>>   	btrfs_scrub_cancel_dev(fs_info, device);
>> @@ -2070,6 +2067,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
>>   		free_fs_devices(cur_devices);
>>   	}
>>   
>> +	ret = btrfs_commit_transaction(trans);
>>   out:
>>   	mutex_unlock(&uuid_mutex);
>>   	return ret;
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 2/2] btrfs: fix missing superblock update in the device delete commit transaction

[Nikolay Borisov]

On  3.07.2018 12:07, Anand Jain wrote:
> 
> 
> On 07/03/2018 01:56 PM, Nikolay Borisov wrote:
>>
>>
>> On  3.07.2018 08:12, Anand Jain wrote:
>>> When a device is deleted, the btrfs_super_block::number_devices is
>>> reduced by 1, but we do that after the commit transaction, so this
>>> change did not made it to the disk and waits for the next commit
>>> transaction whenever it happens.
>>>
>>> This can be easily demonstrated using the following test case where I
>>> use the btrfs device ready cli to read the disk and report.
>>>
>>>    mkfs.btrfs -fq -dsingle -msingle $dev1 $dev2
>>>    mount $dev1 /btrfs
>>>    btrfs dev del $dev2 /btrfs
>>>    btrfs dev ready $dev1; echo RESULT=$? <-- 1
>>>     Without this patch RESULT returns 1, indicating not ready!
>>>
>>>    Testing with a seed device:
>>>
>>>    mkfs.btrfs -fq $dev1
>>>    btrfstune -S1 $dev1
>>>    mount $dev1 /btrfs
>>>    btrfs dev add -f $dev2 /btrfs
>>>    umount /btrfs
>>>    mount $dev2 /btrfs
>>>    btrfs dev del $dev1 /btrfs
>>>    btrfs dev ready $dev2; echo RESULT=$? <-- 1
>>
>> Turn those into fully fledged xfstests
> 
>  Thanks for the review. Though that popped-up in my mind I
>  didn't do it as because its one off bug test. Anyway as you
>  think its a good idea I sent one in the ML.

IMO every bug which can be tested should be tested for. This enables
having a wide range of regressions tests and future changes passing
those regressions test will bear higher confidence of correctness.