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

[David Hildenbrand]

On 21.01.25 19:17, Peter Xu wrote:

On Tue, Jan 21, 2025 at 05:59:56PM +0000, “William Roche wrote:

From: William Roche <william.ro...@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.ro...@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.

Agreed that makes sense.

@@ -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.

Right.

Reviewed-by: David Hildenbrand <da...@redhat.com>

--
Cheers,

David / dhildenb