The 03/14/2021 12:28, Christoph Hellwig wrote: > 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(¤t->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.
Yes, it is. Completely remove this check or add "BUG_ON(current->bio_list);" for an emergency? > > > + > > + 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(¤t->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. > Ok. I'll use the normal kernel style. > > +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. > Yes, I should add kerneldoc comments. > > +{ > > + int ret = 0; > > + > > + if (WARN_ON(((!original) || (!interposer)))) > > + return -EINVAL; > > No need for the inner two levels of braces. Ok. > > > + * interposer should be simple, no a multi-queue device > > + */ > > + if (!interposer->bd_disk->fops->submit_bio) > > Please use queue_is_mq() instead. Ok. > > > + 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? If the original device is removed from the system, the interposer device will be permanently occupied. I need to add an interposer release when deleting a block device. > > > +struct bdev_interposer; > > Not needed any more. Yes. > > > +static inline bool bdev_has_interposer(struct block_device *bdev) > > +{ > > + return (bdev->bd_interposer != NULL); > > +}; > > No need for the braces. Ok. -- Sergei Shtepa Veeam Software developer. -- dm-devel mailing list dm-devel@redhat.com https://listman.redhat.com/mailman/listinfo/dm-devel