* [RFC] BTRFS_DEV_REPLACE_ITEM_STATE_* doesn't match with on disk
@ 2018-11-12 4:58 Anand Jain
2018-11-12 7:50 ` Nikolay Borisov
0 siblings, 1 reply; 6+ messages in thread
From: Anand Jain @ 2018-11-12 4:58 UTC (permalink / raw)
To: linux-btrfs
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?
[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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [RFC] BTRFS_DEV_REPLACE_ITEM_STATE_* doesn't match with on disk
2018-11-12 4:58 [RFC] BTRFS_DEV_REPLACE_ITEM_STATE_* doesn't match with on disk Anand Jain
@ 2018-11-12 7:50 ` Nikolay Borisov
2018-11-13 10:32 ` Anand Jain
2019-08-02 9:46 ` David Sterba
0 siblings, 2 replies; 6+ messages in thread
From: Nikolay Borisov @ 2018-11-12 7:50 UTC (permalink / raw)
To: Anand Jain, linux-btrfs
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
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] BTRFS_DEV_REPLACE_ITEM_STATE_* doesn't match with on disk
2018-11-12 7:50 ` Nikolay Borisov
@ 2018-11-13 10:32 ` Anand Jain
2018-11-21 7:31 ` Anand Jain
2019-08-02 9:46 ` David Sterba
1 sibling, 1 reply; 6+ messages in thread
From: Anand Jain @ 2018-11-13 10:32 UTC (permalink / raw)
To: David Sterba; +Cc: Nikolay Borisov, linux-btrfs
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
>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] BTRFS_DEV_REPLACE_ITEM_STATE_* doesn't match with on disk
2018-11-13 10:32 ` Anand Jain
@ 2018-11-21 7:31 ` Anand Jain
2019-08-02 4:07 ` Anand Jain
0 siblings, 1 reply; 6+ messages in thread
From: Anand Jain @ 2018-11-21 7:31 UTC (permalink / raw)
To: David Sterba, dsterba; +Cc: Nikolay Borisov, linux-btrfs
David, any comments on this please.
Thanks, Anand
On 11/13/2018 06:32 PM, Anand Jain wrote:
>
> 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
>>>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] BTRFS_DEV_REPLACE_ITEM_STATE_* doesn't match with on disk
2018-11-21 7:31 ` Anand Jain
@ 2019-08-02 4:07 ` Anand Jain
0 siblings, 0 replies; 6+ messages in thread
From: Anand Jain @ 2019-08-02 4:07 UTC (permalink / raw)
To: David Sterba, dsterba; +Cc: Nikolay Borisov, linux-btrfs
Ping.
On 11/21/18 3:31 PM, Anand Jain wrote:
>
> David, any comments on this please.
>
> Thanks, Anand
>
>
> On 11/13/2018 06:32 PM, Anand Jain wrote:
>>
>> 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
>>>>
>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [RFC] BTRFS_DEV_REPLACE_ITEM_STATE_* doesn't match with on disk
2018-11-12 7:50 ` Nikolay Borisov
2018-11-13 10:32 ` Anand Jain
@ 2019-08-02 9:46 ` David Sterba
1 sibling, 0 replies; 6+ messages in thread
From: David Sterba @ 2019-08-02 9:46 UTC (permalink / raw)
To: Nikolay Borisov; +Cc: Anand Jain, linux-btrfs
On Mon, Nov 12, 2018 at 09:50:36AM +0200, 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.
I agree it's better to remove them, progs use the define but there's own
defintion in ioctl.h so it's not affected by the kernel header. It's not
like we're removing something in wide use so even if there are some
code that uses it it can be fixed trivially.
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-08-02 9:46 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-11-12 4:58 [RFC] BTRFS_DEV_REPLACE_ITEM_STATE_* doesn't match with on disk Anand Jain
2018-11-12 7:50 ` Nikolay Borisov
2018-11-13 10:32 ` Anand Jain
2018-11-21 7:31 ` Anand Jain
2019-08-02 4:07 ` Anand Jain
2019-08-02 9:46 ` David Sterba
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).