Hi, On 3/7/2023 10:47 PM, Mike Snitzer wrote: > On Mon, Mar 06 2023 at 9:12P -0500, > Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote: > >> Hi, >> >> On 3/7/2023 3:31 AM, Mike Snitzer wrote: >>> On Mon, Mar 06 2023 at 8:49P -0500, >>> Hou Tao <houtao@xxxxxxxxxxxxxxx> wrote: >>> >>>> From: Hou Tao <houtao1@xxxxxxxxxx> >>>> >>>> When neither no_read_workqueue nor no_write_workqueue are enabled, >>>> tasklet_trylock() in crypt_dec_pending() may still return false due to >>>> an uninitialized state, and dm-crypt will do io completion in io_queue >>>> instead of current context unnecessarily. >>> Have you actually experienced this? >> Yes. I had written a bpftrace script to check the completion context of >> blkdev_bio_end_io_simple() when doing direct io read on dm-crypt device. The >> expected context should be unbound workers of crypt_queue, but sometimes the >> context is the bound worker of io_queue. > OK, thanks for clarifying. Curious to know the circumstance (I > thought per-bio-data is zero'd -- but it may be I'm mistaken). The circumstance is just a normal qemu VM running the vanilla kernel for test purpose. According to the implementation of bio_alloc_bioset(), the front pad of bio is not initialized and only bio itself is initialized. AFAIK if CONFIG_INIT_ON_ALLOC_DEFAULT_ON is enabled, per-bio-data may be zeroed. > > I won't be marking this commit for stable@ but if others feel > differently please let me know and I'll do so. (We can always propose > it to stable@, after the fact, even if the commit header doesn't Cc > stable@) > >>>> Fix it by initializing io->tasklet in crypt_io_init(). >>> Really would rather avoid always calling tasklet_init(). But I can >>> optimize it away with a later patch. >> My first though was "io->tasklet.state = 0", but it may be fragile because it >> operated on the internal status of tasklet, so I switch to tasklet_init(). > Yes, I looked into it and came up with the same hack.. and I too felt > it was too fragile due to open-coding direct access to the tasklet's > members. > > I have a patch I just staged that staged that uses jump_labels to > optimize this code. If you might review/test/verify it works well for > you that'd be appreciated: > https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-6.3&id=ae75a25bd83f7c541240449d2fff3a44433e506b > > It builds on your patch, which I added a comment to: > https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-6.3&id=d9fe0a98a2e0a1cf585e8a6555afb33be968bd13 Thanks for the comments. It is fine to me. > > From: Mike Snitzer <snitzer@xxxxxxxxxx> > Date: Mon, 6 Mar 2023 15:58:33 -0500 > Subject: [PATCH] dm crypt: conditionally enable code needed for tasklet usecases > > Use jump_label to limit the need for branching, and tasklet_init(), > unless either of the optional "no_read_workqueue" and/or > "no_write_workqueue" features are used. > > Signed-off-by: Mike Snitzer <snitzer@xxxxxxxxxx> > --- > drivers/md/dm-crypt.c | 35 +++++++++++++++++++++++++++-------- > 1 file changed, 27 insertions(+), 8 deletions(-) > > diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c > index 641457e72603..2d0309ca07f5 100644 > --- a/drivers/md/dm-crypt.c > +++ b/drivers/md/dm-crypt.c > @@ -40,6 +40,7 @@ > #include <keys/user-type.h> > #include <keys/encrypted-type.h> > #include <keys/trusted-type.h> > +#include <linux/jump_label.h> > > #include <linux/device-mapper.h> > > @@ -85,6 +86,8 @@ struct dm_crypt_io { > struct rb_node rb_node; > } CRYPTO_MINALIGN_ATTR; > > +static DEFINE_STATIC_KEY_FALSE(use_tasklet_enabled); > + > struct dm_crypt_request { > struct convert_context *ctx; > struct scatterlist sg_in[4]; > @@ -1730,12 +1733,15 @@ static void crypt_io_init(struct dm_crypt_io *io, struct crypt_config *cc, > io->sector = sector; > io->error = 0; > io->ctx.r.req = NULL; > - /* > - * tasklet_init() here to ensure crypt_dec_pending()'s > - * tasklet_trylock() doesn't incorrectly return false > - * even when tasklet isn't in use. > - */ > - tasklet_init(&io->tasklet, kcryptd_crypt_tasklet, (unsigned long)&io->work); > + if (static_branch_unlikely(&use_tasklet_enabled)) { > + /* > + * tasklet_init() here to ensure crypt_dec_pending()'s > + * tasklet_trylock() doesn't incorrectly return false > + * even when tasklet isn't in use. > + */ > + tasklet_init(&io->tasklet, kcryptd_crypt_tasklet, > + (unsigned long)&io->work); > + } > io->integrity_metadata = NULL; > io->integrity_metadata_from_pool = false; > atomic_set(&io->io_pending, 0); > @@ -1775,6 +1781,10 @@ static void crypt_dec_pending(struct dm_crypt_io *io) > kfree(io->integrity_metadata); > > base_bio->bi_status = error; > + if (!static_branch_unlikely(&use_tasklet_enabled)) { > + bio_endio(base_bio); > + return; > + } Because use_tasklet_enabled can be enabled concurrently, so I think it is still possible that crypt_dec_pending will try-lock an unitialized tasklet if use_tasklet_enabled is enabled when invoking crypt_dec_pending(). > > /* > * If we are running this function from our tasklet, > @@ -2232,8 +2242,9 @@ static void kcryptd_queue_crypt(struct dm_crypt_io *io) > { > struct crypt_config *cc = io->cc; > > - if ((bio_data_dir(io->base_bio) == READ && test_bit(DM_CRYPT_NO_READ_WORKQUEUE, &cc->flags)) || > - (bio_data_dir(io->base_bio) == WRITE && test_bit(DM_CRYPT_NO_WRITE_WORKQUEUE, &cc->flags))) { > + if (static_branch_unlikely(&use_tasklet_enabled) && > + ((bio_data_dir(io->base_bio) == READ && test_bit(DM_CRYPT_NO_READ_WORKQUEUE, &cc->flags)) || > + (bio_data_dir(io->base_bio) == WRITE && test_bit(DM_CRYPT_NO_WRITE_WORKQUEUE, &cc->flags)))) { > /* > * in_hardirq(): Crypto API's skcipher_walk_first() refuses to work in hard IRQ context. > * irqs_disabled(): the kernel may run some IO completion from the idle thread, but > @@ -2746,6 +2757,10 @@ static void crypt_dtr(struct dm_target *ti) > crypt_calculate_pages_per_client(); > spin_unlock(&dm_crypt_clients_lock); > > + if (test_bit(DM_CRYPT_NO_READ_WORKQUEUE, &cc->flags) || > + test_bit(DM_CRYPT_NO_WRITE_WORKQUEUE, &cc->flags)) > + static_branch_dec(&use_tasklet_enabled); > + > dm_audit_log_dtr(DM_MSG_PREFIX, ti, 1); > } > > @@ -3375,6 +3390,10 @@ static int crypt_ctr(struct dm_target *ti, unsigned int argc, char **argv) > ti->limit_swap_bios = true; > ti->accounts_remapped_io = true; > > + if (test_bit(DM_CRYPT_NO_READ_WORKQUEUE, &cc->flags) || > + test_bit(DM_CRYPT_NO_WRITE_WORKQUEUE, &cc->flags)) > + static_branch_inc(&use_tasklet_enabled); > + > dm_audit_log_ctr(DM_MSG_PREFIX, ti, 1); > return 0; > -- dm-devel mailing list dm-devel@xxxxxxxxxx https://listman.redhat.com/mailman/listinfo/dm-devel
