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@xxxxxxxxxx>
---
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,
