Re: [PATCH for-9.1 v5 11/14] memory: Add Error** argument to the global_dirty_log routines

[Yong Huang <yong.huang@xxxxxxxxxx>]

On Wed, Mar 20, 2024 at 11:19 PM Peter Xu <peterx@xxxxxxxxxx> wrote:

On Wed, Mar 20, 2024 at 07:49:07AM +0100, Cédric Le Goater wrote:
> Now that the log_global*() handlers take an Error** parameter and
> return a bool, do the same for memory_global_dirty_log_start() and
> memory_global_dirty_log_stop(). The error is reported in the callers
> for now and it will be propagated in the call stack in the next
> changes.
>
> To be noted a functional change in ram_init_bitmaps(), if the dirty
> pages logger fails to start, there is no need to synchronize the dirty
> pages bitmaps. colo_incoming_start_dirty_log() could be modified in a
> similar way.
>
> Cc: Stefano Stabellini <sstabellini@xxxxxxxxxx>
> Cc: Anthony Perard <anthony.perard@xxxxxxxxxx>
> Cc: Paul Durrant <paul@xxxxxxx>
> Cc: "Michael S. Tsirkin" <mst@xxxxxxxxxx>
> Cc: Paolo Bonzini <pbonzini@xxxxxxxxxx>
> Cc: David Hildenbrand <david@xxxxxxxxxx>
> Cc: Hyman Huang <yong.huang@xxxxxxxxxx>
> Signed-off-by: Cédric Le Goater <clg@xxxxxxxxxx>

Just to mention that for the rest users (dirtylimit/dirtyrate/colo) the
code still just keeps going even if start logging failed even after this
series applied as a whole.  Migration framework handles the failure
gracefully, not yet the rest.

That might be an issue for some, e.g., ideally we should be able to fail a
calc-dirty-rate request, but it's not supported so far.  Adding that could
add quite some burden to this series, so maybe that's fine to be done

Indeed, for the GLOBAL_DIRTY_DIRTY_RATE and GLOBAL_DIRTY_LIMIT

dirty tracking, they should handle the failure path of logging start.

The work may be done once the current patchset is merged. 

 

later.  After all, having a VFIO device (that can fail a start_log()), plus
any of those features should be even rarer, I think?

It seems at least memory_global_dirty_log_sync() can be called even without
start logging, so I expect nothing should crash immediately. I spot one in
colo_incoming_start_dirty_log() already of such use.  My wild guess is it
relies on all log_sync*() hooks to cope with it, e.g. KVM ioctl() should
fail with -ENENT on most archs I think when it sees dirty log not ever
started.

For those bits, I'll wait and see whether Yong or Hailiang (cced) has any
comments. From generic migration/memory side, nothing I see wrong:

Acked-by: Peter Xu <peterx@xxxxxxxxxx>

Thanks,

--
Peter Xu

--

Best regards