Re: [PATCH RFC 03/12] vfio/migration: Throttle vfio_save_block() on data size to read

[Avihai Horon <avihaih@nvidia.com>]

Hi Peter,

Thanks for sending this series.

On 3/20/2026 1:12 AM, Peter Xu wrote:
> External email: Use caution opening links or attachments
>
>
> During precopy phase, VFIO maintains two counters for init/dirty data
> tracking for query estimations.
>
> VFIO fetches data during precopy by reading from the VFIO fd, after
> fetching it'll deduct the read size.
>
> Here since the fd's size can dynamically change, I think it means VFIO may
> read more than what it "thought" were there for fetching.
>
> I highly suspect it's also relevant to a weird case in the function of
> vfio_update_estimated_pending_data(), where when VFIO reads 0 from the FD
> it will _reset_ the two counters, instead of asserting both of them being
> zeros, which looks pretty hackish.
>
> Just guarantee it from userspace level that VFIO won't read more than what
> it expects for now.

The VFIO_MIG_GET_PRECOPY_INFO ioctl returns an estimation of the data 
size currently available for reading. So, even if the ioctl returns X 
bytes, it may be that there are more than X bytes to read or less than X 
bytes.
The code was written in a flexible way to handle such discrepancies.

Because we are dealing with an estimation, I don't think we can assert 
that the counters are zero, and I don't think reading only up to the 
cached size gives us any benefit:
If the estimation is lower than actual available data, we are just 
deferring sending of the remaining data to a later stage.
If the estimation is higher than actual available data, we may still 
read() zero when the cached values are not zero.

I think we should keep the code as is.

Does that make sense?

Thanks.

>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   hw/vfio/migration.c | 18 ++++++++++++------
>   1 file changed, 12 insertions(+), 6 deletions(-)
>
> diff --git a/hw/vfio/migration.c b/hw/vfio/migration.c
> index 83327b6573..851ea783f3 100644
> --- a/hw/vfio/migration.c
> +++ b/hw/vfio/migration.c
> @@ -357,12 +357,18 @@ static int vfio_query_precopy_size(VFIOMigration *migration)
>   }
>
>   /* Returns the size of saved data on success and -errno on error */
> -static ssize_t vfio_save_block(QEMUFile *f, VFIOMigration *migration)
> +static ssize_t vfio_save_block(QEMUFile *f, VFIOMigration *migration,
> +                               bool precopy)
>   {
> -    ssize_t data_size;
> +    ssize_t data_size = migration->data_buffer_size;
> +
> +    if (precopy) {
> +        /* Limit the buffer size to make sure cached stats don't overflow */
> +        data_size = MIN(data_size, migration->precopy_init_size +
> +                        migration->precopy_dirty_size);
> +    }
>
> -    data_size = read(migration->data_fd, migration->data_buffer,
> -                     migration->data_buffer_size);
> +    data_size = read(migration->data_fd, migration->data_buffer, data_size);
>       if (data_size < 0) {
>           /*
>            * Pre-copy emptied all the device state for now. For more information,
> @@ -623,7 +629,7 @@ static int vfio_save_iterate(QEMUFile *f, void *opaque)
>           migration->event_save_iterate_started = true;
>       }
>
> -    data_size = vfio_save_block(f, migration);
> +    data_size = vfio_save_block(f, migration, true);
>       if (data_size < 0) {
>           return data_size;
>       }
> @@ -667,7 +673,7 @@ static int vfio_save_complete_precopy(QEMUFile *f, void *opaque)
>       }
>
>       do {
> -        data_size = vfio_save_block(f, vbasedev->migration);
> +        data_size = vfio_save_block(f, vbasedev->migration, false);
>           if (data_size < 0) {
>               return data_size;
>           }
> --
> 2.50.1
>