[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH v3 17/25] tools/xenstore: rework struct xs_tdb_record_hdr



On 28.07.23 16:59, Julien Grall wrote:
Hi Juergen,

On 28/07/2023 15:32, Juergen Gross wrote:
On 28.07.23 16:08, Julien Grall wrote: >>>> Using multiple commands has also its downside. The first that comes
to my mind if that you need to keep around the data. But, with your proposal, you we wouldn't be able to store it in the database (like for transaction update) as datalen can only be 65KB.

I wasn't aware that a complete transaction needs to be kept in a single data
base record. :-)

IIUC, you are thinking that the client will restore all the state bits by bits. But if you look at the design in docs/designs/xenstore-migration.md, this is a blob.

Of course it is.

I was never assuming that the state would be applied piecemeal, this has to
happen atomically.

I am confused because I don't see how this related to the discussion. Above, you mention a transaction, which I interpreted as the client would open a transaction and do a bunch of "write note", "set permissions"... And then commit the transaction.

I thought this is what you talked about and this would still be atomically. My point with the blob is that the parsing of the state is done by Xenstored, not the client.

My idea was that the transaction would be used to mark the related sub-commands
to belong to a single migration. This would make cleaning up in case of client
death much simpler.

Not using transactions would be possible, too, of course.



It would work perfectly fine to allocate the needed memory via talloc() and to
reference it from a special node being part of the transaction, or to not use
a node at all (see again the XS_CONTROL example).

I am not convinced the complexity is worth it here. To be honest, I think the payload limit should have been relaxed for Live-Update as well as you don't gain much to split. That said, this is less a concern because you are not time constrained.

[...]

But maybe that comment was based on wrong assumptions, like the mentioned
change not violating the protocol. >
I am happy to rewrite the comment so it doesn't lead to think that you (as the maintainer) are open to have a more relax length check.

Yes, please make a suggestion for a proper comment not suggesting we are fine
to violate the wire protocol.

Here we go:

"The payload size is not only currently restricted by the protocol but also the internal implementation (see various BUILD_BUG_ON())."

Hmm, I'm still feeling uneasy to imply that the payload size might be changed.
See above reasoning.

The only way I could imagine this being possible would be a per-ring-page
attribute with both sides agreeing to the max allowed size (the minimum being
today's value).

With that in mind I can hesitantly add the comment, maybe with the addition:
"Any potential change of the maximum payload size needs to be negotiated between
the involved parties."

I am ok with that.

Okay.


Juergen

Attachment: OpenPGP_0xB0DE9DD628BF132F.asc
Description: OpenPGP public key

Attachment: OpenPGP_signature
Description: OpenPGP digital signature


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.