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

[Cédric Le Goater]

On 3/5/24 08:57, Peter Xu wrote:
> 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).

You are right. FlatRange::dirty_log_mask could be out sync with
global_dirty_tracking.

> 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,

Yes. This needs more work to restore the initial state. The current
proposal is relying on save_cleanup() to restore the previous state.
This is not enough.

>    - 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.

That should be done after the loop on listeners even today.

> I don't have a strong opinion on whether do we need similar error report
> interfaces for _stop() and _log_sync(). 

Yes. Let's focus on log_global_start(). The other changes are not necessary
for VFIO.

> 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.

I am fine with that.

Thanks,

C.