[PATCH 0/3] dm-verity: optionally use tasklets in dm-verity

Nathan Huckleberry posted 3 patches 3 years, 8 months ago
drivers/md/dm-bufio.c                         | 29 +++++--
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                 | 87 ++++++++++++++++---
drivers/md/dm-verity.h                        |  5 ++
drivers/md/persistent-data/dm-block-manager.c |  3 +-
include/linux/dm-bufio.h                      |  8 +-
9 files changed, 119 insertions(+), 24 deletions(-)
[PATCH 0/3] dm-verity: optionally use tasklets in dm-verity
Posted by Nathan Huckleberry 3 years, 8 months ago
Using tasklets for disk verification can reduce IO latency.  When there are
accelerated hash instructions it is often better to compute the hash immediately
using a tasklet rather than deferring verification to a work-queue.  This
reduces time spent waiting to schedule work-queue jobs, but requires spending
slightly more time in interrupt context.

A tasklet can only be used for verification if all the required hashes are
already in the dm-bufio cache.  If verification cannot be done in a tasklet, we
fallback the existing work-queue implementation.

To allow tasklets to query the dm-bufio cache, the dm-bufio code must not sleep.
This patchset adds a flag to dm-bufio that disallows sleeping.

The following shows a speed comparison of random reads on a dm-verity device.
The dm-verity device uses a 1G ramdisk for data and a 1G ramdisk for hashes.
One test was run using tasklets and one test was run using the existing
work-queue solution.  Both tests were run when the dm-bufio cache was hot.  The
tasklet implementation performs significantly better since there is no time
waiting for work-queue jobs to be scheduled.

# /data/fio /data/tasklet.fio | grep READ
   READ: bw=181MiB/s (190MB/s), 181MiB/s-181MiB/s (190MB/s-190MB/s), io=512MiB
   (537MB), run=2827-2827msec
# /data/fio /data/workqueue.fio | grep READ
   READ: bw=23.6MiB/s (24.8MB/s), 23.6MiB/s-23.6MiB/s (24.8MB/s-24.8MB/s),
   io=512MiB (537MB), run=21688-21688msec

Nathan Huckleberry (3):
  dm-bufio: Add flags for dm_bufio_client_create
  dm-bufio: Add DM_BUFIO_GET_CANT_SLEEP
  dm-verity: Add try_verify_in_tasklet

 drivers/md/dm-bufio.c                         | 29 +++++--
 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                 | 87 ++++++++++++++++---
 drivers/md/dm-verity.h                        |  5 ++
 drivers/md/persistent-data/dm-block-manager.c |  3 +-
 include/linux/dm-bufio.h                      |  8 +-
 9 files changed, 119 insertions(+), 24 deletions(-)

-- 
2.37.1.359.gd136c6c3e2-goog
Re: [PATCH 0/3] dm-verity: optionally use tasklets in dm-verity
Posted by Christoph Hellwig 3 years, 8 months ago
We've been tying to kill off task lets for about 15 years.  I don't
think adding new users will make you a whole lot of friends..

On Fri, Jul 22, 2022 at 09:38:20AM +0000, Nathan Huckleberry wrote:
> Using tasklets for disk verification can reduce IO latency.  When there are
> accelerated hash instructions it is often better to compute the hash immediately
> using a tasklet rather than deferring verification to a work-queue.  This
> reduces time spent waiting to schedule work-queue jobs, but requires spending
> slightly more time in interrupt context.
> 
> A tasklet can only be used for verification if all the required hashes are
> already in the dm-bufio cache.  If verification cannot be done in a tasklet, we
> fallback the existing work-queue implementation.
> 
> To allow tasklets to query the dm-bufio cache, the dm-bufio code must not sleep.
> This patchset adds a flag to dm-bufio that disallows sleeping.
> 
> The following shows a speed comparison of random reads on a dm-verity device.
> The dm-verity device uses a 1G ramdisk for data and a 1G ramdisk for hashes.
> One test was run using tasklets and one test was run using the existing
> work-queue solution.  Both tests were run when the dm-bufio cache was hot.  The
> tasklet implementation performs significantly better since there is no time
> waiting for work-queue jobs to be scheduled.
> 
> # /data/fio /data/tasklet.fio | grep READ
>    READ: bw=181MiB/s (190MB/s), 181MiB/s-181MiB/s (190MB/s-190MB/s), io=512MiB
>    (537MB), run=2827-2827msec
> # /data/fio /data/workqueue.fio | grep READ
>    READ: bw=23.6MiB/s (24.8MB/s), 23.6MiB/s-23.6MiB/s (24.8MB/s-24.8MB/s),
>    io=512MiB (537MB), run=21688-21688msec
> 
> Nathan Huckleberry (3):
>   dm-bufio: Add flags for dm_bufio_client_create
>   dm-bufio: Add DM_BUFIO_GET_CANT_SLEEP
>   dm-verity: Add try_verify_in_tasklet
> 
>  drivers/md/dm-bufio.c                         | 29 +++++--
>  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                 | 87 ++++++++++++++++---
>  drivers/md/dm-verity.h                        |  5 ++
>  drivers/md/persistent-data/dm-block-manager.c |  3 +-
>  include/linux/dm-bufio.h                      |  8 +-
>  9 files changed, 119 insertions(+), 24 deletions(-)
> 
> -- 
> 2.37.1.359.gd136c6c3e2-goog
> 
---end quoted text---
Re: [dm-devel] [PATCH 0/3] dm-verity: optionally use tasklets in dm-verity
Posted by Bart Van Assche 3 years, 8 months ago
On 7/22/22 09:41, Christoph Hellwig wrote:
> We've been tying to kill off task lets for about 15 years.  I don't
> think adding new users will make you a whole lot of friends..

+1 for not using tasklets. At least in Android the real-time thread 
scheduling latency is important. Tasklets are not visible to the 
scheduler and hence cause latency spikes for real-time threads. These 
latency spikes can be observed by users, e.g. if the real-time thread is 
involved in audio playback.

Bart.
Re: [PATCH 0/3] dm-verity: optionally use tasklets in dm-verity
Posted by Mike Snitzer 3 years, 8 months ago
On Fri, Jul 22 2022 at  2:12P -0400,
Bart Van Assche <bvanassche@acm.org> wrote:

> On 7/22/22 09:41, Christoph Hellwig wrote:
> > We've been tying to kill off task lets for about 15 years.  I don't
> > think adding new users will make you a whole lot of friends..
> 
> +1 for not using tasklets. At least in Android the real-time thread
> scheduling latency is important. Tasklets are not visible to the scheduler
> and hence cause latency spikes for real-time threads. These latency spikes
> can be observed by users, e.g. if the real-time thread is involved in audio
> playback.

OK, then android wouldn't set the _optional_ "try_verify_in_tasklet"

Mike
Re: [PATCH 0/3] dm-verity: optionally use tasklets in dm-verity
Posted by Mike Snitzer 3 years, 8 months ago
On Fri, Jul 22 2022 at 12:41P -0400,
Christoph Hellwig <hch@infradead.org> wrote:

> We've been tying to kill off task lets for about 15 years.  I don't
> think adding new users will make you a whole lot of friends..

I don't have perspective on how serious that effort is. But ~2 years
ago DM introduced another consumer of tasklets in dm-crypt, see:
39d42fa96ba1 dm crypt: add flags to optionally bypass kcryptd workqueues

Given that, and other numerous users, is the effort to remove tasklets
valid? What is the alternative to tasklets?

Mike
Re: [PATCH 0/3] dm-verity: optionally use tasklets in dm-verity
Posted by Sebastian Andrzej Siewior 3 years, 8 months ago
On 2022-07-22 13:12:36 [-0400], Mike Snitzer wrote:
> On Fri, Jul 22 2022 at 12:41P -0400,
> Christoph Hellwig <hch@infradead.org> wrote:
> 
> > We've been tying to kill off task lets for about 15 years.  I don't
> > think adding new users will make you a whole lot of friends..
> 
> I don't have perspective on how serious that effort is. But ~2 years
> ago DM introduced another consumer of tasklets in dm-crypt, see:
> 39d42fa96ba1 dm crypt: add flags to optionally bypass kcryptd workqueues

I tried to get rid of the in_atomic() as it appeared work "magic" in
there and in ended in a pointless discussion…

> Given that, and other numerous users, is the effort to remove tasklets
> valid? What is the alternative to tasklets?

The tasklets end up as anonymous load in the system. It is usually not
visible due to the way accounting usually works (yes we do have full
accounting) and you can't distinguish between work from USB-cam,
storage, … if everything is fed into the same context. This becomes a
problem on a smaller/ slower system of one softirq throttles the other
(say the webcam processing gets delayed due to other tasklets).

With the tasklet/BH context you need to disable BH while acquiring a
spin_lock() so this ends up a per-CPU BKL since a random spin_lock_bh()
is also synchronised again the timer as well as large parts of the
networking subsystem and so on. This seems not to bother anyone in
general it becomes a problem on PREEMPT_RT where this becomes visible.

In general, a tasklet runs after the interrupt handler and were
introduced a long time ago, before we had threaded interrupts available.
Therefore threaded interrupts are a good substitute.

> Mike

Sebastian