| Message ID | 1461763231-17598-4-git-send-email-kwolf@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 27.04.2016 15:20, Kevin Wolf wrote: > When block job errors were introduced, we assigned the iostatus of the > target BDS "just in case". The field has never been accessible for the > user because the target isn't listed in query-block. > > Before we can allow the user to have a second BlockBackend on the > target, we need to clean this up. If anything, we would want to set the > iostatus for the internal BB of the job (which we can always do later), > but certainly not for a separate BB which the job doesn't even use. > > As a nice side effect, this gets us rid of another bs->blk use. > > Signed-off-by: Kevin Wolf <kwolf@redhat.com> > --- > block/backup.c | 8 ++++---- > block/mirror.c | 8 ++++---- > block/stream.c | 3 +-- > blockjob.c | 6 +----- > include/block/blockjob.h | 4 +--- > 5 files changed, 11 insertions(+), 18 deletions(-) Reviewed-by: Max Reitz <mreitz@redhat.com>
On 06.05.2016 14:01, Max Reitz wrote: > On 27.04.2016 15:20, Kevin Wolf wrote: >> When block job errors were introduced, we assigned the iostatus of the >> target BDS "just in case". The field has never been accessible for the >> user because the target isn't listed in query-block. >> >> Before we can allow the user to have a second BlockBackend on the >> target, we need to clean this up. If anything, we would want to set the >> iostatus for the internal BB of the job (which we can always do later), >> but certainly not for a separate BB which the job doesn't even use. >> >> As a nice side effect, this gets us rid of another bs->blk use. >> >> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >> --- >> block/backup.c | 8 ++++---- >> block/mirror.c | 8 ++++---- >> block/stream.c | 3 +-- >> blockjob.c | 6 +----- >> include/block/blockjob.h | 4 +--- >> 5 files changed, 11 insertions(+), 18 deletions(-) > > Reviewed-by: Max Reitz <mreitz@redhat.com> Maybe I need to take that back. Using e.g. blockdev-backup, you can indeed see the target in query-block. e.g.: (echo "{'execute':'qmp_capabilities'} {'execute':'blockdev-backup', 'arguments':{'device':'src','target':'dst', 'sync':'full','on-target-error':'enospc'}}"; sleep 1; echo "{'execute':'query-block'}") | \ x86_64-softmmu/qemu-system-x86_64 \ -drive if=none,id=src,file=test.qcow2 \ -drive if=none,id=dst,file=mnt/out.qcow2 \ -qmp-pretty stdio -nodefaults Where mnt is a file system and test.qcow2 is large enough such that an ENOSPC will occur. Before this patch, you can see "io-status": "nospace" in query-block for dst. After it, it says "io-status": "ok", and after the next it doesn't say anything about the status at all anymore. Max
Am 06.05.2016 um 14:32 hat Max Reitz geschrieben: > On 06.05.2016 14:01, Max Reitz wrote: > > On 27.04.2016 15:20, Kevin Wolf wrote: > >> When block job errors were introduced, we assigned the iostatus of the > >> target BDS "just in case". The field has never been accessible for the > >> user because the target isn't listed in query-block. > >> > >> Before we can allow the user to have a second BlockBackend on the > >> target, we need to clean this up. If anything, we would want to set the > >> iostatus for the internal BB of the job (which we can always do later), > >> but certainly not for a separate BB which the job doesn't even use. > >> > >> As a nice side effect, this gets us rid of another bs->blk use. > >> > >> Signed-off-by: Kevin Wolf <kwolf@redhat.com> > >> --- > >> block/backup.c | 8 ++++---- > >> block/mirror.c | 8 ++++---- > >> block/stream.c | 3 +-- > >> blockjob.c | 6 +----- > >> include/block/blockjob.h | 4 +--- > >> 5 files changed, 11 insertions(+), 18 deletions(-) > > > > Reviewed-by: Max Reitz <mreitz@redhat.com> > > Maybe I need to take that back. Using e.g. blockdev-backup, you can > indeed see the target in query-block. > > e.g.: > > (echo "{'execute':'qmp_capabilities'} > {'execute':'blockdev-backup', > 'arguments':{'device':'src','target':'dst', > 'sync':'full','on-target-error':'enospc'}}"; > sleep 1; > echo "{'execute':'query-block'}") | \ > x86_64-softmmu/qemu-system-x86_64 \ > -drive if=none,id=src,file=test.qcow2 \ > -drive if=none,id=dst,file=mnt/out.qcow2 \ > -qmp-pretty stdio -nodefaults > > Where mnt is a file system and test.qcow2 is large enough such that an > ENOSPC will occur. Hm... Let's pretend we didn't notice. In fact, I don't think this behaviour is documented and it also doesn't make a lot of sense. I would be surprised if libvirt made use of it, as there is still the job iostatus which works in drive-* cases, too. (Eric?) > Before this patch, you can see "io-status": "nospace" in query-block for > dst. After it, it says "io-status": "ok", and after the next it doesn't > say anything about the status at all anymore. Simply letting the field disappear sounds like a nice solution. Kevin
On 06.05.2016 15:31, Kevin Wolf wrote: > Am 06.05.2016 um 14:32 hat Max Reitz geschrieben: >> On 06.05.2016 14:01, Max Reitz wrote: >>> On 27.04.2016 15:20, Kevin Wolf wrote: >>>> When block job errors were introduced, we assigned the iostatus of the >>>> target BDS "just in case". The field has never been accessible for the >>>> user because the target isn't listed in query-block. >>>> >>>> Before we can allow the user to have a second BlockBackend on the >>>> target, we need to clean this up. If anything, we would want to set the >>>> iostatus for the internal BB of the job (which we can always do later), >>>> but certainly not for a separate BB which the job doesn't even use. >>>> >>>> As a nice side effect, this gets us rid of another bs->blk use. >>>> >>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >>>> --- >>>> block/backup.c | 8 ++++---- >>>> block/mirror.c | 8 ++++---- >>>> block/stream.c | 3 +-- >>>> blockjob.c | 6 +----- >>>> include/block/blockjob.h | 4 +--- >>>> 5 files changed, 11 insertions(+), 18 deletions(-) >>> >>> Reviewed-by: Max Reitz <mreitz@redhat.com> >> >> Maybe I need to take that back. Using e.g. blockdev-backup, you can >> indeed see the target in query-block. >> >> e.g.: >> >> (echo "{'execute':'qmp_capabilities'} >> {'execute':'blockdev-backup', >> 'arguments':{'device':'src','target':'dst', >> 'sync':'full','on-target-error':'enospc'}}"; >> sleep 1; >> echo "{'execute':'query-block'}") | \ >> x86_64-softmmu/qemu-system-x86_64 \ >> -drive if=none,id=src,file=test.qcow2 \ >> -drive if=none,id=dst,file=mnt/out.qcow2 \ >> -qmp-pretty stdio -nodefaults >> >> Where mnt is a file system and test.qcow2 is large enough such that an >> ENOSPC will occur. > > Hm... Let's pretend we didn't notice. In fact, I don't think this > behaviour is documented and it also doesn't make a lot of sense. > > I would be surprised if libvirt made use of it, as there is still the > job iostatus which works in drive-* cases, too. > > (Eric?) > >> Before this patch, you can see "io-status": "nospace" in query-block for >> dst. After it, it says "io-status": "ok", and after the next it doesn't >> say anything about the status at all anymore. > > Simply letting the field disappear sounds like a nice solution. I'm not sure I completely agree with that because the iostatus appears to be the only way of obtaining the information whether the block job stopped because of an ENOSPC or because of something different (the BLOCK_JOB_ERROR doesn't tell you). I don't know whether anyone actually needs that information, though, and if you think dropping this is fine, then I guess it's fine with me, too, and my R-b stands. Max
Am 06.05.2016 um 15:40 hat Max Reitz geschrieben: > On 06.05.2016 15:31, Kevin Wolf wrote: > > Am 06.05.2016 um 14:32 hat Max Reitz geschrieben: > >> On 06.05.2016 14:01, Max Reitz wrote: > >>> On 27.04.2016 15:20, Kevin Wolf wrote: > >>>> When block job errors were introduced, we assigned the iostatus of the > >>>> target BDS "just in case". The field has never been accessible for the > >>>> user because the target isn't listed in query-block. > >>>> > >>>> Before we can allow the user to have a second BlockBackend on the > >>>> target, we need to clean this up. If anything, we would want to set the > >>>> iostatus for the internal BB of the job (which we can always do later), > >>>> but certainly not for a separate BB which the job doesn't even use. > >>>> > >>>> As a nice side effect, this gets us rid of another bs->blk use. > >>>> > >>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com> > >>>> --- > >>>> block/backup.c | 8 ++++---- > >>>> block/mirror.c | 8 ++++---- > >>>> block/stream.c | 3 +-- > >>>> blockjob.c | 6 +----- > >>>> include/block/blockjob.h | 4 +--- > >>>> 5 files changed, 11 insertions(+), 18 deletions(-) > >>> > >>> Reviewed-by: Max Reitz <mreitz@redhat.com> > >> > >> Maybe I need to take that back. Using e.g. blockdev-backup, you can > >> indeed see the target in query-block. > >> > >> e.g.: > >> > >> (echo "{'execute':'qmp_capabilities'} > >> {'execute':'blockdev-backup', > >> 'arguments':{'device':'src','target':'dst', > >> 'sync':'full','on-target-error':'enospc'}}"; > >> sleep 1; > >> echo "{'execute':'query-block'}") | \ > >> x86_64-softmmu/qemu-system-x86_64 \ > >> -drive if=none,id=src,file=test.qcow2 \ > >> -drive if=none,id=dst,file=mnt/out.qcow2 \ > >> -qmp-pretty stdio -nodefaults > >> > >> Where mnt is a file system and test.qcow2 is large enough such that an > >> ENOSPC will occur. > > > > Hm... Let's pretend we didn't notice. In fact, I don't think this > > behaviour is documented and it also doesn't make a lot of sense. > > > > I would be surprised if libvirt made use of it, as there is still the > > job iostatus which works in drive-* cases, too. > > > > (Eric?) > > > >> Before this patch, you can see "io-status": "nospace" in query-block for > >> dst. After it, it says "io-status": "ok", and after the next it doesn't > >> say anything about the status at all anymore. > > > > Simply letting the field disappear sounds like a nice solution. > > I'm not sure I completely agree with that because the iostatus appears > to be the only way of obtaining the information whether the block job > stopped because of an ENOSPC or because of something different (the > BLOCK_JOB_ERROR doesn't tell you). > > I don't know whether anyone actually needs that information, though, and > if you think dropping this is fine, then I guess it's fine with me, too, > and my R-b stands. But you don't have this information with the drive-* commands either. I would be okay with reintroducing this information, but in the right place. Modifying the guest device iostatus for block jobs certainly isn't right. Anyway, we need Eric's input to confirm that libvirt doesn't depend on this. Kevin
On 05/06/2016 07:31 AM, Kevin Wolf wrote: >> >> Maybe I need to take that back. Using e.g. blockdev-backup, you can >> indeed see the target in query-block. >> >> Where mnt is a file system and test.qcow2 is large enough such that an >> ENOSPC will occur. > > Hm... Let's pretend we didn't notice. In fact, I don't think this > behaviour is documented and it also doesn't make a lot of sense. > > I would be surprised if libvirt made use of it, as there is still the > job iostatus which works in drive-* cases, too. > > (Eric?) > >> Before this patch, you can see "io-status": "nospace" in query-block for >> dst. After it, it says "io-status": "ok", and after the next it doesn't >> say anything about the status at all anymore. Here's what libvirt does with "io-status": qemuMonitorJSONGetBlockInfo(): /* Missing io-status indicates no error */ if ((status = virJSONValueObjectGetString(dev, "io-status"))) { info->io_status = qemuMonitorBlockIOStatusToError(status); if (info->io_status < 0) goto cleanup; } Then it feeds info->io_status into qemuDomainGetDiskErrors(). However, I'm not sure I know the full context of your question - is io-status still available on the primary BDS, and we are only deleting it from a secondary spot used only during a job? I think libvirt is currently only checking the status of the BDS (ENOSPC when the primary runs out of room), not what happens during a block job. In fact, libvirt does not yet use 'blockdev-backup' at all, let alone blockdev-backup with 'on-target-error':'enospc' (it should, but that's a story for another day). > > Simply letting the field disappear sounds like a nice solution. So at first glance, I think we're okay here, dropping io-status from the block job from this location and instead exposing it from a more natural spot in QMP. And in the worst case, we can use introspection to learn which of two spots exposes the information, if libvirt is trying to be portable to 2.6 and 2.7 at the same time.
On 06.05.2016 16:12, Kevin Wolf wrote: > Am 06.05.2016 um 15:40 hat Max Reitz geschrieben: >> On 06.05.2016 15:31, Kevin Wolf wrote: >>> Am 06.05.2016 um 14:32 hat Max Reitz geschrieben: >>>> On 06.05.2016 14:01, Max Reitz wrote: >>>>> On 27.04.2016 15:20, Kevin Wolf wrote: >>>>>> When block job errors were introduced, we assigned the iostatus of the >>>>>> target BDS "just in case". The field has never been accessible for the >>>>>> user because the target isn't listed in query-block. >>>>>> >>>>>> Before we can allow the user to have a second BlockBackend on the >>>>>> target, we need to clean this up. If anything, we would want to set the >>>>>> iostatus for the internal BB of the job (which we can always do later), >>>>>> but certainly not for a separate BB which the job doesn't even use. >>>>>> >>>>>> As a nice side effect, this gets us rid of another bs->blk use. >>>>>> >>>>>> Signed-off-by: Kevin Wolf <kwolf@redhat.com> >>>>>> --- >>>>>> block/backup.c | 8 ++++---- >>>>>> block/mirror.c | 8 ++++---- >>>>>> block/stream.c | 3 +-- >>>>>> blockjob.c | 6 +----- >>>>>> include/block/blockjob.h | 4 +--- >>>>>> 5 files changed, 11 insertions(+), 18 deletions(-) >>>>> >>>>> Reviewed-by: Max Reitz <mreitz@redhat.com> >>>> >>>> Maybe I need to take that back. Using e.g. blockdev-backup, you can >>>> indeed see the target in query-block. >>>> >>>> e.g.: >>>> >>>> (echo "{'execute':'qmp_capabilities'} >>>> {'execute':'blockdev-backup', >>>> 'arguments':{'device':'src','target':'dst', >>>> 'sync':'full','on-target-error':'enospc'}}"; >>>> sleep 1; >>>> echo "{'execute':'query-block'}") | \ >>>> x86_64-softmmu/qemu-system-x86_64 \ >>>> -drive if=none,id=src,file=test.qcow2 \ >>>> -drive if=none,id=dst,file=mnt/out.qcow2 \ >>>> -qmp-pretty stdio -nodefaults >>>> >>>> Where mnt is a file system and test.qcow2 is large enough such that an >>>> ENOSPC will occur. >>> >>> Hm... Let's pretend we didn't notice. In fact, I don't think this >>> behaviour is documented and it also doesn't make a lot of sense. >>> >>> I would be surprised if libvirt made use of it, as there is still the >>> job iostatus which works in drive-* cases, too. >>> >>> (Eric?) >>> >>>> Before this patch, you can see "io-status": "nospace" in query-block for >>>> dst. After it, it says "io-status": "ok", and after the next it doesn't >>>> say anything about the status at all anymore. >>> >>> Simply letting the field disappear sounds like a nice solution. >> >> I'm not sure I completely agree with that because the iostatus appears >> to be the only way of obtaining the information whether the block job >> stopped because of an ENOSPC or because of something different (the >> BLOCK_JOB_ERROR doesn't tell you). >> >> I don't know whether anyone actually needs that information, though, and >> if you think dropping this is fine, then I guess it's fine with me, too, >> and my R-b stands. > > But you don't have this information with the drive-* commands either. > > I would be okay with reintroducing this information, but in the right > place. Modifying the guest device iostatus for block jobs certainly > isn't right. As discussed on IRC, that sounds right to me. Thus, have an explicit Reviewed-by: Max Reitz <mreitz@redhat.com> > Anyway, we need Eric's input to confirm that libvirt doesn't depend on > this. > > Kevin >
diff --git a/block/backup.c b/block/backup.c index 491fd14..ef7609d 100644 --- a/block/backup.c +++ b/block/backup.c @@ -272,11 +272,11 @@ static BlockErrorAction backup_error_action(BackupBlockJob *job, bool read, int error) { if (read) { - return block_job_error_action(&job->common, job->common.bs, - job->on_source_error, true, error); + return block_job_error_action(&job->common, job->on_source_error, + true, error); } else { - return block_job_error_action(&job->common, job->target, - job->on_target_error, false, error); + return block_job_error_action(&job->common, job->on_target_error, + false, error); } } diff --git a/block/mirror.c b/block/mirror.c index 039f481..c7f51ad 100644 --- a/block/mirror.c +++ b/block/mirror.c @@ -80,11 +80,11 @@ static BlockErrorAction mirror_error_action(MirrorBlockJob *s, bool read, { s->synced = false; if (read) { - return block_job_error_action(&s->common, s->common.bs, - s->on_source_error, true, error); + return block_job_error_action(&s->common, s->on_source_error, + true, error); } else { - return block_job_error_action(&s->common, s->target, - s->on_target_error, false, error); + return block_job_error_action(&s->common, s->on_target_error, + false, error); } } diff --git a/block/stream.c b/block/stream.c index 332b9a1..16c3e90 100644 --- a/block/stream.c +++ b/block/stream.c @@ -163,8 +163,7 @@ wait: } if (ret < 0) { BlockErrorAction action = - block_job_error_action(&s->common, s->common.bs, s->on_error, - true, -ret); + block_job_error_action(&s->common, s->on_error, true, -ret); if (action == BLOCK_ERROR_ACTION_STOP) { n = 0; continue; diff --git a/blockjob.c b/blockjob.c index 9fc37ca..5b840a7 100644 --- a/blockjob.c +++ b/blockjob.c @@ -411,8 +411,7 @@ void block_job_event_ready(BlockJob *job) job->speed, &error_abort); } -BlockErrorAction block_job_error_action(BlockJob *job, BlockDriverState *bs, - BlockdevOnError on_err, +BlockErrorAction block_job_error_action(BlockJob *job, BlockdevOnError on_err, int is_read, int error) { BlockErrorAction action; @@ -443,9 +442,6 @@ BlockErrorAction block_job_error_action(BlockJob *job, BlockDriverState *bs, job->user_paused = true; block_job_pause(job); block_job_iostatus_set_err(job, error); - if (bs->blk && bs != job->bs) { - blk_iostatus_set_err(bs->blk, error); - } } return action; } diff --git a/include/block/blockjob.h b/include/block/blockjob.h index 8bedc49..073a433 100644 --- a/include/block/blockjob.h +++ b/include/block/blockjob.h @@ -383,7 +383,6 @@ void block_job_iostatus_reset(BlockJob *job); /** * block_job_error_action: * @job: The job to signal an error for. - * @bs: The block device on which to set an I/O error. * @on_err: The error action setting. * @is_read: Whether the operation was a read. * @error: The error that was reported. @@ -391,8 +390,7 @@ void block_job_iostatus_reset(BlockJob *job); * Report an I/O error for a block job and possibly stop the VM. Return the * action that was selected based on @on_err and @error. */ -BlockErrorAction block_job_error_action(BlockJob *job, BlockDriverState *bs, - BlockdevOnError on_err, +BlockErrorAction block_job_error_action(BlockJob *job, BlockdevOnError on_err, int is_read, int error); typedef void BlockJobDeferToMainLoopFn(BlockJob *job, void *opaque);
When block job errors were introduced, we assigned the iostatus of the target BDS "just in case". The field has never been accessible for the user because the target isn't listed in query-block. Before we can allow the user to have a second BlockBackend on the target, we need to clean this up. If anything, we would want to set the iostatus for the internal BB of the job (which we can always do later), but certainly not for a separate BB which the job doesn't even use. As a nice side effect, this gets us rid of another bs->blk use. Signed-off-by: Kevin Wolf <kwolf@redhat.com> --- block/backup.c | 8 ++++---- block/mirror.c | 8 ++++---- block/stream.c | 3 +-- blockjob.c | 6 +----- include/block/blockjob.h | 4 +--- 5 files changed, 11 insertions(+), 18 deletions(-)