| Message ID | 20180628180042.3881-2-jsnow@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | block: formalize and test fleecing | expand |
On 06/28/2018 01:00 PM, John Snow wrote: > In the case of image fleecing, the node we choose as the source > for a blockdev-backup is going to be both a root node AND the > backing node for the exported image. It does not qualify as a root > image in this case. > > Loosen the restriction. > > Signed-off-by: John Snow <jsnow@redhat.com> > --- > blockdev.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) In v1, you mentioned that this used to work but then regressed, pinpointing that detail in the commit message might be nice, but not essential (since we didn't test it until now). So, Reviewed-by: Eric Blake <eblake@redhat.com>
On 06/28/2018 02:05 PM, Eric Blake wrote: > On 06/28/2018 01:00 PM, John Snow wrote: >> In the case of image fleecing, the node we choose as the source >> for a blockdev-backup is going to be both a root node AND the >> backing node for the exported image. It does not qualify as a root >> image in this case. >> >> Loosen the restriction. >> >> Signed-off-by: John Snow <jsnow@redhat.com> >> --- >> blockdev.c | 2 +- >> 1 file changed, 1 insertion(+), 1 deletion(-) > > In v1, you mentioned that this used to work but then regressed, > pinpointing that detail in the commit message might be nice, but not > essential (since we didn't test it until now). So, > Really not sure when it regressed; I consider it unimportant as nothing uses it presently: no docs, no tests, nothing in libvirt. My guess, though, is that it worked prior to cef34eebf3d0f252a3b3e9a2a459b6c3ecc56f68. In 2.8, possibly? Kashyap might know, I think he's experimented with this sometime in that timezone. > Reviewed-by: Eric Blake <eblake@redhat.com> >
On Thu, Jun 28, 2018 at 05:19:37PM -0400, John Snow wrote: > > > On 06/28/2018 02:05 PM, Eric Blake wrote: > > On 06/28/2018 01:00 PM, John Snow wrote: > >> In the case of image fleecing, the node we choose as the source > >> for a blockdev-backup is going to be both a root node AND the > >> backing node for the exported image. It does not qualify as a root > >> image in this case. > >> > >> Loosen the restriction. > >> > >> Signed-off-by: John Snow <jsnow@redhat.com> > >> --- > >> blockdev.c | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > > > > In v1, you mentioned that this used to work but then regressed, > > pinpointing that detail in the commit message might be nice, but not > > essential (since we didn't test it until now). So, > > > > Really not sure when it regressed; I consider it unimportant as nothing > uses it presently: no docs, no tests, nothing in libvirt. > > My guess, though, is that it worked prior to > cef34eebf3d0f252a3b3e9a2a459b6c3ecc56f68. So the above commit adds the ability to assign node names for `blockdev-backup`, which wasn't possible during 2.8 timeframe. > In 2.8, possibly? > > Kashyap might know, I think he's experimented with this sometime in that > timezone. I don't recall doing the "allow blockdev-backup from any source" test with 2.8. (But I recall gradually moving from figuring out the source block device via wading through the output of `query-named-block-nodes` to using the 'node-name'.) Looking at my test notes with QEMU 2.8, this was my workflow (see how `blockdev-backup` still doesn't use 'node-name's): (QEMU) blockdev-add driver=qcow2 node-name=node2 file={"driver":"file","filename":"/export/target.qcow2"} backing={"driver":"qcow2","file":{"driver":"file","filename":"/export/base.qcow2"}} (QEMU) query-named-block-nodes # To dig through the source block device "#blockYYY" (QEMU) blockdev-backup device=#block197 target=node1 sync=none (QEMU) nbd-server-start addr={"type":"unix","data":{"path":"./nbd-sock"}} (QEMU) nbd-server-add device=node1 (QEMU) nbd-server-stop Later on I moved (thanks to the Git commit you mentioned earlier) to the much cleaner approach of using 'node-name' (the below syntax is from my test notes with QEMU 2.12): (QEMU) blockdev-add driver=qcow2 node-name=node-E file={"driver":"file","filename":"e.qcow2"} (QEMU) blockdev-backup device=node-B target=node-E sync=full job-id=job0 [...]
On 07/02/2018 03:59 AM, Kashyap Chamarthy wrote: > On Thu, Jun 28, 2018 at 05:19:37PM -0400, John Snow wrote: >> >> >> On 06/28/2018 02:05 PM, Eric Blake wrote: >>> On 06/28/2018 01:00 PM, John Snow wrote: >>>> In the case of image fleecing, the node we choose as the source >>>> for a blockdev-backup is going to be both a root node AND the >>>> backing node for the exported image. It does not qualify as a root >>>> image in this case. >>>> >>>> Loosen the restriction. >>>> >>>> Signed-off-by: John Snow <jsnow@redhat.com> >>>> --- >>>> blockdev.c | 2 +- >>>> 1 file changed, 1 insertion(+), 1 deletion(-) >>> >>> In v1, you mentioned that this used to work but then regressed, >>> pinpointing that detail in the commit message might be nice, but not >>> essential (since we didn't test it until now). So, >>> >> >> Really not sure when it regressed; I consider it unimportant as nothing >> uses it presently: no docs, no tests, nothing in libvirt. >> >> My guess, though, is that it worked prior to >> cef34eebf3d0f252a3b3e9a2a459b6c3ecc56f68. > > So the above commit adds the ability to assign node names for > `blockdev-backup`, which wasn't possible during 2.8 timeframe. > >> In 2.8, possibly? >> >> Kashyap might know, I think he's experimented with this sometime in that >> timezone. > > I don't recall doing the "allow blockdev-backup from any source" test > with 2.8. (But I recall gradually moving from figuring out the source > block device via wading through the output of `query-named-block-nodes` > to using the 'node-name'.) > > Looking at my test notes with QEMU 2.8, this was my > workflow (see how `blockdev-backup` still doesn't use 'node-name's): > > (QEMU) blockdev-add driver=qcow2 node-name=node2 file={"driver":"file","filename":"/export/target.qcow2"} backing={"driver":"qcow2","file":{"driver":"file","filename":"/export/base.qcow2"}} > (QEMU) query-named-block-nodes # To dig through the source block device "#blockYYY" > (QEMU) blockdev-backup device=#block197 target=node1 sync=none > (QEMU) nbd-server-start addr={"type":"unix","data":{"path":"./nbd-sock"}} > (QEMU) nbd-server-add device=node1 > (QEMU) nbd-server-stop > > Later on I moved (thanks to the Git commit you mentioned earlier) to the > much cleaner approach of using 'node-name' (the below syntax is > from my test notes with QEMU 2.12): > > (QEMU) blockdev-add driver=qcow2 node-name=node-E file={"driver":"file","filename":"e.qcow2"} > (QEMU) blockdev-backup device=node-B target=node-E sync=full job-id=job0 > [...] > Without this patch, "allow blockdev-backup from any source", the fleecing workflow prohibits the blockdev-backup command. We're wondering when that regressed. If you managed to get fleecing working without this patch here, then it was working at that time. --js
diff --git a/blockdev.c b/blockdev.c index 58d7570932..526f8b60be 100644 --- a/blockdev.c +++ b/blockdev.c @@ -3517,7 +3517,7 @@ BlockJob *do_blockdev_backup(BlockdevBackup *backup, JobTxn *txn, backup->compress = false; } - bs = qmp_get_root_bs(backup->device, errp); + bs = bdrv_lookup_bs(backup->device, backup->device, errp); if (!bs) { return NULL; }
In the case of image fleecing, the node we choose as the source for a blockdev-backup is going to be both a root node AND the backing node for the exported image. It does not qualify as a root image in this case. Loosen the restriction. Signed-off-by: John Snow <jsnow@redhat.com> --- blockdev.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)