On Fri, Mar 12, 2021 at 06:44:54PM +0300, Sergei Shtepa wrote:

bdev_interposer allows to redirect bio requests to another devices.

I think this warrants a somewhat more detailed description.

The code itself looks pretty good to me now, a bunch of nitpicks and
a question below:

+static noinline blk_qc_t submit_bio_interposed(struct bio *bio)
+{
+	blk_qc_t ret = BLK_QC_T_NONE;
+	struct bio_list bio_list[2] = { };
+	struct gendisk *orig_disk;
+
+	if (current->bio_list) {
+		bio_list_add(&current->bio_list[0], bio);
+		return BLK_QC_T_NONE;
+	}

I don't think this case can ever happen:

 - current->bio_list != NULL means a ->submit_bio or blk_mq_submit_bio
   is active.  But if this device is being interposed this means the
   interposer recurses into itself, which should never happen.  So
   I think we'll want a WARN_ON_ONCE here as a debug check instead.

+
+	orig_disk = bio->bi_bdev->bd_disk;
+	if (unlikely(bio_queue_enter(bio)))
+		return BLK_QC_T_NONE;
+
+	current->bio_list = bio_list;
+
+	do {
+		struct block_device *interposer = bio->bi_bdev->bd_interposer;
+
+		if (unlikely(!interposer)) {
+			/* interposer was removed */
+			bio_list_add(&current->bio_list[0], bio);
+			break;
+		}
+		/* assign bio to interposer device */
+		bio_set_dev(bio, interposer);
+		bio_set_flag(bio, BIO_INTERPOSED);

Reassigning the bi_bdev here means the original source is lost by the
time we reach the interposer.  This initially seemed a little limiting,
but I guess the interposer driver can just record that information
locally, so we should be fine.  The big upside of this is that no
extra argument to submit_bio_checks, which means less changes to the
normal fast path, so if this works for everyone that is a nice
improvement over my draft.

+
+		if (!submit_bio_checks(bio))
+			break;
+		/*
+		 * Because the current->bio_list is initialized,
+		 * the submit_bio callback will always return BLK_QC_T_NONE.
+		 */
+		interposer->bd_disk->fops->submit_bio(bio);
+	} while (false);

I find the do { ... } while (false) idiom here a little strange.  Normal
kernel style would be a goto done instead of the breaks.

+int bdev_interposer_attach(struct block_device *original,
+			   struct block_device *interposer)

A kerneldoc comment for bdev_interposer_attach (and
bdev_interposer_detach) would be nice to explain the API a little more.

+{
+	int ret = 0;
+
+	if (WARN_ON(((!original) || (!interposer))))
+		return -EINVAL;

No need for the inner two levels of braces.

+	 * interposer should be simple, no a multi-queue device
+	 */
+	if (!interposer->bd_disk->fops->submit_bio)

Please use queue_is_mq() instead.

+	if (bdev_has_interposer(original))
+		ret = -EBUSY;
+	else {
+		original->bd_interposer = bdgrab(interposer);

Just thinking out a loud:  what happens if the interposed device
goes away?  Shouldn't we at very least also make sure this
gabs another refererence on bdev as well?

+struct bdev_interposer;

Not needed any more.

+static inline bool bdev_has_interposer(struct block_device *bdev)
+{
+	return (bdev->bd_interposer != NULL);
+};

No need for the braces.