[PATCH 1/3] btrfs: warn for num_devices below 0

[PATCH 1/3] btrfs: warn for num_devices below 0

[Anand Jain]

In preparation to de-duplicate a section of code where we deduce the
num_devices, use warn instead of bug.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index eb78bb8d1108..ce6faeb8bcf8 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -3813,7 +3813,7 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
 	num_devices = fs_info->fs_devices->num_devices;
 	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
 	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
-		BUG_ON(num_devices < 1);
+		WARN_ON(num_devices < 1);
 		num_devices--;
 	}
 	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
-- 
2.7.0

[PATCH 2/3] btrfs: add helper btrfs_num_devices() to deduce num_devices

[Anand Jain]

When the replace is running the fs_devices::num_devices also includes
the replace device, however in some operations like device delete and
balance it needs the actual num_devices without the repalce devices, so
now the function btrfs_num_devices() just provides that.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index ce6faeb8bcf8..59a6d8f42c98 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1931,6 +1931,20 @@ void btrfs_assign_next_active_device(struct btrfs_fs_info *fs_info,
 		fs_info->fs_devices->latest_bdev = next_device->bdev;
 }
 
+static inline u64 btrfs_num_devices(struct btrfs_fs_info *fs_info)
+{
+	u64 num_devices = fs_info->fs_devices->num_devices;
+
+	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
+	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
+		WARN_ON(num_devices < 1);
+		num_devices--;
+	}
+	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
+
+	return num_devices;
+}
+
 int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 		u64 devid)
 {
@@ -1944,13 +1958,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 
 	mutex_lock(&uuid_mutex);
 
-	num_devices = fs_devices->num_devices;
-	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
-	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
-		WARN_ON(num_devices < 1);
-		num_devices--;
-	}
-	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
+	num_devices = btrfs_num_devices(fs_info);
 
 	ret = btrfs_check_raid_min_devices(fs_info, num_devices - 1);
 	if (ret)
@@ -3810,13 +3818,8 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
 		}
 	}
 
-	num_devices = fs_info->fs_devices->num_devices;
-	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
-	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
-		WARN_ON(num_devices < 1);
-		num_devices--;
-	}
-	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
+	num_devices = btrfs_num_devices(fs_info);
+
 	allowed = BTRFS_AVAIL_ALLOC_BIT_SINGLE | BTRFS_BLOCK_GROUP_DUP;
 	if (num_devices > 1)
 		allowed |= (BTRFS_BLOCK_GROUP_RAID0 | BTRFS_BLOCK_GROUP_RAID1);
-- 
2.7.0

[PATCH 3/3] btrfs: add helper function check device delete able

[Anand Jain]

Move the section of the code which performs the check if the device is
indelible, move that into a helper function.

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

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 59a6d8f42c98..feb29c5b44f6 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -1945,6 +1945,33 @@ static inline u64 btrfs_num_devices(struct btrfs_fs_info *fs_info)
 	return num_devices;
 }
 
+static struct btrfs_device *btrfs_device_delete_able(
+				struct btrfs_fs_info *fs_info,
+				const char *device_path, u64 devid)
+{
+	int ret;
+	struct btrfs_device *device;
+
+	ret = btrfs_check_raid_min_devices(fs_info,
+					   btrfs_num_devices(fs_info) - 1);
+	if (ret)
+		return ERR_PTR(ret);
+
+	ret = btrfs_find_device_by_devspec(fs_info, devid, device_path,
+					   &device);
+	if (ret)
+		return ERR_PTR(ret);
+
+	if (test_bit(BTRFS_DEV_STATE_REPLACE_TGT, &device->dev_state))
+		return ERR_PTR(BTRFS_ERROR_DEV_TGT_REPLACE);
+
+	if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state) &&
+	    fs_info->fs_devices->rw_devices == 1)
+		return ERR_PTR(BTRFS_ERROR_DEV_ONLY_WRITABLE);
+
+	return device;
+}
+
 int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 		u64 devid)
 {
@@ -1958,25 +1985,9 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
 
 	mutex_lock(&uuid_mutex);
 
-	num_devices = btrfs_num_devices(fs_info);
-
-	ret = btrfs_check_raid_min_devices(fs_info, num_devices - 1);
-	if (ret)
-		goto out;
-
-	ret = btrfs_find_device_by_devspec(fs_info, devid, device_path,
-					   &device);
-	if (ret)
-		goto out;
-
-	if (test_bit(BTRFS_DEV_STATE_REPLACE_TGT, &device->dev_state)) {
-		ret = BTRFS_ERROR_DEV_TGT_REPLACE;
-		goto out;
-	}
-
-	if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state) &&
-	    fs_info->fs_devices->rw_devices == 1) {
-		ret = BTRFS_ERROR_DEV_ONLY_WRITABLE;
+	device = btrfs_device_delete_able(fs_info, device_path, devid);
+	if (IS_ERR(device)) {
+		ret = PTR_ERR(device);
 		goto out;
 	}
 
-- 
2.7.0

Re: [PATCH 1/3] btrfs: warn for num_devices below 0

[Nikolay Borisov]

On 10.07.2018 21:22, Anand Jain wrote:
> In preparation to de-duplicate a section of code where we deduce the
> num_devices, use warn instead of bug.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/volumes.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index eb78bb8d1108..ce6faeb8bcf8 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -3813,7 +3813,7 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
>  	num_devices = fs_info->fs_devices->num_devices;
>  	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
>  	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
> -		BUG_ON(num_devices < 1);
> +		WARN_ON(num_devices < 1);

Isn't dev_replace_is_ongoing && num_devices < 1 indeed a logical bug
situation? Under what condition can it happen that you deem "non
critical" ?

>  		num_devices--;
>  	}
>  	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
> 

Re: [PATCH 2/3] btrfs: add helper btrfs_num_devices() to deduce num_devices

[Nikolay Borisov]

On 10.07.2018 21:22, Anand Jain wrote:
> When the replace is running the fs_devices::num_devices also includes
> the replace device, however in some operations like device delete and
> balance it needs the actual num_devices without the repalce devices, so
> now the function btrfs_num_devices() just provides that.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/volumes.c | 31 +++++++++++++++++--------------
>  1 file changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index ce6faeb8bcf8..59a6d8f42c98 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1931,6 +1931,20 @@ void btrfs_assign_next_active_device(struct btrfs_fs_info *fs_info,
>  		fs_info->fs_devices->latest_bdev = next_device->bdev;
>  }
>  

Put a comment above the function saying it returns the number of devices
minus the replace device (in case replace is working) otherwise one have
to go and 'git blame' it

> +static inline u64 btrfs_num_devices(struct btrfs_fs_info *fs_info)
> +{
> +	u64 num_devices = fs_info->fs_devices->num_devices;
> +
> +	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
> +	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
> +		WARN_ON(num_devices < 1);
> +		num_devices--;
> +	}
> +	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
> +
> +	return num_devices;
> +}
> +
>  int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
>  		u64 devid)
>  {
> @@ -1944,13 +1958,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
>  
>  	mutex_lock(&uuid_mutex);
>  
> -	num_devices = fs_devices->num_devices;
> -	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
> -	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
> -		WARN_ON(num_devices < 1);
> -		num_devices--;
> -	}
> -	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
> +	num_devices = btrfs_num_devices(fs_info);
>  
>  	ret = btrfs_check_raid_min_devices(fs_info, num_devices - 1);
>  	if (ret)
> @@ -3810,13 +3818,8 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
>  		}
>  	}
>  
> -	num_devices = fs_info->fs_devices->num_devices;
> -	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
> -	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
> -		WARN_ON(num_devices < 1);
> -		num_devices--;
> -	}
> -	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
> +	num_devices = btrfs_num_devices(fs_info);
> +
>  	allowed = BTRFS_AVAIL_ALLOC_BIT_SINGLE | BTRFS_BLOCK_GROUP_DUP;
>  	if (num_devices > 1)
>  		allowed |= (BTRFS_BLOCK_GROUP_RAID0 | BTRFS_BLOCK_GROUP_RAID1);
> 

Re: [PATCH 3/3] btrfs: add helper function check device delete able

[Nikolay Borisov]

On 10.07.2018 21:22, Anand Jain wrote:
> Move the section of the code which performs the check if the device is
> indelible, move that into a helper function.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  fs/btrfs/volumes.c | 49 ++++++++++++++++++++++++++++++-------------------
>  1 file changed, 30 insertions(+), 19 deletions(-)
> 
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 59a6d8f42c98..feb29c5b44f6 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -1945,6 +1945,33 @@ static inline u64 btrfs_num_devices(struct btrfs_fs_info *fs_info)
>  	return num_devices;
>  }
>  
> +static struct btrfs_device *btrfs_device_delete_able(

Ugliest name ever! So this function is not really a predicate, rather
it's used to fetch the struct btrfs_device * to delete. So a more
becoming name would be:

btrfs_get_device_for_delete - though this a bit verbose.

I guess btrfs_can_delete_device is more suitable if you want to follow
this predicate style. At the very least, though, the correct form of the
adjective is deletable so it should be btrfs_device_deletable. But as I
said this function is not really used as a predicate.

> +				struct btrfs_fs_info *fs_info,
> +				const char *device_path, u64 devid)
> +{
> +	int ret;
> +	struct btrfs_device *device;
> +
> +	ret = btrfs_check_raid_min_devices(fs_info,
> +					   btrfs_num_devices(fs_info) - 1);
> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	ret = btrfs_find_device_by_devspec(fs_info, devid, device_path,
> +					   &device);

Not really related to this patchset, but I think the whole
btrfs_find_device_by_devspec -> btrfs_find_device_missing_or_by_path
could be simplified by making those functions return a pointer to
btrfs_device rather than an int error value. That way you eliminate the
ugly "argument as return value" convention.

> +	if (ret)
> +		return ERR_PTR(ret);
> +
> +	if (test_bit(BTRFS_DEV_STATE_REPLACE_TGT, &device->dev_state))
> +		return ERR_PTR(BTRFS_ERROR_DEV_TGT_REPLACE);
> +
> +	if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state) &&
> +	    fs_info->fs_devices->rw_devices == 1)
> +		return ERR_PTR(BTRFS_ERROR_DEV_ONLY_WRITABLE);
> +
> +	return device;
> +}
> +
>  int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
>  		u64 devid)
>  {
> @@ -1958,25 +1985,9 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
>  
>  	mutex_lock(&uuid_mutex);
>  
> -	num_devices = btrfs_num_devices(fs_info);
> -
> -	ret = btrfs_check_raid_min_devices(fs_info, num_devices - 1);
> -	if (ret)
> -		goto out;
> -
> -	ret = btrfs_find_device_by_devspec(fs_info, devid, device_path,
> -					   &device);
> -	if (ret)
> -		goto out;
> -
> -	if (test_bit(BTRFS_DEV_STATE_REPLACE_TGT, &device->dev_state)) {
> -		ret = BTRFS_ERROR_DEV_TGT_REPLACE;
> -		goto out;
> -	}
> -
> -	if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state) &&
> -	    fs_info->fs_devices->rw_devices == 1) {
> -		ret = BTRFS_ERROR_DEV_ONLY_WRITABLE;
> +	device = btrfs_device_delete_able(fs_info, device_path, devid);
> +	if (IS_ERR(device)) {
> +		ret = PTR_ERR(device);
>  		goto out;
>  	}
>  
> 

Re: [PATCH 1/3] btrfs: warn for num_devices below 0

[Anand Jain]

On 07/12/2018 03:13 PM, Nikolay Borisov wrote:
> 
> 
> On 10.07.2018 21:22, Anand Jain wrote:
>> In preparation to de-duplicate a section of code where we deduce the
>> num_devices, use warn instead of bug.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/volumes.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index eb78bb8d1108..ce6faeb8bcf8 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -3813,7 +3813,7 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
>>   	num_devices = fs_info->fs_devices->num_devices;
>>   	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
>>   	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
>> -		BUG_ON(num_devices < 1);
>> +		WARN_ON(num_devices < 1);


> Isn't dev_replace_is_ongoing && num_devices < 1 indeed a logical bug
> situation? Under what condition can it happen that you deem "non
> critical" ?

  In all conditions needs least one device for the FS to be mounted.
  In fact we can just remove it.

Thanks, Anand

>>   		num_devices--;
>>   	}
>>   	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
>>
> --
> 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/3] btrfs: add helper btrfs_num_devices() to deduce num_devices

[Anand Jain]

On 07/12/2018 03:31 PM, Nikolay Borisov wrote:
> 
> 
> On 10.07.2018 21:22, Anand Jain wrote:
>> When the replace is running the fs_devices::num_devices also includes
>> the replace device, however in some operations like device delete and
>> balance it needs the actual num_devices without the repalce devices, so
>> now the function btrfs_num_devices() just provides that.

>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/volumes.c | 31 +++++++++++++++++--------------
>>   1 file changed, 17 insertions(+), 14 deletions(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index ce6faeb8bcf8..59a6d8f42c98 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -1931,6 +1931,20 @@ void btrfs_assign_next_active_device(struct btrfs_fs_info *fs_info,
>>   		fs_info->fs_devices->latest_bdev = next_device->bdev;
>>   }
>>   
> 
> Put a comment above the function saying it returns the number of devices
> minus the replace device (in case replace is working) otherwise one have
> to go and 'git blame' it

good idea will rename it to btrfs_num_devices_minus_replace(), (will
wait to see if there is any better suggested).

Thanks, Anand



>> +static inline u64 btrfs_num_devices(struct btrfs_fs_info *fs_info)
>> +{
>> +	u64 num_devices = fs_info->fs_devices->num_devices;
>> +
>> +	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
>> +	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
>> +		WARN_ON(num_devices < 1);
>> +		num_devices--;
>> +	}
>> +	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
>> +
>> +	return num_devices;
>> +}
>> +
>>   int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
>>   		u64 devid)
>>   {
>> @@ -1944,13 +1958,7 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
>>   
>>   	mutex_lock(&uuid_mutex);
>>   
>> -	num_devices = fs_devices->num_devices;
>> -	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
>> -	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
>> -		WARN_ON(num_devices < 1);
>> -		num_devices--;
>> -	}
>> -	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
>> +	num_devices = btrfs_num_devices(fs_info);
>>   
>>   	ret = btrfs_check_raid_min_devices(fs_info, num_devices - 1);
>>   	if (ret)
>> @@ -3810,13 +3818,8 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
>>   		}
>>   	}
>>   
>> -	num_devices = fs_info->fs_devices->num_devices;
>> -	btrfs_dev_replace_read_lock(&fs_info->dev_replace);
>> -	if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
>> -		WARN_ON(num_devices < 1);
>> -		num_devices--;
>> -	}
>> -	btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
>> +	num_devices = btrfs_num_devices(fs_info);
>> +
>>   	allowed = BTRFS_AVAIL_ALLOC_BIT_SINGLE | BTRFS_BLOCK_GROUP_DUP;
>>   	if (num_devices > 1)
>>   		allowed |= (BTRFS_BLOCK_GROUP_RAID0 | BTRFS_BLOCK_GROUP_RAID1);
>>
> --
> 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/3] btrfs: add helper btrfs_num_devices() to deduce num_devices

[Nikolay Borisov]

On 13.07.2018 14:17, Anand Jain wrote:
> 
> 
> On 07/12/2018 03:31 PM, Nikolay Borisov wrote:
>>
>>
>> On 10.07.2018 21:22, Anand Jain wrote:
>>> When the replace is running the fs_devices::num_devices also includes
>>> the replace device, however in some operations like device delete and
>>> balance it needs the actual num_devices without the repalce devices, so
>>> now the function btrfs_num_devices() just provides that.
> 
>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>> ---
>>>   fs/btrfs/volumes.c | 31 +++++++++++++++++--------------
>>>   1 file changed, 17 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>> index ce6faeb8bcf8..59a6d8f42c98 100644
>>> --- a/fs/btrfs/volumes.c
>>> +++ b/fs/btrfs/volumes.c
>>> @@ -1931,6 +1931,20 @@ void btrfs_assign_next_active_device(struct
>>> btrfs_fs_info *fs_info,
>>>           fs_info->fs_devices->latest_bdev = next_device->bdev;
>>>   }
>>>   
>>
>> Put a comment above the function saying it returns the number of devices
>> minus the replace device (in case replace is working) otherwise one have
>> to go and 'git blame' it
> 
> good idea will rename it to btrfs_num_devices_minus_replace(), (will
> wait to see if there is any better suggested).

Unfortunately I don't have a better name yet but
num_devices_minus_replace is way too verbose so don't use that...

> 
> Thanks, Anand
> 
> 
> 
>>> +static inline u64 btrfs_num_devices(struct btrfs_fs_info *fs_info)
>>> +{
>>> +    u64 num_devices = fs_info->fs_devices->num_devices;
>>> +
>>> +    btrfs_dev_replace_read_lock(&fs_info->dev_replace);
>>> +    if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
>>> +        WARN_ON(num_devices < 1);
>>> +        num_devices--;
>>> +    }
>>> +    btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
>>> +
>>> +    return num_devices;
>>> +}
>>> +
>>>   int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char
>>> *device_path,
>>>           u64 devid)
>>>   {
>>> @@ -1944,13 +1958,7 @@ int btrfs_rm_device(struct btrfs_fs_info
>>> *fs_info, const char *device_path,
>>>         mutex_lock(&uuid_mutex);
>>>   -    num_devices = fs_devices->num_devices;
>>> -    btrfs_dev_replace_read_lock(&fs_info->dev_replace);
>>> -    if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
>>> -        WARN_ON(num_devices < 1);
>>> -        num_devices--;
>>> -    }
>>> -    btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
>>> +    num_devices = btrfs_num_devices(fs_info);
>>>         ret = btrfs_check_raid_min_devices(fs_info, num_devices - 1);
>>>       if (ret)
>>> @@ -3810,13 +3818,8 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
>>>           }
>>>       }
>>>   -    num_devices = fs_info->fs_devices->num_devices;
>>> -    btrfs_dev_replace_read_lock(&fs_info->dev_replace);
>>> -    if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
>>> -        WARN_ON(num_devices < 1);
>>> -        num_devices--;
>>> -    }
>>> -    btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
>>> +    num_devices = btrfs_num_devices(fs_info);
>>> +
>>>       allowed = BTRFS_AVAIL_ALLOC_BIT_SINGLE | BTRFS_BLOCK_GROUP_DUP;
>>>       if (num_devices > 1)
>>>           allowed |= (BTRFS_BLOCK_GROUP_RAID0 |
>>> BTRFS_BLOCK_GROUP_RAID1);
>>>
>> -- 
>> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> -- 
> To unsubscribe from this list: send the line "unsubscribe linux-btrfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: [PATCH 3/3] btrfs: add helper function check device delete able

[Anand Jain]

On 07/12/2018 03:43 PM, Nikolay Borisov wrote:
> 
> 
> On 10.07.2018 21:22, Anand Jain wrote:
>> Move the section of the code which performs the check if the device is
>> indelible, move that into a helper function.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   fs/btrfs/volumes.c | 49 ++++++++++++++++++++++++++++++-------------------
>>   1 file changed, 30 insertions(+), 19 deletions(-)
>>
>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>> index 59a6d8f42c98..feb29c5b44f6 100644
>> --- a/fs/btrfs/volumes.c
>> +++ b/fs/btrfs/volumes.c
>> @@ -1945,6 +1945,33 @@ static inline u64 btrfs_num_devices(struct btrfs_fs_info *fs_info)
>>   	return num_devices;
>>   }
>>   
>> +static struct btrfs_device *btrfs_device_delete_able(
> 
> Ugliest name ever! So this function is not really a predicate, rather
> it's used to fetch the struct btrfs_device * to delete. So a more
> becoming name would be:
> 
> btrfs_get_device_for_delete - though this a bit verbose.
> 
> I guess btrfs_can_delete_device is more suitable if you want to follow
> this predicate style. At the very least, though, the correct form of the
> adjective is deletable so it should be btrfs_device_deletable. But as I
> said this function is not really used as a predicate.

  Its a predicate, return of the device pointer is just a by-product.
  Will use btrfs_device_deletable().


>> +				struct btrfs_fs_info *fs_info,
>> +				const char *device_path, u64 devid)
>> +{
>> +	int ret;
>> +	struct btrfs_device *device;
>> +
>> +	ret = btrfs_check_raid_min_devices(fs_info,
>> +					   btrfs_num_devices(fs_info) - 1);
>> +	if (ret)
>> +		return ERR_PTR(ret);
>> +
>> +	ret = btrfs_find_device_by_devspec(fs_info, devid, device_path,
>> +					   &device);
> 
> Not really related to this patchset, but I think the whole
> btrfs_find_device_by_devspec -> btrfs_find_device_missing_or_by_path
> could be simplified by making those functions return a pointer to
> btrfs_device rather than an int error value. That way you eliminate the
> ugly "argument as return value" convention.

  I agree with you. This is just a fist set of cleanup.

Thanks, Anand


>> +	if (ret)
>> +		return ERR_PTR(ret);
>> +
>> +	if (test_bit(BTRFS_DEV_STATE_REPLACE_TGT, &device->dev_state))
>> +		return ERR_PTR(BTRFS_ERROR_DEV_TGT_REPLACE);
>> +
>> +	if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state) &&
>> +	    fs_info->fs_devices->rw_devices == 1)
>> +		return ERR_PTR(BTRFS_ERROR_DEV_ONLY_WRITABLE);
>> +
>> +	return device;
>> +}
>> +
>>   int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
>>   		u64 devid)
>>   {
>> @@ -1958,25 +1985,9 @@ int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char *device_path,
>>   
>>   	mutex_lock(&uuid_mutex);
>>   
>> -	num_devices = btrfs_num_devices(fs_info);
>> -
>> -	ret = btrfs_check_raid_min_devices(fs_info, num_devices - 1);
>> -	if (ret)
>> -		goto out;
>> -
>> -	ret = btrfs_find_device_by_devspec(fs_info, devid, device_path,
>> -					   &device);
>> -	if (ret)
>> -		goto out;
>> -
>> -	if (test_bit(BTRFS_DEV_STATE_REPLACE_TGT, &device->dev_state)) {
>> -		ret = BTRFS_ERROR_DEV_TGT_REPLACE;
>> -		goto out;
>> -	}
>> -
>> -	if (test_bit(BTRFS_DEV_STATE_WRITEABLE, &device->dev_state) &&
>> -	    fs_info->fs_devices->rw_devices == 1) {
>> -		ret = BTRFS_ERROR_DEV_ONLY_WRITABLE;
>> +	device = btrfs_device_delete_able(fs_info, device_path, devid);
>> +	if (IS_ERR(device)) {
>> +		ret = PTR_ERR(device);
>>   		goto out;
>>   	}
>>   
>>
> --
> 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 3/3] btrfs: add helper function check device delete able

[Nikolay Borisov]

On 13.07.2018 14:27, Anand Jain wrote:
> 
> 
> On 07/12/2018 03:43 PM, Nikolay Borisov wrote:
>>
>>
>> On 10.07.2018 21:22, Anand Jain wrote:
>>> Move the section of the code which performs the check if the device is
>>> indelible, move that into a helper function.
>>>
>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>> ---
>>>   fs/btrfs/volumes.c | 49
>>> ++++++++++++++++++++++++++++++-------------------
>>>   1 file changed, 30 insertions(+), 19 deletions(-)
>>>
>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>> index 59a6d8f42c98..feb29c5b44f6 100644
>>> --- a/fs/btrfs/volumes.c
>>> +++ b/fs/btrfs/volumes.c
>>> @@ -1945,6 +1945,33 @@ static inline u64 btrfs_num_devices(struct
>>> btrfs_fs_info *fs_info)
>>>       return num_devices;
>>>   }
>>>   +static struct btrfs_device *btrfs_device_delete_able(
>>
>> Ugliest name ever! So this function is not really a predicate, rather
>> it's used to fetch the struct btrfs_device * to delete. So a more
>> becoming name would be:
>>
>> btrfs_get_device_for_delete - though this a bit verbose.
>>
>> I guess btrfs_can_delete_device is more suitable if you want to follow
>> this predicate style. At the very least, though, the correct form of the
>> adjective is deletable so it should be btrfs_device_deletable. But as I
>> said this function is not really used as a predicate.
> 
>  Its a predicate, return of the device pointer is just a by-product.
>  Will use btrfs_device_deletable().

Then it's fundamentally wrong, a predicate should really return true or
false. This function actually tries to acquire a device which will only
happen if it meets certain criterion, so I'm inclined to say it's not
really a predicate but rather tries to acquire a reference to a device
which meets certain criteria.

<snip>

Re: [PATCH 3/3] btrfs: add helper function check device delete able

[Anand Jain]

On 07/13/2018 07:28 PM, Nikolay Borisov wrote:
> 
> 
> On 13.07.2018 14:27, Anand Jain wrote:
>>
>>
>> On 07/12/2018 03:43 PM, Nikolay Borisov wrote:
>>>
>>>
>>> On 10.07.2018 21:22, Anand Jain wrote:
>>>> Move the section of the code which performs the check if the device is
>>>> indelible, move that into a helper function.
>>>>
>>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>>> ---
>>>>    fs/btrfs/volumes.c | 49
>>>> ++++++++++++++++++++++++++++++-------------------
>>>>    1 file changed, 30 insertions(+), 19 deletions(-)
>>>>
>>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>>> index 59a6d8f42c98..feb29c5b44f6 100644
>>>> --- a/fs/btrfs/volumes.c
>>>> +++ b/fs/btrfs/volumes.c
>>>> @@ -1945,6 +1945,33 @@ static inline u64 btrfs_num_devices(struct
>>>> btrfs_fs_info *fs_info)
>>>>        return num_devices;
>>>>    }
>>>>    +static struct btrfs_device *btrfs_device_delete_able(
>>>
>>> Ugliest name ever! So this function is not really a predicate, rather
>>> it's used to fetch the struct btrfs_device * to delete. So a more
>>> becoming name would be:
>>>
>>> btrfs_get_device_for_delete - though this a bit verbose.
>>>
>>> I guess btrfs_can_delete_device is more suitable if you want to follow
>>> this predicate style. At the very least, though, the correct form of the
>>> adjective is deletable so it should be btrfs_device_deletable. But as I
>>> said this function is not really used as a predicate.
>>
>>   Its a predicate, return of the device pointer is just a by-product.
>>   Will use btrfs_device_deletable().

> Then it's fundamentally wrong, a predicate should really return true or
> false. This function actually tries to acquire a device which will only
> happen if it meets certain criterion, so I'm inclined to say it's not
> really a predicate but rather tries to acquire a reference to a device
> which meets certain criteria.

  Ok. Will rename it, so that it won't look like predicate. However as
  btrfs_device.. should come fist based on the other functions, will
  use,  btrfs_device_get_for_delete()

Thanks, Anand


> 
> <snip>
> 

Re: [PATCH 2/3] btrfs: add helper btrfs_num_devices() to deduce num_devices

[Anand Jain]

On 07/13/2018 07:17 PM, Nikolay Borisov wrote:
> 
> 
> On 13.07.2018 14:17, Anand Jain wrote:
>>
>>
>> On 07/12/2018 03:31 PM, Nikolay Borisov wrote:
>>>
>>>
>>> On 10.07.2018 21:22, Anand Jain wrote:
>>>> When the replace is running the fs_devices::num_devices also includes
>>>> the replace device, however in some operations like device delete and
>>>> balance it needs the actual num_devices without the repalce devices, so
>>>> now the function btrfs_num_devices() just provides that.
>>
>>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>>> ---
>>>>    fs/btrfs/volumes.c | 31 +++++++++++++++++--------------
>>>>    1 file changed, 17 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
>>>> index ce6faeb8bcf8..59a6d8f42c98 100644
>>>> --- a/fs/btrfs/volumes.c
>>>> +++ b/fs/btrfs/volumes.c
>>>> @@ -1931,6 +1931,20 @@ void btrfs_assign_next_active_device(struct
>>>> btrfs_fs_info *fs_info,
>>>>            fs_info->fs_devices->latest_bdev = next_device->bdev;
>>>>    }
>>>>    
>>>
>>> Put a comment above the function saying it returns the number of devices
>>> minus the replace device (in case replace is working) otherwise one have
>>> to go and 'git blame' it
>>
>> good idea will rename it to btrfs_num_devices_minus_replace(), (will
>> wait to see if there is any better suggested).
> 
> Unfortunately I don't have a better name yet but
> num_devices_minus_replace is way too verbose so don't use that...

  Ok will just add comment. The function name will remain as
  btrfs_num_devices()

Thanks, Anand

>>
>> Thanks, Anand
>>
>>
>>
>>>> +static inline u64 btrfs_num_devices(struct btrfs_fs_info *fs_info)
>>>> +{
>>>> +    u64 num_devices = fs_info->fs_devices->num_devices;
>>>> +
>>>> +    btrfs_dev_replace_read_lock(&fs_info->dev_replace);
>>>> +    if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
>>>> +        WARN_ON(num_devices < 1);
>>>> +        num_devices--;
>>>> +    }
>>>> +    btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
>>>> +
>>>> +    return num_devices;
>>>> +}
>>>> +
>>>>    int btrfs_rm_device(struct btrfs_fs_info *fs_info, const char
>>>> *device_path,
>>>>            u64 devid)
>>>>    {
>>>> @@ -1944,13 +1958,7 @@ int btrfs_rm_device(struct btrfs_fs_info
>>>> *fs_info, const char *device_path,
>>>>          mutex_lock(&uuid_mutex);
>>>>    -    num_devices = fs_devices->num_devices;
>>>> -    btrfs_dev_replace_read_lock(&fs_info->dev_replace);
>>>> -    if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
>>>> -        WARN_ON(num_devices < 1);
>>>> -        num_devices--;
>>>> -    }
>>>> -    btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
>>>> +    num_devices = btrfs_num_devices(fs_info);
>>>>          ret = btrfs_check_raid_min_devices(fs_info, num_devices - 1);
>>>>        if (ret)
>>>> @@ -3810,13 +3818,8 @@ int btrfs_balance(struct btrfs_fs_info *fs_info,
>>>>            }
>>>>        }
>>>>    -    num_devices = fs_info->fs_devices->num_devices;
>>>> -    btrfs_dev_replace_read_lock(&fs_info->dev_replace);
>>>> -    if (btrfs_dev_replace_is_ongoing(&fs_info->dev_replace)) {
>>>> -        WARN_ON(num_devices < 1);
>>>> -        num_devices--;
>>>> -    }
>>>> -    btrfs_dev_replace_read_unlock(&fs_info->dev_replace);
>>>> +    num_devices = btrfs_num_devices(fs_info);
>>>> +
>>>>        allowed = BTRFS_AVAIL_ALLOC_BIT_SINGLE | BTRFS_BLOCK_GROUP_DUP;
>>>>        if (num_devices > 1)
>>>>            allowed |= (BTRFS_BLOCK_GROUP_RAID0 |
>>>> BTRFS_BLOCK_GROUP_RAID1);
>>>>
>>> -- 
>>> 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
>