| Message ID | 20250121175956.3030149-2-william.roche@oracle.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | fallocate missing fd_offset | expand |
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,
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>
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.
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 :)
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;