[PATCH 0/5] Zoned block device support improvments

[PATCH 0/5] Zoned block device support improvments

[Damien Le Moal]

The first 2 patches of this series are not zoned block device specific
and fix compilation warnings and modify .gitignore file to handle
/tools/f2fs_io/f2fs_io.

The last 3 patches improvements of zoned block device support, with the
last patch 5 being the most important.

Damien Le Moal (5):
  f2fs-tools: Fix various compilation warnings
  f2fs-tools: Add f2fs_io to .gitignore
  f2fs-tools: Improve zoned model check
  f2fs-tools: Allow using host-aware devices as regular devices
  f2fs-tools: Fix multi-device format with zoned devices

 .gitignore          |  1 +
 include/f2fs_fs.h   |  8 +++----
 lib/libf2fs.c       | 53 +++++++++++++++++++++++++++++++--------------
 lib/libf2fs_zoned.c | 22 ++++++++++---------
 tools/f2fstat.c     |  2 +-
 5 files changed, 55 insertions(+), 31 deletions(-)

-- 
2.20.1

[PATCH 1/5] f2fs-tools: Fix various compilation warnings

[Damien Le Moal]

Avoid various compilation warnings due to strncpy:

libf2fs.c:590:33: warning: ‘snprintf’ output may be truncated before
the last format character [-Wformat-truncation=]
  snprintf(rootdev, ret, "/dev/%s", buf);

../include/f2fs_fs.h:1384:2: warning: ‘strncpy’ specified bound
depends on the length of the source argument [-Wstringop-overflow=]
  strncpy(buf, features, strlen(features) + 1);

f2fstat.c:243:3: warning: ‘strncpy’ output truncated before
terminating nul copying as many bytes from a string as its length
[-Wstringop-truncation]
   strncpy(ptr_buf, name[i], strlen(name[i]));

This patch does not change any functionality.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 include/f2fs_fs.h | 6 +++---
 lib/libf2fs.c     | 2 +-
 tools/f2fstat.c   | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/include/f2fs_fs.h b/include/f2fs_fs.h
index a730688..e073723 100644
--- a/include/f2fs_fs.h
+++ b/include/f2fs_fs.h
@@ -1379,9 +1379,9 @@ static inline int parse_feature(struct feature *table, const char *features)
 {
 	char *buf, *sub, *next;
 
-	buf = calloc(strlen(features) + 1, sizeof(char));
-	ASSERT(buf);
-	strncpy(buf, features, strlen(features) + 1);
+	buf = strdup(features);
+	if (!buf)
+		return -1;
 
 	for (sub = buf; sub && *sub; sub = next ? next + 1 : NULL) {
 		/* Skip the beginning blanks */
diff --git a/lib/libf2fs.c b/lib/libf2fs.c
index c692bf2..60b84e0 100644
--- a/lib/libf2fs.c
+++ b/lib/libf2fs.c
@@ -587,7 +587,7 @@ char *get_rootdev()
 		return NULL;
 	rootdev[ret] = '\0';
 
-	snprintf(rootdev, ret, "/dev/%s", buf);
+	snprintf(rootdev, ret + 1, "/dev/%s", buf);
 	return rootdev;
 #endif
 }
diff --git a/tools/f2fstat.c b/tools/f2fstat.c
index 7f485b0..0618806 100644
--- a/tools/f2fstat.c
+++ b/tools/f2fstat.c
@@ -240,7 +240,7 @@ void print_head(char *res)
 
 	for (i = 0; i < 20; i++) {
 		ptr = (i == 0) ? strtok(res, " ") : strtok(NULL, " ");
-		strncpy(ptr_buf, name[i], strlen(name[i]));
+		strcpy(ptr_buf, name[i]);
 		if (i == 1) {
 			prev_index = ptr_buf - buf - 1;
 		} else if (i == 7) {
-- 
2.20.1



_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

[PATCH 2/5] f2fs-tools: Add f2fs_io to .gitignore

[Damien Le Moal]

GIT ignore compiled executable tools/f2fs_io/f2fs_io.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 .gitignore | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.gitignore b/.gitignore
index 3f04e85..c1341da 100644
--- a/.gitignore
+++ b/.gitignore
@@ -50,6 +50,7 @@ stamp-h1
 /tools/fibmap.f2fs
 /tools/parse.f2fs
 /tools/f2fscrypt
+/tools/f2fs_io/f2fs_io
 /tools/sg_write_buffer/sg_write_buffer
 
 # cscope files
-- 
2.20.1

[PATCH 3/5] f2fs-tools: Improve zoned model check

[Damien Le Moal]

Return an error if an unknown zoned model is reported for a device.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 include/f2fs_fs.h   |  2 +-
 lib/libf2fs.c       |  8 ++++++--
 lib/libf2fs_zoned.c | 20 +++++++++++---------
 3 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/include/f2fs_fs.h b/include/f2fs_fs.h
index e073723..97ad774 100644
--- a/include/f2fs_fs.h
+++ b/include/f2fs_fs.h
@@ -1257,7 +1257,7 @@ blk_zone_cond_str(struct blk_zone *blkz)
 
 #endif
 
-extern void f2fs_get_zoned_model(int);
+extern int f2fs_get_zoned_model(int);
 extern int f2fs_get_zone_blocks(int);
 extern int f2fs_check_zones(int);
 extern int f2fs_reset_zones(int);
diff --git a/lib/libf2fs.c b/lib/libf2fs.c
index 60b84e0..5ca1bb0 100644
--- a/lib/libf2fs.c
+++ b/lib/libf2fs.c
@@ -925,8 +925,12 @@ int get_device_info(int i)
 	}
 
 #if !defined(WITH_ANDROID) && defined(__linux__)
-	if (S_ISBLK(stat_buf->st_mode))
-		f2fs_get_zoned_model(i);
+	if (S_ISBLK(stat_buf->st_mode)) {
+		if (f2fs_get_zoned_model(i) < 0) {
+			free(stat_buf);
+			return -1;
+                }
+	}
 
 	if (dev->zoned_model != F2FS_ZONED_NONE) {
 		if (dev->zoned_model == F2FS_ZONED_HM)
diff --git a/lib/libf2fs_zoned.c b/lib/libf2fs_zoned.c
index 6e32f32..1a6c5df 100644
--- a/lib/libf2fs_zoned.c
+++ b/lib/libf2fs_zoned.c
@@ -24,39 +24,41 @@
 
 #ifdef HAVE_LINUX_BLKZONED_H
 
-void f2fs_get_zoned_model(int i)
+int f2fs_get_zoned_model(int i)
 {
 	struct device_info *dev = c.devices + i;
 	char str[128];
 	FILE *file;
 	int res;
 
+	/* Assume regular device */
+	dev->zoned_model = F2FS_ZONED_NONE;
+
 	/* Check that this is a zoned block device */
 	snprintf(str, sizeof(str),
 		 "/sys/block/%s/queue/zoned",
 		 basename(dev->path));
 	file = fopen(str, "r");
 	if (!file)
-		goto not_zoned;
+		return 0;
 
 	memset(str, 0, sizeof(str));
 	res = fscanf(file, "%s", str);
 	fclose(file);
 
 	if (res != 1)
-		goto not_zoned;
+		return 0;
 
 	if (strcmp(str, "host-aware") == 0) {
 		dev->zoned_model = F2FS_ZONED_HA;
-		return;
-	}
-	if (strcmp(str, "host-managed") == 0) {
+	} else if (strcmp(str, "host-managed") == 0) {
 		dev->zoned_model = F2FS_ZONED_HM;
-		return;
+	} else {
+		MSG(0, "\tError: Unsupported device zoned model\n");
+		return -1;
 	}
 
-not_zoned:
-	dev->zoned_model = F2FS_ZONED_NONE;
+	return 0;
 }
 
 int f2fs_get_zone_blocks(int i)
-- 
2.20.1

[PATCH 4/5] f2fs-tools: Allow using host-aware devices as regular devices

[Damien Le Moal]

Host-aware zoned block devices can accept random writes anywhere and so
do not require to be handled under F2FS_ZONED_HM mode. Allow host aware
disks to be treated as regular devices if c.zoned_mode is false, that
is, if the -m option is not specified in mkfs.f2fs.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 lib/libf2fs.c | 43 ++++++++++++++++++++++++++++++-------------
 1 file changed, 30 insertions(+), 13 deletions(-)

diff --git a/lib/libf2fs.c b/lib/libf2fs.c
index 5ca1bb0..214c921 100644
--- a/lib/libf2fs.c
+++ b/lib/libf2fs.c
@@ -933,9 +933,21 @@ int get_device_info(int i)
 	}
 
 	if (dev->zoned_model != F2FS_ZONED_NONE) {
-		if (dev->zoned_model == F2FS_ZONED_HM)
+		if (dev->zoned_model == F2FS_ZONED_HM) {
 			c.zoned_model = F2FS_ZONED_HM;
+		} else {
+			/* F2FS_ZONED_HA */
+			if (c.zoned_mode) {
+				c.zoned_model = F2FS_ZONED_HM;
+			} else {
+				MSG(0, "Info: treating host-aware zoned block "
+				    "device as regular device\n");
+				dev->zoned_model = F2FS_ZONED_NONE;
+			}
+		}
+	}
 
+	if (c.zoned_model == F2FS_ZONED_HM) {
 		if (f2fs_get_zone_blocks(i)) {
 			MSG(0, "\tError: Failed to get number of blocks per zone\n");
 			free(stat_buf);
@@ -1071,6 +1083,7 @@ int get_device_info(int i)
 
 int f2fs_get_device_info(void)
 {
+	bool zoned = false;
 	int i;
 
 	for (i = 0; i < c.ndevs; i++)
@@ -1089,22 +1102,26 @@ int f2fs_get_device_info(void)
 		return -1;
 	}
 
+	/* For zoned devices, the zones sizes must be equal */
 	for (i = 0; i < c.ndevs; i++) {
-		if (c.devices[i].zoned_model != F2FS_ZONED_NONE) {
-			if (c.zone_blocks &&
-				c.zone_blocks != c.devices[i].zone_blocks) {
-				MSG(0, "\tError: not support different zone sizes!!!\n");
-				return -1;
-			}
-			c.zone_blocks = c.devices[i].zone_blocks;
+		if (c.devices[i].zoned_model != F2FS_ZONED_HM)
+			continue;
+
+		zoned = true;
+
+		if (c.zone_blocks &&
+		    c.zone_blocks != c.devices[i].zone_blocks) {
+			MSG(0, "\tError: zones of different size are not supported\n");
+			return -1;
 		}
+		c.zone_blocks = c.devices[i].zone_blocks;
 	}
 
-	/*
-	 * Align sections to the device zone size
-	 * and align F2FS zones to the device zones.
-	 */
-	if (c.zone_blocks) {
+	if (zoned) {
+		/*
+		 * Align sections to the device zone size
+		 * and align F2FS zones to the device zones.
+		 */
 		c.segs_per_sec = c.zone_blocks / DEFAULT_BLOCKS_PER_SEGMENT;
 		c.secs_per_zone = 1;
 	} else {
-- 
2.20.1

[PATCH 5/5] f2fs-tools: Fix multi-device format with zoned devices

[Damien Le Moal]

There is no need to require conventional zones for a zoned block device
that is not the first device of a multi-device volume. So exclude the
check on the number of conventional zones of the device if the device
index is not 0.

Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
---
 lib/libf2fs_zoned.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/libf2fs_zoned.c b/lib/libf2fs_zoned.c
index 1a6c5df..2d8244f 100644
--- a/lib/libf2fs_zoned.c
+++ b/lib/libf2fs_zoned.c
@@ -204,7 +204,7 @@ int f2fs_check_zones(int j)
 		goto out;
 	}
 
-	if (dev->zoned_model == F2FS_ZONED_HM &&
+	if (j == 0 && dev->zoned_model == F2FS_ZONED_HM &&
 			!dev->nr_rnd_zones) {
 		ERR_MSG("No conventional zone for super block\n");
 		ret = -1;
-- 
2.20.1

Re: [PATCH 1/5] f2fs-tools: Fix various compilation warnings

[Chao Yu]

On 2019/3/18 14:39, Damien Le Moal wrote:
> Avoid various compilation warnings due to strncpy:
> 
> libf2fs.c:590:33: warning: ‘snprintf’ output may be truncated before
> the last format character [-Wformat-truncation=]
>   snprintf(rootdev, ret, "/dev/%s", buf);
> 
> ../include/f2fs_fs.h:1384:2: warning: ‘strncpy’ specified bound
> depends on the length of the source argument [-Wstringop-overflow=]
>   strncpy(buf, features, strlen(features) + 1);
> 
> f2fstat.c:243:3: warning: ‘strncpy’ output truncated before
> terminating nul copying as many bytes from a string as its length
> [-Wstringop-truncation]
>    strncpy(ptr_buf, name[i], strlen(name[i]));
> 
> This patch does not change any functionality.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,


_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel

Re: [PATCH 2/5] f2fs-tools: Add f2fs_io to .gitignore

[Chao Yu]

On 2019/3/18 14:39, Damien Le Moal wrote:
> GIT ignore compiled executable tools/f2fs_io/f2fs_io.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,

Re: [PATCH 3/5] f2fs-tools: Improve zoned model check

[Chao Yu]

On 2019/3/18 14:39, Damien Le Moal wrote:
> Return an error if an unknown zoned model is reported for a device.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>

Reviewed-by: Chao Yu <yuchao0@huawei.com>

Thanks,

Re: [PATCH 4/5] f2fs-tools: Allow using host-aware devices as regular devices

[Chao Yu]

On 2019/3/18 14:39, Damien Le Moal wrote:
> Host-aware zoned block devices can accept random writes anywhere and so
> do not require to be handled under F2FS_ZONED_HM mode. Allow host aware
> disks to be treated as regular devices if c.zoned_mode is false, that
> is, if the -m option is not specified in mkfs.f2fs.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
>  lib/libf2fs.c | 43 ++++++++++++++++++++++++++++++-------------
>  1 file changed, 30 insertions(+), 13 deletions(-)
> 
> diff --git a/lib/libf2fs.c b/lib/libf2fs.c
> index 5ca1bb0..214c921 100644
> --- a/lib/libf2fs.c
> +++ b/lib/libf2fs.c
> @@ -933,9 +933,21 @@ int get_device_info(int i)
>  	}
>  
>  	if (dev->zoned_model != F2FS_ZONED_NONE) {
> -		if (dev->zoned_model == F2FS_ZONED_HM)
> +		if (dev->zoned_model == F2FS_ZONED_HM) {
>  			c.zoned_model = F2FS_ZONED_HM;
> +		} else {
> +			/* F2FS_ZONED_HA */
> +			if (c.zoned_mode) {
> +				c.zoned_model = F2FS_ZONED_HM;

May I ask why we change the model from F2FS_ZONED_HA to F2FS_ZONED_HM?

Thanks,

> +			} else {
> +				MSG(0, "Info: treating host-aware zoned block "
> +				    "device as regular device\n");
> +				dev->zoned_model = F2FS_ZONED_NONE;
> +			}
> +		}
> +	}
>  
> +	if (c.zoned_model == F2FS_ZONED_HM) {
>  		if (f2fs_get_zone_blocks(i)) {
>  			MSG(0, "\tError: Failed to get number of blocks per zone\n");
>  			free(stat_buf);
> @@ -1071,6 +1083,7 @@ int get_device_info(int i)
>  
>  int f2fs_get_device_info(void)
>  {
> +	bool zoned = false;
>  	int i;
>  
>  	for (i = 0; i < c.ndevs; i++)
> @@ -1089,22 +1102,26 @@ int f2fs_get_device_info(void)
>  		return -1;
>  	}
>  
> +	/* For zoned devices, the zones sizes must be equal */
>  	for (i = 0; i < c.ndevs; i++) {
> -		if (c.devices[i].zoned_model != F2FS_ZONED_NONE) {
> -			if (c.zone_blocks &&
> -				c.zone_blocks != c.devices[i].zone_blocks) {
> -				MSG(0, "\tError: not support different zone sizes!!!\n");
> -				return -1;
> -			}
> -			c.zone_blocks = c.devices[i].zone_blocks;
> +		if (c.devices[i].zoned_model != F2FS_ZONED_HM)
> +			continue;
> +
> +		zoned = true;
> +
> +		if (c.zone_blocks &&
> +		    c.zone_blocks != c.devices[i].zone_blocks) {
> +			MSG(0, "\tError: zones of different size are not supported\n");
> +			return -1;
>  		}
> +		c.zone_blocks = c.devices[i].zone_blocks;
>  	}
>  
> -	/*
> -	 * Align sections to the device zone size
> -	 * and align F2FS zones to the device zones.
> -	 */
> -	if (c.zone_blocks) {
> +	if (zoned) {
> +		/*
> +		 * Align sections to the device zone size
> +		 * and align F2FS zones to the device zones.
> +		 */
>  		c.segs_per_sec = c.zone_blocks / DEFAULT_BLOCKS_PER_SEGMENT;
>  		c.secs_per_zone = 1;
>  	} else {
> 

Re: [PATCH 5/5] f2fs-tools: Fix multi-device format with zoned devices

[Chao Yu]

On 2019/3/18 14:39, Damien Le Moal wrote:
> There is no need to require conventional zones for a zoned block device
> that is not the first device of a multi-device volume. So exclude the
> check on the number of conventional zones of the device if the device
> index is not 0.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
>  lib/libf2fs_zoned.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/libf2fs_zoned.c b/lib/libf2fs_zoned.c
> index 1a6c5df..2d8244f 100644
> --- a/lib/libf2fs_zoned.c
> +++ b/lib/libf2fs_zoned.c
> @@ -204,7 +204,7 @@ int f2fs_check_zones(int j)
>  		goto out;
>  	}
>  

How about adding one line comment here to describe why we handle condition
as below for better readability? I guess using your commit message here is
enough... :)

> -	if (dev->zoned_model == F2FS_ZONED_HM &&
> +	if (j == 0 && dev->zoned_model == F2FS_ZONED_HM &&
>  			!dev->nr_rnd_zones) {
>  		ERR_MSG("No conventional zone for super block\n");
>  		ret = -1;
> 

Re: [PATCH 4/5] f2fs-tools: Allow using host-aware devices as regular devices

[Damien Le Moal]

On 2019/03/21 17:32, Chao Yu wrote:
> On 2019/3/18 14:39, Damien Le Moal wrote:
>> Host-aware zoned block devices can accept random writes anywhere and so
>> do not require to be handled under F2FS_ZONED_HM mode. Allow host aware
>> disks to be treated as regular devices if c.zoned_mode is false, that
>> is, if the -m option is not specified in mkfs.f2fs.
>>
>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
>> ---
>>  lib/libf2fs.c | 43 ++++++++++++++++++++++++++++++-------------
>>  1 file changed, 30 insertions(+), 13 deletions(-)
>>
>> diff --git a/lib/libf2fs.c b/lib/libf2fs.c
>> index 5ca1bb0..214c921 100644
>> --- a/lib/libf2fs.c
>> +++ b/lib/libf2fs.c
>> @@ -933,9 +933,21 @@ int get_device_info(int i)
>>  	}
>>  
>>  	if (dev->zoned_model != F2FS_ZONED_NONE) {
>> -		if (dev->zoned_model == F2FS_ZONED_HM)
>> +		if (dev->zoned_model == F2FS_ZONED_HM) {
>>  			c.zoned_model = F2FS_ZONED_HM;
>> +		} else {
>> +			/* F2FS_ZONED_HA */
>> +			if (c.zoned_mode) {
>> +				c.zoned_model = F2FS_ZONED_HM;
> 
> May I ask why we change the model from F2FS_ZONED_HA to F2FS_ZONED_HM?
> 
> Thanks,

There is some confusion I think in the code between disk zoned model and f2fs
zoned mode. Currently, F2FS_ZONED_HA is not tested anywhere to drive the check
against c.zoned_mode. Before this patch, the code was:

	if (dev->zoned_model != F2FS_ZONED_NONE) {
		if (dev->zoned_model == F2FS_ZONED_HM)
			c.zoned_model = F2FS_ZONED_HM;

So only HM disks end up setting "c.zoned_model = F2FS_ZONED_HM" which requires
c.zoned_mode to be set. For an F2FS_ZONED_HA disk, c.zoned_model stays as NONE
and c.zone_blocks is set, regardless of c.zoned_mode setting. The end result is
that the HA disk will be used as a regular disk when -m is not specified on mkfs
command line, and as a zoned disk when -m is specified. However, many areas of
the code check for c.zoned_model == F2FS_ZONED_HM, and that is always false for
HA disks.

The idea here is to allow a cleaner control over HA disk use: either use them as
regular disks and ignore that they are zoned, or use them in the same manner as
HM disks, ignoring the fact that HA disks can accept random writes. Anything in
between these 2 modes does not really make sense.

Now, rereading the code and explaining all this, I realize that the commit
message is actually not correct at all and misleading to the intent of the
patch. I will revisit it and check again this patch. We could actually drop it
for now and I can resend later.

In any case, I am sending a v2 for this series.

Thanks for your review !


> 
>> +			} else {
>> +				MSG(0, "Info: treating host-aware zoned block "
>> +				    "device as regular device\n");
>> +				dev->zoned_model = F2FS_ZONED_NONE;
>> +			}
>> +		}
>> +	}
>>  
>> +	if (c.zoned_model == F2FS_ZONED_HM) {
>>  		if (f2fs_get_zone_blocks(i)) {
>>  			MSG(0, "\tError: Failed to get number of blocks per zone\n");
>>  			free(stat_buf);
>> @@ -1071,6 +1083,7 @@ int get_device_info(int i)
>>  
>>  int f2fs_get_device_info(void)
>>  {
>> +	bool zoned = false;
>>  	int i;
>>  
>>  	for (i = 0; i < c.ndevs; i++)
>> @@ -1089,22 +1102,26 @@ int f2fs_get_device_info(void)
>>  		return -1;
>>  	}
>>  
>> +	/* For zoned devices, the zones sizes must be equal */
>>  	for (i = 0; i < c.ndevs; i++) {
>> -		if (c.devices[i].zoned_model != F2FS_ZONED_NONE) {
>> -			if (c.zone_blocks &&
>> -				c.zone_blocks != c.devices[i].zone_blocks) {
>> -				MSG(0, "\tError: not support different zone sizes!!!\n");
>> -				return -1;
>> -			}
>> -			c.zone_blocks = c.devices[i].zone_blocks;
>> +		if (c.devices[i].zoned_model != F2FS_ZONED_HM)
>> +			continue;
>> +
>> +		zoned = true;
>> +
>> +		if (c.zone_blocks &&
>> +		    c.zone_blocks != c.devices[i].zone_blocks) {
>> +			MSG(0, "\tError: zones of different size are not supported\n");
>> +			return -1;
>>  		}
>> +		c.zone_blocks = c.devices[i].zone_blocks;
>>  	}
>>  
>> -	/*
>> -	 * Align sections to the device zone size
>> -	 * and align F2FS zones to the device zones.
>> -	 */
>> -	if (c.zone_blocks) {
>> +	if (zoned) {
>> +		/*
>> +		 * Align sections to the device zone size
>> +		 * and align F2FS zones to the device zones.
>> +		 */
>>  		c.segs_per_sec = c.zone_blocks / DEFAULT_BLOCKS_PER_SEGMENT;
>>  		c.secs_per_zone = 1;
>>  	} else {
>>
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 5/5] f2fs-tools: Fix multi-device format with zoned devices

[Damien Le Moal]

On 2019/03/21 17:41, Chao Yu wrote:
> On 2019/3/18 14:39, Damien Le Moal wrote:
>> There is no need to require conventional zones for a zoned block device
>> that is not the first device of a multi-device volume. So exclude the
>> check on the number of conventional zones of the device if the device
>> index is not 0.
>>
>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
>> ---
>>  lib/libf2fs_zoned.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/lib/libf2fs_zoned.c b/lib/libf2fs_zoned.c
>> index 1a6c5df..2d8244f 100644
>> --- a/lib/libf2fs_zoned.c
>> +++ b/lib/libf2fs_zoned.c
>> @@ -204,7 +204,7 @@ int f2fs_check_zones(int j)
>>  		goto out;
>>  	}
>>  
> 
> How about adding one line comment here to describe why we handle condition
> as below for better readability? I guess using your commit message here is
> enough... :)

Indeed, that would be better. Will add that. Thanks !

> 
>> -	if (dev->zoned_model == F2FS_ZONED_HM &&
>> +	if (j == 0 && dev->zoned_model == F2FS_ZONED_HM &&
>>  			!dev->nr_rnd_zones) {
>>  		ERR_MSG("No conventional zone for super block\n");
>>  		ret = -1;
>>
> 


-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 4/5] f2fs-tools: Allow using host-aware devices as regular devices

[Chao Yu]

On 2019/3/21 17:29, Damien Le Moal wrote:
> On 2019/03/21 17:32, Chao Yu wrote:
>> On 2019/3/18 14:39, Damien Le Moal wrote:
>>> Host-aware zoned block devices can accept random writes anywhere and so
>>> do not require to be handled under F2FS_ZONED_HM mode. Allow host aware
>>> disks to be treated as regular devices if c.zoned_mode is false, that
>>> is, if the -m option is not specified in mkfs.f2fs.
>>>
>>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
>>> ---
>>>  lib/libf2fs.c | 43 ++++++++++++++++++++++++++++++-------------
>>>  1 file changed, 30 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/lib/libf2fs.c b/lib/libf2fs.c
>>> index 5ca1bb0..214c921 100644
>>> --- a/lib/libf2fs.c
>>> +++ b/lib/libf2fs.c
>>> @@ -933,9 +933,21 @@ int get_device_info(int i)
>>>  	}
>>>  
>>>  	if (dev->zoned_model != F2FS_ZONED_NONE) {
>>> -		if (dev->zoned_model == F2FS_ZONED_HM)
>>> +		if (dev->zoned_model == F2FS_ZONED_HM) {
>>>  			c.zoned_model = F2FS_ZONED_HM;
>>> +		} else {
>>> +			/* F2FS_ZONED_HA */
>>> +			if (c.zoned_mode) {
>>> +				c.zoned_model = F2FS_ZONED_HM;
>>
>> May I ask why we change the model from F2FS_ZONED_HA to F2FS_ZONED_HM?
>>
>> Thanks,
> 
> There is some confusion I think in the code between disk zoned model and f2fs
> zoned mode. Currently, F2FS_ZONED_HA is not tested anywhere to drive the check
> against c.zoned_mode. Before this patch, the code was:
> 
> 	if (dev->zoned_model != F2FS_ZONED_NONE) {
> 		if (dev->zoned_model == F2FS_ZONED_HM)
> 			c.zoned_model = F2FS_ZONED_HM;
> 
> So only HM disks end up setting "c.zoned_model = F2FS_ZONED_HM" which requires
> c.zoned_mode to be set. For an F2FS_ZONED_HA disk, c.zoned_model stays as NONE
> and c.zone_blocks is set, regardless of c.zoned_mode setting. The end result is
> that the HA disk will be used as a regular disk when -m is not specified on mkfs
> command line, and as a zoned disk when -m is specified. However, many areas of
> the code check for c.zoned_model == F2FS_ZONED_HM, and that is always false for
> HA disks.
> 
> The idea here is to allow a cleaner control over HA disk use: either use them as
> regular disks and ignore that they are zoned, or use them in the same manner as
> HM disks, ignoring the fact that HA disks can accept random writes. Anything in
> between these 2 modes does not really make sense.

Thanks for your explanation.

It looks like with current implementation that we treat HA disk as HM one,
is that mean we consider more about performance? since on HM device we only
allow sequential IO, which can make better performance on initial disk.

> 
> Now, rereading the code and explaining all this, I realize that the commit
> message is actually not correct at all and misleading to the intent of the
> patch. I will revisit it and check again this patch. We could actually drop it
> for now and I can resend later.
> 
> In any case, I am sending a v2 for this series.

Thanks, will wait for that. :)

Thanks,

> 
> Thanks for your review !
> 
> 
>>
>>> +			} else {
>>> +				MSG(0, "Info: treating host-aware zoned block "
>>> +				    "device as regular device\n");
>>> +				dev->zoned_model = F2FS_ZONED_NONE;
>>> +			}
>>> +		}
>>> +	}
>>>  
>>> +	if (c.zoned_model == F2FS_ZONED_HM) {
>>>  		if (f2fs_get_zone_blocks(i)) {
>>>  			MSG(0, "\tError: Failed to get number of blocks per zone\n");
>>>  			free(stat_buf);
>>> @@ -1071,6 +1083,7 @@ int get_device_info(int i)
>>>  
>>>  int f2fs_get_device_info(void)
>>>  {
>>> +	bool zoned = false;
>>>  	int i;
>>>  
>>>  	for (i = 0; i < c.ndevs; i++)
>>> @@ -1089,22 +1102,26 @@ int f2fs_get_device_info(void)
>>>  		return -1;
>>>  	}
>>>  
>>> +	/* For zoned devices, the zones sizes must be equal */
>>>  	for (i = 0; i < c.ndevs; i++) {
>>> -		if (c.devices[i].zoned_model != F2FS_ZONED_NONE) {
>>> -			if (c.zone_blocks &&
>>> -				c.zone_blocks != c.devices[i].zone_blocks) {
>>> -				MSG(0, "\tError: not support different zone sizes!!!\n");
>>> -				return -1;
>>> -			}
>>> -			c.zone_blocks = c.devices[i].zone_blocks;
>>> +		if (c.devices[i].zoned_model != F2FS_ZONED_HM)
>>> +			continue;
>>> +
>>> +		zoned = true;
>>> +
>>> +		if (c.zone_blocks &&
>>> +		    c.zone_blocks != c.devices[i].zone_blocks) {
>>> +			MSG(0, "\tError: zones of different size are not supported\n");
>>> +			return -1;
>>>  		}
>>> +		c.zone_blocks = c.devices[i].zone_blocks;
>>>  	}
>>>  
>>> -	/*
>>> -	 * Align sections to the device zone size
>>> -	 * and align F2FS zones to the device zones.
>>> -	 */
>>> -	if (c.zone_blocks) {
>>> +	if (zoned) {
>>> +		/*
>>> +		 * Align sections to the device zone size
>>> +		 * and align F2FS zones to the device zones.
>>> +		 */
>>>  		c.segs_per_sec = c.zone_blocks / DEFAULT_BLOCKS_PER_SEGMENT;
>>>  		c.secs_per_zone = 1;
>>>  	} else {
>>>
>>
> 
> 

Re: [PATCH 3/5] f2fs-tools: Improve zoned model check

[Jaegeuk Kim]

On 03/18, Damien Le Moal wrote:
> Return an error if an unknown zoned model is reported for a device.
> 
> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
> ---
>  include/f2fs_fs.h   |  2 +-
>  lib/libf2fs.c       |  8 ++++++--
>  lib/libf2fs_zoned.c | 20 +++++++++++---------
>  3 files changed, 18 insertions(+), 12 deletions(-)
> 
> diff --git a/include/f2fs_fs.h b/include/f2fs_fs.h
> index e073723..97ad774 100644
> --- a/include/f2fs_fs.h
> +++ b/include/f2fs_fs.h
> @@ -1257,7 +1257,7 @@ blk_zone_cond_str(struct blk_zone *blkz)
>  
>  #endif
>  
> -extern void f2fs_get_zoned_model(int);
> +extern int f2fs_get_zoned_model(int);
>  extern int f2fs_get_zone_blocks(int);
>  extern int f2fs_check_zones(int);
>  extern int f2fs_reset_zones(int);
> diff --git a/lib/libf2fs.c b/lib/libf2fs.c
> index 60b84e0..5ca1bb0 100644
> --- a/lib/libf2fs.c
> +++ b/lib/libf2fs.c
> @@ -925,8 +925,12 @@ int get_device_info(int i)
>  	}
>  
>  #if !defined(WITH_ANDROID) && defined(__linux__)
> -	if (S_ISBLK(stat_buf->st_mode))
> -		f2fs_get_zoned_model(i);
> +	if (S_ISBLK(stat_buf->st_mode)) {
> +		if (f2fs_get_zoned_model(i) < 0) {
> +			free(stat_buf);
> +			return -1;
> +                }
> +	}
>  
>  	if (dev->zoned_model != F2FS_ZONED_NONE) {
>  		if (dev->zoned_model == F2FS_ZONED_HM)
> diff --git a/lib/libf2fs_zoned.c b/lib/libf2fs_zoned.c
> index 6e32f32..1a6c5df 100644
> --- a/lib/libf2fs_zoned.c
> +++ b/lib/libf2fs_zoned.c
> @@ -24,39 +24,41 @@
>  
>  #ifdef HAVE_LINUX_BLKZONED_H
>  
> -void f2fs_get_zoned_model(int i)
> +int f2fs_get_zoned_model(int i)
>  {
>  	struct device_info *dev = c.devices + i;
>  	char str[128];
>  	FILE *file;
>  	int res;
>  
> +	/* Assume regular device */
> +	dev->zoned_model = F2FS_ZONED_NONE;
> +
>  	/* Check that this is a zoned block device */
>  	snprintf(str, sizeof(str),
>  		 "/sys/block/%s/queue/zoned",
>  		 basename(dev->path));
>  	file = fopen(str, "r");
>  	if (!file)
> -		goto not_zoned;
> +		return 0;
>  
>  	memset(str, 0, sizeof(str));
>  	res = fscanf(file, "%s", str);
>  	fclose(file);
>  
>  	if (res != 1)
> -		goto not_zoned;
> +		return 0;
>  
>  	if (strcmp(str, "host-aware") == 0) {
>  		dev->zoned_model = F2FS_ZONED_HA;
> -		return;
> -	}
> -	if (strcmp(str, "host-managed") == 0) {
> +	} else if (strcmp(str, "host-managed") == 0) {
>  		dev->zoned_model = F2FS_ZONED_HM;
> -		return;
> +	} else {

If "sys/block/%s/queue/zoned" reports "none", we can't format the device. :)

> +		MSG(0, "\tError: Unsupported device zoned model\n");
> +		return -1;
>  	}
>  
> -not_zoned:
> -	dev->zoned_model = F2FS_ZONED_NONE;
> +	return 0;
>  }
>  
>  int f2fs_get_zone_blocks(int i)
> -- 
> 2.20.1

Re: [PATCH 3/5] f2fs-tools: Improve zoned model check

[Damien Le Moal]

On 2019/03/27 3:24, Jaegeuk Kim wrote:
> On 03/18, Damien Le Moal wrote:
>> Return an error if an unknown zoned model is reported for a device.
>>
>> Signed-off-by: Damien Le Moal <damien.lemoal@wdc.com>
>> ---
>>  include/f2fs_fs.h   |  2 +-
>>  lib/libf2fs.c       |  8 ++++++--
>>  lib/libf2fs_zoned.c | 20 +++++++++++---------
>>  3 files changed, 18 insertions(+), 12 deletions(-)
>>
>> diff --git a/include/f2fs_fs.h b/include/f2fs_fs.h
>> index e073723..97ad774 100644
>> --- a/include/f2fs_fs.h
>> +++ b/include/f2fs_fs.h
>> @@ -1257,7 +1257,7 @@ blk_zone_cond_str(struct blk_zone *blkz)
>>  
>>  #endif
>>  
>> -extern void f2fs_get_zoned_model(int);
>> +extern int f2fs_get_zoned_model(int);
>>  extern int f2fs_get_zone_blocks(int);
>>  extern int f2fs_check_zones(int);
>>  extern int f2fs_reset_zones(int);
>> diff --git a/lib/libf2fs.c b/lib/libf2fs.c
>> index 60b84e0..5ca1bb0 100644
>> --- a/lib/libf2fs.c
>> +++ b/lib/libf2fs.c
>> @@ -925,8 +925,12 @@ int get_device_info(int i)
>>  	}
>>  
>>  #if !defined(WITH_ANDROID) && defined(__linux__)
>> -	if (S_ISBLK(stat_buf->st_mode))
>> -		f2fs_get_zoned_model(i);
>> +	if (S_ISBLK(stat_buf->st_mode)) {
>> +		if (f2fs_get_zoned_model(i) < 0) {
>> +			free(stat_buf);
>> +			return -1;
>> +                }
>> +	}
>>  
>>  	if (dev->zoned_model != F2FS_ZONED_NONE) {
>>  		if (dev->zoned_model == F2FS_ZONED_HM)
>> diff --git a/lib/libf2fs_zoned.c b/lib/libf2fs_zoned.c
>> index 6e32f32..1a6c5df 100644
>> --- a/lib/libf2fs_zoned.c
>> +++ b/lib/libf2fs_zoned.c
>> @@ -24,39 +24,41 @@
>>  
>>  #ifdef HAVE_LINUX_BLKZONED_H
>>  
>> -void f2fs_get_zoned_model(int i)
>> +int f2fs_get_zoned_model(int i)
>>  {
>>  	struct device_info *dev = c.devices + i;
>>  	char str[128];
>>  	FILE *file;
>>  	int res;
>>  
>> +	/* Assume regular device */
>> +	dev->zoned_model = F2FS_ZONED_NONE;
>> +
>>  	/* Check that this is a zoned block device */
>>  	snprintf(str, sizeof(str),
>>  		 "/sys/block/%s/queue/zoned",
>>  		 basename(dev->path));
>>  	file = fopen(str, "r");
>>  	if (!file)
>> -		goto not_zoned;
>> +		return 0;
>>  
>>  	memset(str, 0, sizeof(str));
>>  	res = fscanf(file, "%s", str);
>>  	fclose(file);
>>  
>>  	if (res != 1)
>> -		goto not_zoned;
>> +		return 0;
>>  
>>  	if (strcmp(str, "host-aware") == 0) {
>>  		dev->zoned_model = F2FS_ZONED_HA;
>> -		return;
>> -	}
>> -	if (strcmp(str, "host-managed") == 0) {
>> +	} else if (strcmp(str, "host-managed") == 0) {
>>  		dev->zoned_model = F2FS_ZONED_HM;
>> -		return;
>> +	} else {
> 
> If "sys/block/%s/queue/zoned" reports "none", we can't format the device. :)

Arg ! Yes ! Will fix that.

> 
>> +		MSG(0, "\tError: Unsupported device zoned model\n");
>> +		return -1;
>>  	}
>>  
>> -not_zoned:
>> -	dev->zoned_model = F2FS_ZONED_NONE;
>> +	return 0;
>>  }
>>  
>>  int f2fs_get_zone_blocks(int i)
>> -- 
>> 2.20.1
> 


-- 
Damien Le Moal
Western Digital Research