[PATCH 0/3] qcow2: relax subclusters allocation dependencies

[PATCH 0/3] qcow2: relax subclusters allocation dependencies

[Vladimir Sementsov-Ogievskiy]

Hi all!

Parallel small writes to unallocated cluster works bad when subclusters
enabled.

Look, without subclusters, one of write requests will allocate the whole
cluster, and all other writes to this cluster will be independent of
each other, they depend only on the first one that does allocation.

With subclusters, each write to unallocated subcluster will block the
whole cluster for parallel writing.

So, assume we write 8 consecutive 4k chunks in parallel:

Without subclusters, one of the chunks will block all the cluster and
write L2 entry. The remaining 7 chunks are written in parallel.

With subclusters, each of the chunks will allocate new subcluster and
block the whole cluster. All the chunks are dependent on each other and
queue depth becomes 1. That's not good.

Let's improve the situation.

Vladimir Sementsov-Ogievskiy (3):
  simplebench: add img_bench_templater.py
  qcow2: refactor handle_dependencies() loop body
  qcow2: handle_dependencies(): relax conflict detection

 block/qcow2-cluster.c                      | 60 +++++++++------
 scripts/simplebench/img_bench_templater.py | 85 ++++++++++++++++++++++
 scripts/simplebench/table_templater.py     | 62 ++++++++++++++++
 tests/qemu-iotests/271                     |  4 +-
 tests/qemu-iotests/271.out                 |  2 -
 5 files changed, 187 insertions(+), 26 deletions(-)
 create mode 100755 scripts/simplebench/img_bench_templater.py
 create mode 100644 scripts/simplebench/table_templater.py

-- 
2.29.2

[PATCH 1/3] simplebench: add img_bench_templater.py

[Vladimir Sementsov-Ogievskiy]

Add simple grammar-parsing template benchmark.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 scripts/simplebench/img_bench_templater.py | 85 ++++++++++++++++++++++
 scripts/simplebench/table_templater.py     | 62 ++++++++++++++++
 2 files changed, 147 insertions(+)
 create mode 100755 scripts/simplebench/img_bench_templater.py
 create mode 100644 scripts/simplebench/table_templater.py

diff --git a/scripts/simplebench/img_bench_templater.py b/scripts/simplebench/img_bench_templater.py
new file mode 100755
index 0000000000..d18a243d35
--- /dev/null
+++ b/scripts/simplebench/img_bench_templater.py
@@ -0,0 +1,85 @@
+#!/usr/bin/env python3
+#
+# Run img-bench template tests
+#
+# Copyright (c) 2021 Virtuozzo International GmbH.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+
+import sys
+import subprocess
+import re
+import json
+
+import simplebench
+from results_to_text import results_to_text
+from table_templater import Templater
+
+
+def bench_func(env, case):
+    test = templater.gen(env['data'], case['data'])
+
+    p = subprocess.run(test, shell=True, stdout=subprocess.PIPE,
+                       stderr=subprocess.STDOUT, universal_newlines=True)
+
+    if p.returncode == 0:
+        try:
+            m = re.search(r'Run completed in (\d+.\d+) seconds.', p.stdout)
+            return {'seconds': float(m.group(1))}
+        except Exception:
+            return {'error': f'failed to parse qemu-img output: {p.stdout}'}
+    else:
+        return {'error': f'qemu-img failed: {p.returncode}: {p.stdout}'}
+
+
+if __name__ == '__main__':
+    if len(sys.argv) > 1:
+        print("""
+Usage: no arguments. Just pass template test to stdin. Template test is
+a bash script, last command should be qemu-img bench (it's output is parsed
+to get a result). For templating use the following synax:
+
+  column templating: {var1|var2|...} - test will use different values in
+  different columns. You may use several {} constructions in the test, in this
+  case product of all choice-sets will be used.
+
+  row templating: [var1|var2|...] - similar thing to define rows (test-cases)
+
+Test tempalate example:
+
+Assume you want to compare two qemu-img binaries, called qemu-img-old and
+qemu-img-new in your build directory in two test-cases with 4K writes and 64K
+writes. Test may look like this:
+
+qemu_img=/path/to/qemu/build/qemu-img-{old|new}
+$qemu_img create -f qcow2 /ssd/x.qcow2 1G
+$qemu_img bench -c 100 -d 8 [-s 4K|-s 64K] -w -t none -n /ssd/x.qcow2
+
+If pass it to stdin of img_bench_templater.py, the resulting comparison table
+will contain two columns (for two binaries) and two rows (for two test-cases).
+""")
+        sys.exit()
+
+    templater = Templater(sys.stdin.read())
+
+    envs = [{'id': ' / '.join(x), 'data': x} for x in templater.columns]
+    cases = [{'id': ' / '.join(x), 'data': x} for x in templater.rows]
+
+    result = simplebench.bench(bench_func, envs, cases, count=5,
+                               initial_run=False)
+    print(results_to_text(result))
+    with open('results.json', 'w') as f:
+        json.dump(result, f, indent=4)
diff --git a/scripts/simplebench/table_templater.py b/scripts/simplebench/table_templater.py
new file mode 100644
index 0000000000..950f3b3024
--- /dev/null
+++ b/scripts/simplebench/table_templater.py
@@ -0,0 +1,62 @@
+# Parser for test templates
+#
+# Copyright (c) 2021 Virtuozzo International GmbH.
+#
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program.  If not, see <http://www.gnu.org/licenses/>.
+#
+
+import itertools
+from lark import Lark
+
+grammar = """
+start: ( text | column_switch | row_switch )+
+
+column_switch: "{" text ["|" text]+ "}"
+row_switch: "[" text ["|" text]+ "]"
+text: /[^|{}\[\]]+/
+"""
+
+parser = Lark(grammar)
+
+class Templater:
+    def __init__(self, template):
+        self.tree = parser.parse(template)
+
+        c_switches = []
+        r_switches = []
+        for x in self.tree.children:
+            if x.data == 'column_switch':
+                c_switches.append([el.children[0].value for el in x.children])
+            elif x.data == 'row_switch':
+                r_switches.append([el.children[0].value for el in x.children])
+
+        self.columns = list(itertools.product(*c_switches))
+        self.rows = list(itertools.product(*r_switches))
+
+    def gen(self, column, row):
+        i = 0
+        j = 0
+        result = []
+
+        for x in self.tree.children:
+            if x.data == 'text':
+                result.append(x.children[0].value)
+            elif x.data == 'column_switch':
+                result.append(column[i])
+                i += 1
+            elif x.data == 'row_switch':
+                result.append(row[j])
+                j += 1
+
+        return ''.join(result)
-- 
2.29.2

[PATCH 2/3] qcow2: refactor handle_dependencies() loop body

[Vladimir Sementsov-Ogievskiy]

No logic change, just prepare for the following commit.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2-cluster.c | 49 ++++++++++++++++++++++++-------------------
 1 file changed, 28 insertions(+), 21 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index bd0597842f..967121c7e6 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1400,29 +1400,36 @@ static int handle_dependencies(BlockDriverState *bs, uint64_t guest_offset,
 
         if (end <= old_start || start >= old_end) {
             /* No intersection */
-        } else {
-            if (start < old_start) {
-                /* Stop at the start of a running allocation */
-                bytes = old_start - start;
-            } else {
-                bytes = 0;
-            }
+            continue;
+        }
 
-            /* Stop if already an l2meta exists. After yielding, it wouldn't
-             * be valid any more, so we'd have to clean up the old L2Metas
-             * and deal with requests depending on them before starting to
-             * gather new ones. Not worth the trouble. */
-            if (bytes == 0 && *m) {
-                *cur_bytes = 0;
-                return 0;
-            }
+        /* Conflict */
 
-            if (bytes == 0) {
-                /* Wait for the dependency to complete. We need to recheck
-                 * the free/allocated clusters when we continue. */
-                qemu_co_queue_wait(&old_alloc->dependent_requests, &s->lock);
-                return -EAGAIN;
-            }
+        if (start < old_start) {
+            /* Stop at the start of a running allocation */
+            bytes = old_start - start;
+        } else {
+            bytes = 0;
+        }
+
+        /*
+         * Stop if already an l2meta exists. After yielding, it wouldn't
+         * be valid any more, so we'd have to clean up the old L2Metas
+         * and deal with requests depending on them before starting to
+         * gather new ones. Not worth the trouble.
+         */
+        if (bytes == 0 && *m) {
+            *cur_bytes = 0;
+            return 0;
+        }
+
+        if (bytes == 0) {
+            /*
+             * Wait for the dependency to complete. We need to recheck
+             * the free/allocated clusters when we continue.
+             */
+            qemu_co_queue_wait(&old_alloc->dependent_requests, &s->lock);
+            return -EAGAIN;
         }
     }
 
-- 
2.29.2

[PATCH 3/3] qcow2: handle_dependencies(): relax conflict detection

[Vladimir Sementsov-Ogievskiy]

There is no conflict and no dependency if we have parallel writes to
different subclusters of one cluster when cluster itself is already
allocated. So, relax extra dependency.

Measure performance:
First, prepare build/qemu-img-old and build/qemu-img-new images.

cd scripts/simplebench
./img_bench_templater.py

Paste the following to stdin of running script:

qemu_img=../../build/qemu-img-{old|new}
$qemu_img create -f qcow2 -o extended_l2=on /ssd/x.qcow2 1G
$qemu_img bench -c 100000 -d 8 [-s 2K|-s 2K -o 512|-s $((1024*2+512))] \
        -w -t none -n /ssd/x.qcow2

The result:

All results are in seconds

------------------  ---------  ---------
                    old        new
-s 2K               6.7 ± 15%  6.2 ± 12%
                                 -7%
-s 2K -o 512        13 ± 3%    11 ± 5%
                                 -16%
-s $((1024*2+512))  9.5 ± 4%   8.4
                                 -12%
------------------  ---------  ---------

So small writes are more independent now and that helps to keep deeper
io queue which improves performance.

271 iotest output becomes racy for three allocation in one cluster.
Second and third writes may finish in different order. Second and
third requests don't depend on each other any more. Still they both
depend on first request anyway. Keep only one for consistent output.

Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
---
 block/qcow2-cluster.c      | 11 +++++++++++
 tests/qemu-iotests/271     |  4 +---
 tests/qemu-iotests/271.out |  2 --
 3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
index 967121c7e6..8f56de5516 100644
--- a/block/qcow2-cluster.c
+++ b/block/qcow2-cluster.c
@@ -1403,6 +1403,17 @@ static int handle_dependencies(BlockDriverState *bs, uint64_t guest_offset,
             continue;
         }
 
+        if (old_alloc->keep_old_clusters &&
+            (end <= l2meta_cow_start(old_alloc) ||
+             start >= l2meta_cow_end(old_alloc)))
+        {
+            /*
+             * Clusters intersect but COW areas don't. And cluster itself is
+             * already allocated. So, there is no actual conflict.
+             */
+            continue;
+        }
+
         /* Conflict */
 
         if (start < old_start) {
diff --git a/tests/qemu-iotests/271 b/tests/qemu-iotests/271
index 599b849cc6..939e88ee88 100755
--- a/tests/qemu-iotests/271
+++ b/tests/qemu-iotests/271
@@ -866,7 +866,7 @@ echo
 
 _concurrent_io()
 {
-# Allocate three subclusters in the same cluster.
+# Allocate two subclusters in the same cluster.
 # This works because handle_dependencies() checks whether the requests
 # allocate the same cluster, even if the COW regions don't overlap (in
 # this case they don't).
@@ -876,7 +876,6 @@ break write_aio A
 aio_write -P 10 30k 2k
 wait_break A
 aio_write -P 11 20k 2k
-aio_write -P 12 40k 2k
 resume A
 aio_flush
 EOF
@@ -888,7 +887,6 @@ cat <<EOF
 open -o driver=$IMGFMT $TEST_IMG
 read -q -P 10 30k 2k
 read -q -P 11 20k 2k
-read -q -P 12 40k 2k
 EOF
 }
 
diff --git a/tests/qemu-iotests/271.out b/tests/qemu-iotests/271.out
index 81043ba4d7..d94c8fe061 100644
--- a/tests/qemu-iotests/271.out
+++ b/tests/qemu-iotests/271.out
@@ -721,6 +721,4 @@ wrote 2048/2048 bytes at offset 30720
 2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 wrote 2048/2048 bytes at offset 20480
 2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-wrote 2048/2048 bytes at offset 40960
-2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 *** done
-- 
2.29.2

Re: [PATCH 1/3] simplebench: add img_bench_templater.py

[Hanna Reitz]

On 24.07.21 15:38, Vladimir Sementsov-Ogievskiy wrote:
> Add simple grammar-parsing template benchmark.

This doesn’t really say much, and FWIW, for like ten minutes I thought 
this would do something completely different than it did (while I was 
trying to parse the help text).

(I thought this was about formatting an existing test’s output, and that 
“template” were kind of the wrong word, but then it turned out it’s 
exactly the right word, only that this is not about using a test’s 
output as a template, but actually using a template of a test (i.e. a 
test template, not a template test) to generate test instances to 
run...  Which of course is much cooler.)

Functionality-wise, as far as I understand (of course I have no 
knowledge of lark), this looks good to me.  And it’s really quite cool.

I just found the documentation confusing, so I have some suggestions for 
it below.

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   scripts/simplebench/img_bench_templater.py | 85 ++++++++++++++++++++++
>   scripts/simplebench/table_templater.py     | 62 ++++++++++++++++
>   2 files changed, 147 insertions(+)
>   create mode 100755 scripts/simplebench/img_bench_templater.py
>   create mode 100644 scripts/simplebench/table_templater.py
>
> diff --git a/scripts/simplebench/img_bench_templater.py b/scripts/simplebench/img_bench_templater.py
> new file mode 100755
> index 0000000000..d18a243d35
> --- /dev/null
> +++ b/scripts/simplebench/img_bench_templater.py
> @@ -0,0 +1,85 @@
> +#!/usr/bin/env python3
> +#
> +# Run img-bench template tests
> +#
> +# Copyright (c) 2021 Virtuozzo International GmbH.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +
> +import sys
> +import subprocess
> +import re
> +import json
> +
> +import simplebench
> +from results_to_text import results_to_text
> +from table_templater import Templater
> +
> +
> +def bench_func(env, case):
> +    test = templater.gen(env['data'], case['data'])
> +
> +    p = subprocess.run(test, shell=True, stdout=subprocess.PIPE,
> +                       stderr=subprocess.STDOUT, universal_newlines=True)
> +
> +    if p.returncode == 0:
> +        try:
> +            m = re.search(r'Run completed in (\d+.\d+) seconds.', p.stdout)
> +            return {'seconds': float(m.group(1))}
> +        except Exception:
> +            return {'error': f'failed to parse qemu-img output: {p.stdout}'}
> +    else:
> +        return {'error': f'qemu-img failed: {p.returncode}: {p.stdout}'}
> +
> +
> +if __name__ == '__main__':
> +    if len(sys.argv) > 1:
> +        print("""
> +Usage: no arguments. Just pass template test to stdin. Template test is

FWIW, I completely misunderstood this.

At first, this sounded really ambiguous to me; then I thought that 
clearly this must mean that one should pipe the test’s output to this 
script, i.e.

$ path/to/test.sh | scripts/simplebench/img_bench_templater.py

But now after reading more, I finally understand that this is not what 
is meant, but actually literally passing some template of a test script 
to this script, i.e.

$ scripts/simplebench/img_bench_templater.py < path/to/test-template.sh

So, two things; first, I believe it should be a “test template”, not a 
“template test”, because this is about templates for a test, not about a 
test that has something to do with templates.

Second, perhaps we should start with what this does.

Perhaps:

“This script generates performance tests from a test template (example 
below), runs them, and displays the results in a table. The template is 
read from stdin.  It must be written in bash and end with a `qemu-img 
bench` invocation (whose result is parsed to get the test instance’s 
result).”?

> +a bash script, last command should be qemu-img bench (it's output is parsed
> +to get a result). For templating use the following synax:

“Use the following syntax in the template to create the various 
different test instances:”?

> +
> +  column templating: {var1|var2|...} - test will use different values in
> +  different columns. You may use several {} constructions in the test, in this
> +  case product of all choice-sets will be used.
> +
> +  row templating: [var1|var2|...] - similar thing to define rows (test-cases)
> +
> +Test tempalate example:

*template

> +
> +Assume you want to compare two qemu-img binaries, called qemu-img-old and
> +qemu-img-new in your build directory in two test-cases with 4K writes and 64K
> +writes. Test may look like this:

I’d prefer s/Test/The template/.

> +
> +qemu_img=/path/to/qemu/build/qemu-img-{old|new}
> +$qemu_img create -f qcow2 /ssd/x.qcow2 1G
> +$qemu_img bench -c 100 -d 8 [-s 4K|-s 64K] -w -t none -n /ssd/x.qcow2
> +
> +If pass it to stdin of img_bench_templater.py, the resulting comparison table

s/If pass it/When passing this/

> +will contain two columns (for two binaries) and two rows (for two test-cases).
> +""")
> +        sys.exit()
> +
> +    templater = Templater(sys.stdin.read())
> +
> +    envs = [{'id': ' / '.join(x), 'data': x} for x in templater.columns]
> +    cases = [{'id': ' / '.join(x), 'data': x} for x in templater.rows]
> +
> +    result = simplebench.bench(bench_func, envs, cases, count=5,
> +                               initial_run=False)
> +    print(results_to_text(result))
> +    with open('results.json', 'w') as f:
> +        json.dump(result, f, indent=4)

Is this worth documenting?

> diff --git a/scripts/simplebench/table_templater.py b/scripts/simplebench/table_templater.py
> new file mode 100644
> index 0000000000..950f3b3024
> --- /dev/null
> +++ b/scripts/simplebench/table_templater.py
> @@ -0,0 +1,62 @@
> +# Parser for test templates
> +#
> +# Copyright (c) 2021 Virtuozzo International GmbH.
> +#
> +# This program is free software; you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation; either version 2 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
> +#
> +
> +import itertools
> +from lark import Lark
> +
> +grammar = """
> +start: ( text | column_switch | row_switch )+
> +
> +column_switch: "{" text ["|" text]+ "}"
> +row_switch: "[" text ["|" text]+ "]"
> +text: /[^|{}\[\]]+/

So I have no idea how this really works, of course, but does this mean 
that the `text` pattern cannot contain pipe symbols?  I.e. that you 
cannot use pipes in the test template?

Hanna

> +"""
> +
> +parser = Lark(grammar)
> +
> +class Templater:
> +    def __init__(self, template):
> +        self.tree = parser.parse(template)
> +
> +        c_switches = []
> +        r_switches = []
> +        for x in self.tree.children:
> +            if x.data == 'column_switch':
> +                c_switches.append([el.children[0].value for el in x.children])
> +            elif x.data == 'row_switch':
> +                r_switches.append([el.children[0].value for el in x.children])
> +
> +        self.columns = list(itertools.product(*c_switches))
> +        self.rows = list(itertools.product(*r_switches))
> +
> +    def gen(self, column, row):
> +        i = 0
> +        j = 0
> +        result = []
> +
> +        for x in self.tree.children:
> +            if x.data == 'text':
> +                result.append(x.children[0].value)
> +            elif x.data == 'column_switch':
> +                result.append(column[i])
> +                i += 1
> +            elif x.data == 'row_switch':
> +                result.append(row[j])
> +                j += 1
> +
> +        return ''.join(result)

Re: [PATCH 2/3] qcow2: refactor handle_dependencies() loop body

[Eric Blake]

On Sat, Jul 24, 2021 at 04:38:45PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> No logic change, just prepare for the following commit.
> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/qcow2-cluster.c | 49 ++++++++++++++++++++++++-------------------
>  1 file changed, 28 insertions(+), 21 deletions(-)
> 
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index bd0597842f..967121c7e6 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1400,29 +1400,36 @@ static int handle_dependencies(BlockDriverState *bs, uint64_t guest_offset,
>  
>          if (end <= old_start || start >= old_end) {
>              /* No intersection */
> -        } else {
> -            if (start < old_start) {
> -                /* Stop at the start of a running allocation */
> -                bytes = old_start - start;
> -            } else {
> -                bytes = 0;
> -            }
> +            continue;
> +        }
>  
> -            /* Stop if already an l2meta exists. After yielding, it wouldn't

Pre-existing, but...

> -             * be valid any more, so we'd have to clean up the old L2Metas
> -             * and deal with requests depending on them before starting to
> -             * gather new ones. Not worth the trouble. */
> -            if (bytes == 0 && *m) {
> -                *cur_bytes = 0;
> -                return 0;
> -            }
> +        /* Conflict */
>  
> -            if (bytes == 0) {
> -                /* Wait for the dependency to complete. We need to recheck
> -                 * the free/allocated clusters when we continue. */
> -                qemu_co_queue_wait(&old_alloc->dependent_requests, &s->lock);
> -                return -EAGAIN;
> -            }
> +        if (start < old_start) {
> +            /* Stop at the start of a running allocation */
> +            bytes = old_start - start;
> +        } else {
> +            bytes = 0;
> +        }
> +
> +        /*
> +         * Stop if already an l2meta exists. After yielding, it wouldn't

...might as well fix the grammar:  Stop if an l2meta already exists.

> +         * be valid any more, so we'd have to clean up the old L2Metas
> +         * and deal with requests depending on them before starting to
> +         * gather new ones. Not worth the trouble.
> +         */
> +        if (bytes == 0 && *m) {
> +            *cur_bytes = 0;
> +            return 0;
> +        }
> +
> +        if (bytes == 0) {
> +            /*
> +             * Wait for the dependency to complete. We need to recheck
> +             * the free/allocated clusters when we continue.
> +             */
> +            qemu_co_queue_wait(&old_alloc->dependent_requests, &s->lock);
> +            return -EAGAIN;

So the change adds a short-circuiting 'continue', then reduces the
indentation of the rest of the loop body.

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 3/3] qcow2: handle_dependencies(): relax conflict detection

[Eric Blake]

On Sat, Jul 24, 2021 at 04:38:46PM +0300, Vladimir Sementsov-Ogievskiy wrote:
> There is no conflict and no dependency if we have parallel writes to
> different subclusters of one cluster when cluster itself is already

when the cluster itself

> allocated. So, relax extra dependency.
> 
> Measure performance:
> First, prepare build/qemu-img-old and build/qemu-img-new images.
> 
> cd scripts/simplebench
> ./img_bench_templater.py
> 
> Paste the following to stdin of running script:
> 
> qemu_img=../../build/qemu-img-{old|new}
> $qemu_img create -f qcow2 -o extended_l2=on /ssd/x.qcow2 1G
> $qemu_img bench -c 100000 -d 8 [-s 2K|-s 2K -o 512|-s $((1024*2+512))] \
>         -w -t none -n /ssd/x.qcow2
> 
> The result:
> 
> All results are in seconds
> 
> ------------------  ---------  ---------
>                     old        new
> -s 2K               6.7 ± 15%  6.2 ± 12%
>                                  -7%
> -s 2K -o 512        13 ± 3%    11 ± 5%
>                                  -16%
> -s $((1024*2+512))  9.5 ± 4%   8.4
>                                  -12%
> ------------------  ---------  ---------

Cool improvement.

> 
> So small writes are more independent now and that helps to keep deeper
> io queue which improves performance.
> 
> 271 iotest output becomes racy for three allocation in one cluster.
> Second and third writes may finish in different order. Second and
> third requests don't depend on each other any more. Still they both
> depend on first request anyway. Keep only one for consistent output.

Interesting fallout.  Yes, it looks like the test is still robust
enough without the extra request.

> 
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>  block/qcow2-cluster.c      | 11 +++++++++++
>  tests/qemu-iotests/271     |  4 +---
>  tests/qemu-iotests/271.out |  2 --
>  3 files changed, 12 insertions(+), 5 deletions(-)

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

Re: [PATCH 2/3] qcow2: refactor handle_dependencies() loop body

[Hanna Reitz]

On 24.07.21 15:38, Vladimir Sementsov-Ogievskiy wrote:
> No logic change, just prepare for the following commit.
>
> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   block/qcow2-cluster.c | 49 ++++++++++++++++++++++++-------------------
>   1 file changed, 28 insertions(+), 21 deletions(-)

Reviewed-by: Hanna Reitz <hreitz@redhat.com>

Re: [PATCH 3/3] qcow2: handle_dependencies(): relax conflict detection

[Hanna Reitz]

On 24.07.21 15:38, Vladimir Sementsov-Ogievskiy wrote:
> There is no conflict and no dependency if we have parallel writes to
> different subclusters of one cluster when cluster itself is already
> allocated. So, relax extra dependency.
>
> Measure performance:
> First, prepare build/qemu-img-old and build/qemu-img-new images.
>
> cd scripts/simplebench
> ./img_bench_templater.py
>
> Paste the following to stdin of running script:
>
> qemu_img=../../build/qemu-img-{old|new}
> $qemu_img create -f qcow2 -o extended_l2=on /ssd/x.qcow2 1G
> $qemu_img bench -c 100000 -d 8 [-s 2K|-s 2K -o 512|-s $((1024*2+512))] \
>          -w -t none -n /ssd/x.qcow2
>
> The result:
>
> All results are in seconds
>
> ------------------  ---------  ---------
>                      old        new
> -s 2K               6.7 ± 15%  6.2 ± 12%
>                                   -7%
> -s 2K -o 512        13 ± 3%    11 ± 5%
>                                   -16%
> -s $((1024*2+512))  9.5 ± 4%   8.4
>                                   -12%
> ------------------  ---------  ---------
>
> So small writes are more independent now and that helps to keep deeper
> io queue which improves performance.
>
> 271 iotest output becomes racy for three allocation in one cluster.
> Second and third writes may finish in different order. Second and
> third requests don't depend on each other any more. Still they both
> depend on first request anyway. Keep only one for consistent output.

I mean, we could also just filter the result 
(`s/\(20480\|40960\)/FILTERED/` or something).  Perhaps there was some 
idea behind doing three writes, I don’t know exactly.

I think I’d prefer a filter, because I guess this is the only test that 
actually will do two subcluster requests in parallel...?

Hanna

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
> ---
>   block/qcow2-cluster.c      | 11 +++++++++++
>   tests/qemu-iotests/271     |  4 +---
>   tests/qemu-iotests/271.out |  2 --
>   3 files changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
> index 967121c7e6..8f56de5516 100644
> --- a/block/qcow2-cluster.c
> +++ b/block/qcow2-cluster.c
> @@ -1403,6 +1403,17 @@ static int handle_dependencies(BlockDriverState *bs, uint64_t guest_offset,
>               continue;
>           }
>   
> +        if (old_alloc->keep_old_clusters &&
> +            (end <= l2meta_cow_start(old_alloc) ||
> +             start >= l2meta_cow_end(old_alloc)))
> +        {
> +            /*
> +             * Clusters intersect but COW areas don't. And cluster itself is
> +             * already allocated. So, there is no actual conflict.
> +             */
> +            continue;
> +        }
> +
>           /* Conflict */
>   
>           if (start < old_start) {
> diff --git a/tests/qemu-iotests/271 b/tests/qemu-iotests/271
> index 599b849cc6..939e88ee88 100755
> --- a/tests/qemu-iotests/271
> +++ b/tests/qemu-iotests/271
> @@ -866,7 +866,7 @@ echo
>   
>   _concurrent_io()
>   {
> -# Allocate three subclusters in the same cluster.
> +# Allocate two subclusters in the same cluster.
>   # This works because handle_dependencies() checks whether the requests
>   # allocate the same cluster, even if the COW regions don't overlap (in
>   # this case they don't).
> @@ -876,7 +876,6 @@ break write_aio A
>   aio_write -P 10 30k 2k
>   wait_break A
>   aio_write -P 11 20k 2k
> -aio_write -P 12 40k 2k
>   resume A
>   aio_flush
>   EOF
> @@ -888,7 +887,6 @@ cat <<EOF
>   open -o driver=$IMGFMT $TEST_IMG
>   read -q -P 10 30k 2k
>   read -q -P 11 20k 2k
> -read -q -P 12 40k 2k
>   EOF
>   }
>   
> diff --git a/tests/qemu-iotests/271.out b/tests/qemu-iotests/271.out
> index 81043ba4d7..d94c8fe061 100644
> --- a/tests/qemu-iotests/271.out
> +++ b/tests/qemu-iotests/271.out
> @@ -721,6 +721,4 @@ wrote 2048/2048 bytes at offset 30720
>   2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>   wrote 2048/2048 bytes at offset 20480
>   2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> -wrote 2048/2048 bytes at offset 40960
> -2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>   *** done

Re: [PATCH 3/3] qcow2: handle_dependencies(): relax conflict detection

[Vladimir Sementsov-Ogievskiy]

20.08.2021 16:21, Hanna Reitz wrote:
> On 24.07.21 15:38, Vladimir Sementsov-Ogievskiy wrote:
>> There is no conflict and no dependency if we have parallel writes to
>> different subclusters of one cluster when cluster itself is already
>> allocated. So, relax extra dependency.
>>
>> Measure performance:
>> First, prepare build/qemu-img-old and build/qemu-img-new images.
>>
>> cd scripts/simplebench
>> ./img_bench_templater.py
>>
>> Paste the following to stdin of running script:
>>
>> qemu_img=../../build/qemu-img-{old|new}
>> $qemu_img create -f qcow2 -o extended_l2=on /ssd/x.qcow2 1G
>> $qemu_img bench -c 100000 -d 8 [-s 2K|-s 2K -o 512|-s $((1024*2+512))] \
>>          -w -t none -n /ssd/x.qcow2
>>
>> The result:
>>
>> All results are in seconds
>>
>> ------------------  ---------  ---------
>>                      old        new
>> -s 2K               6.7 ± 15%  6.2 ± 12%
>>                                   -7%
>> -s 2K -o 512        13 ± 3%    11 ± 5%
>>                                   -16%
>> -s $((1024*2+512))  9.5 ± 4%   8.4
>>                                   -12%
>> ------------------  ---------  ---------
>>
>> So small writes are more independent now and that helps to keep deeper
>> io queue which improves performance.
>>
>> 271 iotest output becomes racy for three allocation in one cluster.
>> Second and third writes may finish in different order. Second and
>> third requests don't depend on each other any more. Still they both
>> depend on first request anyway. Keep only one for consistent output.
> 
> I mean, we could also just filter the result (`s/\(20480\|40960\)/FILTERED/` or something).  Perhaps there was some idea behind doing three writes, I don’t know exactly.
> 
> I think I’d prefer a filter, because I guess this is the only test that actually will do two subcluster requests in parallel...?
> 

Reasonable, will do

> 
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   block/qcow2-cluster.c      | 11 +++++++++++
>>   tests/qemu-iotests/271     |  4 +---
>>   tests/qemu-iotests/271.out |  2 --
>>   3 files changed, 12 insertions(+), 5 deletions(-)
>>
>> diff --git a/block/qcow2-cluster.c b/block/qcow2-cluster.c
>> index 967121c7e6..8f56de5516 100644
>> --- a/block/qcow2-cluster.c
>> +++ b/block/qcow2-cluster.c
>> @@ -1403,6 +1403,17 @@ static int handle_dependencies(BlockDriverState *bs, uint64_t guest_offset,
>>               continue;
>>           }
>> +        if (old_alloc->keep_old_clusters &&
>> +            (end <= l2meta_cow_start(old_alloc) ||
>> +             start >= l2meta_cow_end(old_alloc)))
>> +        {
>> +            /*
>> +             * Clusters intersect but COW areas don't. And cluster itself is
>> +             * already allocated. So, there is no actual conflict.
>> +             */
>> +            continue;
>> +        }
>> +
>>           /* Conflict */
>>           if (start < old_start) {
>> diff --git a/tests/qemu-iotests/271 b/tests/qemu-iotests/271
>> index 599b849cc6..939e88ee88 100755
>> --- a/tests/qemu-iotests/271
>> +++ b/tests/qemu-iotests/271
>> @@ -866,7 +866,7 @@ echo
>>   _concurrent_io()
>>   {
>> -# Allocate three subclusters in the same cluster.
>> +# Allocate two subclusters in the same cluster.
>>   # This works because handle_dependencies() checks whether the requests
>>   # allocate the same cluster, even if the COW regions don't overlap (in
>>   # this case they don't).
>> @@ -876,7 +876,6 @@ break write_aio A
>>   aio_write -P 10 30k 2k
>>   wait_break A
>>   aio_write -P 11 20k 2k
>> -aio_write -P 12 40k 2k
>>   resume A
>>   aio_flush
>>   EOF
>> @@ -888,7 +887,6 @@ cat <<EOF
>>   open -o driver=$IMGFMT $TEST_IMG
>>   read -q -P 10 30k 2k
>>   read -q -P 11 20k 2k
>> -read -q -P 12 40k 2k
>>   EOF
>>   }
>> diff --git a/tests/qemu-iotests/271.out b/tests/qemu-iotests/271.out
>> index 81043ba4d7..d94c8fe061 100644
>> --- a/tests/qemu-iotests/271.out
>> +++ b/tests/qemu-iotests/271.out
>> @@ -721,6 +721,4 @@ wrote 2048/2048 bytes at offset 30720
>>   2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>>   wrote 2048/2048 bytes at offset 20480
>>   2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>> -wrote 2048/2048 bytes at offset 40960
>> -2 KiB, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>>   *** done
> 


-- 
Best regards,
Vladimir

Re: [PATCH 1/3] simplebench: add img_bench_templater.py

[Vladimir Sementsov-Ogievskiy]

19.08.2021 19:37, Hanna Reitz wrote:
> On 24.07.21 15:38, Vladimir Sementsov-Ogievskiy wrote:
>> Add simple grammar-parsing template benchmark.
> 
> This doesn’t really say much, and FWIW, for like ten minutes I thought this would do something completely different than it did (while I was trying to parse the help text).
> 
> (I thought this was about formatting an existing test’s output, and that “template” were kind of the wrong word, but then it turned out it’s exactly the right word, only that this is not about using a test’s output as a template, but actually using a template of a test (i.e. a test template, not a template test) to generate test instances to run...  Which of course is much cooler.)
> 
> Functionality-wise, as far as I understand (of course I have no knowledge of lark), this looks good to me.  And it’s really quite cool.
> 
> I just found the documentation confusing, so I have some suggestions for it below.
> 
>> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@virtuozzo.com>
>> ---
>>   scripts/simplebench/img_bench_templater.py | 85 ++++++++++++++++++++++
>>   scripts/simplebench/table_templater.py     | 62 ++++++++++++++++
>>   2 files changed, 147 insertions(+)
>>   create mode 100755 scripts/simplebench/img_bench_templater.py
>>   create mode 100644 scripts/simplebench/table_templater.py
>>
>> diff --git a/scripts/simplebench/img_bench_templater.py b/scripts/simplebench/img_bench_templater.py
>> new file mode 100755
>> index 0000000000..d18a243d35
>> --- /dev/null
>> +++ b/scripts/simplebench/img_bench_templater.py
>> @@ -0,0 +1,85 @@
>> +#!/usr/bin/env python3
>> +#
>> +# Run img-bench template tests
>> +#
>> +# Copyright (c) 2021 Virtuozzo International GmbH.
>> +#
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 2 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> +#
>> +
>> +
>> +import sys
>> +import subprocess
>> +import re
>> +import json
>> +
>> +import simplebench
>> +from results_to_text import results_to_text
>> +from table_templater import Templater
>> +
>> +
>> +def bench_func(env, case):
>> +    test = templater.gen(env['data'], case['data'])
>> +
>> +    p = subprocess.run(test, shell=True, stdout=subprocess.PIPE,
>> +                       stderr=subprocess.STDOUT, universal_newlines=True)
>> +
>> +    if p.returncode == 0:
>> +        try:
>> +            m = re.search(r'Run completed in (\d+.\d+) seconds.', p.stdout)
>> +            return {'seconds': float(m.group(1))}
>> +        except Exception:
>> +            return {'error': f'failed to parse qemu-img output: {p.stdout}'}
>> +    else:
>> +        return {'error': f'qemu-img failed: {p.returncode}: {p.stdout}'}
>> +
>> +
>> +if __name__ == '__main__':
>> +    if len(sys.argv) > 1:
>> +        print("""
>> +Usage: no arguments. Just pass template test to stdin. Template test is
> 
> FWIW, I completely misunderstood this.
> 
> At first, this sounded really ambiguous to me; then I thought that clearly this must mean that one should pipe the test’s output to this script, i.e.
> 
> $ path/to/test.sh | scripts/simplebench/img_bench_templater.py
> 
> But now after reading more, I finally understand that this is not what is meant, but actually literally passing some template of a test script to this script, i.e.
> 
> $ scripts/simplebench/img_bench_templater.py < path/to/test-template.sh
> 
> So, two things; first, I believe it should be a “test template”, not a “template test”, because this is about templates for a test, not about a test that has something to do with templates.
> 
> Second, perhaps we should start with what this does.
> 
> Perhaps:
> 
> “This script generates performance tests from a test template (example below), runs them, and displays the results in a table. The template is read from stdin.  It must be written in bash and end with a `qemu-img bench` invocation (whose result is parsed to get the test instance’s result).”?

Yes, that's correct, thanks for wording

> 
>> +a bash script, last command should be qemu-img bench (it's output is parsed
>> +to get a result). For templating use the following synax:
> 
> “Use the following syntax in the template to create the various different test instances:”?
> 
>> +
>> +  column templating: {var1|var2|...} - test will use different values in
>> +  different columns. You may use several {} constructions in the test, in this
>> +  case product of all choice-sets will be used.
>> +
>> +  row templating: [var1|var2|...] - similar thing to define rows (test-cases)
>> +
>> +Test tempalate example:
> 
> *template
> 
>> +
>> +Assume you want to compare two qemu-img binaries, called qemu-img-old and
>> +qemu-img-new in your build directory in two test-cases with 4K writes and 64K
>> +writes. Test may look like this:
> 
> I’d prefer s/Test/The template/.
> 
>> +
>> +qemu_img=/path/to/qemu/build/qemu-img-{old|new}
>> +$qemu_img create -f qcow2 /ssd/x.qcow2 1G
>> +$qemu_img bench -c 100 -d 8 [-s 4K|-s 64K] -w -t none -n /ssd/x.qcow2
>> +
>> +If pass it to stdin of img_bench_templater.py, the resulting comparison table
> 
> s/If pass it/When passing this/
> 
>> +will contain two columns (for two binaries) and two rows (for two test-cases).
>> +""")
>> +        sys.exit()
>> +
>> +    templater = Templater(sys.stdin.read())
>> +
>> +    envs = [{'id': ' / '.join(x), 'data': x} for x in templater.columns]
>> +    cases = [{'id': ' / '.join(x), 'data': x} for x in templater.rows]
>> +
>> +    result = simplebench.bench(bench_func, envs, cases, count=5,
>> +                               initial_run=False)
>> +    print(results_to_text(result))
>> +    with open('results.json', 'w') as f:
>> +        json.dump(result, f, indent=4)
> 
> Is this worth documenting?
> 
>> diff --git a/scripts/simplebench/table_templater.py b/scripts/simplebench/table_templater.py
>> new file mode 100644
>> index 0000000000..950f3b3024
>> --- /dev/null
>> +++ b/scripts/simplebench/table_templater.py
>> @@ -0,0 +1,62 @@
>> +# Parser for test templates
>> +#
>> +# Copyright (c) 2021 Virtuozzo International GmbH.
>> +#
>> +# This program is free software; you can redistribute it and/or modify
>> +# it under the terms of the GNU General Public License as published by
>> +# the Free Software Foundation; either version 2 of the License, or
>> +# (at your option) any later version.
>> +#
>> +# This program is distributed in the hope that it will be useful,
>> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
>> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
>> +# GNU General Public License for more details.
>> +#
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program.  If not, see <http://www.gnu.org/licenses/>.
>> +#
>> +
>> +import itertools
>> +from lark import Lark
>> +
>> +grammar = """
>> +start: ( text | column_switch | row_switch )+
>> +
>> +column_switch: "{" text ["|" text]+ "}"
>> +row_switch: "[" text ["|" text]+ "]"
>> +text: /[^|{}\[\]]+/
> 
> So I have no idea how this really works, of course, but does this mean that the `text` pattern cannot contain pipe symbols?  I.e. that you cannot use pipes in the test template?
> 

Hmm. I didn't try. I hope lark is smart enough to keep pipes that are out of {} [] as is.. But of course, you can't hope that pipe inside {} or [] will work as bash-pipe.

Same thing with other special symbols ("{}" and "[]"). I don't want to care about this too much now. This simple grammar works good for test template in patch 03. If we need something more, we can add a kind of special symbols escaping later.

> 
>> +"""
>> +
>> +parser = Lark(grammar)
>> +
>> +class Templater:
>> +    def __init__(self, template):
>> +        self.tree = parser.parse(template)
>> +
>> +        c_switches = []
>> +        r_switches = []
>> +        for x in self.tree.children:
>> +            if x.data == 'column_switch':
>> +                c_switches.append([el.children[0].value for el in x.children])
>> +            elif x.data == 'row_switch':
>> +                r_switches.append([el.children[0].value for el in x.children])
>> +
>> +        self.columns = list(itertools.product(*c_switches))
>> +        self.rows = list(itertools.product(*r_switches))
>> +
>> +    def gen(self, column, row):
>> +        i = 0
>> +        j = 0
>> +        result = []
>> +
>> +        for x in self.tree.children:
>> +            if x.data == 'text':
>> +                result.append(x.children[0].value)
>> +            elif x.data == 'column_switch':
>> +                result.append(column[i])
>> +                i += 1
>> +            elif x.data == 'row_switch':
>> +                result.append(row[j])
>> +                j += 1
>> +
>> +        return ''.join(result)
> 


-- 
Best regards,
Vladimir

Re: [PATCH 1/3] simplebench: add img_bench_templater.py

[Hanna Reitz]

On 24.08.21 10:53, Vladimir Sementsov-Ogievskiy wrote:
> 19.08.2021 19:37, Hanna Reitz wrote:
>> On 24.07.21 15:38, Vladimir Sementsov-Ogievskiy wrote:

[...]

>> +import itertools
>> +from lark import Lark
>> +
>> +grammar = """
>> +start: ( text | column_switch | row_switch )+
>> +
>> +column_switch: "{" text ["|" text]+ "}"
>> +row_switch: "[" text ["|" text]+ "]"
>> +text: /[^|{}\[\]]+/
>>
>> So I have no idea how this really works, of course, but does this 
>> mean that the `text` pattern cannot contain pipe symbols? I.e. that 
>> you cannot use pipes in the test template?
>>
>
> Hmm. I didn't try. I hope lark is smart enough to keep pipes that are 
> out of {} [] as is.. But of course, you can't hope that pipe inside {} 
> or [] will work as bash-pipe.

Yep, sure.  It’s just that the `text` nonterminal symbol doesn’t look 
like it could match anything with a pipe in it.

> Same thing with other special symbols ("{}" and "[]"). I don't want to 
> care about this too much now. This simple grammar works good for test 
> template in patch 03. If we need something more, we can add a kind of 
> special symbols escaping later.

But yes, if someone trips over this (i.e. we ourselves), we can still 
fix it then.

Hanna

Re: [PATCH 1/3] simplebench: add img_bench_templater.py

[Vladimir Sementsov-Ogievskiy]

24.08.2021 11:59, Hanna Reitz wrote:
> On 24.08.21 10:53, Vladimir Sementsov-Ogievskiy wrote:
>> 19.08.2021 19:37, Hanna Reitz wrote:
>>> On 24.07.21 15:38, Vladimir Sementsov-Ogievskiy wrote:
> 
> [...]
> 
>>> +import itertools
>>> +from lark import Lark
>>> +
>>> +grammar = """
>>> +start: ( text | column_switch | row_switch )+
>>> +
>>> +column_switch: "{" text ["|" text]+ "}"
>>> +row_switch: "[" text ["|" text]+ "]"
>>> +text: /[^|{}\[\]]+/
>>>
>>> So I have no idea how this really works, of course, but does this mean that the `text` pattern cannot contain pipe symbols? I.e. that you cannot use pipes in the test template?
>>>
>>
>> Hmm. I didn't try. I hope lark is smart enough to keep pipes that are out of {} [] as is.. But of course, you can't hope that pipe inside {} or [] will work as bash-pipe.
> 
> Yep, sure.  It’s just that the `text` nonterminal symbol doesn’t look like it could match anything with a pipe in it.

Oops, right. To avoid it, we'll have to split text into "text_inside_brackets" and "text_outside_of_brackets".. As well, let's postpone this complication until we really need it.

> 
>> Same thing with other special symbols ("{}" and "[]"). I don't want to care about this too much now. This simple grammar works good for test template in patch 03. If we need something more, we can add a kind of special symbols escaping later.
> 
> But yes, if someone trips over this (i.e. we ourselves), we can still fix it then.
> 
> Hanna
> 


-- 
Best regards,
Vladimir