Re: [PATCH v1 1/3] softmmu/physmem: fallback to opening guest RAM file as readonly in a MAP_PRIVATE mapping

[David Hildenbrand <david@redhat.com>]

On 17.08.23 15:42, Daniel P. Berrangé wrote:
> On Thu, Aug 10, 2023 at 04:19:45PM +0200, David Hildenbrand wrote:
>>>> Most importantly, we won't be corrupting/touching the original file in any
>>>> case, because it is R/O.
>>>>
>>>> If we really want to be careful, we could clue that behavior to compat
>>>> machines. I'm not really sure yet if we really have to go down that path.
>>>>
>>>> Any other alternatives? I'd like to avoid new flags where not really
>>>> required.
>>>
>>> I was just thinking of a new flag. :) So have you already discussed that
>>> possibility and decided that not a good idea?
>>
>> Not really. I was briefly playing with that idea but already struggled to
>> come up with a reasonable name :)
>>
>> Less toggles and just have it working nice, if possible.
> 
> IMHO having a new flag is desirable, because it is directly
> expressing the desired deployment scenario, such tat we get
> good error reporting upon deployment mistakes, while at the
> same time allowing the readonly usage.
> 
>>> The root issue to me here is we actually have two resources (memory map of
>>> the process, and the file) but we only have one way to describe the
>>> permissions upon the two objects.  I'd think it makes a lot more sense if a
>>> new flag is added, when there's a need to differentiate the two.
>>>
>>> Consider if you see a bunch of qemu instances with:
>>>
>>>     -mem-path $RAM_FILE
>>>
>>> On the same host, which can be as weird as it could be to me.. At least
>>> '-mem-path' looks still like a way to exclusively own a ram file for an
>>> instance. I hesitate the new fallback can confuse people too, while that's
>>> so far not the major use case.
>>
>> Once I learned that this is not a MAP_SHARED mapping, I was extremely
>> confused. For example, vhost-user with "-mem-path" will absolutely not work
>> with "-mem-path", even though the documentation explicitly spells that out
>> (I still have to send a patch to fix that).
>>
>> I guess "-mem-path" was primarily only used to consume hugetlb. Even for
>> tmpfs it will already result in a double memory consumption, just like when
>> using -memory-backend-memfd,share=no.
>>
>> I guess deprecating it was the right decision.
> 
> Regardless of whether its deprecated or not, I think its fine to just
> say people need to use the more verbose memory-backend-file syntax
> if they want to use an unusual deployment configuration where there is
> a readonly backing file.
> 
>>> Nobody may really rely on any existing behavior of the failure, but
>>> changing existing behavior is just always not wanted.  The guideline here
>>> to me is: whether we want existing "-mem-path XXX" users to start using the
>>> fallback in general?  If it's "no", then maybe it implies a new flag is
>>> better?
>>
>> I think we have the following options (there might be more)
>>
>> 1) This patch.
>>
>> 2) New flag for memory-backend-file. We already have "readonly" and
>> "share=". I'm having a hard time coming up with a good name that really
>> describes the subtle difference.
>>
>> 3) Glue behavior to the QEMU machine
>>
>>
>> For 3), one option would be to always open a COW file readonly (as Thiner
>> originally proposed). We could leave "-mem-path" behavior alone and only
>> change memory-backend-file semantics. If the COW file does *not* exist yet,
>> we would refuse to create the file like patch 2+3 do. Therefore, no
>> ftruncate() errors, and fallocate() errors would always happen.
> 
> I'm for (2).

(2) in the form we discussed here is wrong because "readonly" already 
expresses "open this file readonly", not anything else.

-- 
Cheers,

David / dhildenb