Re: [PATCH v3 14/26] memory: Add Error** argument to .log_global*() handlers

[Peter Xu <peterx@redhat.com>]

On Mon, Mar 04, 2024 at 01:28:32PM +0100, Cédric Le Goater wrote:
> @@ -2936,15 +2975,22 @@ void memory_global_dirty_log_start(unsigned int flags)
>      trace_global_dirty_changed(global_dirty_tracking);
>  
>      if (!old_flags) {
> -        MEMORY_LISTENER_CALL_GLOBAL(log_global_start, Forward);
> +        MEMORY_LISTENER_CALL_LOG_GLOBAL(log_global_start, Forward,
> +                                        &local_err);
> +        if (local_err) {
> +            error_report_err(local_err);
> +            return;

Returns here means global_dirty_tracking will keep the new value even if
it's not truly commited globally (in memory_region_transaction_commit()
later below).  I think it'll cause inconsistency: global_dirty_tracking
should reflect the global status of dirty tracking, and that should match
with the MR status cached in FlatViews (which is used in memory core to
reflect address space translations).

For some details on how that flag applied to each MR, feel free to have a
quick look in address_space_update_topology_pass() of the "else if (frold
&& frnew && flatrange_equal(frold, frnew))".

Here IIUC if to fully support a graceful failure (IIUC that is the goal for
VFIO.. and this op should be easily triggerable by the user), then we need
to do proper unwind on both:

  - Call proper log_global_stop() on those who has already been started
    successfully before the current failed log_global_start(), then,

  - Reset global_dirty_tracking to old_flags before return

We may want to make sure trace_global_dirty_changed() is only called when
all things succeeded.

I don't have a strong opinion on whether do we need similar error report
interfaces for _stop() and _log_sync().  I'd still suggest the same that we
drop them to make the patch simpler, but only add such error reports for
log_global_start().  If they never get triggered they're dead code anyway,
so I don't think "having errp for all APIs" is a must-to-have at least to me.

Thanks,

> +        }
>          memory_region_transaction_begin();
>          memory_region_update_pending = true;
>          memory_region_transaction_commit();
>      }
>  }

-- 
Peter Xu