[git pull] device mapper changes for 6.0

[git pull] device mapper changes for 6.0

[Mike Snitzer]

Hi Linus,

Stephen had to deal with a couple trivial merge conflicts late in this
cycle, please see: https://lkml.org/lkml/2022/7/27/1819 and
https://lkml.org/lkml/2022/7/28/302

The following changes since commit 22d0c4080fe49299640d9d6c43154c49794c2825:

  block: simplify disk_set_independent_access_ranges (2022-06-29 08:36:46 -0600)

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

for you to fetch changes up to 9dd1cd3220eca534f2d47afad7ce85f4c40118d8:

  dm: fix dm-raid crash if md_handle_request() splits bio (2022-07-28 17:36:30 -0400)

Please pull, thanks.
Mike

----------------------------------------------------------------
- Refactor DM core's mempool allocation so that it clearer by not
  being split acorss files.

- Improve DM core's BLK_STS_DM_REQUEUE and BLK_STS_AGAIN handling.

- Optimize DM core's more common bio splitting by eliminating the use
  of bio cloning with bio_split+bio_chain. Shift that cloning cost to
  the relatively unlikely dm_io requeue case that only occurs during
  error handling. Introduces dm_io_rewind() that will clone a bio that
  reflects the subset of the original bio that must be requeued.

- Remove DM core's dm_table_get_num_targets() wrapper and audit all
  dm_table_get_target() callers.

- Fix potential for OOM with DM writecache target by setting a default
  MAX_WRITEBACK_JOBS (set to 256MiB or 1/16 of total system memory,
  whichever is smaller).

- Fix DM writecache target's stats that are reported through
  DM-specific table info.

- Fix use-after-free crash in dm_sm_register_threshold_callback().

- Refine DM core's Persistent Reservation handling in preparation for
  broader work Mike Christie is doing to add compatibility with
  Microsoft Windows Failover Cluster.

- Fix various KASAN reported bugs in the DM raid target.

- Fix DM raid target crash due to md_handle_request() bio splitting
  that recurses to block core without properly initializing the bio's
  bi_dev.

- Fix some code comment typos and fix some Documentation formatting.

----------------------------------------------------------------
Bagas Sanjaya (1):
      Documentation: dm writecache: Render status list as list

Christoph Hellwig (2):
      dm: unexport dm_get_reserved_rq_based_ios
      dm: refactor dm_md_mempool allocation

JeongHyeon Lee (1):
      dm verity: fix checkpatch close brace error

Jiang Jian (1):
      dm raid: remove redundant "the" in parse_raid_params() comment

Luo Meng (1):
      dm thin: fix use-after-free crash in dm_sm_register_threshold_callback

Mauro Carvalho Chehab (1):
      Documentation: dm writecache: add blank line before optional parameters

Mike Christie (4):
      dm: Allow dm_call_pr to be used for path searches
      dm: Start pr_reserve from the same starting path
      dm: Fix PR release handling for non All Registrants
      dm: Start pr_preempt from the same starting path

Mike Snitzer (5):
      dm table: remove dm_table_get_num_targets() wrapper
      dm table: audit all dm_table_get_target() callers
      dm table: rename dm_target variable in dm_table_add_target()
      dm: return early from dm_pr_call() if DM device is suspended
      dm: fix dm-raid crash if md_handle_request() splits bio

Mikulas Patocka (8):
      dm writecache: set a default MAX_WRITEBACK_JOBS
      dm kcopyd: use __GFP_HIGHMEM when allocating pages
      dm writecache: return void from functions
      dm writecache: count number of blocks read, not number of read bios
      dm writecache: count number of blocks written, not number of write bios
      dm writecache: count number of blocks discarded, not number of discard bios
      dm raid: fix address sanitizer warning in raid_status
      dm raid: fix address sanitizer warning in raid_resume

Ming Lei (3):
      dm: improve BLK_STS_DM_REQUEUE and BLK_STS_AGAIN handling
      dm: add dm_bio_rewind() API to DM core
      dm: add two stage requeue mechanism

Steven Lung (1):
      dm cache: fix typo in 2 comment blocks

Zhang Jiaming (1):
      dm snapshot: fix typo in snapshot_map() comment

 .../admin-guide/device-mapper/writecache.rst       |  18 +-
 drivers/md/Makefile                                |   2 +-
 drivers/md/dm-cache-metadata.h                     |   2 +-
 drivers/md/dm-cache-target.c                       |   2 +-
 drivers/md/dm-core.h                               |  23 +-
 drivers/md/dm-ima.c                                |   5 +-
 drivers/md/dm-io-rewind.c                          | 166 ++++++++
 drivers/md/dm-ioctl.c                              |   6 +-
 drivers/md/dm-kcopyd.c                             |   2 +-
 drivers/md/dm-raid.c                               |   7 +-
 drivers/md/dm-rq.c                                 |   1 -
 drivers/md/dm-snap.c                               |   2 +-
 drivers/md/dm-table.c                              | 318 +++++++-------
 drivers/md/dm-thin-metadata.c                      |   7 +-
 drivers/md/dm-thin.c                               |   4 +-
 drivers/md/dm-verity-target.c                      |   7 +-
 drivers/md/dm-writecache.c                         |  43 +-
 drivers/md/dm-zone.c                               |   7 +-
 drivers/md/dm.c                                    | 462 +++++++++++++--------
 drivers/md/dm.h                                    |   4 -
 include/linux/device-mapper.h                      |   7 +-
 include/uapi/linux/dm-ioctl.h                      |   4 +-
 22 files changed, 694 insertions(+), 405 deletions(-)
 create mode 100644 drivers/md/dm-io-rewind.c

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

[Mike Snitzer]

Hi Linus,

Stephen had to deal with a couple trivial merge conflicts late in this
cycle, please see: https://lkml.org/lkml/2022/7/27/1819 and
https://lkml.org/lkml/2022/7/28/302

The following changes since commit 22d0c4080fe49299640d9d6c43154c49794c2825:

  block: simplify disk_set_independent_access_ranges (2022-06-29 08:36:46 -0600)

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

for you to fetch changes up to 9dd1cd3220eca534f2d47afad7ce85f4c40118d8:

  dm: fix dm-raid crash if md_handle_request() splits bio (2022-07-28 17:36:30 -0400)

Please pull, thanks.
Mike

----------------------------------------------------------------
- Refactor DM core's mempool allocation so that it clearer by not
  being split acorss files.

- Improve DM core's BLK_STS_DM_REQUEUE and BLK_STS_AGAIN handling.

- Optimize DM core's more common bio splitting by eliminating the use
  of bio cloning with bio_split+bio_chain. Shift that cloning cost to
  the relatively unlikely dm_io requeue case that only occurs during
  error handling. Introduces dm_io_rewind() that will clone a bio that
  reflects the subset of the original bio that must be requeued.

- Remove DM core's dm_table_get_num_targets() wrapper and audit all
  dm_table_get_target() callers.

- Fix potential for OOM with DM writecache target by setting a default
  MAX_WRITEBACK_JOBS (set to 256MiB or 1/16 of total system memory,
  whichever is smaller).

- Fix DM writecache target's stats that are reported through
  DM-specific table info.

- Fix use-after-free crash in dm_sm_register_threshold_callback().

- Refine DM core's Persistent Reservation handling in preparation for
  broader work Mike Christie is doing to add compatibility with
  Microsoft Windows Failover Cluster.

- Fix various KASAN reported bugs in the DM raid target.

- Fix DM raid target crash due to md_handle_request() bio splitting
  that recurses to block core without properly initializing the bio's
  bi_dev.

- Fix some code comment typos and fix some Documentation formatting.

----------------------------------------------------------------
Bagas Sanjaya (1):
      Documentation: dm writecache: Render status list as list

Christoph Hellwig (2):
      dm: unexport dm_get_reserved_rq_based_ios
      dm: refactor dm_md_mempool allocation

JeongHyeon Lee (1):
      dm verity: fix checkpatch close brace error

Jiang Jian (1):
      dm raid: remove redundant "the" in parse_raid_params() comment

Luo Meng (1):
      dm thin: fix use-after-free crash in dm_sm_register_threshold_callback

Mauro Carvalho Chehab (1):
      Documentation: dm writecache: add blank line before optional parameters

Mike Christie (4):
      dm: Allow dm_call_pr to be used for path searches
      dm: Start pr_reserve from the same starting path
      dm: Fix PR release handling for non All Registrants
      dm: Start pr_preempt from the same starting path

Mike Snitzer (5):
      dm table: remove dm_table_get_num_targets() wrapper
      dm table: audit all dm_table_get_target() callers
      dm table: rename dm_target variable in dm_table_add_target()
      dm: return early from dm_pr_call() if DM device is suspended
      dm: fix dm-raid crash if md_handle_request() splits bio

Mikulas Patocka (8):
      dm writecache: set a default MAX_WRITEBACK_JOBS
      dm kcopyd: use __GFP_HIGHMEM when allocating pages
      dm writecache: return void from functions
      dm writecache: count number of blocks read, not number of read bios
      dm writecache: count number of blocks written, not number of write bios
      dm writecache: count number of blocks discarded, not number of discard bios
      dm raid: fix address sanitizer warning in raid_status
      dm raid: fix address sanitizer warning in raid_resume

Ming Lei (3):
      dm: improve BLK_STS_DM_REQUEUE and BLK_STS_AGAIN handling
      dm: add dm_bio_rewind() API to DM core
      dm: add two stage requeue mechanism

Steven Lung (1):
      dm cache: fix typo in 2 comment blocks

Zhang Jiaming (1):
      dm snapshot: fix typo in snapshot_map() comment

 .../admin-guide/device-mapper/writecache.rst       |  18 +-
 drivers/md/Makefile                                |   2 +-
 drivers/md/dm-cache-metadata.h                     |   2 +-
 drivers/md/dm-cache-target.c                       |   2 +-
 drivers/md/dm-core.h                               |  23 +-
 drivers/md/dm-ima.c                                |   5 +-
 drivers/md/dm-io-rewind.c                          | 166 ++++++++
 drivers/md/dm-ioctl.c                              |   6 +-
 drivers/md/dm-kcopyd.c                             |   2 +-
 drivers/md/dm-raid.c                               |   7 +-
 drivers/md/dm-rq.c                                 |   1 -
 drivers/md/dm-snap.c                               |   2 +-
 drivers/md/dm-table.c                              | 318 +++++++-------
 drivers/md/dm-thin-metadata.c                      |   7 +-
 drivers/md/dm-thin.c                               |   4 +-
 drivers/md/dm-verity-target.c                      |   7 +-
 drivers/md/dm-writecache.c                         |  43 +-
 drivers/md/dm-zone.c                               |   7 +-
 drivers/md/dm.c                                    | 462 +++++++++++++--------
 drivers/md/dm.h                                    |   4 -
 include/linux/device-mapper.h                      |   7 +-
 include/uapi/linux/dm-ioctl.h                      |   4 +-
 22 files changed, 694 insertions(+), 405 deletions(-)
 create mode 100644 drivers/md/dm-io-rewind.c

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [git pull] device mapper changes for 6.0

[pr-tracker-bot]

The pull request you sent on Mon, 1 Aug 2022 14:58:49 -0400:

> git://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git tags/for-6.0/dm-changes

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/8374cfe647a1f360be3228b949dd6d753c55c19c

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html

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

[pr-tracker-bot]

The pull request you sent on Mon, 1 Aug 2022 14:58:49 -0400:

> git://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git tags/for-6.0/dm-changes

has been merged into torvalds/linux.git:
https://git.kernel.org/torvalds/c/8374cfe647a1f360be3228b949dd6d753c55c19c

Thank you!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

[git pull] Additional device mapper changes for 6.0

[Mike Snitzer]

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(-)

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

[Mike Snitzer]

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(-)

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [git pull] Additional device mapper changes for 6.0

[Linus Torvalds]

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

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

[Linus Torvalds]

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

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [git pull] Additional device mapper changes for 6.0

[Linus Torvalds]

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

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

[Linus Torvalds]

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

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

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

[pr-tracker-bot]

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!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html

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

[pr-tracker-bot]

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!

-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/prtracker.html

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [git pull] Additional device mapper changes for 6.0

[Mike Snitzer]

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

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

[Mike Snitzer]

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

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [git pull] Additional device mapper changes for 6.0

[Linus Torvalds]

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

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

[Linus Torvalds]

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

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [git pull] Additional device mapper changes for 6.0

[Milan Broz]

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

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

[Milan Broz]

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

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [git pull] Additional device mapper changes for 6.0

[Mike Snitzer]

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

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

[Mike Snitzer]

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

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel

Re: [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

Re: [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

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel