On Wed, Oct 23, 2019 at 01:58:24PM -0700, Bart Van Assche wrote:
> On 2019-10-22 02:41, Ming Lei wrote:
> > On Mon, Oct 21, 2019 at 03:42:57PM -0700, Bart Van Assche wrote:
> >> +int blk_poll(struct request_queue *q, blk_qc_t cookie, bool spin)
> >> +{
> >> + int ret;
> >> +
> >> + if (!percpu_ref_tryget(&q->q_usage_counter))
> >> + return 0;
> >> + ret = __blk_poll(q, cookie, spin);
> >> + blk_queue_exit(q);
> >> +
> >> + return ret;
> >> +}
> >
> > IMO, this change isn't required. Caller of blk_poll is supposed to
> > hold refcount of the request queue, then the related hctx data structure
> > won't go away. When the hctx is in transient state, there can't be IO
> > to be polled, and it is safe to call into IO path.
> >
> > BTW, .poll is absolutely the fast path, we should be careful to add code
> > in this path.
>
> Hi Ming,
>
> I'm not sure whether all blk_poll() callers really hold a refcount on
> the request queue.
Please see __device_add_disk() which takes one extra queue ref, which
won't be dropped until the disk is released.
Meantime blkdev_get() holds gendisk reference via bdev_get_gendisk(),
and blkdev_get() is called by blkdev_open().
The above way should guarantee that the request queue won't go away
when IO is submitted to queue via blkdev fs inode.
> Anyway, I will convert this code change into a
> comment that explains that blk_poll() callers must hold a request queue
> reference.
Good point.
Thanks,
Ming
