Re: [PATCH v2 1/1] system/physmem: take into account fd_offset for file fallocate

[David Hildenbrand <david@redhat.com>]

On 21.01.25 23:54, “William Roche wrote:
> From: William Roche <william.roche@oracle.com>
> 
> Punching a hole in a file with fallocate needs to take into account the
> fd_offset value for a correct file location.
> But guest_memfd internal use doesn't currently consider fd_offset.
> 
> Fixes: 4b870dc4d0c0 ("hostmem-file: add offset option")
> 
> Signed-off-by: William Roche <william.roche@oracle.com>
> ---
>   system/physmem.c | 8 +++++---
>   1 file changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/system/physmem.c b/system/physmem.c
> index c76503aea8..7e4da79311 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -3655,6 +3655,7 @@ int ram_block_discard_range(RAMBlock *rb, uint64_t start, size_t length)
>           need_madvise = (rb->page_size == qemu_real_host_page_size());
>           need_fallocate = rb->fd != -1;
>           if (need_fallocate) {
> +            uint64_t file_offset = start + rb->fd_offset;

Taking another closer look ...

Could likely be "off_t".

>               /* For a file, this causes the area of the file to be zero'd
>                * if read, and for hugetlbfs also causes it to be unmapped
>                * so a userfault will trigger.
> @@ -3689,18 +3690,18 @@ int ram_block_discard_range(RAMBlock *rb, uint64_t start, size_t length)
>               }
>   
>               ret = fallocate(rb->fd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
> -                            start, length);
> +                            file_offset, length);
>               if (ret) {
>                   ret = -errno;
>                   error_report("%s: Failed to fallocate %s:%" PRIx64 " +%zx (%d)",
> -                             __func__, rb->idstr, start, length, ret);
> +                             __func__, rb->idstr, file_offset, length, ret);
>                   goto err;
>               }
>   #else
>               ret = -ENOSYS;
>               error_report("%s: fallocate not available/file"
>                            "%s:%" PRIx64 " +%zx (%d)",
> -                         __func__, rb->idstr, start, length, ret);
> +                         __func__, rb->idstr, file_offset, length, ret);
>               goto err;
>   #endif

Thinking again, both error_report() should likely not have the offset 
replaced?

We are printing essentially the parameters to ram_block_discard_range() 
-- range in the ramblock -- just like in the "Failed to discard range" 
range.

So maybe just leave it like is or print the file offset additionally? 
(which might only make sense in the "Failed to fallocate" case).


Thanks!


-- 
Cheers,

David / dhildenb