| Message ID | 20221209142331.26395-1-sergei.shtepa@veeam.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | blksnap - block devices snapshots module | expand |
On Fri, Dec 09, 2022 at 03:23:10PM +0100, Sergei Shtepa wrote: > Sergei Shtepa (21): > documentation, blkfilter: Block Device Filtering Mechanism > block, blkfilter: Block Device Filtering Mechanism > documentation, capability: fix Generic Block Device Capability > documentation, blksnap: Block Devices Snapshots Module > block, blksnap: header file of the module interface > block, blksnap: module management interface functions > block, blksnap: init() and exit() functions > block, blksnap: interaction with sysfs > block, blksnap: attaching and detaching the filter and handling I/O > units > block, blksnap: map of change block tracking > block, blksnap: minimum data storage unit of the original block device > block, blksnap: buffer in memory for the minimum data storage unit > block, blksnap: functions and structures for performing block I/O > operations > block, blksnap: storage for storing difference blocks > block, blksnap: event queue from the difference storage > block, blksnap: owner of information about overwritten blocks of the > original block device > block, blksnap: snapshot image block device > block, blksnap: snapshot > block, blksnap: Kconfig and Makefile > block, blksnap: adds a blksnap to the kernel tree > block, blksnap: adds a maintainer for new files > Per convention in block subsystem (see for example `git log --no-merges -- drivers/block/`), the patch subject prefix should looks like "block component: some patch" (e.g. "blksnap: do something"). Thanks.
Hi Bagas! Thanks a lot for the review. I will definitely correct the shortcomings that you have indicated.
On Fri, Dec 09 2022 at 9:23P -0500, Sergei Shtepa <sergei.shtepa@veeam.com> wrote: > Hi Jens. Hi Jonathan. Hi all. > > I am happy to offer a modified version of the Block Devices Snapshots > Module. It allows to create non-persistent snapshots of any block devices. > The main purpose of such snapshots is to provide backups of block devices. > See more in Documentation/block/blksnap.rst. > > The Block Device Filtering Mechanism is added to the block layer. This > allows to attach and detach block device filters to the block layer. > Filters allow to extend the functionality of the block layer. > See more in Documentation/block/blkfilter.rst. > > A tool, a library for working with blksnap and tests can be found at > www.github.com/veeam/blksnap. > > The first version was suggested at 13 June 2022. Many thanks to > Christoph Hellwig and Randy Dunlap for the review of that version. > > Changes: > - Forgotten "static" declarations have been added. > - The text of the comments has been corrected. > - It is possible to connect only one filter, since there are no others in > upstream. > - Do not have additional locks for attach/detach filter. > - blksnap.h moved to include/uapi/. > - #pragma once and commented code removed. > - uuid_t removed from user API. > - Removed default values for module parameters from the configuration file. > - The debugging code for tracking memory leaks has been removed. > - Simplified Makefile. > - Optimized work with large memory buffers, CBT tables are now in virtual > memory. > - The allocation code of minor numbers has been optimized. > - The implementation of the snapshot image block device has been > simplified, now it is a bio-based block device. > - Removed initialization of global variables with null values. > - Only one bio is used to copy one chunk. > - Checked on ppc64le. > > The v1 version was suggested at 2 November 2022. Many thanks to Fabio > Fantoni for his for his participation in the "blksnap" project on github > and Jonathan Corbet for his article https://lwn.net/Articles/914031/. > Thanks to the impartial kernel test robot. > > Changes: > - Added documentation for Block Device Filtering Mechanism. > - Added documentation for Block Devices Snapshots Module (blksnap). > - The MAINTAINERS file has been updated. > - Optimized queue code for snapshot images. > - Fixed comments, log messages and code for better readability. [this reply got long...] I think it is important to revisit how we've gotten to this point. The catalyst for blkfilter and now blksnap was that I removed the export for blk_mq_submit_bio -- which veeam was using for enablement of their commercial backup software product, the offending commit was 681cc5e8667e ("dm: fix request-based DM to not bounce through indirect dm_submit_bio") The way veeam started to address this change was to request Red Hat modify RHEL (by reverting my change in RHEL8, whereby restoring the export) to give them a stopgap while they worked to identify a more lasting solution to them having depended on such a fragile interface with which to hijack IO for all Linux block devices. They then came up with blk-interposer. I tried to be helpful and replied quite regularly to blk-interposer patchsets, e.g.: https://listman.redhat.com/archives/dm-devel/2021-March/045900.html https://listman.redhat.com/archives/dm-devel/2021-March/045838.html (I won't dig out more pointers, but can if you doubt this assertion). The last reply I got on this topic was very dense and so I tabled it with the idea of revisiting it. But I dropped the ball and never did reply to this: https://listman.redhat.com/archives/dm-devel/2021-April/046184.html Sorry. But that wasn't out of malice. I was just busy with other things and blk-interposer took the back seat. I never imagined that my inaction would foster completely abandoning the approach. But my thanks is I'm now made to defend myself on LWN: https://lwn.net/Articles/920245/ https://lwn.net/Articles/920249/ I happened to trip over that LWN thread because I saw Hannes reference "blksnap" as something that somehow is a tolerated advance but other efforts are not: https://lore.kernel.org/linux-block/06e4d03c-3ecf-7e91-b80e-6600b3618b98@suse.de/ blksnap really needs to stand on its own merits and it could be that in conjunction with blkfilter it does. But the way it has evolved has been antithetical to how to properly engage the Linux community and subsystem mainatiners like myself. The PR campaign to raise awareness with LWN became more important than cc'ing me. That says it all really. But hopefully you can take my words as my truth: I think what you're wanting to do is useful. I never intended to act as some gatekeeper. I don't have a problem with what your goals are, I just ask that _how_ you achieve your goals be done with care and consideration (the attempts I reviewed prior to your most recent work were lacking). But my one and only request for this line of development would be: I _really_ want DM code to be able to used as an endpoint for IO remapping associated with any new block core hook added to accomplish dynamic remapping of IO. If I need to take an active role in the development of that capability, so be it. I've yet to look closely at all this code (but wow there is quite a lot under drivers/block/blksnap). I'll have a look at the block-core changes and then try to make sense of things from there. But you've already bypassed me, my hope is that Jens and Christoph agree that we need this line of development to be in service to other areas of the Linux block subsystem and its drivers that were established for the purposes of remapping IO. It cannot just be the subset needed to cement veeam's ability to use Linux for its purposes (but I completely understand that is the point of veeam's exercise). Mike > > Sergei Shtepa (21): > documentation, blkfilter: Block Device Filtering Mechanism > block, blkfilter: Block Device Filtering Mechanism > documentation, capability: fix Generic Block Device Capability > documentation, blksnap: Block Devices Snapshots Module > block, blksnap: header file of the module interface > block, blksnap: module management interface functions > block, blksnap: init() and exit() functions > block, blksnap: interaction with sysfs > block, blksnap: attaching and detaching the filter and handling I/O > units > block, blksnap: map of change block tracking > block, blksnap: minimum data storage unit of the original block device > block, blksnap: buffer in memory for the minimum data storage unit > block, blksnap: functions and structures for performing block I/O > operations > block, blksnap: storage for storing difference blocks > block, blksnap: event queue from the difference storage > block, blksnap: owner of information about overwritten blocks of the > original block device > block, blksnap: snapshot image block device > block, blksnap: snapshot > block, blksnap: Kconfig and Makefile > block, blksnap: adds a blksnap to the kernel tree > block, blksnap: adds a maintainer for new files > > Documentation/block/blkfilter.rst | 50 ++ > Documentation/block/blksnap.rst | 348 ++++++++++++++ > Documentation/block/capability.rst | 3 + > Documentation/block/index.rst | 2 + > MAINTAINERS | 14 + > block/bdev.c | 70 +++ > block/blk-core.c | 19 +- > drivers/block/Kconfig | 2 + > drivers/block/Makefile | 2 + > drivers/block/blksnap/Kconfig | 12 + > drivers/block/blksnap/Makefile | 18 + > drivers/block/blksnap/cbt_map.c | 268 +++++++++++ > drivers/block/blksnap/cbt_map.h | 114 +++++ > drivers/block/blksnap/chunk.c | 345 ++++++++++++++ > drivers/block/blksnap/chunk.h | 139 ++++++ > drivers/block/blksnap/ctrl.c | 410 ++++++++++++++++ > drivers/block/blksnap/ctrl.h | 9 + > drivers/block/blksnap/diff_area.c | 655 +++++++++++++++++++++++++ > drivers/block/blksnap/diff_area.h | 177 +++++++ > drivers/block/blksnap/diff_buffer.c | 133 ++++++ > drivers/block/blksnap/diff_buffer.h | 75 +++ > drivers/block/blksnap/diff_io.c | 168 +++++++ > drivers/block/blksnap/diff_io.h | 118 +++++ > drivers/block/blksnap/diff_storage.c | 317 +++++++++++++ > drivers/block/blksnap/diff_storage.h | 93 ++++ > drivers/block/blksnap/event_queue.c | 86 ++++ > drivers/block/blksnap/event_queue.h | 63 +++ > drivers/block/blksnap/main.c | 164 +++++++ > drivers/block/blksnap/params.h | 12 + > drivers/block/blksnap/snapimage.c | 275 +++++++++++ > drivers/block/blksnap/snapimage.h | 69 +++ > drivers/block/blksnap/snapshot.c | 670 ++++++++++++++++++++++++++ > drivers/block/blksnap/snapshot.h | 78 +++ > drivers/block/blksnap/sysfs.c | 80 ++++ > drivers/block/blksnap/sysfs.h | 7 + > drivers/block/blksnap/tracker.c | 683 +++++++++++++++++++++++++++ > drivers/block/blksnap/tracker.h | 74 +++ > drivers/block/blksnap/version.h | 10 + > include/linux/blk_types.h | 2 + > include/linux/blkdev.h | 71 +++ > include/uapi/linux/blksnap.h | 549 +++++++++++++++++++++ > 41 files changed, 6452 insertions(+), 2 deletions(-) > create mode 100644 Documentation/block/blkfilter.rst > create mode 100644 Documentation/block/blksnap.rst > create mode 100644 drivers/block/blksnap/Kconfig > create mode 100644 drivers/block/blksnap/Makefile > create mode 100644 drivers/block/blksnap/cbt_map.c > create mode 100644 drivers/block/blksnap/cbt_map.h > create mode 100644 drivers/block/blksnap/chunk.c > create mode 100644 drivers/block/blksnap/chunk.h > create mode 100644 drivers/block/blksnap/ctrl.c > create mode 100644 drivers/block/blksnap/ctrl.h > create mode 100644 drivers/block/blksnap/diff_area.c > create mode 100644 drivers/block/blksnap/diff_area.h > create mode 100644 drivers/block/blksnap/diff_buffer.c > create mode 100644 drivers/block/blksnap/diff_buffer.h > create mode 100644 drivers/block/blksnap/diff_io.c > create mode 100644 drivers/block/blksnap/diff_io.h > create mode 100644 drivers/block/blksnap/diff_storage.c > create mode 100644 drivers/block/blksnap/diff_storage.h > create mode 100644 drivers/block/blksnap/event_queue.c > create mode 100644 drivers/block/blksnap/event_queue.h > create mode 100644 drivers/block/blksnap/main.c > create mode 100644 drivers/block/blksnap/params.h > create mode 100644 drivers/block/blksnap/snapimage.c > create mode 100644 drivers/block/blksnap/snapimage.h > create mode 100644 drivers/block/blksnap/snapshot.c > create mode 100644 drivers/block/blksnap/snapshot.h > create mode 100644 drivers/block/blksnap/sysfs.c > create mode 100644 drivers/block/blksnap/sysfs.h > create mode 100644 drivers/block/blksnap/tracker.c > create mode 100644 drivers/block/blksnap/tracker.h > create mode 100644 drivers/block/blksnap/version.h > create mode 100644 include/uapi/linux/blksnap.h > > -- > 2.20.1 >
On 1/17/23 22:04, Mike Snitzer wrote: > On Fri, Dec 09 2022 at 9:23P -0500, > Sergei Shtepa <sergei.shtepa@veeam.com> wrote: > >> Hi Jens. Hi Jonathan. Hi all. >> >> I am happy to offer a modified version of the Block Devices Snapshots >> Module. It allows to create non-persistent snapshots of any block devices. >> The main purpose of such snapshots is to provide backups of block devices. >> See more in Documentation/block/blksnap.rst. >> >> The Block Device Filtering Mechanism is added to the block layer. This >> allows to attach and detach block device filters to the block layer. >> Filters allow to extend the functionality of the block layer. >> See more in Documentation/block/blkfilter.rst. >> >> A tool, a library for working with blksnap and tests can be found at >> www.github.com/veeam/blksnap. >> >> The first version was suggested at 13 June 2022. Many thanks to >> Christoph Hellwig and Randy Dunlap for the review of that version. >> >> Changes: >> - Forgotten "static" declarations have been added. >> - The text of the comments has been corrected. >> - It is possible to connect only one filter, since there are no others in >> upstream. >> - Do not have additional locks for attach/detach filter. >> - blksnap.h moved to include/uapi/. >> - #pragma once and commented code removed. >> - uuid_t removed from user API. >> - Removed default values for module parameters from the configuration file. >> - The debugging code for tracking memory leaks has been removed. >> - Simplified Makefile. >> - Optimized work with large memory buffers, CBT tables are now in virtual >> memory. >> - The allocation code of minor numbers has been optimized. >> - The implementation of the snapshot image block device has been >> simplified, now it is a bio-based block device. >> - Removed initialization of global variables with null values. >> - Only one bio is used to copy one chunk. >> - Checked on ppc64le. >> >> The v1 version was suggested at 2 November 2022. Many thanks to Fabio >> Fantoni for his for his participation in the "blksnap" project on github >> and Jonathan Corbet for his article https://lwn.net/Articles/914031/. >> Thanks to the impartial kernel test robot. >> >> Changes: >> - Added documentation for Block Device Filtering Mechanism. >> - Added documentation for Block Devices Snapshots Module (blksnap). >> - The MAINTAINERS file has been updated. >> - Optimized queue code for snapshot images. >> - Fixed comments, log messages and code for better readability. > > [this reply got long...] > [ .. ] > > But you've already bypassed me, my hope is that Jens and Christoph > agree that we need this line of development to be in service to other > areas of the Linux block subsystem and its drivers that were > established for the purposes of remapping IO. It cannot just be > the subset needed to cement veeam's ability to use Linux for its > purposes (but I completely understand that is the point of veeam's > exercise). > That's why I proposed my topic at LSF/MM, precisely to figure out how to handle these issues. Cheers, Hannes
DearMike I am very glad that you noticed my patch version 2. I'm sure your experience will help. On 1/17/23 22:04, Mike Snitzer wrote: > Subject: > Re: [PATCH v2 00/21] blksnap - block devices snapshots module > From: > Mike Snitzer <snitzer@redhat.com> > Date: > 1/17/23, 22:04 > > To: > Sergei Shtepa <sergei.shtepa@veeam.com> > CC: > "axboe@kernel.dk" <axboe@kernel.dk>, "corbet@lwn.net" <corbet@lwn.net>, "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>, "linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org> > > > On Fri, Dec 09 2022 at 9:23P -0500, > Sergei Shtepa wrote: > > > Hi Jens. Hi Jonathan. Hi all. > > > > I am happy to offer a modified version of the Block Devices Snapshots > > Module. It allows to create non-persistent snapshots of any block devices. > > The main purpose of such snapshots is to provide backups of block devices. > > See more in Documentation/block/blksnap.rst. > > > > The Block Device Filtering Mechanism is added to the block layer. This > > allows to attach and detach block device filters to the block layer. > > Filters allow to extend the functionality of the block layer. > > See more in Documentation/block/blkfilter.rst. > > > > A tool, a library for working with blksnap and tests can be found at > > https://github.com/veeam/blksnap. > > > > The first version was suggested at 13 June 2022. Many thanks to > > Christoph Hellwig and Randy Dunlap for the review of that version. > > > > Changes: > > - Forgotten "static" declarations have been added. > > - The text of the comments has been corrected. > > - It is possible to connect only one filter, since there are no others in > > upstream. > > - Do not have additional locks for attach/detach filter. > > - blksnap.h moved to include/uapi/. > > - #pragma once and commented code removed. > > - uuid_t removed from user API. > > - Removed default values for module parameters from the configuration file. > > - The debugging code for tracking memory leaks has been removed. > > - Simplified Makefile. > > - Optimized work with large memory buffers, CBT tables are now in virtual > > memory. > > - The allocation code of minor numbers has been optimized. > > - The implementation of the snapshot image block device has been > > simplified, now it is a bio-based block device. > > - Removed initialization of global variables with null values. > > - Only one bio is used to copy one chunk. > > - Checked on ppc64le. > > > > The v1 version was suggested at 2 November 2022. Many thanks to Fabio > > Fantoni for his for his participation in the "blksnap" project on github > > and Jonathan Corbet for his article https://lwn.net/Articles/914031/. > > Thanks to the impartial kernel test robot. > > > > Changes: > > - Added documentation for Block Device Filtering Mechanism. > > - Added documentation for Block Devices Snapshots Module (blksnap). > > - The MAINTAINERS file has been updated. > > - Optimized queue code for snapshot images. > > - Fixed comments, log messages and code for better readability. > > [this reply got long...] > > I think it is important to revisit how we've gotten to this point. > The catalyst for blkfilter and now blksnap was that I removed the > export for blk_mq_submit_bio -- which veeam was using for enablement > of their commercial backup software product, the offending commit was > 681cc5e8667e ("dm: fix request-based DM to not bounce through indirect > dm_submit_bio") > > The way veeam started to address this change was to request Red Hat > modify RHEL (by reverting my change in RHEL8, whereby restoring the > export) to give them a stopgap while they worked to identify a more > lasting solution to them having depended on such a fragile interface > with which to hijack IO for all Linux block devices. > Thank you so much for your help with the function blk_mq_submit_bio(). This fix helped the veeamsnap module to continue working on RHEL 8 with a minimal set of crutches. But besides RHEL 8, there are other distributions that required support. The catalyst was the optimization of Christoph and the removal of the make_request_fn() callback function in v5.9. See: https://elixir.bootlin.com/linux/v5.8.18/source/include/linux/blkdev.h#L406 Overriding this function allowed handling I/O units. > They then came up with blk-interposer. I tried to be helpful and > replied quite regularly to blk-interposer patchsets, e.g.: > https://listman.redhat.com/archives/dm-devel/2021-March/045900.html > https://listman.redhat.com/archives/dm-devel/2021-March/045838.html > (I won't dig out more pointers, but can if you doubt this assertion). > The last reply I got on this topic was very dense and so I > tabled it with the idea of revisiting it. But I dropped the ball and > never did reply to this: > https://listman.redhat.com/archives/dm-devel/2021-April/046184.html > > Sorry. But that wasn't out of malice. I was just busy with other > things and blk-interposer took the back seat. I never imagined that my > inaction would foster completely abandoning the approach. > And yes, I’ve spent several months trying to refine the DM in order to implement the feature of attaching DM devices on the fly, that you said you would like to have in DM. Alas, I have not achieved success. I figured that it would take a lot of changes in the DM code before its non-persistent snapshots can be used for backup, and some architectural changes will be required. Although you have been very helpful for a while, at some point you stopped providing any feedback and thus I started to explore other possibilities to solve the problem. Seeing your annoyance now, I wonder what would be the better way to continue my work in the given circumstances? What kind of action would you expect from me in such situation? Being left without any feedback and guidance on DM, I decided to work directly with the community to find better solution. That is, I took another route (blksnap) not because I thought the initial DM-based approach proposed by you was inferior or bad, but because only this direction remained available to me. > But my thanks is I'm now made to defend myself on LWN: > https://lwn.net/Articles/920245/ > https://lwn.net/Articles/920249/ > > I happened to trip over that LWN thread because I saw Hannes reference > "blksnap" as something that somehow is a tolerated advance but other > efforts are not: > https://lore.kernel.org/linux-block/06e4d03c-3ecf-7e91-b80e-6600b3618b98@suse.de/ > > blksnap really needs to stand on its own merits and it could be that > in conjunction with blkfilter it does. But the way it has evolved has > been antithetical to how to properly engage the Linux community and > subsystem mainatiners like myself. The PR campaign to raise awareness > with LWN became more important than cc'ing me. That says it all > really. > As for the article on LWN: I am very grateful to Jonathan for his article. It attract some attention to my work. However, it’s wasn’t a deliberate PR company from Veeam. In fact, Veeam has nothing to do with the article. Yes, I work for the company, but the company has neither requested the article, nor has started any other PR regarding the matter. If I were organizing a PR campaign, the article would be similar to this: https://github.com/veeam/blksnap/blob/master/doc/Something-wrong-with-snapshots-for-Linux.md But be careful! In the article I try to change the reader's opinion about snapshots. Feedback, as usual, is welcome. > But hopefully you can take my words as my truth: I think what you're > wanting to do is useful. I never intended to act as some gatekeeper. I > don't have a problem with what your goals are, I just ask that _how_ > you achieve your goals be done with care and consideration (the > attempts I reviewed prior to your most recent work were lacking). > Mike, I assure you, I have one goal. Snapshots of block devices in the Linux kernel for backup. I think we got off to a good start initially, but then something went wrong (miscommunications, misunderstanding, and a lack of time) so here we are. If you could clarify what could have been the proper way of dealing with that situation from my side, I would really appreciate that. > But my one and only request for this line of development would be: I > _really_ want DM code to be able to used as an endpoint for IO > remapping associated with any new block core hook added to accomplish > dynamic remapping of IO. If I need to take an active role in the > development of that capability, so be it. > I am sure that this is quite achievable. If the dm-dev team has special needs when attaching DM devices via blkfilter, we will be able to upgrade it. At the moment I don't see any problems with this. I can promise you to add you to СС when sending next patches. Sounds good? > I've yet to look closely at all this code (but wow there is quite a > lot under drivers/block/blksnap). I'll have a look at the block-core > changes and then try to make sense of things from there. > Yes, that's right. There are quite a few changes in block-core. From the blksnap, it is enough to view tracker.c. I/O handling is implemented there. But it's probably better to wait for the next version of patch that takes into account Christoph's comments. There are a lot of changes, and first of all in the interface. > But you've already bypassed me, my hope is that Jens and Christoph > agree that we need this line of development to be in service to other > areas of the Linux block subsystem and its drivers that were > established for the purposes of remapping IO. It cannot just be > the subset needed to cement veeam's ability to use Linux for its > purposes (but I completely understand that is the point of veeam's > exercise). > > Mike It’s not about Veeam at all. I am sure that my work will help many backup vendors and average users to build more robust and efficient backup tools. So, the argument that I do it just because Veeam needs it does not hold any water – I know that many people need the feature, not just Veeam. > > > > > > > Sergei Shtepa (21): > > documentation, blkfilter: Block Device Filtering Mechanism > > block, blkfilter: Block Device Filtering Mechanism > > documentation, capability: fix Generic Block Device Capability > > documentation, blksnap: Block Devices Snapshots Module > > block, blksnap: header file of the module interface > > block, blksnap: module management interface functions > > block, blksnap: init() and exit() functions > > block, blksnap: interaction with sysfs > > block, blksnap: attaching and detaching the filter and handling I/O > > units > > block, blksnap: map of change block tracking > > block, blksnap: minimum data storage unit of the original block device > > block, blksnap: buffer in memory for the minimum data storage unit > > block, blksnap: functions and structures for performing block I/O > > operations > > block, blksnap: storage for storing difference blocks > > block, blksnap: event queue from the difference storage > > block, blksnap: owner of information about overwritten blocks of the > > original block device > > block, blksnap: snapshot image block device > > block, blksnap: snapshot > > block, blksnap: Kconfig and Makefile > > block, blksnap: adds a blksnap to the kernel tree > > block, blksnap: adds a maintainer for new files > > > > Documentation/block/blkfilter.rst | 50 ++ > > Documentation/block/blksnap.rst | 348 ++++++++++++++ > > Documentation/block/capability.rst | 3 + > > Documentation/block/index.rst | 2 + > > MAINTAINERS | 14 + > > block/bdev.c | 70 +++ > > block/blk-core.c | 19 +- > > drivers/block/Kconfig | 2 + > > drivers/block/Makefile | 2 + > > drivers/block/blksnap/Kconfig | 12 + > > drivers/block/blksnap/Makefile | 18 + > > drivers/block/blksnap/cbt_map.c | 268 +++++++++++ > > drivers/block/blksnap/cbt_map.h | 114 +++++ > > drivers/block/blksnap/chunk.c | 345 ++++++++++++++ > > drivers/block/blksnap/chunk.h | 139 ++++++ > > drivers/block/blksnap/ctrl.c | 410 ++++++++++++++++ > > drivers/block/blksnap/ctrl.h | 9 + > > drivers/block/blksnap/diff_area.c | 655 +++++++++++++++++++++++++ > > drivers/block/blksnap/diff_area.h | 177 +++++++ > > drivers/block/blksnap/diff_buffer.c | 133 ++++++ > > drivers/block/blksnap/diff_buffer.h | 75 +++ > > drivers/block/blksnap/diff_io.c | 168 +++++++ > > drivers/block/blksnap/diff_io.h | 118 +++++ > > drivers/block/blksnap/diff_storage.c | 317 +++++++++++++ > > drivers/block/blksnap/diff_storage.h | 93 ++++ > > drivers/block/blksnap/event_queue.c | 86 ++++ > > drivers/block/blksnap/event_queue.h | 63 +++ > > drivers/block/blksnap/main.c | 164 +++++++ > > drivers/block/blksnap/params.h | 12 + > > drivers/block/blksnap/snapimage.c | 275 +++++++++++ > > drivers/block/blksnap/snapimage.h | 69 +++ > > drivers/block/blksnap/snapshot.c | 670 ++++++++++++++++++++++++++ > > drivers/block/blksnap/snapshot.h | 78 +++ > > drivers/block/blksnap/sysfs.c | 80 ++++ > > drivers/block/blksnap/sysfs.h | 7 + > > drivers/block/blksnap/tracker.c | 683 +++++++++++++++++++++++++++ > > drivers/block/blksnap/tracker.h | 74 +++ > > drivers/block/blksnap/version.h | 10 + > > include/linux/blk_types.h | 2 + > > include/linux/blkdev.h | 71 +++ > > include/uapi/linux/blksnap.h | 549 +++++++++++++++++++++ > > 41 files changed, 6452 insertions(+), 2 deletions(-) > > create mode 100644 Documentation/block/blkfilter.rst > > create mode 100644 Documentation/block/blksnap.rst > > create mode 100644 drivers/block/blksnap/Kconfig > > create mode 100644 drivers/block/blksnap/Makefile > > create mode 100644 drivers/block/blksnap/cbt_map.c > > create mode 100644 drivers/block/blksnap/cbt_map.h > > create mode 100644 drivers/block/blksnap/chunk.c > > create mode 100644 drivers/block/blksnap/chunk.h > > create mode 100644 drivers/block/blksnap/ctrl.c > > create mode 100644 drivers/block/blksnap/ctrl.h > > create mode 100644 drivers/block/blksnap/diff_area.c > > create mode 100644 drivers/block/blksnap/diff_area.h > > create mode 100644 drivers/block/blksnap/diff_buffer.c > > create mode 100644 drivers/block/blksnap/diff_buffer.h > > create mode 100644 drivers/block/blksnap/diff_io.c > > create mode 100644 drivers/block/blksnap/diff_io.h > > create mode 100644 drivers/block/blksnap/diff_storage.c > > create mode 100644 drivers/block/blksnap/diff_storage.h > > create mode 100644 drivers/block/blksnap/event_queue.c > > create mode 100644 drivers/block/blksnap/event_queue.h > > create mode 100644 drivers/block/blksnap/main.c > > create mode 100644 drivers/block/blksnap/params.h > > create mode 100644 drivers/block/blksnap/snapimage.c > > create mode 100644 drivers/block/blksnap/snapimage.h > > create mode 100644 drivers/block/blksnap/snapshot.c > > create mode 100644 drivers/block/blksnap/snapshot.h > > create mode 100644 drivers/block/blksnap/sysfs.c > > create mode 100644 drivers/block/blksnap/sysfs.h > > create mode 100644 drivers/block/blksnap/tracker.c > > create mode 100644 drivers/block/blksnap/tracker.h > > create mode 100644 drivers/block/blksnap/version.h > > create mode 100644 include/uapi/linux/blksnap.h > > > > -- > > 2.20.1 > > >
On Tue, Jan 24 2023 at 6:34P -0500, Sergei Shtepa <sergei.shtepa@veeam.com> wrote: > DearMike > > I am very glad that you noticed my patch version 2. > I'm sure your experience will help. > > On 1/17/23 22:04, Mike Snitzer wrote: > > Subject: > > Re: [PATCH v2 00/21] blksnap - block devices snapshots module > > From: > > Mike Snitzer <snitzer@redhat.com> > > Date: > > 1/17/23, 22:04 > > > > To: > > Sergei Shtepa <sergei.shtepa@veeam.com> > > CC: > > "axboe@kernel.dk" <axboe@kernel.dk>, "corbet@lwn.net" <corbet@lwn.net>, "linux-block@vger.kernel.org" <linux-block@vger.kernel.org>, "linux-doc@vger.kernel.org" <linux-doc@vger.kernel.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org> > > > > > > On Fri, Dec 09 2022 at 9:23P -0500, > > Sergei Shtepa wrote: > > > > > Hi Jens. Hi Jonathan. Hi all. > > > > > > I am happy to offer a modified version of the Block Devices Snapshots > > > Module. It allows to create non-persistent snapshots of any block devices. > > > The main purpose of such snapshots is to provide backups of block devices. > > > See more in Documentation/block/blksnap.rst. > > > > > > The Block Device Filtering Mechanism is added to the block layer. This > > > allows to attach and detach block device filters to the block layer. > > > Filters allow to extend the functionality of the block layer. > > > See more in Documentation/block/blkfilter.rst. > > > > > > A tool, a library for working with blksnap and tests can be found at > > > https://github.com/veeam/blksnap. > > > > > > The first version was suggested at 13 June 2022. Many thanks to > > > Christoph Hellwig and Randy Dunlap for the review of that version. > > > > > > Changes: > > > - Forgotten "static" declarations have been added. > > > - The text of the comments has been corrected. > > > - It is possible to connect only one filter, since there are no others in > > > upstream. > > > - Do not have additional locks for attach/detach filter. > > > - blksnap.h moved to include/uapi/. > > > - #pragma once and commented code removed. > > > - uuid_t removed from user API. > > > - Removed default values for module parameters from the configuration file. > > > - The debugging code for tracking memory leaks has been removed. > > > - Simplified Makefile. > > > - Optimized work with large memory buffers, CBT tables are now in virtual > > > memory. > > > - The allocation code of minor numbers has been optimized. > > > - The implementation of the snapshot image block device has been > > > simplified, now it is a bio-based block device. > > > - Removed initialization of global variables with null values. > > > - Only one bio is used to copy one chunk. > > > - Checked on ppc64le. > > > > > > The v1 version was suggested at 2 November 2022. Many thanks to Fabio > > > Fantoni for his for his participation in the "blksnap" project on github > > > and Jonathan Corbet for his article https://lwn.net/Articles/914031/. > > > Thanks to the impartial kernel test robot. > > > > > > Changes: > > > - Added documentation for Block Device Filtering Mechanism. > > > - Added documentation for Block Devices Snapshots Module (blksnap). > > > - The MAINTAINERS file has been updated. > > > - Optimized queue code for snapshot images. > > > - Fixed comments, log messages and code for better readability. > > > > [this reply got long...] > > > > I think it is important to revisit how we've gotten to this point. > > The catalyst for blkfilter and now blksnap was that I removed the > > export for blk_mq_submit_bio -- which veeam was using for enablement > > of their commercial backup software product, the offending commit was > > 681cc5e8667e ("dm: fix request-based DM to not bounce through indirect > > dm_submit_bio") > > > > The way veeam started to address this change was to request Red Hat > > modify RHEL (by reverting my change in RHEL8, whereby restoring the > > export) to give them a stopgap while they worked to identify a more > > lasting solution to them having depended on such a fragile interface > > with which to hijack IO for all Linux block devices. > > > > Thank you so much for your help with the function blk_mq_submit_bio(). > This fix helped the veeamsnap module to continue working on RHEL 8 > with a minimal set of crutches. But besides RHEL 8, there are other > distributions that required support. > > The catalyst was the optimization of Christoph and the removal of > the make_request_fn() callback function in v5.9. See: > https://elixir.bootlin.com/linux/v5.8.18/source/include/linux/blkdev.h#L406 > Overriding this function allowed handling I/O units. > > > They then came up with blk-interposer. I tried to be helpful and > > replied quite regularly to blk-interposer patchsets, e.g.: > > https://listman.redhat.com/archives/dm-devel/2021-March/045900.html > > https://listman.redhat.com/archives/dm-devel/2021-March/045838.html > > (I won't dig out more pointers, but can if you doubt this assertion). > > The last reply I got on this topic was very dense and so I > > tabled it with the idea of revisiting it. But I dropped the ball and > > never did reply to this: > > https://listman.redhat.com/archives/dm-devel/2021-April/046184.html > > > > Sorry. But that wasn't out of malice. I was just busy with other > > things and blk-interposer took the back seat. I never imagined that my > > inaction would foster completely abandoning the approach. > > > > And yes, I’ve spent several months trying to refine the DM in order to > implement the feature of attaching DM devices on the fly, that you said you > would like to have in DM. Alas, I have not achieved success. I figured that > it would take a lot of changes in the DM code before its non-persistent > snapshots can be used for backup, and some architectural changes will be > required. Although you have been very helpful for a while, at some point > you stopped providing any feedback and thus I started to explore other > possibilities to solve the problem. > > Seeing your annoyance now, I wonder what would be the better way to continue > my work in the given circumstances? What kind of action would you expect from > me in such situation? Being left without any feedback and guidance on DM, > I decided to work directly with the community to find better solution. > > That is, I took another route (blksnap) not because I thought the initial > DM-based approach proposed by you was inferior or bad, but because only this > direction remained available to me. I'm late to respond to your email again because Red Hat now imposes gmail and its mail filtering is abysmal. I should just use snitzer@kernel.org since it is able to ensure I am aware of replies.. anyway, that's my problem to sort out! ;) You're fine to approach the implementation however you and others think best. I have a couple core points I'll raise in the block core patch. But yes, I was hoping we could effectively make it so that any block device could be augmented to remap bios (either in terms of existing DM targets or a more minimalist approach that meets your snapshot needs). Feels somewhat like failure to not be able to make that advancement, but nothing is "done" yet... let's see what we can do. > > But my thanks is I'm now made to defend myself on LWN: > > https://lwn.net/Articles/920245/ > > https://lwn.net/Articles/920249/ > > > > I happened to trip over that LWN thread because I saw Hannes reference > > "blksnap" as something that somehow is a tolerated advance but other > > efforts are not: > > https://lore.kernel.org/linux-block/06e4d03c-3ecf-7e91-b80e-6600b3618b98@suse.de/ > > > > blksnap really needs to stand on its own merits and it could be that > > in conjunction with blkfilter it does. But the way it has evolved has > > been antithetical to how to properly engage the Linux community and > > subsystem mainatiners like myself. The PR campaign to raise awareness > > with LWN became more important than cc'ing me. That says it all > > really. > > > > As for the article on LWN: > > I am very grateful to Jonathan for his article. It attract some attention > to my work. However, it’s wasn’t a deliberate PR company from Veeam. > In fact, Veeam has nothing to do with the article. > > Yes, I work for the company, but the company has neither requested the > article, nor has started any other PR regarding the matter. > > If I were organizing a PR campaign, the article would be similar to this: > https://github.com/veeam/blksnap/blob/master/doc/Something-wrong-with-snapshots-for-Linux.md > But be careful! In the article I try to change the reader's opinion about > snapshots. Feedback, as usual, is welcome. We can just move past all that. No lasting issue. I took the comments about DM in that lwn article too personal. > > But hopefully you can take my words as my truth: I think what you're > > wanting to do is useful. I never intended to act as some gatekeeper. I > > don't have a problem with what your goals are, I just ask that _how_ > > you achieve your goals be done with care and consideration (the > > attempts I reviewed prior to your most recent work were lacking). > > > > Mike, I assure you, I have one goal. Snapshots of block devices in the > Linux kernel for backup. > > I think we got off to a good start initially, but then something went wrong > (miscommunications, misunderstanding, and a lack of time) so here we are. > If you could clarify what could have been the proper way of dealing with > that situation from my side, I would really appreciate that. I will have to look closer at how you're able to ensure consistency between upper layer (FS) and the block layer. The fsfreeze hooks were added long ago with FS-to-block consistency (ensuring IO flushed and halted, etc). > > But my one and only request for this line of development would be: I > > _really_ want DM code to be able to used as an endpoint for IO > > remapping associated with any new block core hook added to accomplish > > dynamic remapping of IO. If I need to take an active role in the > > development of that capability, so be it. > > > > I am sure that this is quite achievable. If the dm-dev team has special > needs when attaching DM devices via blkfilter, we will be able to upgrade it. > At the moment I don't see any problems with this. > I can promise you to add you to СС when sending next patches. > > Sounds good? Sure. > > I've yet to look closely at all this code (but wow there is quite a > > lot under drivers/block/blksnap). I'll have a look at the block-core > > changes and then try to make sense of things from there. > > > > Yes, that's right. There are quite a few changes in block-core. > From the blksnap, it is enough to view tracker.c. I/O handling is > implemented there. > But it's probably better to wait for the next version of patch that takes > into account Christoph's comments. There are a lot of changes, and first > of all in the interface. OK, I'll still reply to the block core patch now-ish. > > But you've already bypassed me, my hope is that Jens and Christoph > > agree that we need this line of development to be in service to other > > areas of the Linux block subsystem and its drivers that were > > established for the purposes of remapping IO. It cannot just be > > the subset needed to cement veeam's ability to use Linux for its > > purposes (but I completely understand that is the point of veeam's > > exercise). > > It’s not about Veeam at all. I am sure that my work will help many backup > vendors and average users to build more robust and efficient backup tools. > So, the argument that I do it just because Veeam needs it does not hold > any water – I know that many people need the feature, not just Veeam. No other snapshot consumers have shown themselves. Using them as some sort of implied consensus on what is needed for generic Linux snapshot is a bit of a leap. All you really have are your requirements. Doesn't really help to say you represent the interests of all interested parties if they remain nameless and in the background. Mike
Hi Mike and Sergei, > > It’s not about Veeam at all. I am sure that my work will help many > > backup vendors and average users to build more robust and efficient backup tools. > > So, the argument that I do it just because Veeam needs it does not > > hold any water – I know that many people need the feature, not just Veeam. > > No other snapshot consumers have shown themselves. Using them as some sort of implied consensus on what is needed for generic Linux snapshot is a bit of a leap. All you really have are your requirements. Doesn't really help to say you represent the interests of all interested parties if they remain nameless and in the background. I'm speaking on behalf of Comet Backup, another commercial vendor in this space. I just want to chime in and say we're very closely following the development of this patch set. Our end-user customers have an "arbitrary" Linux system where we don't control the filesystem or block device layer. Having a point-in-time consistent snapshot is essential for a high quality backup, and it's table-stakes on Windows with the VSS subsystem. But a very large number of installs in-the-wild are using plain ext4 without lvm, and we have no remaining viable mechanisms for either filesystem-level or block device-level backup. For this use case, the snapshots are generally short-lived. The underlying mechanism doesn't affect us much - whether it's live-swapping DM devices, or explicitly tracking bio's (or if somehow ext4 and xfs magically got fs-level snapshotting support). But most of all, we'd love to see something upstream. I'd also point you towards Datto's out-of-tree 'dattobd' block device driver with the same objective: https://github.com/datto/dattobd (LGPLv2+). It was working in a slightly different way by using ftrace to hook submit_bio_noacct. Regards, Mason Giles
Hi Jens. Hi Jonathan. Hi all. I am happy to offer a modified version of the Block Devices Snapshots Module. It allows to create non-persistent snapshots of any block devices. The main purpose of such snapshots is to provide backups of block devices. See more in Documentation/block/blksnap.rst. The Block Device Filtering Mechanism is added to the block layer. This allows to attach and detach block device filters to the block layer. Filters allow to extend the functionality of the block layer. See more in Documentation/block/blkfilter.rst. A tool, a library for working with blksnap and tests can be found at www.github.com/veeam/blksnap. The first version was suggested at 13 June 2022. Many thanks to Christoph Hellwig and Randy Dunlap for the review of that version. Changes: - Forgotten "static" declarations have been added. - The text of the comments has been corrected. - It is possible to connect only one filter, since there are no others in upstream. - Do not have additional locks for attach/detach filter. - blksnap.h moved to include/uapi/. - #pragma once and commented code removed. - uuid_t removed from user API. - Removed default values for module parameters from the configuration file. - The debugging code for tracking memory leaks has been removed. - Simplified Makefile. - Optimized work with large memory buffers, CBT tables are now in virtual memory. - The allocation code of minor numbers has been optimized. - The implementation of the snapshot image block device has been simplified, now it is a bio-based block device. - Removed initialization of global variables with null values. - Only one bio is used to copy one chunk. - Checked on ppc64le. The v1 version was suggested at 2 November 2022. Many thanks to Fabio Fantoni for his for his participation in the "blksnap" project on github and Jonathan Corbet for his article https://lwn.net/Articles/914031/. Thanks to the impartial kernel test robot. Changes: - Added documentation for Block Device Filtering Mechanism. - Added documentation for Block Devices Snapshots Module (blksnap). - The MAINTAINERS file has been updated. - Optimized queue code for snapshot images. - Fixed comments, log messages and code for better readability. Sergei Shtepa (21): documentation, blkfilter: Block Device Filtering Mechanism block, blkfilter: Block Device Filtering Mechanism documentation, capability: fix Generic Block Device Capability documentation, blksnap: Block Devices Snapshots Module block, blksnap: header file of the module interface block, blksnap: module management interface functions block, blksnap: init() and exit() functions block, blksnap: interaction with sysfs block, blksnap: attaching and detaching the filter and handling I/O units block, blksnap: map of change block tracking block, blksnap: minimum data storage unit of the original block device block, blksnap: buffer in memory for the minimum data storage unit block, blksnap: functions and structures for performing block I/O operations block, blksnap: storage for storing difference blocks block, blksnap: event queue from the difference storage block, blksnap: owner of information about overwritten blocks of the original block device block, blksnap: snapshot image block device block, blksnap: snapshot block, blksnap: Kconfig and Makefile block, blksnap: adds a blksnap to the kernel tree block, blksnap: adds a maintainer for new files Documentation/block/blkfilter.rst | 50 ++ Documentation/block/blksnap.rst | 348 ++++++++++++++ Documentation/block/capability.rst | 3 + Documentation/block/index.rst | 2 + MAINTAINERS | 14 + block/bdev.c | 70 +++ block/blk-core.c | 19 +- drivers/block/Kconfig | 2 + drivers/block/Makefile | 2 + drivers/block/blksnap/Kconfig | 12 + drivers/block/blksnap/Makefile | 18 + drivers/block/blksnap/cbt_map.c | 268 +++++++++++ drivers/block/blksnap/cbt_map.h | 114 +++++ drivers/block/blksnap/chunk.c | 345 ++++++++++++++ drivers/block/blksnap/chunk.h | 139 ++++++ drivers/block/blksnap/ctrl.c | 410 ++++++++++++++++ drivers/block/blksnap/ctrl.h | 9 + drivers/block/blksnap/diff_area.c | 655 +++++++++++++++++++++++++ drivers/block/blksnap/diff_area.h | 177 +++++++ drivers/block/blksnap/diff_buffer.c | 133 ++++++ drivers/block/blksnap/diff_buffer.h | 75 +++ drivers/block/blksnap/diff_io.c | 168 +++++++ drivers/block/blksnap/diff_io.h | 118 +++++ drivers/block/blksnap/diff_storage.c | 317 +++++++++++++ drivers/block/blksnap/diff_storage.h | 93 ++++ drivers/block/blksnap/event_queue.c | 86 ++++ drivers/block/blksnap/event_queue.h | 63 +++ drivers/block/blksnap/main.c | 164 +++++++ drivers/block/blksnap/params.h | 12 + drivers/block/blksnap/snapimage.c | 275 +++++++++++ drivers/block/blksnap/snapimage.h | 69 +++ drivers/block/blksnap/snapshot.c | 670 ++++++++++++++++++++++++++ drivers/block/blksnap/snapshot.h | 78 +++ drivers/block/blksnap/sysfs.c | 80 ++++ drivers/block/blksnap/sysfs.h | 7 + drivers/block/blksnap/tracker.c | 683 +++++++++++++++++++++++++++ drivers/block/blksnap/tracker.h | 74 +++ drivers/block/blksnap/version.h | 10 + include/linux/blk_types.h | 2 + include/linux/blkdev.h | 71 +++ include/uapi/linux/blksnap.h | 549 +++++++++++++++++++++ 41 files changed, 6452 insertions(+), 2 deletions(-) create mode 100644 Documentation/block/blkfilter.rst create mode 100644 Documentation/block/blksnap.rst create mode 100644 drivers/block/blksnap/Kconfig create mode 100644 drivers/block/blksnap/Makefile create mode 100644 drivers/block/blksnap/cbt_map.c create mode 100644 drivers/block/blksnap/cbt_map.h create mode 100644 drivers/block/blksnap/chunk.c create mode 100644 drivers/block/blksnap/chunk.h create mode 100644 drivers/block/blksnap/ctrl.c create mode 100644 drivers/block/blksnap/ctrl.h create mode 100644 drivers/block/blksnap/diff_area.c create mode 100644 drivers/block/blksnap/diff_area.h create mode 100644 drivers/block/blksnap/diff_buffer.c create mode 100644 drivers/block/blksnap/diff_buffer.h create mode 100644 drivers/block/blksnap/diff_io.c create mode 100644 drivers/block/blksnap/diff_io.h create mode 100644 drivers/block/blksnap/diff_storage.c create mode 100644 drivers/block/blksnap/diff_storage.h create mode 100644 drivers/block/blksnap/event_queue.c create mode 100644 drivers/block/blksnap/event_queue.h create mode 100644 drivers/block/blksnap/main.c create mode 100644 drivers/block/blksnap/params.h create mode 100644 drivers/block/blksnap/snapimage.c create mode 100644 drivers/block/blksnap/snapimage.h create mode 100644 drivers/block/blksnap/snapshot.c create mode 100644 drivers/block/blksnap/snapshot.h create mode 100644 drivers/block/blksnap/sysfs.c create mode 100644 drivers/block/blksnap/sysfs.h create mode 100644 drivers/block/blksnap/tracker.c create mode 100644 drivers/block/blksnap/tracker.h create mode 100644 drivers/block/blksnap/version.h create mode 100644 include/uapi/linux/blksnap.h