[RFC,0/4] undelete subvolume online version
mbox series

Message ID 20180805104001.5488-1-lufq.fnst@cn.fujitsu.com
Headers show
Series
  • undelete subvolume online version
Related show

Message

Lu Fengqi Aug. 5, 2018, 10:39 a.m. UTC
This patchset will add the BTRFS_IOC_SUBVOL_UNDELETE ioctl for online
btrfs subvolume undelete.

And btrfs subvolume undelete subcommand was added to btrfs-progs.

So user can use the following command to recover all the subolume that
is left on the device. The recovered subvolume will be link to <dest> dir
named to <name_prefix><subvol_id>.

# btrfs subvolume undelete [-p <name_prefix>] <dest>

btrfs online undelete version:
https://github.com/littleroad/linux.git undelete

btrfs-progs online undelete version:
https://github.com/littleroad/btrfs-progs.git online_undelete

Issue: #82

Lu Fengqi (4):
  btrfs: factor out btrfs_link_subvol from create_subvol
  btrfs: don't BUG_ON() in btrfs_link_subvol()
  btrfs: undelete: introduce btrfs_undelete_subvolume
  btrfs: undelete: Add the btrfs_ioctl_undelete

 fs/btrfs/ioctl.c           | 270 +++++++++++++++++++++++++++++++++----
 include/uapi/linux/btrfs.h |   9 ++
 2 files changed, 255 insertions(+), 24 deletions(-)

Comments

David Sterba Aug. 7, 2018, 4:39 p.m. UTC | #1
On Sun, Aug 05, 2018 at 06:39:57PM +0800, Lu Fengqi wrote:
> This patchset will add the BTRFS_IOC_SUBVOL_UNDELETE ioctl for online
> btrfs subvolume undelete.
> 
> And btrfs subvolume undelete subcommand was added to btrfs-progs.
> 
> So user can use the following command to recover all the subolume that
> is left on the device. The recovered subvolume will be link to <dest> dir
> named to <name_prefix><subvol_id>.

Hm, I don't agree with the proposed interface - to recover all deleted
subvolumes. IMO this should recover just one subvolume of a given id a
to given directory.

The ioctl structure has to be reworked, I've skimmed the code and saw
some suspicious things but will have a look after the interface is
settled.
--
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
Lu Fengqi Aug. 8, 2018, 2:44 a.m. UTC | #2
On Tue, Aug 07, 2018 at 06:39:50PM +0200, David Sterba wrote:
>On Sun, Aug 05, 2018 at 06:39:57PM +0800, Lu Fengqi wrote:
>> This patchset will add the BTRFS_IOC_SUBVOL_UNDELETE ioctl for online
>> btrfs subvolume undelete.
>> 
>> And btrfs subvolume undelete subcommand was added to btrfs-progs.
>> 
>> So user can use the following command to recover all the subolume that
>> is left on the device. The recovered subvolume will be link to <dest> dir
>> named to <name_prefix><subvol_id>.
>
>Hm, I don't agree with the proposed interface - to recover all deleted
>subvolumes. IMO this should recover just one subvolume of a given id a
>to given directory.

Thank you for taking the time to respond. I may have thought too much about
the interface before. In my imagination, the cleaner kthread is like a
monster that devours user data at any time, so the user must perform an
online undelete operation as soon as possible, so there is no time to
determine the subvol_id that should be passed. However, I have to admit
that I don't know much about the user's actual usage scenarios, I will
accept the interface you provided. Of course, I really like this because it
greatly simplifies the ioctl structure.

>
>The ioctl structure has to be reworked, I've skimmed the code and saw
>some suspicious things but will have a look after the interface is
>settled.

When I rework the ioctl structure, I will carefully recheck the
incorrect place in the code.
Qu Wenruo Aug. 8, 2018, 6:11 a.m. UTC | #3
On 2018年08月08日 00:39, David Sterba wrote:
> On Sun, Aug 05, 2018 at 06:39:57PM +0800, Lu Fengqi wrote:
>> This patchset will add the BTRFS_IOC_SUBVOL_UNDELETE ioctl for online
>> btrfs subvolume undelete.
>>
>> And btrfs subvolume undelete subcommand was added to btrfs-progs.
>>
>> So user can use the following command to recover all the subolume that
>> is left on the device. The recovered subvolume will be link to <dest> dir
>> named to <name_prefix><subvol_id>.
> 
> Hm, I don't agree with the proposed interface - to recover all deleted
> subvolumes. IMO this should recover just one subvolume of a given id a
> to given directory.
> 
> The ioctl structure has to be reworked, I've skimmed the code and saw
> some suspicious things but will have a look after the interface is
> settled.

My concern is, is such purpose really needed?

Yes, it's possible user made some mistake and want to get back the data.
But putting an ioctl for 'undelete', then user may consider btrfs is so
powerful that can undelete everything.
In short, this undelete feature gives user too high expectation.

And don't expect user really to read man pages. There are already tons
of reports where user execute btrfs check --repair without realizing
--repair is pretty dangerous (and thanks to the work done to repair, it
normally doesn't cause catastrophic result, but sometimes it indeed
causes extra damage)

And when user tried and failed due to deleted tree blocks, they will get
even more frustrated or even betrayed.

I prefer to put such undelete as an off-line rescue tool, instead of
making it online with an ioctl interface.

Thanks,
Qu

> --
> 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
>
Lu Fengqi Aug. 8, 2018, 6:53 a.m. UTC | #4
On Wed, Aug 08, 2018 at 02:11:24PM +0800, Qu Wenruo wrote:
>
>
>On 2018年08月08日 00:39, David Sterba wrote:
>> On Sun, Aug 05, 2018 at 06:39:57PM +0800, Lu Fengqi wrote:
>>> This patchset will add the BTRFS_IOC_SUBVOL_UNDELETE ioctl for online
>>> btrfs subvolume undelete.
>>>
>>> And btrfs subvolume undelete subcommand was added to btrfs-progs.
>>>
>>> So user can use the following command to recover all the subolume that
>>> is left on the device. The recovered subvolume will be link to <dest> dir
>>> named to <name_prefix><subvol_id>.
>> 
>> Hm, I don't agree with the proposed interface - to recover all deleted
>> subvolumes. IMO this should recover just one subvolume of a given id a
>> to given directory.
>> 
>> The ioctl structure has to be reworked, I've skimmed the code and saw
>> some suspicious things but will have a look after the interface is
>> settled.
>
>My concern is, is such purpose really needed?
>
>Yes, it's possible user made some mistake and want to get back the data.
>But putting an ioctl for 'undelete', then user may consider btrfs is so
>powerful that can undelete everything.
>In short, this undelete feature gives user too high expectation.
>
>And don't expect user really to read man pages. There are already tons

There is no more way about the too high expectation of users. If we provide
a feature with a sufficiently detailed man page, but users do not read the
man page when using this feature, I can only think that they are not
responsible for their own data. So, this seems to be a problem they need to
consider.

>of reports where user execute btrfs check --repair without realizing
>--repair is pretty dangerous (and thanks to the work done to repair, it
>normally doesn't cause catastrophic result, but sometimes it indeed
>causes extra damage)

The good news is that online undelete is not as dangerous as btrfs check
--repair. In fact, I think it is safe enough.

>
>And when user tried and failed due to deleted tree blocks, they will get
>even more frustrated or even betrayed.

As mentioned previous, maybe we should do what we think is right, such as
give the user more abilities to protect/recover their data, not to take
care of any sensitive users?

>
>I prefer to put such undelete as an off-line rescue tool, instead of
>making it online with an ioctl interface.

I also think that the offline undelete is more useful. After all, umount
immediately to prevent further data loss is always the most effective after
a mistake. However, since we can give the ability of online undelete to a
user which couldn't umount the filesystem easily, and don't have any side
effect on existing features. IMHO, there is no reason to reject this.
Qu Wenruo Aug. 8, 2018, 7:20 a.m. UTC | #5
On 2018年08月08日 14:53, Lu Fengqi wrote:
> On Wed, Aug 08, 2018 at 02:11:24PM +0800, Qu Wenruo wrote:
>>
>>
>> On 2018年08月08日 00:39, David Sterba wrote:
>>> On Sun, Aug 05, 2018 at 06:39:57PM +0800, Lu Fengqi wrote:
>>>> This patchset will add the BTRFS_IOC_SUBVOL_UNDELETE ioctl for online
>>>> btrfs subvolume undelete.
>>>>
>>>> And btrfs subvolume undelete subcommand was added to btrfs-progs.
>>>>
>>>> So user can use the following command to recover all the subolume that
>>>> is left on the device. The recovered subvolume will be link to <dest> dir
>>>> named to <name_prefix><subvol_id>.
>>>
>>> Hm, I don't agree with the proposed interface - to recover all deleted
>>> subvolumes. IMO this should recover just one subvolume of a given id a
>>> to given directory.
>>>
>>> The ioctl structure has to be reworked, I've skimmed the code and saw
>>> some suspicious things but will have a look after the interface is
>>> settled.
>>
>> My concern is, is such purpose really needed?
>>
>> Yes, it's possible user made some mistake and want to get back the data.
>> But putting an ioctl for 'undelete', then user may consider btrfs is so
>> powerful that can undelete everything.
>> In short, this undelete feature gives user too high expectation.
>>
>> And don't expect user really to read man pages. There are already tons
> 
> There is no more way about the too high expectation of users. If we provide
> a feature with a sufficiently detailed man page, but users do not read the
> man page when using this feature, I can only think that they are not
> responsible for their own data. So, this seems to be a problem they need to
> consider.

IMHO this ioctl is design only for a pretty niche use case.
As long as it's a designed ioctl, it's some kind of a "main" feature.

Even it's users' response to read the man page, we still need to bring
the surprise to minimal.
By all mean, undelete is a minor use case and should be prevented from
the very beginning.

> 
>> of reports where user execute btrfs check --repair without realizing
>> --repair is pretty dangerous (and thanks to the work done to repair, it
>> normally doesn't cause catastrophic result, but sometimes it indeed
>> causes extra damage)
> 
> The good news is that online undelete is not as dangerous as btrfs check
> --repair. In fact, I think it is safe enough.

Yes, for the safety part I completely agree.

> 
>>
>> And when user tried and failed due to deleted tree blocks, they will get
>> even more frustrated or even betrayed.
> 
> As mentioned previous, maybe we should do what we think is right, such as
> give the user more abilities to protect/recover their data, not to take
> care of any sensitive users?

From my personal experience, adding a new ioctl means a lot of new
maintenance burden, especially when the need is small (and sometimes
raises user expectation).

Personally speaking, we already have a lot of so-called "optimization"
which makes development pretty tricky.
We should pay more attention when introduce a new feature, evaluating
the user-case and possible complexity carefully, since removing a
feature is way more complex than adding a new one.

(Although thankfully, current undelete is pretty simple, but I'm not
sure if it will become more and more complex)

> 
>>
>> I prefer to put such undelete as an off-line rescue tool, instead of
>> making it online with an ioctl interface.
> 
> I also think that the offline undelete is more useful. After all, umount
> immediately to prevent further data loss is always the most effective after
> a mistake. However, since we can give the ability of online undelete to a
> user which couldn't umount the filesystem easily,

Well, when the deletion happens, there is no much difference to do
online or offline rescue.
Either way, sane user should already stop its business and check the
system carefully.

> and don't have any side
> effect on existing features. IMHO, there is no reason to reject this.

I just don't want to regret several years later.
Currently for undelete, we already need to take a lot of factors into
consideration, (although most of them are not affected) qgroup, balance,
pending snapshot (and I may miss a lot of more).

I just don't want later development to take undelete into consideration
for such a niche use-case.

Thanks,
Qu