[PATCH 1/8] imsm: parse bad block log in metadata on startup

[PATCH 1/8] imsm: parse bad block log in metadata on startup

[Tomasz Majchrzak]

Always allocate memory for all log entries to avoid a need for memory
allocation when monitor requests to record a bad block.

Also some extra checks added to make static code analyzer happy.

Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
---
 super-intel.c | 158 +++++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 112 insertions(+), 46 deletions(-)

diff --git a/super-intel.c b/super-intel.c
index c146bbd..5d6d534 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -217,22 +217,24 @@ struct imsm_super {
 } __attribute__ ((packed));
 
 #define BBM_LOG_MAX_ENTRIES 254
+#define BBM_LOG_MAX_LBA_ENTRY_VAL 256		/* Represents 256 LBAs */
+#define BBM_LOG_SIGNATURE 0xABADB10C
+
+struct bbm_log_block_addr {
+	__u16 w1;
+	__u32 dw1;
+} __attribute__ ((__packed__));
 
 struct bbm_log_entry {
-	__u64 defective_block_start;
-#define UNREADABLE 0xFFFFFFFF
-	__u32 spare_block_offset;
-	__u16 remapped_marked_count;
-	__u16 disk_ordinal;
+	__u8 marked_count;		/* Number of blocks marked - 1 */
+	__u8 disk_ordinal;		/* Disk entry within the imsm_super */
+	struct bbm_log_block_addr defective_block_start;
 } __attribute__ ((__packed__));
 
 struct bbm_log {
 	__u32 signature; /* 0xABADB10C */
 	__u32 entry_count;
-	__u32 reserved_spare_block_count; /* 0 */
-	__u32 reserved; /* 0xFFFF */
-	__u64 first_spare_lba;
-	struct bbm_log_entry mapped_block_entries[BBM_LOG_MAX_ENTRIES];
+	struct bbm_log_entry marked_block_entries[BBM_LOG_MAX_ENTRIES];
 } __attribute__ ((__packed__));
 
 #ifndef MDASSEMBLE
@@ -785,6 +787,92 @@ static struct imsm_dev *get_imsm_dev(struct intel_super *super, __u8 index)
 	return NULL;
 }
 
+#if BYTE_ORDER == LITTLE_ENDIAN
+static inline unsigned long long __le48_to_cpu(const struct bbm_log_block_addr
+					       *addr)
+{
+	return ((((__u64)addr->dw1) << 16) | addr->w1);
+}
+
+static inline struct bbm_log_block_addr __cpu_to_le48(unsigned long long sec)
+{
+	struct bbm_log_block_addr addr;
+
+	addr.w1 =  (__u16)(sec & 0xFFFF);
+	addr.dw1 = (__u32)((sec >> 16) & 0xFFFFFFFF);
+	return addr;
+}
+#elif BYTE_ORDER == BIG_ENDIAN
+static inline unsigned long long __le48_to_cpu(const struct bbm_log_block_addr
+					       *addr)
+{
+	return ((((__u64)__le32_to_cpu(addr->dw1)) << 16) |
+		__le16_to_cpu(addr->w1));
+}
+
+static inline struct bbm_log_block_addr __cpu_to_le48(unsigned long long sec)
+{
+	struct bbm_log_block_addr addr;
+
+	addr.w1 =  __cpu_to_le16((__u16)(sec & 0xFFFF));
+	addr.dw1 = __cpu_to_le32((__u32)(sec >> 16) & 0xFFFFFFFF);
+	return addr;
+}
+#else
+#  error "unknown endianness."
+#endif
+
+#ifndef MDASSEMBLE
+/* get size of the bbm log */
+static __u32 get_imsm_bbm_log_size(struct bbm_log *log)
+{
+	if (!log || log->entry_count == 0)
+		return 0;
+
+	return sizeof(log->signature) +
+		sizeof(log->entry_count) +
+		log->entry_count * sizeof(struct bbm_log_entry);
+}
+#endif /* MDASSEMBLE */
+
+/* allocate and load BBM log from metadata */
+static int load_bbm_log(struct intel_super *super)
+{
+	struct imsm_super *mpb = super->anchor;
+	__u32 bbm_log_size =  __le32_to_cpu(mpb->bbm_log_size);
+
+	super->bbm_log = malloc(sizeof(struct bbm_log));
+	if (!super->bbm_log)
+		return 1;
+
+	if (bbm_log_size) {
+		struct bbm_log *log = (void *)mpb +
+			__le32_to_cpu(mpb->mpb_size) - bbm_log_size;
+		__u32 entry_count;
+
+		if (bbm_log_size < sizeof(log->signature) +
+		    sizeof(log->entry_count))
+			return 2;
+
+		entry_count = __le32_to_cpu(log->entry_count);
+		if ((__le32_to_cpu(log->signature) != BBM_LOG_SIGNATURE) ||
+		    (entry_count > BBM_LOG_MAX_ENTRIES))
+			return 3;
+
+		if (bbm_log_size !=
+		    sizeof(log->signature) + sizeof(log->entry_count) +
+		    entry_count * sizeof(struct bbm_log_entry))
+			return 4;
+
+		memcpy(super->bbm_log, log, bbm_log_size);
+	} else {
+		super->bbm_log->signature = __cpu_to_le32(BBM_LOG_SIGNATURE);
+		super->bbm_log->entry_count = 0;
+	}
+
+	return 0;
+}
+
 /*
  * for second_map:
  *  == MAP_0 get first map
@@ -1433,7 +1521,7 @@ static void examine_super_imsm(struct supertype *st, char *homehost)
 	printf("          Disks : %d\n", mpb->num_disks);
 	printf("   RAID Devices : %d\n", mpb->num_raid_devs);
 	print_imsm_disk(__get_imsm_disk(mpb, super->disks->index), super->disks->index, reserved);
-	if (super->bbm_log) {
+	if (get_imsm_bbm_log_size(super->bbm_log)) {
 		struct bbm_log *log = super->bbm_log;
 
 		printf("\n");
@@ -1441,9 +1529,6 @@ static void examine_super_imsm(struct supertype *st, char *homehost)
 		printf("       Log Size : %d\n", __le32_to_cpu(mpb->bbm_log_size));
 		printf("      Signature : %x\n", __le32_to_cpu(log->signature));
 		printf("    Entry Count : %d\n", __le32_to_cpu(log->entry_count));
-		printf("   Spare Blocks : %d\n",  __le32_to_cpu(log->reserved_spare_block_count));
-		printf("    First Spare : %llx\n",
-		       (unsigned long long) __le64_to_cpu(log->first_spare_lba));
 	}
 	for (i = 0; i < mpb->num_raid_devs; i++) {
 		struct mdinfo info;
@@ -3628,19 +3713,6 @@ static int parse_raid_devices(struct intel_super *super)
 	return 0;
 }
 
-/* retrieve a pointer to the bbm log which starts after all raid devices */
-struct bbm_log *__get_imsm_bbm_log(struct imsm_super *mpb)
-{
-	void *ptr = NULL;
-
-	if (__le32_to_cpu(mpb->bbm_log_size)) {
-		ptr = mpb;
-		ptr += mpb->mpb_size - __le32_to_cpu(mpb->bbm_log_size);
-	}
-
-	return ptr;
-}
-
 /*******************************************************************************
  * Function:	check_mpb_migr_compatibility
  * Description:	Function checks for unsupported migration features:
@@ -3790,12 +3862,6 @@ static int load_imsm_mpb(int fd, struct intel_super *super, char *devname)
 		return 3;
 	}
 
-	/* FIXME the BBM log is disk specific so we cannot use this global
-	 * buffer for all disks.  Ok for now since we only look at the global
-	 * bbm_log_size parameter to gate assembly
-	 */
-	super->bbm_log = __get_imsm_bbm_log(super->anchor);
-
 	return 0;
 }
 
@@ -3839,6 +3905,9 @@ load_and_parse_mpb(int fd, struct intel_super *super, char *devname, int keep_fd
 	if (err)
 		return err;
 	err = parse_raid_devices(super);
+	if (err)
+		return err;
+	err = load_bbm_log(super);
 	clear_hi(super);
 	return err;
 }
@@ -3903,6 +3972,8 @@ static void __free_imsm(struct intel_super *super, int free_disks)
 		free(elem);
 		elem = next;
 	}
+	if (super->bbm_log)
+		free(super->bbm_log);
 	super->hba = NULL;
 }
 
@@ -4508,7 +4579,7 @@ static int get_super_block(struct intel_super **super_list, char *devnm, char *d
 		*super_list = s;
 	} else {
 		if (s)
-			free(s);
+			free_imsm(s);
 		if (dfd >= 0)
 			close(dfd);
 	}
@@ -4570,6 +4641,8 @@ static int load_super_imsm(struct supertype *st, int fd, char *devname)
 	free_super_imsm(st);
 
 	super = alloc_super();
+	if (!super)
+		return 1;
 	/* Load hba and capabilities if they exist.
 	 * But do not preclude loading metadata in case capabilities or hba are
 	 * non-compliant and ignore_hw_compat is set.
@@ -4912,7 +4985,7 @@ static int init_super_imsm(struct supertype *st, mdu_array_info_t *info,
 
 	super = alloc_super();
 	if (super && posix_memalign(&super->buf, 512, mpb_size) != 0) {
-		free(super);
+		free_imsm(super);
 		super = NULL;
 	}
 	if (!super) {
@@ -4922,7 +4995,7 @@ static int init_super_imsm(struct supertype *st, mdu_array_info_t *info,
 	if (posix_memalign(&super->migr_rec_buf, 512, MIGR_REC_BUF_SIZE) != 0) {
 		pr_err("could not allocate migr_rec buffer\n");
 		free(super->buf);
-		free(super);
+		free_imsm(super);
 		return 0;
 	}
 	memset(super->buf, 0, mpb_size);
@@ -5489,11 +5562,6 @@ static int store_super_imsm(struct supertype *st, int fd)
 #endif
 }
 
-static int imsm_bbm_log_size(struct imsm_super *mpb)
-{
-	return __le32_to_cpu(mpb->bbm_log_size);
-}
-
 #ifndef MDASSEMBLE
 static int validate_geometry_imsm_container(struct supertype *st, int level,
 					    int layout, int raiddisks, int chunk,
@@ -5529,6 +5597,10 @@ static int validate_geometry_imsm_container(struct supertype *st, int level,
 	 * note that there is no fd for the disks in array.
 	 */
 	super = alloc_super();
+	if (!super) {
+		close(fd);
+		return 0;
+	}
 	rv = find_intel_hba_capability(fd, super, verbose > 0 ? dev : NULL);
 	if (rv != 0) {
 #if DEBUG
@@ -6760,12 +6832,6 @@ static struct mdinfo *container_content_imsm(struct supertype *st, char *subarra
 		pr_err("Unsupported attributes in IMSM metadata.Arrays activation is blocked.\n");
 	}
 
-	/* check for bad blocks */
-	if (imsm_bbm_log_size(super->anchor)) {
-		pr_err("BBM log found in IMSM metadata.Arrays activation is blocked.\n");
-		sb_errors = 1;
-	}
-
 	/* count spare devices, not used in maps
 	 */
 	for (d = super->disks; d; d = d->next)
-- 
1.8.3.1

[PATCH 2/8] imsm: write bad block log on metadata sync

[Tomasz Majchrzak]

Pre-allocate memory for largest possible bad block sectionwhen monitor
is being opened to avoid a need for memory allocation on metadata sync.

If memory for a structure has been allocated in mpb buffer but it hasn't
been used yet, it will be taken by next buffer grow, leading to
insufficient memory on metadata flush. Start tracking such memory and
take it into calculation when growing a buffer. Also assert has been
added to debug mode to warn when more metadata has been written than
memory allocated.

Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
---
 super-intel.c | 49 +++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 45 insertions(+), 4 deletions(-)

diff --git a/super-intel.c b/super-intel.c
index 5d6d534..0591c55 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -81,7 +81,8 @@
 					MPB_ATTRIB_RAID1           | \
 					MPB_ATTRIB_RAID10          | \
 					MPB_ATTRIB_RAID5           | \
-					MPB_ATTRIB_EXP_STRIPE_SIZE)
+					MPB_ATTRIB_EXP_STRIPE_SIZE | \
+					MPB_ATTRIB_BBM)
 
 /* Define attributes that are unused but not harmful */
 #define MPB_ATTRIB_IGNORED		(MPB_ATTRIB_NEVER_USE)
@@ -361,6 +362,7 @@ struct intel_super {
 		array, it indicates that mdmon is allowed to clean migration
 		record */
 	size_t len; /* size of the 'buf' allocation */
+	size_t extra_space; /* extra space in 'buf' that is not used yet */
 	void *next_buf; /* for realloc'ing buf from the manager */
 	size_t next_len;
 	int updates_pending; /* count of pending updates for mdmon */
@@ -420,6 +422,7 @@ enum imsm_update_type {
 	update_takeover,
 	update_general_migration_checkpoint,
 	update_size_change,
+	update_prealloc_badblocks_mem,
 };
 
 struct imsm_update_activate_spare {
@@ -508,6 +511,10 @@ struct imsm_update_add_remove_disk {
 	enum imsm_update_type type;
 };
 
+struct imsm_update_prealloc_bb_mem {
+	enum imsm_update_type type;
+};
+
 static const char *_sys_dev_type[] = {
 	[SYS_DEV_UNKNOWN] = "Unknown",
 	[SYS_DEV_SAS] = "SAS",
@@ -3250,6 +3257,8 @@ static size_t disks_to_mpb_size(int disks)
 	size += (4 - 2) * sizeof(struct imsm_map);
 	/* 4 possible disk_ord_tbl's */
 	size += 4 * (disks - 1) * sizeof(__u32);
+	/* maximum bbm log */
+	size += sizeof(struct bbm_log);
 
 	return size;
 }
@@ -3710,6 +3719,8 @@ static int parse_raid_devices(struct intel_super *super)
 		super->len = len;
 	}
 
+	super->extra_space += space_needed;
+
 	return 0;
 }
 
@@ -4844,6 +4855,7 @@ static int init_super_imsm_volume(struct supertype *st, mdu_array_info_t *info,
 		super->anchor = mpb_new;
 		mpb->mpb_size = __cpu_to_le32(size_new);
 		memset(mpb_new + size_old, 0, size_round - size_old);
+		super->len = size_round;
 	}
 	super->current_vol = idx;
 
@@ -5382,6 +5394,7 @@ static int write_super_imsm(struct supertype *st, int doclose)
 	__u32 mpb_size = sizeof(struct imsm_super) - sizeof(struct imsm_disk);
 	int num_disks = 0;
 	int clear_migration_record = 1;
+	__u32 bbm_log_size;
 
 	/* 'generation' is incremented everytime the metadata is written */
 	generation = __le32_to_cpu(mpb->generation_num);
@@ -5419,9 +5432,23 @@ static int write_super_imsm(struct supertype *st, int doclose)
 		if (is_gen_migration(dev2))
 			clear_migration_record = 0;
 	}
-	mpb_size += __le32_to_cpu(mpb->bbm_log_size);
+
+	bbm_log_size = get_imsm_bbm_log_size(super->bbm_log);
+
+	if (bbm_log_size) {
+		memcpy((void *)mpb + mpb_size, super->bbm_log, bbm_log_size);
+		mpb->attributes |= MPB_ATTRIB_BBM;
+	} else
+		mpb->attributes &= ~MPB_ATTRIB_BBM;
+
+	super->anchor->bbm_log_size = __cpu_to_le32(bbm_log_size);
+	mpb_size += bbm_log_size;
 	mpb->mpb_size = __cpu_to_le32(mpb_size);
 
+#ifdef DEBUG
+	assert(super->len == 0 || mpb_size <= super->len);
+#endif
+
 	/* recalculate checksum */
 	sum = __gen_imsm_checksum(mpb);
 	mpb->check_sum = __cpu_to_le32(sum);
@@ -7104,6 +7131,7 @@ static int imsm_open_new(struct supertype *c, struct active_array *a,
 {
 	struct intel_super *super = c->sb;
 	struct imsm_super *mpb = super->anchor;
+	struct imsm_update_prealloc_bb_mem u;
 
 	if (atoi(inst) >= mpb->num_raid_devs) {
 		pr_err("subarry index %d, out of range\n", atoi(inst));
@@ -7112,6 +7140,10 @@ static int imsm_open_new(struct supertype *c, struct active_array *a,
 
 	dprintf("imsm: open_new %s\n", inst);
 	a->info.container_member = atoi(inst);
+
+	u.type = update_prealloc_badblocks_mem;
+	imsm_update_metadata_locally(c, &u, sizeof(u));
+
 	return 0;
 }
 
@@ -8854,6 +8886,9 @@ static void imsm_process_update(struct supertype *st,
 		}
 		break;
 	}
+	case update_prealloc_badblocks_mem: {
+		break;
+	}
 	default:
 		pr_err("error: unsuported process update type:(type: %d)\n",	type);
 	}
@@ -9094,6 +9129,11 @@ static int imsm_prepare_update(struct supertype *st,
 	case update_add_remove_disk:
 		/* no update->len needed */
 		break;
+	case update_prealloc_badblocks_mem: {
+		super->extra_space += sizeof(struct bbm_log) -
+			get_imsm_bbm_log_size(super->bbm_log);
+		break;
+	}
 	default:
 		return 0;
 	}
@@ -9104,12 +9144,13 @@ static int imsm_prepare_update(struct supertype *st,
 	else
 		buf_len = super->len;
 
-	if (__le32_to_cpu(mpb->mpb_size) + len > buf_len) {
+	if (__le32_to_cpu(mpb->mpb_size) + super->extra_space + len > buf_len) {
 		/* ok we need a larger buf than what is currently allocated
 		 * if this allocation fails process_update will notice that
 		 * ->next_len is set and ->next_buf is NULL
 		 */
-		buf_len = ROUND_UP(__le32_to_cpu(mpb->mpb_size) + len, 512);
+		buf_len = ROUND_UP(__le32_to_cpu(mpb->mpb_size) +
+				   super->extra_space + len, 512);
 		if (super->next_buf)
 			free(super->next_buf);
 
-- 
1.8.3.1

[PATCH 3/8] imsm: give md list of known bad blocks on startup

[Tomasz Majchrzak]

On create set bad block support flag for each drive. On assmble also
provide a list of known bad blocks. Bad blocks are stored in metadata
per disk so they have to be checked against volume boundaries
beforehand.

Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
---
 super-intel.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 59 insertions(+)

diff --git a/super-intel.c b/super-intel.c
index 0591c55..77a2ca9 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -880,6 +880,56 @@ static int load_bbm_log(struct intel_super *super)
 	return 0;
 }
 
+/* checks if bad block is within volume boundaries */
+static int is_bad_block_in_volume(const struct bbm_log_entry *entry,
+			const unsigned long long start_sector,
+			const unsigned long long size)
+{
+	unsigned long long bb_start;
+	unsigned long long bb_end;
+
+	bb_start = __le48_to_cpu(&entry->defective_block_start);
+	bb_end = bb_start + (entry->marked_count + 1);
+
+	if (((bb_start >= start_sector) && (bb_start < start_sector + size)) ||
+	    ((bb_end >= start_sector) && (bb_end <= start_sector + size)))
+		return 1;
+
+	return 0;
+}
+
+/* get list of bad blocks on a drive for a volume */
+static void get_volume_badblocks(const struct bbm_log *log, const __u8 idx,
+			const unsigned long long start_sector,
+			const unsigned long long size,
+			struct md_bb *bbs)
+{
+	__u32 count = 0;
+	__u32 i;
+
+	for (i = 0; i < log->entry_count; i++) {
+		const struct bbm_log_entry *ent =
+			&log->marked_block_entries[i];
+		struct md_bb_entry *bb;
+
+		if ((ent->disk_ordinal == idx) &&
+		    is_bad_block_in_volume(ent, start_sector, size)) {
+
+			if (!bbs->entries) {
+				bbs->entries = xmalloc(BBM_LOG_MAX_ENTRIES *
+						     sizeof(*bb));
+				if (!bbs->entries)
+					break;
+			}
+
+			bb = &bbs->entries[count++];
+			bb->sector = __le48_to_cpu(&ent->defective_block_start);
+			bb->length = ent->marked_count + 1;
+		}
+	}
+	bbs->count = count;
+}
+
 /*
  * for second_map:
  *  == MAP_0 get first map
@@ -2887,6 +2937,7 @@ static void getinfo_super_imsm_volume(struct supertype *st, struct mdinfo *info,
 							info->array.level,
 							info->array.chunk_size,
 							info->component_size);
+	info->bb.supported = 0;
 
 	memset(info->uuid, 0, sizeof(info->uuid));
 	info->recovery_start = MaxSector;
@@ -3053,6 +3104,7 @@ static void getinfo_super_imsm(struct supertype *st, struct mdinfo *info, char *
 	info->name[0] = 0;
 	info->recovery_start = MaxSector;
 	info->recovery_blocked = imsm_reshape_blocks_arrays_changes(st->sb);
+	info->bb.supported = 0;
 
 	/* do we have the all the insync disks that we expect? */
 	mpb = super->anchor;
@@ -6986,6 +7038,12 @@ static struct mdinfo *container_content_imsm(struct supertype *st, char *subarra
 			info_d->events = __le32_to_cpu(mpb->generation_num);
 			info_d->data_offset = pba_of_lba0(map);
 			info_d->component_size = blocks_per_member(map);
+
+			info_d->bb.supported = 0;
+			get_volume_badblocks(super->bbm_log, ord_to_idx(ord),
+					     info_d->data_offset,
+					     info_d->component_size,
+					     &info_d->bb);
 		}
 		/* now that the disk list is up-to-date fixup recovery_start */
 		update_recovery_start(super, dev, this);
@@ -7989,6 +8047,7 @@ static struct mdinfo *imsm_activate_spare(struct active_array *a,
 		di->data_offset = pba_of_lba0(map);
 		di->component_size = a->info.component_size;
 		di->container_member = inst;
+		di->bb.supported = 0;
 		super->random = random32();
 		di->next = rv;
 		rv = di;
-- 
1.8.3.1

[PATCH 4/8] imsm: record new bad block in bad block log

[Tomasz Majchrzak]

Check for a duplicate first or try to merge it with existing bad block.
If block range exceeds BBM_LOG_MAX_LBA_ENTRY_VAL (256) blocks, it must
be split into multiple ranges. Fail if maximum number of bad blocks has
been already reached.

Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
---
 super-intel.c | 142 ++++++++++++++++++++++++++++++++++++++++++++++++++++++----
 1 file changed, 134 insertions(+), 8 deletions(-)

diff --git a/super-intel.c b/super-intel.c
index 77a2ca9..5c54b8c 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -840,6 +840,86 @@ static __u32 get_imsm_bbm_log_size(struct bbm_log *log)
 		sizeof(log->entry_count) +
 		log->entry_count * sizeof(struct bbm_log_entry);
 }
+
+/* check if bad block is not partially stored in bbm log */
+static int is_stored_in_bbm(struct bbm_log *log, const __u8 idx, const unsigned
+			    long long sector, const int length, __u32 *pos)
+{
+	__u32 i;
+
+	for (i = *pos; i < log->entry_count; i++) {
+		struct bbm_log_entry *entry = &log->marked_block_entries[i];
+		unsigned long long bb_start;
+		unsigned long long bb_end;
+
+		bb_start = __le48_to_cpu(&entry->defective_block_start);
+		bb_end = bb_start + (entry->marked_count + 1);
+
+		if ((entry->disk_ordinal == idx) && (bb_start >= sector) &&
+		    (bb_end <= sector + length)) {
+			*pos = i;
+			return 1;
+		}
+	}
+	return 0;
+}
+
+/* record new bad block in bbm log */
+static int record_new_badblock(struct bbm_log *log, const __u8 idx, unsigned
+			       long long sector, int length)
+{
+	int new_bb = 0;
+	__u32 pos = 0;
+	struct bbm_log_entry *entry = NULL;
+
+	while (is_stored_in_bbm(log, idx, sector, length, &pos)) {
+		struct bbm_log_entry *e = &log->marked_block_entries[pos];
+
+		if ((e->marked_count + 1 == BBM_LOG_MAX_LBA_ENTRY_VAL) &&
+		    (__le48_to_cpu(&e->defective_block_start) == sector)) {
+			sector += BBM_LOG_MAX_LBA_ENTRY_VAL;
+			length -= BBM_LOG_MAX_LBA_ENTRY_VAL;
+			pos = pos + 1;
+			continue;
+		}
+		entry = e;
+		break;
+	}
+
+	if (entry) {
+		int cnt = (length <= BBM_LOG_MAX_LBA_ENTRY_VAL) ? length :
+			BBM_LOG_MAX_LBA_ENTRY_VAL;
+		entry->defective_block_start = __cpu_to_le48(sector);
+		entry->marked_count = cnt - 1;
+		if (cnt == length)
+			return 1;
+		sector += cnt;
+		length -= cnt;
+	}
+
+	new_bb = ROUND_UP(length, BBM_LOG_MAX_LBA_ENTRY_VAL) /
+		BBM_LOG_MAX_LBA_ENTRY_VAL;
+	if (log->entry_count + new_bb > BBM_LOG_MAX_ENTRIES)
+		return 0;
+
+	while (length > 0) {
+		int cnt = (length <= BBM_LOG_MAX_LBA_ENTRY_VAL) ? length :
+			BBM_LOG_MAX_LBA_ENTRY_VAL;
+		struct bbm_log_entry *entry =
+			&log->marked_block_entries[log->entry_count];
+
+		entry->defective_block_start = __cpu_to_le48(sector);
+		entry->marked_count = cnt - 1;
+		entry->disk_ordinal = idx;
+
+		sector += cnt;
+		length -= cnt;
+
+		log->entry_count++;
+	}
+
+	return new_bb;
+}
 #endif /* MDASSEMBLE */
 
 /* allocate and load BBM log from metadata */
@@ -7560,6 +7640,25 @@ skip_mark_checkpoint:
 	return consistent;
 }
 
+static int imsm_disk_slot_to_ord(struct active_array *a, int slot)
+{
+	int inst = a->info.container_member;
+	struct intel_super *super = a->container->sb;
+	struct imsm_dev *dev = get_imsm_dev(super, inst);
+	struct imsm_map *map = get_imsm_map(dev, MAP_0);
+
+	if (slot > map->num_members) {
+		pr_err("imsm: imsm_disk_slot_to_ord %d out of range 0..%d\n",
+		       slot, map->num_members - 1);
+		return -1;
+	}
+
+	if (slot < 0)
+		return -1;
+
+	return get_imsm_ord_tbl_ent(dev, slot, MAP_0);
+}
+
 static void imsm_set_disk(struct active_array *a, int n, int state)
 {
 	int inst = a->info.container_member;
@@ -7570,19 +7669,14 @@ static void imsm_set_disk(struct active_array *a, int n, int state)
 	struct mdinfo *mdi;
 	int recovery_not_finished = 0;
 	int failed;
-	__u32 ord;
+	int ord;
 	__u8 map_state;
 
-	if (n > map->num_members)
-		pr_err("imsm: set_disk %d out of range 0..%d\n",
-			n, map->num_members - 1);
-
-	if (n < 0)
+	ord = imsm_disk_slot_to_ord(a, n);
+	if (ord < 0)
 		return;
 
 	dprintf("imsm: set_disk %d:%x\n", n, state);
-
-	ord = get_imsm_ord_tbl_ent(dev, n, MAP_0);
 	disk = get_imsm_disk(super, ord_to_idx(ord));
 
 	/* check for new failures */
@@ -9477,6 +9571,37 @@ int validate_container_imsm(struct mdinfo *info)
 }
 #ifndef MDASSEMBLE
 /*******************************************************************************
+* Function:   imsm_record_badblock
+* Description: This routine stores new bad block record in BBM log
+*
+* Parameters:
+*     a		: array containing a bad block
+*     slot	: disk number containing a bad block
+*     sector	: bad block sector
+*     length	: bad block sectors range
+* Returns:
+*     1 : Success
+*     0 : Error
+******************************************************************************/
+static int imsm_record_badblock(struct active_array *a, int slot,
+			  unsigned long long sector, int length)
+{
+	struct intel_super *super = a->container->sb;
+	int ord;
+	int ret;
+
+	ord = imsm_disk_slot_to_ord(a, slot);
+	if (ord < 0)
+		return 0;
+
+	ret = record_new_badblock(super->bbm_log, ord_to_idx(ord), sector,
+				   length);
+	if (ret)
+		super->updates_pending++;
+
+	return ret;
+}
+/*******************************************************************************
  * Function:	init_migr_record_imsm
  * Description:	Function inits imsm migration record
  * Parameters:
@@ -11026,5 +11151,6 @@ struct superswitch super_imsm = {
 	.activate_spare = imsm_activate_spare,
 	.process_update = imsm_process_update,
 	.prepare_update = imsm_prepare_update,
+	.record_bad_block = imsm_record_badblock,
 #endif /* MDASSEMBLE */
 };
-- 
1.8.3.1

[PATCH 5/8] imsm: clear bad block from bad block log

[Tomasz Majchrzak]

Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
---
 super-intel.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 53 insertions(+)

diff --git a/super-intel.c b/super-intel.c
index 5c54b8c..0b84012 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -920,6 +920,28 @@ static int record_new_badblock(struct bbm_log *log, const __u8 idx, unsigned
 
 	return new_bb;
 }
+
+/* clear given bad block */
+static int clear_badblock(struct bbm_log *log, const __u8 idx, const unsigned
+			  long long sector, const int length) {
+	__u32 i = 0;
+
+	while (i < log->entry_count) {
+		struct bbm_log_entry *entries = log->marked_block_entries;
+
+		if ((entries[i].disk_ordinal == idx) &&
+		    (__le48_to_cpu(&entries[i].defective_block_start) ==
+		     sector) && (entries[i].marked_count + 1 == length)) {
+			if (i < log->entry_count - 1)
+				entries[i] = entries[log->entry_count - 1];
+			log->entry_count--;
+			break;
+		}
+		i++;
+	}
+
+	return 1;
+}
 #endif /* MDASSEMBLE */
 
 /* allocate and load BBM log from metadata */
@@ -9602,6 +9624,36 @@ static int imsm_record_badblock(struct active_array *a, int slot,
 	return ret;
 }
 /*******************************************************************************
+* Function:   imsm_clear_badblock
+* Description: This routine clears bad block record from BBM log
+*
+* Parameters:
+*     a		: array containing a bad block
+*     slot	: disk number containing a bad block
+*     sector	: bad block sector
+*     length	: bad block sectors range
+* Returns:
+*     1 : Success
+*     0 : Error
+******************************************************************************/
+static int imsm_clear_badblock(struct active_array *a, int slot,
+			unsigned long long sector, int length)
+{
+	struct intel_super *super = a->container->sb;
+	int ord;
+	int ret;
+
+	ord = imsm_disk_slot_to_ord(a, slot);
+	if (ord < 0)
+		return 0;
+
+	ret = clear_badblock(super->bbm_log, ord_to_idx(ord), sector, length);
+	if (ret)
+		super->updates_pending++;
+
+	return ret;
+}
+/*******************************************************************************
  * Function:	init_migr_record_imsm
  * Description:	Function inits imsm migration record
  * Parameters:
@@ -11152,5 +11204,6 @@ struct superswitch super_imsm = {
 	.process_update = imsm_process_update,
 	.prepare_update = imsm_prepare_update,
 	.record_bad_block = imsm_record_badblock,
+	.clear_bad_block  = imsm_clear_badblock,
 #endif /* MDASSEMBLE */
 };
-- 
1.8.3.1

[PATCH 6/8] imsm: clear bad blocks if disk becomes unavailable

[Tomasz Majchrzak]

If a disk fails or goes missing, clear the bad blocks associated with it
from metadata. If necessary, update disk ordinals.

Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
---
 super-intel.c | 46 +++++++++++++++++++++++++++++++++++++++-------
 1 file changed, 39 insertions(+), 7 deletions(-)

diff --git a/super-intel.c b/super-intel.c
index 0b84012..efcad01 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -921,6 +921,24 @@ static int record_new_badblock(struct bbm_log *log, const __u8 idx, unsigned
 	return new_bb;
 }
 
+/* clear all bad blocks for given disk */
+static void clear_disk_badblocks(struct bbm_log *log, const __u8 idx)
+{
+	__u32 i = 0;
+
+	while (i < log->entry_count) {
+		struct bbm_log_entry *entries = log->marked_block_entries;
+
+		if (entries[i].disk_ordinal == idx) {
+			if (i < log->entry_count - 1)
+				entries[i] = entries[log->entry_count - 1];
+			log->entry_count--;
+		} else {
+			i++;
+		}
+	}
+}
+
 /* clear given bad block */
 static int clear_badblock(struct bbm_log *log, const __u8 idx, const unsigned
 			  long long sector, const int length) {
@@ -7331,7 +7349,8 @@ static int is_resyncing(struct imsm_dev *dev)
 }
 
 /* return true if we recorded new information */
-static int mark_failure(struct imsm_dev *dev, struct imsm_disk *disk, int idx)
+static int mark_failure(struct intel_super *super,
+			struct imsm_dev *dev, struct imsm_disk *disk, int idx)
 {
 	__u32 ord;
 	int slot;
@@ -7373,12 +7392,16 @@ static int mark_failure(struct imsm_dev *dev, struct imsm_disk *disk, int idx)
 	}
 	if (map->failed_disk_num == 0xff)
 		map->failed_disk_num = slot;
+
+	clear_disk_badblocks(super->bbm_log, ord_to_idx(ord));
+
 	return 1;
 }
 
-static void mark_missing(struct imsm_dev *dev, struct imsm_disk *disk, int idx)
+static void mark_missing(struct intel_super *super,
+			 struct imsm_dev *dev, struct imsm_disk *disk, int idx)
 {
-	mark_failure(dev, disk, idx);
+	mark_failure(super, dev, disk, idx);
 
 	if (disk->scsi_id == __cpu_to_le32(~(__u32)0))
 		return;
@@ -7414,7 +7437,7 @@ static void handle_missing(struct intel_super *super, struct imsm_dev *dev)
 			end_migration(dev, super, map_state);
 	}
 	for (dl = super->missing; dl; dl = dl->next)
-		mark_missing(dev, &dl->disk, dl->index);
+		mark_missing(super, dev, &dl->disk, dl->index);
 	super->updates_pending++;
 }
 
@@ -7703,7 +7726,7 @@ static void imsm_set_disk(struct active_array *a, int n, int state)
 
 	/* check for new failures */
 	if (state & DS_FAULTY) {
-		if (mark_failure(dev, disk, ord_to_idx(ord)))
+		if (mark_failure(super, dev, disk, ord_to_idx(ord)))
 			super->updates_pending++;
 	}
 
@@ -8770,7 +8793,7 @@ static int apply_takeover_update(struct imsm_update_takeover *u,
 	for (du = super->missing; du; du = du->next)
 		if (du->index >= 0) {
 			set_imsm_ord_tbl_ent(map, du->index, du->index);
-			mark_missing(dv->dev, &du->disk, du->index);
+			mark_missing(super, dv->dev, &du->disk, du->index);
 		}
 
 	return 1;
@@ -9345,8 +9368,9 @@ static void imsm_delete(struct intel_super *super, struct dl **dlp, unsigned ind
 	struct dl *iter;
 	struct imsm_dev *dev;
 	struct imsm_map *map;
-	int i, j, num_members;
+	unsigned int i, j, num_members;
 	__u32 ord;
+	struct bbm_log *log = super->bbm_log;
 
 	dprintf("deleting device[%d] from imsm_super\n", index);
 
@@ -9379,6 +9403,14 @@ static void imsm_delete(struct intel_super *super, struct dl **dlp, unsigned ind
 		}
 	}
 
+	for (i = 0; i < log->entry_count; i++) {
+		struct bbm_log_entry *entry = &log->marked_block_entries[i];
+
+		if (entry->disk_ordinal <= index)
+			continue;
+		entry->disk_ordinal--;
+	}
+
 	mpb->num_disks--;
 	super->updates_pending++;
 	if (*dlp) {
-- 
1.8.3.1

[PATCH 7/8] imsm: provide list of bad blocks for an array

[Tomasz Majchrzak]

Provide list of bad blocks using memory allocated in advance so it's
safe to call it from monitor.

Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
---
 super-intel.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/super-intel.c b/super-intel.c
index efcad01..e795730 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -390,6 +390,7 @@ struct intel_super {
 	struct intel_hba *hba; /* device path of the raid controller for this metadata */
 	const struct imsm_orom *orom; /* platform firmware support */
 	struct intel_super *next; /* (temp) list for disambiguating family_num */
+	struct md_bb bb;	/* memory for get_bad_blocks call */
 };
 
 struct intel_disk {
@@ -4163,6 +4164,7 @@ static void __free_imsm(struct intel_super *super, int free_disks)
 static void free_imsm(struct intel_super *super)
 {
 	__free_imsm(super, 1);
+	free(super->bb.entries);
 	free(super);
 }
 
@@ -4183,6 +4185,14 @@ static struct intel_super *alloc_super(void)
 
 	super->current_vol = -1;
 	super->create_offset = ~((unsigned long long) 0);
+
+	super->bb.entries = malloc(BBM_LOG_MAX_ENTRIES *
+				   sizeof(struct md_bb_entry));
+	if (!super->bb.entries) {
+		free(super);
+		return NULL;
+	}
+
 	return super;
 }
 
@@ -9686,6 +9696,34 @@ static int imsm_clear_badblock(struct active_array *a, int slot,
 	return ret;
 }
 /*******************************************************************************
+* Function:   imsm_get_badblocks
+* Description: This routine get list of bad blocks for an array
+*
+* Parameters:
+*     a		: array
+*     slot	: disk number
+* Returns:
+*     bb	: structure containing bad blocks
+*     NULL	: error
+******************************************************************************/
+static struct md_bb *imsm_get_badblocks(struct active_array *a, int slot)
+{
+	int inst = a->info.container_member;
+	struct intel_super *super = a->container->sb;
+	struct imsm_dev *dev = get_imsm_dev(super, inst);
+	struct imsm_map *map = get_imsm_map(dev, MAP_0);
+	int ord;
+
+	ord = imsm_disk_slot_to_ord(a, slot);
+	if (ord < 0)
+		return NULL;
+
+	get_volume_badblocks(super->bbm_log, ord_to_idx(ord), pba_of_lba0(map),
+			     blocks_per_member(map), &super->bb);
+
+	return &super->bb;
+}
+/*******************************************************************************
  * Function:	init_migr_record_imsm
  * Description:	Function inits imsm migration record
  * Parameters:
@@ -11237,5 +11275,6 @@ struct superswitch super_imsm = {
 	.prepare_update = imsm_prepare_update,
 	.record_bad_block = imsm_record_badblock,
 	.clear_bad_block  = imsm_clear_badblock,
+	.get_bad_blocks   = imsm_get_badblocks,
 #endif /* MDASSEMBLE */
 };
-- 
1.8.3.1

[PATCH 8/8] imsm: implement "--examine-badblocks" command

[Tomasz Majchrzak]

Implement "--examine-badblocks" command to provide list of bad blocks in
metadata for a disk.

Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
---
 super-intel.c | 56 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 56 insertions(+)

diff --git a/super-intel.c b/super-intel.c
index e795730..c534afd 100644
--- a/super-intel.c
+++ b/super-intel.c
@@ -9724,6 +9724,61 @@ static struct md_bb *imsm_get_badblocks(struct active_array *a, int slot)
 	return &super->bb;
 }
 /*******************************************************************************
+* Function:   examine_badblocks_imsm
+* Description: Prints list of bad blocks on a disk to the standard output
+*
+* Parameters:
+*     st	: metadata handler
+*     fd	: open file descriptor for device
+*     devname	: device name
+* Returns:
+*     0 : Success
+*     1 : Error
+******************************************************************************/
+static int examine_badblocks_imsm(struct supertype *st, int fd, char *devname)
+{
+	struct intel_super *super = st->sb;
+	struct bbm_log *log = super->bbm_log;
+	struct dl *d = NULL;
+	int any = 0;
+
+	for (d = super->disks; d ; d = d->next) {
+		if (strcmp(d->devname, devname) == 0)
+			break;
+	}
+
+	if ((d == NULL) || (d->index < 0)) { /* serial mismatch probably */
+		pr_err("%s doesn't appear to be part of a raid array\n",
+		       devname);
+		return 1;
+	}
+
+	if (log != NULL) {
+		unsigned int i;
+		struct bbm_log_entry *entry = &log->marked_block_entries[0];
+
+		for (i = 0; i < log->entry_count; i++) {
+			if (entry[i].disk_ordinal == d->index) {
+				unsigned long long sector = __le48_to_cpu(
+					&entry[i].defective_block_start);
+				int cnt = entry[i].marked_count + 1;
+
+				if (!any) {
+					printf("Bad-blocks on %s:\n", devname);
+					any = 1;
+				}
+
+				printf("%20llu for %d sectors\n", sector, cnt);
+			}
+		}
+	}
+
+	if (!any)
+		printf("No bad-blocks list configured on %s\n", devname);
+
+	return 0;
+}
+/*******************************************************************************
  * Function:	init_migr_record_imsm
  * Description:	Function inits imsm migration record
  * Parameters:
@@ -11241,6 +11296,7 @@ struct superswitch super_imsm = {
 	.manage_reshape = imsm_manage_reshape,
 	.recover_backup = recover_backup_imsm,
 	.copy_metadata = copy_metadata_imsm,
+	.examine_badblocks = examine_badblocks_imsm,
 #endif
 	.match_home	= match_home_imsm,
 	.uuid_from_super= uuid_from_super_imsm,
-- 
1.8.3.1

Re: [PATCH 1/8] imsm: parse bad block log in metadata on startup

[Jes Sorensen]

Tomasz Majchrzak <tomasz.majchrzak@intel.com> writes:
> Always allocate memory for all log entries to avoid a need for memory
> allocation when monitor requests to record a bad block.
>
> Also some extra checks added to make static code analyzer happy.
>
> Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
> ---
>  super-intel.c | 158 +++++++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 112 insertions(+), 46 deletions(-)

Thanks - just a note, I am at Linux Plumbers in Santa Fe this week, and
on PTO next week, so it'll probably be two weeks before I will have
time to look at this.

If you haven't heard from my by three weeks from now, please feel free
to turn on all caps and yell at me.

Cheers,
Jes

> diff --git a/super-intel.c b/super-intel.c
> index c146bbd..5d6d534 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -217,22 +217,24 @@ struct imsm_super {
>  } __attribute__ ((packed));
>  
>  #define BBM_LOG_MAX_ENTRIES 254
> +#define BBM_LOG_MAX_LBA_ENTRY_VAL 256		/* Represents 256 LBAs */
> +#define BBM_LOG_SIGNATURE 0xABADB10C
> +
> +struct bbm_log_block_addr {
> +	__u16 w1;
> +	__u32 dw1;
> +} __attribute__ ((__packed__));
>  
>  struct bbm_log_entry {
> -	__u64 defective_block_start;
> -#define UNREADABLE 0xFFFFFFFF
> -	__u32 spare_block_offset;
> -	__u16 remapped_marked_count;
> -	__u16 disk_ordinal;
> +	__u8 marked_count;		/* Number of blocks marked - 1 */
> +	__u8 disk_ordinal;		/* Disk entry within the imsm_super */
> +	struct bbm_log_block_addr defective_block_start;
>  } __attribute__ ((__packed__));
>  
>  struct bbm_log {
>  	__u32 signature; /* 0xABADB10C */
>  	__u32 entry_count;
> -	__u32 reserved_spare_block_count; /* 0 */
> -	__u32 reserved; /* 0xFFFF */
> -	__u64 first_spare_lba;
> -	struct bbm_log_entry mapped_block_entries[BBM_LOG_MAX_ENTRIES];
> +	struct bbm_log_entry marked_block_entries[BBM_LOG_MAX_ENTRIES];
>  } __attribute__ ((__packed__));
>  
>  #ifndef MDASSEMBLE
> @@ -785,6 +787,92 @@ static struct imsm_dev *get_imsm_dev(struct intel_super *super, __u8 index)
>  	return NULL;
>  }
>  
> +#if BYTE_ORDER == LITTLE_ENDIAN
> +static inline unsigned long long __le48_to_cpu(const struct bbm_log_block_addr
> +					       *addr)
> +{
> +	return ((((__u64)addr->dw1) << 16) | addr->w1);
> +}
> +
> +static inline struct bbm_log_block_addr __cpu_to_le48(unsigned long long sec)
> +{
> +	struct bbm_log_block_addr addr;
> +
> +	addr.w1 =  (__u16)(sec & 0xFFFF);
> +	addr.dw1 = (__u32)((sec >> 16) & 0xFFFFFFFF);
> +	return addr;
> +}
> +#elif BYTE_ORDER == BIG_ENDIAN
> +static inline unsigned long long __le48_to_cpu(const struct bbm_log_block_addr
> +					       *addr)
> +{
> +	return ((((__u64)__le32_to_cpu(addr->dw1)) << 16) |
> +		__le16_to_cpu(addr->w1));
> +}
> +
> +static inline struct bbm_log_block_addr __cpu_to_le48(unsigned long long sec)
> +{
> +	struct bbm_log_block_addr addr;
> +
> +	addr.w1 =  __cpu_to_le16((__u16)(sec & 0xFFFF));
> +	addr.dw1 = __cpu_to_le32((__u32)(sec >> 16) & 0xFFFFFFFF);
> +	return addr;
> +}
> +#else
> +#  error "unknown endianness."
> +#endif
> +
> +#ifndef MDASSEMBLE
> +/* get size of the bbm log */
> +static __u32 get_imsm_bbm_log_size(struct bbm_log *log)
> +{
> +	if (!log || log->entry_count == 0)
> +		return 0;
> +
> +	return sizeof(log->signature) +
> +		sizeof(log->entry_count) +
> +		log->entry_count * sizeof(struct bbm_log_entry);
> +}
> +#endif /* MDASSEMBLE */
> +
> +/* allocate and load BBM log from metadata */
> +static int load_bbm_log(struct intel_super *super)
> +{
> +	struct imsm_super *mpb = super->anchor;
> +	__u32 bbm_log_size =  __le32_to_cpu(mpb->bbm_log_size);
> +
> +	super->bbm_log = malloc(sizeof(struct bbm_log));
> +	if (!super->bbm_log)
> +		return 1;
> +
> +	if (bbm_log_size) {
> +		struct bbm_log *log = (void *)mpb +
> +			__le32_to_cpu(mpb->mpb_size) - bbm_log_size;
> +		__u32 entry_count;
> +
> +		if (bbm_log_size < sizeof(log->signature) +
> +		    sizeof(log->entry_count))
> +			return 2;
> +
> +		entry_count = __le32_to_cpu(log->entry_count);
> +		if ((__le32_to_cpu(log->signature) != BBM_LOG_SIGNATURE) ||
> +		    (entry_count > BBM_LOG_MAX_ENTRIES))
> +			return 3;
> +
> +		if (bbm_log_size !=
> +		    sizeof(log->signature) + sizeof(log->entry_count) +
> +		    entry_count * sizeof(struct bbm_log_entry))
> +			return 4;
> +
> +		memcpy(super->bbm_log, log, bbm_log_size);
> +	} else {
> +		super->bbm_log->signature = __cpu_to_le32(BBM_LOG_SIGNATURE);
> +		super->bbm_log->entry_count = 0;
> +	}
> +
> +	return 0;
> +}
> +
>  /*
>   * for second_map:
>   *  == MAP_0 get first map
> @@ -1433,7 +1521,7 @@ static void examine_super_imsm(struct supertype *st, char *homehost)
>  	printf("          Disks : %d\n", mpb->num_disks);
>  	printf("   RAID Devices : %d\n", mpb->num_raid_devs);
>  	print_imsm_disk(__get_imsm_disk(mpb, super->disks->index), super->disks->index, reserved);
> -	if (super->bbm_log) {
> +	if (get_imsm_bbm_log_size(super->bbm_log)) {
>  		struct bbm_log *log = super->bbm_log;
>  
>  		printf("\n");
> @@ -1441,9 +1529,6 @@ static void examine_super_imsm(struct supertype *st, char *homehost)
>  		printf("       Log Size : %d\n", __le32_to_cpu(mpb->bbm_log_size));
>  		printf("      Signature : %x\n", __le32_to_cpu(log->signature));
>  		printf("    Entry Count : %d\n", __le32_to_cpu(log->entry_count));
> -		printf("   Spare Blocks : %d\n",  __le32_to_cpu(log->reserved_spare_block_count));
> -		printf("    First Spare : %llx\n",
> -		       (unsigned long long) __le64_to_cpu(log->first_spare_lba));
>  	}
>  	for (i = 0; i < mpb->num_raid_devs; i++) {
>  		struct mdinfo info;
> @@ -3628,19 +3713,6 @@ static int parse_raid_devices(struct intel_super *super)
>  	return 0;
>  }
>  
> -/* retrieve a pointer to the bbm log which starts after all raid devices */
> -struct bbm_log *__get_imsm_bbm_log(struct imsm_super *mpb)
> -{
> -	void *ptr = NULL;
> -
> -	if (__le32_to_cpu(mpb->bbm_log_size)) {
> -		ptr = mpb;
> -		ptr += mpb->mpb_size - __le32_to_cpu(mpb->bbm_log_size);
> -	}
> -
> -	return ptr;
> -}
> -
>  /*******************************************************************************
>   * Function:	check_mpb_migr_compatibility
>   * Description:	Function checks for unsupported migration features:
> @@ -3790,12 +3862,6 @@ static int load_imsm_mpb(int fd, struct intel_super *super, char *devname)
>  		return 3;
>  	}
>  
> -	/* FIXME the BBM log is disk specific so we cannot use this global
> -	 * buffer for all disks.  Ok for now since we only look at the global
> -	 * bbm_log_size parameter to gate assembly
> -	 */
> -	super->bbm_log = __get_imsm_bbm_log(super->anchor);
> -
>  	return 0;
>  }
>  
> @@ -3839,6 +3905,9 @@ load_and_parse_mpb(int fd, struct intel_super *super, char *devname, int keep_fd
>  	if (err)
>  		return err;
>  	err = parse_raid_devices(super);
> +	if (err)
> +		return err;
> +	err = load_bbm_log(super);
>  	clear_hi(super);
>  	return err;
>  }
> @@ -3903,6 +3972,8 @@ static void __free_imsm(struct intel_super *super, int free_disks)
>  		free(elem);
>  		elem = next;
>  	}
> +	if (super->bbm_log)
> +		free(super->bbm_log);
>  	super->hba = NULL;
>  }
>  
> @@ -4508,7 +4579,7 @@ static int get_super_block(struct intel_super **super_list, char *devnm, char *d
>  		*super_list = s;
>  	} else {
>  		if (s)
> -			free(s);
> +			free_imsm(s);
>  		if (dfd >= 0)
>  			close(dfd);
>  	}
> @@ -4570,6 +4641,8 @@ static int load_super_imsm(struct supertype *st, int fd, char *devname)
>  	free_super_imsm(st);
>  
>  	super = alloc_super();
> +	if (!super)
> +		return 1;
>  	/* Load hba and capabilities if they exist.
>  	 * But do not preclude loading metadata in case capabilities or hba are
>  	 * non-compliant and ignore_hw_compat is set.
> @@ -4912,7 +4985,7 @@ static int init_super_imsm(struct supertype *st, mdu_array_info_t *info,
>  
>  	super = alloc_super();
>  	if (super && posix_memalign(&super->buf, 512, mpb_size) != 0) {
> -		free(super);
> +		free_imsm(super);
>  		super = NULL;
>  	}
>  	if (!super) {
> @@ -4922,7 +4995,7 @@ static int init_super_imsm(struct supertype *st, mdu_array_info_t *info,
>  	if (posix_memalign(&super->migr_rec_buf, 512, MIGR_REC_BUF_SIZE) != 0) {
>  		pr_err("could not allocate migr_rec buffer\n");
>  		free(super->buf);
> -		free(super);
> +		free_imsm(super);
>  		return 0;
>  	}
>  	memset(super->buf, 0, mpb_size);
> @@ -5489,11 +5562,6 @@ static int store_super_imsm(struct supertype *st, int fd)
>  #endif
>  }
>  
> -static int imsm_bbm_log_size(struct imsm_super *mpb)
> -{
> -	return __le32_to_cpu(mpb->bbm_log_size);
> -}
> -
>  #ifndef MDASSEMBLE
>  static int validate_geometry_imsm_container(struct supertype *st, int level,
>  					    int layout, int raiddisks, int chunk,
> @@ -5529,6 +5597,10 @@ static int validate_geometry_imsm_container(struct supertype *st, int level,
>  	 * note that there is no fd for the disks in array.
>  	 */
>  	super = alloc_super();
> +	if (!super) {
> +		close(fd);
> +		return 0;
> +	}
>  	rv = find_intel_hba_capability(fd, super, verbose > 0 ? dev : NULL);
>  	if (rv != 0) {
>  #if DEBUG
> @@ -6760,12 +6832,6 @@ static struct mdinfo *container_content_imsm(struct supertype *st, char *subarra
>  		pr_err("Unsupported attributes in IMSM metadata.Arrays activation is blocked.\n");
>  	}
>  
> -	/* check for bad blocks */
> -	if (imsm_bbm_log_size(super->anchor)) {
> -		pr_err("BBM log found in IMSM metadata.Arrays activation is blocked.\n");
> -		sb_errors = 1;
> -	}
> -
>  	/* count spare devices, not used in maps
>  	 */
>  	for (d = super->disks; d; d = d->next)

Re: [PATCH 1/8] imsm: parse bad block log in metadata on startup

[Jes Sorensen]

Tomasz Majchrzak <tomasz.majchrzak@intel.com> writes:
> Always allocate memory for all log entries to avoid a need for memory
> allocation when monitor requests to record a bad block.
>
> Also some extra checks added to make static code analyzer happy.
>
> Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
> ---
>  super-intel.c | 158 +++++++++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 112 insertions(+), 46 deletions(-)

Tomasz,

A couple of comments on this one:

First of all: *Always* provide a cover letter when submitting
multi-patch sets.

> diff --git a/super-intel.c b/super-intel.c
> index c146bbd..5d6d534 100644
> --- a/super-intel.c
> +++ b/super-intel.c
> @@ -217,22 +217,24 @@ struct imsm_super {
>  } __attribute__ ((packed));
>  
>  #define BBM_LOG_MAX_ENTRIES 254
> +#define BBM_LOG_MAX_LBA_ENTRY_VAL 256		/* Represents 256 LBAs */
> +#define BBM_LOG_SIGNATURE 0xABADB10C

I know the old code is messy with regard to this, but lets get it right
from now on. Numbers are lower-case and #define NAMES are upper case.

> @@ -785,6 +787,92 @@ static struct imsm_dev *get_imsm_dev(struct intel_super *super, __u8 index)
>  	return NULL;
>  }
>  
> +#if BYTE_ORDER == LITTLE_ENDIAN
> +static inline unsigned long long __le48_to_cpu(const struct bbm_log_block_addr
> +					       *addr)
> +{
> +	return ((((__u64)addr->dw1) << 16) | addr->w1);
> +}
> +
> +static inline struct bbm_log_block_addr __cpu_to_le48(unsigned long long sec)
> +{
> +	struct bbm_log_block_addr addr;
> +
> +	addr.w1 =  (__u16)(sec & 0xFFFF);
> +	addr.dw1 = (__u32)((sec >> 16) & 0xFFFFFFFF);
> +	return addr;
> +}

Again, 0xffff/0xffffffff

> +#elif BYTE_ORDER == BIG_ENDIAN
> +static inline unsigned long long __le48_to_cpu(const struct bbm_log_block_addr
> +					       *addr)
> +{
> +	return ((((__u64)__le32_to_cpu(addr->dw1)) << 16) |
> +		__le16_to_cpu(addr->w1));
> +}
> +
> +static inline struct bbm_log_block_addr __cpu_to_le48(unsigned long long sec)
> +{
> +	struct bbm_log_block_addr addr;
> +
> +	addr.w1 =  __cpu_to_le16((__u16)(sec & 0xFFFF));
> +	addr.dw1 = __cpu_to_le32((__u32)(sec >> 16) & 0xFFFFFFFF);
> +	return addr;
> +}

Given that __cpu_to_le*()/__le*_to_cpu*() are no-ops on little-endian,
you don't really need two versions of these. The big-endian version
should suffice for both, which makes it far less cluttered. Unless I got
something wrong here of course.

> +#else
> +#  error "unknown endianness."
> +#endif
> +
> +#ifndef MDASSEMBLE
> +/* get size of the bbm log */
> +static __u32 get_imsm_bbm_log_size(struct bbm_log *log)
> +{
> +	if (!log || log->entry_count == 0)
> +		return 0;
> +
> +	return sizeof(log->signature) +
> +		sizeof(log->entry_count) +
> +		log->entry_count * sizeof(struct bbm_log_entry);
> +}
> +#endif /* MDASSEMBLE */
> +
> +/* allocate and load BBM log from metadata */
> +static int load_bbm_log(struct intel_super *super)
> +{
> +	struct imsm_super *mpb = super->anchor;
> +	__u32 bbm_log_size =  __le32_to_cpu(mpb->bbm_log_size);
> +
> +	super->bbm_log = malloc(sizeof(struct bbm_log));
> +	if (!super->bbm_log)
> +		return 1;
> +
> +	if (bbm_log_size) {
> +		struct bbm_log *log = (void *)mpb +
> +			__le32_to_cpu(mpb->mpb_size) - bbm_log_size;
> +		__u32 entry_count;

Put the assignment on a separate line please when it's this long.

> +
> +		if (bbm_log_size < sizeof(log->signature) +
> +		    sizeof(log->entry_count))
> +			return 2;
> +
> +		entry_count = __le32_to_cpu(log->entry_count);
> +		if ((__le32_to_cpu(log->signature) != BBM_LOG_SIGNATURE) ||
> +		    (entry_count > BBM_LOG_MAX_ENTRIES))
> +			return 3;
> +
> +		if (bbm_log_size !=
> +		    sizeof(log->signature) + sizeof(log->entry_count) +
> +		    entry_count * sizeof(struct bbm_log_entry))
> +			return 4;
> +
> +		memcpy(super->bbm_log, log, bbm_log_size);
> +	} else {
> +		super->bbm_log->signature = __cpu_to_le32(BBM_LOG_SIGNATURE);
> +		super->bbm_log->entry_count = 0;
> +	}

This part looks problematic - you don't clear super->bbm_log and if
bbm_log_size == 0 you end up leaving it with random data. Wouldn't it be
better to just use calloc()?

Jes

Re: [PATCH 2/8] imsm: write bad block log on metadata sync

[Jes Sorensen]

Tomasz Majchrzak <tomasz.majchrzak@intel.com> writes:
> Pre-allocate memory for largest possible bad block sectionwhen monitor
> is being opened to avoid a need for memory allocation on metadata sync.
>
> If memory for a structure has been allocated in mpb buffer but it hasn't
> been used yet, it will be taken by next buffer grow, leading to
> insufficient memory on metadata flush. Start tracking such memory and
> take it into calculation when growing a buffer. Also assert has been
> added to debug mode to warn when more metadata has been written than
> memory allocated.
>
> Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
> ---
>  super-intel.c | 49 +++++++++++++++++++++++++++++++++++++++++++++----
>  1 file changed, 45 insertions(+), 4 deletions(-)
>
> diff --git a/super-intel.c b/super-intel.c
> index 5d6d534..0591c55 100644
> --- a/super-intel.c
> +++ b/super-intel.c

[snip]

> @@ -8854,6 +8886,9 @@ static void imsm_process_update(struct supertype *st,
>  		}
>  		break;
>  	}
> +	case update_prealloc_badblocks_mem: {
> +		break;
> +	}
>  	default:
>  		pr_err("error: unsuported process update type:(type: %d)\n",	type);
>  	}

Please don't include those awful brackets if you don't need them here.

> @@ -9094,6 +9129,11 @@ static int imsm_prepare_update(struct supertype *st,
>  	case update_add_remove_disk:
>  		/* no update->len needed */
>  		break;
> +	case update_prealloc_badblocks_mem: {
> +		super->extra_space += sizeof(struct bbm_log) -
> +			get_imsm_bbm_log_size(super->bbm_log);
> +		break;
> +	}
>  	default:
>  		return 0;
>  	}

Same here

Thanks,
Jes

Re: [PATCH 1/8] imsm: parse bad block log in metadata on startup

[Tomasz Majchrzak]

On Wed, Nov 16, 2016 at 09:43:18AM -0500, Jes Sorensen wrote:
> Tomasz Majchrzak <tomasz.majchrzak@intel.com> writes:
> > Always allocate memory for all log entries to avoid a need for memory
> > allocation when monitor requests to record a bad block.
> >
> > Also some extra checks added to make static code analyzer happy.
> >
> > Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
> > ---
> >  super-intel.c | 158 +++++++++++++++++++++++++++++++++++++++++-----------------
> >  1 file changed, 112 insertions(+), 46 deletions(-)
> 
> > @@ -785,6 +787,92 @@ static struct imsm_dev *get_imsm_dev(struct intel_super *super, __u8 index)
> >  	return NULL;
> >  }
> >  
> > +#if BYTE_ORDER == LITTLE_ENDIAN
> > +static inline unsigned long long __le48_to_cpu(const struct bbm_log_block_addr
> > +					       *addr)
> > +{
> > +	return ((((__u64)addr->dw1) << 16) | addr->w1);
> > +}
> > +
> > +static inline struct bbm_log_block_addr __cpu_to_le48(unsigned long long sec)
> > +{
> > +	struct bbm_log_block_addr addr;
> > +
> > +	addr.w1 =  (__u16)(sec & 0xFFFF);
> > +	addr.dw1 = (__u32)((sec >> 16) & 0xFFFFFFFF);
> > +	return addr;
> > +}
> 
> Again, 0xffff/0xffffffff
> 
> > +#elif BYTE_ORDER == BIG_ENDIAN
> > +static inline unsigned long long __le48_to_cpu(const struct bbm_log_block_addr
> > +					       *addr)
> > +{
> > +	return ((((__u64)__le32_to_cpu(addr->dw1)) << 16) |
> > +		__le16_to_cpu(addr->w1));
> > +}
> > +
> > +static inline struct bbm_log_block_addr __cpu_to_le48(unsigned long long sec)
> > +{
> > +	struct bbm_log_block_addr addr;
> > +
> > +	addr.w1 =  __cpu_to_le16((__u16)(sec & 0xFFFF));
> > +	addr.dw1 = __cpu_to_le32((__u32)(sec >> 16) & 0xFFFFFFFF);
> > +	return addr;
> > +}
> 
> Given that __cpu_to_le*()/__le*_to_cpu*() are no-ops on little-endian,
> you don't really need two versions of these. The big-endian version
> should suffice for both, which makes it far less cluttered. Unless I got
> something wrong here of course.
> 
> > +#else
> > +#  error "unknown endianness."
> > +#endif
> > +

Hi Jes,

Internally IMSM metadata stores bad block address as 48-bit little-endian
value. Those functions provide conversion from/to a structure consisting of
2 fields (32 + 16 bits). For little-endian CPU it's just about shifting bits,
for big-endian CPU bits have to be swapped. IMSM is not available on
big-endian platforms so big-endian implementation is provided for
completeness. I haven't changed it in my latest patch.

Regards,

Tomek

Re: [PATCH 1/8] imsm: parse bad block log in metadata on startup

[Jes Sorensen]

Tomasz Majchrzak <tomasz.majchrzak@intel.com> writes:
> On Wed, Nov 16, 2016 at 09:43:18AM -0500, Jes Sorensen wrote:
>> Tomasz Majchrzak <tomasz.majchrzak@intel.com> writes:
>> > Always allocate memory for all log entries to avoid a need for memory
>> > allocation when monitor requests to record a bad block.
>> >
>> > Also some extra checks added to make static code analyzer happy.
>> >
>> > Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
>> > ---
>> >  super-intel.c | 158
>> > +++++++++++++++++++++++++++++++++++++++++-----------------
>> >  1 file changed, 112 insertions(+), 46 deletions(-)
>> 
>> > @@ -785,6 +787,92 @@ static struct imsm_dev *get_imsm_dev(struct intel_super *super, __u8 index)
>> >  	return NULL;
>> >  }
>> >  
>> > +#if BYTE_ORDER == LITTLE_ENDIAN
>> > +static inline unsigned long long __le48_to_cpu(const struct bbm_log_block_addr
>> > +					       *addr)
>> > +{
>> > +	return ((((__u64)addr->dw1) << 16) | addr->w1);
>> > +}
>> > +
>> > +static inline struct bbm_log_block_addr __cpu_to_le48(unsigned long long sec)
>> > +{
>> > +	struct bbm_log_block_addr addr;
>> > +
>> > +	addr.w1 =  (__u16)(sec & 0xFFFF);
>> > +	addr.dw1 = (__u32)((sec >> 16) & 0xFFFFFFFF);
>> > +	return addr;
>> > +}
>> 
>> Again, 0xffff/0xffffffff
>> 
>> > +#elif BYTE_ORDER == BIG_ENDIAN
>> > +static inline unsigned long long __le48_to_cpu(const struct
>> > bbm_log_block_addr
>> > +					       *addr)
>> > +{
>> > +	return ((((__u64)__le32_to_cpu(addr->dw1)) << 16) |
>> > +		__le16_to_cpu(addr->w1));
>> > +}
>> > +
>> > +static inline struct bbm_log_block_addr __cpu_to_le48(unsigned
>> > long long sec)
>> > +{
>> > +	struct bbm_log_block_addr addr;
>> > +
>> > +	addr.w1 =  __cpu_to_le16((__u16)(sec & 0xFFFF));
>> > +	addr.dw1 = __cpu_to_le32((__u32)(sec >> 16) & 0xFFFFFFFF);
>> > +	return addr;
>> > +}
>> 
>> Given that __cpu_to_le*()/__le*_to_cpu*() are no-ops on little-endian,
>> you don't really need two versions of these. The big-endian version
>> should suffice for both, which makes it far less cluttered. Unless I got
>> something wrong here of course.
>> 
>> > +#else
>> > +#  error "unknown endianness."
>> > +#endif
>> > +
>
> Hi Jes,
>
> Internally IMSM metadata stores bad block address as 48-bit little-endian
> value. Those functions provide conversion from/to a structure consisting of
> 2 fields (32 + 16 bits). For little-endian CPU it's just about shifting bits,
> for big-endian CPU bits have to be swapped. IMSM is not available on
> big-endian platforms so big-endian implementation is provided for
> completeness. I haven't changed it in my latest patch.

Hi Tomek,

My point is that __cpu_to_le{16,32}() are no-ops on little-endian so the
above function using those macros would be valid for both endianess.
While I do understand that no big endian system is available with IMSM
BIOS support, we should still allow big endian users to put IMSM disks
their systems for recovery purposes etc.

Cheers,
Jes