| Message ID | 20220304212623.34016-2-snitzer@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | block/dm: support bio polling | expand |
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.
On Fri, Mar 04 2022 at 4:39P -0500, Jens Axboe <axboe@kernel.dk> 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.
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); + } blk_queue_exit(q); return ret; } 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); + /* * The disk queue should now be all set with enough information about * the device for the elevator code to pick an adequate default diff --git a/include/linux/blkdev.h b/include/linux/blkdev.h index f757f9c2871f..51f1b1ddbed2 100644 --- a/include/linux/blkdev.h +++ b/include/linux/blkdev.h @@ -1455,6 +1455,8 @@ enum blk_unique_id { struct block_device_operations { void (*submit_bio)(struct bio *bio); + int (*poll_bio)(struct bio *bio, struct io_comp_batch *iob, + unsigned int flags); int (*open) (struct block_device *, fmode_t); void (*release) (struct gendisk *, fmode_t); int (*rw_page)(struct block_device *, sector_t, struct page *, unsigned int);