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

[David Hildenbrand <david@redhat.com>]

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