Imsm OROM compatibility/boot support fixes
cover Imsm OROM compatibility/boot support fixes
This series of patches contain fixes regarding OROM and mdadm compatibility connected with handling of recovery/degraded states and missing disks. OROM behaves little differently when it marks disk as missing (see description of patches for details)
Last one in the series does not allow to modify metadata (in particular does not finish migration) on disks when array is set to read only by kernel (eg. during restart of system, when root partition lies on raid device).
---
Przemyslaw Czarnowski (4):
imsm: monitor: do not finish migration if there are no failed disks
imsm: clean up missing disks if there are any left after migration
imsm: avoid double change of serial number of missing device
imsm: monitor: do not finish recovery, when raid goes to read-only
monitor.c | 1 + super-intel.c | 33 ++++++++++++++++++++++++--------- 2 files changed, 25 insertions(+), 9 deletions(-)
PATCH 1/4 imsm: monitor: do not finish migration if there are no failed disks
Transition from "degraded" to "recovery" made in OROM is slightly different than the same transision in mdadm. Missing disk is not removed from list of raid devices, but just from map. Therefore mdadm should not end migration basing on existence of list of missing disks but should rely on count of failed disks.
--- super-intel.c | 9 ++++++++- 1 files changed, 8 insertions(+), 1 deletions(-)
super-intel.c +8 -1
@@ -6953,6 +6953,12 @@ static void handle_missing(struct intel_super *super, struct imsm_dev *dev)if (!super->missing)return ; +/* When orom adds replacement for missing disk it does +* not remove entry of missing disk, but just updates map with +* new added disk. So it is not enough just to test if there is +* any missing disk, we have to look if there are any failed disks +* in map to stop migration */ +dprintf ("imsm: mark missing\n" );/* end process for initialization and rebuild only */
@@ -6963,7 +6969,8 @@ static void handle_missing(struct intel_super *super, struct imsm_dev *dev) failed =imsm_count_failed (super, dev,MAP_0 ); map_state =imsm_check_degraded (super, dev, failed,MAP_0 ); -end_migration (dev, super, map_state); +if (failed) +end_migration (dev, super, map_state); }for (dl = super->missing; dl; dl = dl->next)mark_missing (dev, &dl->disk, dl->index);
PATCH 2/4 imsm: clean up missing disks if there are any left after migration
In reply to Przemyslaw CzarnowskiIt is for compatibility with OROM way to set "recovery" state.
--- super-intel.c | 9 +++++++-- 1 files changed, 7 insertions(+), 2 deletions(-)
super-intel.c +7 -2
@@ -7211,6 +7211,8 @@ skip_mark_checkpoint:return consistent; } +static void imsm_delete (struct intel_super *super,struct dl **dlp,unsigned index); +static void imsm_set_disk (struct active_array *a,int n,int state) {int inst = a->info.container_member;
@@ -7260,6 +7262,11 @@ static void imsm_set_disk(struct active_array *a, int n, int state)dprintf ("while rebuilding" );end_migration (dev, super, map_state); map =get_imsm_map (dev,MAP_0 ); +/* sweep list of missing disks, OROM might leave +* some residuals from old members */ +while (super->missing) { +imsm_delete (super, &super->missing, super->missing->index); + } map->failed_disk_num = ~0 ; super->updates_pending++; a->last_checkpoint =0 ;
@@ -7792,8 +7799,6 @@ static int remove_disk_super(struct intel_super *super, int major, int minor)return 0 ; } -static void imsm_delete (struct intel_super *super,struct dl **dlp,unsigned index); -static int add_remove_disk_update (struct intel_super *super) {int check_degraded =0 ;
PATCH 3/4 imsm: avoid double change of serial number of missing device
In reply to Przemyslaw CzarnowskiIf degraded state is set in OROM, it do not set 'failed' flag for missing, device, just changes serial number, sets scsiId to 0xffff and sets highest bit in ord table in map. When mdadm replaces missing disk, the one left there is marked as failed and missing again (changing already marked serial number).
--- super-intel.c | 15 +++++++++------ 1 files changed, 9 insertions(+), 6 deletions(-)
super-intel.c +9 -6
@@ -6908,12 +6908,15 @@ static int mark_failure(struct imsm_dev *dev, struct imsm_disk *disk, int idx)if (is_failed (disk) && (ord &IMSM_ORD_REBUILD ))return 0 ; -memcpy (buf, disk->serial,MAX_RAID_SERIAL_LEN ); - buf[MAX_RAID_SERIAL_LEN ] ='\000' ; -strcat (buf,":0" ); -if ((len =strlen (buf)) >=MAX_RAID_SERIAL_LEN ) - shift = len -MAX_RAID_SERIAL_LEN +1 ; -strncpy ((char *)disk->serial, &buf[shift],MAX_RAID_SERIAL_LEN ); +/* do not add ":0", OROM has already done it */ +if (!(ord &IMSM_ORD_REBUILD )) { +memcpy (buf, disk->serial,MAX_RAID_SERIAL_LEN ); + buf[MAX_RAID_SERIAL_LEN ] ='\000' ; +strcat (buf,":0" ); +if ((len =strlen (buf)) >=MAX_RAID_SERIAL_LEN ) + shift = len -MAX_RAID_SERIAL_LEN +1 ; +strncpy ((char *)disk->serial, &buf[shift],MAX_RAID_SERIAL_LEN ); + } disk->status |=FAILED_DISK ;set_imsm_ord_tbl_ent (map, slot, idx |IMSM_ORD_REBUILD );
PATCH 4/4 imsm: monitor: do not finish recovery, when raid goes to read-only
In reply to Przemyslaw CzarnowskiIf kernel forces raid to be read-only (eg. during reboot, and root partition lies on array) monitor should not finish recovery and leave metadata untouched.
--- monitor.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-)
monitor.c +1 -0
@@ -304,6 +304,7 @@ static int read_and_act(struct active_array *a) }if (!deactivate && + a->curr_state != readonly && a->curr_action == idle && a->prev_action == recover) {/* A recovery has finished. Some disks may be in sync now,
Re: [PATCH 2/4] imsm: clean up missing disks if there are any left after migration
In reply to Przemyslaw CzarnowskiQuoted 21 lines · Przemyslaw Czarnowski
It is for compatibility with OROM way to set "recovery" state.
Signed-off-by Przemyslaw Czarnowski <[email protected]>---
super-intel.c | 9 +++++++--1 files changed, 7 insertions(+), 2 deletions(-)super-intel.c +2
-0@@ -7211,6 +7211,8 @@ skip_mark_checkpoint: return consistent; } +static void imsm_delete (struct intel_super *super,struct dl **dlp,unsigned index); + static void imsm_set_disk(struct active_array *a, int n, int state) { int inst = a->info.container_member;
super-intel.c static void imsm_set_disk(struct active_array *a, int n, int state) · line 7269
Quoted 8 lines
super-intel.c +5
-0@@ -7260,6 +7262,11 @@ static void imsm_set_disk(struct active_array *a, int n, int state) dprintf("while rebuilding"); end_migration(dev, super, map_state); map = get_imsm_map(dev, MAP_0); + /* sweep list of missing disks, OROM might leave + * some residuals from old members */ + while (super->missing) { + imsm_delete(super, &super->missing, super->missing->index); + }
No, the expectation is that the list of disks is only mutated by the monitor from imsm_process_update() where we can be sure that the manager thread is not walking the disk list. I.e. we get away from needing locks by just making sure the monitor can't race with the manager.
--
Dan
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 4/4] imsm: monitor: do not finish recovery, when raid goes to read-only
In reply to Przemyslaw CzarnowskiQuoted 3 lines · Przemyslaw Czarnowski
If kernel forces raid to be read-only (eg. during reboot, and root partition lies on array) monitor should not finish recovery and leave metadata untouched.
I'm not sure I understand the point of this one, can you clarify? Even though the rootfs is readonly at initial boot it may still require the root block device to be readwrite. For example xfs needs to do journal playback even when mounting ro. So if the monitor is running at all it means that the array was started read-write (or at least read-auto) and should be fine to complete recovery.
--
Dan
Re: [PATCH 3/4] imsm: avoid double change of serial number of missing device
In reply to Przemyslaw CzarnowskiQuoted 17 lines · Przemyslaw Czarnowski
If degraded state is set in OROM, it do not set 'failed' flag for missing, device, just changes serial number, sets scsiId to
0xffffand sets highest bit in ord table in map. When mdadm replaces missing disk, the one left there is marked as failed and missing again (changing already marked serial number).Signed-off-by Przemyslaw Czarnowski <[email protected]>---
super-intel.c | 15 +++++++++------1 files changed, 9 insertions(+), 6 deletions(-)super-intel.c +0
-0@@ -6908,12 +6908,15 @@ static int mark_failure(struct imsm_dev *dev, struct imsm_disk *disk, int idx) if (is_failed(disk) && (ord & IMSM_ORD_REBUILD))
super-intel.c static int mark_failure(struct imsm_dev *dev, struct imsm_disk *disk, int idx) · line 6912
Quoted 10 lines
super-intel.c +2
-6return 0; - memcpy(buf, disk->serial, MAX_RAID_SERIAL_LEN); - buf[MAX_RAID_SERIAL_LEN] = '\000'; - strcat(buf, ":0"); - if ((len = strlen(buf)) >= MAX_RAID_SERIAL_LEN) - shift = len - MAX_RAID_SERIAL_LEN + 1; - strncpy((char *)disk->serial, &buf[shift], MAX_RAID_SERIAL_LEN); + /* do not add ":0", OROM has already done it */ + if (!(ord & IMSM_ORD_REBUILD)) {
What about the case where we are rebuilding to this disk and then get a failure, doesn't this prevent the serial number from getting updated?
The metadata expectation, as far as I can tell, is in-metadata-serial != serial-retrieved-from-disk. So if it gets mutated twice what harm does that cause?
super-intel.c static int mark_failure(struct imsm_dev *dev, struct imsm_disk *disk, int idx) · line 6922
Quoted 11 lines
super-intel.c +7
-0+ memcpy(buf, disk->serial, MAX_RAID_SERIAL_LEN); + buf[MAX_RAID_SERIAL_LEN] = '\000'; + strcat(buf, ":0"); + if ((len = strlen(buf)) >= MAX_RAID_SERIAL_LEN) + shift = len - MAX_RAID_SERIAL_LEN + 1; + strncpy((char *)disk->serial, &buf[shift], MAX_RAID_SERIAL_LEN); + } disk->status |= FAILED_DISK; set_imsm_ord_tbl_ent(map, slot, idx | IMSM_ORD_REBUILD);
--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/4] imsm: monitor: do not finish migration if there are no failed disks
In reply to Przemyslaw CzarnowskiQuoted 29 lines · Przemyslaw Czarnowski
Transition from "degraded" to "recovery" made in OROM is slightly different than the same transision in mdadm. Missing disk is not removed from list of raid devices, but just from map. Therefore mdadm should not end migration basing on existence of list of missing disks but should rely on count of failed disks.
Signed-off-by Przemyslaw Czarnowski <[email protected]>---
super-intel.c | 9 ++++++++-1 files changed, 8 insertions(+), 1 deletions(-)super-intel.c +6
-0@@ -6953,6 +6953,12 @@ static void handle_missing(struct intel_super *super, struct imsm_dev *dev) if (!super->missing) return; + /* When orom adds replacement for missing disk it does + * not remove entry of missing disk, but just updates map with + * new added disk. So it is not enough just to test if there is + * any missing disk, we have to look if there are any failed disks + * in map to stop migration */ + dprintf("imsm: mark missing\n"); /* end process for initialization and rebuild only*/
super-intel.c static void handle_missing(struct intel_super *super, struct imsm_dev *dev) · line 6973
Quoted 6 lines
super-intel.c +2
-1@@ -6963,7 +6969,8 @@ static void handle_missing(struct intel_super *super, struct imsm_dev *dev) failed = imsm_count_failed(super, dev, MAP_0); map_state = imsm_check_degraded(super, dev, failed, MAP_0); - end_migration(dev, super, map_state); + if (failed) + end_migration(dev, super, map_state);
Doesn't this need to be something like "failed >= max_degraded" since some recovery scenarios can continue in the presence of failed disks like raid10, raid6. Can you describe a bit more what the user visible behavior is when the OROM leaves these stale entries?
This may be the right thing to do, but handle_missing is also called in the case where we know a disk we had previously has been removed and in that case we certainly want to restart recovery, right? --
To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Quoted 17 lines · On Thu, 05 Apr 2012 17:28:12 +0200 Przemyslaw Czarnowski…
This series of patches contain fixes regarding OROM and mdadm compatibility connected with handling of recovery/degraded states and missing disks. OROM behaves little differently when it marks disk as missing (see description of patches for details)
Last one in the series does not allow to modify metadata (in particular does not finish migration) on disks when array is set to read only by kernel (eg. during restart of system, when root partition lies on raid device).---
Przemyslaw Czarnowski (4):
imsm: monitor: do not finish migration if there are no failed disks
imsm: clean up missing disks if there are any left after migration
imsm: avoid double change of serial number of missing device
imsm: monitor: do not finish recovery, when raid goes to read-only
Quoted 8 lines
monitor.c | 1 + super-intel.c | 33 ++++++++++++++++++++++++--------- 2 files changed, 25 insertions(+), 9 deletions(-)--
To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Given Dan's comments, I won't apply these just now.
NeilBrown