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.
