On Fri, Jun 10 2022 at 12:26P -0400, Mike Snitzer <[email protected]> wrote:
> On Thu, Jun 09 2022 at 7:43P -0400, > Shin'ichiro Kawasaki <[email protected]> wrote: > > > After the commit ca522482e3ea ("dm: pass NULL bdev to bio_alloc_clone"), > > bdev is no longer set to clone bio for ->map function. Instead, each DM > > targets shall set bdev to the clone bio by calling bio_set_dev() before > > issuing IO. Also the commit ensured that dm_zone_endio() is called from > > clone_endio() only when DM targets set bdev to the clone bio. > > > > However, crypt_map() of dm-crypt does not call bio_set_dev() for every > > clone bio. Then dm_zone_endio() is not called at completion of the bios > > and zone locks are not properly unlocked. This triggers a hang when > > blktests block/004 is run for dm-crypt on zoned block devices [1]. To > > avoid the hang, call bio_set_dev() for every bio in crypt_map(). > > > > [1] > > > > [ 6596.702977][T55017] run blktests block/004 at 2022-06-07 20:18:01 > > <snip> > > Please refrain from putting stack traces in patch headers whenever > possible. Really no need for this, especially given how long this one > is! > > I revised the header as follows: > https://git.kernel.org/pub/scm/linux/kernel/git/device-mapper/linux-dm.git/commit/?h=dm-5.19&id=cae0053631cd4b02bb882b53c7da20652b038049 > > > Fixes: ca522482e3ea ("dm: pass NULL bdev to bio_alloc_clone") > > Signed-off-by: Shin'ichiro Kawasaki <[email protected]> > > --- > > drivers/md/dm-crypt.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/md/dm-crypt.c b/drivers/md/dm-crypt.c > > index 159c6806c19b..c68523a89428 100644 > > --- a/drivers/md/dm-crypt.c > > +++ b/drivers/md/dm-crypt.c > > @@ -3378,6 +3378,8 @@ static int crypt_map(struct dm_target *ti, struct bio > > *bio) > > struct dm_crypt_io *io; > > struct crypt_config *cc = ti->private; > > > > + bio_set_dev(bio, cc->dev->bdev); > > + > > /* > > * If bio is REQ_PREFLUSH or REQ_OP_DISCARD, just bypass crypt queues. > > * - for REQ_PREFLUSH device-mapper core ensures that no IO is in-flight > > @@ -3385,7 +3387,6 @@ static int crypt_map(struct dm_target *ti, struct bio > > *bio) > > */ > > if (unlikely(bio->bi_opf & REQ_PREFLUSH || > > bio_op(bio) == REQ_OP_DISCARD)) { > > - bio_set_dev(bio, cc->dev->bdev); > > if (bio_sectors(bio)) > > bio->bi_iter.bi_sector = cc->start + > > dm_target_offset(ti, bio->bi_iter.bi_sector); > > -- > > 2.36.1 > > > > BUT something isn't quite adding up with why you need this change > given commit ca522482e3ea has this compatibility code: > > diff --git a/drivers/md/dm.c b/drivers/md/dm.c > index 9650ba2075b8..d62f1354ecbf 100644 > --- a/drivers/md/dm.c > +++ b/drivers/md/dm.c > @@ -581,7 +581,9 @@ static struct dm_io *alloc_io(struct mapped_device *md, > struct bio *bio) > struct dm_target_io *tio; > struct bio *clone; > > - clone = bio_alloc_clone(bio->bi_bdev, bio, GFP_NOIO, &md->io_bs); > + clone = bio_alloc_clone(NULL, bio, GFP_NOIO, &md->io_bs); > + /* Set default bdev, but target must bio_set_dev() before issuing IO > */ > + clone->bi_bdev = md->disk->part0; > > tio = clone_to_tio(clone); > tio->flags = 0; > > The clone bio passed to crypt_map() _should_ be the same as was passed > before commit ca522482e3ea (It gets set to md->disk->part0 rather than > bio->bi_bdev but they really should point to the same top-level DM > bdev). > > So why is your extra override to have dm-crypt point to its underlying > data device important now? Think the issue is not that the clone->bi_bdev isn't set. It is that it is merely not changed from the md->disk->part0 default. Commit ca522482e3ea added this to clone_endio: if (likely(bio->bi_bdev != md->disk->part0)) { ... if (static_branch_unlikely(&zoned_enabled) && unlikely(blk_queue_is_zoned(q))) dm_zone_endio(io, bio); } So dm_zone_endio() isn't called unless you force dm-crypt (or any other target) to remap the clone bio to a different bdev. It is weird that a bio is completing without having been remapped by dm-crypt; but there could be any number of reasons why that hasn't happened. Anyway, I think a correct fix needs to be to the logic in clone_endio(). Could be that its best to just remove the gating likely() conditional. Can you please test this patch instead of your patch? Thanks, Mike diff --git a/drivers/md/dm.c b/drivers/md/dm.c index 8b21155d3c4f..d8f16183bf27 100644 --- a/drivers/md/dm.c +++ b/drivers/md/dm.c @@ -1016,23 +1016,19 @@ static void clone_endio(struct bio *bio) struct dm_io *io = tio->io; struct mapped_device *md = io->md; - if (likely(bio->bi_bdev != md->disk->part0)) { - struct request_queue *q = bdev_get_queue(bio->bi_bdev); - - if (unlikely(error == BLK_STS_TARGET)) { - if (bio_op(bio) == REQ_OP_DISCARD && - !bdev_max_discard_sectors(bio->bi_bdev)) - disable_discard(md); - else if (bio_op(bio) == REQ_OP_WRITE_ZEROES && - !q->limits.max_write_zeroes_sectors) - disable_write_zeroes(md); - } - - if (static_branch_unlikely(&zoned_enabled) && - unlikely(blk_queue_is_zoned(q))) - dm_zone_endio(io, bio); + if (unlikely(error == BLK_STS_TARGET)) { + if (bio_op(bio) == REQ_OP_DISCARD && + !bdev_max_discard_sectors(bio->bi_bdev)) + disable_discard(md); + else if (bio_op(bio) == REQ_OP_WRITE_ZEROES && + !bdev_write_zeroes_sectors(bio->bi_bdev)) + disable_write_zeroes(md); } + if (static_branch_unlikely(&zoned_enabled) && + unlikely(blk_queue_is_zoned(bdev_get_queue(bio->bi_bdev)))) + dm_zone_endio(io, bio); + if (endio) { int r = endio(ti, bio, &error); switch (r) { -- dm-devel mailing list [email protected] https://listman.redhat.com/mailman/listinfo/dm-devel
