| Message ID | 20180805104001.5488-1-lufq.fnst@cn.fujitsu.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | undelete subvolume online version | expand |
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
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.
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 >
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.
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