Re: [dm-devel] [RFC PATCH V2 2/3] block: add ->poll_bio to block_device_operations

[Ming Lei <ming.lei@redhat.com>]

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

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel