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

[William Roche]

Thank you Peter and David for your feedback.


On 1/21/25 19:25, David Hildenbrand wrote:
> 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>
[...]
> > 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.

Ok.

> 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);

I also had this nit - as I should have used rb->fd_offset.

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

Ok I'll remove the ram_block_discard_guest_memfd_range() modifications 
but include a small comment indicating that we ignore fd_offset in this 
case.

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

I'm preparing a v2 that I'll send in a few hours.

William.