Re: [RFC] BTRFS_DEV_REPLACE_ITEM_STATE_* doesn't match with on disk

[Anand Jain <anand.jain@oracle.com>]

David, Gentle ping.

Thanks, Anand

On 11/12/2018 03:50 PM, Nikolay Borisov wrote:
> 
> 
> On 12.11.18 г. 6:58 ч., Anand Jain wrote:
>>
>> The dev_replace_state defines are miss matched between the
>> BTRFS_IOCTL_DEV_REPLACE_STATE_* and BTRFS_DEV_REPLACE_ITEM_STATE_* [1].
>>
>> [1]
>> -----------------------------
>> btrfs.h:#define BTRFS_IOCTL_DEV_REPLACE_STATE_FINISHED        2
>> btrfs.h:#define BTRFS_IOCTL_DEV_REPLACE_STATE_CANCELED        3
>> btrfs.h:#define BTRFS_IOCTL_DEV_REPLACE_STATE_SUSPENDED        4
>>
>> btrfs_tree.h:#define BTRFS_DEV_REPLACE_ITEM_STATE_SUSPENDED    2
>> btrfs_tree.h:#define BTRFS_DEV_REPLACE_ITEM_STATE_FINISHED    3
>> btrfs_tree.h:#define BTRFS_DEV_REPLACE_ITEM_STATE_CANCELED    4
>> -----------------------------
>>
>> The BTRFS_DEV_REPLACE_ITEM_STATE_* series is unused in both btrfs.ko and
>> btrfs-progs, the on-disk also follows BTRFS_IOCTL_DEV_REPLACE_STATE_*
>> (we set dev_replace->replace_state using the
>> BTRFS_IOCTL_DEV_REPLACE_STATE_* defines and write to the on-disk).
>>
>>   359         btrfs_set_dev_replace_replace_state(eb, ptr,
>>   360                 dev_replace->replace_state);
>>
>> IMO it should be ok to delete the BTRFS_DEV_REPLACE_ITEM_STATE_*
>> altogether? But how about the userland progs other than btrfs-progs?
>> If not at least fix the miss match as in [2], any comments?
> 
> Unfortunately you are right. This seems to stem from sloppy job back in
> the days of initial dev-replace support. BTRFS_DEV_REPLACE_ITEM_STATE_*
> were added in e922e087a35c ("Btrfs: enhance btrfs structures for device
> replace support"), yet they were never used. And the
> IOCTL_DEV_REPLACE_STATE* were added in e93c89c1aaaa ("Btrfs: add new
> sources for device replace code").
> 
> It looks like the ITEM_STATE* definitions were stillborn so to speak and
> personally I'm in favor of removing them. They shouldn't have been
> merged in the first place and indeed the patch doesn't even have a
> Reviewed-by tag. So it originated from the, I'd say, spartan days of
> btrfs development...
> 
> David,  any code which is using BTRFS_DEV_REPLACE_ITEM_STATE_SUSPENDED
> is inherently broken, so how about we remove those definitions, then
> when it's compilation is broken in the future the author will actually
> have a chance to fix it, though it's highly unlikely anyone is relying
> on those definitions.
> 
> 
>>
>> [2]
>> --------------------------------------
>> diff --git a/include/uapi/linux/btrfs_tree.h
>> b/include/uapi/linux/btrfs_tree.h
>> index aff1356c2bb8..9ffa7534cadf 100644
>> --- a/include/uapi/linux/btrfs_tree.h
>> +++ b/include/uapi/linux/btrfs_tree.h
>> @@ -805,9 +805,9 @@ struct btrfs_dev_stats_item {
>>   #define BTRFS_DEV_REPLACE_ITEM_CONT_READING_FROM_SRCDEV_MODE_AVOID     1
>>   #define BTRFS_DEV_REPLACE_ITEM_STATE_NEVER_STARTED     0
>>   #define BTRFS_DEV_REPLACE_ITEM_STATE_STARTED           1
>> -#define BTRFS_DEV_REPLACE_ITEM_STATE_SUSPENDED         2
>> -#define BTRFS_DEV_REPLACE_ITEM_STATE_FINISHED          3
>> -#define BTRFS_DEV_REPLACE_ITEM_STATE_CANCELED          4
>> +#define BTRFS_DEV_REPLACE_ITEM_STATE_FINISHED          2
>> +#define BTRFS_DEV_REPLACE_ITEM_STATE_CANCELED          3
>> +#define BTRFS_DEV_REPLACE_ITEM_STATE_SUSPENDED         4
>>
>>   struct btrfs_dev_replace_item {
>>          /*
>> --------------------------------------
>>
>>
>> Thanks, Anand
>>