diff mbox series

[1/2] btrfs-progs: check for no result before using results

Message ID 1545016436-4137-1-git-send-email-anand.jain@oracle.com (mailing list archive)
State New, archived
Headers show
Series [1/2] btrfs-progs: check for no result before using results | expand

Commit Message

Anand Jain Dec. 17, 2018, 3:13 a.m. UTC 
User space understands the ioctl BTRFS_IOC_DEV_REPLACE command status
using the struct btrfs_ioctl_dev_replace_args::result, and so userspace
initializes this to BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_RESULT, so exclude
this value in checking for the error.

Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
 cmds-replace.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Nikolay Borisov Dec. 17, 2018, 6:55 a.m. UTC | #1 
On 17.12.18 г. 5:13 ч., Anand Jain wrote:
> User space understands the ioctl BTRFS_IOC_DEV_REPLACE command status
> using the struct btrfs_ioctl_dev_replace_args::result, and so userspace
> initializes this to BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_RESULT, so exclude
> this value in checking for the error.
> 
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
> ---
>  cmds-replace.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/cmds-replace.c b/cmds-replace.c
> index b30e6c781e64..42de4de8c031 100644
> --- a/cmds-replace.c
> +++ b/cmds-replace.c
> @@ -296,6 +296,8 @@ static int cmd_replace_start(int argc, char **argv)
>  		}
>  
>  		if (start_args.result !=
> +		    BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_RESULT &&
> +		    start_args.result !=

While this change is OK, it really is redundant, since we do
IOC_DEV_REPLACE with CMD_STATUS, meaning in kernel space we always call
btrfs_dev_replace_status which always overwrites ->result member.


Also looking at the other 3 cmds available for this IOCTL it's always
guaranteed for ->result to be overwritten if it executes btrfs code.

OTOH  if the capable, memdup or an unrecognised ->cmd  is detected then
an ordinary error code is returned, in which case the ret < 0 check
executes and laves via "leave_with_error" label.

While your patch is OK code wise it's really a no op

>  		    BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR) {
>  			error("ioctl(DEV_REPLACE_START) on '%s' returns error: %s",
>  				path,
>
Anand Jain Dec. 17, 2018, 7:49 a.m. UTC | #2 
On 12/17/2018 02:55 PM, Nikolay Borisov wrote:
> 
> 
> On 17.12.18 г. 5:13 ч., Anand Jain wrote:
>> User space understands the ioctl BTRFS_IOC_DEV_REPLACE command status
>> using the struct btrfs_ioctl_dev_replace_args::result, and so userspace
>> initializes this to BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_RESULT, so exclude
>> this value in checking for the error.
>>
>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>> ---
>>   cmds-replace.c | 2 ++
>>   1 file changed, 2 insertions(+)
>>
>> diff --git a/cmds-replace.c b/cmds-replace.c
>> index b30e6c781e64..42de4de8c031 100644
>> --- a/cmds-replace.c
>> +++ b/cmds-replace.c
>> @@ -296,6 +296,8 @@ static int cmd_replace_start(int argc, char **argv)
>>   		}
>>   
>>   		if (start_args.result !=
>> +		    BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_RESULT &&
>> +		    start_args.result !=
> 
> While this change is OK, it really is redundant, since we do
> IOC_DEV_REPLACE with CMD_STATUS, meaning in kernel space we always call
> btrfs_dev_replace_status which always overwrites ->result member.

> Also looking at the other 3 cmds available for this IOCTL it's always
> guaranteed for ->result to be overwritten if it executes btrfs code.

> OTOH  if the capable, memdup or an unrecognised ->cmd  is detected then
> an ordinary error code is returned, in which case the ret < 0 check
> executes and laves via "leave_with_error" label.
> 
> While your patch is OK code wise it's really a no op

  Did you miss the point that BTRFS_FS_EXCL_OP can be set by some other
  thread such as balance?.
  So in this context BTRFS_IOCTL_DEV_REPLACE_CMD_STATUS fails to
  report BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS and as the replace thread
  moves further ahead, the BTRFS_IOCTL_DEV_REPLACE_CMD_START will know
  that BTRFS_FS_EXCL_OP is set and the kernel does not reset the
  start_args.result value set by the user land which is
  BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_RESULT.

  Besides checking for BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_RESULT is a
  right thing in general.


>>   		    BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR) {
>>   			error("ioctl(DEV_REPLACE_START) on '%s' returns error: %s",
>>   				path,
>>
Nikolay Borisov Dec. 17, 2018, 8:47 a.m. UTC | #3 
On 17.12.18 г. 9:49 ч., Anand Jain wrote:
> 
> 
> On 12/17/2018 02:55 PM, Nikolay Borisov wrote:
>>
>>
>> On 17.12.18 г. 5:13 ч., Anand Jain wrote:
>>> User space understands the ioctl BTRFS_IOC_DEV_REPLACE command status
>>> using the struct btrfs_ioctl_dev_replace_args::result, and so userspace
>>> initializes this to BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_RESULT, so exclude
>>> this value in checking for the error.
>>>
>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>> ---
>>>   cmds-replace.c | 2 ++
>>>   1 file changed, 2 insertions(+)
>>>
>>> diff --git a/cmds-replace.c b/cmds-replace.c
>>> index b30e6c781e64..42de4de8c031 100644
>>> --- a/cmds-replace.c
>>> +++ b/cmds-replace.c
>>> @@ -296,6 +296,8 @@ static int cmd_replace_start(int argc, char **argv)
>>>           }
>>>             if (start_args.result !=
>>> +            BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_RESULT &&
>>> +            start_args.result !=
>>
>> While this change is OK, it really is redundant, since we do
>> IOC_DEV_REPLACE with CMD_STATUS, meaning in kernel space we always call
>> btrfs_dev_replace_status which always overwrites ->result member.
> 
>> Also looking at the other 3 cmds available for this IOCTL it's always
>> guaranteed for ->result to be overwritten if it executes btrfs code.
> 
>> OTOH  if the capable, memdup or an unrecognised ->cmd  is detected then
>> an ordinary error code is returned, in which case the ret < 0 check
>> executes and laves via "leave_with_error" label.
>>
>> While your patch is OK code wise it's really a no op
> 
>  Did you miss the point that BTRFS_FS_EXCL_OP can be set by some other
>  thread such as balance?.
>  So in this context BTRFS_IOCTL_DEV_REPLACE_CMD_STATUS fails to>  report BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS and as the replace thread

Then perhaps btrfs_dev_replace_status should be modified so that
->status is set to an appropriate value. Looking at it
replace_dev_result2string will also have to be extended to recognize
BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS.

>  moves further ahead, the BTRFS_IOCTL_DEV_REPLACE_CMD_START will know
>  that BTRFS_FS_EXCL_OP is set and the kernel does not reset the
>  start_args.result value set by the user land which is
>  BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_RESULT.

So what? the ioctl call will return BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS
which is positive (and you add handling for that in the second patch).

> 
>  Besides checking for BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_RESULT is a
>  right thing in general.

What is the real problem you are trying to solve here?

> 
> 
>>>               BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR) {
>>>               error("ioctl(DEV_REPLACE_START) on '%s' returns error:
>>> %s",
>>>                   path,
>>>
Anand Jain Dec. 17, 2018, 10:10 a.m. UTC | #4 
On 12/17/2018 04:47 PM, Nikolay Borisov wrote:
> 
> 
> On 17.12.18 г. 9:49 ч., Anand Jain wrote:
>>
>>
>> On 12/17/2018 02:55 PM, Nikolay Borisov wrote:
>>>
>>>
>>> On 17.12.18 г. 5:13 ч., Anand Jain wrote:
>>>> User space understands the ioctl BTRFS_IOC_DEV_REPLACE command status
>>>> using the struct btrfs_ioctl_dev_replace_args::result, and so userspace
>>>> initializes this to BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_RESULT, so exclude
>>>> this value in checking for the error.
>>>>
>>>> Signed-off-by: Anand Jain <anand.jain@oracle.com>
>>>> ---
>>>>    cmds-replace.c | 2 ++
>>>>    1 file changed, 2 insertions(+)
>>>>
>>>> diff --git a/cmds-replace.c b/cmds-replace.c
>>>> index b30e6c781e64..42de4de8c031 100644
>>>> --- a/cmds-replace.c
>>>> +++ b/cmds-replace.c
>>>> @@ -296,6 +296,8 @@ static int cmd_replace_start(int argc, char **argv)
>>>>            }
>>>>              if (start_args.result !=
>>>> +            BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_RESULT &&
>>>> +            start_args.result !=
>>>
>>> While this change is OK, it really is redundant, since we do
>>> IOC_DEV_REPLACE with CMD_STATUS, meaning in kernel space we always call
>>> btrfs_dev_replace_status which always overwrites ->result member.
>>
>>> Also looking at the other 3 cmds available for this IOCTL it's always
>>> guaranteed for ->result to be overwritten if it executes btrfs code.
>>
>>> OTOH  if the capable, memdup or an unrecognised ->cmd  is detected then
>>> an ordinary error code is returned, in which case the ret < 0 check
>>> executes and laves via "leave_with_error" label.
>>>
>>> While your patch is OK code wise it's really a no op
>>
>>   Did you miss the point that BTRFS_FS_EXCL_OP can be set by some other
>>   thread such as balance?.
>>   So in this context BTRFS_IOCTL_DEV_REPLACE_CMD_STATUS fails to>  report BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS and as the replace thread
> 
> Then perhaps btrfs_dev_replace_status should be modified so that
> ->status is set to an appropriate value.

  I disagree, BTRFS_IOCTL_DEV_REPLACE_CMD_STATUS is only to know the
  replace status, so if a user want to know the last replace status
  and if balance is running you are suggesting it should fail with
  BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS. That's wrong.


> Looking at it
> replace_dev_result2string will also have to be extended to recognize
> BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS.

  BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS is a generic btrfs error code,
  and is part of btrfs_err_str(), it can be merged, but outside of this
  patch as a cleanup patch.

>>   moves further ahead, the BTRFS_IOCTL_DEV_REPLACE_CMD_START will know
>>   that BTRFS_FS_EXCL_OP is set and the kernel does not reset the
>>   start_args.result value set by the user land which is
>>   BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_RESULT.
> 
> So what? the ioctl call will return BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS
> which is positive (and you add handling for that in the second patch).

>>
>>   Besides checking for BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_RESULT is a
>>   right thing in general.
> 
> What is the real problem you are trying to solve here?

  As in the patch 2/2

  ERROR: ioctl(DEV_REPLACE_START) on '/btrfs' returns error: <illegal 
result value>

  Now with this patch it shall print only if there is a real error if
  updated by the kernel.

Thanks, Anand

>>
>>
>>>>                BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR) {
>>>>                error("ioctl(DEV_REPLACE_START) on '%s' returns error:
>>>> %s",
>>>>                    path,
>>>>
diff mbox series

Patch

diff --git a/cmds-replace.c b/cmds-replace.c
index b30e6c781e64..42de4de8c031 100644
--- a/cmds-replace.c
+++ b/cmds-replace.c
@@ -296,6 +296,8 @@  static int cmd_replace_start(int argc, char **argv)
 		}
 
 		if (start_args.result !=
+		    BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_RESULT &&
+		    start_args.result !=
 		    BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR) {
 			error("ioctl(DEV_REPLACE_START) on '%s' returns error: %s",
 				path,