From: Jens Axboe <axboe@kernel.dk>
To: Mike Snitzer <snitzer@redhat.com>
Cc: linux-block@vger.kernel.org, dm-devel@redhat.com, hch@lst.de,
ming.lei@redhat.com
Subject: Re: [dm-devel] [PATCH v4 1/2] block: add ->poll_bio to block_device_operations
Date: Fri, 4 Mar 2022 14:39:26 -0700 [thread overview]
Message-ID: <68dc8fb0-86df-effe-4ef2-8ed9c350d836@kernel.dk> (raw)
In-Reply-To: <20220304212623.34016-2-snitzer@redhat.com>
On 3/4/22 2:26 PM, Mike Snitzer wrote:
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 94bf37f8e61d..e739c6264331 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -985,10 +985,16 @@ int bio_poll(struct bio *bio, struct io_comp_batch *iob, unsigned int flags)
>
> if (blk_queue_enter(q, BLK_MQ_REQ_NOWAIT))
> return 0;
> - if (WARN_ON_ONCE(!queue_is_mq(q)))
> - ret = 0; /* not yet implemented, should not happen */
> - else
> + if (queue_is_mq(q)) {
> ret = blk_mq_poll(q, cookie, iob, flags);
> + } else {
> + struct gendisk *disk = q->disk;
> +
> + if (disk && disk->fops->poll_bio)
> + ret = disk->fops->poll_bio(bio, iob, flags);
> + else
> + ret = !WARN_ON_ONCE(1);
This is an odd way to do it, would be a lot more readable as
ret = 0;
WARN_ON_ONCE(1);
if we even need that WARN_ON?
> diff --git a/block/genhd.c b/block/genhd.c
> index e351fac41bf2..eb43fa63ba47 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -410,6 +410,8 @@ int __must_check device_add_disk(struct device *parent, struct gendisk *disk,
> struct device *ddev = disk_to_dev(disk);
> int ret;
>
> + WARN_ON_ONCE(queue_is_mq(disk->queue) && disk->fops->poll_bio);
Also seems kind of useless, maybe at least combine it with failing to
add the disk. This is a "I'm developing some new driver or feature"
failure, and would be more visible that way. And if you do that, then
the WARN_ON_ONCE() seems pointless anyway, and I'd just do:
if (queue_is_mq(disk->queue) && disk->fops->poll_bio)
return -EINVAL;
or something like that, with a comment saying why that doesn't make any
sense.
--
Jens Axboe
--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel
WARNING: multiple messages have this Message-ID (diff)
From: Jens Axboe <axboe@kernel.dk>
To: Mike Snitzer <snitzer@redhat.com>
Cc: ming.lei@redhat.com, hch@lst.de, dm-devel@redhat.com,
linux-block@vger.kernel.org
Subject: Re: [PATCH v4 1/2] block: add ->poll_bio to block_device_operations
Date: Fri, 4 Mar 2022 14:39:26 -0700 [thread overview]
Message-ID: <68dc8fb0-86df-effe-4ef2-8ed9c350d836@kernel.dk> (raw)
In-Reply-To: <20220304212623.34016-2-snitzer@redhat.com>
On 3/4/22 2:26 PM, Mike Snitzer wrote:
> diff --git a/block/blk-core.c b/block/blk-core.c
> index 94bf37f8e61d..e739c6264331 100644
> --- a/block/blk-core.c
> +++ b/block/blk-core.c
> @@ -985,10 +985,16 @@ int bio_poll(struct bio *bio, struct io_comp_batch *iob, unsigned int flags)
>
> if (blk_queue_enter(q, BLK_MQ_REQ_NOWAIT))
> return 0;
> - if (WARN_ON_ONCE(!queue_is_mq(q)))
> - ret = 0; /* not yet implemented, should not happen */
> - else
> + if (queue_is_mq(q)) {
> ret = blk_mq_poll(q, cookie, iob, flags);
> + } else {
> + struct gendisk *disk = q->disk;
> +
> + if (disk && disk->fops->poll_bio)
> + ret = disk->fops->poll_bio(bio, iob, flags);
> + else
> + ret = !WARN_ON_ONCE(1);
This is an odd way to do it, would be a lot more readable as
ret = 0;
WARN_ON_ONCE(1);
if we even need that WARN_ON?
> diff --git a/block/genhd.c b/block/genhd.c
> index e351fac41bf2..eb43fa63ba47 100644
> --- a/block/genhd.c
> +++ b/block/genhd.c
> @@ -410,6 +410,8 @@ int __must_check device_add_disk(struct device *parent, struct gendisk *disk,
> struct device *ddev = disk_to_dev(disk);
> int ret;
>
> + WARN_ON_ONCE(queue_is_mq(disk->queue) && disk->fops->poll_bio);
Also seems kind of useless, maybe at least combine it with failing to
add the disk. This is a "I'm developing some new driver or feature"
failure, and would be more visible that way. And if you do that, then
the WARN_ON_ONCE() seems pointless anyway, and I'd just do:
if (queue_is_mq(disk->queue) && disk->fops->poll_bio)
return -EINVAL;
or something like that, with a comment saying why that doesn't make any
sense.
--
Jens Axboe
next prev parent reply other threads:[~2022-03-04 21:39 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-03-04 21:26 [dm-devel] [PATCH v4 0/2] block/dm: support bio polling Mike Snitzer
2022-03-04 21:26 ` Mike Snitzer
2022-03-04 21:26 ` [dm-devel] [PATCH v4 1/2] block: add ->poll_bio to block_device_operations Mike Snitzer
2022-03-04 21:26 ` Mike Snitzer
2022-03-04 21:39 ` Jens Axboe [this message]
2022-03-04 21:39 ` Jens Axboe
2022-03-05 1:30 ` [dm-devel] " Mike Snitzer
2022-03-05 1:30 ` Mike Snitzer
2022-03-04 21:26 ` [dm-devel] [PATCH v4 2/2] dm: support bio polling Mike Snitzer
2022-03-04 21:26 ` Mike Snitzer
2022-03-05 1:43 ` [dm-devel] [PATCH v4 0/2] block/dm: " Ming Lei
2022-03-05 1:43 ` Ming Lei
2022-03-05 2:14 ` [dm-devel] " Mike Snitzer
2022-03-05 2:14 ` Mike Snitzer
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=68dc8fb0-86df-effe-4ef2-8ed9c350d836@kernel.dk \
--to=axboe@kernel.dk \
--cc=dm-devel@redhat.com \
--cc=hch@lst.de \
--cc=linux-block@vger.kernel.org \
--cc=ming.lei@redhat.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 an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.