Received-SPF: pass (google.com: domain of linux-kernel+bounces-47173-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Sender: Tejun Heo <htejun@gmail.com>
Date: Wed, 31 Jan 2024 11:32:32 -1000
From: Tejun Heo <tj@kernel.org>
To: Mikulas Patocka <mpatocka@redhat.com>
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
Message-ID: <Zbq8cE3Y2ZL6dl8r@slm.duckdns.org>
References: <20240130091300.2968534-1-tj@kernel.org>
 <20240130091300.2968534-9-tj@kernel.org>
 <c2539f87-b4fe-ac7d-64d9-cbf8db929c7@redhat.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <c2539f87-b4fe-ac7d-64d9-cbf8db929c7@redhat.com>

Hello,

On Wed, Jan 31, 2024 at 10:19:07PM +0100, Mikulas Patocka wrote:
> > @@ -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?

I don't know, so just did the dumb thing. If the caller always guarantees
that the work items are never queued at the same time, reusing is fine.
However, the followings might be useful to keep on mind:

- work_struct is pretty small - 4 pointers.

- INIT_WORK() on a queued work item isn't gonna be pretty.

- Flushing and no-concurrent-execution guarantee are broken on INIT_WORK().
  e.g. If you queue_work(), INIT_WORK(), flush_work(), the flush isn't
  actually going to wait for the work item to finish. Also, if you do
  queue_work(), INIT_WORK(), queue_work(), the two queued work item
  instances may end up running concurrently.

Muxing a single work item carries more risks of subtle bugs, but in some
cases, the way it's used is clear (e.g. sequential chaining) and that's
fine.

Thanks.

-- 
tejun