* [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).