[dm-devel] [PATCH 2/6] dm crypt: Handle DM_CRYPT_NO_*_WORKQUEUE more explicit.

[Ignat Korchagin]

On Sat, Feb 13, 2021 at 11:11 AM Sebastian Andrzej Siewior
<bigeasy at linutronix.de> wrote:
>
> By looking at the handling of DM_CRYPT_NO_*_WORKQUEUE in
> kcryptd_queue_crypt() it appears that READ and WRITE requests might be
> handled in the tasklet context as long as interrupts are disabled or it
> is handled in hardirq context.
>
> The WRITE requests should always be fed in preemptible context. There
> are other requirements in the write path which sleep or acquire a mutex.
>
> The READ requests should come from the storage driver, likely not in a
> preemptible context. The source of the requests depends on the driver
> and other factors like multiple queues in the block layer.

My personal opinion: I really don't like the guesswork and
assumptions. If we want
to remove the usage of in_*irq() and alike, we should propagate the execution
context from the source. Storage drivers have this information and can
pass it on to the device-mapper framework, which in turn can pass it
on to dm modules.

Assuming WRITE requests are always in preemptible context might break with the
addition of some new type of obscure storage hardware.

In our testing we saw a lot of cases with SATA disks, where READ requests come
from preemptible contexts, so probably don't want to pay (no matter how small)
tasklet setup overhead, not to mention executing it in softirq, which
is hard later to
attribute to a specific process in metrics.

In other words, I think we should be providing support for this in the
device-mapper
framework itself, not start from individual modules.

> To simplify the handling of DM_CRYPT_NO_*_WORKQUEUE, handle READ
> requests always in tasklet/softirq context since the requests will be
> passed in hard or softirq context.
> Handle the WRITE requests directly because they are already in
> preemptible context and must not be passed to the taslket/softirq.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy at linutronix.de>
> ---
>  drivers/md/dm-crypt.c | 26 +++++++++-----------------
>  1 file changed, 9 insertions(+), 17 deletions(-)
>
> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
> index 506655e5eecba..a498de3604a67 100644
> --- a/drivers/md/dm-crypt.c
> +++ b/drivers/md/dm-crypt.c
> @@ -2215,30 +2215,22 @@ static void kcryptd_crypt_tasklet(struct tasklet_struct *t)
>  {
>         struct dm_crypt_io *io = from_tasklet(io, t, tasklet);
>
> -       if (bio_data_dir(io->base_bio) == READ)
> -               kcryptd_crypt_read_convert(io);
> -       else
> -               kcryptd_crypt_write_convert(io);
Should we add BUG_ON(bio_data_dir(io->base_bio) != READ) here?
> +       kcryptd_crypt_read_convert(io);
>  }
>
>  static void kcryptd_queue_crypt(struct dm_crypt_io *io)
>  {
>         struct crypt_config *cc = io->cc;
>
> -       if ((bio_data_dir(io->base_bio) == READ && test_bit(DM_CRYPT_NO_READ_WORKQUEUE, &cc->flags)) ||
> -           (bio_data_dir(io->base_bio) == WRITE && test_bit(DM_CRYPT_NO_WRITE_WORKQUEUE, &cc->flags))) {
> -               /*
> -                * in_irq(): Crypto API's skcipher_walk_first() refuses to work in hard IRQ context.
> -                * irqs_disabled(): the kernel may run some IO completion from the idle thread, but
> -                * it is being executed with irqs disabled.
> -                */
> -               if (in_irq() || irqs_disabled()) {
> -                       tasklet_setup(&io->tasklet, kcryptd_crypt_tasklet);
> -                       tasklet_schedule(&io->tasklet);
> -                       return;
> -               }
> +       if (bio_data_dir(io->base_bio) == READ &&
> +           test_bit(DM_CRYPT_NO_READ_WORKQUEUE, &cc->flags)) {
> +               tasklet_setup(&io->tasklet, kcryptd_crypt_tasklet);
> +               tasklet_schedule(&io->tasklet);
> +               return;
>
> -               kcryptd_crypt(&io->work);
> +       } else if (bio_data_dir(io->base_bio) == WRITE &&
> +                  test_bit(DM_CRYPT_NO_WRITE_WORKQUEUE, &cc->flags)) {
> +               kcryptd_crypt_write_convert(io);
>                 return;
>         }
>
> --
> 2.30.0
>