On Mon, Jun 21, 2021 at 09:25:02AM +0200, Christoph Hellwig wrote:
> > + struct gendisk *disk = bio->bi_bdev->bd_disk;
> > + struct request_queue *q = disk->queue;
> > blk_qc_t cookie = READ_ONCE(bio->bi_cookie);
> > int ret;
> >
> > - if (cookie == BLK_QC_T_NONE || !blk_queue_poll(q))
> > + if ((queue_is_mq(q) && cookie == BLK_QC_T_NONE) ||
> > + !blk_queue_poll(q))
> > return 0;
>
> How does polling for a bio without a cookie make sense even when
> polling bio based?
It isn't necessary to use bio->bi_cookie, that is why I doesn't use it,
which actually provides one free 32bit in bio for bio based driver.
>
> But if we come up for a good rationale for this I'd really
> split the conditions to make them more readable:
>
> if (!test_bit(QUEUE_FLAG_POLL, &q->queue_flags))
> return 0;
> if (queue_is_mq(q) && cookie == BLK_QC_T_NONE)
> return 0;
OK.
>
> > + if (!queue_is_mq(q)) {
> > + if (disk->fops->poll_bio) {
> > + ret = disk->fops->poll_bio(bio, flags);
> > + } else {
> > + WARN_ON_ONCE(1);
> > + ret = 0;
> > + }
> > + } else {
> > ret = blk_mq_poll(q, cookie, flags);
>
> I'd go for someting like:
>
> if (queue_is_mq(q))
> ret = blk_mq_poll(q, cookie, flags);
> else if (disk->fops->poll_bio)
> ret = disk->fops->poll_bio(bio, flags);
> else
> WARN_ON_ONCE(1);
>
> with ret initialized to 0 at declaration time.
Fine.
>
> > struct block_device_operations {
> > void (*submit_bio)(struct bio *bio);
> > + /* ->poll_bio is for bio driver only */
>
> I'd drop the comment, this is already nicely documented in add_disk
> together with the actual check. We also don't note this for submit_bio
> here.
OK.
thanks,
Ming
