[f2fs-dev] [PATCH v2] libf2fs: don't allow mkfs / fsck on non power-of-2 zoned devices

[f2fs-dev] [PATCH v2] libf2fs: don't allow mkfs / fsck on non power-of-2 zoned devices

[Pankaj Raghav]

From: Luis Chamberlain <mcgrof@kernel.org>

f2fs currently only work with zoned storage devices with a zone
size which is a power of 2 (PO2). So check if a non-power of 2
zoned device is found, and if so disallow its use. This prevents
users from incorrectly using these devices.

This is a non-issue today given today's kernel does not allow NPO2
zoned devices to exist as a block device. But NPO2 zoned devices do exist
so proactively put a stop-gap measure in place to prevent it from being
incorrectly used.

Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
Signed-off-by: Pankaj Raghav <p.raghav@samsung.com>
---
Changes since v1:
- Squash the commits for clarity (Damien)
- f2fs_get_zone_chunk_sectors can return uint32_t (Damien)
- Include the units for zone size in msg info (Damien)
- Sections can be npo2 but it should only be a multiple of 2MB (Jaegeuk)

 include/f2fs_fs.h   |  1 +
 lib/libf2fs.c       | 17 +++++++++++++++--
 lib/libf2fs_zoned.c | 34 ++++++++++++++++++++++------------
 3 files changed, 38 insertions(+), 14 deletions(-)

diff --git a/include/f2fs_fs.h b/include/f2fs_fs.h
index d236437..83c5b33 100644
--- a/include/f2fs_fs.h
+++ b/include/f2fs_fs.h
@@ -386,6 +386,7 @@ struct device_info {
 	u_int32_t nr_zones;
 	u_int32_t nr_rnd_zones;
 	size_t zone_blocks;
+	uint64_t zone_size;
 	size_t *zone_cap_blocks;
 };
 
diff --git a/lib/libf2fs.c b/lib/libf2fs.c
index 420dfda..8104667 100644
--- a/lib/libf2fs.c
+++ b/lib/libf2fs.c
@@ -882,6 +882,11 @@ static int open_check_fs(char *path, int flag)
 	return open(path, O_RDONLY | flag);
 }
 
+static int is_power_of_2(unsigned long n)
+{
+	return (n != 0 && ((n & (n - 1)) == 0));
+}
+
 int get_device_info(int i)
 {
 	int32_t fd = 0;
@@ -1043,6 +1048,13 @@ int get_device_info(int i)
 			return -1;
 		}
 
+		if (!is_power_of_2(dev->zone_size)) {
+			MSG(0, "\tError: zoned: illegal zone size %lu (not a power of 2)\n",
+					dev->zone_size);
+			free(stat_buf);
+			return -1;
+		}
+
 		/*
 		 * Check zone configuration: for the first disk of a
 		 * multi-device volume, conventional zones are needed.
@@ -1055,8 +1067,9 @@ int get_device_info(int i)
 		MSG(0, "Info: Host-%s zoned block device:\n",
 				(dev->zoned_model == F2FS_ZONED_HA) ?
 					"aware" : "managed");
-		MSG(0, "      %u zones, %u randomly writeable zones\n",
-				dev->nr_zones, dev->nr_rnd_zones);
+		MSG(0, "      %u zones, %lu zone size(bytes), %u randomly writeable zones\n",
+				dev->nr_zones, dev->zone_size,
+				dev->nr_rnd_zones);
 		MSG(0, "      %lu blocks per zone\n",
 				dev->zone_blocks);
 	}
diff --git a/lib/libf2fs_zoned.c b/lib/libf2fs_zoned.c
index ce73b9a..48a23c0 100644
--- a/lib/libf2fs_zoned.c
+++ b/lib/libf2fs_zoned.c
@@ -146,40 +146,50 @@ int f2fs_get_zoned_model(int i)
 	return 0;
 }
 
-int f2fs_get_zone_blocks(int i)
+uint32_t f2fs_get_zone_chunk_sectors(struct device_info *dev)
 {
-	struct device_info *dev = c.devices + i;
-	uint64_t sectors;
+	uint32_t sectors;
 	char str[PATH_MAX];
 	FILE *file;
 	int res;
 
-	/* Get zone size */
-	dev->zone_blocks = 0;
-
 	res = get_sysfs_path(dev, "queue/chunk_sectors", str, sizeof(str));
 	if (res != 0) {
 		MSG(0, "\tError: Failed to get device sysfs attribute path\n");
-		return -1;
+		return 0;
 	}
 
 	file = fopen(str, "r");
 	if (!file)
-		return -1;
+		return 0;
 
 	memset(str, 0, sizeof(str));
 	res = fscanf(file, "%s", str);
 	fclose(file);
 
 	if (res != 1)
-		return -1;
+		return 0;
 
-	sectors = atol(str);
+	sectors = atoi(str);
+
+	return sectors;
+}
+
+int f2fs_get_zone_blocks(int i)
+{
+	struct device_info *dev = c.devices + i;
+	uint64_t sectors;
+
+	/* Get zone size */
+	dev->zone_blocks = 0;
+
+	sectors = f2fs_get_zone_chunk_sectors(dev);
 	if (!sectors)
 		return -1;
 
-	dev->zone_blocks = sectors >> (F2FS_BLKSIZE_BITS - 9);
-	sectors = (sectors << 9) / c.sector_size;
+	dev->zone_size = sectors << SECTOR_SHIFT;
+	dev->zone_blocks = sectors >> (F2FS_BLKSIZE_BITS - SECTOR_SHIFT);
+	sectors = dev->zone_size / c.sector_size;
 
 	/*
 	 * Total number of zones: there may
-- 
2.25.1



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

Re: [f2fs-dev] [PATCH v2] libf2fs: don't allow mkfs / fsck on non power-of-2 zoned devices

[Jaegeuk Kim]

On 04/13, Pankaj Raghav wrote:
> From: Luis Chamberlain <mcgrof@kernel.org>
> 
> f2fs currently only work with zoned storage devices with a zone
> size which is a power of 2 (PO2). So check if a non-power of 2
> zoned device is found, and if so disallow its use. This prevents
> users from incorrectly using these devices.
> 
> This is a non-issue today given today's kernel does not allow NPO2
> zoned devices to exist as a block device. But NPO2 zoned devices do exist
> so proactively put a stop-gap measure in place to prevent it from being
> incorrectly used.
> 
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> Signed-off-by: Pankaj Raghav <p.raghav@samsung.com>
> ---
> Changes since v1:
> - Squash the commits for clarity (Damien)
> - f2fs_get_zone_chunk_sectors can return uint32_t (Damien)
> - Include the units for zone size in msg info (Damien)
> - Sections can be npo2 but it should only be a multiple of 2MB (Jaegeuk)
> 
>  include/f2fs_fs.h   |  1 +
>  lib/libf2fs.c       | 17 +++++++++++++++--
>  lib/libf2fs_zoned.c | 34 ++++++++++++++++++++++------------
>  3 files changed, 38 insertions(+), 14 deletions(-)
> 
> diff --git a/include/f2fs_fs.h b/include/f2fs_fs.h
> index d236437..83c5b33 100644
> --- a/include/f2fs_fs.h
> +++ b/include/f2fs_fs.h
> @@ -386,6 +386,7 @@ struct device_info {
>  	u_int32_t nr_zones;
>  	u_int32_t nr_rnd_zones;
>  	size_t zone_blocks;
> +	uint64_t zone_size;
>  	size_t *zone_cap_blocks;
>  };
>  
> diff --git a/lib/libf2fs.c b/lib/libf2fs.c
> index 420dfda..8104667 100644
> --- a/lib/libf2fs.c
> +++ b/lib/libf2fs.c
> @@ -882,6 +882,11 @@ static int open_check_fs(char *path, int flag)
>  	return open(path, O_RDONLY | flag);
>  }
>  
> +static int is_power_of_2(unsigned long n)

So, this needs to check 2MB alignment only?

> +{
> +	return (n != 0 && ((n & (n - 1)) == 0));
> +}
> +
>  int get_device_info(int i)
>  {
>  	int32_t fd = 0;
> @@ -1043,6 +1048,13 @@ int get_device_info(int i)
>  			return -1;
>  		}
>  
> +		if (!is_power_of_2(dev->zone_size)) {
> +			MSG(0, "\tError: zoned: illegal zone size %lu (not a power of 2)\n",
> +					dev->zone_size);
> +			free(stat_buf);
> +			return -1;
> +		}
> +
>  		/*
>  		 * Check zone configuration: for the first disk of a
>  		 * multi-device volume, conventional zones are needed.
> @@ -1055,8 +1067,9 @@ int get_device_info(int i)
>  		MSG(0, "Info: Host-%s zoned block device:\n",
>  				(dev->zoned_model == F2FS_ZONED_HA) ?
>  					"aware" : "managed");
> -		MSG(0, "      %u zones, %u randomly writeable zones\n",
> -				dev->nr_zones, dev->nr_rnd_zones);
> +		MSG(0, "      %u zones, %lu zone size(bytes), %u randomly writeable zones\n",
> +				dev->nr_zones, dev->zone_size,
> +				dev->nr_rnd_zones);
>  		MSG(0, "      %lu blocks per zone\n",
>  				dev->zone_blocks);
>  	}
> diff --git a/lib/libf2fs_zoned.c b/lib/libf2fs_zoned.c
> index ce73b9a..48a23c0 100644
> --- a/lib/libf2fs_zoned.c
> +++ b/lib/libf2fs_zoned.c
> @@ -146,40 +146,50 @@ int f2fs_get_zoned_model(int i)
>  	return 0;
>  }
>  
> -int f2fs_get_zone_blocks(int i)
> +uint32_t f2fs_get_zone_chunk_sectors(struct device_info *dev)
>  {
> -	struct device_info *dev = c.devices + i;
> -	uint64_t sectors;
> +	uint32_t sectors;
>  	char str[PATH_MAX];
>  	FILE *file;
>  	int res;
>  
> -	/* Get zone size */
> -	dev->zone_blocks = 0;
> -
>  	res = get_sysfs_path(dev, "queue/chunk_sectors", str, sizeof(str));
>  	if (res != 0) {
>  		MSG(0, "\tError: Failed to get device sysfs attribute path\n");
> -		return -1;
> +		return 0;
>  	}
>  
>  	file = fopen(str, "r");
>  	if (!file)
> -		return -1;
> +		return 0;
>  
>  	memset(str, 0, sizeof(str));
>  	res = fscanf(file, "%s", str);
>  	fclose(file);
>  
>  	if (res != 1)
> -		return -1;
> +		return 0;
>  
> -	sectors = atol(str);
> +	sectors = atoi(str);
> +
> +	return sectors;
> +}
> +
> +int f2fs_get_zone_blocks(int i)
> +{
> +	struct device_info *dev = c.devices + i;
> +	uint64_t sectors;
> +
> +	/* Get zone size */
> +	dev->zone_blocks = 0;
> +
> +	sectors = f2fs_get_zone_chunk_sectors(dev);
>  	if (!sectors)
>  		return -1;
>  
> -	dev->zone_blocks = sectors >> (F2FS_BLKSIZE_BITS - 9);
> -	sectors = (sectors << 9) / c.sector_size;
> +	dev->zone_size = sectors << SECTOR_SHIFT;
> +	dev->zone_blocks = sectors >> (F2FS_BLKSIZE_BITS - SECTOR_SHIFT);
> +	sectors = dev->zone_size / c.sector_size;
>  
>  	/*
>  	 * Total number of zones: there may
> -- 
> 2.25.1
> 
> 
> 
> _______________________________________________
> Linux-f2fs-devel mailing list
> Linux-f2fs-devel@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel


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

Re: [f2fs-dev] [PATCH v2] libf2fs: don't allow mkfs / fsck on non power-of-2 zoned devices

[Pankaj Raghav]

Hi Jaegeuk,

On 2022-04-13 19:03, Jaegeuk Kim wrote
>> @@ -882,6 +882,11 @@ static int open_check_fs(char *path, int flag)
>>  	return open(path, O_RDONLY | flag);
>>  }
>>  
>> +static int is_power_of_2(unsigned long n)
> 
> So, this needs to check 2MB alignment only?
> 
As I explained in the v1 thread, zoned support for f2fs assumes po2 zone
sizes. For e.g.,
static int __f2fs_issue_discard_zone(struct f2fs_sb_info *sbi,
		struct block_device *bdev, block_t blkstart, block_t blklen)
{
	sector_t sector, nr_sects;
	block_t lblkstart = blkstart;
	int devi = 0;
...
...
		// Assumes zone sectors to be po2
		if (sector & (bdev_zone_sectors(bdev) - 1) ||
				nr_sects != bdev_zone_sectors(bdev)) {
			f2fs_err(sbi, "(%d) %s: Unaligned zone reset attempted (block %x + %x)",
				 devi, sbi->s_ndevs ? FDEV(devi).path : "",
				 blkstart, blklen);
			return -EIO;
		}

...
}

So until non power of 2 zoned devices are supported in block layer and
f2fs, it is safer to reject non power of 2 devices during mkfs time. I
hope it clarifies.


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

Re: [f2fs-dev] [PATCH v2] libf2fs: don't allow mkfs / fsck on non power-of-2 zoned devices

[Jaegeuk Kim]

On 04/13, Pankaj Raghav wrote:
> Hi Jaegeuk,
> 
> On 2022-04-13 19:03, Jaegeuk Kim wrote
> >> @@ -882,6 +882,11 @@ static int open_check_fs(char *path, int flag)
> >>  	return open(path, O_RDONLY | flag);
> >>  }
> >>  
> >> +static int is_power_of_2(unsigned long n)
> > 
> > So, this needs to check 2MB alignment only?
> > 
> As I explained in the v1 thread, zoned support for f2fs assumes po2 zone
> sizes. For e.g.,
> static int __f2fs_issue_discard_zone(struct f2fs_sb_info *sbi,
> 		struct block_device *bdev, block_t blkstart, block_t blklen)
> {
> 	sector_t sector, nr_sects;
> 	block_t lblkstart = blkstart;
> 	int devi = 0;
> ...
> ...
> 		// Assumes zone sectors to be po2

Well, I think this will be aligned to 2MB, if the device gives NPO2? IOWs, this
is just checking the granularity, not PO2.

> 		if (sector & (bdev_zone_sectors(bdev) - 1) ||
> 				nr_sects != bdev_zone_sectors(bdev)) {
> 			f2fs_err(sbi, "(%d) %s: Unaligned zone reset attempted (block %x + %x)",
> 				 devi, sbi->s_ndevs ? FDEV(devi).path : "",
> 				 blkstart, blklen);
> 			return -EIO;
> 		}
> 
> ...
> }
> 
> So until non power of 2 zoned devices are supported in block layer and
> f2fs, it is safer to reject non power of 2 devices during mkfs time. I
> hope it clarifies.


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

Re: [f2fs-dev] [PATCH v2] libf2fs: don't allow mkfs / fsck on non power-of-2 zoned devices

[Pankaj Raghav]

On 2022-04-13 21:45, Jaegeuk Kim wrote:>> As I explained in the v1
thread, zoned support for f2fs assumes po2 zone
>> sizes. For e.g.,
>> static int __f2fs_issue_discard_zone(struct f2fs_sb_info *sbi,
>> 		struct block_device *bdev, block_t blkstart, block_t blklen)
>> {
>> 	sector_t sector, nr_sects;
>> 	block_t lblkstart = blkstart;
>> 	int devi = 0;
>> ...
>> ...
>> 		// Assumes zone sectors to be po2
s/Assumes zone sectors to be po2/Works correctly only if the base
alignment is a power of 2 value./

Taken from align.h:
/* @a is a power of 2 value */
...
#define IS_ALIGNED(x, a)    (((x) & ((typeof(x))(a) - 1)) == 0)

> 
> Well, I think this will be aligned to 2MB, if the device gives NPO2? IOWs, this
Could you elaborate what you mean by `this will be aligned to 2MB`?
> is just checking the granularity, not PO2.
Yeah, so whenever we send a discard or a zone reset, the block zoned
device expects the `sector` to be the start of a device zone. The check
below essentially checks if the `sector` aligns with the `zone size
sector` of the device. But to check for alignment using this `sector &
(bdev_zone_sectors(bdev) - 1)` will only work if
`bdev_zone_sectors(bdev)` is a power of 2.

Even if the device zone size is a multiple of 2MB, these checks will
fail for a non power of 2 zone size device. The solution is simple, it
is to make this alignment check generic but until this is done in the
kernel, mkfs.f2fs should fail to mount for a non power of 2 device.
> 
>> 		if (sector & (bdev_zone_sectors(bdev) - 1) ||
>> 				nr_sects != bdev_zone_sectors(bdev)) {
>> 			f2fs_err(sbi, "(%d) %s: Unaligned zone reset attempted (block %x + %x)",
>> 				 devi, sbi->s_ndevs ? FDEV(devi).path : "",
>> 				 blkstart, blklen);
I tried it locally in QEMU along with my local changes to remove power
of 2 constraint in the block layer and zns device, and indeed the
`Unaligned zone reset attempted` error popped up for a zns drive with a
zone size 96MB.

When I changed the condition from `sector & (bdev_zone_sectors(bdev) -
1)` to `sector % bdev_zone_sectors(bdev)`, the error went away because
the calculation doesn't depend on the alignment base to be a power of 2.


TL;DR until we change some calculations in f2fs for zoned device to be
generic, we need to bail out during mkfs time for non power of 2 zone
size devices.


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

Re: [f2fs-dev] [PATCH v2] libf2fs: don't allow mkfs / fsck on non power-of-2 zoned devices

[Jaegeuk Kim]

On 04/19, Pankaj Raghav wrote:
> On 2022-04-13 21:45, Jaegeuk Kim wrote:>> As I explained in the v1
> thread, zoned support for f2fs assumes po2 zone
> >> sizes. For e.g.,
> >> static int __f2fs_issue_discard_zone(struct f2fs_sb_info *sbi,
> >> 		struct block_device *bdev, block_t blkstart, block_t blklen)
> >> {
> >> 	sector_t sector, nr_sects;
> >> 	block_t lblkstart = blkstart;
> >> 	int devi = 0;
> >> ...
> >> ...
> >> 		// Assumes zone sectors to be po2
> s/Assumes zone sectors to be po2/Works correctly only if the base
> alignment is a power of 2 value./
> 
> Taken from align.h:
> /* @a is a power of 2 value */
> ...
> #define IS_ALIGNED(x, a)    (((x) & ((typeof(x))(a) - 1)) == 0)
> 
> > 
> > Well, I think this will be aligned to 2MB, if the device gives NPO2? IOWs, this
> Could you elaborate what you mean by `this will be aligned to 2MB`?
> > is just checking the granularity, not PO2.
> Yeah, so whenever we send a discard or a zone reset, the block zoned
> device expects the `sector` to be the start of a device zone. The check
> below essentially checks if the `sector` aligns with the `zone size
> sector` of the device. But to check for alignment using this `sector &
> (bdev_zone_sectors(bdev) - 1)` will only work if
> `bdev_zone_sectors(bdev)` is a power of 2.
> 
> Even if the device zone size is a multiple of 2MB, these checks will
> fail for a non power of 2 zone size device. The solution is simple, it
> is to make this alignment check generic but until this is done in the
> kernel, mkfs.f2fs should fail to mount for a non power of 2 device.
> > 
> >> 		if (sector & (bdev_zone_sectors(bdev) - 1) ||
> >> 				nr_sects != bdev_zone_sectors(bdev)) {
> >> 			f2fs_err(sbi, "(%d) %s: Unaligned zone reset attempted (block %x + %x)",
> >> 				 devi, sbi->s_ndevs ? FDEV(devi).path : "",
> >> 				 blkstart, blklen);
> I tried it locally in QEMU along with my local changes to remove power
> of 2 constraint in the block layer and zns device, and indeed the
> `Unaligned zone reset attempted` error popped up for a zns drive with a
> zone size 96MB.
> 
> When I changed the condition from `sector & (bdev_zone_sectors(bdev) -
> 1)` to `sector % bdev_zone_sectors(bdev)`, the error went away because
> the calculation doesn't depend on the alignment base to be a power of 2.
> 
> 
> TL;DR until we change some calculations in f2fs for zoned device to be
> generic, we need to bail out during mkfs time for non power of 2 zone
> size devices.

Ah, I see. Yeah, that in f2fs is actually PO2. Let me merge this patch to
force it before getting that relax. Thanks,



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

Re: [f2fs-dev] [PATCH v2] libf2fs: don't allow mkfs / fsck on non power-of-2 zoned devices

[Pankaj Raghav]

On 2022-04-20 01:07, Jaegeuk Kim wrote:>> TL;DR until we change some
calculations in f2fs for zoned device to be
>> generic, we need to bail out during mkfs time for non power of 2 zone
>> size devices.
> 
> Ah, I see. Yeah, that in f2fs is actually PO2. Let me merge this patch to
> force it before getting that relax. Thanks,
> 
Thanks Jaegeuk. We are working on relaxing the po2 constraint for device
zone sizes for zoned devices in the kernel and we will be sending the
patches soon. I am not sure if we will relax these constraints in f2fs
during the first round of patches but we will eventually get there. When
that happens, we can relax these constraints also in f2fs-tools :).


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

Re: [f2fs-dev] [PATCH v2] libf2fs: don't allow mkfs / fsck on non power-of-2 zoned devices

[Pankaj Raghav]

On 2022-04-20 01:07, Jaegeuk Kim wrote:

>> TL;DR until we change some
calculations in f2fs for zoned device to be
>> generic, we need to bail out during mkfs time for non power of 2 zone
>> size devices.
>
> Ah, I see. Yeah, that in f2fs is actually PO2. Let me merge this patch to
> force it before getting that relax. Thanks,
>
Thanks Jaegeuk. We are working on relaxing the po2 constraint for device
zone sizes for zoned devices in the kernel and we will be sending the
patches soon. I am not sure if we will relax these constraints in f2fs
during the first round of patches but we will eventually get there. When
that happens, we can relax these constraints also in f2fs-tools :).


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

Re: [f2fs-dev] [PATCH v2] libf2fs: don't allow mkfs / fsck on non power-of-2 zoned devices

[Chao Yu]

On 2022/4/13 20:29, Pankaj Raghav wrote:
> From: Luis Chamberlain <mcgrof@kernel.org>
> 
> f2fs currently only work with zoned storage devices with a zone
> size which is a power of 2 (PO2). So check if a non-power of 2
> zoned device is found, and if so disallow its use. This prevents
> users from incorrectly using these devices.
> 
> This is a non-issue today given today's kernel does not allow NPO2
> zoned devices to exist as a block device. But NPO2 zoned devices do exist
> so proactively put a stop-gap measure in place to prevent it from being
> incorrectly used.
> 
> Signed-off-by: Luis Chamberlain <mcgrof@kernel.org>
> Signed-off-by: Pankaj Raghav <p.raghav@samsung.com>

Reviewed-by: Chao Yu <chao@kernel.org>

Thanks,


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