[v2,00/21] blksnap - block devices snapshots module

  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
  

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 1/1/23 08:18, Hillf Danton wrote:
> Subject:
> Re: [PATCH v2 17/21] block, blksnap: snapshot image block device
> From:
> Hillf Danton <hdanton@sina.com>
> Date:
> 1/1/23, 08:18
> 
> To:
> Sergei Shtepa <sergei.shtepa@veeam.com>
> CC:
> "axboe@kernel.dk" <axboe@kernel.dk>, "corbet@lwn.net" <corbet@lwn.net>, "linux-mm@kvack.org" <linux-mm@kvack.org>, "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
> 
> 
> This is the first time you've received an email from this sender 
> hdanton@sina.com, please exercise caution when clicking on links or opening 
> attachments.
> 
> 
> On 9 Dec 2022 15:23:27 +0100 Sergei Shtepa
>  > Provides the operation of block devices of snapshot images. Read and
>  > write operations are redirected to the regions of difference blocks for
>  > block device (struct diff_area).
>  >
>  > Signed-off-by: Sergei Shtepa
>  > ---
> 
> Thanks for your patchset.

Thanks for the review.

> 
>  > +static int snapimage_kthread_worker_fn(void *param)
>  > +{
>  > + struct snapimage *snapimage = param;
>  > + struct bio *bio;
>  > + int ret = 0;
>  > +
>  > + while (!kthread_should_stop()) {
>  > + bio = get_bio_from_queue(snapimage);
>  > + if (!bio) {
>  > + schedule_timeout_interruptible(HZ / 100);
>  > + continue;
>  > + }
> 
> Given the wake_up_process() below, s$HZ / 100$HZ * 1000$ to avoid
> unnecessary wakeups [1].
> 
> And because of no signal handling added, use schedule_timeout_idle() instead.

Yes, this function will be rewritten to eliminate unnecessary wake-ups.
The code that Christoph proposed in the letter at Dec 15 should work well.

> 
> [1] 
> https://nam10.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F20210419085027.761150-2-elver%40google.com%2F&data=05%7C01%7Csergei.shtepa%40veeam.com%7C765d591d47ad4d93857208daebc92508%7Cba07baab431b49edadd7cbc3542f5140%7C1%7C1%7C638081546440884849%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=rbINOq7m31DFhjHLnQKFHQuoWB64ZhmnWbAVLuNwv7w%3D&reserved=0
> 
>  > +
>  > + snapimage_process_bio(snapimage, bio);
>  > + }
>  > +
>  > + while ((bio = get_bio_from_queue(snapimage)))
>  > + snapimage_process_bio(snapimage, bio);
>  > +
>  > + return ret;
>  > +}
>  > +
>  > +static void snapimage_submit_bio(struct bio *bio)
>  > +{
>  > + struct snapimage *snapimage = bio->bi_bdev->bd_disk->private_data;
>  > + gfp_t gfp = GFP_NOIO;
>  > +
>  > + if (bio->bi_opf & REQ_NOWAIT)
>  > + gfp |= GFP_NOWAIT;
>  > + if (snapimage->is_ready) {
>  > + spin_lock(&snapimage->queue_lock);
>  > + bio_list_add(&snapimage->queue, bio);
>  > + spin_unlock(&snapimage->queue_lock);
>  > +
>  > + wake_up_process(snapimage->worker);
>  > + } else
>  > + bio_io_error(bio);
>  > +}
>
  

On 1/1/23 12:05, Hillf Danton wrote:
> Subject:
> Re: [PATCH v2 18/21] block, blksnap: snapshot
> From:
> Hillf Danton <hdanton@sina.com>
> Date:
> 1/1/23, 12:05
> 
> To:
> Sergei Shtepa <sergei.shtepa@veeam.com>
> CC:
> axboe@kernel.dk, corbet@lwn.net, linux-mm@kvack.org, linux-kernel@vger.kernel.org
> 
> 
> On 9 Dec 2022 15:23:28 +0100 Sergei Shtepa <sergei.shtepa@veeam.com>
>> +int snapshot_create(struct blk_snap_dev *dev_id_array, unsigned int count,
>> +		    uuid_t *id)
>> +{
>> +	struct snapshot *snapshot = NULL;
>> +	int ret;
>> +	unsigned int inx;
>> +
>> +	pr_info("Create snapshot for devices:\n");
>> +	for (inx = 0; inx < count; ++inx)
>> +		pr_info("\t%u:%u\n", dev_id_array[inx].mj,
>> +			dev_id_array[inx].mn);
>> +
>> +	ret = check_same_devices(dev_id_array, count);
>> +	if (ret)
>> +		return ret;
>> +
>> +	snapshot = snapshot_new(count);
>> +	if (IS_ERR(snapshot)) {
>> +		pr_err("Unable to create snapshot: failed to allocate snapshot structure\n");
>> +		return PTR_ERR(snapshot);
>> +	}
>> +
>> +	ret = -ENODEV;
>> +	for (inx = 0; inx < count; ++inx) {
>> +		dev_t dev_id =
>> +			MKDEV(dev_id_array[inx].mj, dev_id_array[inx].mn);
>> +		struct tracker *tracker;
>> +
>> +		tracker = tracker_create_or_get(dev_id);
>> +		if (IS_ERR(tracker)) {
>> +			pr_err("Unable to create snapshot\n");
>> +			pr_err("Failed to add device [%u:%u] to snapshot tracking\n",
>> +			       MAJOR(dev_id), MINOR(dev_id));
>> +			ret = PTR_ERR(tracker);
>> +			goto fail;
>> +		}
>> +
>> +		snapshot->tracker_array[inx] = tracker;
>> +		snapshot->count++;
>> +	}
>> +
>> +	down_write(&snapshots_lock);
>> +	list_add_tail(&snapshots, &snapshot->link);
>> +	up_write(&snapshots_lock);
> Given list_for_each_entry below, typo wrt &snapshots found in the fresh 2023?
> 

Thanks.
It seems I just swapped the parameters by mistake.
It should be better:
```
	down_write(&snapshots_lock);
	list_add_tail(&snapshot->link, &snapshots);
	up_write(&snapshots_lock);
```

>> +
>> +	uuid_copy(id, &snapshot->id);
>> +	pr_info("Snapshot %pUb was created\n", &snapshot->id);
>> +	return 0;
>> +fail:
>> +	pr_err("Snapshot cannot be created\n");
>> +
>> +	snapshot_put(snapshot);
>> +	return ret;
>> +}
>> +
>> +static struct snapshot *snapshot_get_by_id(uuid_t *id)
>> +{
>> +	struct snapshot *snapshot = NULL;
>> +	struct snapshot *s;
>> +
>> +	down_read(&snapshots_lock);
>> +	if (list_empty(&snapshots))
>> +		goto out;
>> +
>> +	list_for_each_entry(s, &snapshots, link) {
>> +		if (uuid_equal(&s->id, id)) {
>> +			snapshot = s;
>> +			snapshot_get(snapshot);
>> +			break;
>> +		}
>> +	}
>> +out:
>> +	up_read(&snapshots_lock);
>> +	return snapshot;
>> +}
  

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