* [Qemu-devel] [PATCH 0/2] block-stream: include base into job node list @ 2019-02-21 15:26 Andrey Shinkevich 2019-02-21 15:26 ` [Qemu-devel] [PATCH 1/2] iotests: 030 TestParallelOps non-shared base node Andrey Shinkevich 2019-02-21 15:26 ` [Qemu-devel] [PATCH 2/2] block-stream: include base into job node list Andrey Shinkevich 0 siblings, 2 replies; 7+ messages in thread From: Andrey Shinkevich @ 2019-02-21 15:26 UTC (permalink / raw) To: qemu-devel, qemu-block Cc: berto, mreitz, kwolf, den, vsementsov, andrey.shinkevich The block-stream job needs to own the base node as the limiter for the copy-on-read operation. So, the base node is included in the job node list by calling to the function block_job_add_bdrv(). Also, the block-stream job would not allow the base node to go away due to the graph modification, e.g. when a filter node is inserted between the bottom node and the base node. For that reason, the flag BLK_PERM_GRAPH_MOD is unset in the shared permission bit mask of the base node. Andrey Shinkevich (2): iotests: 030 TestParallelOps non-shared base node block-stream: include base node into the job protected list block/stream.c | 9 +++++++++ tests/qemu-iotests/030 | 34 ++++++++++++++++++---------------- 2 files changed, 27 insertions(+), 16 deletions(-) -- 1.8.3.1 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH 1/2] iotests: 030 TestParallelOps non-shared base node 2019-02-21 15:26 [Qemu-devel] [PATCH 0/2] block-stream: include base into job node list Andrey Shinkevich @ 2019-02-21 15:26 ` Andrey Shinkevich 2019-02-21 15:26 ` [Qemu-devel] [PATCH 2/2] block-stream: include base into job node list Andrey Shinkevich 1 sibling, 0 replies; 7+ messages in thread From: Andrey Shinkevich @ 2019-02-21 15:26 UTC (permalink / raw) To: qemu-devel, qemu-block Cc: berto, mreitz, kwolf, den, vsementsov, andrey.shinkevich The test case TestParallelOps::test_stream_parallel in #030 fails if a base node is protected by the block-stream running job that includes the base node into the job node list (block_job_add_bdrv) without BLK_PERM_GRAPH_MOD shared permission. The block-stream job would own the base node not allowing it to go away due to the graph modification, e.g. when a filter node is inserted above a top node of the parallel job. That's, the base node was shared between the two parallel jobs. This patch excludes sharing the node by inserting additional nodes inbetween. Furthermore, the block-stream job needs to own base node as the limit to copy-on-read operation. The insertion of additional nodes incures changes of node numbers in two other cases: test_overlapping_1 test_stream_base_node_name because the nodes with written data got other numbers. Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> --- tests/qemu-iotests/030 | 34 ++++++++++++++++++---------------- 1 file changed, 18 insertions(+), 16 deletions(-) diff --git a/tests/qemu-iotests/030 b/tests/qemu-iotests/030 index 276e06b..c801be1 100755 --- a/tests/qemu-iotests/030 +++ b/tests/qemu-iotests/030 @@ -153,7 +153,8 @@ class TestSingleDrive(iotests.QMPTestCase): class TestParallelOps(iotests.QMPTestCase): num_ops = 4 # Number of parallel block-stream operations - num_imgs = num_ops * 2 + 1 + step = 3 # base + source + target + num_imgs = num_ops * step image_len = num_ops * 512 * 1024 imgs = [] @@ -174,14 +175,15 @@ class TestParallelOps(iotests.QMPTestCase): '-o', 'backing_file=%s' % self.imgs[i-1], self.imgs[i]) # Put data into the images we are copying data from - odd_img_indexes = [x for x in reversed(range(self.num_imgs)) if x % 2 == 1] - for i in range(len(odd_img_indexes)): + source_img_indexes = \ + list(reversed(list(range(self.num_imgs)[1::self.step]))) + for i in range(len(source_img_indexes)): # Alternate between 256KB and 512KB. # This way jobs will not finish in the same order they were created num_kb = 256 + 256 * (i % 2) qemu_io('-f', iotests.imgfmt, '-c', 'write -P 0xFF %dk %dk' % (i * 512, num_kb), - self.imgs[odd_img_indexes[i]]) + self.imgs[source_img_indexes[i]]) # Attach the drive to the VM self.vm = iotests.VM() @@ -199,14 +201,14 @@ class TestParallelOps(iotests.QMPTestCase): self.assert_no_active_block_jobs() # Check that the maps don't match before the streaming operations - for i in range(2, self.num_imgs, 2): + for i in range(2, self.num_imgs, self.step): self.assertNotEqual(qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', self.imgs[i]), qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', self.imgs[i-1]), 'image file map matches backing file before streaming') # Create all streaming jobs pending_jobs = [] - for i in range(2, self.num_imgs, 2): + for i in range(2, self.num_imgs, self.step): node_name = 'node%d' % i job_id = 'stream-%s' % node_name pending_jobs.append(job_id) @@ -226,7 +228,7 @@ class TestParallelOps(iotests.QMPTestCase): self.vm.shutdown() # Check that all maps match now - for i in range(2, self.num_imgs, 2): + for i in range(2, self.num_imgs, self.step): self.assertEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[i]), qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[i-1]), 'image file map does not match backing file after streaming') @@ -237,16 +239,16 @@ class TestParallelOps(iotests.QMPTestCase): self.assert_no_active_block_jobs() # Set a speed limit to make sure that this job blocks the rest - result = self.vm.qmp('block-stream', device='node4', job_id='stream-node4', base=self.imgs[1], speed=1024*1024) + result = self.vm.qmp('block-stream', device='node5', job_id='stream-node5', base=self.imgs[1], speed=1024*1024) self.assert_qmp(result, 'return', {}) - result = self.vm.qmp('block-stream', device='node5', job_id='stream-node5', base=self.imgs[2]) + result = self.vm.qmp('block-stream', device='node6', job_id='stream-node6', base=self.imgs[2]) self.assert_qmp(result, 'error/class', 'GenericError') result = self.vm.qmp('block-stream', device='node3', job_id='stream-node3', base=self.imgs[2]) self.assert_qmp(result, 'error/class', 'GenericError') - result = self.vm.qmp('block-stream', device='node4', job_id='stream-node4-v2') + result = self.vm.qmp('block-stream', device='node5', job_id='stream-node5-v2') self.assert_qmp(result, 'error/class', 'GenericError') # block-commit should also fail if it touches nodes used by the stream job @@ -260,7 +262,7 @@ class TestParallelOps(iotests.QMPTestCase): result = self.vm.qmp('block-commit', device='drive0', base=self.imgs[0], top=self.imgs[1], job_id='commit-node0') self.assert_qmp(result, 'error/class', 'GenericError') - self.wait_until_completed(drive='stream-node4') + self.wait_until_completed(drive='stream-node5') self.assert_no_active_block_jobs() # Similar to test_overlapping_1, but with block-commit @@ -383,8 +385,8 @@ class TestParallelOps(iotests.QMPTestCase): def test_stream_base_node_name(self): self.assert_no_active_block_jobs() - self.assertNotEqual(qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', self.imgs[4]), - qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', self.imgs[3]), + self.assertNotEqual(qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', self.imgs[5]), + qemu_io('-f', iotests.imgfmt, '-rU', '-c', 'map', self.imgs[4]), 'image file map matches backing file before streaming') # Error: the base node does not exist @@ -404,7 +406,7 @@ class TestParallelOps(iotests.QMPTestCase): self.assert_qmp(result, 'error/class', 'GenericError') # Success: the base node is a backing file of the top node - result = self.vm.qmp('block-stream', device='node4', base_node='node2', job_id='stream') + result = self.vm.qmp('block-stream', device='node5', base_node='node3', job_id='stream') self.assert_qmp(result, 'return', {}) self.wait_until_completed(drive='stream') @@ -412,8 +414,8 @@ class TestParallelOps(iotests.QMPTestCase): self.assert_no_active_block_jobs() self.vm.shutdown() - self.assertEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[4]), - qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[3]), + self.assertEqual(qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[5]), + qemu_io('-f', iotests.imgfmt, '-c', 'map', self.imgs[4]), 'image file map matches backing file after streaming') class TestQuorum(iotests.QMPTestCase): -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [Qemu-devel] [PATCH 2/2] block-stream: include base into job node list 2019-02-21 15:26 [Qemu-devel] [PATCH 0/2] block-stream: include base into job node list Andrey Shinkevich 2019-02-21 15:26 ` [Qemu-devel] [PATCH 1/2] iotests: 030 TestParallelOps non-shared base node Andrey Shinkevich @ 2019-02-21 15:26 ` Andrey Shinkevich 2019-02-25 16:39 ` Vladimir Sementsov-Ogievskiy 2019-03-08 14:00 ` Alberto Garcia 1 sibling, 2 replies; 7+ messages in thread From: Andrey Shinkevich @ 2019-02-21 15:26 UTC (permalink / raw) To: qemu-devel, qemu-block Cc: berto, mreitz, kwolf, den, vsementsov, andrey.shinkevich The block-stream job needs to own the base node as the limiter of the copy-on-read operation. So, the base node is included in the job node list (block_job_add_bdrv). Also, the block-stream job would not allow the base node to go away due to the graph modification, e.g. when a filter node is inserted between the bottom node and the base node. For that reason, the flag BLK_PERM_GRAPH_MOD is unset in the shared permission bit mask of the base node. Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> --- block/stream.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/block/stream.c b/block/stream.c index 7a49ac0..c8f93d4 100644 --- a/block/stream.c +++ b/block/stream.c @@ -259,6 +259,15 @@ void stream_start(const char *job_id, BlockDriverState *bs, &error_abort); } + if (base) { + /* + * The base node should not disappear during the job. + */ + block_job_add_bdrv(&s->common, "base", base, 0, + BLK_PERM_ALL & ~BLK_PERM_GRAPH_MOD, + &error_abort); + } + s->base = base; s->backing_file_str = g_strdup(backing_file_str); s->bs_read_only = bs_read_only; -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] block-stream: include base into job node list 2019-02-21 15:26 ` [Qemu-devel] [PATCH 2/2] block-stream: include base into job node list Andrey Shinkevich @ 2019-02-25 16:39 ` Vladimir Sementsov-Ogievskiy 2019-02-26 13:33 ` Alberto Garcia 2019-03-08 14:00 ` Alberto Garcia 1 sibling, 1 reply; 7+ messages in thread From: Vladimir Sementsov-Ogievskiy @ 2019-02-25 16:39 UTC (permalink / raw) To: Andrey Shinkevich, qemu-devel, qemu-block Cc: berto, mreitz, kwolf, Denis Lunev 21.02.2019 18:26, Andrey Shinkevich wrote: > The block-stream job needs to own the base node as the limiter > of the copy-on-read operation. So, the base node is included in > the job node list (block_job_add_bdrv). > Also, the block-stream job would not allow the base node to go > away due to the graph modification, e.g. when a filter node is > inserted between the bottom node and the base node. > For that reason, the flag BLK_PERM_GRAPH_MOD is unset in the > shared permission bit mask of the base node. > > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> > --- > block/stream.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/block/stream.c b/block/stream.c > index 7a49ac0..c8f93d4 100644 > --- a/block/stream.c > +++ b/block/stream.c > @@ -259,6 +259,15 @@ void stream_start(const char *job_id, BlockDriverState *bs, > &error_abort); > } > > + if (base) { > + /* > + * The base node should not disappear during the job. > + */ > + block_job_add_bdrv(&s->common, "base", base, 0, > + BLK_PERM_ALL & ~BLK_PERM_GRAPH_MOD, > + &error_abort); > + } > + > s->base = base; > s->backing_file_str = g_strdup(backing_file_str); > s->bs_read_only = bs_read_only; > Hmm, intersting, is BLK_PERM_GRAPH_MOD a good way to lock relations in node graph? bdrv_replace_node don't check this permission. So, I don't understand, how this permission works.. Graph modification operation in difference with read or write are not done through BdrvChild at all. Are there any ideas or work started for some another mechanism of "freezing" a subgraph during an operation? -- Best regards, Vladimir ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] block-stream: include base into job node list 2019-02-25 16:39 ` Vladimir Sementsov-Ogievskiy @ 2019-02-26 13:33 ` Alberto Garcia 2019-02-26 16:39 ` Andrey Shinkevich 0 siblings, 1 reply; 7+ messages in thread From: Alberto Garcia @ 2019-02-26 13:33 UTC (permalink / raw) To: Vladimir Sementsov-Ogievskiy, Andrey Shinkevich, qemu-devel, qemu-block Cc: mreitz, kwolf, Denis Lunev On Mon 25 Feb 2019 05:39:14 PM CET, Vladimir Sementsov-Ogievskiy wrote: >> --- a/block/stream.c >> +++ b/block/stream.c >> @@ -259,6 +259,15 @@ void stream_start(const char *job_id, BlockDriverState *bs, >> &error_abort); >> } >> >> + if (base) { >> + /* >> + * The base node should not disappear during the job. >> + */ >> + block_job_add_bdrv(&s->common, "base", base, 0, >> + BLK_PERM_ALL & ~BLK_PERM_GRAPH_MOD, >> + &error_abort); >> + } >> + >> s->base = base; >> s->backing_file_str = g_strdup(backing_file_str); >> s->bs_read_only = bs_read_only; >> > > > Hmm, intersting, is BLK_PERM_GRAPH_MOD a good way to lock relations in > node graph? > > bdrv_replace_node don't check this permission. So, I don't understand, > how this permission works.. Graph modification operation in difference > with read or write are not done through BdrvChild at all. > > Are there any ideas or work started for some another mechanism of > "freezing" a subgraph during an operation? See this discussion: https://lists.gnu.org/archive/html/qemu-block/2018-11/msg00387.html And these patches (currently under review): https://lists.gnu.org/archive/html/qemu-block/2019-01/msg00622.html Berto ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] block-stream: include base into job node list 2019-02-26 13:33 ` Alberto Garcia @ 2019-02-26 16:39 ` Andrey Shinkevich 0 siblings, 0 replies; 7+ messages in thread From: Andrey Shinkevich @ 2019-02-26 16:39 UTC (permalink / raw) To: Alberto Garcia, Vladimir Sementsov-Ogievskiy, qemu-devel, qemu-block Cc: mreitz, kwolf, Denis Lunev On 26/02/2019 16:33, Alberto Garcia wrote: > On Mon 25 Feb 2019 05:39:14 PM CET, Vladimir Sementsov-Ogievskiy wrote: >>> --- a/block/stream.c >>> +++ b/block/stream.c >>> @@ -259,6 +259,15 @@ void stream_start(const char *job_id, BlockDriverState *bs, >>> &error_abort); >>> } >>> >>> + if (base) { >>> + /* >>> + * The base node should not disappear during the job. >>> + */ >>> + block_job_add_bdrv(&s->common, "base", base, 0, >>> + BLK_PERM_ALL & ~BLK_PERM_GRAPH_MOD, >>> + &error_abort); >>> + } >>> + >>> s->base = base; >>> s->backing_file_str = g_strdup(backing_file_str); >>> s->bs_read_only = bs_read_only; >>> >> >> >> Hmm, intersting, is BLK_PERM_GRAPH_MOD a good way to lock relations in >> node graph? >> >> bdrv_replace_node don't check this permission. So, I don't understand, >> how this permission works.. Graph modification operation in difference >> with read or write are not done through BdrvChild at all. >> >> Are there any ideas or work started for some another mechanism of >> "freezing" a subgraph during an operation? > > See this discussion: > > https://lists.gnu.org/archive/html/qemu-block/2018-11/msg00387.html > > And these patches (currently under review): > > https://lists.gnu.org/archive/html/qemu-block/2019-01/msg00622.html > > Berto > The bdrv_freeze_backing_chain() excludes the base BlockDriverState. And we would like to protect the base as well when running the block-stream. -- With the best regards, Andrey Shinkevich ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [Qemu-devel] [PATCH 2/2] block-stream: include base into job node list 2019-02-21 15:26 ` [Qemu-devel] [PATCH 2/2] block-stream: include base into job node list Andrey Shinkevich 2019-02-25 16:39 ` Vladimir Sementsov-Ogievskiy @ 2019-03-08 14:00 ` Alberto Garcia 1 sibling, 0 replies; 7+ messages in thread From: Alberto Garcia @ 2019-03-08 14:00 UTC (permalink / raw) To: Andrey Shinkevich, qemu-devel, qemu-block; +Cc: mreitz, kwolf, den, vsementsov On Thu 21 Feb 2019 04:26:39 PM CET, Andrey Shinkevich wrote: > The block-stream job needs to own the base node as the limiter > of the copy-on-read operation. So, the base node is included in > the job node list (block_job_add_bdrv). > Also, the block-stream job would not allow the base node to go > away due to the graph modification, e.g. when a filter node is > inserted between the bottom node and the base node. > For that reason, the flag BLK_PERM_GRAPH_MOD is unset in the > shared permission bit mask of the base node. > > Signed-off-by: Andrey Shinkevich <andrey.shinkevich@virtuozzo.com> > --- > block/stream.c | 9 +++++++++ > 1 file changed, 9 insertions(+) > > diff --git a/block/stream.c b/block/stream.c > index 7a49ac0..c8f93d4 100644 > --- a/block/stream.c > +++ b/block/stream.c > @@ -259,6 +259,15 @@ void stream_start(const char *job_id, BlockDriverState *bs, > &error_abort); > } > > + if (base) { > + /* > + * The base node should not disappear during the job. > + */ > + block_job_add_bdrv(&s->common, "base", base, 0, > + BLK_PERM_ALL & ~BLK_PERM_GRAPH_MOD, > + &error_abort); I'm not sure if I understand the ~BLK_PERM_GRAPH_MOD bit, what's the consequence of not removing that permission? Berto ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2019-03-08 14:00 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-02-21 15:26 [Qemu-devel] [PATCH 0/2] block-stream: include base into job node list Andrey Shinkevich 2019-02-21 15:26 ` [Qemu-devel] [PATCH 1/2] iotests: 030 TestParallelOps non-shared base node Andrey Shinkevich 2019-02-21 15:26 ` [Qemu-devel] [PATCH 2/2] block-stream: include base into job node list Andrey Shinkevich 2019-02-25 16:39 ` Vladimir Sementsov-Ogievskiy 2019-02-26 13:33 ` Alberto Garcia 2019-02-26 16:39 ` Andrey Shinkevich 2019-03-08 14:00 ` Alberto Garcia
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.