Re: [PATCH v4 1/2] block: add ->poll_bio to block_device_operations

[Jens Axboe <axboe@xxxxxxxxx>]

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