[PATCH 1/3] DDF tests: allow to run on systems without /dev/sda

[PATCH 1/3] DDF tests: allow to run on systems without /dev/sda

[mwilck]

Some ddf tests scripts assume that /dev/sda is always present.
That's wrong e.g. on VMs. Use a more general approach.
---
 tests/10ddf-create          |   10 ++++++----
 tests/10ddf-fail-two-spares |    5 +++--
 tests/env-ddf-template      |    9 +++++++++
 3 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/tests/10ddf-create b/tests/10ddf-create
index 50c85ae..2f7747c 100644
--- a/tests/10ddf-create
+++ b/tests/10ddf-create
@@ -9,6 +9,8 @@
 # add some data, tear down the array, reassemble
 # and make sure it is still there.
 set -e
+. tests/env-ddf-template
+sda=$(get_rootdev) || exit 1
 
 mdadm -CR /dev/md/ddf0 -e ddf -n 5 $dev8 $dev9 $dev10 $dev11 $dev12
 mdadm -CR r5 -l5 -n5 /dev/md/ddf0 -z 5000
@@ -23,10 +25,10 @@ testdev /dev/md/r10 2 5000 512
 # r0/r10 will use 4608 due to chunk size, so that leaves 23552 for the rest
 testdev /dev/md/r1 1 23552 64
 testdev /dev/md/r0 3 23552 512
-dd if=/dev/sda of=/dev/md/r0 || true
-dd if=/dev/sda of=/dev/md/r10 || true
-dd if=/dev/sda of=/dev/md/r1 || true
-dd if=/dev/sda of=/dev/md/r5 || true
+dd if=$sda of=/dev/md/r0 || true
+dd if=$sda of=/dev/md/r10 || true
+dd if=$sda of=/dev/md/r1 || true
+dd if=$sda of=/dev/md/r5 || true
 
 s0=`sha1sum /dev/md/r0`
 s10=`sha1sum /dev/md/r10`
diff --git a/tests/10ddf-fail-two-spares b/tests/10ddf-fail-two-spares
index cc2cbb4..fa6e2e8 100644
--- a/tests/10ddf-fail-two-spares
+++ b/tests/10ddf-fail-two-spares
@@ -1,5 +1,6 @@
 # Simulate two disks failing shorty after each other
 . tests/env-ddf-template
+sda=$(get_rootdev) || exit 1
 tmp=$(mktemp /tmp/mdtest-XXXXXX)
 
 mdadm --zero-superblock $dev8 $dev9 $dev10 $dev11 $dev12 $dev13
@@ -13,8 +14,8 @@ mdadm -CR $member0 -l raid6 -n 4 $dev10 $dev11 $dev12 $dev13 -z 16384
 #	    >/tmp/mdmon.txt 2>&1
 mdadm -CR $member1 -l raid10 -n 4 $dev10 $dev11 $dev12 $dev13 -z 16384
 
-dd if=/dev/sda of=$member0 bs=1M
-dd if=/dev/sda of=$member1 bs=1M skip=16
+dd if=$sda of=$member0 bs=1M
+dd if=$sda of=$member1 bs=1M skip=16
 
 check wait
 
diff --git a/tests/env-ddf-template b/tests/env-ddf-template
index 1c1ca12..aa57fab 100644
--- a/tests/env-ddf-template
+++ b/tests/env-ddf-template
@@ -1,3 +1,12 @@
+get_rootdev() {
+    local dev=$(stat -c %D /)
+    local maj=$(expr $dev : '\(..\)')
+    local min=${dev#$maj}
+    local bd=/dev/$(basename $(readlink /sys/dev/block/$((0x$maj)):$((0x$min))))
+    [ -b $bd ] || exit 1
+    echo $bd
+}
+
 get_sysdir() {
     local mddev=$1
     [ -L $mddev ] && mddev=$(readlink -f $mddev)
-- 
1.7.3.4

[PATCH 2/3] DDF test: make sure mdmon isn't started by systemd

[mwilck]

For testing we usually want the locally built mdmon, not the
one systemd prefers.
---
 tests/env-ddf-template |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/tests/env-ddf-template b/tests/env-ddf-template
index aa57fab..a607c10 100644
--- a/tests/env-ddf-template
+++ b/tests/env-ddf-template
@@ -104,3 +104,6 @@ member1=/dev/md/vol1
 member2=/dev/md/vol2
 member3=/dev/md/vol3
 member4=/dev/md/vol4
+
+# We don't want systemd to start system mdmon; start our own
+export MDADM_NO_SYSTEMCTL=1
-- 
1.7.3.4

[PATCH 3/3] DDF: compare_super_ddf: fix sequence number check

[mwilck]

The sequence number check in compare_super_ddf was broken,
anchor sequence number is always -1.

With this patch, mdadm will refuse to add a disk with non-matching
sequence number.

This fixes Francis Moreau's problem reported with subject
"mdadm 3.3 fails to kick out non fresh disk".

FIXME: More work is needed here. Currently mdadm won't even add the
disk to the container, that's wrong. It should be added as a spare.
---
 super-ddf.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/super-ddf.c b/super-ddf.c
index 3673cb3..002b271 100644
--- a/super-ddf.c
+++ b/super-ddf.c
@@ -3892,10 +3892,10 @@ static int compare_super_ddf(struct supertype *st, struct supertype *tst)
 	if (memcmp(first->anchor.guid, second->anchor.guid, DDF_GUID_LEN) != 0)
 		return 2;
 
-	if (!be32_eq(first->anchor.seq, second->anchor.seq)) {
-		dprintf("%s: sequence number mismatch %u/%u\n", __func__,
-			be32_to_cpu(first->anchor.seq),
-			be32_to_cpu(second->anchor.seq));
+	if (!be32_eq(first->active->seq, second->active->seq)) {
+		dprintf("%s: sequence number mismatch %u<->%u\n", __func__,
+			be32_to_cpu(first->active->seq),
+			be32_to_cpu(second->active->seq));
 		return 3;
 	}
 	if (first->max_part != second->max_part ||
-- 
1.7.3.4

Re: [PATCH 3/3] DDF: compare_super_ddf: fix sequence number check

[Martin Wilck]

On 09/14/2013 10:47 PM, mwilck@arcor.de wrote:
> The sequence number check in compare_super_ddf was broken,
> anchor sequence number is always -1.
> 
> With this patch, mdadm will refuse to add a disk with non-matching
> sequence number.
> 
> This fixes Francis Moreau's problem reported with subject
> "mdadm 3.3 fails to kick out non fresh disk".
> 
> FIXME: More work is needed here. Currently mdadm won't even add the
> disk to the container, that's wrong. It should be added as a spare.
> ---
>  super-ddf.c |    8 ++++----
>  1 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/super-ddf.c b/super-ddf.c
> index 3673cb3..002b271 100644
> --- a/super-ddf.c
> +++ b/super-ddf.c
> @@ -3892,10 +3892,10 @@ static int compare_super_ddf(struct supertype *st, struct supertype *tst)
>  	if (memcmp(first->anchor.guid, second->anchor.guid, DDF_GUID_LEN) != 0)
>  		return 2;
>  
> -	if (!be32_eq(first->anchor.seq, second->anchor.seq)) {
> -		dprintf("%s: sequence number mismatch %u/%u\n", __func__,
> -			be32_to_cpu(first->anchor.seq),
> -			be32_to_cpu(second->anchor.seq));
> +	if (!be32_eq(first->active->seq, second->active->seq)) {
> +		dprintf("%s: sequence number mismatch %u<->%u\n", __func__,
> +			be32_to_cpu(first->active->seq),
> +			be32_to_cpu(second->active->seq));
>  		return 3;
>  	}
>  	if (first->max_part != second->max_part ||

While the fix itself is certainly correct, I don't think any more that
this is the right thing to do. If we reject a disk in compare_super()
just because of a sequence number mismatch, we won't be able to handle
incremental scanning gracefully, because mdadm will never even try to
add it any more. This is exactly what Francis just noticed. However,
just ignoring the sequence number as the code did without the patch is
also incorrect, as Francis noted previously in the "fails to kick out
non fresh disk" thread.

The container loading code already has some logic to deal with different
sequence numbers during assembly (just pick the highest). In case of
incremental assembly, things get more complex. We must be prepared to
read the disk with the "best" (i.e. highest) meta data after other disk,
possibly after some subarrays have already been started. It is possible
that the new best meta data indicates failures previously loaded disks
and already started arrays.

I am still trying to understand exactly how mdadm, mdmon, and kernel
have to play together during incremental assembly in order to get this
right. I think most of this  should take place in mdmon/manager, but I
may be wrong about that.

Martin