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

From: Mike Snitzer <snitzer@redhat.com>
To: Jens Axboe <axboe@kernel.dk>
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 20:30:46 -0500	[thread overview]
Message-ID: <YiK9Rgvx0Z3aOGNQ@redhat.com> (raw)
In-Reply-To: <68dc8fb0-86df-effe-4ef2-8ed9c350d836@kernel.dk>

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.