[Qemu-devel] [PATCH 0/2] block-stream: include base into job node list

[Qemu-devel] [PATCH 0/2] block-stream: include base into job node list

[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

[Qemu-devel] [PATCH 1/2] iotests: 030 TestParallelOps non-shared base node

[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

[Qemu-devel] [PATCH 2/2] block-stream: include base into job node list

[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

Re: [Qemu-devel] [PATCH 2/2] block-stream: include base into job node list

[Vladimir Sementsov-Ogievskiy]

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

Re: [Qemu-devel] [PATCH 2/2] block-stream: include base into job node list

[Alberto Garcia]

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

Re: [Qemu-devel] [PATCH 2/2] block-stream: include base into job node list

[Andrey Shinkevich]

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

Re: [Qemu-devel] [PATCH 2/2] block-stream: include base into job node list

[Alberto Garcia]

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