> -----Original Message----- > From: dan.j.williams@xxxxxxxxx [mailto:dan.j.williams@xxxxxxxxx] On > Behalf Of Dan Williams > Sent: Thursday, February 24, 2011 6:43 PM > To: Wojcik, Krzysztof > Cc: neilb@xxxxxxx; linux-raid@xxxxxxxxxxxxxxx; Neubauer, Wojciech; > Kwolek, Adam; Ciechanowski, Ed > Subject: Re: [PATCH] FIX: imsm: OROM does not recognize degraded arrays > > Thanks Krzysztof, comments below: > > On Wed, Feb 23, 2011 at 10:53 PM, Krzysztof Wojcik > <krzysztof.wojcik@xxxxxxxxx> wrote: > > Defect description: > > When we create an redundant array in mdadm and then degrade it > > by disk removing, Option ROM and Windows OS does not detect any > array. > > Reason: > > Metadata created and updated after degrading array is not compatible > > with IMSM standard. > > > > This patch synchronizes the metadata according IMSM requirements. > > Following inconsistencies have been fixed: > > - reset all fields in imsm_dev during creation to avoid random values > > - init dev status during creation to proper state > > - not reset CONFIGURED_DISK flag when disk is missing > > - add ":0" suffix to the serial number for missing/failed disks > > - update medatada signature after takeover operation > > - mark map state as degraded after raid0->raid10 takeover > > > > Signed-off-by: Krzysztof Wojcik <krzysztof.wojcik@xxxxxxxxx> > > --- > > super-intel.c | 27 +++++++++++++++++---------- > > 1 files changed, 17 insertions(+), 10 deletions(-) > > > > diff --git a/super-intel.c b/super-intel.c > > index c101dca..a40b7e5 100644 > > --- a/super-intel.c > > +++ b/super-intel.c > > @@ -3437,6 +3437,8 @@ static int init_super_imsm_volume(struct > supertype *st, mdu_array_info_t *info, > > fprintf(stderr, Name": could not allocate raid > device\n"); > > return 0; > > } > > + memset(dev, 0, (sizeof(*dev) + sizeof(__u32) * (info- > >raid_disks - 1))); > > + > > Consider changing the allocation to calloc, or store the size in a > temporary variable so we don't duplicate the size calculation. OK. Changed > > > strncpy((char *) dev->volume, name, MAX_RAID_SERIAL_LEN); > > if (info->level == 1) > > array_blocks = info_to_blocks_per_member(info); > > @@ -3449,8 +3451,7 @@ static int init_super_imsm_volume(struct > supertype *st, mdu_array_info_t *info, > > > > dev->size_low = __cpu_to_le32((__u32) array_blocks); > > dev->size_high = __cpu_to_le32((__u32) (array_blocks >> 32)); > > - dev->status = __cpu_to_le32(0); > > - dev->reserved_blocks = __cpu_to_le32(0); > > + dev->status = (DEV_READ_COALESCING | DEV_WRITE_COALESCING); > > The orom will fail to assemble arrays without these status bits set? These bits are not the direct cause that OROM not assemble the array but it is not compatible with metadata created under Windows and OROM and may cause problems in other scenarios. > > > vol = &dev->vol; > > vol->migr_state = 0; > > set_migr_type(dev, MIGR_INIT); > > @@ -5046,6 +5047,7 @@ static int mark_failure(struct imsm_dev *dev, > struct imsm_disk *disk, int idx) > > __u32 ord; > > int slot; > > struct imsm_map *map; > > + char buf[PATH_MAX]; > > This is a very large stack allocation which is potentially problematic > for mdmon. OK. Changed. > > > > > /* new failures are always set in map[0] */ > > map = get_imsm_map(dev, 0); > > @@ -5057,9 +5059,12 @@ static int mark_failure(struct imsm_dev *dev, > struct imsm_disk *disk, int idx) > > ord = __le32_to_cpu(map->disk_ord_tbl[slot]); > > if (is_failed(disk) && (ord & IMSM_ORD_REBUILD)) > > return 0; > > + sprintf(buf, "%s:0", disk->serial); > > + int len = strlen(buf); > > + unsigned int shift = len - MAX_RAID_SERIAL_LEN + 1; > > + strncpy((char *)disk->serial, &buf[shift], > MAX_RAID_SERIAL_LEN); > > Other option-roms truncated the serial number just to make sure it did > not match (see mark_missing()), are you sure the ":0" matters? And if > we are already tolerant of chopping characters why not just > append/overwrite the end of the serial number. I adapted the method to the one used by OROM- append ":0" at the end of serial number and cut at the beginning if size > MAX_RAID_SERIAL_LEN. > > > disk->status |= FAILED_DISK; > > - disk->status &= ~CONFIGURED_DISK; > > The Windows driver was observed making this modification, but maybe > only in the "missing" case. Have you verified that this works in the > failed but still present case (versus the device gone case). This was verified under OROM and Windows. If disk has been configured and become member of an array CONFIGURED_DISK bit is set and it is not reset even though disk is missing or failed. > > > set_imsm_ord_tbl_ent(map, slot, idx | IMSM_ORD_REBUILD); > > if (map->failed_disk_num == 0xff) > > map->failed_disk_num = slot; > > @@ -6063,16 +6068,16 @@ static int apply_takeover_update(struct > imsm_update_takeover *u, > > *space_list = *space; > > du = (void *)space; > > memcpy(du, super->disks, sizeof(*du)); > > - du->disk.status = FAILED_DISK; > > - du->disk.scsi_id = 0; > > + du->disk.status = CONFIGURED_DISK | > FAILED_DISK; > > + du->disk.scsi_id = ~0; > > Looks like the code has grown some open coded duplications of what > mark_failure() is doing. Let's unify this. Keep subtle compatibility > logic confined to one location. OK. Changed > > -- > Dan -- To unsubscribe from this list: send the line "unsubscribe linux-raid" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
