bcachefs: Improved superblock-related error messages

This patch converts bch2_sb_validate() and the .validate methods for the
various superblock sections to take printbuf, to which they can print
detailed error messages, including printing the entire section that was
invalid.

This is a great improvement over the previous situation, where we could
only return static strings that didn't have precise information about
what was wrong.
Signed-off-by: Kent Overstreet <kent.overstreet@gmail.com>

fs/bcachefs/disk_groups.c

@@ -17,24 +17,20 @@ static int group_cmp(const void *_l, const void *_r)
 		strncmp(l->label, r->label, sizeof(l->label));
 }
 
-static const char *bch2_sb_disk_groups_validate(struct bch_sb *sb,
-						struct bch_sb_field *f)
+static int bch2_sb_disk_groups_validate(struct bch_sb *sb,
+					struct bch_sb_field *f,
+					struct printbuf *err)
 {
 	struct bch_sb_field_disk_groups *groups =
 		field_to_type(f, disk_groups);
 	struct bch_disk_group *g, *sorted = NULL;
-	struct bch_sb_field_members *mi;
-	struct bch_member *m;
-	unsigned i, nr_groups, len;
-	const char *err = NULL;
-
-	mi		= bch2_sb_get_members(sb);
-	groups		= bch2_sb_get_disk_groups(sb);
-	nr_groups	= disk_groups_nr(groups);
+	struct bch_sb_field_members *mi = bch2_sb_get_members(sb);
+	unsigned nr_groups = disk_groups_nr(groups);
+	unsigned i, len;
+	int ret = -EINVAL;
 
-	for (m = mi->members;
-	     m < mi->members + sb->nr_devices;
-	     m++) {
+	for (i = 0; i < sb->nr_devices; i++) {
+		struct bch_member *m = mi->members + i;
 		unsigned g;
 
 		if (!BCH_MEMBER_GROUP(m))
@@ -42,45 +38,53 @@ static const char *bch2_sb_disk_groups_validate(struct bch_sb *sb,
 
 		g = BCH_MEMBER_GROUP(m) - 1;
 
-		if (g >= nr_groups ||
-		    BCH_GROUP_DELETED(&groups->entries[g]))
-			return "disk has invalid group";
+		if (g >= nr_groups) {
+			pr_buf(err, "disk %u has invalid label %u (have %u)",
+			       i, g, nr_groups);
+			return -EINVAL;
+		}
+
+		if (BCH_GROUP_DELETED(&groups->entries[g])) {
+			pr_buf(err, "disk %u has deleted label %u", i, g);
+			return -EINVAL;
+		}
 	}
 
 	if (!nr_groups)
-		return NULL;
+		return 0;
+
+	for (i = 0; i < nr_groups; i++) {
+		g = groups->entries + i;
 
-	for (g = groups->entries;
-	     g < groups->entries + nr_groups;
-	     g++) {
 		if (BCH_GROUP_DELETED(g))
 			continue;
 
 		len = strnlen(g->label, sizeof(g->label));
 		if (!len) {
-			err = "group with empty label";
-			goto err;
+			pr_buf(err, "label %u empty", i);
+			return -EINVAL;
 		}
 	}
 
 	sorted = kmalloc_array(nr_groups, sizeof(*sorted), GFP_KERNEL);
 	if (!sorted)
-		return "cannot allocate memory";
+		return -ENOMEM;
 
 	memcpy(sorted, groups->entries, nr_groups * sizeof(*sorted));
 	sort(sorted, nr_groups, sizeof(*sorted), group_cmp, NULL);
 
-	for (i = 0; i + 1 < nr_groups; i++)
-		if (!BCH_GROUP_DELETED(sorted + i) &&
-		    !group_cmp(sorted + i, sorted + i + 1)) {
-			err = "duplicate groups";
+	for (g = sorted; g + 1 < sorted + nr_groups; g++)
+		if (!BCH_GROUP_DELETED(g) &&
+		    !group_cmp(&g[0], &g[1])) {
+			pr_buf(err, "duplicate label %llu.", BCH_GROUP_PARENT(g));
+			bch_scnmemcpy(err, g->label, strnlen(g->label, sizeof(g->label)));
 			goto err;
 		}
 
-	err = NULL;
+	ret = 0;
 err:
 	kfree(sorted);
-	return err;
+	return 0;
 }
 
 static void bch2_sb_disk_groups_to_text(struct printbuf *out,

fs/bcachefs/journal_seq_blacklist.c

@@ -189,27 +189,34 @@ int bch2_blacklist_table_initialize(struct bch_fs *c)
 	return 0;
 }
 
-static const char *
-bch2_sb_journal_seq_blacklist_validate(struct bch_sb *sb,
-				       struct bch_sb_field *f)
+static int bch2_sb_journal_seq_blacklist_validate(struct bch_sb *sb,
+						  struct bch_sb_field *f,
+						  struct printbuf *err)
 {
 	struct bch_sb_field_journal_seq_blacklist *bl =
 		field_to_type(f, journal_seq_blacklist);
-	struct journal_seq_blacklist_entry *i;
-	unsigned nr = blacklist_nr_entries(bl);
+	unsigned i, nr = blacklist_nr_entries(bl);
 
-	for (i = bl->start; i < bl->start + nr; i++) {
-		if (le64_to_cpu(i->start) >=
-		    le64_to_cpu(i->end))
-			return "entry start >= end";
-
-		if (i + 1 < bl->start + nr &&
-		    le64_to_cpu(i[0].end) >
-		    le64_to_cpu(i[1].start))
-			return "entries out of order";
+	for (i = 0; i < nr; i++) {
+		struct journal_seq_blacklist_entry *e = bl->start + i;
+
+		if (le64_to_cpu(e->start) >=
+		    le64_to_cpu(e->end)) {
+			pr_buf(err, "entry %u start >= end (%llu >= %llu)",
+			       i, le64_to_cpu(e->start), le64_to_cpu(e->end));
+			return -EINVAL;
+		}
+
+		if (i + 1 < nr &&
+		    le64_to_cpu(e[0].end) >
+		    le64_to_cpu(e[1].start)) {
+			pr_buf(err, "entry %u out of order with next entry (%llu > %llu)",
+			       i + 1, le64_to_cpu(e[0].end), le64_to_cpu(e[1].start));
+			return -EINVAL;
+		}
 	}
 
-	return NULL;
+	return 0;
 }
 
 static void bch2_sb_journal_seq_blacklist_to_text(struct printbuf *out,

fs/bcachefs/quota.c

@@ -6,15 +6,17 @@
 #include "subvolume.h"
 #include "super-io.h"
 
-static const char *bch2_sb_validate_quota(struct bch_sb *sb,
-					  struct bch_sb_field *f)
+static int bch2_sb_validate_quota(struct bch_sb *sb, struct bch_sb_field *f,
+				  struct printbuf *err)
 {
 	struct bch_sb_field_quota *q = field_to_type(f, quota);
 
-	if (vstruct_bytes(&q->field) != sizeof(*q))
-		return "invalid field quota: wrong size";
+	if (vstruct_bytes(&q->field) < sizeof(*q)) {
+		pr_buf(err, "wrong size (got %llu should be %zu)",
+		       vstruct_bytes(&q->field), sizeof(*q));
+	}
 
-	return NULL;
+	return 0;
 }
 
 const struct bch_sb_field_ops bch_sb_field_ops_quota = {

fs/bcachefs/replicas.c

@@ -41,18 +41,19 @@ void bch2_replicas_entry_to_text(struct printbuf *out,
 {
 	unsigned i;
 
-	pr_buf(out, "%s: %u/%u [",
-	       bch2_data_types[e->data_type],
-	       e->nr_required,
-	       e->nr_devs);
+	if (e->data_type < BCH_DATA_NR)
+		pr_buf(out, "%s", bch2_data_types[e->data_type]);
+	else
+		pr_buf(out, "(invalid data type %u)", e->data_type);
 
+	pr_buf(out, ": %u/%u [", e->nr_required, e->nr_devs);
 	for (i = 0; i < e->nr_devs; i++)
 		pr_buf(out, i ? " %u" : "%u", e->devs[i]);
 	pr_buf(out, "]");
 }
 
 void bch2_cpu_replicas_to_text(struct printbuf *out,
-			      struct bch_replicas_cpu *r)
+			       struct bch_replicas_cpu *r)
 {
 	struct bch_replicas_entry *e;
 	bool first = true;
@@ -815,67 +816,78 @@ static int bch2_cpu_replicas_to_sb_replicas(struct bch_fs *c,
 	return 0;
 }
 
-static const char *check_dup_replicas_entries(struct bch_replicas_cpu *cpu_r)
+static int bch2_cpu_replicas_validate(struct bch_replicas_cpu *cpu_r,
+				      struct bch_sb *sb,
+				      struct printbuf *err)
 {
-	unsigned i;
+	struct bch_sb_field_members *mi = bch2_sb_get_members(sb);
+	unsigned i, j;
 
 	sort_cmp_size(cpu_r->entries,
 		      cpu_r->nr,
 		      cpu_r->entry_size,
 		      memcmp, NULL);
 
-	for (i = 0; i + 1 < cpu_r->nr; i++) {
-		struct bch_replicas_entry *l =
+	for (i = 0; i < cpu_r->nr; i++) {
+		struct bch_replicas_entry *e =
 			cpu_replicas_entry(cpu_r, i);
-		struct bch_replicas_entry *r =
-			cpu_replicas_entry(cpu_r, i + 1);
-
-		BUG_ON(memcmp(l, r, cpu_r->entry_size) > 0);
 
-		if (!memcmp(l, r, cpu_r->entry_size))
-			return "duplicate replicas entry";
-	}
+		if (e->data_type >= BCH_DATA_NR) {
+			pr_buf(err, "invalid data type in entry ");
+			bch2_replicas_entry_to_text(err, e);
+			return -EINVAL;
+		}
 
-	return NULL;
-}
+		if (!e->nr_devs) {
+			pr_buf(err, "no devices in entry ");
+			bch2_replicas_entry_to_text(err, e);
+			return -EINVAL;
+		}
 
-static const char *bch2_sb_validate_replicas(struct bch_sb *sb, struct bch_sb_field *f)
-{
-	struct bch_sb_field_replicas *sb_r = field_to_type(f, replicas);
-	struct bch_sb_field_members *mi = bch2_sb_get_members(sb);
-	struct bch_replicas_cpu cpu_r = { .entries = NULL };
-	struct bch_replicas_entry *e;
-	const char *err;
-	unsigned i;
+		if (e->nr_required > 1 &&
+		    e->nr_required >= e->nr_devs) {
+			pr_buf(err, "bad nr_required in entry ");
+			bch2_replicas_entry_to_text(err, e);
+			return -EINVAL;
+		}
 
-	for_each_replicas_entry(sb_r, e) {
-		err = "invalid replicas entry: invalid data type";
-		if (e->data_type >= BCH_DATA_NR)
-			goto err;
+		for (j = 0; j < e->nr_devs; j++)
+			if (!bch2_dev_exists(sb, mi, e->devs[j])) {
+				pr_buf(err, "invalid device %u in entry ", e->devs[j]);
+				bch2_replicas_entry_to_text(err, e);
+				return -EINVAL;
+			}
 
-		err = "invalid replicas entry: no devices";
-		if (!e->nr_devs)
-			goto err;
+		if (i + 1 < cpu_r->nr) {
+			struct bch_replicas_entry *n =
+				cpu_replicas_entry(cpu_r, i + 1);
 
-		err = "invalid replicas entry: bad nr_required";
-		if (e->nr_required > 1 &&
-		    e->nr_required >= e->nr_devs)
-			goto err;
+			BUG_ON(memcmp(e, n, cpu_r->entry_size) > 0);
 
-		err = "invalid replicas entry: invalid device";
-		for (i = 0; i < e->nr_devs; i++)
-			if (!bch2_dev_exists(sb, mi, e->devs[i]))
-				goto err;
+			if (!memcmp(e, n, cpu_r->entry_size)) {
+				pr_buf(err, "duplicate replicas entry ");
+				bch2_replicas_entry_to_text(err, e);
+				return -EINVAL;
+			}
+		}
 	}
 
-	err = "cannot allocate memory";
+	return 0;
+}
+
+static int bch2_sb_validate_replicas(struct bch_sb *sb, struct bch_sb_field *f,
+				     struct printbuf *err)
+{
+	struct bch_sb_field_replicas *sb_r = field_to_type(f, replicas);
+	struct bch_replicas_cpu cpu_r;
+	int ret;
+
 	if (__bch2_sb_replicas_to_cpu_replicas(sb_r, &cpu_r))
-		goto err;
+		return -ENOMEM;
 
-	err = check_dup_replicas_entries(&cpu_r);
-err:
+	ret = bch2_cpu_replicas_validate(&cpu_r, sb, err);
 	kfree(cpu_r.entries);
-	return err;
+	return ret;
 }
 
 static void bch2_sb_replicas_to_text(struct printbuf *out,
@@ -900,38 +912,19 @@ const struct bch_sb_field_ops bch_sb_field_ops_replicas = {
 	.to_text	= bch2_sb_replicas_to_text,
 };
 
-static const char *bch2_sb_validate_replicas_v0(struct bch_sb *sb, struct bch_sb_field *f)
+static int bch2_sb_validate_replicas_v0(struct bch_sb *sb, struct bch_sb_field *f,
+					struct printbuf *err)
 {
 	struct bch_sb_field_replicas_v0 *sb_r = field_to_type(f, replicas_v0);
-	struct bch_sb_field_members *mi = bch2_sb_get_members(sb);
-	struct bch_replicas_cpu cpu_r = { .entries = NULL };
-	struct bch_replicas_entry_v0 *e;
-	const char *err;
-	unsigned i;
+	struct bch_replicas_cpu cpu_r;
+	int ret;
 
-	for_each_replicas_entry_v0(sb_r, e) {
-		err = "invalid replicas entry: invalid data type";
-		if (e->data_type >= BCH_DATA_NR)
-			goto err;
-
-		err = "invalid replicas entry: no devices";
-		if (!e->nr_devs)
-			goto err;
-
-		err = "invalid replicas entry: invalid device";
-		for (i = 0; i < e->nr_devs; i++)
-			if (!bch2_dev_exists(sb, mi, e->devs[i]))
-				goto err;
-	}
-
-	err = "cannot allocate memory";
 	if (__bch2_sb_replicas_v0_to_cpu_replicas(sb_r, &cpu_r))
-		goto err;
+		return -ENOMEM;
 
-	err = check_dup_replicas_entries(&cpu_r);
-err:
+	ret = bch2_cpu_replicas_validate(&cpu_r, sb, err);
 	kfree(cpu_r.entries);
-	return err;
+	return ret;
 }
 
 const struct bch_sb_field_ops bch_sb_field_ops_replicas_v0 = {

fs/bcachefs/super-io.h

@@ -38,9 +38,8 @@ BCH_SB_FIELDS()
 extern const char * const bch2_sb_fields[];
 
 struct bch_sb_field_ops {
-	const char *	(*validate)(struct bch_sb *, struct bch_sb_field *);
-	void		(*to_text)(struct printbuf *, struct bch_sb *,
-				   struct bch_sb_field *);
+	int	(*validate)(struct bch_sb *, struct bch_sb_field *, struct printbuf *);
+	void	(*to_text)(struct printbuf *, struct bch_sb *, struct bch_sb_field *);
 };
 
 static inline __le64 bch2_sb_magic(struct bch_fs *c)
@@ -66,8 +65,6 @@ int bch2_sb_from_fs(struct bch_fs *, struct bch_dev *);
 void bch2_free_super(struct bch_sb_handle *);
 int bch2_sb_realloc(struct bch_sb_handle *, unsigned);
 
-const char *bch2_sb_validate(struct bch_sb_handle *);
-
 int bch2_read_super(const char *, struct bch_opts *, struct bch_sb_handle *);
 int bch2_write_super(struct bch_fs *);
 void __bch2_check_set_feature(struct bch_fs *, unsigned);

fs/bcachefs/super.c

@@ -1604,18 +1604,20 @@ int bch2_dev_add(struct bch_fs *c, const char *path)
 	struct bch_sb_field_members *mi;
 	struct bch_member dev_mi;
 	unsigned dev_idx, nr_devices, u64s;
+	char *_errbuf;
+	struct printbuf errbuf;
 	int ret;
 
+	_errbuf = kmalloc(4096, GFP_KERNEL);
+	if (!_errbuf)
+		return -ENOMEM;
+
+	errbuf = _PBUF(_errbuf, 4096);
+
 	ret = bch2_read_super(path, &opts, &sb);
 	if (ret) {
 		bch_err(c, "device add error: error reading super: %i", ret);
-		return ret;
-	}
-
-	err = bch2_sb_validate(&sb);
-	if (err) {
-		bch_err(c, "device add error: error validating super: %s", err);
-		return -EINVAL;
+		goto err;
 	}
 
 	dev_mi = bch2_sb_get_members(sb.sb)->members[sb.sb->dev_idx];
@@ -1623,19 +1625,21 @@ int bch2_dev_add(struct bch_fs *c, const char *path)
 	err = bch2_dev_may_add(sb.sb, c);
 	if (err) {
 		bch_err(c, "device add error: %s", err);
-		return -EINVAL;
+		ret = -EINVAL;
+		goto err;
 	}
 
 	ca = __bch2_dev_alloc(c, &dev_mi);
 	if (!ca) {
 		bch2_free_super(&sb);
-		return -ENOMEM;
+		ret = -ENOMEM;
+		goto err;
 	}
 
 	ret = __bch2_dev_attach_bdev(ca, &sb);
 	if (ret) {
 		bch2_dev_free(ca);
-		return ret;
+		goto err;
 	}
 
 	ret = bch2_dev_journal_alloc(ca);
@@ -1727,10 +1731,12 @@ int bch2_dev_add(struct bch_fs *c, const char *path)
 	if (ca)
 		bch2_dev_free(ca);
 	bch2_free_super(&sb);
+	kfree(_errbuf);
 	return ret;
 err_late:
 	up_write(&c->state_lock);
-	return -EINVAL;
+	ca = NULL;
+	goto err;
 }
 
 /* Hot add existing device to running filesystem: */
@@ -1896,20 +1902,28 @@ struct bch_fs *bch2_fs_open(char * const *devices, unsigned nr_devices,
 	struct bch_sb_field_members *mi;
 	unsigned i, best_sb = 0;
 	const char *err;
+	char *_errbuf = NULL;
+	struct printbuf errbuf;
 	int ret = 0;
 
+	if (!try_module_get(THIS_MODULE))
+		return ERR_PTR(-ENODEV);
+
 	pr_verbose_init(opts, "");
 
 	if (!nr_devices) {
-		c = ERR_PTR(-EINVAL);
-		goto out2;
+		ret = -EINVAL;
+		goto err;
 	}
 
-	if (!try_module_get(THIS_MODULE)) {
-		c = ERR_PTR(-ENODEV);
-		goto out2;
+	_errbuf = kmalloc(4096, GFP_KERNEL);
+	if (!_errbuf) {
+		ret = -ENOMEM;
+		goto err;
 	}
 
+	errbuf = _PBUF(_errbuf, 4096);
+
 	sb = kcalloc(nr_devices, sizeof(*sb), GFP_KERNEL);
 	if (!sb) {
 		ret = -ENOMEM;
@@ -1921,9 +1935,6 @@ struct bch_fs *bch2_fs_open(char * const *devices, unsigned nr_devices,
 		if (ret)
 			goto err;
 
-		err = bch2_sb_validate(&sb[i]);
-		if (err)
-			goto err_print;
 	}
 
 	for (i = 1; i < nr_devices; i++)
@@ -1976,8 +1987,8 @@ struct bch_fs *bch2_fs_open(char * const *devices, unsigned nr_devices,
 	}
 out:
 	kfree(sb);
+	kfree(_errbuf);
 	module_put(THIS_MODULE);
-out2:
 	pr_verbose_init(opts, "ret %i", PTR_ERR_OR_ZERO(c));
 	return c;
 err_print:
@@ -1994,81 +2005,6 @@ struct bch_fs *bch2_fs_open(char * const *devices, unsigned nr_devices,
 	goto out;
 }
 
-static const char *__bch2_fs_open_incremental(struct bch_sb_handle *sb,
-					      struct bch_opts opts)
-{
-	const char *err;
-	struct bch_fs *c;
-	bool allocated_fs = false;
-	int ret;
-
-	err = bch2_sb_validate(sb);
-	if (err)
-		return err;
-
-	mutex_lock(&bch_fs_list_lock);
-	c = __bch2_uuid_to_fs(sb->sb->uuid);
-	if (c) {
-		closure_get(&c->cl);
-
-		err = bch2_dev_in_fs(c->disk_sb.sb, sb->sb);
-		if (err)
-			goto err;
-	} else {
-		allocated_fs = true;
-		c = bch2_fs_alloc(sb->sb, opts);
-
-		err = "bch2_fs_alloc() error";
-		if (IS_ERR(c))
-			goto err;
-	}
-
-	err = "bch2_dev_online() error";
-
-	mutex_lock(&c->sb_lock);
-	if (bch2_dev_attach_bdev(c, sb)) {
-		mutex_unlock(&c->sb_lock);
-		goto err;
-	}
-	mutex_unlock(&c->sb_lock);
-
-	if (!c->opts.nostart && bch2_fs_may_start(c)) {
-		err = "error starting filesystem";
-		ret = bch2_fs_start(c);
-		if (ret)
-			goto err;
-	}
-
-	closure_put(&c->cl);
-	mutex_unlock(&bch_fs_list_lock);
-
-	return NULL;
-err:
-	mutex_unlock(&bch_fs_list_lock);
-
-	if (allocated_fs && !IS_ERR(c))
-		bch2_fs_stop(c);
-	else if (c)
-		closure_put(&c->cl);
-
-	return err;
-}
-
-const char *bch2_fs_open_incremental(const char *path)
-{
-	struct bch_sb_handle sb;
-	struct bch_opts opts = bch2_opts_empty();
-	const char *err;
-
-	if (bch2_read_super(path, &opts, &sb))
-		return "error reading superblock";
-
-	err = __bch2_fs_open_incremental(&sb, opts);
-	bch2_free_super(&sb);
-
-	return err;
-}
-
 /* Global interfaces/init */
 
 static void bcachefs_exit(void)

fs/bcachefs/super.h

@@ -254,6 +254,5 @@ void bch2_fs_stop(struct bch_fs *);
 
 int bch2_fs_start(struct bch_fs *);
 struct bch_fs *bch2_fs_open(char * const *, unsigned, struct bch_opts);
-const char *bch2_fs_open_incremental(const char *path);
 
 #endif /* _BCACHEFS_SUPER_H */