* [PATCH 0/1] fallocate missing fd_offset
@ 2025-01-21 17:59 “William Roche
2025-01-21 17:59 ` [PATCH 1/1] system/physmem: take into account fd_offset for file fallocate “William Roche
0 siblings, 1 reply; 6+ messages in thread
From: “William Roche @ 2025-01-21 17:59 UTC (permalink / raw)
To: david, qemu-devel, peterx, pbonzini, philmd; +Cc: william.roche
From: William Roche <william.roche@oracle.com>
Working on the poisoned memory recovery mechanisms with David
Hildenbrand, it appeared that the file hole punching done with
the memory discard functions are missing the file offset value
fd_offset to correctly modify the right file location.
I'm not sure that guest_memfd would use a non zero fd_offset, but I'm
also modifying the ram_block_discard_guest_memfd_range() function to
include this fallocate use case too.
The fix is checkpatch.pl clean
make check runs fine on both ARM and x86
William Roche (1):
system/physmem: take into account fd_offset for file fallocate
system/physmem.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
--
2.43.5
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 1/1] system/physmem: take into account fd_offset for file fallocate
2025-01-21 17:59 [PATCH 0/1] fallocate missing fd_offset “William Roche
@ 2025-01-21 17:59 ` “William Roche
2025-01-21 18:17 ` Peter Xu
0 siblings, 1 reply; 6+ messages in thread
From: “William Roche @ 2025-01-21 17:59 UTC (permalink / raw)
To: david, qemu-devel, peterx, pbonzini, philmd; +Cc: william.roche
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
}
@@ -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
return ret;
--
2.43.5
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] system/physmem: take into account fd_offset for file fallocate
2025-01-21 17:59 ` [PATCH 1/1] system/physmem: take into account fd_offset for file fallocate “William Roche
@ 2025-01-21 18:17 ` Peter Xu
2025-01-21 18:25 ` David Hildenbrand
0 siblings, 1 reply; 6+ messages in thread
From: Peter Xu @ 2025-01-21 18:17 UTC (permalink / raw)
To: “William Roche; +Cc: david, qemu-devel, pbonzini, philmd
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.
> @@ -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.
Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] system/physmem: take into account fd_offset for file fallocate
2025-01-21 18:17 ` Peter Xu
@ 2025-01-21 18:25 ` David Hildenbrand
2025-01-21 18:38 ` William Roche
0 siblings, 1 reply; 6+ messages in thread
From: David Hildenbrand @ 2025-01-21 18:25 UTC (permalink / raw)
To: Peter Xu, “William Roche; +Cc: qemu-devel, pbonzini, philmd
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] system/physmem: take into account fd_offset for file fallocate
2025-01-21 18:25 ` David Hildenbrand
@ 2025-01-21 18:38 ` William Roche
2025-01-21 18:42 ` David Hildenbrand
0 siblings, 1 reply; 6+ messages in thread
From: William Roche @ 2025-01-21 18:38 UTC (permalink / raw)
To: David Hildenbrand, Peter Xu; +Cc: qemu-devel, pbonzini, philmd
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.
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/1] system/physmem: take into account fd_offset for file fallocate
2025-01-21 18:38 ` William Roche
@ 2025-01-21 18:42 ` David Hildenbrand
0 siblings, 0 replies; 6+ messages in thread
From: David Hildenbrand @ 2025-01-21 18:42 UTC (permalink / raw)
To: William Roche, Peter Xu; +Cc: qemu-devel, pbonzini, philmd
On 21.01.25 19:38, William Roche wrote:
> 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.
Ah, sneaky :)
--
Cheers,
David / dhildenb
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2025-01-21 18:43 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-01-21 17:59 [PATCH 0/1] fallocate missing fd_offset “William Roche
2025-01-21 17:59 ` [PATCH 1/1] system/physmem: take into account fd_offset for file fallocate “William Roche
2025-01-21 18:17 ` Peter Xu
2025-01-21 18:25 ` David Hildenbrand
2025-01-21 18:38 ` William Roche
2025-01-21 18:42 ` David Hildenbrand
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).