Received-SPF: pass (google.com: domain of linux-kernel+bounces-47155-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Wed, 31 Jan 2024 22:23:14 +0100 (CET)
From: Mikulas Patocka <mpatocka@redhat.com>
To: Tejun Heo <tj@kernel.org>
cc: torvalds@linux-foundation.org, linux-kernel@vger.kernel.org, 
    dm-devel@lists.linux.dev, msnitzer@redhat.com, ignat@cloudflare.com, 
    damien.lemoal@wdc.com, houtao1@huawei.com, peterz@infradead.org, 
    mingo@kernel.org, netdev@vger.kernel.org, allen.lkml@gmail.com, 
    kernel-team@meta.com, Alasdair Kergon <agk@redhat.com>, 
    Mike Snitzer <snitzer@kernel.org>
Subject: Re: [PATCH 7/8] dm-crypt: Convert from tasklet to BH workqueue
In-Reply-To: <20240130091300.2968534-8-tj@kernel.org>
Message-ID: <ddf2e764-a6e5-1024-4ae9-ef42f7a86cbe@redhat.com>
References: <20240130091300.2968534-1-tj@kernel.org> <20240130091300.2968534-8-tj@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII

This seems OK.

Reviewed-by: Mikulas Patocka <mpatocka@redhat.com>


On Mon, 29 Jan 2024, Tejun Heo wrote:

> The only generic interface to execute asynchronously in the BH context is
> tasklet; however, it's marked deprecated and has some design flaws. To
> replace tasklets, BH workqueue support was recently added. A BH workqueue
> behaves similarly to regular workqueues except that the queued work items
> are executed in the BH context.
> 
> This patch converts dm-crypt from tasklet to BH workqueue.
> 
> Like a regular workqueue, a BH workqueue allows freeing the currently
> executing work item. Converting from tasklet to BH workqueue removes the
> need for deferring bio_endio() again to a work item, which was buggy anyway.
> 
> I tested this lightly with "--perf-no_read_workqueue
> --perf-no_write_workqueue" + some code modifications, but would really
> -appreciate if someone who knows the code base better could take a look.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Link: http://lkml.kernel.org/r/82b964f0-c2c8-a2c6-5b1f-f3145dc2c8e5@redhat.com
> Cc: Mikulas Patocka <mpatocka@redhat.com>
> Cc: Linus Torvalds <torvalds@linux-foundation.org>
> Cc: Alasdair Kergon <agk@redhat.com>
> Cc: Mike Snitzer <snitzer@kernel.org>
> Cc: dm-devel@lists.linux.dev
> ---
>  drivers/md/dm-crypt.c | 36 ++----------------------------------
>  1 file changed, 2 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c
> index 855b482cbff1..619c762d4072 100644
> --- a/drivers/md/dm-crypt.c
> +++ b/drivers/md/dm-crypt.c
> @@ -73,11 +73,8 @@ 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;
>  
>  	atomic_t io_pending;
> @@ -1762,7 +1759,6 @@ 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);
>  }
>  
> @@ -1771,13 +1767,6 @@ static void crypt_inc_pending(struct dm_crypt_io *io)
>  	atomic_inc(&io->io_pending);
>  }
>  
> -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.
> @@ -1800,21 +1789,6 @@ static void crypt_dec_pending(struct dm_crypt_io *io)
>  		kfree(io->integrity_metadata);
>  
>  	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);
>  }
>  
> @@ -2246,11 +2220,6 @@ 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;
> @@ -2263,9 +2232,8 @@ static void kcryptd_queue_crypt(struct dm_crypt_io *io)
>  		 * it is being executed with irqs disabled.
>  		 */
>  		if (in_hardirq() || irqs_disabled()) {
> -			io->in_tasklet = true;
> -			tasklet_init(&io->tasklet, kcryptd_crypt_tasklet, (unsigned long)&io->work);
> -			tasklet_schedule(&io->tasklet);
> +			INIT_WORK(&io->work, kcryptd_crypt);
> +			queue_work(system_bh_wq, &io->work);
>  			return;
>  		}
>  
> -- 
> 2.43.0
>