[PATCH 6.6] Revert "dm-crypt, dm-verity: disable tasklets"

[PATCH 6.6] Revert "dm-crypt, dm-verity: disable tasklets"

[Li Lingfeng]

This reverts commit 5735a2671ffb70ea29ca83969fe01316ee2ed6fc which is
commit 0a9bab391e336489169b95cb0d4553d921302189 upstream.

Tasklet is thought to cause memory corruption [1], so it was disabled in
dm-crypt and dm-verity. However, memory corruption may not happen since
cc->io_queue is created without WQ_UNBOUND [2].
Revert commit 5735a2671ffb ("dm-crypt, dm-verity: disable tasklets") to
bring tasklet back.

[1] https://lore.kernel.org/all/d390d7ee-f142-44d3-822a-87949e14608b@suse.de/T/
[2] https://lore.kernel.org/all/4d331659-badd-749d-fba1-271543631a8a@huawei.com/

Signed-off-by: Li Lingfeng <lilingfeng3@huawei.com>
---
 drivers/md/dm-crypt.c         | 38 +++++++++++++++++++++++++++++++++--
 drivers/md/dm-verity-target.c | 26 ++++++++++++++++++++++--
 drivers/md/dm-verity.h        |  1 +
 3 files changed, 61 insertions(+), 4 deletions(-)

diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
index aa6bb5b4704b..a60d91d02e28 100644
--- a/drivers/md/dm-crypt.c
+++ b/drivers/md/dm-crypt.c
@@ -75,8 +75,10 @@ struct dm_crypt_io {
 	struct bio *base_bio;
 	u8 *integrity_metadata;
 	bool integrity_metadata_from_pool:1;
+	bool in_tasklet:1;
 
 	struct work_struct work;
+	struct tasklet_struct tasklet;
 
 	struct convert_context ctx;
 
@@ -1775,6 +1777,7 @@ static void crypt_io_init(struct dm_crypt_io *io, struct crypt_config *cc,
 	io->ctx.r.req = NULL;
 	io->integrity_metadata = NULL;
 	io->integrity_metadata_from_pool = false;
+	io->in_tasklet = false;
 	atomic_set(&io->io_pending, 0);
 }
 
@@ -1785,6 +1788,13 @@ static void crypt_inc_pending(struct dm_crypt_io *io)
 
 static void kcryptd_queue_read(struct dm_crypt_io *io);
 
+static void kcryptd_io_bio_endio(struct work_struct *work)
+{
+	struct dm_crypt_io *io = container_of(work, struct dm_crypt_io, work);
+
+	bio_endio(io->base_bio);
+}
+
 /*
  * One of the bios was finished. Check for completion of
  * the whole request and correctly clean up the buffer.
@@ -1817,6 +1827,20 @@ static void crypt_dec_pending(struct dm_crypt_io *io)
 
 	base_bio->bi_status = error;
 
+	/*
+	 * If we are running this function from our tasklet,
+	 * we can't call bio_endio() here, because it will call
+	 * clone_endio() from dm.c, which in turn will
+	 * free the current struct dm_crypt_io structure with
+	 * our tasklet. In this case we need to delay bio_endio()
+	 * execution to after the tasklet is done and dequeued.
+	 */
+	if (io->in_tasklet) {
+		INIT_WORK(&io->work, kcryptd_io_bio_endio);
+		queue_work(cc->io_queue, &io->work);
+		return;
+	}
+
 	bio_endio(base_bio);
 }
 
@@ -2291,6 +2315,11 @@ static void kcryptd_crypt(struct work_struct *work)
 		kcryptd_crypt_write_convert(io);
 }
 
+static void kcryptd_crypt_tasklet(unsigned long work)
+{
+	kcryptd_crypt((struct work_struct *)work);
+}
+
 static void kcryptd_queue_crypt(struct dm_crypt_io *io)
 {
 	struct crypt_config *cc = io->cc;
@@ -2302,10 +2331,15 @@ static void kcryptd_queue_crypt(struct dm_crypt_io *io)
 		 * irqs_disabled(): the kernel may run some IO completion from the idle thread, but
 		 * it is being executed with irqs disabled.
 		 */
-		if (!(in_hardirq() || irqs_disabled())) {
-			kcryptd_crypt(&io->work);
+		if (in_hardirq() || irqs_disabled()) {
+			io->in_tasklet = true;
+			tasklet_init(&io->tasklet, kcryptd_crypt_tasklet, (unsigned long)&io->work);
+			tasklet_schedule(&io->tasklet);
 			return;
 		}
+
+		kcryptd_crypt(&io->work);
+		return;
 	}
 
 	INIT_WORK(&io->work, kcryptd_crypt);
diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
index 49e4a35d7019..0bb126eadc0d 100644
--- a/drivers/md/dm-verity-target.c
+++ b/drivers/md/dm-verity-target.c
@@ -701,6 +701,23 @@ static void verity_work(struct work_struct *w)
 	verity_finish_io(io, errno_to_blk_status(verity_verify_io(io)));
 }
 
+static void verity_tasklet(unsigned long data)
+{
+	struct dm_verity_io *io = (struct dm_verity_io *)data;
+	int err;
+
+	io->in_tasklet = true;
+	err = verity_verify_io(io);
+	if (err == -EAGAIN || err == -ENOMEM) {
+		/* fallback to retrying with work-queue */
+		INIT_WORK(&io->work, verity_work);
+		queue_work(io->v->verify_wq, &io->work);
+		return;
+	}
+
+	verity_finish_io(io, errno_to_blk_status(err));
+}
+
 static void verity_end_io(struct bio *bio)
 {
 	struct dm_verity_io *io = bio->bi_private;
@@ -713,8 +730,13 @@ static void verity_end_io(struct bio *bio)
 		return;
 	}
 
-	INIT_WORK(&io->work, verity_work);
-	queue_work(io->v->verify_wq, &io->work);
+	if (static_branch_unlikely(&use_tasklet_enabled) && io->v->use_tasklet) {
+		tasklet_init(&io->tasklet, verity_tasklet, (unsigned long)io);
+		tasklet_schedule(&io->tasklet);
+	} else {
+		INIT_WORK(&io->work, verity_work);
+		queue_work(io->v->verify_wq, &io->work);
+	}
 }
 
 /*
diff --git a/drivers/md/dm-verity.h b/drivers/md/dm-verity.h
index db93a91169d5..7e495cc375b0 100644
--- a/drivers/md/dm-verity.h
+++ b/drivers/md/dm-verity.h
@@ -87,6 +87,7 @@ struct dm_verity_io {
 	bool in_tasklet;
 
 	struct work_struct work;
+	struct tasklet_struct tasklet;
 
 	char *recheck_buffer;
 
-- 
2.31.1

Re: [PATCH 6.6] Revert "dm-crypt, dm-verity: disable tasklets"

[Greg KH]

On Thu, Apr 11, 2024 at 05:15:39PM +0800, Li Lingfeng wrote:
> This reverts commit 5735a2671ffb70ea29ca83969fe01316ee2ed6fc which is
> commit 0a9bab391e336489169b95cb0d4553d921302189 upstream.
> 
> Tasklet is thought to cause memory corruption [1], so it was disabled in
> dm-crypt and dm-verity. However, memory corruption may not happen since
> cc->io_queue is created without WQ_UNBOUND [2].
> Revert commit 5735a2671ffb ("dm-crypt, dm-verity: disable tasklets") to
> bring tasklet back.
> 
> [1] https://lore.kernel.org/all/d390d7ee-f142-44d3-822a-87949e14608b@suse.de/T/
> [2] https://lore.kernel.org/all/4d331659-badd-749d-fba1-271543631a8a@huawei.com/
> 
> Signed-off-by: Li Lingfeng <lilingfeng3@huawei.com>
> ---
>  drivers/md/dm-crypt.c         | 38 +++++++++++++++++++++++++++++++++--
>  drivers/md/dm-verity-target.c | 26 ++++++++++++++++++++++--
>  drivers/md/dm-verity.h        |  1 +
>  3 files changed, 61 insertions(+), 4 deletions(-)

What is the commit id of this in Linus's tree?  And why only 6.6.y?

thanks,

greg k-h

Re: [PATCH 6.6] Revert "dm-crypt, dm-verity: disable tasklets"

[Li Lingfeng]

There is no need to revert it in Linus's tree since the expanded 
workqueue can work like tasklet.
https://lore.kernel.org/lkml/20240130091300.2968534-1-tj@kernel.org/

I'm not sure if the problem described in [1] could be triggered by a 
scenario I haven't considered. So I only send the revert patch to 6.6 
mainly for comments.
Of course, I think the other branches need it too.

Thanks

在 2024/4/11 17:52, Greg KH 写道:
> On Thu, Apr 11, 2024 at 05:15:39PM +0800, Li Lingfeng wrote:
>> This reverts commit 5735a2671ffb70ea29ca83969fe01316ee2ed6fc which is
>> commit 0a9bab391e336489169b95cb0d4553d921302189 upstream.
>>
>> Tasklet is thought to cause memory corruption [1], so it was disabled in
>> dm-crypt and dm-verity. However, memory corruption may not happen since
>> cc->io_queue is created without WQ_UNBOUND [2].
>> Revert commit 5735a2671ffb ("dm-crypt, dm-verity: disable tasklets") to
>> bring tasklet back.
>>
>> [1] https://lore.kernel.org/all/d390d7ee-f142-44d3-822a-87949e14608b@suse.de/T/
>> [2] https://lore.kernel.org/all/4d331659-badd-749d-fba1-271543631a8a@huawei.com/
>>
>> Signed-off-by: Li Lingfeng <lilingfeng3@huawei.com>
>> ---
>>   drivers/md/dm-crypt.c         | 38 +++++++++++++++++++++++++++++++++--
>>   drivers/md/dm-verity-target.c | 26 ++++++++++++++++++++++--
>>   drivers/md/dm-verity.h        |  1 +
>>   3 files changed, 61 insertions(+), 4 deletions(-)
> What is the commit id of this in Linus's tree?  And why only 6.6.y?
>
> thanks,
>
> greg k-h
>

Re: [PATCH 6.6] Revert "dm-crypt, dm-verity: disable tasklets"

[Mikulas Patocka]

Hi

I NACK this.


On Thu, 11 Apr 2024, Li Lingfeng wrote:

> This reverts commit 5735a2671ffb70ea29ca83969fe01316ee2ed6fc which is
> commit 0a9bab391e336489169b95cb0d4553d921302189 upstream.
> 
> Tasklet is thought to cause memory corruption [1], so it was disabled in
> dm-crypt and dm-verity. However, memory corruption may not happen since
> cc->io_queue is created without WQ_UNBOUND [2].
> Revert commit 5735a2671ffb ("dm-crypt, dm-verity: disable tasklets") to
> bring tasklet back.
> 
> [1] https://lore.kernel.org/all/d390d7ee-f142-44d3-822a-87949e14608b@suse.de/T/
> [2] https://lore.kernel.org/all/4d331659-badd-749d-fba1-271543631a8a@huawei.com/

Regarding [2] - if you add mdelay, you can make the race condition less 
reproducible, but it is not a proper fix and the race condition stays 
there unfixed.

Workqueues and ksoftirqd may be scheduled arbitrarily, there is no 
guarantee that they will be executed in a particular order, even if they 
are executed on the same CPU.

Mikulas

> Signed-off-by: Li Lingfeng <lilingfeng3@huawei.com>
> ---
>  drivers/md/dm-crypt.c         | 38 +++++++++++++++++++++++++++++++++--
>  drivers/md/dm-verity-target.c | 26 ++++++++++++++++++++++--
>  drivers/md/dm-verity.h        |  1 +
>  3 files changed, 61 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
> index aa6bb5b4704b..a60d91d02e28 100644
> --- a/drivers/md/dm-crypt.c
> +++ b/drivers/md/dm-crypt.c
> @@ -75,8 +75,10 @@ struct dm_crypt_io {
>  	struct bio *base_bio;
>  	u8 *integrity_metadata;
>  	bool integrity_metadata_from_pool:1;
> +	bool in_tasklet:1;
>  
>  	struct work_struct work;
> +	struct tasklet_struct tasklet;
>  
>  	struct convert_context ctx;
>  
> @@ -1775,6 +1777,7 @@ static void crypt_io_init(struct dm_crypt_io *io, struct crypt_config *cc,
>  	io->ctx.r.req = NULL;
>  	io->integrity_metadata = NULL;
>  	io->integrity_metadata_from_pool = false;
> +	io->in_tasklet = false;
>  	atomic_set(&io->io_pending, 0);
>  }
>  
> @@ -1785,6 +1788,13 @@ static void crypt_inc_pending(struct dm_crypt_io *io)
>  
>  static void kcryptd_queue_read(struct dm_crypt_io *io);
>  
> +static void kcryptd_io_bio_endio(struct work_struct *work)
> +{
> +	struct dm_crypt_io *io = container_of(work, struct dm_crypt_io, work);
> +
> +	bio_endio(io->base_bio);
> +}
> +
>  /*
>   * One of the bios was finished. Check for completion of
>   * the whole request and correctly clean up the buffer.
> @@ -1817,6 +1827,20 @@ static void crypt_dec_pending(struct dm_crypt_io *io)
>  
>  	base_bio->bi_status = error;
>  
> +	/*
> +	 * If we are running this function from our tasklet,
> +	 * we can't call bio_endio() here, because it will call
> +	 * clone_endio() from dm.c, which in turn will
> +	 * free the current struct dm_crypt_io structure with
> +	 * our tasklet. In this case we need to delay bio_endio()
> +	 * execution to after the tasklet is done and dequeued.
> +	 */
> +	if (io->in_tasklet) {
> +		INIT_WORK(&io->work, kcryptd_io_bio_endio);
> +		queue_work(cc->io_queue, &io->work);
> +		return;
> +	}
> +
>  	bio_endio(base_bio);
>  }
>  
> @@ -2291,6 +2315,11 @@ static void kcryptd_crypt(struct work_struct *work)
>  		kcryptd_crypt_write_convert(io);
>  }
>  
> +static void kcryptd_crypt_tasklet(unsigned long work)
> +{
> +	kcryptd_crypt((struct work_struct *)work);
> +}
> +
>  static void kcryptd_queue_crypt(struct dm_crypt_io *io)
>  {
>  	struct crypt_config *cc = io->cc;
> @@ -2302,10 +2331,15 @@ static void kcryptd_queue_crypt(struct dm_crypt_io *io)
>  		 * irqs_disabled(): the kernel may run some IO completion from the idle thread, but
>  		 * it is being executed with irqs disabled.
>  		 */
> -		if (!(in_hardirq() || irqs_disabled())) {
> -			kcryptd_crypt(&io->work);
> +		if (in_hardirq() || irqs_disabled()) {
> +			io->in_tasklet = true;
> +			tasklet_init(&io->tasklet, kcryptd_crypt_tasklet, (unsigned long)&io->work);
> +			tasklet_schedule(&io->tasklet);
>  			return;
>  		}
> +
> +		kcryptd_crypt(&io->work);
> +		return;
>  	}
>  
>  	INIT_WORK(&io->work, kcryptd_crypt);
> diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
> index 49e4a35d7019..0bb126eadc0d 100644
> --- a/drivers/md/dm-verity-target.c
> +++ b/drivers/md/dm-verity-target.c
> @@ -701,6 +701,23 @@ static void verity_work(struct work_struct *w)
>  	verity_finish_io(io, errno_to_blk_status(verity_verify_io(io)));
>  }
>  
> +static void verity_tasklet(unsigned long data)
> +{
> +	struct dm_verity_io *io = (struct dm_verity_io *)data;
> +	int err;
> +
> +	io->in_tasklet = true;
> +	err = verity_verify_io(io);
> +	if (err == -EAGAIN || err == -ENOMEM) {
> +		/* fallback to retrying with work-queue */
> +		INIT_WORK(&io->work, verity_work);
> +		queue_work(io->v->verify_wq, &io->work);
> +		return;
> +	}
> +
> +	verity_finish_io(io, errno_to_blk_status(err));
> +}
> +
>  static void verity_end_io(struct bio *bio)
>  {
>  	struct dm_verity_io *io = bio->bi_private;
> @@ -713,8 +730,13 @@ static void verity_end_io(struct bio *bio)
>  		return;
>  	}
>  
> -	INIT_WORK(&io->work, verity_work);
> -	queue_work(io->v->verify_wq, &io->work);
> +	if (static_branch_unlikely(&use_tasklet_enabled) && io->v->use_tasklet) {
> +		tasklet_init(&io->tasklet, verity_tasklet, (unsigned long)io);
> +		tasklet_schedule(&io->tasklet);
> +	} else {
> +		INIT_WORK(&io->work, verity_work);
> +		queue_work(io->v->verify_wq, &io->work);
> +	}
>  }
>  
>  /*
> diff --git a/drivers/md/dm-verity.h b/drivers/md/dm-verity.h
> index db93a91169d5..7e495cc375b0 100644
> --- a/drivers/md/dm-verity.h
> +++ b/drivers/md/dm-verity.h
> @@ -87,6 +87,7 @@ struct dm_verity_io {
>  	bool in_tasklet;
>  
>  	struct work_struct work;
> +	struct tasklet_struct tasklet;
>  
>  	char *recheck_buffer;
>  
> -- 
> 2.31.1
> 

Re: [PATCH 6.6] Revert "dm-crypt, dm-verity: disable tasklets"

[Li Lingfeng]

Hi

I'm having difficulty understanding "Workqueues and ksoftirqd may be 
scheduled arbitrarily".
This is my understanding:
kcryptd_queue_crypt
  tasklet_schedule
   __tasklet_schedule
    __tasklet_schedule_common
     raise_softirq_irqoff
      wakeup_softirqd
       wake_up_process // ksoftirqd

run_ksoftirqd
  __do_softirq
   softirq_handle_begin
    __local_bh_disable_ip // Turn off preemption
<---------- [1] ---------->
   tasklet_action // h->action
    tasklet_action_common
     tasklet_trylock
      kcryptd_crypt_tasklet // t->func(t->data)
      ...
       queue_work(cc->io_queue, &io->work)
<---------- [2] ---------->
     tasklet_unlock

// workqueue process
kcryptd_io_bio_endio
  ...
  // free tasklet_struct

Since preemption has been turned off at [1], I'm confused about how the 
CPU can be scheduled out to do work first at [2].
Would you mind explaining it to me?

Thanks

在 2024/4/11 20:37, Mikulas Patocka 写道:
> Hi
>
> I NACK this.
>
>
> On Thu, 11 Apr 2024, Li Lingfeng wrote:
>
>> This reverts commit 5735a2671ffb70ea29ca83969fe01316ee2ed6fc which is
>> commit 0a9bab391e336489169b95cb0d4553d921302189 upstream.
>>
>> Tasklet is thought to cause memory corruption [1], so it was disabled in
>> dm-crypt and dm-verity. However, memory corruption may not happen since
>> cc->io_queue is created without WQ_UNBOUND [2].
>> Revert commit 5735a2671ffb ("dm-crypt, dm-verity: disable tasklets") to
>> bring tasklet back.
>>
>> [1] https://lore.kernel.org/all/d390d7ee-f142-44d3-822a-87949e14608b@suse.de/T/
>> [2] https://lore.kernel.org/all/4d331659-badd-749d-fba1-271543631a8a@huawei.com/
> Regarding [2] - if you add mdelay, you can make the race condition less
> reproducible, but it is not a proper fix and the race condition stays
> there unfixed.
>
> Workqueues and ksoftirqd may be scheduled arbitrarily, there is no
> guarantee that they will be executed in a particular order, even if they
> are executed on the same CPU.
>
> Mikulas
>
>> Signed-off-by: Li Lingfeng <lilingfeng3@huawei.com>
>> ---
>>   drivers/md/dm-crypt.c         | 38 +++++++++++++++++++++++++++++++++--
>>   drivers/md/dm-verity-target.c | 26 ++++++++++++++++++++++--
>>   drivers/md/dm-verity.h        |  1 +
>>   3 files changed, 61 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
>> index aa6bb5b4704b..a60d91d02e28 100644
>> --- a/drivers/md/dm-crypt.c
>> +++ b/drivers/md/dm-crypt.c
>> @@ -75,8 +75,10 @@ struct dm_crypt_io {
>>   	struct bio *base_bio;
>>   	u8 *integrity_metadata;
>>   	bool integrity_metadata_from_pool:1;
>> +	bool in_tasklet:1;
>>   
>>   	struct work_struct work;
>> +	struct tasklet_struct tasklet;
>>   
>>   	struct convert_context ctx;
>>   
>> @@ -1775,6 +1777,7 @@ static void crypt_io_init(struct dm_crypt_io *io, struct crypt_config *cc,
>>   	io->ctx.r.req = NULL;
>>   	io->integrity_metadata = NULL;
>>   	io->integrity_metadata_from_pool = false;
>> +	io->in_tasklet = false;
>>   	atomic_set(&io->io_pending, 0);
>>   }
>>   
>> @@ -1785,6 +1788,13 @@ static void crypt_inc_pending(struct dm_crypt_io *io)
>>   
>>   static void kcryptd_queue_read(struct dm_crypt_io *io);
>>   
>> +static void kcryptd_io_bio_endio(struct work_struct *work)
>> +{
>> +	struct dm_crypt_io *io = container_of(work, struct dm_crypt_io, work);
>> +
>> +	bio_endio(io->base_bio);
>> +}
>> +
>>   /*
>>    * One of the bios was finished. Check for completion of
>>    * the whole request and correctly clean up the buffer.
>> @@ -1817,6 +1827,20 @@ static void crypt_dec_pending(struct dm_crypt_io *io)
>>   
>>   	base_bio->bi_status = error;
>>   
>> +	/*
>> +	 * If we are running this function from our tasklet,
>> +	 * we can't call bio_endio() here, because it will call
>> +	 * clone_endio() from dm.c, which in turn will
>> +	 * free the current struct dm_crypt_io structure with
>> +	 * our tasklet. In this case we need to delay bio_endio()
>> +	 * execution to after the tasklet is done and dequeued.
>> +	 */
>> +	if (io->in_tasklet) {
>> +		INIT_WORK(&io->work, kcryptd_io_bio_endio);
>> +		queue_work(cc->io_queue, &io->work);
>> +		return;
>> +	}
>> +
>>   	bio_endio(base_bio);
>>   }
>>   
>> @@ -2291,6 +2315,11 @@ static void kcryptd_crypt(struct work_struct *work)
>>   		kcryptd_crypt_write_convert(io);
>>   }
>>   
>> +static void kcryptd_crypt_tasklet(unsigned long work)
>> +{
>> +	kcryptd_crypt((struct work_struct *)work);
>> +}
>> +
>>   static void kcryptd_queue_crypt(struct dm_crypt_io *io)
>>   {
>>   	struct crypt_config *cc = io->cc;
>> @@ -2302,10 +2331,15 @@ static void kcryptd_queue_crypt(struct dm_crypt_io *io)
>>   		 * irqs_disabled(): the kernel may run some IO completion from the idle thread, but
>>   		 * it is being executed with irqs disabled.
>>   		 */
>> -		if (!(in_hardirq() || irqs_disabled())) {
>> -			kcryptd_crypt(&io->work);
>> +		if (in_hardirq() || irqs_disabled()) {
>> +			io->in_tasklet = true;
>> +			tasklet_init(&io->tasklet, kcryptd_crypt_tasklet, (unsigned long)&io->work);
>> +			tasklet_schedule(&io->tasklet);
>>   			return;
>>   		}
>> +
>> +		kcryptd_crypt(&io->work);
>> +		return;
>>   	}
>>   
>>   	INIT_WORK(&io->work, kcryptd_crypt);
>> diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
>> index 49e4a35d7019..0bb126eadc0d 100644
>> --- a/drivers/md/dm-verity-target.c
>> +++ b/drivers/md/dm-verity-target.c
>> @@ -701,6 +701,23 @@ static void verity_work(struct work_struct *w)
>>   	verity_finish_io(io, errno_to_blk_status(verity_verify_io(io)));
>>   }
>>   
>> +static void verity_tasklet(unsigned long data)
>> +{
>> +	struct dm_verity_io *io = (struct dm_verity_io *)data;
>> +	int err;
>> +
>> +	io->in_tasklet = true;
>> +	err = verity_verify_io(io);
>> +	if (err == -EAGAIN || err == -ENOMEM) {
>> +		/* fallback to retrying with work-queue */
>> +		INIT_WORK(&io->work, verity_work);
>> +		queue_work(io->v->verify_wq, &io->work);
>> +		return;
>> +	}
>> +
>> +	verity_finish_io(io, errno_to_blk_status(err));
>> +}
>> +
>>   static void verity_end_io(struct bio *bio)
>>   {
>>   	struct dm_verity_io *io = bio->bi_private;
>> @@ -713,8 +730,13 @@ static void verity_end_io(struct bio *bio)
>>   		return;
>>   	}
>>   
>> -	INIT_WORK(&io->work, verity_work);
>> -	queue_work(io->v->verify_wq, &io->work);
>> +	if (static_branch_unlikely(&use_tasklet_enabled) && io->v->use_tasklet) {
>> +		tasklet_init(&io->tasklet, verity_tasklet, (unsigned long)io);
>> +		tasklet_schedule(&io->tasklet);
>> +	} else {
>> +		INIT_WORK(&io->work, verity_work);
>> +		queue_work(io->v->verify_wq, &io->work);
>> +	}
>>   }
>>   
>>   /*
>> diff --git a/drivers/md/dm-verity.h b/drivers/md/dm-verity.h
>> index db93a91169d5..7e495cc375b0 100644
>> --- a/drivers/md/dm-verity.h
>> +++ b/drivers/md/dm-verity.h
>> @@ -87,6 +87,7 @@ struct dm_verity_io {
>>   	bool in_tasklet;
>>   
>>   	struct work_struct work;
>> +	struct tasklet_struct tasklet;
>>   
>>   	char *recheck_buffer;
>>   
>> -- 
>> 2.31.1
>>

Re: [PATCH 6.6] Revert "dm-crypt, dm-verity: disable tasklets"

[Mikulas Patocka]

[-- Attachment #1: Type: text/plain, Size: 1228 bytes --]



On Fri, 12 Apr 2024, Li Lingfeng wrote:

> Hi
> 
> I'm having difficulty understanding "Workqueues and ksoftirqd may be scheduled
> arbitrarily".
> This is my understanding:
> kcryptd_queue_crypt
>  tasklet_schedule
>   __tasklet_schedule
>    __tasklet_schedule_common
>     raise_softirq_irqoff
>      wakeup_softirqd
>       wake_up_process // ksoftirqd
> 
> run_ksoftirqd
>  __do_softirq
>   softirq_handle_begin
>    __local_bh_disable_ip // Turn off preemption
> <---------- [1] ---------->
>   tasklet_action // h->action
>    tasklet_action_common
>     tasklet_trylock
>      kcryptd_crypt_tasklet // t->func(t->data)
>      ...
>       queue_work(cc->io_queue, &io->work)
> <---------- [2] ---------->
>     tasklet_unlock
> 
> // workqueue process
> kcryptd_io_bio_endio
>  ...
>  // free tasklet_struct
> 
> Since preemption has been turned off at [1], I'm confused about how the CPU
> can be scheduled out to do work first at [2].
> Would you mind explaining it to me?
> 
> Thanks

Yes, you are right that scheduling is disabled when ksoftirqd processes a 
softirq task.

But the upstream kernel switched to bh workqueues anyway, so there is no 
need to submit a different solution to the stable kernels.

Mikulas