* [PATCH 1/2] btrfs-progs: check for no result before using results
@ 2018-12-17 3:13 Anand Jain
2018-12-17 3:13 ` [PATCH 2/2] btrfs-progs: replace: gracefully handle the exclusive operation report Anand Jain
2018-12-17 6:55 ` [PATCH 1/2] btrfs-progs: check for no result before using results Nikolay Borisov
0 siblings, 2 replies; 7+ messages in thread
From: Anand Jain @ 2018-12-17 3:13 UTC (permalink / raw)
To: linux-btrfs
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 !=
BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR) {
error("ioctl(DEV_REPLACE_START) on '%s' returns error: %s",
path,
--
1.8.3.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/2] btrfs-progs: replace: gracefully handle the exclusive operation report
2018-12-17 3:13 [PATCH 1/2] btrfs-progs: check for no result before using results Anand Jain
@ 2018-12-17 3:13 ` Anand Jain
2018-12-17 7:20 ` Nikolay Borisov
2018-12-17 6:55 ` [PATCH 1/2] btrfs-progs: check for no result before using results Nikolay Borisov
1 sibling, 1 reply; 7+ messages in thread
From: Anand Jain @ 2018-12-17 3:13 UTC (permalink / raw)
To: linux-btrfs
Replace start fails to report the appropriate error if balance is already
running, as below,
btrfs rep start -B -f /dev/sdb /dev/sde /btrfs
ERROR: ioctl(DEV_REPLACE_START) on '/btrfs' returns error: <illegal
result value>
Fix it by checking if the return is > 0, as the kernel returns
BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS for the above cli, if the balance is
running.
Signed-off-by: Anand Jain <anand.jain@oracle.com>
---
cmds-replace.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/cmds-replace.c b/cmds-replace.c
index 42de4de8c031..696f24c5c6d5 100644
--- a/cmds-replace.c
+++ b/cmds-replace.c
@@ -295,6 +295,10 @@ static int cmd_replace_start(int argc, char **argv)
goto leave_with_error;
}
+ if (ret > 0)
+ error("ioctl(DEV_REPLACE_START) '%s': %s", path,
+ btrfs_err_str(ret));
+
if (start_args.result !=
BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_RESULT &&
start_args.result !=
--
1.8.3.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] btrfs-progs: check for no result before using results
2018-12-17 3:13 [PATCH 1/2] btrfs-progs: check for no result before using results Anand Jain
2018-12-17 3:13 ` [PATCH 2/2] btrfs-progs: replace: gracefully handle the exclusive operation report Anand Jain
@ 2018-12-17 6:55 ` Nikolay Borisov
2018-12-17 7:49 ` Anand Jain
1 sibling, 1 reply; 7+ messages in thread
From: Nikolay Borisov @ 2018-12-17 6:55 UTC (permalink / raw)
To: Anand Jain, linux-btrfs
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,
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 2/2] btrfs-progs: replace: gracefully handle the exclusive operation report
2018-12-17 3:13 ` [PATCH 2/2] btrfs-progs: replace: gracefully handle the exclusive operation report Anand Jain
@ 2018-12-17 7:20 ` Nikolay Borisov
0 siblings, 0 replies; 7+ messages in thread
From: Nikolay Borisov @ 2018-12-17 7:20 UTC (permalink / raw)
To: Anand Jain, linux-btrfs
On 17.12.18 г. 5:13 ч., Anand Jain wrote:
> Replace start fails to report the appropriate error if balance is already
> running, as below,
>
> btrfs rep start -B -f /dev/sdb /dev/sde /btrfs
> ERROR: ioctl(DEV_REPLACE_START) on '/btrfs' returns error: <illegal
> result value>
>
> Fix it by checking if the return is > 0, as the kernel returns
> BTRFS_ERROR_DEV_EXCL_RUN_IN_PROGRESS for the above cli, if the balance is
> running.
>
> Signed-off-by: Anand Jain <anand.jain@oracle.com>
Reviewed-by: Nikolay Borisov <nborisov@suse.com>
> ---
> cmds-replace.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/cmds-replace.c b/cmds-replace.c
> index 42de4de8c031..696f24c5c6d5 100644
> --- a/cmds-replace.c
> +++ b/cmds-replace.c
> @@ -295,6 +295,10 @@ static int cmd_replace_start(int argc, char **argv)
> goto leave_with_error;
> }
>
> + if (ret > 0)
> + error("ioctl(DEV_REPLACE_START) '%s': %s", path,
> + btrfs_err_str(ret));
> +
> if (start_args.result !=
> BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_RESULT &&
> start_args.result !=
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] btrfs-progs: check for no result before using results
2018-12-17 6:55 ` [PATCH 1/2] btrfs-progs: check for no result before using results Nikolay Borisov
@ 2018-12-17 7:49 ` Anand Jain
2018-12-17 8:47 ` Nikolay Borisov
0 siblings, 1 reply; 7+ messages in thread
From: Anand Jain @ 2018-12-17 7:49 UTC (permalink / raw)
To: Nikolay Borisov, linux-btrfs
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,
>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] btrfs-progs: check for no result before using results
2018-12-17 7:49 ` Anand Jain
@ 2018-12-17 8:47 ` Nikolay Borisov
2018-12-17 10:10 ` Anand Jain
0 siblings, 1 reply; 7+ messages in thread
From: Nikolay Borisov @ 2018-12-17 8:47 UTC (permalink / raw)
To: Anand Jain, Nikolay Borisov, linux-btrfs
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,
>>>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] btrfs-progs: check for no result before using results
2018-12-17 8:47 ` Nikolay Borisov
@ 2018-12-17 10:10 ` Anand Jain
0 siblings, 0 replies; 7+ messages in thread
From: Anand Jain @ 2018-12-17 10:10 UTC (permalink / raw)
To: Nikolay Borisov, linux-btrfs
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,
>>>>
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-12-17 10:10 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-17 3:13 [PATCH 1/2] btrfs-progs: check for no result before using results Anand Jain
2018-12-17 3:13 ` [PATCH 2/2] btrfs-progs: replace: gracefully handle the exclusive operation report Anand Jain
2018-12-17 7:20 ` Nikolay Borisov
2018-12-17 6:55 ` [PATCH 1/2] btrfs-progs: check for no result before using results Nikolay Borisov
2018-12-17 7:49 ` Anand Jain
2018-12-17 8:47 ` Nikolay Borisov
2018-12-17 10:10 ` Anand Jain
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).