Bugreport ddf rebuild problems

Can you please try the attached patch?

Regards,
Martin

Quoted 1 line · Martin Wilck

Can you please try the attached patch?

DON'T use it, it's broken. I will send a better one in a minute.

Quoted 6 lines

Regards,
Martin

Hi Albert,

Quoted 3 lines · Martin Wilck

Quoted 1 line · Martin Wilck

Can you please try the attached patch?

DON'T use it, it's broken. I will send a better one in a minute.

My latest patch series should fix this. The fix for your problem is 3/4, and I added your test case as 4/4. Retest is welcome.

Martin

Quoted 5 lines

Quoted 5 lines

Regards,
Martin

Hi Martin,

I can confirm that patch 3/4 indeed fixed the problem.

Thanks for your quick fix.

Kind regards,

Albert

Quoted 16 lines · Martin Wilck

Hi Albert,

Quoted 3 lines · Martin Wilck

Quoted 1 line · Martin Wilck

Can you please try the attached patch?

DON'T use it, it's broken. I will send a better one in a minute.

My latest patch series should fix this. The fix for your problem is 3/4, and I added your test case as 4/4. Retest is welcome.

Martin

Quoted 5 lines

Quoted 5 lines

Regards,
Martin

Hi Martin,

I noticed another problem, with or without your patch, the problem occurs:

Zeroed the superblocks (just in case):
mdadm --zero-superblock /dev/loop[1-6]

Created the container:
mdadm -CR /dev/md127 -e ddf -l container -n 5 /dev/loop[1-5]

Created an md device in the container, it used /dev/loop4 and /dev/loop5, rebuild and finished mdadm -CR /dev/md0 -l raid1 -n 2 /dev/md127

I fail one of the disks, it rebuild with one of the available unused disks in the container:
mdadm -f /dev/md0 /dev/loop4

I add another md device.
mdadm -CR /dev/md1 -l raid5 -n 3 /dev/md127

This last one is the odd bit, it is build using the previously failed disk. Looks like the container is not aware that /dev/loop4 has failed, en reuses it. Which is wrong. So the failed status is not kept.

Regards,

Albert

Hi Albert, Neil,

I just submitted a new patch series; patch 3/5 integrates your 2nd case as a new unit test and 4/5 should fix it.

However @Neil: I am not yet entirely happy with this solution. AFAICS there is a possible race condition here, if a disk fails and mdadm -CR is called to create a new array before the metadata reflecting the failure is written to disk. If a disk failure happens in one array, mdmon will call reconcile_failed() to propagate the failure to other already known arrays in the same container, by writing "faulty" to the sysfs state attribute. It can't do that for a new container though.

I thought that process_update() may need to check the kernel state of array members against meta data state when a new VD configuration record is received, but that's impossible because we can't call open() on the respective sysfs files. It could be done in prepare_update(), but that would require major changes, I wanted to ask you first.

Another option would be changing manage_new(). But we don't seem to have a suitable metadata handler method to pass the meta data state to the manager....

Ideas?

Quoted 19 lines · On Mon, 05 Aug 2013 23:24:28 +0200 Martin Wilck <[email protected]> wrote:

Hi Albert, Neil,

I just submitted a new patch series; patch 3/5 integrates your 2nd case as a new unit test and 4/5 should fix it.

However @Neil: I am not yet entirely happy with this solution. AFAICS there is a possible race condition here, if a disk fails and mdadm -CR is called to create a new array before the metadata reflecting the failure is written to disk. If a disk failure happens in one array, mdmon will call reconcile_failed() to propagate the failure to other already known arrays in the same container, by writing "faulty" to the sysfs state attribute. It can't do that for a new container though.

I thought that process_update() may need to check the kernel state of array members against meta data state when a new VD configuration record is received, but that's impossible because we can't call open() on the respective sysfs files. It could be done in prepare_update(), but that would require major changes, I wanted to ask you first.

Quoted 5 lines

Another option would be changing manage_new(). But we don't seem to have a suitable metadata handler method to pass the meta data state to the manager....

Ideas?

Thanks for the patches - I applied them all.

Is there a race here? When "mdadm -C" looks at the metadata the device will either be an active member of another array, or it will be marked faulty. Either way mdadm won't use it.

If the first array was created to use only (say) half of each device and the second array was created with a size to fit in the other half of the device then it might get interesting.
"mdadm -C" might see that everything looks good, create the array using the second half of that drive that has just failed, and give that info to mdmon.

I suspect that ddf_open_new (which currently looks like it is just a stub) needs to help out here.

When manage_new() gets told about a new array it will collect relevant info from sysfs and call ->open_new() to make sure it matches the metadata. ddf_open_new should check that all the devices in the array are recorded as working in the metadata. If any are failed, it can write 'faulty' to the relevant state_fd.

Possibly the same thing can be done generically in manage_new() as you suggested. After the new array has been passed over to the monitor thread, manage_new() could check if any devices should be failed much like reconcile_failed() does and just fail them.

Does that make any sense? Did I miss something?

Thanks,
NeilBrown

When we create an array while mdmon is working on an event (e.g. disk failure), the meta data on disk may not be up-to-date.

Patch "DDF: ddf_open_new: check device status for new subarray" added some checks for in the monitor for that situation - in particular, to handle a freshly created array with faulty disks. The remaining problem is that the kernel may start syncing the disks before this situation is detected. This patch delays recovery until mdmon finished checking.

tests/10ddf-fail-create-race should succeed reliably with this patch and "DDF: ddf_open_new: check device status for new subarray". Without, it will fail sporadically.

Signed-off-by Martin Wilck <[email protected]>

---
 Create.c    |    8 ++++++++
 managemon.c |    6 ++++++
 2 files changed, 14 insertions(+), 0 deletions(-)

Create.c +8 -0

index ac22f77..f9b7db2 100644
@@ -993,6 +993,14 @@ int Create(struct supertype *st, char *mddev,
 				need_mdmon = 0;
 				break;
 			default:
+				/*
+				 * The meta data we saw on disk may not be
+				 * up-to-date. The monitor will check and
+				 * possibly fail. Avoid a resync happening
+				 * in the kernel before that.
+				 */
+				sysfs_set_str(&info, NULL, "sync_action",
+					      "frozen");
 				err = sysfs_set_str(&info, NULL, "array_state",
 						    "readonly");
 				break;

managemon.c +0 -0

index f40bbdb..5bc54da 100644
@@ -744,6 +744,12 @@ static void manage_new(struct mdstat_ent *mdstat,
 		new->container = NULL;
 		free_aa(new);

Quoted 46 lines · On Tue, 6 Aug 2013 23:38:03 +0200 [email protected] wrote:

When we create an array while mdmon is working on an event (e.g. disk failure), the meta data on disk may not be up-to-date.

Patch "DDF: ddf_open_new: check device status for new subarray" added some checks for in the monitor for that situation - in particular, to handle a freshly created array with faulty disks. The remaining problem is that the kernel may start syncing the disks before this situation is detected. This patch delays recovery until mdmon finished checking.

tests/10ddf-fail-create-race should succeed reliably with this patch and "DDF: ddf_open_new: check device status for new subarray". Without, it will fail sporadically.

Signed-off-by Martin Wilck <[email protected]>

---
 Create.c    |    8 ++++++++
 managemon.c |    6 ++++++
 2 files changed, 14 insertions(+), 0 deletions(-)

Create.c +8 -0

index ac22f77..f9b7db2 100644
@@ -993,6 +993,14 @@ int Create(struct supertype *st, char *mddev,
 				need_mdmon = 0;
 				break;
 			default:
+				/*
+				 * The meta data we saw on disk may not be
+				 * up-to-date. The monitor will check and
+				 * possibly fail. Avoid a resync happening
+				 * in the kernel before that.
+				 */
+				sysfs_set_str(&info, NULL, "sync_action",
+					      "frozen");
 				err = sysfs_set_str(&info, NULL, "array_state",
 						    "readonly");
 				break;

managemon.c +0 -0

index f40bbdb..5bc54da 100644
@@ -744,6 +744,12 @@ static void manage_new(struct mdstat_ent *mdstat,
 		new->container = NULL;
 		free_aa(new);

managemon.c static void manage_new(struct mdstat_ent *mdstat, · line 755

Quoted 10 lines

managemon.c +6 -0

 	} else {
+		/*
+		 * Create() set this to frozen.
+		 * This relies on the kernel clear FROZEN status
+		 * if an invalid value is written to sync_action.
+		 */
+		sysfs_set_str(&new->info, NULL, "sync_action", "");
 		replace_array(container, victim, new);
 		if (failed) {
 			new->check_degraded = 1;

I don't think that this patch should be necessary. If you find it makes a difference, then I'm missing something.

Recovery never starts while the array is readonly - the kernel doesn't allow it.
So mdmon should be in complete control as it is the only one that is allowed to change the array from readonly.

Whether it quite does the right thing in this case I can't promise, but it should be able to do the right thing without any extra help from mdadm.

And writing an empty string to 'sync_action' will not do anything useful. You probably mean to write "idle".

I've applied and push out the previous patches but not this one.

Quoted 21 lines · NeilBrown

Quoted 19 lines · On Mon, 05 Aug 2013 23:24:28 +0200 Martin Wilck <[email protected]> wrote:

Hi Albert, Neil,

I just submitted a new patch series; patch 3/5 integrates your 2nd case as a new unit test and 4/5 should fix it.

However @Neil: I am not yet entirely happy with this solution. AFAICS there is a possible race condition here, if a disk fails and mdadm -CR is called to create a new array before the metadata reflecting the failure is written to disk. If a disk failure happens in one array, mdmon will call reconcile_failed() to propagate the failure to other already known arrays in the same container, by writing "faulty" to the sysfs state attribute. It can't do that for a new container though.

I thought that process_update() may need to check the kernel state of array members against meta data state when a new VD configuration record is received, but that's impossible because we can't call open() on the respective sysfs files. It could be done in prepare_update(), but that would require major changes, I wanted to ask you first.

Quoted 6 lines

Quoted 5 lines

Another option would be changing manage_new(). But we don't seem to have a suitable metadata handler method to pass the meta data state to the manager....

Ideas?

Thanks for the patches - I applied them all.

I don't see them in the public repo yet.

Quoted 3 lines

Is there a race here? When "mdadm -C" looks at the metadata the device will either be an active member of another array, or it will be marked faulty. Either way mdadm won't use it.

That's right, thanks.

Quoted 5 lines

If the first array was created to use only (say) half of each device and the second array was created with a size to fit in the other half of the device then it might get interesting.
"mdadm -C" might see that everything looks good, create the array using the second half of that drive that has just failed, and give that info to mdmon.

Yes, I have created a test case for this (10ddf-fail-create-race) which I am going to submit soon.

Quoted 2 lines

I suspect that ddf_open_new (which currently looks like it is just a stub) needs to help out here.

Great idea, I made an implementation. I found that I needed to freeze the array in Create(), too, to avoid the kernel starting a rebuild before the mdmon checked the correctness of the new array. Please review that, I'm not 100% positive I got it right.

Quoted 6 lines

When manage_new() gets told about a new array it will collect relevant info from sysfs and call ->open_new() to make sure it matches the metadata. ddf_open_new should check that all the devices in the array are recorded as working in the metadata. If any are failed, it can write 'faulty' to the relevant state_fd.

Quoted 6 lines

Possibly the same thing can be done generically in manage_new() as you suggested. After the new array has been passed over to the monitor thread, manage_new() could check if any devices should be failed much like reconcile_failed() does and just fail them.

Does that make any sense? Did I miss something?

It makes a lot of sense.

While testing, I found another minor problem case:

1 disk fails in array taking half size
2 mdmon activates spare
3 mdadm -C is called and finds old meta data, allocates extent at
offset 0 on the spare
4 Create() gets an error writing to the "size" sysfs attribute because
offset 0 has been grabbed by the spare recovery already

That's not too bad, after all, because the array won't be created. The user just needs to re-issue his mdadm -C command which will now succeed because the meta data should have been written to disk in the meantime.

That said, some kind of locking between mdadm and mdmon (mdadm won't read meta data as long as mdmon is busy writing them) might be desirable. It would be even better to do all meta data operations through mdmon, mdadm just sending messages to it. That would be a major architectural change for mdadm, but it would avoid this kind of "different meta data here and there" problem altogether.

Thanks
Martin

Quoted 2 lines

Thanks,
NeilBrown

Quoted 21 lines · NeilBrown

Quoted 19 lines · On Mon, 05 Aug 2013 23:24:28 +0200 Martin Wilck <[email protected]> wrote:

Hi Albert, Neil,

I just submitted a new patch series; patch 3/5 integrates your 2nd case as a new unit test and 4/5 should fix it.

However @Neil: I am not yet entirely happy with this solution. AFAICS there is a possible race condition here, if a disk fails and mdadm -CR is called to create a new array before the metadata reflecting the failure is written to disk. If a disk failure happens in one array, mdmon will call reconcile_failed() to propagate the failure to other already known arrays in the same container, by writing "faulty" to the sysfs state attribute. It can't do that for a new container though.

I thought that process_update() may need to check the kernel state of array members against meta data state when a new VD configuration record is received, but that's impossible because we can't call open() on the respective sysfs files. It could be done in prepare_update(), but that would require major changes, I wanted to ask you first.

Quoted 7 lines

Quoted 5 lines

Another option would be changing manage_new(). But we don't seem to have a suitable metadata handler method to pass the meta data state to the manager....

Ideas?

Thanks for the patches - I applied them all.

I don't see them in the public repo yet.

Quoted 3 lines

Is there a race here? When "mdadm -C" looks at the metadata the device will either be an active member of another array, or it will be marked faulty. Either way mdadm won't use it.

That's right, thanks.

Quoted 5 lines

If the first array was created to use only (say) half of each device and the second array was created with a size to fit in the other half of the device then it might get interesting.
"mdadm -C" might see that everything looks good, create the array using the second half of that drive that has just failed, and give that info to mdmon.

Yes, I have created a test case for this (10ddf-fail-create-race) which I am going to submit soon.

Quoted 2 lines

I suspect that ddf_open_new (which currently looks like it is just a stub) needs to help out here.

Great idea, I made an implementation. I found that I needed to freeze the array in Create(), too, to avoid the kernel starting a rebuild before the mdmon checked the correctness of the new array. Please review that, I'm not 100% positive I got it right.

Quoted 6 lines

When manage_new() gets told about a new array it will collect relevant info from sysfs and call ->open_new() to make sure it matches the metadata. ddf_open_new should check that all the devices in the array are recorded as working in the metadata. If any are failed, it can write 'faulty' to the relevant state_fd.

Quoted 6 lines

Possibly the same thing can be done generically in manage_new() as you suggested. After the new array has been passed over to the monitor thread, manage_new() could check if any devices should be failed much like reconcile_failed() does and just fail them.

Does that make any sense? Did I miss something?

It makes a lot of sense.

While testing, I found another minor problem case:

1 disk fails in array taking half size
2 mdmon activates spare
3 mdadm -C is called and finds old meta data, allocates extent at
offset 0 on the spare
4 Create() gets an error writing to the "size" sysfs attribute because
offset 0 has been grabbed by the spare recovery already

That's not too bad, after all, because the array won't be created. The user just needs to re-issue his mdadm -C command which will now succeed because the meta data should have been written to disk in the meantime.

That said, some kind of locking between mdadm and mdmon (mdadm won't read meta data as long as mdmon is busy writing them) might be desirable. It would be even better to do all meta data operations through mdmon, mdadm just sending messages to it. That would be a major architectural change for mdadm, but it would avoid this kind of "different meta data here and there" problem altogether.