Re: [PATCH] zonefs: add sanity check for aggregated conventional zones

[Damien Le Moal <damien.lemoal@opensource.wdc.com>]

On 11/4/22 18:47, Johannes Thumshirn wrote:
> On 04.11.22 01:26, Damien Le Moal wrote:
>> On 11/3/22 19:32, Johannes Thumshirn wrote:
>>> 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 an error in case this case happens and fail the mount.
>>>
>>> Signed-off-by: Johannes Thumshirn <johannes.thumshirn@wdc.com>
>>> ---
>>>  fs/zonefs/super.c | 27 +++++++++++++++++++++------
>>>  1 file changed, 21 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/fs/zonefs/super.c b/fs/zonefs/super.c
>>> index 860f0b1032c6..605364638720 100644
>>> --- a/fs/zonefs/super.c
>>> +++ b/fs/zonefs/super.c
>>> @@ -1407,6 +1407,14 @@ 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 (zi->i_zone_size > bdev_zone_sectors(sb->s_bdev) << SECTOR_SHIFT &&
>>> +	    !sbi->s_features & ZONEFS_F_AGGRCNV) {
>>> +		zonefs_err(sb,
>>> +			   "zone size %llu doesn't match device's zone sectors %llu\n",
>>> +			   zi->i_zone_size,
>>> +			   bdev_zone_sectors(sb->s_bdev) << SECTOR_SHIFT);
>>> +		return -EINVAL;
>>> +	}
>>>  
>>>  	zi->i_max_size = min_t(loff_t, MAX_LFS_FILESIZE,
>>>  			       zone->capacity << SECTOR_SHIFT);
>>> @@ -1485,7 +1493,7 @@ static struct dentry *zonefs_create_inode(struct dentry *parent,
>>>  dput:
>>>  	dput(dentry);
>>>  
>>> -	return NULL;
>>> +	return ERR_PTR(ret);
>>>  }
>>>  
>>>  struct zonefs_zone_data {
>>> @@ -1505,7 +1513,7 @@ static int zonefs_create_zgroup(struct zonefs_zone_data *zd,
>>>  	struct blk_zone *zone, *next, *end;
>>>  	const char *zgroup_name;
>>>  	char *file_name;
>>> -	struct dentry *dir;
>>> +	struct dentry *dir, *ret2;
>>>  	unsigned int n = 0;
>>>  	int ret;
>>>  
>>> @@ -1523,8 +1531,11 @@ static int zonefs_create_zgroup(struct zonefs_zone_data *zd,
>>>  		zgroup_name = "seq";
>>>  
>>>  	dir = zonefs_create_inode(sb->s_root, zgroup_name, NULL, type);
>>> -	if (!dir) {
>>> -		ret = -ENOMEM;
>>> +	if (IS_ERR_OR_NULL(dir)) {
>>> +		if (!dir)
>>> +			ret = -ENOMEM;
>>
>> It would be cleaner to return ERR_PTR(-ENOMEM) instead of NULL in
>> zonefs_create_inode(). This way, this can simply be:
>> 		if (IS_ERR(dir)) {
>> 			ret = PTR_ERR(dir);
>> 			goto free;
>> 		}
>>
>> And the hunk below would be similar too.
>>
> 
> Agreed, I'll update the patch. Or do you want to do it when squashing it
> into your "zonefs: fix zone report size in __zonefs_io_error()" patch?

If you can do the squashing, that would be nice too :)

-- 
Damien Le Moal
Western Digital Research