[dm-devel] [PATCH 0/6] dm crypt: Replace the usage of in_interrupt().

[dm-devel] [PATCH 0/6] dm crypt: Replace the usage of in_interrupt().

[Sebastian Andrzej Siewior]

Folks,

in the discussion about preempt count consistency across kernel
configurations:

   https://lore.kernel.org/r/20200914204209.256266093@linutronix.de/

it was concluded that the usage of in_interrupt() and related context
checks should be removed from non-core code.

In the long run, usage of 'preemptible, in_*irq etc.' should be banned from
driver code completely.

This series addresses dm-crypt related usage of the macro. There are no
mode users in drivers/md.

Sebastian



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

[dm-devel] [PATCH 1/6] dm crypt: Use tasklet_setup().

[Sebastian Andrzej Siewior]

tasklet_setup() has the beauty of passing the argument as part of the
structure instead of an integer value which needs casting.

Use tasklet_setup().

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/md/dm-crypt.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 68be387581a7e..506655e5eecba 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -2211,9 +2211,14 @@ static void kcryptd_crypt(struct work_struct *work)
 		kcryptd_crypt_write_convert(io);
 }
 
-static void kcryptd_crypt_tasklet(unsigned long work)
+static void kcryptd_crypt_tasklet(struct tasklet_struct *t)
 {
-	kcryptd_crypt((struct work_struct *)work);
+	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);
 }
 
 static void kcryptd_queue_crypt(struct dm_crypt_io *io)
@@ -2228,7 +2233,7 @@ static void kcryptd_queue_crypt(struct dm_crypt_io *io)
 		 * it is being executed with irqs disabled.
 		 */
 		if (in_irq() || irqs_disabled()) {
-			tasklet_init(&io->tasklet, kcryptd_crypt_tasklet, (unsigned long)&io->work);
+			tasklet_setup(&io->tasklet, kcryptd_crypt_tasklet);
 			tasklet_schedule(&io->tasklet);
 			return;
 		}
-- 
2.30.0


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

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

[Sebastian Andrzej Siewior]

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.

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@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);
+	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


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

[dm-devel] [PATCH 3/6] dm crypt: Add 'atomic' argument to kcryptd_crypt_read_convert()

[Sebastian Andrzej Siewior]

kcryptd_crypt_read_convert() can be invoked from atomic context
(softirq/tasklet) and preemptible context (the workqueue).

Add an argument `atomic' to kcryptd_crypt_read_convert().
This argument can be passed to crypt_convert() replacing the test for
DM_CRYPT_NO_READ_WORKQUEUE.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/md/dm-crypt.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index a498de3604a67..f5eafc32d32c5 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -2120,7 +2120,7 @@ static void kcryptd_crypt_read_continue(struct work_struct *work)
 	crypt_dec_pending(io);
 }
 
-static void kcryptd_crypt_read_convert(struct dm_crypt_io *io)
+static void kcryptd_crypt_read_convert(struct dm_crypt_io *io, bool atomic)
 {
 	struct crypt_config *cc = io->cc;
 	blk_status_t r;
@@ -2130,8 +2130,7 @@ static void kcryptd_crypt_read_convert(struct dm_crypt_io *io)
 	crypt_convert_init(cc, &io->ctx, io->base_bio, io->base_bio,
 			   io->sector);
 
-	r = crypt_convert(cc, &io->ctx,
-			  test_bit(DM_CRYPT_NO_READ_WORKQUEUE, &cc->flags), true);
+	r = crypt_convert(cc, &io->ctx, atomic, true);
 	/*
 	 * Crypto API backlogged the request, because its queue was full
 	 * and we're in softirq context, so continue from a workqueue
@@ -2206,7 +2205,7 @@ static void kcryptd_crypt(struct work_struct *work)
 	struct dm_crypt_io *io = container_of(work, struct dm_crypt_io, work);
 
 	if (bio_data_dir(io->base_bio) == READ)
-		kcryptd_crypt_read_convert(io);
+		kcryptd_crypt_read_convert(io, false);
 	else
 		kcryptd_crypt_write_convert(io);
 }
@@ -2215,7 +2214,7 @@ static void kcryptd_crypt_tasklet(struct tasklet_struct *t)
 {
 	struct dm_crypt_io *io = from_tasklet(io, t, tasklet);
 
-	kcryptd_crypt_read_convert(io);
+	kcryptd_crypt_read_convert(io, true);
 }
 
 static void kcryptd_queue_crypt(struct dm_crypt_io *io)
-- 
2.30.0


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

[dm-devel] [PATCH 4/6] dm crypt: Revisit the atomic argument passed to crypt_convert().

[Sebastian Andrzej Siewior]

The atomic argument of crypto_convert() is used to decide if
cond_resched() may be invoked.

kcryptd_crypt_write_continue() and kcryptd_crypt_read_continue() pass
true here but both are invoked by a worker where scheduling is possible.

kcryptd_crypt_write_convert() is invoked from preemptible context even
if DM_CRYPT_NO_WRITE_WORKQUEUE is set.

Set the atomic argument to false in the three cases because
cond_resched() is not forbidden.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/md/dm-crypt.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index f5eafc32d32c5..1151a0108ae78 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -2019,7 +2019,7 @@ static void kcryptd_crypt_write_continue(struct work_struct *work)
 	wait_for_completion(&ctx->restart);
 	reinit_completion(&ctx->restart);
 
-	r = crypt_convert(cc, &io->ctx, true, false);
+	r = crypt_convert(cc, &io->ctx, false, false);
 	if (r)
 		io->error = r;
 	crypt_finished = atomic_dec_and_test(&ctx->cc_pending);
@@ -2065,8 +2065,7 @@ static void kcryptd_crypt_write_convert(struct dm_crypt_io *io)
 	sector += bio_sectors(clone);
 
 	crypt_inc_pending(io);
-	r = crypt_convert(cc, ctx,
-			  test_bit(DM_CRYPT_NO_WRITE_WORKQUEUE, &cc->flags), true);
+	r = crypt_convert(cc, ctx, false, true);
 	/*
 	 * Crypto API backlogged the request, because its queue was full
 	 * and we're in softirq context, so continue from a workqueue
@@ -2110,7 +2109,7 @@ static void kcryptd_crypt_read_continue(struct work_struct *work)
 	wait_for_completion(&io->ctx.restart);
 	reinit_completion(&io->ctx.restart);
 
-	r = crypt_convert(cc, &io->ctx, true, false);
+	r = crypt_convert(cc, &io->ctx, false, false);
 	if (r)
 		io->error = r;
 
-- 
2.30.0


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

[dm-devel] [PATCH 5/6] dm crypt: Replace the in_interrupt() usage in crypt_convert().

[Sebastian Andrzej Siewior]

crypt_convert() is always invoked with atomic == true if invoked from an
atomic context.

The usage of in_interrupt() in drivers is phased out and Linus clearly
requested that code which changes behaviour depending on context should
either be separated or the context be conveyed in an argument passed by
the caller, which usually knows the context.

Use the `atomic' argument instead of in_interrupt() to check if it is
safe to wait on a completion.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/md/dm-crypt.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 1151a0108ae78..0cdfee10d5a23 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -1575,7 +1575,7 @@ static blk_status_t crypt_convert(struct crypt_config *cc,
 		 * but the driver request queue is full, let's wait.
 		 */
 		case -EBUSY:
-			if (in_interrupt()) {
+			if (atomic) {
 				if (try_wait_for_completion(&ctx->restart)) {
 					/*
 					 * we don't have to block to wait for completion,
-- 
2.30.0


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

[dm-devel] [PATCH 6/6] dm crypt: Use `atomic' argument for memory allocation.

[Sebastian Andrzej Siewior]

crypt_alloc_req_*() is using in_interrupt() to figure out the correct
gfp_t mask for memory allocation.

The usage of in_interrupt() in non-core code is phased out. Ideally the
information of the calling context should be passed by the callers or the
functions be split as appropriate.

The top-most caller has already an `atomic' argument which is true if invoked
from an atomic context.

Use the `atomic' argument to create an allocation mask and pass it down
to crypt_alloc_req_*().

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/md/dm-crypt.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index 0cdfee10d5a23..40c35efb9e929 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -1456,12 +1456,12 @@ static void kcryptd_async_done(struct crypto_async_request *async_req,
 			       int error);
 
 static int crypt_alloc_req_skcipher(struct crypt_config *cc,
-				     struct convert_context *ctx)
+				    struct convert_context *ctx, gfp_t gfp)
 {
 	unsigned key_index = ctx->cc_sector & (cc->tfms_count - 1);
 
 	if (!ctx->r.req) {
-		ctx->r.req = mempool_alloc(&cc->req_pool, in_interrupt() ? GFP_ATOMIC : GFP_NOIO);
+		ctx->r.req = mempool_alloc(&cc->req_pool, gfp);
 		if (!ctx->r.req)
 			return -ENOMEM;
 	}
@@ -1480,10 +1480,10 @@ static int crypt_alloc_req_skcipher(struct crypt_config *cc,
 }
 
 static int crypt_alloc_req_aead(struct crypt_config *cc,
-				 struct convert_context *ctx)
+				struct convert_context *ctx, gfp_t gfp)
 {
 	if (!ctx->r.req_aead) {
-		ctx->r.req_aead = mempool_alloc(&cc->req_pool, in_interrupt() ? GFP_ATOMIC : GFP_NOIO);
+		ctx->r.req_aead = mempool_alloc(&cc->req_pool, gfp);
 		if (!ctx->r.req_aead)
 			return -ENOMEM;
 	}
@@ -1501,13 +1501,13 @@ static int crypt_alloc_req_aead(struct crypt_config *cc,
 	return 0;
 }
 
-static int crypt_alloc_req(struct crypt_config *cc,
-			    struct convert_context *ctx)
+static int crypt_alloc_req(struct crypt_config *cc, struct convert_context *ctx,
+			   gfp_t gfp)
 {
 	if (crypt_integrity_aead(cc))
-		return crypt_alloc_req_aead(cc, ctx);
+		return crypt_alloc_req_aead(cc, ctx, gfp);
 	else
-		return crypt_alloc_req_skcipher(cc, ctx);
+		return crypt_alloc_req_skcipher(cc, ctx, gfp);
 }
 
 static void crypt_free_req_skcipher(struct crypt_config *cc,
@@ -1556,7 +1556,7 @@ static blk_status_t crypt_convert(struct crypt_config *cc,
 
 	while (ctx->iter_in.bi_size && ctx->iter_out.bi_size) {
 
-		r = crypt_alloc_req(cc, ctx);
+		r = crypt_alloc_req(cc, ctx, atomic ? GFP_ATOMIC : GFP_NOIO);
 		if (r) {
 			complete(&ctx->restart);
 			return BLK_STS_DEV_RESOURCE;
-- 
2.30.0


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

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

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

Re: [dm-devel] [PATCH 4/6] dm crypt: Revisit the atomic argument passed to crypt_convert().

[Ignat Korchagin]

On Sat, Feb 13, 2021 at 11:11 AM Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
>
> The atomic argument of crypto_convert() is used to decide if
> cond_resched() may be invoked.
>
> kcryptd_crypt_write_continue() and kcryptd_crypt_read_continue() pass
> true here but both are invoked by a worker where scheduling is possible.
>
> kcryptd_crypt_write_convert() is invoked from preemptible context even
> if DM_CRYPT_NO_WRITE_WORKQUEUE is set.
>
> Set the atomic argument to false in the three cases because
> cond_resched() is not forbidden.

"atomic" parameter name might be confusing here as usually it is
related to the execution context.
But here it has additional meaning - it is also bound to the
DM_NO_*_WORKQUEUE flags. Basically,
as a user, if I set these flags - my intention is that dm-crypt should
process requests ASAP, so I don't
want it to voluntarily yield CPU even if the context is preemptible.

> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/md/dm-crypt.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
> index f5eafc32d32c5..1151a0108ae78 100644
> --- a/drivers/md/dm-crypt.c
> +++ b/drivers/md/dm-crypt.c
> @@ -2019,7 +2019,7 @@ static void kcryptd_crypt_write_continue(struct work_struct *work)
>         wait_for_completion(&ctx->restart);
>         reinit_completion(&ctx->restart);
>
> -       r = crypt_convert(cc, &io->ctx, true, false);
> +       r = crypt_convert(cc, &io->ctx, false, false);
>         if (r)
>                 io->error = r;
>         crypt_finished = atomic_dec_and_test(&ctx->cc_pending);
> @@ -2065,8 +2065,7 @@ static void kcryptd_crypt_write_convert(struct dm_crypt_io *io)
>         sector += bio_sectors(clone);
>
>         crypt_inc_pending(io);
> -       r = crypt_convert(cc, ctx,
> -                         test_bit(DM_CRYPT_NO_WRITE_WORKQUEUE, &cc->flags), true);
> +       r = crypt_convert(cc, ctx, false, true);
>         /*
>          * Crypto API backlogged the request, because its queue was full
>          * and we're in softirq context, so continue from a workqueue
> @@ -2110,7 +2109,7 @@ static void kcryptd_crypt_read_continue(struct work_struct *work)
>         wait_for_completion(&io->ctx.restart);
>         reinit_completion(&io->ctx.restart);
>
> -       r = crypt_convert(cc, &io->ctx, true, false);
> +       r = crypt_convert(cc, &io->ctx, false, false);
>         if (r)
>                 io->error = r;
>
> --
> 2.30.0
>

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

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

[Mike Snitzer]

On Sat, Feb 13 2021 at  9:31am -0500,
Ignat Korchagin <ignat@cloudflare.com> wrote:

> On Sat, Feb 13, 2021 at 11:11 AM Sebastian Andrzej Siewior
> <bigeasy@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.

I'm missing where DM core has the opportunity to convey this context in
a clean manner.

Any quick patch that shows the type of transform you'd like to see would
be appreciated.. doesn't need to be comprehensive, just enough for me or
others to carry through to completion.
 
> 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.

I think your concerns are valid... it does seem like this patch is
assuming too much.

Mike

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

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

[Ignat Korchagin]

On Thu, Mar 11, 2021 at 6:25 PM Mike Snitzer <snitzer@redhat.com> wrote:
>
> On Sat, Feb 13 2021 at  9:31am -0500,
> Ignat Korchagin <ignat@cloudflare.com> wrote:
>
> > On Sat, Feb 13, 2021 at 11:11 AM Sebastian Andrzej Siewior
> > <bigeasy@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.
>
> I'm missing where DM core has the opportunity to convey this context in
> a clean manner.

Does DM core currently even have this context from the drivers?

> Any quick patch that shows the type of transform you'd like to see would
> be appreciated.. doesn't need to be comprehensive, just enough for me or
> others to carry through to completion.

I didn't think it through well, but from the top of my head maybe the
execution context
info can be passed over between different storage layers in the bio
structure? For example,
if a driver completes a read in interrupt context - it sets a flag in
the bio structure and passes
it up the stack. Later, if an intermediate layer changes the execution
context (for example,
dm-crypt offloading the bio processing to a workqueue), that layer
updates the flag and so
on. The same applies to write path: writes are generally started in a
preemptible context, but
if we have some obscure DM module, which will schedule a tasklet for a
write, that module must
update the flag in the bio structure.

Basically, the idea is that a bio processing code will get the current
execution context from an upper/lower
layer and if the code itself changes the execution context, that code
is able to update the execution context
info in the bio before passing it on.

This thinking may be flawed of course as I don't know enough details
about the Linux storage layers and how
well the ownership of bios are defined.

> > 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.
>
> I think your concerns are valid... it does seem like this patch is
> assuming too much.
>
> Mike
>

Ignat

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