Re: [Xen-devel] [PATCH 1/2] xenstore: add support for reading directory with many children

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 25.10.16 at 15:47, <JGross@xxxxxxxx> wrote:
> On 25/10/16 15:20, Jan Beulich wrote:
>>>>> On 25.10.16 at 13:41, <JGross@xxxxxxxx> wrote:
>>> On 25/10/16 11:06, Jan Beulich wrote:
>>>>>>> On 25.10.16 at 07:52, <JGross@xxxxxxxx> wrote:
>>>>> --- a/tools/xenstore/xenstored_transaction.c
>>>>> +++ b/tools/xenstore/xenstored_transaction.c
>>>>> @@ -79,6 +79,11 @@ struct transaction
>>>>>  
>>>>>   /* List of changed domains - to record the changed domain entry number 
>>>>> */
>>>>>   struct list_head changed_domains;
>>>>> +
>>>>> + /* Temporary buffer for XS_DIRECTORY_PART. */
>>>>> + char *dirpart_buf;
>>>>> + unsigned buf_off;
>>>>> + unsigned buf_len;
>>>>>  };
>>>>
>>>> The buffer you allocate for this can - by nature of this change - be
>>>> huge, and there's one per transaction. Isn't this causing a resource
>>>> utilization issue?
>>>
>>> It will be allocated only when needed and its size is less than that of
>>> the node to be read.
>> 
>> That's not addressing my concern. A DomU could have multiple
>> transactions each with such an operation in progress. And the
>> space here doesn't get accounted against any of the quota.
> 
> It is accounted against the maximum number of transactions.
> 
> Additionally the buffer will be kept only if not all of the data
> could be transferred in the first iteration of the read.

Which nevertheless means an ill behaved guest could (if its quota
allow) create a node with sufficiently many children and then start
as many transactions as it can, and do a partial directory listing
over each of them. Perhaps, considering that you mainly need this
for Dom0, the new verb could be restricted to the control domain
for now?

>>>> And then - why did you put this function here instead of in
>>>> xenstored_core.c, e.g. next to send_directory()?
>>>
>>> It is using the xenstored_transaction.c private struct transaction.
>>> Putting it in xenstored_core.c would have required to move the
>>> struct definition into a header file.
>> 
>> Yeah, I've realized this after having sent the reply, but then again
>> this also relates to the first question above (whether this should be
>> a per-transaction item in the first place). In particular I don't think
>> this being per transaction helps with allowing the guest to (easily)
>> run multiple such queries in parallel, as commonly simple operations
>> get run with null transactions.
> 
> The XS_DIRECTORY_PART command is valid in a transaction only. This is
> a prerequisite to be able to split the operation into multiple pieces
> while keeping the consistency.

But I'm sure other models could be come up with, ideally without
requiring meaningful amounts of temporary storage. For instance
nodes could be versioned, and if the version changed between
iterations, the caller could be signaled to start over. And it looks
like such a version wouldn't even need to be bumped for child
additions, but only for child removals.

Jan

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
https://lists.xen.org/xen-devel