Re: [PATCH] qemu: Propagate storage errors on migration

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On 19.01.2015 22:58, John Ferlan wrote:
> 
> 
> On 01/09/2015 07:22 AM, Michal Privoznik wrote:
>> There's one bug that got more visible with my patches that
>> automatically precreate storage on migration. The problem is, if
>> there's not enough disk space on destination, we successfully finish
>> the migration instead of failing.
>>
>> Firstly, we instruct qemu to copy the storage. And as it copies the
>> data, one write() will return -ENOSPC, eventually, to which qemu
>> reacts by emitting BLOCK_JOB_ERROR. The event is successfully ignored
>> by us. Then, since the block job has finished, BLOCK_JOB_COMPLETED
>> event is emitted too.
>>
>> Secondly, we are not checking for the block job completion as we
>> should. Currently, we do the check by issuing 'query-block-jobs' and
>> then looking in the command's output for the job we are interested in.
>> If course, we don't fail if the job is not there, in which case the
> 
> s/If/Of
> 
>> number of total bytes to be transferred and bytes already transferred,
>> well they equal both to zero. This is actually the line causing the
> 
> s/well they equal both to zero/will both be equal to zero.
> 
>> bug as it sees both numbers equal to each other and gives green light
>> to the actual migration.
>>
>> The fix consist of two parts:
>>
>> In the first, code handling BLOCK_JOB_ERROR is added. It's a monitor
>> event and we should have handler for that anyway.
>>
>> In the second part, the code doing drive mirroring is rewritten so it
>> waits for the events instead of querying on the monitor repeatedly.
>>
>> Signed-off-by: Michal Privoznik <mprivozn@xxxxxxxxxx>
>> ---
>>  src/conf/domain_conf.c       |  3 ++-
>>  src/conf/domain_conf.h       |  1 +
>>  src/qemu/qemu_migration.c    | 25 +++++++------------------
>>  src/qemu/qemu_monitor_json.c | 10 ++++++++++
>>  src/qemu/qemu_process.c      | 14 ++++++++++++++
>>  5 files changed, 34 insertions(+), 19 deletions(-)
>>
> 
> The concept seems reasonable - of course the code has change since you
> first sent this, so a rebase will be necessary...
> 
>> diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c
>> index d1a483a..751a9b5 100644
>> --- a/src/conf/domain_conf.c
>> +++ b/src/conf/domain_conf.c
>> @@ -787,7 +787,8 @@ VIR_ENUM_IMPL(virDomainDiskMirrorState, VIR_DOMAIN_DISK_MIRROR_STATE_LAST,
>>                "none",
>>                "yes",
>>                "abort",
>> -              "pivot")
>> +              "pivot",
>> +              "error")
>>  
>>  VIR_ENUM_IMPL(virDomainLoader,
>>                VIR_DOMAIN_LOADER_TYPE_LAST,
>> diff --git a/src/conf/domain_conf.h b/src/conf/domain_conf.h
>> index ac1f4f8..f18fa80 100644
>> --- a/src/conf/domain_conf.h
>> +++ b/src/conf/domain_conf.h
>> @@ -654,6 +654,7 @@ typedef enum {
>>      VIR_DOMAIN_DISK_MIRROR_STATE_READY, /* Job in second phase */
>>      VIR_DOMAIN_DISK_MIRROR_STATE_ABORT, /* Job aborted, waiting for event */
>>      VIR_DOMAIN_DISK_MIRROR_STATE_PIVOT, /* Job pivoted, waiting for event */
>> +    VIR_DOMAIN_DISK_MIRROR_STATE_ERROR, /* Job failed */
>>  
>>      VIR_DOMAIN_DISK_MIRROR_STATE_LAST
>>  } virDomainDiskMirrorState;
>> diff --git a/src/qemu/qemu_migration.c b/src/qemu/qemu_migration.c
>> index 77e0b35..e2309ea 100644
>> --- a/src/qemu/qemu_migration.c
>> +++ b/src/qemu/qemu_migration.c
>> @@ -1743,7 +1743,6 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
>>  
>>      for (i = 0; i < vm->def->ndisks; i++) {
>>          virDomainDiskDefPtr disk = vm->def->disks[i];
>> -        virDomainBlockJobInfo info;
>>  
>>          /* skip shared, RO and source-less disks */
>>          if (disk->src->shared || disk->src->readonly ||
>> @@ -1768,6 +1767,7 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
>>          if (mon_ret < 0)
>>              goto error;
>>  
>> +        disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_COPY;
> 
> I assume this is how we "ensure" we get into the else of
> qemuProcessHandleBlockJob. 

Exactly.

> Is setting this outside the context of this
> bugfix fixing a bug?  That is should it have been set all along? Curious
> mostly.

Well, it wouldn't hurt anything it it was set. On the other hand, it
wouldn't help anything either.

> 
>>          lastGood = i;
>>  
>>          /* wait for completion */
>> @@ -1775,38 +1775,27 @@ qemuMigrationDriveMirror(virQEMUDriverPtr driver,
>>              /* Poll every 500ms for progress & to allow cancellation */
>>              struct timespec ts = { .tv_sec = 0, .tv_nsec = 500 * 1000 * 1000ull };
>>  
>> -            memset(&info, 0, sizeof(info));
>> -
>> -            if (qemuDomainObjEnterMonitorAsync(driver, vm,
>> -                                               QEMU_ASYNC_JOB_MIGRATION_OUT) < 0)
>> -                goto error;
>>              if (priv->job.asyncAbort) {
>>                  /* explicitly do this *after* we entered the monitor,
>>                   * as this is a critical section so we are guaranteed
>>                   * priv->job.asyncAbort will not change */
> 
> So, since we're not entering the monitor any more - does the text need
> to change?

Oh, right.

> Also, is it 'safe' to look at priv->job.current->type?  Or
> even priv->job.asyncAbort since we won't have a monitor lock?

I think it is. If you look at qemuDomainAbortJob() impl, there's
qemuDomainObjAbortAsyncJob() called with only @vm lock held, outside of
Enter/ExitMonitor(). And the qemuDomainObjAbortAsyncJob() just just a
wrapper over direct access to priv->job.asyncAbort.

> 
> Considering the recent series on ExitMonitor error checks vis-a-vis the
> vm dying during some monitor interaction could this end up being an
> infinite loop?  That is would the necessary mirrorState be set in the
> case where the vm dies?

Interesting. I don't think that can happen. If vm dies,
disk->mirrorState should go to error state.

> 
>> -                qemuDomainObjExitMonitor(driver, vm);
>>                  priv->job.current->type = VIR_DOMAIN_JOB_CANCELLED;
>>                  virReportError(VIR_ERR_OPERATION_ABORTED, _("%s: %s"),
>>                                 qemuDomainAsyncJobTypeToString(priv->job.asyncJob),
>>                                 _("canceled by client"));
> 
> s/canceled/cancelled
> 
> (I see there's also another place in qemuMigrationUpdateJobStatus as
> well as libvirt-domain and virsh* modules, sigh)
> 
>>                  goto error;
>>              }
>> -            mon_ret = qemuMonitorBlockJobInfo(priv->mon, diskAlias, &info,
>> -                                              NULL);
>> -            qemuDomainObjExitMonitor(driver, vm);
>>  
>> -            if (mon_ret < 0)
>> -                goto error;
>> -
>> -            if (info.cur == info.end) {
>> +            if (disk->mirrorState == VIR_DOMAIN_DISK_MIRROR_STATE_READY) {
>>                  VIR_DEBUG("Drive mirroring of '%s' completed", diskAlias);
>>                  break;
>> +            } else if (disk->mirrorState == VIR_DOMAIN_DISK_MIRROR_STATE_ERROR) {
>> +                virReportError(VIR_ERR_OPERATION_ABORTED, _("%s: %s"),
>> +                               qemuDomainAsyncJobTypeToString(priv->job.asyncJob),
>> +                               _("canceled by destination"));
> 
> s/canceled/cancelled
> 
>> +                goto error;
>>              }
>>  
>> -            /* XXX Frankly speaking, we should listen to the events,
>> -             * instead of doing this. But this works for now and we
>> -             * are doing something similar in migration itself anyway */
>> -
>>              virObjectUnlock(vm);
>>  
>>              nanosleep(&ts, NULL);
>> diff --git a/src/qemu/qemu_monitor_json.c b/src/qemu/qemu_monitor_json.c
>> index e567aa7..5d6bbca 100644
>> --- a/src/qemu/qemu_monitor_json.c
>> +++ b/src/qemu/qemu_monitor_json.c
>> @@ -76,6 +76,7 @@ static void qemuMonitorJSONHandlePMWakeup(qemuMonitorPtr mon, virJSONValuePtr da
>>  static void qemuMonitorJSONHandlePMSuspend(qemuMonitorPtr mon, virJSONValuePtr data);
>>  static void qemuMonitorJSONHandleBlockJobCompleted(qemuMonitorPtr mon, virJSONValuePtr data);
>>  static void qemuMonitorJSONHandleBlockJobCanceled(qemuMonitorPtr mon, virJSONValuePtr data);
>> +static void qemuMonitorJSONHandleBlockJobError(qemuMonitorPtr mon, virJSONValuePtr data);
>>  static void qemuMonitorJSONHandleBlockJobReady(qemuMonitorPtr mon, virJSONValuePtr data);
>>  static void qemuMonitorJSONHandleBalloonChange(qemuMonitorPtr mon, virJSONValuePtr data);
>>  static void qemuMonitorJSONHandlePMSuspendDisk(qemuMonitorPtr mon, virJSONValuePtr data);
>> @@ -94,6 +95,7 @@ static qemuEventHandler eventHandlers[] = {
>>      { "BLOCK_IO_ERROR", qemuMonitorJSONHandleIOError, },
>>      { "BLOCK_JOB_CANCELLED", qemuMonitorJSONHandleBlockJobCanceled, },
>>      { "BLOCK_JOB_COMPLETED", qemuMonitorJSONHandleBlockJobCompleted, },
>> +    { "BLOCK_JOB_ERROR", qemuMonitorJSONHandleBlockJobError, },
>>      { "BLOCK_JOB_READY", qemuMonitorJSONHandleBlockJobReady, },
>>      { "DEVICE_DELETED", qemuMonitorJSONHandleDeviceDeleted, },
>>      { "DEVICE_TRAY_MOVED", qemuMonitorJSONHandleTrayChange, },
>> @@ -842,6 +844,14 @@ qemuMonitorJSONHandleBlockJobCanceled(qemuMonitorPtr mon,
>>  }
>>  
>>  static void
>> +qemuMonitorJSONHandleBlockJobError(qemuMonitorPtr mon,
>> +                                   virJSONValuePtr data)
>> +{
>> +    qemuMonitorJSONHandleBlockJobImpl(mon, data,
>> +                                      VIR_DOMAIN_BLOCK_JOB_FAILED);
>> +}
>> +
>> +static void
>>  qemuMonitorJSONHandleBlockJobReady(qemuMonitorPtr mon,
>>                                     virJSONValuePtr data)
>>  {
>> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c
>> index c18204b..b29ecf1 100644
>> --- a/src/qemu/qemu_process.c
>> +++ b/src/qemu/qemu_process.c
>> @@ -1110,6 +1110,20 @@ qemuProcessHandleBlockJob(qemuMonitorPtr mon ATTRIBUTE_UNUSED,
>>                  disk->mirrorJob = VIR_DOMAIN_BLOCK_JOB_TYPE_UNKNOWN;
>>                  save = true;
>>              }
>> +        } else if (disk->mirrorJob == VIR_DOMAIN_BLOCK_JOB_TYPE_COPY) {
>> +            switch ((virConnectDomainEventBlockJobStatus) status) {
>> +            case VIR_DOMAIN_BLOCK_JOB_READY:
>> +            case VIR_DOMAIN_BLOCK_JOB_COMPLETED:
> 
> "theoretically" if status == COMPLETED we shouldn't be here either since
> that the first "if" checks that...
> 
>> +                disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_READY;
>> +                break;
>> +            case VIR_DOMAIN_BLOCK_JOB_CANCELED:
>> +            case VIR_DOMAIN_BLOCK_JOB_FAILED:
>> +                disk->mirrorState = VIR_DOMAIN_DISK_MIRROR_STATE_ERROR;
>> +                break;
>> +            case VIR_DOMAIN_BLOCK_JOB_LAST:
>> +                VIR_DEBUG("should not get here");
>> +                break;
> 
> Not that we'd get here, but what effect is not setting mirrorState
> 
> Is a switch overkill in this instance?

I bet compiler will evaluate what's best and generate efficient code.

Michal

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]