Re: [PATCH] blk-mq: Return invalid cookie if bio was split

On Fri, Oct 07, 2016 at 12:06:27AM +0800, Ming Lei wrote:

Hi Keith,

On Thu, Oct 6, 2016 at 12:51 AM, Keith Busch [off-list ref] wrote:

quoted

On Wed, Oct 05, 2016 at 11:19:39AM +0800, Ming Lei wrote:

quoted

But .poll() need to check if the specific request is completed or not,
then blk_poll() can set 'current' as RUNNING if it is completed.

blk_poll():
                ...
                ret = q->mq_ops->poll(hctx, blk_qc_t_to_tag(cookie));
                if (ret > 0) {
                        hctx->poll_success++;
                        set_current_state(TASK_RUNNING);
                        return true;
                }

Right, but the task could be waiting on a whole lot more than just that
one tag, so setting the task to running before knowing those all complete
doesn't sound right.

quoted

I am glad to take a look the patch if you post it out.

Here's what I got for block + nvme. It relies on all the requests to
complete to set the task back to running.

Yeah, but your patch doesn't add that change, and looks 'task_struct *'
in submission path need to be stored in request or somewhere else.

The polling function shouldn't have to set the task to running. The
task_struct is the dio's "waiter", and dio_bio_end_io sets its state
to noromal running when every bio submitted and split chained bios
complete. Hopefully those all complete through the ->poll(), and
blk_poll will automatically observe the state changed.

Then looks the whole hw queue is polled and only the queue num
in cookie matters.

In theory, there might be one race:

- one dio need to submit several bios(suppose there are two bios: A and B)
- A is submitted to hardware queue M
- B is submitted to hardware queue N because the current task is migrated
to another CPU
- then only hardware queue N is polled

Yeah, in that case we'd rely on the queue M's interrupt handler to do
the completion.

Avoiding the context switch was the biggest win for polling, as I
understand it. If the task is being migrated to other CPUs, I think
we've already lost the latency benefit we'd have got.