[PATCH 0/2] zonefs fixes

[PATCH 0/2] zonefs fixes

[Damien Le Moal]

The first patch fixes a bug with the execution of zone report after an
IO error. The second patch removes unused code (not technically a bug
but this removes a warning when compiling with clang).

Damien Le Moal (2):
  zonefs: fix zone report size in __zonefs_io_error()
  zonefs: Remove to_attr() helper function

 fs/zonefs/super.c | 12 ++++++++++--
 fs/zonefs/sysfs.c |  5 -----
 2 files changed, 10 insertions(+), 7 deletions(-)

-- 
2.38.1

[PATCH 1/2] zonefs: fix zone report size in __zonefs_io_error()

[Damien Le Moal]

When an IO error occurs, the function __zonefs_io_error() is used to
issue a zone report to obtain the latest zone information from the
device. This function gets a zone report for all zones used as storage
for a file, which is always 1 zone except for files representing
aggregated conventional zones.

The number of zones of a zone report for a file is calculated in
__zonefs_io_error() by doing a bit-shift of the inode i_zone_size field,
which is equal to or larger than the device zone size. However, this
calculation does not take into account that the last zone of a zoned
device may be smaller than the zone size reported by bdev_zone_sectors()
(which is used to set the bit shift size). As a result, if an error
occurs for an IO targetting such last smaller zone, the zone report will
ask for 0 zones, leading to an invalid zone report.

Fix this by using the fact that all files require a 1 zone report,
except if the inode i_zone_size field indicates a zone size larger than
the device zone size. This exception case corresponds to aggregated
conventional zone files.

Fixes: e3c3155bc95a ("zonefs: add zone-capacity support")
Cc: <stable@vger.kernel.org>
Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 fs/zonefs/super.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/fs/zonefs/super.c b/fs/zonefs/super.c
index 860f0b1032c6..f689bd3596cf 100644
--- a/fs/zonefs/super.c
+++ b/fs/zonefs/super.c
@@ -478,14 +478,22 @@ static void __zonefs_io_error(struct inode *inode, bool write)
 	struct super_block *sb = inode->i_sb;
 	struct zonefs_sb_info *sbi = ZONEFS_SB(sb);
 	unsigned int noio_flag;
-	unsigned int nr_zones =
-		zi->i_zone_size >> (sbi->s_zone_sectors_shift + SECTOR_SHIFT);
+	unsigned int nr_zones = 1;
 	struct zonefs_ioerr_data err = {
 		.inode = inode,
 		.write = write,
 	};
 	int ret;
 
+	/*
+	 * The only files that have more than one zone are conventional zone
+	 * files with aggregated conventional zones, for which the inode zone
+	 * size is always larger than the device zone size.
+	 */
+	if (zi->i_zone_size > bdev_zone_sectors(sb->s_bdev))
+		nr_zones = zi->i_zone_size >>
+			(sbi->s_zone_sectors_shift + SECTOR_SHIFT);
+
 	/*
 	 * Memory allocations in blkdev_report_zones() can trigger a memory
 	 * reclaim which may in turn cause a recursion into zonefs as well as
-- 
2.38.1

Re: [PATCH 1/2] zonefs: fix zone report size in __zonefs_io_error()

[Johannes Thumshirn]

On 31.10.22 04:00, Damien Le Moal wrote:
> +	/*
> +	 * The only files that have more than one zone are conventional zone
> +	 * files with aggregated conventional zones, for which the inode zone
> +	 * size is always larger than the device zone size.
> +	 */
> +	if (zi->i_zone_size > bdev_zone_sectors(sb->s_bdev))
> +		nr_zones = zi->i_zone_size >>
> +			(sbi->s_zone_sectors_shift + SECTOR_SHIFT);
> +

I wonder if we should also have a check/assertion like this somewhere: 
WARN_ON_ONCE(zi->i_zone_size > bdev_zone_sectors(sb->sbdev) && 
	!sbi->s_features & ZONEFS_F_AGGRCNV)
	

Re: [PATCH 1/2] zonefs: fix zone report size in __zonefs_io_error()

[Damien Le Moal]

On 11/2/22 18:28, Johannes Thumshirn wrote:
> On 31.10.22 04:00, Damien Le Moal wrote:
>> +	/*
>> +	 * The only files that have more than one zone are conventional zone
>> +	 * files with aggregated conventional zones, for which the inode zone
>> +	 * size is always larger than the device zone size.
>> +	 */
>> +	if (zi->i_zone_size > bdev_zone_sectors(sb->s_bdev))
>> +		nr_zones = zi->i_zone_size >>
>> +			(sbi->s_zone_sectors_shift + SECTOR_SHIFT);
>> +
> 
> I wonder if we should also have a check/assertion like this somewhere: 
> WARN_ON_ONCE(zi->i_zone_size > bdev_zone_sectors(sb->sbdev) && 
> 	!sbi->s_features & ZONEFS_F_AGGRCNV)

Well, this is set when the inode is created on mount. So we could add the
check there, but I do not really see the point since we would be checking
exactly what we are doing. So the only chance warn ever showing would be
memory corruption, but then we'll likely have bigger problems anyway. No ?

> 	

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/2] zonefs: fix zone report size in __zonefs_io_error()

[Johannes Thumshirn]

On 02.11.22 10:44, Damien Le Moal wrote:
> On 11/2/22 18:28, Johannes Thumshirn wrote:
>> On 31.10.22 04:00, Damien Le Moal wrote:
>>> +	/*
>>> +	 * The only files that have more than one zone are conventional zone
>>> +	 * files with aggregated conventional zones, for which the inode zone
>>> +	 * size is always larger than the device zone size.
>>> +	 */
>>> +	if (zi->i_zone_size > bdev_zone_sectors(sb->s_bdev))
>>> +		nr_zones = zi->i_zone_size >>
>>> +			(sbi->s_zone_sectors_shift + SECTOR_SHIFT);
>>> +
>>
>> I wonder if we should also have a check/assertion like this somewhere: 
>> WARN_ON_ONCE(zi->i_zone_size > bdev_zone_sectors(sb->sbdev) && 
>> 	!sbi->s_features & ZONEFS_F_AGGRCNV)
> 
> Well, this is set when the inode is created on mount. So we could add the
> check there, but I do not really see the point since we would be checking
> exactly what we are doing. So the only chance warn ever showing would be
> memory corruption, but then we'll likely have bigger problems anyway. No ?

Something like this:

From f90acf1ca3f84d37a3bdb570abf89e186697c0d4 Mon Sep 17 00:00:00 2001
Message-Id: <f90acf1ca3f84d37a3bdb570abf89e186697c0d4.1667383842.git.johannes.thumshirn@wdc.com>
From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
Date: Wed, 2 Nov 2022 02:57:35 -0700
Subject: [PATCH] zonefs: add sanity check for aggregated conventional zones

When initializing a file inode, check if the zone's size if bigger than
the number of device zone sectors. This can only be the case if we mount
the filesystem with the -oaggr_cnv mount option.

Emit a warning if this case happens and we do not have the mount option
set. Also if the -oerror=read-only mount option is set, mark the
filesystem as read-only.

Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
---
 fs/zonefs/super.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/fs/zonefs/super.c b/fs/zonefs/super.c
index 860f0b1032c6..7c0b776a7bc4 100644
--- a/fs/zonefs/super.c
+++ b/fs/zonefs/super.c
@@ -1407,6 +1407,15 @@ static int zonefs_init_file_inode(struct inode *inode, struct blk_zone *zone,
        zi->i_ztype = type;
        zi->i_zsector = zone->start;
        zi->i_zone_size = zone->len << SECTOR_SHIFT;
+       if (WARN_ON(zi->i_zone_size > bdev_zone_sectors(sb->s_bdev) &&
+                   !sbi->s_features & ZONEFS_F_AGGRCNV)) {
+               if ((sbi->s_mount_opts & ZONEFS_MNTOPT_ERRORS_RO) &&
+                   !sb_rdonly(sb)) {
+                       zonefs_warn(sb, "remounting filesystem read-only\n");
+                       sb->s_flags |= SB_RDONLY;
+               }
+               return -EINVAL;
+       }
 
        zi->i_max_size = min_t(loff_t, MAX_LFS_FILESIZE,
                               zone->capacity << SECTOR_SHIFT);
-- 
2.37.3

Re: [PATCH 1/2] zonefs: fix zone report size in __zonefs_io_error()

[Damien Le Moal]

On 11/2/22 19:11, Johannes Thumshirn wrote:
> On 02.11.22 10:44, Damien Le Moal wrote:
>> On 11/2/22 18:28, Johannes Thumshirn wrote:
>>> On 31.10.22 04:00, Damien Le Moal wrote:
>>>> +	/*
>>>> +	 * The only files that have more than one zone are conventional zone
>>>> +	 * files with aggregated conventional zones, for which the inode zone
>>>> +	 * size is always larger than the device zone size.
>>>> +	 */
>>>> +	if (zi->i_zone_size > bdev_zone_sectors(sb->s_bdev))
>>>> +		nr_zones = zi->i_zone_size >>
>>>> +			(sbi->s_zone_sectors_shift + SECTOR_SHIFT);
>>>> +
>>>
>>> I wonder if we should also have a check/assertion like this somewhere: 
>>> WARN_ON_ONCE(zi->i_zone_size > bdev_zone_sectors(sb->sbdev) && 
>>> 	!sbi->s_features & ZONEFS_F_AGGRCNV)
>>
>> Well, this is set when the inode is created on mount. So we could add the
>> check there, but I do not really see the point since we would be checking
>> exactly what we are doing. So the only chance warn ever showing would be
>> memory corruption, but then we'll likely have bigger problems anyway. No ?
> 
> Something like this:
> 
> From f90acf1ca3f84d37a3bdb570abf89e186697c0d4 Mon Sep 17 00:00:00 2001
> Message-Id: <f90acf1ca3f84d37a3bdb570abf89e186697c0d4.1667383842.git.johannes.thumshirn@wdc.com>
> From: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> Date: Wed, 2 Nov 2022 02:57:35 -0700
> Subject: [PATCH] zonefs: add sanity check for aggregated conventional zones
> 
> When initializing a file inode, check if the zone's size if bigger than
> the number of device zone sectors. This can only be the case if we mount
> the filesystem with the -oaggr_cnv mount option.
> 
> Emit a warning if this case happens and we do not have the mount option
> set. Also if the -oerror=read-only mount option is set, mark the
> filesystem as read-only.
> 
> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
> ---
>  fs/zonefs/super.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/fs/zonefs/super.c b/fs/zonefs/super.c
> index 860f0b1032c6..7c0b776a7bc4 100644
> --- a/fs/zonefs/super.c
> +++ b/fs/zonefs/super.c
> @@ -1407,6 +1407,15 @@ static int zonefs_init_file_inode(struct inode *inode, struct blk_zone *zone,
>         zi->i_ztype = type;
>         zi->i_zsector = zone->start;
>         zi->i_zone_size = zone->len << SECTOR_SHIFT;
> +       if (WARN_ON(zi->i_zone_size > bdev_zone_sectors(sb->s_bdev) &&
> +                   !sbi->s_features & ZONEFS_F_AGGRCNV)) {
> +               if ((sbi->s_mount_opts & ZONEFS_MNTOPT_ERRORS_RO) &&
> +                   !sb_rdonly(sb)) {
> +                       zonefs_warn(sb, "remounting filesystem read-only\n");
> +                       sb->s_flags |= SB_RDONLY;

This is during mount. So let's fail the mount...

> +               }
> +               return -EINVAL;
> +       }
>  
>         zi->i_max_size = min_t(loff_t, MAX_LFS_FILESIZE,
>                                zone->capacity << SECTOR_SHIFT);

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH 1/2] zonefs: fix zone report size in __zonefs_io_error()

[Damien Le Moal]

On 11/2/22 18:28, Johannes Thumshirn wrote:
> On 31.10.22 04:00, Damien Le Moal wrote:
>> +	/*
>> +	 * The only files that have more than one zone are conventional zone
>> +	 * files with aggregated conventional zones, for which the inode zone
>> +	 * size is always larger than the device zone size.
>> +	 */
>> +	if (zi->i_zone_size > bdev_zone_sectors(sb->s_bdev))
>> +		nr_zones = zi->i_zone_size >>
>> +			(sbi->s_zone_sectors_shift + SECTOR_SHIFT);
>> +
> 
> I wonder if we should also have a check/assertion like this somewhere: 
> WARN_ON_ONCE(zi->i_zone_size > bdev_zone_sectors(sb->sbdev) && 
> 	!sbi->s_features & ZONEFS_F_AGGRCNV)
> 	

I think It would be good to squash your patch checking zi->i_zone_size on
mount with this one. Can you send that or do you want me to do it ?

-- 
Damien Le Moal
Western Digital Research

[PATCH 2/2] zonefs: Remove to_attr() helper function

[Damien Le Moal]

to_attr() in zonefs sysfs code is unused. Remove this function code.

Signed-off-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 fs/zonefs/sysfs.c | 5 -----
 1 file changed, 5 deletions(-)

diff --git a/fs/zonefs/sysfs.c b/fs/zonefs/sysfs.c
index 9cb6755ce39a..9920689dc098 100644
--- a/fs/zonefs/sysfs.c
+++ b/fs/zonefs/sysfs.c
@@ -15,11 +15,6 @@ struct zonefs_sysfs_attr {
 	ssize_t (*show)(struct zonefs_sb_info *sbi, char *buf);
 };
 
-static inline struct zonefs_sysfs_attr *to_attr(struct attribute *attr)
-{
-	return container_of(attr, struct zonefs_sysfs_attr, attr);
-}
-
 #define ZONEFS_SYSFS_ATTR_RO(name) \
 static struct zonefs_sysfs_attr zonefs_sysfs_attr_##name = __ATTR_RO(name)
 
-- 
2.38.1

Re: [PATCH 2/2] zonefs: Remove to_attr() helper function

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>