Re: [Devel] [PATCH hci-8.1] drivers/vhost/blk: add support for worker ioctls

[Alexander Ivanov <alexander.ivanov@virtuozzo.com>]

>> @@ -909,6 +909,14 @@ static long vhost_blk_ioctl(struct file *f, unsigned int ioctl,
>>   		return vhost_blk_set_features(blk, features);
>>   	case VHOST_RESET_OWNER:
>>   		return vhost_blk_reset_owner(blk);
>> +	case VHOST_NEW_WORKER:
>> +	case VHOST_FREE_WORKER:
>> +	case VHOST_ATTACH_VRING_WORKER:
>> +	case VHOST_GET_VRING_WORKER:
>> +		mutex_lock(&blk->dev.mutex);
>> +		ret = vhost_worker_ioctl(&blk->dev, ioctl, argp);
>> +		mutex_unlock(&blk->dev.mutex);
>> +		return ret;
> [Severity: Medium]
> Can a completed request be stranded here, hanging vhost_blk_flush()?
>
> Once user-created workers can be attached to vhost-blk virtqueues, a vq
> can lose its worker. When a worker dies, vhost_worker_killed() does:
>
>      rcu_assign_pointer(vq->worker, NULL);
>
> After that, a bio that completes goes through vhost_blk_req_done():
>
>      if (atomic_dec_and_test(&req->bio_nr)) {
>          llist_add(&req->llnode, &req->blk_vq->llhead);
>          vhost_vq_work_queue(&req->blk_vq->vq, &req->blk_vq->work);
>      }
>
> vhost_vq_work_queue() finds rcu_dereference(vq->worker) == NULL, queues
> nothing and returns false. vhost_blk_req_done() ignores that return value.
>
> Only vhost_blk_handle_host_kick() calls vhost_blk_req_cleanup() and
> forget_request(), and that work will never run. The request therefore
> stays on llhead. Its bio, iov and bounce buffer are not freed, and
> req_inflight[] is never decremented.
>
> vhost_blk_flush() then does an untimed wait:
>
>      wait_event(blk->flush_wait,
>                 !atomic_read(&blk->req_inflight[flush_bin]));
>
> vhost_dev_flush() skips killed workers, because __vhost_worker_flush()
> returns early when worker->killed is set, so nothing wakes this wait.
>
> This wait is reachable from several paths:
>
>      vhost_blk_release()->vhost_blk_drop_backend()->vhost_blk_flush()
>      vhost_blk_reset_owner()->vhost_blk_drop_backend()->vhost_blk_flush()
>      vhost_blk_set_features()->vhost_blk_flush()
>
> The release path can hang, so the backend file reference is not dropped
> and the vhost_blk is not freed.
>
> The hang can already occur for the default worker in vhost_task mode.
> This patch also exposes it to user-created workers, through
> VHOST_NEW_WORKER and VHOST_ATTACH_VRING_WORKER.
>
> Should vhost_blk_req_done() handle a false return from
> vhost_vq_work_queue(), for example by cleaning up the request directly?
This hang is pre-existing and not introduced by this patch, since the
default worker can already hit it in vhost_task mode, so we will fix it
later in a separate patch.
_______________________________________________
Devel mailing list
Devel@openvz.org
https://lists.openvz.org/mailman/listinfo/devel