On Fri, Mar 04 2022 at 4:39P -0500,
Jens Axboe <axboe@xxxxxxxxx> wrote:
> 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?
Would be a pretty glaring oversight for a bio-based driver developer
to forget to define ->poll_bio but remember to clear BLK_QC_T_NONE in
bio->bi_cookie and set QUEUE_FLAG_POLL in queue flags.
Silent failure it is! ;)
> > 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.
Absolutely. The thought did cross my mind that it seemed WARN_ON heavy.
Will fix it up and send v5.
--
dm-devel mailing list
dm-devel@xxxxxxxxxx
https://listman.redhat.com/mailman/listinfo/dm-devel
