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

[William Roche]

On 1/22/25 09:01, David Hildenbrand wrote:
> On 21.01.25 23:54, “William Roche wrote:
> > From: William Roche <william.roche@oracle.com>
> > [...]
> > --- a/system/physmem.c
> > +++ b/system/physmem.c
> > @@ -3655,6 +3655,7 @@ int ram_block_discard_range(RAMBlock *rb, 
> > uint64_t start, size_t length)
> >           need_madvise = (rb->page_size == qemu_real_host_page_size());
> >           need_fallocate = rb->fd != -1;
> >           if (need_fallocate) {
> > +            uint64_t file_offset = start + rb->fd_offset;
>
> Taking another closer look ...
>
> Could likely be "off_t".

Right.


> >               /* For a file, this causes the area of the file to be 
> > zero'd
> >                * if read, and for hugetlbfs also causes it to be unmapped
> >                * so a userfault will trigger.
> > @@ -3689,18 +3690,18 @@ 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);
> > +                            file_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, file_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, file_offset, length, ret);
> >               goto err;
> >   #endif
>
> Thinking again, both error_report() should likely not have the offset 
> replaced?
>
> We are printing essentially the parameters to ram_block_discard_range() 
> -- range in the ramblock -- just like in the "Failed to discard range" 
> range.
>
> So maybe just leave it like is or print the file offset additionally? 
> (which might only make sense in the "Failed to fallocate" case).

I understand that the start value may be clearer to read than the global 
file_offset. So I'm slightly modifying the error message to show 
<start>+<fd_offset> (without space) which would usually be <start>+0

For example:
 ram_block_discard_range: Failed to fallocate ram-node1:f2db000+0 +1000 
(-5)

instead of:
 ram_block_discard_range: Failed to fallocate ram-node1:f2db000 +1000 (-5)

The length notation isn't changing, coming afterwards with a space -- so 
that it continues to match all the other similar range error messages in 
system/physmem.c.

I also align the "fallocate not available/file" message to show the 
extra +<fd_offset> after the <start>.


I'm sending a v3 version now.

William.