Re: [PATCH v3 3/3] xen: optimize xenbus driver for multiple concurrent xenstore accesses

[Juergen Gross]

On 07/02/17 23:39, Boris Ostrovsky wrote:
> On 02/07/2017 12:51 PM, Boris Ostrovsky wrote:
>> On 01/24/2017 11:23 AM, Juergen Gross wrote:
>>> On 24/01/17 14:47, Boris Ostrovsky wrote:
>>>> On 01/23/2017 01:59 PM, Boris Ostrovsky wrote:
>>>>> On 01/23/2017 05:09 AM, Juergen Gross wrote:
>>>>>> Handling of multiple concurrent Xenstore accesses through xenbus driver
>>>>>> either from the kernel or user land is rather lame today: xenbus is
>>>>>> capable to have one access active only at one point of time.
>>>>>>
>>>>>>
>>>> This patch appears to break save/restore:
>>> Hmm, tried multiple times, but I can't reproduce this issue.
>>>
>>> Anything special in the setup? I tried a 64 bit pv guest and did
>>> "xl save".
>>>
>>> Do I have to run some load in parallel?
>> Any luck reproducing this? I am still failing the test on dumpdata but I
>> couldn't reproduce it on another system.
> 
> 
> The problem appears to be xs_state_users being non-zero when we call
> xs_suspend_enter().
> 
> From what I understand this is caused by xs_request_exit() not
> decrementing it when closing a transaction. This seems to be happening
> when XS_TRANSACTION_END transaction returns XS_ERROR (I haven't traced
> what causes this error but it doesn't appear to cause any visible harm).

Aah, of course: this will happen for a transaction failing due to a
conflict (EAGAIN case).

> Does the patch below make sense?

As the xenbus driver is checking for the transaction id to be valid this
is okay, I think.

I have noticed another problem, though: a user client mixing Xenstore
accesses with and without transactions could be blocked when doing a
non-transactional access hindering it from completing the transaction.

I'll send an updated version including your fix.

> 
> diff --git a/drivers/xen/xenbus/xenbus_xs.c b/drivers/xen/xenbus/xenbus_xs.c
> index e62cb09..ffd5fac 100644
> --- a/drivers/xen/xenbus/xenbus_xs.c
> +++ b/drivers/xen/xenbus/xenbus_xs.c
> @@ -140,7 +140,7 @@ void xs_request_exit(struct xb_req_data *req)
>         spin_lock(&xs_state_lock);
>         xs_state_users--;
>         if ((req->type == XS_TRANSACTION_START && req->msg.type ==
> XS_ERROR) ||
> -           req->msg.type == XS_TRANSACTION_END)
> +           req->type == XS_TRANSACTION_END)
>                 xs_state_users--;
>         spin_unlock(&xs_state_lock);
> 
> 
> I ran a few tests on dumpdata and they completed successfully. I'll keep
> this for the overnight runs too, with a different Xen version.

Thanks.


Juergen