On Wed, Mar 20, 2024 at 11:19 PM Peter Xu <pet...@redhat.com> 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 <sstabell...@kernel.org> > > Cc: Anthony Perard <anthony.per...@citrix.com> > > Cc: Paul Durrant <p...@xen.org> > > Cc: "Michael S. Tsirkin" <m...@redhat.com> > > Cc: Paolo Bonzini <pbonz...@redhat.com> > > Cc: David Hildenbrand <da...@redhat.com> > > Cc: Hyman Huang <yong.hu...@smartx.com> > > Signed-off-by: Cédric Le Goater <c...@redhat.com> > > 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 <pet...@redhat.com> > > Thanks, > > -- > Peter Xu > > -- Best regards