Re: [PATCH v1 1/4] softmmu/physmem: Warn with ram_block_discard_range() on MAP_PRIVATE file mapping

[Peter Xu]

On Tue, Jun 20, 2023 at 03:03:51PM +0200, David Hildenbrand wrote:
> ram_block_discard_range() cannot possibly do the right thing in
> MAP_PRIVATE file mappings in the general case.
> 
> To achieve the documented semantics, we also have to punch a hole into
> the file, possibly messing with other MAP_PRIVATE/MAP_SHARED mappings
> of such a file.
> 
> For example, using VM templating -- see commit b17fbbe55cba ("migration:
> allow private destination ram with x-ignore-shared") -- in combination with
> any mechanism that relies on discarding of RAM is problematic. This
> includes:
> * Postcopy live migration
> * virtio-balloon inflation/deflation or free-page-reporting
> * virtio-mem
> 
> So at least warn that there is something possibly dangerous is going on
> when using ram_block_discard_range() in these cases.

The issue is probably valid.

One thing I worry is when the user (or, qemu instance) exclusively owns the
file, just forgot to attach share=on, where it used to work perfectly then
it'll show this warning.  But I agree maybe it's good to remind them just
to attach the share=on.

For real private mem users, the warning can of real help, one should
probably leverage things like file snapshot provided by modern file
systems, so each VM should just have its own snapshot ram file to use then
map it share=on I suppose.

For the long term, maybe we should simply support private mem here simply
by a MADV_DONTNEED.  I assume that's the right semantics for postcopy (just
need to support MINOR faults, though; MISSING faults definitely will stop
working.. but for all the rest framework shouldn't need much change), and I
hope that's also the semantics that balloon/virtio-mem wants here.  Not
sure whether/when that's strongly needed, assuming the corner case above
can still be work arounded properly by other means.

For now, a warning looks all sane.

> 
> Signed-off-by: David Hildenbrand <david@redhat.com>

Acked-by: Peter Xu <peterx@redhat.com>

> ---
>  softmmu/physmem.c | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
> 
> diff --git a/softmmu/physmem.c b/softmmu/physmem.c
> index 6bdd944fe8..27c7219c82 100644
> --- a/softmmu/physmem.c
> +++ b/softmmu/physmem.c
> @@ -3451,6 +3451,24 @@ int ram_block_discard_range(RAMBlock *rb, uint64_t 
> start, size_t length)
>               * so a userfault will trigger.
>               */
>  #ifdef CONFIG_FALLOCATE_PUNCH_HOLE
> +            /*
> +             * We'll discard data from the actual file, even though we only
> +             * have a MAP_PRIVATE mapping, possibly messing with other
> +             * MAP_PRIVATE/MAP_SHARED mappings. There is no easy way to
> +             * change that behavior whithout violating the promised
> +             * semantics of ram_block_discard_range().
> +             *
> +             * Only warn, because it work as long as nobody else uses that
> +             * file.
> +             */
> +            if (!qemu_ram_is_shared(rb)) {
> +                warn_report_once("ram_block_discard_range: Discarding RAM"
> +                                 " in private file mappings is possibly"
> +                                 " dangerous, because it will modify the"
> +                                 " underlying file and will affect other"
> +                                 " users of the file");
> +            }
> +
>              ret = fallocate(rb->fd, FALLOC_FL_PUNCH_HOLE | 
> FALLOC_FL_KEEP_SIZE,
>                              start, length);
>              if (ret) {
> -- 
> 2.40.1
> 

-- 
Peter Xu