Re: [PATCH v2 06/20] qemu_file: total_transferred is not used anymore

[Peter Xu]

On Tue, May 30, 2023 at 08:39:27PM +0200, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/qemu-file.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/migration/qemu-file.c b/migration/qemu-file.c
> index eb0497e532..6b6deea19b 100644
> --- a/migration/qemu-file.c
> +++ b/migration/qemu-file.c
> @@ -41,9 +41,6 @@ struct QEMUFile {
>      QIOChannel *ioc;
>      bool is_writable;
>  
> -    /* The sum of bytes transferred on the wire */
> -    uint64_t total_transferred;
> -
>      int buf_index;
>      int buf_size; /* 0 when writing */
>      uint8_t buf[IO_BUF_SIZE];
> @@ -287,7 +284,6 @@ void qemu_fflush(QEMUFile *f)
>              qemu_file_set_error_obj(f, -EIO, local_error);
>          } else {
>              uint64_t size = iov_size(f->iov, f->iovcnt);
> -            f->total_transferred += size;

I think this patch is another example why I think sometimes the way patch
is split are pretty much adding more complexity on review...

Here we removed a variable operation but it seems all fine if it's not used
anywhere.  But it also means current code base (before this patch applied)
doesn't make sense already because it contains this useless addition.  So
IMHO it means some previous patch does it just wrong.

I think it means it's caused by a wrong split of patches, then each patch
stops to make much sense as a standalone one.

I can go back and try to find whatever patch on the list that will explain
this.  But it'll also go into git log.  Anyone reads this later will be
confused once again.  Even harder for them to figure out what happened.

Do you think we could reorganize the patches so each of a single patch
explains itself?

The other thing is about priority of patches - I still have ~80 patches
pending reviews on migration only.. Would you think it makes sense we pick
up important ones first and merge them with higher priority?

What I have in mind are:

  - The regression you mentioned on qemu_fflush() when ram save (I hope I
    understand the problem right, though...).

  - The slowness of migration-test.  I'm not sure whether you have anything
    better than either Dan's approach or the switchover-hold flags, I just
    think that seems more important to resolve for us upstream.  We can
    merge Dan's or mine, you can also propose something else, but IMHO that
    seems to be a higher priority?

And whatever else I haven't noticed.  I'll continue reading but I'm sure
you know the best on this..  so I'd really rely on you.

What do you think?

Thanks,

-- 
Peter Xu