On 18/02/2015 12:57, Pavel Dovgalyuk wrote: > > + > + if (replay_file) { > + if (replay_mode == REPLAY_MODE_PLAY) { > + replay_mutex_lock(); > + if (!skip_async_events(EVENT_CHECKPOINT + checkpoint)) { > + if (replay_data_kind == EVENT_ASYNC) { > + replay_read_events(checkpoint); > + replay_fetch_data_kind(); > + res = replay_data_kind != EVENT_ASYNC; > + replay_mutex_unlock(); > + return res; > + } > + replay_mutex_unlock(); > + return res; > + } > + replay_has_unread_data = 0; > + replay_read_events(checkpoint); > + replay_fetch_data_kind(); > + res = replay_data_kind != EVENT_ASYNC; > + replay_mutex_unlock(); > + return res; > + } else if (replay_mode == REPLAY_MODE_RECORD) { > + replay_mutex_lock(); > + replay_put_event(EVENT_CHECKPOINT + checkpoint); > + replay_save_events(checkpoint); > + replay_mutex_unlock(); > + } > + } > + > + return true;
This is cleaner: if (!replay_file) { return true; } replay_mutex_lock(); if (replay_mode == REPLAY_MODE_PLAY) { if (skip_async_events(EVENT_CHECKPOINT + checkpoint)) { replay_has_unread_data = 0; } else if (replay_data_kind != EVENT_ASYNC) { res = false; goto out; } replay_read_events(checkpoint); replay_fetch_data_kind(); res = replay_data_kind != EVENT_ASYNC; } else if (replay_mode == REPLAY_MODE_RECORD) { replay_put_event(EVENT_CHECKPOINT + checkpoint); replay_save_events(checkpoint); res = true; } out: replay_mutex_unlock(); return res; I'm not sure however of the meaning of "res = replay_data_kind != EVENT_ASYNC;". Why not just return true since skip_async_events has returned true? Can you add a comment? Paolo > + if (!replay_checkpoint(CHECKPOINT_BDRV_DRAIN)) { > + /* Do not wait anymore, we stopped at some place in > + the middle of execution during replay */ > + return; > + } > busy = false; > > QTAILQ_FOREACH(bs, &bdrv_states, device_list) { > @@ -2004,6 +2010,12 @@ void bdrv_drain_all(void) > aio_context_release(aio_context); > } > } > + if (replay_mode == REPLAY_MODE_PLAY) { > + /* Skip checkpoints from the log */ > + while (replay_checkpoint(CHECKPOINT_BDRV_DRAIN)) { > + /* Nothing */ > + } > + } > } Can you explain this? Otherwise the patch looks great.