| Message ID | Yu1rOopN++GWylUi@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [git,pull] Additional device mapper changes for 6.0 | expand |
On Fri, Aug 5, 2022 at 12:10 PM Mike Snitzer <snitzer@kernel.org> wrote: > > All said: I think it worthwhile to merge these changes for 6.0, rather > than hold until 6.1, now that we have confidence this _optional_ DM > verity feature is working as expected. Please be aware there was a > small linux-next merge fixup needed: > https://lore.kernel.org/all/20220805125744.475531-1-broonie@kernel.org/T/ Well, more importantly, the verity_target version numbers clash. I used the newer "{1, 9, 0}" version number, but if you want it to be "{1, 9, 1}" to show that it's a superset of the previous one, you should do that yourself. That said, the best option would be to remove version numbers entirely. They are a completely broken concept as an ABI, and *never* work. Feature bitmasks work. Version numbers don't. Version numbers fundamentally break when something is backported or any other non-linearity happens. Please don't use version numbers for ABI issues. Version numbers are for human consumption, nothing more, and shouldn't be used for anything that has semantics. Linus
On Sat, Aug 6, 2022 at 11:09 AM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > Feature bitmasks work. Version numbers don't. Version numbers > fundamentally break when something is backported or any other > non-linearity happens. Side note: even feature bitmaps should be discouraged as an interface, unless there's some fundamental need for actually negotiating some kind of initial state. For 99% of all kernel cases, the better option is to simply just rely on unsupported features erroring out (ie making sure unsupported argument flags are checked and cause errors, rather than silently ignored). So while version numbers are actively broken as an interface description, often feature bitmask are just pointless and wrong too. And yes, lots of things get this wrong, and have "I implement feature Xyz", and then you have pain and gnashing of teeth when versions change and you have to support them all. It's just a horrible design pattern. Linus
The pull request you sent on Fri, 5 Aug 2022 15:10:50 -0400:
> git://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git tags/for-6.0/dm-changes-2
has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/20cf903a0c407cef19300e5c85a03c82593bde36
Thank you!
On Sat, Aug 06 2022 at 2:09P -0400, Linus Torvalds <torvalds@linux-foundation.org> wrote: > On Fri, Aug 5, 2022 at 12:10 PM Mike Snitzer <snitzer@kernel.org> wrote: > > > > All said: I think it worthwhile to merge these changes for 6.0, rather > > than hold until 6.1, now that we have confidence this _optional_ DM > > verity feature is working as expected. Please be aware there was a > > small linux-next merge fixup needed: > > https://lore.kernel.org/all/20220805125744.475531-1-broonie@kernel.org/T/ > > Well, more importantly, the verity_target version numbers clash. > > I used the newer "{1, 9, 0}" version number, but if you want it to be > "{1, 9, 1}" to show that it's a superset of the previous one, you > should do that yourself. You did the right thing. > That said, the best option would be to remove version numbers > entirely. They are a completely broken concept as an ABI, and *never* > work. > > Feature bitmasks work. Version numbers don't. Version numbers > fundamentally break when something is backported or any other > non-linearity happens. > > Please don't use version numbers for ABI issues. Version numbers are > for human consumption, nothing more, and shouldn't be used for > anything that has semantics. Yes, I know you mentioned this before and I said I'd look to switch to feature bitmasks. Yet here we are. Sorry about that, but I will take a serious look at fixing this over the next development cycle(s). There is just quite a bit of innertia in these version numbers across all the disparate userspace tools that use DM. So the transition needs some design, planning and coordination but I'll get it done. Really ;) Thanks, Mike
On Sat, Aug 6, 2022 at 11:30 AM Mike Snitzer <snitzer@kernel.org> wrote: > > > > > Please don't use version numbers for ABI issues. Version numbers are > > for human consumption, nothing more, and shouldn't be used for > > anything that has semantics. > > Yes, I know you mentioned this before and I said I'd look to switch to > feature bitmasks. Yet here we are. Sorry about that, but I will take > a serious look at fixing this over the next development cycle(s). Well, right now we're in the situation where there are certain kernels that say that they implement "version 1.9" of the thing, but they don't actually implement the "version 1.8.1" extensions. So anybody who asks for "v1.8.1 or newer" will literally get random behavior. And yes, that random behavior hopefully then doesn't happen with any *tagged* kernel version, but it's an example of how broken version numbers are as an ABI. Imagine you are bisecting something entirely unrelated, and then end up testing one of those "this says it does 1.9, but doesn't do 1.8.1" kernels.. Presumably (and hopefully) these features are all so esoteric that absolutely nobody cares. IOW, I sincerely _hope_ the solution to the version number mess is "nobody actually uses that field anyway". Because if it matters, it's broken. It's broken by design, but we literally seem to have one example of active breakage in the tree right now. Linus
Hi, Just a few notes on why we use target versions in libcryptsetup, as I am perhaps one user of this field there. TL;DR: it is *only* for hinting to users what is possibly wrong after activation fails because there is *no* proper error reporting from the device-mapper. On 06/08/2022 20:36, Linus Torvalds wrote: > On Sat, Aug 6, 2022 at 11:30 AM Mike Snitzer <snitzer@kernel.org> wrote: ... >> Yes, I know you mentioned this before and I said I'd look to switch to >> feature bitmasks. Yet here we are. Sorry about that, but I will take >> a serious look at fixing this over the next development cycle(s). Please don't just replace it with bitmaps. It will not bring any better interface while adding more magic with handling compatibility, as we need to use both... see below. > Well, right now we're in the situation where there are certain kernels > that say that they implement "version 1.9" of the thing, but they > don't actually implement the "version 1.8.1" extensions. I cannot speak for the others, but for veritysetup (libcryptsetup), the worst that can happen is that the user will get a wrong error message (or just a generic message "something failed, bye"). (All the crypto options are tricky, I would like to keep at least basic usability and better errors like "seems tasklets are not supported, retrying without tasklets flags.") In principle, we use activation flags/options as Linus describes - try to set it, then deal with the failure. And *this* is the real problem that needs to be solved - there is no proper userspace interface that says what went wrong. The userspace sees only -EINVAL from ioctl() and a generic message. Perhaps in the syslog is more info, but usually only at debug level (that is often not visible), and parsing syslog is not the option for us either. What is even more problematic is that the error string in DM target is often set (e.g. ti->error = "Integrity profile tag size mismatch.";) but later discarded, and it never reaches neither log nor userspace calling the failing ioctl(). If the device-mapper can fix this, we can easily thrash the magic that consults the target version and determines what went wrong. Then you can forget the version and feature bitmaps and send us a proper (ideally structured) error message in ioctl() reply. Milan
On Sun, Aug 07 2022 at 3:37P -0400, Milan Broz <gmazyland@gmail.com> wrote: > Hi, > > Just a few notes on why we use target versions in libcryptsetup, > as I am perhaps one user of this field there. > > TL;DR: it is *only* for hinting to users what is possibly wrong > after activation fails because there is *no* proper error reporting > from the device-mapper. DM's core and target versions aren't intended to be in service of error reporting. You abusing them like that is a fundamental problem. [[Unfortunate tangent but you've left me no choice: Your general tone and misinformation-using-broad-strokes makes me both sad and angry. I will restrain myself in this reply but your position drips with general FUD and loathing. This is way more "Milan being Milan" than I've ever experienced. Could be you've been storing it and it all just gushed out, no idea. But it's a lot to try to take with grace. As you know I'm a very direct person. I speak my mind too. But I've learned to try to avoid alarmist rhetoric that amounts to throwing people(s) under the bus (better late than never). But if you're going to resort that you better be _very_ certain it's justified. Yet as cathartic as it might seem, even then it isn't the correct answer. If you want to remain being respected please treat others with respect. Only you know why you are flailing about with such an attitude, please come to terms with that. I wish you well and certainly don't want DM to be some constant or reoccurring source of such negativity (for you or anyone).]] > On 06/08/2022 20:36, Linus Torvalds wrote: > > On Sat, Aug 6, 2022 at 11:30 AM Mike Snitzer <snitzer@kernel.org> wrote: > ... > > > Yes, I know you mentioned this before and I said I'd look to switch to > > > feature bitmasks. Yet here we are. Sorry about that, but I will take > > > a serious look at fixing this over the next development cycle(s). > > Please don't just replace it with bitmaps. > > It will not bring any better interface while adding more magic with > handling compatibility, as we need to use both... see below. (I saw your "below", it lacked a coherent explanation for why "we need to use both" as a rule moving forward) When done properly it will _not_ require both. The version number would be incremented one final time and would serve to allow existing userspace to run unmodified. But from that point on the bitmap flags should be used and all userspace converted to use them. > > Well, right now we're in the situation where there are certain kernels > > that say that they implement "version 1.9" of the thing, but they > > don't actually implement the "version 1.8.1" extensions. > > I cannot speak for the others, but for veritysetup (libcryptsetup), > the worst that can happen is that the user will get a wrong error message > (or just a generic message "something failed, bye"). You know how to send email to report specific problems and/or submit patches. But I really don't recall anything in this category being reported by you, certainly not recently... maybe you've just internalized or I somehow missed it? > (All the crypto options are tricky, I would like to keep at least basic > usability and better errors like "seems tasklets are not supported, > retrying without tasklets flags.") dm-verity's optional "try_verify_in_tasklet" is using tasklets as an implementation detail, if they cannot be used (e.g. for FEC) then why would fallback to normal verification using a workqueue be reported? Or are you referring to something you saw when using dm-crypt's no_{read,write}_workqueue options? Or are you saying that both the new dm-verity try_verify_in_tasklet option and the dm-crypt no_{read,write}_workqueue options should fallback to removing those flags and try without them? That is a level of AI I have no interest in adding or supporting. The user asked for something, if it isn't possible then it should fail. But please be more specific. > In principle, we use activation flags/options as Linus describes - try > to set it, then deal with the failure. > > And *this* is the real problem that needs to be solved - there is no proper > userspace interface that says what went wrong. > > The userspace sees only -EINVAL from ioctl() and a generic message. "Please extend the DM ioctls to somehow add ti->error to the userspace response" is a fine feature request. Should help no matter what. (Can look to have a phased approach to the error reporting payload, start with errno and error message, add more "structured" payload over time. Are you referring to JSON or some other format? Whatever systemd uses?). > Perhaps in the syslog is more info, but usually only at debug level > (that is often not visible), and parsing syslog is not the option for us either. All errors should be emitted with pr_err() using DMERR(). I've made a conscious effort to convert DMWARN() to DMERR() when appropriate. But I'll audit all the DM core code and then work through the various targets. If there are incorrect log levels being used it is a bug, please report and/or fix. > What is even more problematic is that the error string in DM target is > often set (e.g. ti->error = "Integrity profile tag size mismatch.";) but later > discarded, and it never reaches neither log nor userspace calling the failing > ioctl(). Again, if you see a bug: please report and/or fix it. > If the device-mapper can fix this, we can easily thrash the magic that > consults the target version and determines what went wrong. There is no way to properly use version numbers to derive what actually went wrong. Could you narrow down and isolate the possible failure based on version in specific cases? Sure.. but it is insanely fragile (especially with stable@ and distro kernels). > Then you can forget the version and feature bitmaps and send > us a proper (ideally structured) error message in ioctl() reply. OK, I can just avoid switching to feature bitmaps entirely, stop bumping version numbers, and focus on better error reporting. Then all of userspace can rely on errors reported to fail and inform user actions. But I'm pretty confident lvm2 will have something to say on this... I'll take all input into consideration. Mike
Mike, there was nothing personal in my reply - sorry if you see it this way. Anyway, please stop ad-hominem attacks on me! I just described what I see as a problem that prevents us from dropping version parsing. Technical comments, below, but really, these should go to dm-devel only to not waste time of others. On 07/08/2022 20:14, Mike Snitzer wrote: >> TL;DR: it is *only* for hinting to users what is possibly wrong >> after activation fails because there is *no* proper error reporting >> from the device-mapper. > > DM's core and target versions aren't intended to be in service of > error reporting. You abusing them like that is a fundamental problem. Perhaps, but there was nothing better. If I missed something, we can definitely make the code better. TBH, I do even think that it uses the same logic as libdevmapper library (and perhaps it dates even before I started to maintain it). I do not see fundamental problem here, though. I take is as "The dm-integrity was introduced in kernel/target X", then I do not expect it working in X-1... >> Please don't just replace it with bitmaps. >> >> It will not bring any better interface while adding more magic with >> handling compatibility, as we need to use both... see below. > > (I saw your "below", it lacked a coherent explanation for why "we need > to use both" as a rule moving forward) > > When done properly it will _not_ require both. The version number would > be incremented one final time and would serve to allow existing > userspace to run unmodified. But from that point on the bitmap flags > should be used and all userspace converted to use them. I just meant that if userspace want to support older kernels, we need to support both. If it does not bring fixes for the problem I described, it is just more code with no effect (for libcryptsetup). But if you see other reasons, then of course it makes sense. >> I cannot speak for the others, but for veritysetup (libcryptsetup), >> the worst that can happen is that the user will get a wrong error message >> (or just a generic message "something failed, bye"). > > You know how to send email to report specific problems and/or submit > patches. But I really don't recall anything in this category being > reported by you, certainly not recently... maybe you've just > internalized or I somehow missed it? I am sure I mentioned this, but years ago... what I am talking about 1) Some ti->error messages are lost, e.g. in dm-crypt, I think example is IV generators constructors if (ret < 0) { ti->error = "Error creating IV"; ... (And yes, I should fix this myself.) 2) Targets use macros like DMERR, these generate syslog message. Getting these messages into userspace is problematic. But perhaps this is more problem for libdevmapper we use. >> (All the crypto options are tricky, I would like to keep at least basic >> usability and better errors like "seems tasklets are not supported, >> retrying without tasklets flags.") > > dm-verity's optional "try_verify_in_tasklet" is using tasklets as an > implementation detail, if they cannot be used (e.g. for FEC) then why > would fallback to normal verification using a workqueue be reported? I am talking about situation when user explicitly requests to use tasklets on CLI and kernel does not support it. Then there must be an error message. I am not sure if we should automatically fallback to non-tasklets, but we do this already in other situations (enable-discards, keyring support, ...) > > Or are you referring to something you saw when using dm-crypt's > no_{read,write}_workqueue options? > > Or are you saying that both the new dm-verity try_verify_in_tasklet > option and the dm-crypt no_{read,write}_workqueue options should > fallback to removing those flags and try without them? > > That is a level of AI I have no interest in adding or supporting. > The user asked for something, if it isn't possible then it should > fail. And nobody asked for this as we are already doing this in userspace. It was really just example to demonstrate when we use target version. > "Please extend the DM ioctls to somehow add ti->error to the userspace > response" is a fine feature request. Should help no matter what. > > (Can look to have a phased approach to the error reporting payload, > start with errno and error message, add more "structured" payload over > time. Are you referring to JSON or some other format? Whatever systemd > uses?). Great, let's discuss this later. > >> Perhaps in the syslog is more info, but usually only at debug level >> (that is often not visible), and parsing syslog is not the option for us either. > > All errors should be emitted with pr_err() using DMERR(). I've made a > conscious effort to convert DMWARN() to DMERR() when appropriate. But > I'll audit all the DM core code and then work through the various > targets. > > If there are incorrect log levels being used it is a bug, please > report and/or fix. Yes, I tried to say that syslog itself as source is problematic (if you activate many devices in parallel; in multi-tenant environment when you should not see logs from different users etc). > There is no way to properly use version numbers to derive what > actually went wrong. Could you narrow down and isolate the possible > failure based on version in specific cases? Sure.. but it is insanely > fragile (especially with stable@ and distro kernels). It works pretty reliably for years with some minor exceptions that can be ignored. Milan
Hi Linus, In my previous 6.0 pull request I should have forecast the potential for sending another one. The changes in this pull request add an optional feature to the DM verity target. These changes were proposed for inclussion on dm-devel a couple weeks before the merge window opened. I reviewed and worked the changes with Nathan and others for about a week. At that time the changes still had some clear issues (exposed with the additional testing Milan Broz provided in terms of a revised cryptsetup testsuite that introduced veritysetup's --use-tasklets). I had to put it aside once this merge window opened but Nathan continued to investigate. Nathan found and fixed the remaining issues on Tuesday/Wednesday: https://listman.redhat.com/archives/dm-devel/2022-August/051766.html Yesterday I then folded the fixes in, and refined the code a bit further in response. The crytpsetup testsuite passes now (both with and without the use of veritysetup's --use-tasklets flag). All said: I think it worthwhile to merge these changes for 6.0, rather than hold until 6.1, now that we have confidence this _optional_ DM verity feature is working as expected. Please be aware there was a small linux-next merge fixup needed: https://lore.kernel.org/all/20220805125744.475531-1-broonie@kernel.org/T/ The following changes since commit 9dd1cd3220eca534f2d47afad7ce85f4c40118d8: dm: fix dm-raid crash if md_handle_request() splits bio (2022-07-28 17:36:30 -0400) are available in the Git repository at: git://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git tags/for-6.0/dm-changes-2 for you to fetch changes up to 12907efde6ad984f2d76cc1a7dbaae132384d8a5: dm verity: have verify_wq use WQ_HIGHPRI if "try_verify_in_tasklet" (2022-08-04 15:59:52 -0400) Please pull, thanks! Mike ---------------------------------------------------------------- - Add flags argument to dm_bufio_client_create and introduce DM_BUFIO_CLIENT_NO_SLEEP flag to have dm-bufio use spinlock rather than mutex for its locking. - Add optional "try_verify_in_tasklet" feature to DM verity target. This feature gives users the option to improve IO latency by using a tasklet to verify, using hashes in bufio's cache, rather than wait to schedule a work item via workqueue. But if there is a bufio cache miss, or an error, then the tasklet will fallback to using workqueue. - Incremental changes to both dm-bufio and the DM verity target to use jump_label to minimize cost of branching associated with the niche "try_verify_in_tasklet" feature. DM-bufio in particular is used by quite a few other DM targets so it doesn't make sense to incur additional bufio cost in those targets purely for the benefit of this niche verity feature if the feature isn't ever used. - Optimize verity_verify_io, which is used by both workqueue and tasklet based verification, if FEC is not configured or tasklet based verification isn't used. - Remove DM verity target's verify_wq's use of the WQ_CPU_INTENSIVE flag since it uses WQ_UNBOUND. Also, use the WQ_HIGHPRI flag if "try_verify_in_tasklet" is specified. ---------------------------------------------------------------- Mike Snitzer (7): dm verity: allow optional args to alter primary args handling dm bufio: conditionally enable branching for DM_BUFIO_CLIENT_NO_SLEEP dm verity: conditionally enable branching for "try_verify_in_tasklet" dm verity: optimize verity_verify_io if FEC not configured dm verity: only copy bvec_iter in verity_verify_io if in_tasklet dm verity: remove WQ_CPU_INTENSIVE flag since using WQ_UNBOUND dm verity: have verify_wq use WQ_HIGHPRI if "try_verify_in_tasklet" Nathan Huckleberry (3): dm bufio: Add flags argument to dm_bufio_client_create dm bufio: Add DM_BUFIO_CLIENT_NO_SLEEP flag dm verity: Add optional "try_verify_in_tasklet" feature drivers/md/dm-bufio.c | 32 +++++- drivers/md/dm-ebs-target.c | 3 +- drivers/md/dm-integrity.c | 2 +- drivers/md/dm-snap-persistent.c | 2 +- drivers/md/dm-verity-fec.c | 4 +- drivers/md/dm-verity-target.c | 160 ++++++++++++++++++++++---- drivers/md/dm-verity.h | 6 +- drivers/md/persistent-data/dm-block-manager.c | 3 +- include/linux/dm-bufio.h | 8 +- 9 files changed, 187 insertions(+), 33 deletions(-)