| Message ID | 1458816494-483-2-git-send-email-anand.jain@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Thu, Mar 24, 2016 at 06:48:13PM +0800, Anand Jain wrote: > Sysfs create context should come in the last, so that we > don't have to undo sysfs operation for the reason that any > other operation has failed. Moving the sysfs call will make a visible change: in the old code, the sysfs node exists during the whole replace process, while in the new code it appears only after it finishes. While this is not necessarily a problem, I'd like to check that his is an intended change, as it's not mentioned in the changelog. Besides, the sysfs node seems to be added unconditionally, so if the scrub is running in parallel (checked a few lines above the new code), we'll happily add the target device although no replace happened. -- 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
On 04/26/2016 12:23 AM, David Sterba wrote: > On Thu, Mar 24, 2016 at 06:48:13PM +0800, Anand Jain wrote: >> Sysfs create context should come in the last, so that we >> don't have to undo sysfs operation for the reason that any >> other operation has failed. > > Moving the sysfs call will make a visible change: in the old code, the > sysfs node exists during the whole replace process, while in the new > code it appears only after it finishes. > While this is not necessarily a > problem, I'd like to check that his is an intended change, as it's not > mentioned in the changelog. Ah. I missed this point. > Besides, the sysfs node seems to be added unconditionally, so if the > scrub is running in parallel (checked a few lines above the new code), > we'll happily add the target device although no replace happened. Replace may take a long time, We definitely need devid 0 shown at sysfs. Apparently we don't have the undo-sysfs part in the original code, which this patch tried to mitigate, but it needs a deeper fix. I have to pull this patch out and leave the original bug as is for now. Further we need to handle fail return of btrfs_dev_replace_finishing() which seeks sysfs and rest of the stuff un-done as well. Thanks, Anand -- 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
diff --git a/fs/btrfs/dev-replace.c b/fs/btrfs/dev-replace.c index d38cad37ba27..025f42ef5ab3 100644 --- a/fs/btrfs/dev-replace.c +++ b/fs/btrfs/dev-replace.c @@ -397,10 +397,6 @@ int btrfs_dev_replace_start(struct btrfs_root *root, args->result = BTRFS_IOCTL_DEV_REPLACE_RESULT_NO_ERROR; btrfs_dev_replace_unlock(dev_replace, 1); - ret = btrfs_sysfs_add_device_link(tgt_device->fs_devices, tgt_device); - if (ret) - btrfs_err(fs_info, "kobj add dev failed %d\n", ret); - btrfs_wait_ordered_roots(fs_info, -1); /* force writing the updated state information to disk */ @@ -428,6 +424,9 @@ int btrfs_dev_replace_start(struct btrfs_root *root, WARN_ON(ret); } + if (btrfs_sysfs_add_device_link(tgt_device->fs_devices, tgt_device)) + btrfs_err(fs_info, "kobj add dev failed during replace\n"); + return ret; leave:
Sysfs create context should come in the last, so that we don't have to undo sysfs operation for the reason that any other operation has failed. Signed-off-by: Anand Jain <anand.jain@oracle.com> --- fs/btrfs/dev-replace.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)