Re: dm crypt: set bdev to clone bio

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Jun 10 2022 at 12:26P -0400,
Mike Snitzer <snitzer@xxxxxxxxxx> wrote:

> On Thu, Jun 09 2022 at  7:43P -0400,
> Shin'ichiro Kawasaki <shinichiro.kawasaki@xxxxxxx> 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 <shinichiro.kawasaki@xxxxxxx>
> > ---
> >  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
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel




[Index of Archives]     [DM Crypt]     [Fedora Desktop]     [ATA RAID]     [Fedora Marketing]     [Fedora Packaging]     [Fedora SELinux]     [Yosemite Discussion]     [KDE Users]     [Fedora Docs]

  Powered by Linux