From: Christoph Hellwig <hch@infradead.org>
To: Sergei Shtepa <sergei.shtepa@veeam.com>
Cc: Christoph Hellwig <hch@infradead.org>,
Mike Snitzer <snitzer@redhat.com>,
Alasdair Kergon <agk@redhat.com>, Hannes Reinecke <hare@suse.de>,
Jens Axboe <axboe@kernel.dk>,
dm-devel@redhat.com, linux-block@vger.kernel.org,
linux-kernel@vger.kernel.org, linux-api@vger.kernel.org,
pavel.tide@veeam.com
Subject: Re: [PATCH v7 2/3] block: add bdev_interposer
Date: Sun, 14 Mar 2021 09:28:23 +0000 [thread overview]
Message-ID: <20210314092823.GB3773360@infradead.org> (raw)
In-Reply-To: <1615563895-28565-3-git-send-email-sergei.shtepa@veeam.com>
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.
> +
> + 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.
> +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.
next prev parent reply other threads:[~2021-03-14 9:29 UTC|newest]
Thread overview: 25+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-12 15:44 [PATCH v7 0/3] block device interposer Sergei Shtepa
2021-03-12 15:44 ` [PATCH v7 1/3] block: add blk_mq_is_queue_frozen() Sergei Shtepa
2021-03-12 19:06 ` Mike Snitzer
2021-03-14 9:14 ` Christoph Hellwig
2021-03-15 12:06 ` Sergei Shtepa
2021-03-12 15:44 ` [PATCH v7 2/3] block: add bdev_interposer Sergei Shtepa
2021-03-14 9:28 ` Christoph Hellwig [this message]
2021-03-15 13:06 ` Sergei Shtepa
2021-03-16 8:09 ` Ming Lei
2021-03-16 16:35 ` Sergei Shtepa
2021-03-17 3:03 ` Ming Lei
2021-03-17 12:22 ` Sergei Shtepa
2021-03-17 15:04 ` Mike Snitzer
2021-03-17 18:14 ` Sergei Shtepa
2021-03-17 19:13 ` Mike Snitzer
2021-03-18 14:56 ` Sergei Shtepa
2021-03-17 14:58 ` Mike Snitzer
2021-03-12 15:44 ` [PATCH v7 3/3] dm: add DM_INTERPOSED_FLAG Sergei Shtepa
2021-03-12 19:00 ` Mike Snitzer
2021-03-15 12:29 ` Sergei Shtepa
2021-03-14 9:30 ` Christoph Hellwig
2021-03-15 13:25 ` Sergei Shtepa
2021-03-16 15:23 ` Christoph Hellwig
2021-03-16 15:25 ` Christoph Hellwig
2021-03-16 16:20 ` Sergei Shtepa
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210314092823.GB3773360@infradead.org \
--to=hch@infradead.org \
--cc=agk@redhat.com \
--cc=axboe@kernel.dk \
--cc=dm-devel@redhat.com \
--cc=hare@suse.de \
--cc=linux-api@vger.kernel.org \
--cc=linux-block@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=pavel.tide@veeam.com \
--cc=sergei.shtepa@veeam.com \
--cc=snitzer@redhat.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).