All of lore.kernel.org
 help / color / mirror / Atom feed
From: Milan Broz <gmazyland@gmail.com>
To: Mike Snitzer <snitzer@kernel.org>
Cc: Eric Biggers <ebiggers@kernel.org>,
	dm-devel@redhat.com, Sami Tolvanen <samitolvanen@google.com>,
	Nathan Huckleberry <nhuck@google.com>
Subject: Re: [dm-devel] [PATCH v2 0/6] dm verity: optionally use tasklets
Date: Tue, 26 Jul 2022 23:44:45 +0200	[thread overview]
Message-ID: <6a158b48-10d3-6427-bc44-e7f78513380b@gmail.com> (raw)
In-Reply-To: <20220726160959.89247-1-snitzer@kernel.org>

On 26/07/2022 18:09, Mike Snitzer wrote:
> Hi,
> 
> Please see this updated patchset that reflects what I've staged for
> the 5.20 merge window, see:
> https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/log/?h=dm-5.20
> 
> My only outstanding question, from previous v1 patchset, is: should
> the verify_wq be created using WQ_HIGHPRI instead of WQ_CPU_INTENSIVE?
> (I doubt it has a significant impact on performance but if you have
> insight on why you made that change, and if it meaningful, I'd happily
> apply the change).
> 
> I've tested using cryptsetup's testsuite (which has dm-verity tests)
> but I haven't tested the "try_verify_in_tasklet" feature.

Hi Mike,

I added new veritysetup option --use-tasklets for testing to a new branch
https://gitlab.com/cryptsetup/cryptsetup/-/commits/verify-tasklet

I tried to run verity-compat-test (on that branch above), not used the flag yet,
just in one simple option flag test (see the branch).

But with your patches (on top of 5.19.0-rc8) and my testing 32bit VM:

- FEC tests are skipped even if FEC is enabled in kernel.
I think the whole FEC is broken with your patches.
(Beware, test will skip FEC quietly! Usually CONFIG_DM_VERITY_FEC is disabled,
so we have to ignore it.)

- Also I see this warning (in that simple option test I added).

: =====================================================
: WARNING: SOFTIRQ-safe -> SOFTIRQ-unsafe lock order detected
: 5.19.0-rc8+ #767 Not tainted
: -----------------------------------------------------
: kworker/u16:6/2488 [HC0[0]:SC0[0]:HE0:SE1] is trying to acquire:
: f7a38090 (global_spinlock){+.+.}-{2:2}, at: adjust_total_allocated+0x95/0x120 [dm_bufio]
: \x0aand this task is already holding:
: c555086c (&c->spinlock){..-.}-{2:2}, at: dm_bufio_lock+0x54/0x60 [dm_bufio]
: which would create a new lock dependency:
:  (&c->spinlock){..-.}-{2:2} -> (global_spinlock){+.+.}-{2:2}
: \x0abut this new dependency connects a SOFTIRQ-irq-safe lock:
:  (&c->spinlock){..-.}-{2:2}
: \x0a... which became SOFTIRQ-irq-safe at:
:   lock_acquire+0xb2/0x2b0
:   _raw_spin_lock_irqsave_nested+0x3b/0x90
:   dm_bufio_lock+0x54/0x60 [dm_bufio]
:   new_read+0x32/0x120 [dm_bufio]
:   dm_bufio_get+0xd/0x10 [dm_bufio]
:   verity_verify_level+0x199/0x220 [dm_verity]
:   verity_hash_for_block+0x26/0xf0 [dm_verity]
:   verity_verify_io+0x134/0x490 [dm_verity]
:   verity_tasklet+0xf/0x7f [dm_verity]
:   tasklet_action_common.constprop.0+0xd0/0xf0
:   tasklet_action+0x21/0x30
:   __do_softirq+0xb4/0x4c5
:   run_ksoftirqd+0x35/0x50
:   smpboot_thread_fn+0x174/0x230
:   kthread+0xd2/0x100
:   ret_from_fork+0x19/0x24
: \x0ato a SOFTIRQ-irq-unsafe lock:
:  (global_spinlock){+.+.}-{2:2}
: \x0a... which became SOFTIRQ-irq-unsafe at:
: ...
:   lock_acquire+0xb2/0x2b0
:   _raw_spin_lock+0x28/0x70
:   adjust_total_allocated+0x95/0x120 [dm_bufio]
:   __link_buffer+0xb2/0xf0 [dm_bufio]
:   __bufio_new+0x20b/0x2b0 [dm_bufio]
:   dm_bufio_prefetch+0x90/0x1f0 [dm_bufio]
:   verity_prefetch_io+0x142/0x180 [dm_verity]
:   process_one_work+0x246/0x530
:   worker_thread+0x47/0x3e0
:   kthread+0xd2/0x100
:   ret_from_fork+0x19/0x24
: \x0aother info that might help us debug this:\x0a
:  Possible interrupt unsafe locking scenario:\x0a
:        CPU0                    CPU1
:        ----                    ----
:   lock(global_spinlock);
:                                local_irq_disable();
:                                lock(&c->spinlock);
:                                lock(global_spinlock);
:   <Interrupt>
:     lock(&c->spinlock);
: \x0a *** DEADLOCK ***\x0a
: 3 locks held by kworker/u16:6/2488:
:  #0: c55494b8 ((wq_completion)kverityd){+.+.}-{0:0}, at: process_one_work+0x1d0/0x530
:  #1: c6c49f38 ((work_completion)(&pw->work)){+.+.}-{0:0}, at: process_one_work+0x1d0/0x530
:  #2: c555086c (&c->spinlock){..-.}-{2:2}, at: dm_bufio_lock+0x54/0x60 [dm_bufio]
: \x0athe dependencies between SOFTIRQ-irq-safe lock and the holding lock:
: -> (&c->spinlock){..-.}-{2:2} ops: 2 {
:    IN-SOFTIRQ-W at:
:                     lock_acquire+0xb2/0x2b0
:                     _raw_spin_lock_irqsave_nested+0x3b/0x90
:                     dm_bufio_lock+0x54/0x60 [dm_bufio]
:                     new_read+0x32/0x120 [dm_bufio]
:                     dm_bufio_get+0xd/0x10 [dm_bufio]
:                     verity_verify_level+0x199/0x220 [dm_verity]
:                     verity_hash_for_block+0x26/0xf0 [dm_verity]
:                     verity_verify_io+0x134/0x490 [dm_verity]
:                     verity_tasklet+0xf/0x7f [dm_verity]
:                     tasklet_action_common.constprop.0+0xd0/0xf0
:                     tasklet_action+0x21/0x30
:                     __do_softirq+0xb4/0x4c5
:                     run_ksoftirqd+0x35/0x50
:                     smpboot_thread_fn+0x174/0x230
:                     kthread+0xd2/0x100
:                     ret_from_fork+0x19/0x24
:    INITIAL USE at:
:                    lock_acquire+0xb2/0x2b0
:                    _raw_spin_lock_irqsave_nested+0x3b/0x90
:                    dm_bufio_lock+0x54/0x60 [dm_bufio]
:                    dm_bufio_prefetch+0x4b/0x1f0 [dm_bufio]
:                    verity_prefetch_io+0x3c/0x180 [dm_verity]
:                    process_one_work+0x246/0x530
:                    worker_thread+0x47/0x3e0
:                    kthread+0xd2/0x100
:                    ret_from_fork+0x19/0x24
:  }
:  ... key      at: [<f7a384e8>] __key.24+0x0/0xffffeb18 [dm_bufio]
: \x0athe dependencies between the lock to be acquired
:  and SOFTIRQ-irq-unsafe lock:
: -> (global_spinlock){+.+.}-{2:2} ops: 129329 {
:    HARDIRQ-ON-W at:
:                     lock_acquire+0xb2/0x2b0
:                     _raw_spin_lock+0x28/0x70
:                     adjust_total_allocated+0x95/0x120 [dm_bufio]
:                     __link_buffer+0xb2/0xf0 [dm_bufio]
:                     __bufio_new+0x20b/0x2b0 [dm_bufio]
:                     dm_bufio_prefetch+0x90/0x1f0 [dm_bufio]
:                     verity_prefetch_io+0x142/0x180 [dm_verity]
:                     process_one_work+0x246/0x530
:                     worker_thread+0x47/0x3e0
:                     kthread+0xd2/0x100
:                     ret_from_fork+0x19/0x24
:    SOFTIRQ-ON-W at:
:                     lock_acquire+0xb2/0x2b0
:                     _raw_spin_lock+0x28/0x70
:                     adjust_total_allocated+0x95/0x120 [dm_bufio]
:                     __link_buffer+0xb2/0xf0 [dm_bufio]
:                     __bufio_new+0x20b/0x2b0 [dm_bufio]
:                     dm_bufio_prefetch+0x90/0x1f0 [dm_bufio]
:                     verity_prefetch_io+0x142/0x180 [dm_verity]
:                     process_one_work+0x246/0x530
:                     worker_thread+0x47/0x3e0
:                     kthread+0xd2/0x100
:                     ret_from_fork+0x19/0x24
:    INITIAL USE at:
:                    lock_acquire+0xb2/0x2b0
:                    _raw_spin_lock+0x28/0x70
:                    adjust_total_allocated+0x95/0x120 [dm_bufio]
:                    __link_buffer+0xb2/0xf0 [dm_bufio]
:                    __bufio_new+0x20b/0x2b0 [dm_bufio]
:                    dm_bufio_prefetch+0x90/0x1f0 [dm_bufio]
:                    verity_prefetch_io+0x142/0x180 [dm_verity]
:                    process_one_work+0x246/0x530
:                    worker_thread+0x47/0x3e0
:                    kthread+0xd2/0x100
:                    ret_from_fork+0x19/0x24
:  }
:  ... key      at: [<f7a38090>] global_spinlock+0x10/0xffffef80 [dm_bufio]
:  ... acquired at:
:    lock_acquire+0xb2/0x2b0
:    _raw_spin_lock+0x28/0x70
:    adjust_total_allocated+0x95/0x120 [dm_bufio]
:    __link_buffer+0xb2/0xf0 [dm_bufio]
:    __bufio_new+0x20b/0x2b0 [dm_bufio]
:    dm_bufio_prefetch+0x90/0x1f0 [dm_bufio]
:    verity_prefetch_io+0x3c/0x180 [dm_verity]
:    process_one_work+0x246/0x530
:    worker_thread+0x47/0x3e0
:    kthread+0xd2/0x100
:    ret_from_fork+0x19/0x24
:
: \x0astack backtrace:
: CPU: 1 PID: 2488 Comm: kworker/u16:6 Not tainted 5.19.0-rc8+ #767
: Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 11/12/2020
: Workqueue: kverityd verity_prefetch_io [dm_verity]
: Call Trace:
:  dump_stack_lvl+0x68/0x98
:  dump_stack+0xd/0x10
:  print_bad_irq_dependency.cold+0x1f2/0x1f8
:  __lock_acquire+0x2522/0x2840
:  ? __this_cpu_preempt_check+0xf/0x11
:  lock_acquire+0xb2/0x2b0
:  ? adjust_total_allocated+0x95/0x120 [dm_bufio]
:  ? __this_cpu_preempt_check+0xf/0x11
:  _raw_spin_lock+0x28/0x70
:  ? adjust_total_allocated+0x95/0x120 [dm_bufio]
:  adjust_total_allocated+0x95/0x120 [dm_bufio]
:  __link_buffer+0xb2/0xf0 [dm_bufio]
:  ? alloc_buffer+0xc3/0x100 [dm_bufio]
:  __bufio_new+0x20b/0x2b0 [dm_bufio]
:  dm_bufio_prefetch+0x90/0x1f0 [dm_bufio]
:  verity_prefetch_io+0x3c/0x180 [dm_verity]
:  process_one_work+0x246/0x530
:  ? 0xc1000000
:  worker_thread+0x47/0x3e0
:  kthread+0xd2/0x100
:  ? process_one_work+0x530/0x530
:  ? kthread_complete_and_exit+0x20/0x20
:  ret_from_fork+0x19/0x24


m.

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


  parent reply	other threads:[~2022-07-26 21:44 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-07-26 16:09 [dm-devel] [PATCH v2 0/6] dm verity: optionally use tasklets Mike Snitzer
2022-07-26 16:09 ` [dm-devel] [PATCH v2 1/6] dm bufio: Add flags argument to dm_bufio_client_create Mike Snitzer
2022-07-26 16:09 ` [dm-devel] [PATCH v2 2/6] dm bufio: Add DM_BUFIO_CLIENT_NO_SLEEP flag Mike Snitzer
2022-07-27 15:25   ` Mikulas Patocka
2022-07-27 15:47     ` Mike Snitzer
2022-07-27 19:53       ` Nathan Huckleberry
2022-07-28 22:37         ` Mike Snitzer
2022-07-26 16:09 ` [dm-devel] [PATCH v2 3/6] dm verity: Add optional "try_verify_in_tasklet" feature Mike Snitzer
2022-07-26 16:09 ` [dm-devel] [PATCH v2 4/6] dm verity: allow optional args to alter primary args handling Mike Snitzer
2022-07-26 16:09 ` [dm-devel] [PATCH v2 5/6] dm bufio: conditionally enable branching for DM_BUFIO_CLIENT_NO_SLEEP Mike Snitzer
2022-07-26 16:09 ` [dm-devel] [PATCH v2 6/6] dm verity: conditionally enable branching for "try_verify_in_tasklet" Mike Snitzer
2022-07-26 20:18 ` [dm-devel] [PATCH v2 0/6] dm verity: optionally use tasklets Nathan Huckleberry
2022-07-26 21:44 ` Milan Broz [this message]
2022-07-26 23:04   ` Mike Snitzer
2022-07-27  8:23     ` Milan Broz
2022-08-03  1:39       ` Nathan Huckleberry
2022-08-03 16:17         ` Mike Snitzer
2022-08-03 18:29           ` [dm-devel] [PATCH] Fixes 6890e9b8c7d0a1062bbf4f854b6be3723836ad9a Nathan Huckleberry
2022-08-04 20:22             ` [dm-devel] " Mike Snitzer

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=6a158b48-10d3-6427-bc44-e7f78513380b@gmail.com \
    --to=gmazyland@gmail.com \
    --cc=dm-devel@redhat.com \
    --cc=ebiggers@kernel.org \
    --cc=nhuck@google.com \
    --cc=samitolvanen@google.com \
    --cc=snitzer@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body. 
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.