Re: [Qemu-devel] [PATCH 11/21] backup: move r/w error handling code to r/w functions

[Stefan Hajnoczi]

On Fri, Dec 23, 2016 at 05:28:54PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> It simplifies code: we do not need error_is_read parameter and
> retrying in backup_loop. Also, retrying for read and write are separate,
> so we will not reread if write failed after successfull read.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy 
> ---
>  block/backup.c | 141 
> ++---
>  1 file changed, 74 insertions(+), 67 deletions(-)

Reviewed-by: Stefan Hajnoczi 


signature.asc
Description: PGP signature

[Qemu-devel] [PATCH 11/21] backup: move r/w error handling code to r/w functions

[Vladimir Sementsov-Ogievskiy]

It simplifies code: we do not need error_is_read parameter and
retrying in backup_loop. Also, retrying for read and write are separate,
so we will not reread if write failed after successfull read.

Signed-off-by: Vladimir Sementsov-Ogievskiy 
---
 block/backup.c | 141 ++---
 1 file changed, 74 insertions(+), 67 deletions(-)

diff --git a/block/backup.c b/block/backup.c
index 5c31607..442e6da 100644
--- a/block/backup.c
+++ b/block/backup.c
@@ -95,18 +95,65 @@ static void cow_request_end(CowRequest *req)
 qemu_co_queue_restart_all(>wait_queue);
 }
 
+static BlockErrorAction backup_error_action(BackupBlockJob *job,
+bool read, int error)
+{
+if (read) {
+return block_job_error_action(>common, job->on_source_error,
+  true, error);
+} else {
+return block_job_error_action(>common, job->on_target_error,
+  false, error);
+}
+}
+
+static bool coroutine_fn yield_and_check(BackupBlockJob *job)
+{
+if (block_job_is_cancelled(>common)) {
+return true;
+}
+
+/* we need to yield so that bdrv_drain_all() returns.
+ * (without, VM does not reboot)
+ */
+if (job->common.speed) {
+uint64_t delay_ns = ratelimit_calculate_delay(>limit,
+  job->sectors_read);
+job->sectors_read = 0;
+block_job_sleep_ns(>common, QEMU_CLOCK_REALTIME, delay_ns);
+} else {
+block_job_sleep_ns(>common, QEMU_CLOCK_REALTIME, 0);
+}
+
+if (block_job_is_cancelled(>common)) {
+return true;
+}
+
+return false;
+}
+
 static int coroutine_fn backup_do_read(BackupBlockJob *job,
int64_t offset, unsigned int bytes,
QEMUIOVector *qiov,
bool is_write_notifier)
 {
-int ret = blk_co_preadv(job->common.blk, offset, bytes, qiov,
-is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0);
+int ret;
+BdrvRequestFlags flags = is_write_notifier ? BDRV_REQ_NO_SERIALISING : 0;
+
+retry:
+ret = blk_co_preadv(job->common.blk, offset, bytes, qiov, flags);
 if (ret < 0) {
 trace_backup_do_read_fail(job, offset, bytes, ret);
+
+BlockErrorAction action = backup_error_action(job, true, -ret);
+if (action != BLOCK_ERROR_ACTION_REPORT && !yield_and_check(job)) {
+goto retry;
+}
+
+return ret;
 }
 
-return ret;
+return 0;
 }
 
 static int coroutine_fn backup_do_write(BackupBlockJob *job,
@@ -114,9 +161,13 @@ static int coroutine_fn backup_do_write(BackupBlockJob 
*job,
 QEMUIOVector *qiov)
 {
 int ret;
+bool zeroes;
+
 assert(qiov->niov == 1);
+zeroes = buffer_is_zero(qiov->iov->iov_base, qiov->iov->iov_len);
 
-if (buffer_is_zero(qiov->iov->iov_base, qiov->iov->iov_len)) {
+retry:
+if (zeroes) {
 ret = blk_co_pwrite_zeroes(job->target, offset, bytes,
BDRV_REQ_MAY_UNMAP);
 } else {
@@ -125,14 +176,20 @@ static int coroutine_fn backup_do_write(BackupBlockJob 
*job,
 }
 if (ret < 0) {
 trace_backup_do_write_fail(job, offset, bytes, ret);
+
+BlockErrorAction action = backup_error_action(job, false, -ret);
+if (action != BLOCK_ERROR_ACTION_REPORT && !yield_and_check(job)) {
+goto retry;
+}
+
+return ret;
 }
 
-return ret;
+return 0;
 }
 
 static int coroutine_fn backup_copy_cluster(BackupBlockJob *job,
 int64_t cluster,
-bool *error_is_read,
 bool is_write_notifier,
 void *bounce_buffer)
 {
@@ -156,17 +213,11 @@ static int coroutine_fn 
backup_copy_cluster(BackupBlockJob *job,
 ret = backup_do_read(job, offset, bounce_qiov.size, _qiov,
  is_write_notifier);
 if (ret < 0) {
-if (error_is_read) {
-*error_is_read = true;
-}
 return ret;
 }
 
 ret = backup_do_write(job, offset, bounce_qiov.size, _qiov);
 if (ret < 0) {
-if (error_is_read) {
-*error_is_read = false;
-}
 return ret;
 }
 
@@ -181,7 +232,6 @@ static int coroutine_fn backup_copy_cluster(BackupBlockJob 
*job,
 
 static int coroutine_fn backup_do_cow(BackupBlockJob *job,
   int64_t sector_num, int nb_sectors,
-  bool *error_is_read,
   bool is_write_notifier)
 {
 BlockBackend *blk = job->common.blk;
@@ -212,8 +262,7 @@ static int coroutine_fn