This seems OK. Reviewed-by: Mikulas Patocka <mpato...@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 <t...@kernel.org> > Link: http://lkml.kernel.org/r/82b964f0-c2c8-a2c6-5b1f-f3145dc2c...@redhat.com > Cc: Mikulas Patocka <mpato...@redhat.com> > Cc: Linus Torvalds <torva...@linux-foundation.org> > Cc: Alasdair Kergon <a...@redhat.com> > Cc: Mike Snitzer <snit...@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 >
