Upstream status

[Fantu]

@SergeiShtepa Hi, thanks for your works on blksnap kernel module, I think will be very useful for improve backup on linux. I'm curious about the status of its possible upstream integration, from https://github.com/veeam/blksnap/commit/85bad8f010d08fb0d08fc2c33e5ca3630dfe2a82 seems you prepared a patches serie for upstream (for send a mail with it) but I not found it posted searching on lkml.org and google. I saw also other series of months ago on other branches but I not found them on upstream mailing list and found only older your works of 1 year ago or more. Thanks for any reply and sorry for my bad english.

[Fantu]

@SergeiShtepa you are right that master branch is normally development/unstable but for incomplete work in progress I think can be better do in a branch itself until a more complete state (and open a PR for track comment/review etc..), this can be useful if users/developer will approcing to blksnap, instead there is a more high risk that consider blksnap project "too bad and escape"

I link an example: https://github.com/linuxmint/muffin/pull/601 was a rebase on newer mutter after near ten years from fork, was a more big task and was a work in progress for 2 major cinnamon versions (in the meantime an intermediate version on master continued to be developed as, especially at the beginning, there were many shortcomings and problems and it could only be used for specific development and tests) A similar thing I suppose can be done with blksnap major changes, at least until "complete", I don't mean bug free, is ok merging in master even if still have many bugs but not very if is incomplete and definitely not working. separate branch and open PR I think can be done also for only few days needed to complete but you start to push the work done for now

maybe for this, leave it as it is, unfortunately for now the participation in the project is still almost nil (outside the ML) apart from a very small contribution from me :(

have also stable branches are good (you already keep one for example) then it would be good to have a versioning I was already thinking to open a specific "feature request" to talk about it, I'll do when I'll have more time

sorry for my bad english and if I not explained good

[SergeiShtepa]

ok. Thanks for the feedback. I'll leave it as it is for now. In the future, I will not be lazy and will make feature-branches.

[Fantu]

@SergeiShtepa hi, is there any news about next version for upstream? even if not all changes was done and patches rebased if you want already push like what was for example https://github.com/SergeiShtepa/linux/commits/blksnap_lk6.1-rc8 in the meantime I tried to prepare automatic tests to quickly test the build of the next versions, even if unfortunately I had difficulties with the other archs on amd64 both builtin and module are working and I also found the tests for ppc archs v2 on ppc fails but seems for same errors already reported by "kernel test robot" https://github.com/Fantu/linux-blksnap/actions/runs/3824817943

[SergeiShtepa]

is there any news about next version for upstream?

Working with this comment. There is a lot of work to do. Interface changes change the logic of object ownership. I hope it will be better :-).

You don't even have to try to build standalone modules on any architectures other than amd64. But the patch should work on arm64el and ppc64el. I tested the patch with a fresh kernel on POWERPC. I haven't tried big endian yet.

./usr/include/linux/blksnap.h:508:15: error: expected declaration specifiers or '...' before 'sizeof'
  508 | static_assert(sizeof(struct blk_snap_snapshot_event) == 4096,

I don't understand yet what kind of problem static_assert creates. I'll probably have to cut out these checks.

[Fantu]

@SergeiShtepa seems that static_assert should be changed in a way that compile in any case, from a fast search I found this probably useful: https://stackoverflow.com/questions/3385515/static-assert-in-c/54993033#54993033 I don't have time to check in detail now, I hope can be useful, or it would be to ask Christoph Hellwig if you can't find a solution

for all ppc build tests should be enough apply https://github.com/Fantu/linux-blksnap/commit/336b28ecf30e880a45c4b92eccb31bd697722338 after solve the static_assert error or I'll test after you push the new patch to git

seems there are still other thing to do before complete https://docs.kernel.org/process/submit-checklist.html

[SergeiShtepa]

Work on patch v3 is in development in branch. The code has never been run yet. Perhaps it can be compiled. The main change is the interface. Feedback is welcome.

[Fantu]

build on amd64 with config similar to debian kernel packages was successfull both blksnap builtin and module...but after looking logs I saw don't built with blksnap because of changed config, my mistake of don't do more checks before so probably I'll post the result tomorrow

instead a "full config" build (as needed prerequisite for kernel submit) on ppc tests failed:

./usr/include/linux/blksnap.h:87:32: error: C++ style comments are not allowed in ISO C90
   87 |         __u64 device_capacity; // ? see BLKGETSIZE64
      |                                ^
./usr/include/linux/blksnap.h:87:32: note: (this will be reported only once per input file)
./usr/include/linux/blksnap.h:337:15: error: expected declaration specifiers or '...' before 'sizeof'
  337 | static_assert(sizeof(struct blksnap_snapshot_event) == 4096,
      |               ^~~~~~
./usr/include/linux/blksnap.h:338:9: error: expected declaration specifiers or '...' before string constant
  338 |         "The size struct blksnap_snapshot_event should be equal to the size of the page.");
      |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
make[4]: *** [/linux/usr/include/Makefile:91: usr/include/linux/blksnap.hdrtest] Error 1 

edit: build on amd64 with config similar to debian kernel packages was successfull both blksnap builtin and module there is a warning: (compiled with W=1) 2023-01-04T23:43:53.3525736Z ../drivers/block/blksnap/diff_storage.c:30: warning: Function parameter or member 'bdev_path' not described in 'storage_bdev'

[Fantu]

@SergeiShtepa I did another fast search related to header error... in https://github.com/SergeiShtepa/linux/blob/master/usr/include/Makefile: # Unlike the kernel space, exported headers are written in standard C.

So I tried a fast test with https://github.com/Fantu/linux-blksnap/commit/895a41fcdc62466a79b488e3e75e21f4c44c1869 and ppc tests "full" continued, I'll check result tomorrow

After I found also another thing in https://github.com/SergeiShtepa/linux/blob/master/include/linux/build_bug.h

/**
 * static_assert - check integer constant expression at build time
 *
 * static_assert() is a wrapper for the C11 _Static_assert, with a
 * little macro magic to make the message optional (defaulting to the
 * stringification of the tested expression).
 *
 * Contrary to BUILD_BUG_ON(), static_assert() can be used at global
 * scope, but requires the expression to be an integer constant
 * expression (i.e., it is not enough that __builtin_constant_p() is
 * true for expr).
 *
 * Also note that BUILD_BUG_ON() fails the build if the condition is
 * true, while static_assert() fails the build if the expression is
 * false.
 */
#define static_assert(expr, ...) __static_assert(expr, ##__VA_ARGS__, #expr)
#define __static_assert(expr, msg, ...) _Static_assert(expr, msg)

I hope these info found can help you to do the better solution, good night

EDIT: ppc tests was failed for other errors also latest rebase still have one (or more exactly warning) so disabled werror and all 6 ppc tests was success: https://github.com/Fantu/linux-blksnap/actions/runs/3878215326

patch v2 of "documentation: fix Generic Block Device Capability" was posted: patchwork lore

[Fantu]

@SergeiShtepa I also built kernel for armhf and arm64, always with success with https://github.com/Fantu/linux-blksnap/commit/ab376a878e5c5868c112d59e759d63b7c9aecdd0 (even if static assert probably should be improved or changed) No additional warning (with W=1) but only: drivers/block/blksnap/diff_storage.c:30: warning: Function parameter or member 'bdev_path' not described in 'storage_bdev' So now I have things ready for fast build on all next patches update on amd64, armhf and arm64 (that I suppose are the most used) and ppc (ppc32 ppc64 ppc64le and also with diffent configs). Is there other archs you think important to keep tested on any change?

[Fantu]

@SergeiShtepa I saw a reply from DM maintainer in v2 patch posted: https://lore.kernel.org/lkml/Y8cNVv4O+vjL+aAy@redhat.com/ And also reply on lwn to the message of Conan_Kudo: https://lwn.net/Articles/914908/ https://lwn.net/Articles/920245/ And other reply to you: https://lwn.net/Articles/915293/ I think it's good to answer in the mailing list of the patch and understand what would be the best way to proceed together with the DM and BLOCK maintainers (and other developers if needed).

[SergeiShtepa]

@Fantu Yep. Thanks. I see. In progress.

[Fantu]

@SergeiShtepa hi, I didn't saw replies on https://lore.kernel.org/lkml/20221209142331.26395-1-sergei.shtepa@veeam.com/ are not arrived to ML or you didn't had time to reply for now?

I think is important to clarify, especially for:

... the way it has evolved has been antithetical to how to properly engage the Linux community and subsystem maintainers ...

that can risk to decrease reviews and support

I haven't been following blksnap for upstream since the beginning so I don't have a fully know all the facts to be able to answer.

and about:

The PR campaign to raise awareness with LWN became more important than cc'ing me.

Is there other LWN article related to blksnap that I don't found? I saw only the one recent wrote by corbet

but is good see this:

... I think what you're wanting to do is useful.

[SergeiShtepa]

Hi, @Fantu ! In this case, the most stupid thing that can be done is to write a quick, ill-conceived, emotional response. We are preparing an answer, don't worry.

[SergeiShtepa]

Hi all.

The upstream version of blksnap changes the interface. A branch blksnap_interface_v2 was created to check the health of the upstream version. The out-of-tree module in this branch has not been changed yet.

In the future, I would like the user-space code to work equally with both the module in-tree and out-of-tree one. But at the moment this is a secondary task.

[SergeiShtepa]

Hi! The patch v3 for kernel v6.3.0-rc4 is available for discussion. I will be glad of your feedback.

[Fantu]

@SergeiShtepa good v3 have big changes and and it was also "lightened"

for now I did only a fast look

patch subjects now is better, I think that many of them should have also "add" like: documentation: add Block Device Filtering Mechanism but maybe I'm wrong and it's not necessary

about MAINTAINERS add of new entries splitted in many commits seems strange to me, it was suggested by Christoph?

MAINTAINERS changes in patch 02/15 still have 2 typo "inlcude" that need to be fixed in "include"

Christoph did many changes, especially blkfilters (but not only) so seems good to me add him with Co-developed-by (see https://docs.kernel.org/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by) at least in blkfilter patch (if you want do it ask to him before)

I hope to have some time in the weekend for do some tests and look better

[SergeiShtepa]

about MAINTAINERS add of new entries splitted in many commits seems strange to me, it was suggested by Christoph?

I do not know how to do it correctly, but it seems to me quite logical. This separation makes patches independent. A file was added in the commit, information about maintainer was also added in it. This allowed me to check that all the files were added correctly, which means it will make it easier to check the patch, I think.

MAINTAINERS changes in patch 02/15 still have 2 typo "inlcude" that need to be fixed in "include"

Thanks, it will be fixed.

Christoph did many changes, especially blkfilters (but not only) so seems good to me add him with Co-developed-by (see https://docs.kernel.org/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by) at least in blkfilter patch (if you want do it ask to him before)

Yes, I read it. But it remains a mystery to me exactly how to specify co-authors of the project. Do I need to use git "magic" or do I need to enter these lines into the commit text?

I hope to have some time in the weekend for do some tests and look better

That would be great. I will also take the time to test next week.

[Fantu]

Christoph did many changes, especially blkfilters (but not only) so seems good to me add him with Co-developed-by (see https://docs.kernel.org/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by) at least in blkfilter patch (if you want do it ask to him before)

Yes, I read it. But it remains a mystery to me exactly how to specify co-authors of the project. Do I need to use git "magic" or do I need to enter these lines into the commit text? Seems explained well in kernel doc for me but is possible check also some commits in kernel git, about git tools support add of single Signed-off-by: of the author automatically (if setted), when more are needed should be added manually in commit description

for example if I'm not wrong in this case on "footer" of commit description should be:

Co-developed-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Christoph Hellwig <hch@lst.de>
Signed-off-by: Sergei Shtepa <sergei.shtepa@veeam.com>

looking many commits seems major of people on patches developed by multiple author use only multiple Signed-off-by: without Co-developed-by: so probably is correct also without Co-developed-by: anyway should be asked permission to Christoph Hellwig before add and also if he want suggest other fixes before submit

with this big add many changes, many review/suggest, many versions of patch submitted etc... I understand is difficult add all things related to commit description complete and good but I think is good add atleast Christoph and atleast in blkfilter part for the big contributing gave and when someone will give its review and/or approvals will also should be added (Reviewed-by: and Acked-by:) but this should be easy as will be specified in the replay, here one example: https://lore.kernel.org/all/e088f7f1-827e-f0e5-4fc0-df0dcefbb873@opensource.wdc.com/

after initial upstream inclusion should be done good in any patch with all needed things including also Reported-by:, Tested-by:, Reviewed-by:, Suggested-by: and Fixes:

About add my "Tested-by:" I think is not good as I only did some build tests and superficial usage tests, as I would also do for this new version, unfortunately it would take a few whole days for complete and serious tests

[Fantu]

for now build on amd64 (both builtin and module) and armhf and didn't gave errors or warning

additional kernel-doc comments check with scripts/kernel-doc spotted:

drivers/block/blksnap/diff_area.h:106: warning: Function parameter or member 'physical_blksz' not described in 'diff_area'
drivers/block/blksnap/diff_area.h:106: warning: Function parameter or member 'logical_blksz' not described in 'diff_area'

EDIT: done also build on arm64 and some ppc config

[SergeiShtepa]

for now build on amd64 (both builtin and module) and armhf and didn't gave errors or warning

additional kernel-doc comments check with scripts/kernel-doc spotted:

drivers/block/blksnap/diff_area.h:106: warning: Function parameter or member 'physical_blksz' not described in 'diff_area'
drivers/block/blksnap/diff_area.h:106: warning: Function parameter or member 'logical_blksz' not described in 'diff_area'

EDIT: done also build on arm64 and some ppc config

Thanks!

[Fantu]

@SergeiShtepa I saw now your latest commits on https://github.com/veeam/blksnap/tree/stable-v2.0, v2 will be only with upstream kernel module and not the external module? or is only temporary thing?

[SergeiShtepa]

Hi @Fantu . I expect that this branch will be the main one, for users, after the appearance of blksnap in the upstream. I hope this event will happen in the near future. Then it will no longer be necessary to supply blksnap separately for new kernels. I don't know what I'm going to do in the master branch yet. There is one idea, but I doubt it yet. I don't plan any other stable branches.

[SergeiShtepa]

And @Fantu - check please my latest commits in branch stable-v2.0. I have corrected the actions.

[Fantu]

@SergeiShtepa from a fast look seems ok

[SergeiShtepa]

Hi! v3 on patchwork and lore. There were some problems with the server 'vger.kernel.org' therefore, the emails appeared on the server only yesterday.

[Fantu]

@SergeiShtepa I spotted and write only yesterday about the "Changes:" without the version "in vN" that can be confused with note of previous version above :( I hope do not cause problems for those who will read. this version should be better anyway as all the warnings of complete build and other thorough checks have been fixed so no reports of kernel robots should arrive most likely and perhaps it will be better seen by some reviewers who may ignore it if the prerequisites for posting new patches are not met (older versions didn't meet all prerequisites)

[SergeiShtepa]

Yes, thanks, I read it. But pay attention, the date of sending the emails is 04.04.2023. The process of sending the emails was long this time.

[Fantu]

good news, seems that patches for upstream received the first "Tested-by:", of Donald Buczek that seems had done a deep testing

[SergeiShtepa]

Hi. News: v4 patch prepared. See: https://github.com/veeam/blksnap/tree/master/patches/lk6.4-rc4 Feedback is welcome!

[Fantu]

hi, I haven't continued the usage tests lately because I wanted to disconnect from pc in my free time and in the last few days disconnecting for most of the day for health reasons I did only a fast build test (amd64 builtin/module and docs) of lk6.4-rc4-v2 and I not found errors and warning from a fast look @SergeiShtepa have changelog only of latest version in cover I suppose is not a good things, v2 also no longer even has the link to v3

[SergeiShtepa]

Hi! Thanks! OK, I will make a full log of changes, as in the previous version of the patch. Be healthy! We all need to be able to listen to our body when it needs help.

[SergeiShtepa]

Hi. patch v4 was sent. But due to the "unwarmed" graylist on the mail server @vger.kernel.org, the first mail was torn off from the rest of the patchset. Header: https://lore.kernel.org/linux-block/20230609115206.4649-1-sergei.shtepa@veeam.com/T/#u Patchset: https://lore.kernel.org/linux-block/20230609115858.4737-1-sergei.shtepa@veeam.com/T/#t Whether this is a problem or not, I do not know. I will be waiting for comments on this.

[SergeiShtepa]

patch v5. For "kernel/git/axboe/linux-block.git" branch "for-6.5/block". Link. lore. patchwork.

[Fantu]

@SergeiShtepa the latest mail shipments seem to have been a bit chaotic^^'' v4 splitted and double v5 I saw and replies "scattered" there is good news that Christoph Hellwig https://lore.kernel.org/linux-block/ZIcsijGWeyk%2FFjHs@infradead.org/T/#mb373f939657cfe8c2c2439ef4f5886be21bb061c seems consider blksnap ready for inclusion (there are still some things to do from the replies I hope that mail send issues don't discourage reviewers

[SergeiShtepa]

Yeah, mail... For the server vger.kernel.org server veeam.com is unknown. His IP is usually not on the gray list, because I rarely send emails. And if I send the entire patchset at once, then it perceives it as an attack on the server and blocks the senders IP. Therefore, the last unsuccessful time I sent emails with a timeout per minute. However, I did not take into account that the session for git send-mail cannot be longer than 10 minutes...

I hope that in the future I will be able to organize work with mail so that such problems do not arise.

[Fantu]

looking https://github.com/torvalds/linux after the latest "block merge" of 2 days ago seems that "for-next" of block was fully merged (https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git/log/?h=for-next) so I think can blksnap can be rebased on latest https://github.com/torvalds/linux (now that include all the block changes) for working on next version of the patch

[SergeiShtepa]

Yes, thanks Fabio. We already have 6.5-rc2 . I apologize for the late response. I was on vacation. There's still a lot of work to do on redesigning the ioctl extension of the difference storage. And blk-crypto...

[SergeiShtepa]

Hi! Little update.

In the 'stable-v2.0' branch of this repository and in the branch 'blksnap-master' based on vanilla 6.6-rc2 of linux repository , the concept of storing snapshot differences in a file was implemented. The ability to create a difference storage on a block device under the snapshot has been removed. Storing differences on a block device remains possible, but only in the case of exclusive ownership of it.

An interesting consequence of these changes:

• Now it is possible to create a difference storage file with the O_TMPFILE flag. The file will be automatically deleted when its descriptor is closed when the snapshot is released.
• Now it is possible to use files in memory and in the 'swap' partition thanks to files on tmpfs.

I think that it is possible to return the ability to create a difference storage on a block device under snapshots. But this will require support from file systems. It can be quite a difficult job. I plan to start it when I realize that the blksnap module is in demand by users and they really need this feature.

In the near future, I will be engaged in testing, improve documentation and preparing a patch. I invite everyone to take part in testing the current version.

[SergeiShtepa]

Hi. Patch lk6.7-rc1-v2 has been prepared. I plan to send it next week.

[Fantu]

Tested build on amd64 (both builtin and module), armhf, arm64 and various ppc config. I don't have time to deep check and testing it in these days but seems to me achieved the main requirements for integration, and that with the latest changes reviewer had recommended I assume is approaching. @SergeiShtepa before send, I suggest taking a look at https://github.com/veeam/blksnap/pull/79 relating to the patches cover

[SergeiShtepa]

V6: lore.kernel.org/ patchwork.kernel.org

[SergeiShtepa]

Hi. Patch lk6.8-rc1 has been prepared. It also available as a branch in my fork.

[Fantu]

Hi, I don't have enough time to test its use, I just launched some amd64 builds (builtin and module), armhf, arm64 and some ppc. (from automatic tests I prepared in past). One ppc build failed for an issue not related to blksnap. On armhf build (https://github.com/Fantu/linux-blksnap/actions/runs/7629304370/job/20782415299) I spotted this warning:

2024-01-23T17:55:07.5898286Z drivers/block/blksnap/main.c: In function ‘ioctl_snapshot_create’:
2024-01-23T17:55:07.5901218Z ##[warning]drivers/block/blksnap/main.c:196:30: warning: cast to pointer from integer of different size [-Wint-to-pointer-cast]
2024-01-23T17:55:07.5903698Z   196 |         fname = strndup_user((const char __user *)karg.diff_storage_filename,
2024-01-23T17:55:07.5904914Z       |                              ^

[SergeiShtepa]

v7 https://patchwork.kernel.org/project/linux-block/list/?series=824711 https://lore.kernel.org/all/20240209160204.1471421-1-sergei.shtepa@linux.dev/

[Fantu]

This time the send of patch serie seems complete and without duplicate 👍

[SergeiShtepa]

Yep. I have reduce the number of patches in set and greatly reduced the list of recipients. The script get_maintainer.pl returns such a long list that it can no longer be used to generate a list of recipients. The test bot should respond soon. He will not be able to apply patches to the master branch and build.

[Fantu]

Looking kernel bot message of older version seems it try also with linux-block for-next: https://lore.kernel.org/lkml/202211031239.9fxTTo7W-lkp@intel.com/

url: https://github.com/intel-lab-lkp/linux/commits/Sergei-Shtepa/blksnap-creating-non-persistent-snapshots-for-backup/20221103-004434 base: https://git.kernel.org/pub/scm/linux/kernel/git/axboe/linux-block.git for-next

So I think that if will apply correctly with master or linux-block don't report error, unless it detect other errors and/or warnings

[SergeiShtepa]

I checked it out. The patch can be successfully applied to Jens branch block/for-next and can be successfully build. Jens create this branch at the end of last week.

[Fantu]

Of v7 I didn't pay attention to the patches subjects, from patch 3/8 to patch 8/8 I think it should be "blksnap:" or "block/blksnap:" instead of "block:" at the beginning of the subjects

[Fantu]

@SergeiShtepa hi, Christoph did write to you to find out if the latest version was good enough for the upstream application, or need other improvements, or did he not have time to review it? I haven't seen any public responses to the latest version, I suspect that someone who was interested missed it due to some patches with incorrect titles. Perhaps even with the fewest mail recipients, but that seems necessary to avoid it being blocked and being sent back double (or more), which would annoy the recipients :(