[PATCH 1/4 v2] mdadm: bad block support for external metadata - initialization

[PATCH 1/4 v2] mdadm: bad block support for external metadata - initialization

[Tomasz Majchrzak]

If metadata handler provides support for bad blocks, tell md by writing
'external_bbl' to rdev state file (both on create and assemble),
followed by a list of known bad blocks written via sysfs 'bad_blocks'
file.

Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
Reviewed-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
---
 mdadm.h | 13 +++++++++++++
 sysfs.c | 29 ++++++++++++++++++++++++++++-
 2 files changed, 41 insertions(+), 1 deletion(-)

diff --git a/mdadm.h b/mdadm.h
index 0516c82..5156ea4 100755
--- a/mdadm.h
+++ b/mdadm.h
@@ -237,6 +237,17 @@ struct dlm_lksb {
 
 extern const char Name[];
 
+struct md_bb_entry {
+	unsigned long long sector;
+	int length;
+};
+
+struct md_bb {
+	int supported;
+	int count;
+	struct md_bb_entry *entries;
+};
+
 /* general information that might be extracted from a superblock */
 struct mdinfo {
 	mdu_array_info_t	array;
@@ -311,6 +322,8 @@ struct mdinfo {
 
 	/* info read from sysfs */
 	char		sysfs_array_state[20];
+
+	struct md_bb bb;
 };
 
 struct createinfo {
diff --git a/sysfs.c b/sysfs.c
index d28e21a..c7a8e66 100644
--- a/sysfs.c
+++ b/sysfs.c
@@ -50,8 +50,12 @@ void sysfs_free(struct mdinfo *sra)
 		while (sra->devs) {
 			struct mdinfo *d = sra->devs;
 			sra->devs = d->next;
+			if (d->bb.entries)
+				free(d->bb.entries);
 			free(d);
 		}
+		if (sra->bb.entries)
+			free(sra->bb.entries);
 		free(sra);
 		sra = sra2;
 	}
@@ -259,7 +263,7 @@ struct mdinfo *sysfs_read(int fd, char *devnm, unsigned long options)
 		dbase = base + strlen(base);
 		*dbase++ = '/';
 
-		dev = xmalloc(sizeof(*dev));
+		dev = xcalloc(1, sizeof(*dev));
 
 		/* Always get slot, major, minor */
 		strcpy(dbase, "slot");
@@ -687,6 +691,7 @@ int sysfs_add_disk(struct mdinfo *sra, struct mdinfo *sd, int resume)
 	char nm[PATH_MAX];
 	char *dname;
 	int rv;
+	int i;
 
 	sprintf(dv, "%d:%d", sd->disk.major, sd->disk.minor);
 	rv = sysfs_set_str(sra, NULL, "new_dev", dv);
@@ -718,6 +723,28 @@ int sysfs_add_disk(struct mdinfo *sra, struct mdinfo *sd, int resume)
 		if (resume)
 			sysfs_set_num(sra, sd, "recovery_start", sd->recovery_start);
 	}
+	if (sd->bb.supported) {
+		if (sysfs_set_str(sra, sd, "state", "external_bbl")) {
+			/*
+			 * backward compatibility - if kernel doesn't support
+			 * bad blocks for external metadata, let it continue
+			 * as long as there are none known so far
+			 */
+			if (sd->bb.count) {
+				pr_err("The kernel has no support for bad blocks in external metadata\n");
+				return -1;
+			}
+		}
+
+		for (i = 0; i < sd->bb.count; i++) {
+			char s[30];
+			const struct md_bb_entry *entry = &sd->bb.entries[i];
+
+			snprintf(s, sizeof(s) - 1, "%llu %d\n", entry->sector,
+				 entry->length);
+			rv |= sysfs_set_str(sra, sd, "bad_blocks", s);
+		}
+	}
 	return rv;
 }
 
-- 
1.8.3.1

[PATCH 2/4 v2] mdmon: bad block support for external metadata - sysfs file open

[Tomasz Majchrzak]

Open 'badblocks' and 'unacknowledged_bad_blocks' sysfs files for each
disk in the array. Add them to the list of files observed by monitor.

Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
Reviewed-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
---
 managemon.c | 17 +++++++++++++++++
 mdadm.h     |  2 ++
 monitor.c   |  7 ++++++-
 3 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/managemon.c b/managemon.c
index 6d1b3d8..3c1d4cb 100644
--- a/managemon.c
+++ b/managemon.c
@@ -115,6 +115,8 @@ static void close_aa(struct active_array *aa)
 	for (d = aa->info.devs; d; d = d->next) {
 		close(d->recovery_fd);
 		close(d->state_fd);
+		close(d->bb_fd);
+		close(d->ubb_fd);
 	}
 
 	if (aa->action_fd >= 0)
@@ -433,6 +435,21 @@ static int disk_init_and_add(struct mdinfo *disk, struct mdinfo *clone,
 		close(disk->recovery_fd);
 		return -1;
 	}
+	disk->bb_fd = sysfs_open2(aa->info.sys_name, disk->sys_name,
+				 "bad_blocks");
+	if (disk->bb_fd < 0) {
+		close(disk->recovery_fd);
+		close(disk->state_fd);
+		return -1;
+	}
+	disk->ubb_fd = sysfs_open2(aa->info.sys_name, disk->sys_name,
+				  "unacknowledged_bad_blocks");
+	if (disk->ubb_fd < 0) {
+		close(disk->recovery_fd);
+		close(disk->state_fd);
+		close(disk->bb_fd);
+		return -1;
+	}
 	disk->prev_state = read_dev_state(disk->state_fd);
 	disk->curr_state = disk->prev_state;
 	disk->next = aa->info.devs;
diff --git a/mdadm.h b/mdadm.h
index 5156ea4..1a1c7af 100755
--- a/mdadm.h
+++ b/mdadm.h
@@ -311,6 +311,8 @@ struct mdinfo {
 	/* Device info for mdmon: */
 	int recovery_fd;
 	int state_fd;
+	int bb_fd;
+	int ubb_fd;
 	#define DS_FAULTY	1
 	#define	DS_INSYNC	2
 	#define	DS_WRITE_MOSTLY	4
diff --git a/monitor.c b/monitor.c
index 4c79ce2..56c6500 100644
--- a/monitor.c
+++ b/monitor.c
@@ -454,6 +454,8 @@ static int read_and_act(struct active_array *a)
 				dprintf_cont(" %d:removed", mdi->disk.raid_disk);
 				close(mdi->state_fd);
 				close(mdi->recovery_fd);
+				close(mdi->bb_fd);
+				close(mdi->ubb_fd);
 				mdi->state_fd = -1;
 			} else
 				ret |= ARRAY_BUSY;
@@ -583,8 +585,11 @@ static int wait_and_act(struct supertype *container, int nowait)
 		add_fd(&rfds, &maxfd, a->info.state_fd);
 		add_fd(&rfds, &maxfd, a->action_fd);
 		add_fd(&rfds, &maxfd, a->sync_completed_fd);
-		for (mdi = a->info.devs ; mdi ; mdi = mdi->next)
+		for (mdi = a->info.devs ; mdi ; mdi = mdi->next) {
 			add_fd(&rfds, &maxfd, mdi->state_fd);
+			add_fd(&rfds, &maxfd, mdi->bb_fd);
+			add_fd(&rfds, &maxfd, mdi->ubb_fd);
+		}
 
 		ap = &(*ap)->next;
 	}
-- 
1.8.3.1

[PATCH 3/4 v2] mdmon: bad block support for external metadata - store bad blocks

[Tomasz Majchrzak]

If md has changed the state to 'blocked' and metadata handler supports
bad blocks, try process them first. If metadata handler has successfully
stored bad block, acknowledge it to md via 'badblocks' sysfs file. If
metadata handler has failed to store the new bad block (ie. lack of
space), remove bad block support for a disk by writing "-external_bbl"
to state sysfs file. If all bad blocks have been acknowledged, request
to unblock the array.

Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
Acked-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
---
 mdadm.h   |   4 +++
 monitor.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 116 insertions(+)

diff --git a/mdadm.h b/mdadm.h
index 1a1c7af..05a2e3e 100755
--- a/mdadm.h
+++ b/mdadm.h
@@ -1050,6 +1050,10 @@ extern struct superswitch {
 	/* validate container after assemble */
 	int (*validate_container)(struct mdinfo *info);
 
+	/* records new bad block in metadata */
+	int (*record_bad_block)(struct active_array *a, int n,
+					unsigned long long sector, int length);
+
 	int swapuuid; /* true if uuid is bigending rather than hostendian */
 	int external;
 	const char *name; /* canonical metadata name */
diff --git a/monitor.c b/monitor.c
index 56c6500..9de9c8d 100644
--- a/monitor.c
+++ b/monitor.c
@@ -31,6 +31,10 @@ static char *sync_actions[] = {
 	"idle", "reshape", "resync", "recover", "check", "repair", NULL
 };
 
+enum bb_action {
+	RECORD_BB = 1,
+};
+
 static int write_attr(char *attr, int fd)
 {
 	return write(fd, attr, strlen(attr));
@@ -158,6 +162,104 @@ int read_dev_state(int fd)
 	return rv;
 }
 
+int process_ubb(struct active_array *a, struct mdinfo *mdi, const unsigned long
+		long sector, const int length, const char *buf,
+		const int buf_len)
+{
+	struct superswitch *ss = a->container->ss;
+
+	/*
+	 * record bad block in metadata first, then acknowledge it to the driver
+	 * via sysfs file
+	 */
+	if ((ss->record_bad_block(a, mdi->disk.raid_disk, sector, length)) &&
+	    (write(mdi->bb_fd, buf, buf_len) == buf_len))
+		return 1;
+
+	/*
+	 * failed to store or acknowledge bad block, switch of bad block support
+	 * to get it out of blocked state
+	 */
+	sysfs_set_str(&a->info, mdi, "state", "-external_bbl");
+	return -1;
+}
+
+static int read_bb_file(int fd, struct active_array *a, struct mdinfo *mdi,
+			enum bb_action action, void *arg)
+{
+	char buf[30];
+	int n = 0;
+	int ret = 0;
+	int read_again = 0;
+	int off = 0;
+	int pos = 0;
+	int preserve_pos = (action == RECORD_BB ? 0 : 1);
+
+	if (lseek(fd, 0, SEEK_SET) == (off_t) -1)
+		return -1;
+
+	do {
+		read_again = 0;
+		n = read(fd, buf + pos, sizeof(buf) - 1 - pos);
+		if (n < 0)
+			return -1;
+		n += pos;
+
+		buf[n] = '\0';
+		off = 0;
+
+		while (off < n) {
+			unsigned long long sector;
+			int length;
+			char newline;
+			int consumed;
+			int matched;
+			int rc;
+
+			/* kernel sysfs file format: "sector length\n" */
+			matched = sscanf(buf + off, "%llu %d%c%n", &sector,
+					 &length, &newline, &consumed);
+			if ((matched != 3) && (off > 0)) {
+				/* truncated entry, read again */
+				if (preserve_pos) {
+					pos = sizeof(buf) - off - 1;
+					memmove(buf, buf + off, pos);
+				} else {
+					if (lseek(fd, 0, SEEK_SET) ==
+					    (off_t) -1)
+						return -1;
+				}
+				read_again = 1;
+				break;
+			}
+			if (matched != 3)
+				return -1;
+			if (newline != '\n')
+				return -1;
+			if (length <= 0)
+				return -1;
+
+			if (action == RECORD_BB)
+				rc = process_ubb(a, mdi, sector, length,
+						  buf + off, consumed);
+			else
+				rc = -1;
+
+			if (rc < 0)
+				return rc;
+			ret += rc;
+			off += consumed;
+		}
+	} while (read_again);
+
+	return ret;
+}
+
+static int process_dev_ubb(struct active_array *a, struct mdinfo *mdi)
+{
+	return read_bb_file(mdi->ubb_fd, a, mdi, RECORD_BB, NULL);
+}
+
 static void signal_manager(void)
 {
 	/* tgkill(getpid(), mon_tid, SIGUSR1); */
@@ -256,6 +358,16 @@ static int read_and_act(struct active_array *a)
 					  &mdi->recovery_start);
 			mdi->curr_state = read_dev_state(mdi->state_fd);
 		}
+		/*
+		 * If array is blocked and metadata handler is able to handle
+		 * BB, check if you can acknowledge them to md driver. If
+		 * successful, clear faulty state and unblock the array.
+		 */
+		if ((mdi->curr_state & DS_BLOCKED) &&
+		    a->container->ss->record_bad_block &&
+		    (process_dev_ubb(a, mdi) > 0)) {
+			mdi->next_state |= DS_UNBLOCK;
+		}
 	}
 
 	gettimeofday(&tv, NULL);
-- 
1.8.3.1

[PATCH 4/4 v4] mdmon: bad block support for external metadata - clear bad blocks

[Tomasz Majchrzak]

If an update of acknowledged bad blocks file is notified, read entire
bad block list from sysfs file and compare it against local list of bad
blocks. If any obsolete entries are found, remove them from metadata.

As mdmon cannot perform any memory allocation, new superswitch method
get_bad_blocks is expected to return a list of bad blocks in metadata
without allocating memory. It's up to metadata handler to allocate all
required memory in advance.

Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
Reviewed-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
---
 mdadm.h   |  7 ++++++
 monitor.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
 2 files changed, 86 insertions(+), 2 deletions(-)

diff --git a/mdadm.h b/mdadm.h
index 05a2e3e..7f1a1b8 100755
--- a/mdadm.h
+++ b/mdadm.h
@@ -1054,6 +1054,13 @@ extern struct superswitch {
 	int (*record_bad_block)(struct active_array *a, int n,
 					unsigned long long sector, int length);
 
+	/* clears bad block from metadata */
+	int (*clear_bad_block)(struct active_array *a, int n,
+					unsigned long long sector, int length);
+
+	/* get list of bad blocks from metadata */
+	struct md_bb *(*get_bad_blocks)(struct active_array *a, int n);
+
 	int swapuuid; /* true if uuid is bigending rather than hostendian */
 	int external;
 	const char *name; /* canonical metadata name */
diff --git a/monitor.c b/monitor.c
index 9de9c8d..3d60fef 100644
--- a/monitor.c
+++ b/monitor.c
@@ -33,6 +33,7 @@ static char *sync_actions[] = {
 
 enum bb_action {
 	RECORD_BB = 1,
+	COMPARE_BB,
 };
 
 static int write_attr(char *attr, int fd)
@@ -184,6 +185,49 @@ int process_ubb(struct active_array *a, struct mdinfo *mdi, const unsigned long
 	return -1;
 }
 
+int compare_bb(struct active_array *a, struct mdinfo *mdi, const unsigned long
+	       long sector, const unsigned int length, void *arg)
+{
+	struct superswitch *ss = a->container->ss;
+	struct md_bb *bb = (struct md_bb *) arg;
+	int record = 1;
+	int i;
+
+	for (i = 0; i < bb->count; i++) {
+		unsigned long long start = bb->entries[i].sector;
+		unsigned long long len = bb->entries[i].length;
+
+		/*
+		 * bad block in metadata exactly matches bad block in kernel
+		 * list, just remove it from a list
+		 */
+		if ((start == sector) && (len == length)) {
+			if (i < bb->count - 1)
+				bb->entries[i] = bb->entries[bb->count - 1];
+			bb->count -= 1;
+			record = 0;
+			break;
+		}
+		/*
+		 * bad block in metadata spans bad block in kernel list,
+		 * clear it and record new bad block
+		 */
+		if ((sector >= start) && (sector + length <= start + len)) {
+			ss->clear_bad_block(a, mdi->disk.raid_disk, start, len);
+			break;
+		}
+	}
+
+	/* record all bad blocks not in metadata list */
+	if (record && (ss->record_bad_block(a, mdi->disk.raid_disk, sector,
+					     length) <= 0)) {
+		sysfs_set_str(&a->info, mdi, "state", "-external_bbl");
+		return -1;
+	}
+
+	return 1;
+}
+
 static int read_bb_file(int fd, struct active_array *a, struct mdinfo *mdi,
 			enum bb_action action, void *arg)
 {
@@ -242,6 +286,8 @@ static int read_bb_file(int fd, struct active_array *a, struct mdinfo *mdi,
 			if (action == RECORD_BB)
 				rc = process_ubb(a, mdi, sector, length,
 						  buf + off, consumed);
+			else if (action == COMPARE_BB)
+				rc = compare_bb(a, mdi, sector, length, arg);
 			else
 				rc = -1;
 
@@ -260,6 +306,34 @@ static int process_dev_ubb(struct active_array *a, struct mdinfo *mdi)
 	return read_bb_file(mdi->ubb_fd, a, mdi, RECORD_BB, NULL);
 }
 
+static int check_for_cleared_bb(struct active_array *a, struct mdinfo *mdi)
+{
+	struct superswitch *ss = a->container->ss;
+	struct md_bb *bb;
+	int i;
+
+	/*
+	 * Get a list of bad blocks for an array, then read list of
+	 * acknowledged bad blocks from kernel and compare it against metadata
+	 * list, clear all bad blocks remaining in metadata list
+	 */
+	bb = ss->get_bad_blocks(a, mdi->disk.raid_disk);
+	if (!bb)
+		return -1;
+
+	if (read_bb_file(mdi->bb_fd, a, mdi, COMPARE_BB, bb) < 0)
+		return -1;
+
+	for (i = 0; i < bb->count; i++) {
+		unsigned long long sector = bb->entries[i].sector;
+		int length = bb->entries[i].length;
+
+		ss->clear_bad_block(a, mdi->disk.raid_disk, sector, length);
+	}
+
+	return 0;
+}
+
 static void signal_manager(void)
 {
 	/* tgkill(getpid(), mon_tid, SIGUSR1); */
@@ -326,7 +400,7 @@ static void signal_manager(void)
 
 #define ARRAY_DIRTY 1
 #define ARRAY_BUSY 2
-static int read_and_act(struct active_array *a)
+static int read_and_act(struct active_array *a, fd_set *fds)
 {
 	unsigned long long sync_completed;
 	int check_degraded = 0;
@@ -368,6 +442,8 @@ static int read_and_act(struct active_array *a)
 		    (process_dev_ubb(a, mdi) > 0)) {
 			mdi->next_state |= DS_UNBLOCK;
 		}
+		if (FD_ISSET(mdi->bb_fd, fds))
+			check_for_cleared_bb(a, mdi);
 	}
 
 	gettimeofday(&tv, NULL);
@@ -754,6 +830,7 @@ static int wait_and_act(struct supertype *container, int nowait)
 		if (rv == -1) {
 			if (errno == EINTR) {
 				rv = 0;
+				FD_ZERO(&rfds);
 				dprintf("monitor: caught signal\n");
 			} else
 				dprintf("monitor: error %d in pselect\n",
@@ -795,7 +872,7 @@ static int wait_and_act(struct supertype *container, int nowait)
 			signal_manager();
 		}
 		if (a->container && !a->to_remove) {
-			int ret = read_and_act(a);
+			int ret = read_and_act(a, &rfds);
 			rv |= 1;
 			dirty_arrays += !!(ret & ARRAY_DIRTY);
 			/* when terminating stop manipulating the array after it
-- 
1.8.3.1

Re: [PATCH 1/4 v2] mdadm: bad block support for external metadata - initialization

[Tomasz Majchrzak]

On Thu, Oct 27, 2016 at 10:53:42AM +0200, Tomasz Majchrzak wrote:
> If metadata handler provides support for bad blocks, tell md by writing
> 'external_bbl' to rdev state file (both on create and assemble),
> followed by a list of known bad blocks written via sysfs 'bad_blocks'
> file.
> 
> Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
> Reviewed-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
> ---
>  mdadm.h | 13 +++++++++++++
>  sysfs.c | 29 ++++++++++++++++++++++++++++-
>  2 files changed, 41 insertions(+), 1 deletion(-)

Hi Jes,

Do you have any comments for this patch set? It is a generic code for bad
block support for external metadata. It comes ahead of my other patch set
which provides IMSM implementation of new functionality.

Regards,

Tomek

Re: [PATCH 1/4 v2] mdadm: bad block support for external metadata - initialization

[Jes Sorensen]

Tomasz Majchrzak <tomasz.majchrzak@intel.com> writes:
> On Thu, Oct 27, 2016 at 10:53:42AM +0200, Tomasz Majchrzak wrote:
>> If metadata handler provides support for bad blocks, tell md by writing
>> 'external_bbl' to rdev state file (both on create and assemble),
>> followed by a list of known bad blocks written via sysfs 'bad_blocks'
>> file.
>> 
>> Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
>> Reviewed-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
>> ---
>>  mdadm.h | 13 +++++++++++++
>>  sysfs.c | 29 ++++++++++++++++++++++++++++-
>>  2 files changed, 41 insertions(+), 1 deletion(-)
>
> Hi Jes,
>
> Do you have any comments for this patch set? It is a generic code for bad
> block support for external metadata. It comes ahead of my other patch set
> which provides IMSM implementation of new functionality.
>
> Regards,
>
> Tomek

Hi Tomek,

I thought I had responded to this one - I'll have a look early next week
(holiday here).

Cheers,
jes

Re: [PATCH 1/4 v2] mdadm: bad block support for external metadata - initialization

[Jes Sorensen]

Tomasz Majchrzak <tomasz.majchrzak@intel.com> writes:
> If metadata handler provides support for bad blocks, tell md by writing
> 'external_bbl' to rdev state file (both on create and assemble),
> followed by a list of known bad blocks written via sysfs 'bad_blocks'
> file.
>
> Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
> Reviewed-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
> ---
>  mdadm.h | 13 +++++++++++++
>  sysfs.c | 29 ++++++++++++++++++++++++++++-
>  2 files changed, 41 insertions(+), 1 deletion(-)

Hi Tomasz,

Most of this one looks fine, just one minor nit.

> diff --git a/mdadm.h b/mdadm.h
> index 0516c82..5156ea4 100755
> --- a/mdadm.h
> +++ b/mdadm.h
> @@ -237,6 +237,17 @@ struct dlm_lksb {
>  
>  extern const char Name[];
>  
> +struct md_bb_entry {
> +	unsigned long long sector;
> +	int length;
> +};
> +
> +struct md_bb {
> +	int supported;
> +	int count;
> +	struct md_bb_entry *entries;
> +};
> +
>  /* general information that might be extracted from a superblock */
>  struct mdinfo {
>  	mdu_array_info_t	array;
> @@ -311,6 +322,8 @@ struct mdinfo {
>  
>  	/* info read from sysfs */
>  	char		sysfs_array_state[20];
> +
> +	struct md_bb bb;
>  };
>  
>  struct createinfo {
> diff --git a/sysfs.c b/sysfs.c
> index d28e21a..c7a8e66 100644
> --- a/sysfs.c
> +++ b/sysfs.c
> @@ -50,8 +50,12 @@ void sysfs_free(struct mdinfo *sra)
>  		while (sra->devs) {
>  			struct mdinfo *d = sra->devs;
>  			sra->devs = d->next;
> +			if (d->bb.entries)
> +				free(d->bb.entries);
>  			free(d);
>  		}
> +		if (sra->bb.entries)
> +			free(sra->bb.entries);
>  		free(sra);
>  		sra = sra2;
>  	}

free() handles NULL pointers, so no need to check first.

Cheers,
Jes

Re: [PATCH 1/4 v2] mdadm: bad block support for external metadata - initialization

[Jes Sorensen]

Jes Sorensen <Jes.Sorensen@redhat.com> writes:
> Tomasz Majchrzak <tomasz.majchrzak@intel.com> writes:
>> On Thu, Oct 27, 2016 at 10:53:42AM +0200, Tomasz Majchrzak wrote:
>>> If metadata handler provides support for bad blocks, tell md by writing
>>> 'external_bbl' to rdev state file (both on create and assemble),
>>> followed by a list of known bad blocks written via sysfs 'bad_blocks'
>>> file.
>>> 
>>> Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
>>> Reviewed-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
>>> ---
>>>  mdadm.h | 13 +++++++++++++
>>>  sysfs.c | 29 ++++++++++++++++++++++++++++-
>>>  2 files changed, 41 insertions(+), 1 deletion(-)
>>
>> Hi Jes,
>>
>> Do you have any comments for this patch set? It is a generic code for bad
>> block support for external metadata. It comes ahead of my other patch set
>> which provides IMSM implementation of new functionality.
>>
>> Regards,
>>
>> Tomek
>
> Hi Tomek,
>
> I thought I had responded to this one - I'll have a look early next week
> (holiday here).

Tomek,

It mostly look fine, there is a nitpick in patch 1/4. In general I
discourage the use of unnecessary parenthesis in multi case if
statements, but it's not a showstopper. Ie.
 if ((foo == x) && (bar != y))

If you send me an updated 1/4 then I should be able to apply it.

and of course, always use a cover letter when sending out multi-patch
sets in the future.

Cheers,
Jes

[PATCH 1/4] mdadm: bad block support for external metadata - initialization

[Tomasz Majchrzak]

If metadata handler provides support for bad blocks, tell md by writing
'external_bbl' to rdev state file (both on create and assemble),
followed by a list of known bad blocks written via sysfs 'bad_blocks'
file.

Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
Reviewed-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
---
 mdadm.h | 13 +++++++++++++
 sysfs.c | 27 ++++++++++++++++++++++++++-
 2 files changed, 39 insertions(+), 1 deletion(-)

diff --git a/mdadm.h b/mdadm.h
index 240ab7f..4aa1d30 100755
--- a/mdadm.h
+++ b/mdadm.h
@@ -237,6 +237,17 @@ struct dlm_lksb {
 
 extern const char Name[];
 
+struct md_bb_entry {
+	unsigned long long sector;
+	int length;
+};
+
+struct md_bb {
+	int supported;
+	int count;
+	struct md_bb_entry *entries;
+};
+
 /* general information that might be extracted from a superblock */
 struct mdinfo {
 	mdu_array_info_t	array;
@@ -311,6 +322,8 @@ struct mdinfo {
 
 	/* info read from sysfs */
 	char		sysfs_array_state[20];
+
+	struct md_bb bb;
 };
 
 struct createinfo {
diff --git a/sysfs.c b/sysfs.c
index f8a9f0b..84c7348 100644
--- a/sysfs.c
+++ b/sysfs.c
@@ -52,8 +52,10 @@ void sysfs_free(struct mdinfo *sra)
 		while (sra->devs) {
 			struct mdinfo *d = sra->devs;
 			sra->devs = d->next;
+			free(d->bb.entries);
 			free(d);
 		}
+		free(sra->bb.entries);
 		free(sra);
 		sra = sra2;
 	}
@@ -261,7 +263,7 @@ struct mdinfo *sysfs_read(int fd, char *devnm, unsigned long options)
 		dbase = base + strlen(base);
 		*dbase++ = '/';
 
-		dev = xmalloc(sizeof(*dev));
+		dev = xcalloc(1, sizeof(*dev));
 
 		/* Always get slot, major, minor */
 		strcpy(dbase, "slot");
@@ -691,6 +693,7 @@ int sysfs_add_disk(struct mdinfo *sra, struct mdinfo *sd, int resume)
 	char nm[PATH_MAX];
 	char *dname;
 	int rv;
+	int i;
 
 	sprintf(dv, "%d:%d", sd->disk.major, sd->disk.minor);
 	rv = sysfs_set_str(sra, NULL, "new_dev", dv);
@@ -722,6 +725,28 @@ int sysfs_add_disk(struct mdinfo *sra, struct mdinfo *sd, int resume)
 		if (resume)
 			sysfs_set_num(sra, sd, "recovery_start", sd->recovery_start);
 	}
+	if (sd->bb.supported) {
+		if (sysfs_set_str(sra, sd, "state", "external_bbl")) {
+			/*
+			 * backward compatibility - if kernel doesn't support
+			 * bad blocks for external metadata, let it continue
+			 * as long as there are none known so far
+			 */
+			if (sd->bb.count) {
+				pr_err("The kernel has no support for bad blocks in external metadata\n");
+				return -1;
+			}
+		}
+
+		for (i = 0; i < sd->bb.count; i++) {
+			char s[30];
+			const struct md_bb_entry *entry = &sd->bb.entries[i];
+
+			snprintf(s, sizeof(s) - 1, "%llu %d\n", entry->sector,
+				 entry->length);
+			rv |= sysfs_set_str(sra, sd, "bad_blocks", s);
+		}
+	}
 	return rv;
 }
 
-- 
1.8.3.1

Re: [PATCH 1/4] mdadm: bad block support for external metadata - initialization

[Jes Sorensen]

Tomasz Majchrzak <tomasz.majchrzak@intel.com> writes:
> If metadata handler provides support for bad blocks, tell md by writing
> 'external_bbl' to rdev state file (both on create and assemble),
> followed by a list of known bad blocks written via sysfs 'bad_blocks'
> file.
>
> Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
> Reviewed-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
> ---
>  mdadm.h | 13 +++++++++++++
>  sysfs.c | 27 ++++++++++++++++++++++++++-
>  2 files changed, 39 insertions(+), 1 deletion(-)

Applied!

Thanks,
Jes

Re: [PATCH 2/4 v2] mdmon: bad block support for external metadata - sysfs file open

[Jes Sorensen]

Tomasz Majchrzak <tomasz.majchrzak@intel.com> writes:
> Open 'badblocks' and 'unacknowledged_bad_blocks' sysfs files for each
> disk in the array. Add them to the list of files observed by monitor.
>
> Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
> Reviewed-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
> ---
>  managemon.c | 17 +++++++++++++++++
>  mdadm.h     |  2 ++
>  monitor.c   |  7 ++++++-
>  3 files changed, 25 insertions(+), 1 deletion(-)

Applied!

Thanks,
Jes

Re: [PATCH 3/4 v2] mdmon: bad block support for external metadata - store bad blocks

[Jes Sorensen]

Tomasz Majchrzak <tomasz.majchrzak@intel.com> writes:
> If md has changed the state to 'blocked' and metadata handler supports
> bad blocks, try process them first. If metadata handler has successfully
> stored bad block, acknowledge it to md via 'badblocks' sysfs file. If
> metadata handler has failed to store the new bad block (ie. lack of
> space), remove bad block support for a disk by writing "-external_bbl"
> to state sysfs file. If all bad blocks have been acknowledged, request
> to unblock the array.
>
> Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
> Acked-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
> ---
>  mdadm.h   |   4 +++
>  monitor.c | 112 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 116 insertions(+)

Applied!

Thanks,
Jes

Re: [PATCH 4/4 v4] mdmon: bad block support for external metadata - clear bad blocks

[Jes Sorensen]

Tomasz Majchrzak <tomasz.majchrzak@intel.com> writes:
> If an update of acknowledged bad blocks file is notified, read entire
> bad block list from sysfs file and compare it against local list of bad
> blocks. If any obsolete entries are found, remove them from metadata.
>
> As mdmon cannot perform any memory allocation, new superswitch method
> get_bad_blocks is expected to return a list of bad blocks in metadata
> without allocating memory. It's up to metadata handler to allocate all
> required memory in advance.
>
> Signed-off-by: Tomasz Majchrzak <tomasz.majchrzak@intel.com>
> Reviewed-by: Artur Paszkiewicz <artur.paszkiewicz@intel.com>
> ---
>  mdadm.h   |  7 ++++++
>  monitor.c | 81 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 86 insertions(+), 2 deletions(-)

Applied!

Thanks,
Jes

Re: [PATCH 4/4 v4] mdmon: bad block support for external metadata - clear bad blocks

[NeilBrown]

On Thu, 27 Oct 2016, Tomasz Majchrzak wrote:
> If an update of acknowledged bad blocks file is notified, read entire
> bad block list from sysfs file and compare it against local list of bad
> blocks. If any obsolete entries are found, remove them from metadata.
> 
> As mdmon cannot perform any memory allocation, new superswitch method
> get_bad_blocks is expected to return a list of bad blocks in metadata
> without allocating memory. It's up to metadata handler to allocate all
> required memory in advance.

hi,
 only 5 years late to this party :-)

 I recently had cause the look at this code and ... there are problems.

 Primarily, it assumes that the "bad_blocks" file contains a complete
 list of bad blocks known to the kernel.  This is not correct.
 As the documentation and nearby comments say, the contents of this file
 is truncated to PAGE_SIZE.  It is not meant to be a complete list, only
 an indicative list.
 There is no way to get a complete list from the kernel once the list
 gets too large.  Probably we should design and implement a reliable way
 to extract this information.  I imagine it would be something like
 unacknowledged_bad_blocks, in that mdmon could read some information,
 then confirm that it has been read, then read some more.  But until
 that it done, this code should be careful not to assume that the list
 is complete - at least not without checking.

 Secondly, the interface with the metadata handler is a bit odd.
 The 'check_for_cleared_bb' essentially does:
   - call ->record_bad_block  for all blocks known to the kernel
   - call ->clear_bad_block   for all blocks that were in the metadata
  in that order.
  It isn't quite that simple as there are optimisations:
    if a range from kernel exactly matches a range in metadata, the
      range is neither recorded or cleared
    If a range from the kernel is a subrange of a range in metadata,
      then the larger range is cleared before the new range is added,
      AS WELL AS after.

  If there are other overlaps, then the kernel range is recorded
  before the metadata range is cleared.  This *seems* wrong.  I would
  expect this to clear part of the range that had just been added.

  However, it doesn't.  The ->clear_bad_block interface *DOESN'T* remove
  all the block in the range from the bbl.  Rather, if the exact range
  given appears as one of the ranges in the bbl, then that range is
  deleted.  Otherwise no change happens.
  These semantics are surprising.  The net result is that the code
  probably works with the imsm backend.  However if someone else wrote a
  different backend which implemented ->clear_bad_block to actually
  remove the entire range from the bbl, then it would clear more blocks
  than it should.

  I think it would be really good to re-implement this code in a way
  that was more maintainable.
  I don't think "check_for_cleared_bb()" should *ever* record new bad
  block ranges.  They get recorded through the unacknowledged_bad_block
  processing.  "check_for_cleared_bb()" should ONLY delete blocks from the
  bbl, and it should ONLY do that if it certain that the information in
  "bad_blocks" is complete.

  It should read bad_blocks in a single read().  If the returned data
  ends with a newline, and is not a power-of-2 in size, then it is
  safe to assume that it is complete.
  If it doesn't end with a newline, then it is definitely not complete.
  If it is a power-of-2 less than 4096, then it can be assumed to be
  complete.  If it is exactly 4096 bytes, or a larger power of two, then
  it is not safe to assume that it is complete.

Thanks,
NeilBrown

Re: [PATCH 4/4 v4] mdmon: bad block support for external metadata - clear bad blocks

[Tkaczyk, Mariusz]

Hi Neil,
Thanks for your analysis and suggestions.
I will try to address them.

We are testing current implementation and it is working (at least for
tested scenarios).

Thanks,
Mariusz

On 29.09.2021 01:56, NeilBrown wrote:
> On Thu, 27 Oct 2016, Tomasz Majchrzak wrote:
>> If an update of acknowledged bad blocks file is notified, read entire
>> bad block list from sysfs file and compare it against local list of bad
>> blocks. If any obsolete entries are found, remove them from metadata.
>>
>> As mdmon cannot perform any memory allocation, new superswitch method
>> get_bad_blocks is expected to return a list of bad blocks in metadata
>> without allocating memory. It's up to metadata handler to allocate all
>> required memory in advance.
> 
> hi,
>   only 5 years late to this party :-)
> 
>   I recently had cause the look at this code and ... there are problems.
> 
>   Primarily, it assumes that the "bad_blocks" file contains a complete
>   list of bad blocks known to the kernel.  This is not correct.
>   As the documentation and nearby comments say, the contents of this file
>   is truncated to PAGE_SIZE.  It is not meant to be a complete list, only
>   an indicative list.
>   There is no way to get a complete list from the kernel once the list
>   gets too large.  Probably we should design and implement a reliable way
>   to extract this information.  I imagine it would be something like
>   unacknowledged_bad_blocks, in that mdmon could read some information,
>   then confirm that it has been read, then read some more.  But until
>   that it done, this code should be careful not to assume that the list
>   is complete - at least not without checking.
> 
>   Secondly, the interface with the metadata handler is a bit odd.
>   The 'check_for_cleared_bb' essentially does:
>     - call ->record_bad_block  for all blocks known to the kernel
>     - call ->clear_bad_block   for all blocks that were in the metadata
>    in that order.
>    It isn't quite that simple as there are optimisations:
>      if a range from kernel exactly matches a range in metadata, the
>        range is neither recorded or cleared
>      If a range from the kernel is a subrange of a range in metadata,
>        then the larger range is cleared before the new range is added,
>        AS WELL AS after.
> 
>    If there are other overlaps, then the kernel range is recorded
>    before the metadata range is cleared.  This *seems* wrong.  I would
>    expect this to clear part of the range that had just been added.
> 
>    However, it doesn't.  The ->clear_bad_block interface *DOESN'T* remove
>    all the block in the range from the bbl.  Rather, if the exact range
>    given appears as one of the ranges in the bbl, then that range is
>    deleted.  Otherwise no change happens.
>    These semantics are surprising.  The net result is that the code
>    probably works with the imsm backend.  However if someone else wrote a
>    different backend which implemented ->clear_bad_block to actually
>    remove the entire range from the bbl, then it would clear more blocks
>    than it should.
> 
>    I think it would be really good to re-implement this code in a way
>    that was more maintainable.
>    I don't think "check_for_cleared_bb()" should *ever* record new bad
>    block ranges.  They get recorded through the unacknowledged_bad_block
>    processing.  "check_for_cleared_bb()" should ONLY delete blocks from the
>    bbl, and it should ONLY do that if it certain that the information in
>    "bad_blocks" is complete.
> 
>    It should read bad_blocks in a single read().  If the returned data
>    ends with a newline, and is not a power-of-2 in size, then it is
>    safe to assume that it is complete.
>    If it doesn't end with a newline, then it is definitely not complete.
>    If it is a power-of-2 less than 4096, then it can be assumed to be
>    complete.  If it is exactly 4096 bytes, or a larger power of two, then
>    it is not safe to assume that it is complete.
> 
> Thanks,
> NeilBrown
>