[PATCH v4 0/2]ndctl/namespace: Fix and improve write-infoblock

[PATCH v4 0/2]ndctl/namespace: Fix and improve write-infoblock

[Tarun Sahu]

This series resolves some issues with write-infoblock 
command and provide support to write-infoblock for sector 
mode namespace

write-infoblock command has issues regarding updating the 
align, uuid, parent_uuid. In case of no parameter passed 
for it, this command used to overwrite the existing values 
with defaults.

In PATCH 1/2 these parameters will be set to their original 
values, incase, values hasn't been passed in command 
arguments

write-infoblock command doesn't have support for sector/BTT 
mode namespaces. They can be converted to fsdax, but can 
not be written being in sector mode.

In PATCH 2/2, It creates a functionality which write 
infoblock of Sector/BTT namespace. Currently only uuid, 
parent_uuid can be updated. In future, Support for other 
parameters can easily be integrated in the
functionality.

---
v2:
  Updated the commit message (rephrasing) in patch 1/2
  Moved the ns_info struct to namespace.c from namespace.h
  put the results after --- to avoid long commit message

v3:
  reformat the commit message to meet 100 column condition

v4:
  - Moved the struct ns_info definition to the beginning of
  the block 
  - Initialized the buf of ns_info structure in ns_info_init
  - Change the format of comment in code from "//" to "/**/"
  - reword the commit message of patch 2/2

Tarun Sahu (2):
  ndctl/namespace: Fix multiple issues with write-infoblock
  ndctl/namespace: Implement write-infoblock for sector mode namespaces

 ndctl/namespace.c | 314 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 231 insertions(+), 83 deletions(-)

-- 
2.35.1

[PATCH v4 1/2] ndctl/namespace: Fix multiple issues with write-infoblock

[Tarun Sahu]

Write-infoblock command has the below issues,
1 - Oerwriting the existing alignment value with the default value when
      not passed as parameter.
2 - Changing the mode of the namespace to fsdax when -m not specified
3 - Incorrectly updating the uuid and parent_uuid if corresponding
parameter is not specified

Considering the above three issues, we first needed to read the
original infoblock if available, and update the align, uuid, parent_uuid
to its original value while writing the infoblock if corresponding
parameter is not passed.

This will resolve the 1, 3 issues. To resolve 2, instead of setting
the value of param.mode to fsdax by default, set it to unknown and
fetch the value of namespace mode type while reading the namespace info
and update only the parameter passed related to the namespace mode.

Originally the file-read-infoblock function read the infoblock in
binary or json format based on parameter -j (passed or not).
By default getting json and parsing the original values is one of
the way to resolve the issue, but it will create complexity in
functionality as everytime, there is a need to update the parameter,
the type of namespace mode will have to be checked that is too by
reading the signature values of infoblock.
(ref: parse_namespace_infoblock) as namespace is disabled for
read/write infoblock command.

Instead, To make the process easier and to support future adaptability
for incoming issues or feature, where details of namespace infoblock,
mode, offset in backup infoblock is required in general for fsdax,
devdax, or sector mode, I created a new structure which store,
buffer of infoblock, mode of infoblock, and offset value at which the
infoblock is available in buffer. (struct ns_info)

struct ns_info{
    void *ns_sb_buf;
    enum ndctl_namespace_mode mode;
    size_t offset;
}

There is one constructor ns_info_init(struct ns_info *ns_info) which
allocate INFOBLOCK_SZ bytes of memory to ns_info->ns_sb_buf. This buf
once allocated, can be reused in file_read_infoblock,
file_write_infoblock, write_pfn_sb, parse_namespace_infoblock, without
worrying about creating buf and freeing buf at many places for the same
purpose/task. this structure can also store the infoblock data in this
buffer, which can be checked for its original values to update it while
writing the infoblock.

There is one destructor ns_info_destroy(struct ns_info *ns_info) which
free the buffer, allocated at the time of ns_info_init(struct ns_info).

Signed-off-by: Tarun Sahu <tsahu@linux.ibm.com>
Reviewed-by: Shivaprasad G Bhat <sbhat@linux.ibm.com>
---
 ndctl/namespace.c | 260 +++++++++++++++++++++++++++++++---------------
 1 file changed, 177 insertions(+), 83 deletions(-)

diff --git a/ndctl/namespace.c b/ndctl/namespace.c
index 257b58c..e890fc2 100644
--- a/ndctl/namespace.c
+++ b/ndctl/namespace.c
@@ -62,6 +62,12 @@ static struct parameters {
 	.autorecover = true,
 };
 
+struct ns_info {
+	void *ns_sb_buf;
+	enum ndctl_namespace_mode mode;
+	size_t offset;
+};
+
 const char *cmd_name = "namespace";
 
 void builtin_xaction_namespace_reset(void)
@@ -256,7 +262,7 @@ static int set_defaults(enum device_action action)
 			break;
 		}
 	} else if (action == ACTION_WRITE_INFOBLOCK) {
-		param.mode = "fsdax";
+		param.mode = "unknown";
 	} else if (!param.reconfig && param.type) {
 		if (strcmp(param.type, "pmem") == 0)
 			param.mode = "fsdax";
@@ -1750,70 +1756,110 @@ err:
 
 #define INFOBLOCK_SZ SZ_8K
 
-static int parse_namespace_infoblock(char *_buf, struct ndctl_namespace *ndns,
+static int ns_info_init(struct ns_info *ns_info)
+{
+	char *buf = NULL;
+
+	if (!ns_info)
+		return -EINVAL;
+	/*
+	 * Initializing it to null so that it can be checked properly in
+	 * ns_info_destroy function
+	 */
+	ns_info->ns_sb_buf = NULL;
+	buf = calloc(1, INFOBLOCK_SZ);
+	if (!buf)
+		return -ENOMEM;
+	ns_info->ns_sb_buf = buf;
+	ns_info->mode = NDCTL_NS_MODE_UNKNOWN;
+	return 0;
+}
+
+static void ns_info_destroy(struct ns_info *ns_info)
+{
+	if (ns_info && ns_info->ns_sb_buf)
+		free(ns_info->ns_sb_buf);
+}
+
+static int parse_namespace_infoblock(struct ns_info *ns_info, struct ndctl_namespace *ndns,
 		const char *path, struct read_infoblock_ctx *ri_ctx)
 {
 	int rc;
-	void *buf = _buf;
+	void *buf = ns_info->ns_sb_buf;
 	unsigned long flags = param.human ? UTIL_JSON_HUMAN : 0;
 	struct btt_sb *btt1_sb = buf + SZ_4K, *btt2_sb = buf;
-	struct json_object *jblock = NULL, *jblocks = ri_ctx->jblocks;
+	struct json_object *jblock = NULL, *jblocks = NULL;
 	struct pfn_sb *pfn_sb = buf + SZ_4K, *dax_sb = buf + SZ_4K;
 
-	if (!param.json && !param.human) {
-		rc = fwrite(buf, 1, INFOBLOCK_SZ, ri_ctx->f_out);
-		if (rc != INFOBLOCK_SZ)
-			return -EIO;
-		fflush(ri_ctx->f_out);
-		return 0;
-	}
+	if (ri_ctx) {
+		jblocks = ri_ctx->jblocks;
+
+		if (!param.json && !param.human) {
+			rc = fwrite(buf, 1, INFOBLOCK_SZ, ri_ctx->f_out);
+			if (rc != INFOBLOCK_SZ)
+				return -EIO;
+			fflush(ri_ctx->f_out);
+			return 0;
+		}
 
-	if (!jblocks) {
-		jblocks = json_object_new_array();
-		if (!jblocks)
-			return -ENOMEM;
-		ri_ctx->jblocks = jblocks;
+		if (!jblocks) {
+			jblocks = json_object_new_array();
+			if (!jblocks)
+				return -ENOMEM;
+			ri_ctx->jblocks = jblocks;
+		}
 	}
 
 	if (memcmp(btt1_sb->signature, BTT_SIG, BTT_SIG_LEN) == 0) {
-		jblock = btt_parse(btt1_sb, ndns, path, flags);
-		if (jblock)
-			json_object_array_add(jblocks, jblock);
+		ns_info->offset = BTT1_START_OFFSET;
+		ns_info->mode = NDCTL_NS_MODE_SECTOR;
+		if (ri_ctx) {
+			jblock = btt_parse(btt1_sb, ndns, path, flags);
+			if (jblock)
+				json_object_array_add(jblocks, jblock);
+		}
 	}
 
 	if (memcmp(btt2_sb->signature, BTT_SIG, BTT_SIG_LEN) == 0) {
-		jblock = btt_parse(btt2_sb, ndns, path, flags);
-		if (jblock)
-			json_object_array_add(jblocks, jblock);
+		ns_info->offset = BTT2_START_OFFSET;
+		ns_info->mode = NDCTL_NS_MODE_SECTOR;
+		if (ri_ctx) {
+			jblock = btt_parse(btt2_sb, ndns, path, flags);
+			if (jblock)
+				json_object_array_add(jblocks, jblock);
+		}
 	}
 
 	if (memcmp(pfn_sb->signature, PFN_SIG, PFN_SIG_LEN) == 0) {
-		jblock = pfn_parse(pfn_sb, ndns, path, flags);
-		if (jblock)
-			json_object_array_add(jblocks, jblock);
+		ns_info->offset = SZ_4K;
+		ns_info->mode = NDCTL_NS_MODE_FSDAX;
+		if (ri_ctx) {
+			jblock = pfn_parse(pfn_sb, ndns, path, flags);
+			if (jblock)
+				json_object_array_add(jblocks, jblock);
+		}
 	}
 
 	if (memcmp(dax_sb->signature, DAX_SIG, PFN_SIG_LEN) == 0) {
-		jblock = pfn_parse(dax_sb, ndns, path, flags);
-		if (jblock)
-			json_object_array_add(jblocks, jblock);
+		ns_info->offset = SZ_4K;
+		ns_info->mode = NDCTL_NS_MODE_DEVDAX;
+		if (ri_ctx) {
+			jblock = pfn_parse(dax_sb, ndns, path, flags);
+			if (jblock)
+				json_object_array_add(jblocks, jblock);
+		}
 	}
 
 	return 0;
 }
 
 static int file_read_infoblock(const char *path, struct ndctl_namespace *ndns,
-		struct read_infoblock_ctx *ri_ctx)
+		struct read_infoblock_ctx *ri_ctx, struct ns_info *ns_info)
 {
 	const char *devname = ndns ? ndctl_namespace_get_devname(ndns) : "";
 	const char *cmd = "read-infoblock";
-	void *buf = NULL;
 	int fd = -1, rc;
 
-	buf = calloc(1, INFOBLOCK_SZ);
-	if (!buf)
-		return -ENOMEM;
-
 	if (!path) {
 		fd = STDIN_FILENO;
 		path = "<stdin>";
@@ -1827,7 +1873,7 @@ static int file_read_infoblock(const char *path, struct ndctl_namespace *ndns,
 		goto out;
 	}
 
-	rc = read(fd, buf, INFOBLOCK_SZ);
+	rc = read(fd, ns_info->ns_sb_buf, INFOBLOCK_SZ);
 	if (rc < INFOBLOCK_SZ) {
 		pr_verbose("%s: %s failed to read %s: %s\n",
 				cmd, devname, path, strerror(errno));
@@ -1837,10 +1883,8 @@ static int file_read_infoblock(const char *path, struct ndctl_namespace *ndns,
 			rc = -EIO;
 		goto out;
 	}
-
-	rc = parse_namespace_infoblock(buf, ndns, path, ri_ctx);
+	rc = parse_namespace_infoblock(ns_info, ndns, path, ri_ctx);
 out:
-	free(buf);
 	if (fd >= 0 && fd != STDIN_FILENO)
 		close(fd);
 	return rc;
@@ -1857,10 +1901,10 @@ static unsigned long PHYS_PFN(unsigned long long phys)
 
 /* Derived from nd_pfn_init() in kernel version v5.5 */
 static int write_pfn_sb(int fd, unsigned long long size, const char *sig,
-		void *buf)
+		struct ns_info *ns_info)
 {
 	unsigned long npfns, align, pfn_align;
-	struct pfn_sb *pfn_sb = buf + SZ_4K;
+	struct pfn_sb *pfn_sb = ns_info->ns_sb_buf + SZ_4K;
 	unsigned long long start, offset;
 	uuid_t uuid, parent_uuid;
 	u32 end_trunc, start_pad;
@@ -1874,24 +1918,62 @@ static int write_pfn_sb(int fd, unsigned long long size, const char *sig,
 	align = max(pfn_align, SUBSECTION_SIZE);
 	if (param.uuid)
 		uuid_parse(param.uuid, uuid);
-	else
-		uuid_generate(uuid);
-
+	else {
+		switch (ns_info->mode) {
+		case NDCTL_NS_MODE_FSDAX:
+		case NDCTL_NS_MODE_DEVDAX:
+			memcpy(uuid,
+				((struct pfn_sb *)(ns_info->ns_sb_buf + ns_info->offset))->uuid,
+				sizeof(uuid_t));
+			break;
+		case NDCTL_NS_MODE_SECTOR:
+			memcpy(uuid,
+				((struct btt_sb *)(ns_info->ns_sb_buf + ns_info->offset))->uuid,
+				sizeof(uuid_t));
+			break;
+		default:
+			uuid_generate(uuid);
+			break;
+		}
+	}
 	if (param.parent_uuid)
 		uuid_parse(param.parent_uuid, parent_uuid);
-	else
-		memset(parent_uuid, 0, sizeof(uuid_t));
+	else {
+		switch (ns_info->mode) {
+		case NDCTL_NS_MODE_FSDAX:
+		case NDCTL_NS_MODE_DEVDAX:
+			memcpy(parent_uuid,
+				((struct pfn_sb *)(ns_info->ns_sb_buf + ns_info->offset))
+				->parent_uuid, sizeof(uuid_t));
+			break;
+		case NDCTL_NS_MODE_SECTOR:
+			memcpy(parent_uuid,
+				((struct btt_sb *)(ns_info->ns_sb_buf + ns_info->offset))
+				->parent_uuid, sizeof(uuid_t));
+			break;
+		default:
+			memset(parent_uuid, 0, sizeof(uuid_t));
+			break;
+		}
+	}
 
 	if (strcmp(param.map, "dev") == 0)
 		mode = PFN_MODE_PMEM;
 	else
 		mode = PFN_MODE_RAM;
 
+	/*
+	 * If originally the namespace was of sector type with backup
+	 * infoblock offset 0, resetting those bits.
+	 */
+	memset(ns_info->ns_sb_buf, 0, SZ_4K);
+
 	/*
 	 * Unlike the kernel implementation always set start_pad and
 	 * end_trunc relative to the specified --offset= option to allow
 	 * testing legacy / "buggy" configurations.
 	 */
+
 	start_pad = ALIGN(start, align) - start;
 	end_trunc = start + size - ALIGN_DOWN(start + size, align);
 	if (mode == PFN_MODE_PMEM) {
@@ -1933,17 +2015,16 @@ static int write_pfn_sb(int fd, unsigned long long size, const char *sig,
 	checksum = fletcher64(pfn_sb, sizeof(*pfn_sb), 0);
 	pfn_sb->checksum = cpu_to_le64(checksum);
 
-	rc = write(fd, buf, INFOBLOCK_SZ);
+	rc = write(fd, ns_info->ns_sb_buf, INFOBLOCK_SZ);
 	if (rc < INFOBLOCK_SZ)
 		return -EIO;
 	return 0;
 }
 
-static int file_write_infoblock(const char *path)
+static int file_write_infoblock(const char *path, struct ns_info *ns_info)
 {
 	unsigned long long size = parse_size64(param.size);
 	int fd = -1, rc;
-	void *buf;
 
 	if (param.std_out)
 		fd = STDOUT_FILENO;
@@ -1980,25 +2061,30 @@ static int file_write_infoblock(const char *path)
 		}
 	}
 
-	buf = calloc(INFOBLOCK_SZ, 1);
-	if (!buf) {
-		rc = -ENOMEM;
-		goto out;
-	}
-
 	switch (util_nsmode(param.mode)) {
 	case NDCTL_NS_MODE_FSDAX:
-		rc = write_pfn_sb(fd, size, PFN_SIG, buf);
+		rc = write_pfn_sb(fd, size, PFN_SIG, ns_info);
 		break;
 	case NDCTL_NS_MODE_DEVDAX:
-		rc = write_pfn_sb(fd, size, DAX_SIG, buf);
+		rc = write_pfn_sb(fd, size, DAX_SIG, ns_info);
+		break;
+	case NDCTL_NS_MODE_UNKNOWN:
+		switch (ns_info->mode) {
+		case NDCTL_NS_MODE_FSDAX:
+			rc = write_pfn_sb(fd, size, PFN_SIG, ns_info);
+			break;
+		case NDCTL_NS_MODE_DEVDAX:
+			rc = write_pfn_sb(fd, size, DAX_SIG, ns_info);
+			break;
+		default:
+			rc = -EINVAL;
+			break;
+		}
 		break;
 	default:
 		rc = -EINVAL;
 		break;
 	}
-
-	free(buf);
 out:
 	if (fd >= 0 && fd != STDOUT_FILENO)
 		close(fd);
@@ -2026,12 +2112,10 @@ static int namespace_rw_infoblock(struct ndctl_namespace *ndns,
 		struct read_infoblock_ctx *ri_ctx, int write)
 {
 	int rc;
-	uuid_t uuid;
-	char str[40];
 	char path[50];
-	const char *save;
 	const char *cmd = write ? "write-infoblock" : "read-infoblock";
 	const char *devname = ndctl_namespace_get_devname(ndns);
+	struct ns_info ns_info;
 
 	if (ndctl_namespace_is_active(ndns)) {
 		pr_verbose("%s: %s enabled, must be disabled\n", cmd, devname);
@@ -2045,21 +2129,22 @@ static int namespace_rw_infoblock(struct ndctl_namespace *ndns,
 		goto out;
 	}
 
-	save = param.parent_uuid;
-	if (!param.parent_uuid) {
-		ndctl_namespace_get_uuid(ndns, uuid);
-		uuid_unparse(uuid, str);
-		param.parent_uuid = str;
-	}
-
 	sprintf(path, "/dev/%s", ndctl_namespace_get_block_device(ndns));
-	if (write) {
+	if (ns_info_init(&ns_info) != 0)
+		goto out;
+
+	rc = file_read_infoblock(path, ndns, ri_ctx, &ns_info);
+	if (!rc && write) {
 		unsigned long long align;
 		bool align_provided = true;
 
 		if (!param.align) {
 			align = ndctl_get_default_alignment(ndns);
-
+			if (ns_info.mode == NDCTL_NS_MODE_FSDAX ||
+					ns_info.mode == NDCTL_NS_MODE_DEVDAX) {
+				align = ((struct pfn_sb *)(ns_info.ns_sb_buf + ns_info.offset))->
+					align;
+			}
 			if (asprintf((char **)&param.align, "%llu", align) < 0) {
 				rc = -EINVAL;
 				goto out;
@@ -2078,18 +2163,16 @@ static int namespace_rw_infoblock(struct ndctl_namespace *ndns,
 				rc = -EINVAL;
 			}
 		}
-
 		if (!rc)
-			rc = file_write_infoblock(path);
+			rc = file_write_infoblock(path, &ns_info);
 
 		if (!align_provided) {
 			free((char *)param.align);
 			param.align = NULL;
 		}
-	} else
-		rc = file_read_infoblock(path, ndns, ri_ctx);
-	param.parent_uuid = save;
+	}
 out:
+	ns_info_destroy(&ns_info);
 	ndctl_namespace_set_raw_mode(ndns, 0);
 	ndctl_namespace_disable_invalidate(ndns);
 	return rc;
@@ -2121,22 +2204,33 @@ static int do_xaction_namespace(const char *namespace,
 		}
 
 		if (param.infile || !namespace) {
-			rc = file_read_infoblock(param.infile, NULL, &ri_ctx);
-			if (ri_ctx.jblocks)
-				util_display_json_array(ri_ctx.f_out, ri_ctx.jblocks, 0);
-			if (rc >= 0)
-				(*processed)++;
+			struct ns_info ns_info;
+
+			rc = ns_info_init(&ns_info);
+			if (!rc) {
+				rc = file_read_infoblock(param.infile, NULL, &ri_ctx, &ns_info);
+				if (ri_ctx.jblocks)
+					util_display_json_array(ri_ctx.f_out, ri_ctx.jblocks, 0);
+				if (rc >= 0)
+					(*processed)++;
+			}
+			ns_info_destroy(&ns_info);
 			return rc;
 		}
 	}
 
 	if (action == ACTION_WRITE_INFOBLOCK && !namespace) {
+		struct ns_info ns_info;
 		if (!param.align)
 			param.align = "2M";
 
-		rc = file_write_infoblock(param.outfile);
-		if (rc >= 0)
-			(*processed)++;
+		rc = ns_info_init(&ns_info);
+		if (!rc) {
+			rc = file_write_infoblock(param.outfile, &ns_info);
+			if (rc >= 0)
+				(*processed)++;
+		}
+		ns_info_destroy(&ns_info);
 		return rc;
 	}
 
-- 
2.35.1

[PATCH v4 2/2] ndctl/namespace: Implement write-infoblock for sector mode namespaces

[Tarun Sahu]

While writing to the infoblock for sector mode, collected
namespace info in ns_info structure can be used to write the
original infoblock values except the ones that have been
provided by parameter arguments to write-infoblock command.

Currently, this patch allows only uuid and parent_uuid to be
changed. In future, support for other values can be implemented
easily by adding condition inside write_btt_sb() function, which
has been created to write BTT sector mode specific namespace.

$ ./ndctl read-infoblock namespace0.0 -j
[
  {
    "dev":"namespace0.0",
    "signature":"BTT_ARENA_INFO",
    "uuid":"7296ebd6-7039-4d02-9ed9-ac5f351ff0e4",
    "parent_uuid":"46850d31-8fb5-4492-b630-9f3cd7b77500",
    "flags":0,
    "version":"2.0",
    "external_lbasize":4096,
    "external_nlba":8380160,
    "internal_lbasize":4096,
    "internal_nlba":8380416,
    "nfree":256,
    "infosize":4096,
    "nextoff":0,
    "dataoff":4096,
    "mapoff":34326196224,
    "logoff":34359717888,
    "info2off":34359734272
  }
]
read 1 infoblock

$ ./ndctl write-infoblock namespace0.0 \
>	--uuid "d0b19883-0874-4847-8c71-71549590949c"
wrote 1 infoblock

$ ./ndctl read-infoblock namespace0.0 -j
[
  {
    "dev":"namespace0.0",
    "signature":"BTT_ARENA_INFO",
    "uuid":"d0b19883-0874-4847-8c71-71549590949c",
    "parent_uuid":"46850d31-8fb5-4492-b630-9f3cd7b77500",
    "flags":0,
    "version":"2.0",
    "external_lbasize":4096,
    "external_nlba":8380160,
    "internal_lbasize":4096,
    "internal_nlba":8380416,
    "nfree":256,
    "infosize":4096,
    "nextoff":0,
    "dataoff":4096,
    "mapoff":34326196224,
    "logoff":34359717888,
    "info2off":34359734272
  }
]
read 1 infoblock

Signed-off-by: Tarun Sahu <tsahu@linux.ibm.com>
---
 ndctl/namespace.c | 54 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/ndctl/namespace.c b/ndctl/namespace.c
index e890fc2..2b0c821 100644
--- a/ndctl/namespace.c
+++ b/ndctl/namespace.c
@@ -253,6 +253,7 @@ static int set_defaults(enum device_action action)
 			break;
 		case NDCTL_NS_MODE_FSDAX:
 		case NDCTL_NS_MODE_DEVDAX:
+		case NDCTL_NS_MODE_SECTOR:
 			break;
 		default:
 			if (action == ACTION_WRITE_INFOBLOCK) {
@@ -2021,6 +2022,48 @@ static int write_pfn_sb(int fd, unsigned long long size, const char *sig,
 	return 0;
 }
 
+static int write_btt_sb(const int fd, unsigned long long size, struct ns_info *ns_info)
+{
+	int rc = 0;
+	uuid_t uuid, parent_uuid;
+
+	// updating the original values which are asked to change,
+	// rest will be unchanged
+	if (param.uuid) {
+		rc = uuid_parse(param.uuid, uuid);
+		if (rc) {
+			error("Failed to parse UUID");
+			return rc;
+		}
+		memcpy(((struct btt_sb *)(ns_info->ns_sb_buf + ns_info->offset))->uuid,
+				uuid, sizeof(uuid_t));
+	}
+	if (param.parent_uuid) {
+		rc = uuid_parse(param.parent_uuid, parent_uuid);
+		if (rc) {
+			error("Failed to parse UUID");
+			return rc;
+		}
+		memcpy(((struct btt_sb *)(ns_info->ns_sb_buf + ns_info->offset))->parent_uuid,
+				parent_uuid, sizeof(uuid_t));
+	}
+
+	if (pwrite(fd, ns_info->ns_sb_buf + ns_info->offset, sizeof(struct btt_sb),
+			       ns_info->offset) < 0) {
+		pr_verbose("Unable to write the info block: %s\n",
+				strerror(errno));
+		rc = -errno;
+	}
+
+	if (pwrite(fd, ns_info->ns_sb_buf + ns_info->offset, sizeof(struct btt_sb),
+				size - sizeof(struct btt_sb)) < 0) {
+		pr_verbose("Unable to write the info block: %s\n",
+			strerror(errno));
+		rc = -errno;
+	}
+	return rc;
+}
+
 static int file_write_infoblock(const char *path, struct ns_info *ns_info)
 {
 	unsigned long long size = parse_size64(param.size);
@@ -2068,6 +2111,14 @@ static int file_write_infoblock(const char *path, struct ns_info *ns_info)
 	case NDCTL_NS_MODE_DEVDAX:
 		rc = write_pfn_sb(fd, size, DAX_SIG, ns_info);
 		break;
+	case NDCTL_NS_MODE_SECTOR:
+		if (ns_info->mode == NDCTL_NS_MODE_SECTOR)
+			rc = write_btt_sb(fd, size, ns_info);
+		else {
+			pr_verbose("Conversion from non-sector to sector mode not allowed");
+			rc = -EPERM;
+		}
+		break;
 	case NDCTL_NS_MODE_UNKNOWN:
 		switch (ns_info->mode) {
 		case NDCTL_NS_MODE_FSDAX:
@@ -2076,6 +2127,9 @@ static int file_write_infoblock(const char *path, struct ns_info *ns_info)
 		case NDCTL_NS_MODE_DEVDAX:
 			rc = write_pfn_sb(fd, size, DAX_SIG, ns_info);
 			break;
+		case NDCTL_NS_MODE_SECTOR:
+			rc = write_btt_sb(fd, size, ns_info);
+			break;
 		default:
 			rc = -EINVAL;
 			break;
-- 
2.35.1

Re: [PATCH v4 0/2]ndctl/namespace: Fix and improve write-infoblock

[Tarun Sahu]

Hi, 

Just a gentle reminder. May you please look into these patches.
Please let me know if any changes are required.

Thanks
Tarun

On Fri, 2022-05-27 at 16:00 +0530, Tarun Sahu wrote:
> This series resolves some issues with write-infoblock 
> command and provide support to write-infoblock for sector 
> mode namespace
> 
> write-infoblock command has issues regarding updating the 
> align, uuid, parent_uuid. In case of no parameter passed 
> for it, this command used to overwrite the existing values 
> with defaults.
> 
> In PATCH 1/2 these parameters will be set to their original 
> values, incase, values hasn't been passed in command 
> arguments
> 
> write-infoblock command doesn't have support for sector/BTT 
> mode namespaces. They can be converted to fsdax, but can 
> not be written being in sector mode.
> 
> In PATCH 2/2, It creates a functionality which write 
> infoblock of Sector/BTT namespace. Currently only uuid, 
> parent_uuid can be updated. In future, Support for other 
> parameters can easily be integrated in the
> functionality.
> 
> ---
> v2:
>   Updated the commit message (rephrasing) in patch 1/2
>   Moved the ns_info struct to namespace.c from namespace.h
>   put the results after --- to avoid long commit message
> 
> v3:
>   reformat the commit message to meet 100 column condition
> 
> v4:
>   - Moved the struct ns_info definition to the beginning of
>   the block 
>   - Initialized the buf of ns_info structure in ns_info_init
>   - Change the format of comment in code from "//" to "/**/"
>   - reword the commit message of patch 2/2
> 
> Tarun Sahu (2):
>   ndctl/namespace: Fix multiple issues with write-infoblock
>   ndctl/namespace: Implement write-infoblock for sector mode
> namespaces
> 
>  ndctl/namespace.c | 314 ++++++++++++++++++++++++++++++++++----------
> --
>  1 file changed, 231 insertions(+), 83 deletions(-)
> 

Re: [PATCH v4 0/2]ndctl/namespace: Fix and improve write-infoblock

[Tarun Sahu]

Hi, 

Just a gentle reminder. May you please look into these patches.
Please let me know if any changes are required.

Thanks
Tarun


On Fri, 2022-05-27 at 16:00 +0530, Tarun Sahu wrote:
> This series resolves some issues with write-infoblock 
> command and provide support to write-infoblock for sector 
> mode namespace
> 
> write-infoblock command has issues regarding updating the 
> align, uuid, parent_uuid. In case of no parameter passed 
> for it, this command used to overwrite the existing values 
> with defaults.
> 
> In PATCH 1/2 these parameters will be set to their original 
> values, incase, values hasn't been passed in command 
> arguments
> 
> write-infoblock command doesn't have support for sector/BTT 
> mode namespaces. They can be converted to fsdax, but can 
> not be written being in sector mode.
> 
> In PATCH 2/2, It creates a functionality which write 
> infoblock of Sector/BTT namespace. Currently only uuid, 
> parent_uuid can be updated. In future, Support for other 
> parameters can easily be integrated in the
> functionality.
> 
> ---
> v2:
>   Updated the commit message (rephrasing) in patch 1/2
>   Moved the ns_info struct to namespace.c from namespace.h
>   put the results after --- to avoid long commit message
> 
> v3:
>   reformat the commit message to meet 100 column condition
> 
> v4:
>   - Moved the struct ns_info definition to the beginning of
>   the block 
>   - Initialized the buf of ns_info structure in ns_info_init
>   - Change the format of comment in code from "//" to "/**/"
>   - reword the commit message of patch 2/2
> 
> Tarun Sahu (2):
>   ndctl/namespace: Fix multiple issues with write-infoblock
>   ndctl/namespace: Implement write-infoblock for sector mode
> namespaces
> 
>  ndctl/namespace.c | 314 ++++++++++++++++++++++++++++++++++----------
> --
>  1 file changed, 231 insertions(+), 83 deletions(-)
> 

Re: [PATCH v4 0/2]ndctl/namespace: Fix and improve write-infoblock

[Verma, Vishal L]

On Fri, 2022-05-27 at 16:00 +0530, Tarun Sahu wrote:
> This series resolves some issues with write-infoblock 
> command and provide support to write-infoblock for sector 
> mode namespace
> 
> write-infoblock command has issues regarding updating the 
> align, uuid, parent_uuid. In case of no parameter passed 
> for it, this command used to overwrite the existing values 
> with defaults.
> 
> In PATCH 1/2 these parameters will be set to their original 
> values, incase, values hasn't been passed in command 
> arguments
> 
> write-infoblock command doesn't have support for sector/BTT 
> mode namespaces. They can be converted to fsdax, but can 
> not be written being in sector mode.
> 
> In PATCH 2/2, It creates a functionality which write 
> infoblock of Sector/BTT namespace. Currently only uuid, 
> parent_uuid can be updated. In future, Support for other 
> parameters can easily be integrated in the
> functionality.

Hi Tarun,

Sorry for the delay in reviewing these. From a quick look, it looks
like you're adding read-modify-write functionality to the write-
infoblock command - is that correct?

I think the original intent of these commands was, purely as a
debug/test aid, that the user would be responsible for reading the
namespace if needed, and using that as a basis for writing the
infoblockl if an RMW operation is desired. However for the most part,
write-infoblock just creates infoblocks out of thin air, and optionally
writes it to a namepspace. I'm not sure adding a read-modify-write here
is really that useful, unless you have a specific use case for this
sort of thing?

> 
> ---
> v2:
>   Updated the commit message (rephrasing) in patch 1/2
>   Moved the ns_info struct to namespace.c from namespace.h
>   put the results after --- to avoid long commit message
> 
> v3:
>   reformat the commit message to meet 100 column condition
> 
> v4:
>   - Moved the struct ns_info definition to the beginning of
>   the block 
>   - Initialized the buf of ns_info structure in ns_info_init
>   - Change the format of comment in code from "//" to "/**/"
>   - reword the commit message of patch 2/2
> 
> Tarun Sahu (2):
>   ndctl/namespace: Fix multiple issues with write-infoblock
>   ndctl/namespace: Implement write-infoblock for sector mode
> namespaces
> 
>  ndctl/namespace.c | 314 ++++++++++++++++++++++++++++++++++----------
> --
>  1 file changed, 231 insertions(+), 83 deletions(-)
> 

Re: [PATCH v4 0/2]ndctl/namespace: Fix and improve write-infoblock

[Tarun Sahu]

Hi,

> Hi Tarun,
> 
> Sorry for the delay in reviewing these. From a quick look, it looks
> like you're adding read-modify-write functionality to the write-
> infoblock command - is that correct?
> 
> I think the original intent of these commands was, purely as a
> debug/test aid, that the user would be responsible for reading the
> namespace if needed, and using that as a basis for writing the
> infoblockl if an RMW operation is desired. However for the most part,
> write-infoblock just creates infoblocks out of thin air, and
> optionally
> writes it to a namepspace. I'm not sure adding a read-modify-write
> here
> is really that useful, unless you have a specific use case for this
> sort of thing?

From the man page, it is not clear that, non-provided args values will
get updated without user concern, and It comes as surprise to user as
user didnt intend to change that. For Example, If user only want to
change the value of uuid of a namespace infoblock, So They perform
write-infoblock with particular uuid but didnt pass align value which
they intended not to change but write-infoblock changes it to default
value.

This patch, read-modify-write takes care of other arguments which are
not passed for the concerned namespace as part of write-infoblock by
retaining the values of them.

Other patch of this series, takes
advantage of the above read-modify-write functionality and implement
write-infoblock for sector/BTT namespaces. Which currently allows only
uuid and parent_uuid updates.