[PATCH] nvme-tcp: Fix possible race of io_work and direct send

[Or Gerlitz]

On Fri, Jan 8, 2021 at 10:20 PM Sagi Grimberg <sagi at grimberg.me> wrote:

> Well, this is a heuristic anyways, too bad we need to
> disable preemption just for this...
>
> I'm assuming that this makes the issue go away?
> --
> diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
> index cf856103025a..ea07336ca179 100644
> --- a/drivers/nvme/host/tcp.c
> +++ b/drivers/nvme/host/tcp.c
> @@ -281,8 +281,11 @@ static inline void nvme_tcp_queue_request(struct
> nvme_tcp_request *req,
>                  bool sync, bool last)
>   {
>          struct nvme_tcp_queue *queue = req->queue;
> +       int cpu;
>          bool empty;
>
> +       cpu = get_cpu();
> +       put_cpu();
>          empty = llist_add(&req->lentry, &queue->req_list) &&
>                  list_empty(&queue->send_list) && !queue->request;
>
> @@ -291,8 +294,8 @@ static inline void nvme_tcp_queue_request(struct
> nvme_tcp_request *req,
>           * directly, otherwise queue io_work. Also, only do that if we
>           * are on the same cpu, so we don't introduce contention.
>           */
> -       if (queue->io_cpu == smp_processor_id() &&
> -           sync && empty && mutex_trylock(&queue->send_mutex)) {
> +       if (queue->io_cpu == cpu && sync && empty &&


if preemption happens here, the value returned by get_cpu() is not where
we run when the actual comparison takes place. So what you are saying
is that overall correctness will not be hurt? worth putting a comment
here for readers / future modifiers of the code

> +           mutex_trylock(&queue->send_mutex)) {
>                  queue->more_requests = !last;
>                  nvme_tcp_send_all(queue);
>                  queue->more_requests = false;