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

[Peter Xu <peterx@redhat.com>]

On Tue, Jan 21, 2025 at 05:59:56PM +0000, “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.
> 
> Fixes: 4b870dc4d0c0 ("hostmem-file: add offset option")
> 
> Signed-off-by: William Roche <william.roche@oracle.com>
> ---
>  system/physmem.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/system/physmem.c b/system/physmem.c
> index c76503aea8..687ca94875 100644
> --- a/system/physmem.c
> +++ b/system/physmem.c
> @@ -3689,18 +3689,20 @@ 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);
> +                            start + rb->fd_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, start + rb->fd_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, start + rb->fd_offset, length,
> +                         ret);
>              goto err;
>  #endif
>          }

We do have plenty of fd_offset bugs then.. this makes sense to me. Nitpick
is we could use a var to cache the total offset.

> @@ -3748,17 +3750,17 @@ int ram_block_discard_guest_memfd_range(RAMBlock *rb, uint64_t start,
>  
>  #ifdef CONFIG_FALLOCATE_PUNCH_HOLE
>      ret = fallocate(rb->guest_memfd, FALLOC_FL_PUNCH_HOLE | FALLOC_FL_KEEP_SIZE,
> -                    start, length);
> +                    start + rb->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, start + rb->fd_offset, length, ret);
>      }
>  #else
>      ret = -ENOSYS;
>      error_report("%s: fallocate not available %s:%" PRIx64 " +%zx (%d)",
> -                 __func__, rb->idstr, start, length, ret);
> +                 __func__, rb->idstr, start + rb->fd_offset, length, ret);
>  #endif

IIUC the offset doesn't apply to gmemfd, see:

        new_block->guest_memfd = kvm_create_guest_memfd(new_block->max_length,
                                                        0, errp);

So my understanding is no matter how the host offset was specified, it
ignores it at least in the qemu gmemfd code to always offset from 0, which
makes sense to me, as gmemfd is anonymous anyway, and can be created more
than one for each VM, so I don't yet see why a gmemfd needs an offset indeed.

Thanks,

-- 
Peter Xu