[dm-devel] [git pull] Additional device mapper changes for 6.0

[Milan Broz]

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