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

[ThinerLogoer]

At 2023-08-12 03:00:54, "David Hildenbrand" <david@redhat.com> wrote:
>On 11.08.23 07:49, ThinerLogoer wrote:
>> At 2023-08-11 05:24:43, "Peter Xu" <peterx@redhat.com> wrote:
>>> On Fri, Aug 11, 2023 at 01:06:12AM +0800, ThinerLogoer wrote:
>>>>> 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
>>>>>
>>>>
>>>> 4) '-deny-private-discard' argv, or environment variable, or both
>>>
>>> I'd personally vote for (2).  How about "fdperm"?  To describe when we want
>>> to use different rw permissions on the file (besides the access permission
>>> of the memory we already provided with "readonly"=XXX).  IIUC the only sane
>>> value will be ro/rw/default, where "default" should just use the same rw
>>> permission as the memory ("readonly"=XXX).
>>>
>>> Would that be relatively clean and also work in this use case?
>>>
>>> (the other thing I'd wish we don't have that fallback is, as long as we
>>> have any of that "fallback" we'll need to be compatible with it since
>>> then, and for ever...)
>> 
>> If it must be (2), I would vote (2) + (4), with (4) adjust the default 
>> behavior of said `fdperm`.
>> Mainly because (private+discard) is itself not a good practice and (4) serves
>> as a good tool to help catch existing (private+discard) problems.
>
>Instead of fdperm, maybe we could find a better name.
>
>The man page of "open" says: The argument flags must include one of the 
>following access modes: O_RDONLY, O_WRONLY, or O_RDWR.  These request 
>opening the file read-only, write-only, or read/write, respectively.
>
>So maybe something a bit more mouthful like "file-access-mode" would be 
>better.

Maybe "fd-mode"or "open-mode" to make it shorter and also non ambiguous.

"open-access-mode" can also be considered.

Btw my chatgpt agent suggested 10 names to me, in case you feel hard to
decide a name:

    access-mode
    file-mode
    open-mode
    file-permission
    file-access-mode (aha!)
    file-access-permission
    file-io-access-mode
    file-open-permission
    file-operation-mode
    file-read-write-mode


>
>We could have the options
>*readwrite
>*readonly
>*auto
>
>Whereby "auto" is the default.

I like the "auto" here.

>
>Specifying:
>
>* "readonly=true,file-access-mode=readwrite" would fail.
>* "shared=true,readonly=false,file-access-mode=readonly" would fail.
>* "shared=false,readonly=false,file-access-mode=readonly" would work.

Yeah, sanitizing the arguments here is wise.

>
>In theory, we could adjust the mode of "auto" with compat machines. But 
>maybe it would be sufficient to do something like the following
>
>1) shared=true,readonly=false
>       -> readwrite
>2) shared=true,readonly=true
>       > readonly
>2) shared=false,readonly=true
>       -> readonly
>3) shared=false,readonly=true
>       hugetlb? -> readwrite
>       otherwise -> readonly

Looks like there is some typo here. I assume it was:

1) shared=true,readonly=false
        -> readwrite
2) shared=true,readonly=true
        -> readonly
3) shared=false,readonly=true
        -> readonly
4) shared=false,readonly=false
        hugetlb? -> readwrite
        otherwise -> readonly


>Reason being the traditional usage of hugetlb with MAP_PRIVATE where I 
>can see possible users with postcopy. Further, VM templating with 
>hugetlb might not be that common ... users could still modify the 
>behavior if they really have to.
>
>That would make your use case work automatically with file-backed memory.

This seems a good idea. I am good with the solution you proposed
here as well.


--

Regards,

logoerthiner