Message-ID: <20210316152500.GA55711@infradead.org>
Date:   Tue, 16 Mar 2021 15:25:00 +0000
From:   Christoph Hellwig <hch@...radead.org>
To:     Sergei Shtepa <sergei.shtepa@...am.com>
Cc:     Christoph Hellwig <hch@...radead.org>,
        Mike Snitzer <snitzer@...hat.com>,
        Alasdair Kergon <agk@...hat.com>,
        Hannes Reinecke <hare@...e.de>, Jens Axboe <axboe@...nel.dk>,
        "dm-devel@...hat.com" <dm-devel@...hat.com>,
        "linux-block@...r.kernel.org" <linux-block@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-api@...r.kernel.org" <linux-api@...r.kernel.org>,
        Pavel Tide <Pavel.TIde@...am.com>
Subject: Re: [PATCH v7 3/3] dm: add DM_INTERPOSED_FLAG

On Tue, Mar 16, 2021 at 03:23:14PM +0000, Christoph Hellwig wrote:
> On Mon, Mar 15, 2021 at 04:25:09PM +0300, Sergei Shtepa wrote:
> > The 03/14/2021 12:30, Christoph Hellwig wrote:
> > > On Fri, Mar 12, 2021 at 06:44:55PM +0300, Sergei Shtepa wrote:
> > > > DM_INTERPOSED_FLAG allow to create DM targets on "the fly".
> > > > Underlying block device opens without a flag FMODE_EXCL.
> > > > DM target receives bio from the original device via
> > > > bdev_interposer.
> > > 
> > > This is more of a philopical comment, but the idea of just letting the
> > > interposed reopen the device by itself seems like a bad idea.  I think
> > > that is probably better hidden in the block layer interposer attachment
> > > function, which could do the extra blkdev_get_by_dev for the caller.
> > 
> > I suppose this cannot be implemented, since we need to change the behavior
> > for block devices that already have been opened.
> 
> That's not what I mean.  Take a look at the patch relative to your
> series to let me know what you think.  The new blkdev_interposer_attach
> now takes a dev_t + mode for the original device and opens it on
> behalf of the interposer.  It also moves the queue freezing into the
> API, which should address the concerns about the helper and adds a few
> more sanity checks.

And now actually with the diff:


diff --git a/block/blk-mq.c b/block/blk-mq.c
index 2f188a865024ac..d4d7c1caa43966 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -161,19 +161,6 @@ int blk_mq_freeze_queue_wait_timeout(struct request_queue *q,
 }
 EXPORT_SYMBOL_GPL(blk_mq_freeze_queue_wait_timeout);
 
-bool blk_mq_is_queue_frozen(struct request_queue *q)
-{
-	bool frozen;
-
-	mutex_lock(&q->mq_freeze_lock);
-	frozen = percpu_ref_is_dying(&q->q_usage_counter) &&
-		 percpu_ref_is_zero(&q->q_usage_counter);
-	mutex_unlock(&q->mq_freeze_lock);
-
-	return frozen;
-}
-EXPORT_SYMBOL_GPL(blk_mq_is_queue_frozen);
-
 /*
  * Guarantee no request is in use, so we can change any data structure of
  * the queue afterward.
diff --git a/block/genhd.c b/block/genhd.c
index fa406b972371ae..64d6338b08cc87 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -1944,51 +1944,70 @@ static void disk_release_events(struct gendisk *disk)
 	kfree(disk->ev);
 }
 
-int bdev_interposer_attach(struct block_device *original,
+struct block_device *blkdev_interposer_attach(dev_t dev, fmode_t mode,
 			   struct block_device *interposer)
 {
+	struct block_device *bdev;
 	int ret = 0;
 
-	if (WARN_ON(((!original) || (!interposer))))
-		return -EINVAL;
-	/*
-	 * interposer should be simple, no a multi-queue device
-	 */
-	if (!interposer->bd_disk->fops->submit_bio)
-		return -EINVAL;
+	if (WARN_ON_ONCE(!bdev_is_partition(interposer)))
+		return ERR_PTR(-EINVAL);
+	if (WARN_ON_ONCE(!queue_is_mq(interposer->bd_disk->queue)))
+		return ERR_PTR(-EINVAL);
 
-	if (WARN_ON(!blk_mq_is_queue_frozen(original->bd_disk->queue)))
-		return -EPERM;
+	bdev = blkdev_get_by_dev(dev, mode, NULL);
+	if (IS_ERR(bdev))
+		return bdev;
 
-	mutex_lock(&bdev_interposer_attach_lock);
+	ret = -EINVAL;
+	if (WARN_ON_ONCE(bdev_nr_sectors(bdev) != bdev_nr_sectors(interposer)))
+		goto out;
 
-	if (bdev_has_interposer(original))
-		ret = -EBUSY;
-	else {
-		original->bd_interposer = bdgrab(interposer);
-		if (!original->bd_interposer)
-			ret = -ENODEV;
-	}
+	blk_mq_freeze_queue(bdev->bd_disk->queue);
+	blk_mq_quiesce_queue(bdev->bd_disk->queue);
 
+	mutex_lock(&bdev_interposer_attach_lock);
+	ret = -EBUSY;
+	if (bdev_has_interposer(bdev))
+		goto out_unlock;
+	ret = -ENODEV;
+	bdev->bd_interposer = bdgrab(interposer);
+	if (!bdev->bd_interposer)
+		goto out_unlock;
+	ret = 0;
+out_unlock:
 	mutex_unlock(&bdev_interposer_attach_lock);
 
-	return ret;
+	blk_mq_unquiesce_queue(bdev->bd_disk->queue);
+	blk_mq_unfreeze_queue(bdev->bd_disk->queue);
+out:
+	if (ret) {
+		blkdev_put(bdev, mode);
+		bdev = ERR_PTR(ret);
+	}
+
+	return bdev;
 }
-EXPORT_SYMBOL_GPL(bdev_interposer_attach);
+EXPORT_SYMBOL_GPL(blkdev_interposer_attach);
 
-void bdev_interposer_detach(struct block_device *original)
+void blkdev_interposer_detach(struct block_device *bdev, fmode_t mode)
 {
-	if (WARN_ON(!original))
-		return;
+	struct block_device *interposer;
 
-	if (WARN_ON(!blk_mq_is_queue_frozen(original->bd_disk->queue)))
+	if (WARN_ON_ONCE(!bdev_has_interposer(bdev)))
 		return;
 
+	blk_mq_freeze_queue(bdev->bd_disk->queue);
+	blk_mq_quiesce_queue(bdev->bd_disk->queue);
+
 	mutex_lock(&bdev_interposer_attach_lock);
-	if (bdev_has_interposer(original)) {
-		bdput(original->bd_interposer);
-		original->bd_interposer = NULL;
-	}
+	interposer = bdev->bd_interposer;
+	bdev->bd_interposer = NULL;
 	mutex_unlock(&bdev_interposer_attach_lock);
+
+	blk_mq_unquiesce_queue(bdev->bd_disk->queue);
+	blk_mq_unfreeze_queue(bdev->bd_disk->queue);
+
+	blkdev_put(interposer, mode);
 }
-EXPORT_SYMBOL_GPL(bdev_interposer_detach);
+EXPORT_SYMBOL_GPL(blkdev_interposer_detach);
diff --git a/drivers/md/dm-table.c b/drivers/md/dm-table.c
index f6e2eb3f894940..fde57bb5105025 100644
--- a/drivers/md/dm-table.c
+++ b/drivers/md/dm-table.c
@@ -359,18 +359,6 @@ dev_t dm_get_dev_t(const char *path)
 }
 EXPORT_SYMBOL_GPL(dm_get_dev_t);
 
-static inline void dm_disk_freeze(struct gendisk *disk)
-{
-	blk_mq_freeze_queue(disk->queue);
-	blk_mq_quiesce_queue(disk->queue);
-}
-
-static inline void dm_disk_unfreeze(struct gendisk *disk)
-{
-	blk_mq_unquiesce_queue(disk->queue);
-	blk_mq_unfreeze_queue(disk->queue);
-}
-
 /*
  * Add a device to the list, or just increment the usage count if
  * it's already present.
@@ -418,29 +406,11 @@ int dm_get_device(struct dm_target *ti, const char *path, fmode_t mode,
 		refcount_inc(&dd->count);
 	}
 
-	if (t->md->is_interposed) {
-		struct block_device *original = dd->dm_dev->bdev;
-		struct block_device *interposer = t->md->disk->part0;
-
-		if ((ti->begin != 0) || (ti->len < bdev_nr_sectors(original))) {
-			dm_put_device(ti, dd->dm_dev);
-			DMERR("The interposer device should not be less than the original.");
-			return -EINVAL;
-		}
-
-		/*
-		 * Attach mapped interposer device to original.
-		 * It is quite convenient that device mapper creates
-		 * one disk for one block device.
-		 */
-		dm_disk_freeze(original->bd_disk);
-		r = bdev_interposer_attach(original, interposer);
-		dm_disk_unfreeze(original->bd_disk);
-		if (r) {
-			dm_put_device(ti, dd->dm_dev);
-			DMERR("Failed to attach dm interposer.");
-			return r;
-		}
+	if (t->md->is_interposed &&
+	    (ti->begin != 0 || ti->len < bdev_nr_sectors(dd->dm_dev->bdev))) {
+		dm_put_device(ti, dd->dm_dev);
+		DMERR("The interposer device should not be less than the original.");
+		return -EINVAL;
 	}
 
 	*result = dd->dm_dev;
@@ -496,11 +466,6 @@ void dm_put_device(struct dm_target *ti, struct dm_dev *d)
 		       dm_device_name(md), d->name);
 		return;
 	}
-	if (md->is_interposed) {
-		dm_disk_freeze(d->bdev->bd_disk);
-		bdev_interposer_detach(d->bdev);
-		dm_disk_unfreeze(d->bdev->bd_disk);
-	}
 
 	if (refcount_dec_and_test(&dd->count)) {
 		dm_put_table_device(md, d);
diff --git a/drivers/md/dm.c b/drivers/md/dm.c
index c488e9554aa000..532ce17064b1c1 100644
--- a/drivers/md/dm.c
+++ b/drivers/md/dm.c
@@ -763,10 +763,12 @@ static int open_table_device(struct table_device *td, dev_t dev,
 	BUG_ON(td->dm_dev.bdev);
 
 	if (md->is_interposed) {
-
-		bdev = blkdev_get_by_dev(dev, td->dm_dev.mode, NULL);
-		if (IS_ERR(bdev))
+		bdev = blkdev_interposer_attach(dev, td->dm_dev.mode,
+						md->disk->part0);
+		if (IS_ERR(bdev)) {
+			DMERR("Failed to attach dm interposer.");
 			return PTR_ERR(bdev);
+		}
 	} else {
 		bdev = blkdev_get_by_dev(dev, td->dm_dev.mode | FMODE_EXCL, _dm_claim_ptr);
 		if (IS_ERR(bdev))
@@ -793,9 +795,9 @@ static void close_table_device(struct table_device *td, struct mapped_device *md
 	if (!td->dm_dev.bdev)
 		return;
 
-	if (td->dm_dev.is_interposed)
-		blkdev_put(td->dm_dev.bdev, td->dm_dev.mode);
-	else {
+	if (td->dm_dev.is_interposed) {
+		blkdev_interposer_detach(td->dm_dev.bdev, td->dm_dev.mode);
+	} else {
 		bd_unlink_disk_holder(td->dm_dev.bdev, dm_disk(md));
 		blkdev_put(td->dm_dev.bdev, td->dm_dev.mode | FMODE_EXCL);
 	}
diff --git a/include/linux/blk-mq.h b/include/linux/blk-mq.h
index 6f01971abf7b9b..2c473c9b899089 100644
--- a/include/linux/blk-mq.h
+++ b/include/linux/blk-mq.h
@@ -533,7 +533,6 @@ void blk_freeze_queue_start(struct request_queue *q);
 void blk_mq_freeze_queue_wait(struct request_queue *q);
 int blk_mq_freeze_queue_wait_timeout(struct request_queue *q,
 				     unsigned long timeout);
-bool blk_mq_is_queue_frozen(struct request_queue *q);
 
 int blk_mq_map_queues(struct blk_mq_queue_map *qmap);
 void blk_mq_update_nr_hw_queues(struct blk_mq_tag_set *set, int nr_hw_queues);
diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h
index 90f62b4197da91..fbc510162c3827 100644
--- a/include/linux/blkdev.h
+++ b/include/linux/blkdev.h
@@ -2036,8 +2036,8 @@ static inline bool bdev_has_interposer(struct block_device *bdev)
 	return (bdev->bd_interposer != NULL);
 };
 
-int bdev_interposer_attach(struct block_device *original,
+struct block_device *blkdev_interposer_attach(dev_t dev, fmode_t mode,
 			   struct block_device *interposer);
-void bdev_interposer_detach(struct block_device *original);
+void blkdev_interposer_detach(struct block_device *bdev, fmode_t mode);
 
 #endif /* _LINUX_BLKDEV_H */

Powered by blists - more mailing lists