Received-SPF: pass (google.com: domain of linux-kernel+bounces-47151-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Wed, 31 Jan 2024 22:19:07 +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, bob.liu@oracle.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 8/8] dm-verity: Convert from tasklet to BH workqueue
In-Reply-To: <20240130091300.2968534-9-tj@kernel.org>
Message-ID: <c2539f87-b4fe-ac7d-64d9-cbf8db929c7@redhat.com>
References: <20240130091300.2968534-1-tj@kernel.org> <20240130091300.2968534-9-tj@kernel.org>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII


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-verity from tasklet to BH workqueue.
> 
> This is a minimal conversion which doesn't rename the related names
> including the "try_verify_in_tasklet" option. If this patch is applied, a
> follow-up patch would be necessary. I couldn't decide whether the option
> name would need to be updated too.
> 
> Only compile tested. I don't know how to verity.

Download the cryptsetup package with "git clone 
https://gitlab.com/cryptsetup/cryptsetup" and run the testsuite: 
/autogen.sh && ./configure && make && make check

> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Alasdair Kergon <agk@redhat.com>
> Cc: Mike Snitzer <snitzer@kernel.org>
> Cc: Mikulas Patocka <mpatocka@redhat.com>
> Cc: dm-devel@lists.linux.dev
> ---
>  drivers/md/dm-verity-target.c | 8 ++++----
>  drivers/md/dm-verity.h        | 2 +-
>  2 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/md/dm-verity-target.c b/drivers/md/dm-verity-target.c
> index 14e58ae70521..911261de2d08 100644
> --- a/drivers/md/dm-verity-target.c
> +++ b/drivers/md/dm-verity-target.c
> @@ -645,9 +645,9 @@ 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)
> +static void verity_bh_work(struct work_struct *w)
>  {
> -	struct dm_verity_io *io = (struct dm_verity_io *)data;
> +	struct dm_verity_io *io = container_of(w, struct dm_verity_io, bh_work);
>  	int err;
>  
>  	io->in_tasklet = true;
> @@ -675,8 +675,8 @@ static void verity_end_io(struct bio *bio)
>  	}
>  
>  	if (static_branch_unlikely(&use_tasklet_enabled) && io->v->use_tasklet) {
> -		tasklet_init(&io->tasklet, verity_tasklet, (unsigned long)io);
> -		tasklet_schedule(&io->tasklet);
> +		INIT_WORK(&io->bh_work, verity_bh_work);
> +		queue_work(system_bh_wq, &io->bh_work);
>  	} 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 f9d522c870e6..7c16f834f31a 100644
> --- a/drivers/md/dm-verity.h
> +++ b/drivers/md/dm-verity.h
> @@ -83,7 +83,7 @@ struct dm_verity_io {
>  	struct bvec_iter iter;
>  
>  	struct work_struct work;
> -	struct tasklet_struct tasklet;
> +	struct work_struct bh_work;
>  
>  	/*
>  	 * Three variably-size fields follow this struct:

Do we really need two separate work_structs here? They are never submitted 
concurrently, so I think that one would be enough. Or, am I missing 
something?

Mikulas