| Message ID | 20190506203344.30781-1-jsnow@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | blockdev-backup: don't check aio_context too early | expand |
Am 06.05.2019 um 22:33 hat John Snow geschrieben: > in blockdev_backup_prepare, we check to make sure that the target is > associated with a compatible aio context. However, do_blockdev_backup is > called later and has some logic to move the target to a compatible > aio_context. The transaction version will fail certain commands > needlessly early as a result. > > Allow blockdev_backup_prepare to simply call do_blockdev_backup, which > will ultimately decide if the contexts are compatible or not. > > Note: the transaction version has always disallowed this operation since > its initial commit bd8baecd (2014), whereas the version of > qmp_blockdev_backup at the time, from commit c29c1dd312f, tried to > enforce the aio_context switch instead. It's not clear, and I can't see > from the mailing list archives at the time, why the two functions take a > different approach. It wasn't until later in efd7556708b (2016) that the > standalone version tried to determine if it could set the context or > not. > > Reported-by: aihua liang <aliang@redhat.com> > Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1683498 Signed-off-by is missing, and a testcase, too. :-) > diff --git a/blockdev.c b/blockdev.c > index 79fbac8450..a81d88980c 100644 > --- a/blockdev.c > +++ b/blockdev.c > @@ -1872,10 +1872,6 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp) > } > > aio_context = bdrv_get_aio_context(bs); > - if (aio_context != bdrv_get_aio_context(target)) { > - error_setg(errp, "Backup between two IO threads is not implemented"); > - return; > - } > aio_context_acquire(aio_context); > state->bs = bs; The actual change looks good to me. Kevin
On 5/7/19 4:50 AM, Kevin Wolf wrote: > Am 06.05.2019 um 22:33 hat John Snow geschrieben: >> in blockdev_backup_prepare, we check to make sure that the target is >> associated with a compatible aio context. However, do_blockdev_backup is >> called later and has some logic to move the target to a compatible >> aio_context. The transaction version will fail certain commands >> needlessly early as a result. >> >> Allow blockdev_backup_prepare to simply call do_blockdev_backup, which >> will ultimately decide if the contexts are compatible or not. >> >> Note: the transaction version has always disallowed this operation since >> its initial commit bd8baecd (2014), whereas the version of >> qmp_blockdev_backup at the time, from commit c29c1dd312f, tried to >> enforce the aio_context switch instead. It's not clear, and I can't see >> from the mailing list archives at the time, why the two functions take a >> different approach. It wasn't until later in efd7556708b (2016) that the >> standalone version tried to determine if it could set the context or >> not. >> >> Reported-by: aihua liang <aliang@redhat.com> >> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1683498 > > Signed-off-by is missing, and a testcase, too. :-) > I am apparently exceedingly good at breaking git-publish, sorry about that. >> diff --git a/blockdev.c b/blockdev.c >> index 79fbac8450..a81d88980c 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -1872,10 +1872,6 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp) >> } >> >> aio_context = bdrv_get_aio_context(bs); >> - if (aio_context != bdrv_get_aio_context(target)) { >> - error_setg(errp, "Backup between two IO threads is not implemented"); >> - return; >> - } >> aio_context_acquire(aio_context); >> state->bs = bs; > > The actual change looks good to me. > > Kevin > As far as the test case goes, I'll see if I can reproduce it without a real guest. If you test it with no guest and empty drives with the VM paused, the error never gets thrown. Maybe it's enough to start an empty guest and do some junk writes to make the bitmap non-empty. Thanks, --js
On 5/7/19 4:50 AM, Kevin Wolf wrote: > Am 06.05.2019 um 22:33 hat John Snow geschrieben: >> in blockdev_backup_prepare, we check to make sure that the target is >> associated with a compatible aio context. However, do_blockdev_backup is >> called later and has some logic to move the target to a compatible >> aio_context. The transaction version will fail certain commands >> needlessly early as a result. >> >> Allow blockdev_backup_prepare to simply call do_blockdev_backup, which >> will ultimately decide if the contexts are compatible or not. >> >> Note: the transaction version has always disallowed this operation since >> its initial commit bd8baecd (2014), whereas the version of >> qmp_blockdev_backup at the time, from commit c29c1dd312f, tried to >> enforce the aio_context switch instead. It's not clear, and I can't see >> from the mailing list archives at the time, why the two functions take a >> different approach. It wasn't until later in efd7556708b (2016) that the >> standalone version tried to determine if it could set the context or >> not. >> >> Reported-by: aihua liang <aliang@redhat.com> >> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1683498 > > Signed-off-by is missing, and a testcase, too. :-) > >> diff --git a/blockdev.c b/blockdev.c >> index 79fbac8450..a81d88980c 100644 >> --- a/blockdev.c >> +++ b/blockdev.c >> @@ -1872,10 +1872,6 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp) >> } >> >> aio_context = bdrv_get_aio_context(bs); >> - if (aio_context != bdrv_get_aio_context(target)) { >> - error_setg(errp, "Backup between two IO threads is not implemented"); >> - return; >> - } >> aio_context_acquire(aio_context); >> state->bs = bs; > > The actual change looks good to me. > > Kevin > (See the Red Hat BZ for details on the test I am more or less trying to replicate in iotests -- but the jist of it is using an iothread on one device and not specifying one for the other, then backing up both to two blockdev-add created nodes not attached to any device.) Is there some trick to getting this to fail with accel=qtest? I'm noticing that if the VM is paused or if I use accel=qtest, the iothread contexts for the drives appear to go ... unresolved? and the test passes. I've only had luck (so far) reproducing this with accel=kvm on a running VM (the drives can be empty) and I don't actually know why that is just yet -- I guess these aio_context objects get resolved at runtime? Anyway, this seems to be a little tricky so far, maybe you have some advice for me? --js (Also note: doing a full backup and an incremental backup for two perfectly empty 64GB qcow2 drives takes over 6 seconds in total. It probably shouldn't, right? There's something about the initial backup that appears to take a pretty long time.)
Am 09.05.2019 um 00:55 hat John Snow geschrieben: > > On 5/7/19 4:50 AM, Kevin Wolf wrote: > > Am 06.05.2019 um 22:33 hat John Snow geschrieben: > >> in blockdev_backup_prepare, we check to make sure that the target is > >> associated with a compatible aio context. However, do_blockdev_backup is > >> called later and has some logic to move the target to a compatible > >> aio_context. The transaction version will fail certain commands > >> needlessly early as a result. > >> > >> Allow blockdev_backup_prepare to simply call do_blockdev_backup, which > >> will ultimately decide if the contexts are compatible or not. > >> > >> Note: the transaction version has always disallowed this operation since > >> its initial commit bd8baecd (2014), whereas the version of > >> qmp_blockdev_backup at the time, from commit c29c1dd312f, tried to > >> enforce the aio_context switch instead. It's not clear, and I can't see > >> from the mailing list archives at the time, why the two functions take a > >> different approach. It wasn't until later in efd7556708b (2016) that the > >> standalone version tried to determine if it could set the context or > >> not. > >> > >> Reported-by: aihua liang <aliang@redhat.com> > >> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1683498 > > > > Signed-off-by is missing, and a testcase, too. :-) > > > >> diff --git a/blockdev.c b/blockdev.c > >> index 79fbac8450..a81d88980c 100644 > >> --- a/blockdev.c > >> +++ b/blockdev.c > >> @@ -1872,10 +1872,6 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp) > >> } > >> > >> aio_context = bdrv_get_aio_context(bs); > >> - if (aio_context != bdrv_get_aio_context(target)) { > >> - error_setg(errp, "Backup between two IO threads is not implemented"); > >> - return; > >> - } > >> aio_context_acquire(aio_context); > >> state->bs = bs; > > > > The actual change looks good to me. > > > > Kevin > > > > (See the Red Hat BZ for details on the test I am more or less trying to > replicate in iotests -- but the jist of it is using an iothread on one > device and not specifying one for the other, then backing up both to two > blockdev-add created nodes not attached to any device.) > > Is there some trick to getting this to fail with accel=qtest? I'm > noticing that if the VM is paused or if I use accel=qtest, the iothread > contexts for the drives appear to go ... unresolved? and the test passes. > > I've only had luck (so far) reproducing this with accel=kvm on a running > VM (the drives can be empty) and I don't actually know why that is just > yet -- I guess these aio_context objects get resolved at runtime? > > Anyway, this seems to be a little tricky so far, maybe you have some > advice for me? Are you using virtio-blk? I think it only actually enables dataplane (and moves its node to an iothread) during initialisation of the guest driver. This makes it quite annoying for testing, I've had the same problem before. However, virtio-scsi does what you would expect and puts the node into the right AioContext directly on creation. So maybe just switching to virtio-scsi solves you problem? > (Also note: doing a full backup and an incremental backup for two > perfectly empty 64GB qcow2 drives takes over 6 seconds in total. It > probably shouldn't, right? There's something about the initial backup > that appears to take a pretty long time.) Sounds like Someone (TM) should have a closer look, yes. Kevin
On 5/9/19 4:00 AM, Kevin Wolf wrote: > Am 09.05.2019 um 00:55 hat John Snow geschrieben: >> >> On 5/7/19 4:50 AM, Kevin Wolf wrote: >>> Am 06.05.2019 um 22:33 hat John Snow geschrieben: >>>> in blockdev_backup_prepare, we check to make sure that the target is >>>> associated with a compatible aio context. However, do_blockdev_backup is >>>> called later and has some logic to move the target to a compatible >>>> aio_context. The transaction version will fail certain commands >>>> needlessly early as a result. >>>> >>>> Allow blockdev_backup_prepare to simply call do_blockdev_backup, which >>>> will ultimately decide if the contexts are compatible or not. >>>> >>>> Note: the transaction version has always disallowed this operation since >>>> its initial commit bd8baecd (2014), whereas the version of >>>> qmp_blockdev_backup at the time, from commit c29c1dd312f, tried to >>>> enforce the aio_context switch instead. It's not clear, and I can't see >>>> from the mailing list archives at the time, why the two functions take a >>>> different approach. It wasn't until later in efd7556708b (2016) that the >>>> standalone version tried to determine if it could set the context or >>>> not. >>>> >>>> Reported-by: aihua liang <aliang@redhat.com> >>>> Fixes: https://bugzilla.redhat.com/show_bug.cgi?id=1683498 >>> >>> Signed-off-by is missing, and a testcase, too. :-) >>> >>>> diff --git a/blockdev.c b/blockdev.c >>>> index 79fbac8450..a81d88980c 100644 >>>> --- a/blockdev.c >>>> +++ b/blockdev.c >>>> @@ -1872,10 +1872,6 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp) >>>> } >>>> >>>> aio_context = bdrv_get_aio_context(bs); >>>> - if (aio_context != bdrv_get_aio_context(target)) { >>>> - error_setg(errp, "Backup between two IO threads is not implemented"); >>>> - return; >>>> - } >>>> aio_context_acquire(aio_context); >>>> state->bs = bs; >>> >>> The actual change looks good to me. >>> >>> Kevin >>> >> >> (See the Red Hat BZ for details on the test I am more or less trying to >> replicate in iotests -- but the jist of it is using an iothread on one >> device and not specifying one for the other, then backing up both to two >> blockdev-add created nodes not attached to any device.) >> >> Is there some trick to getting this to fail with accel=qtest? I'm >> noticing that if the VM is paused or if I use accel=qtest, the iothread >> contexts for the drives appear to go ... unresolved? and the test passes. >> >> I've only had luck (so far) reproducing this with accel=kvm on a running >> VM (the drives can be empty) and I don't actually know why that is just >> yet -- I guess these aio_context objects get resolved at runtime? >> >> Anyway, this seems to be a little tricky so far, maybe you have some >> advice for me? > > Are you using virtio-blk? I think it only actually enables dataplane > (and moves its node to an iothread) during initialisation of the guest > driver. This makes it quite annoying for testing, I've had the same > problem before. > > However, virtio-scsi does what you would expect and puts the node into > the right AioContext directly on creation. > > So maybe just switching to virtio-scsi solves you problem? > Aha. I will give that a try, thanks! I was indeed using virtio-blk. >> (Also note: doing a full backup and an incremental backup for two >> perfectly empty 64GB qcow2 drives takes over 6 seconds in total. It >> probably shouldn't, right? There's something about the initial backup >> that appears to take a pretty long time.) > > Sounds like Someone (TM) should have a closer look, yes. > > Kevin > OK, I'll dig into that after I finish this piece. Thank you, --js
diff --git a/blockdev.c b/blockdev.c index 79fbac8450..a81d88980c 100644 --- a/blockdev.c +++ b/blockdev.c @@ -1872,10 +1872,6 @@ static void blockdev_backup_prepare(BlkActionState *common, Error **errp) } aio_context = bdrv_get_aio_context(bs); - if (aio_context != bdrv_get_aio_context(target)) { - error_setg(errp, "Backup between two IO threads is not implemented"); - return; - } aio_context_acquire(aio_context); state->bs = bs;