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

[William Roche <william.roche@oracle.com>]

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.