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

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 <david@redhat.com>

--
Cheers,

David / dhildenb