Re: [f2fs-dev] [RESEND] f2fs: add sanity compress level check for compressed file

[Chao Yu <chao@kernel.org>]

On 2023/4/3 11:46, Chao Yu wrote:
> On 2023/3/31 0:28, Yangtao Li wrote:
>> Commit 3fde13f817e2 ("f2fs: compress: support compress level")
>> forgot to do basic compress level check, let's add it.
>>
>> Signed-off-by: Yangtao Li <frank.li@vivo.com>
>> ---
>>    fs/f2fs/inode.c             | 94 +++++++++++++++++++++++++------------
>>    include/linux/zstd_lib.h    |  3 ++
>>    lib/zstd/compress/clevels.h |  4 --
>>    3 files changed, 67 insertions(+), 34 deletions(-)
>>
>> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
>> index bb5b365a195d..e63f75168700 100644
>> --- a/fs/f2fs/inode.c
>> +++ b/fs/f2fs/inode.c
>> @@ -10,6 +10,8 @@
>>    #include <linux/buffer_head.h>
>>    #include <linux/writeback.h>
>>    #include <linux/sched/mm.h>
>> +#include <linux/lz4.h>
>> +#include <linux/zstd.h>
>>    
>>    #include "f2fs.h"
>>    #include "node.h"
>> @@ -202,6 +204,66 @@ void f2fs_inode_chksum_set(struct f2fs_sb_info *sbi, struct page *page)
>>    	ri->i_inode_checksum = cpu_to_le32(f2fs_inode_chksum(sbi, page));
>>    }
>>    
>> +static bool sanity_check_compress_inode(struct inode *inode,
>> +			struct f2fs_inode *ri)
>> +{
>> +	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>> +	unsigned char compress_level;
>> +
>> +	if (ri->i_compress_algorithm >= COMPRESS_MAX) {
>> +		set_sbi_flag(sbi, SBI_NEED_FSCK);
>> +		f2fs_warn(sbi,
>> +			"%s: inode (ino=%lx) has unsupported compress algorithm: %u, run fsck to fix",
>> +			__func__, inode->i_ino, ri->i_compress_algorithm);
>> +		return false;
>> +	}
>> +	if (le64_to_cpu(ri->i_compr_blocks) >
>> +			SECTOR_TO_BLOCK(inode->i_blocks)) {
>> +		set_sbi_flag(sbi, SBI_NEED_FSCK);
>> +		f2fs_warn(sbi,
>> +			"%s: inode (ino=%lx) has inconsistent i_compr_blocks:%llu, i_blocks:%llu, run fsck to fix",
>> +			__func__, inode->i_ino, le64_to_cpu(ri->i_compr_blocks),
>> +			SECTOR_TO_BLOCK(inode->i_blocks));
>> +		return false;
>> +	}
>> +	if (ri->i_log_cluster_size < MIN_COMPRESS_LOG_SIZE ||
>> +		ri->i_log_cluster_size > MAX_COMPRESS_LOG_SIZE) {
>> +		set_sbi_flag(sbi, SBI_NEED_FSCK);
>> +		f2fs_warn(sbi,
>> +			"%s: inode (ino=%lx) has unsupported log cluster size: %u, run fsck to fix",
>> +			__func__, inode->i_ino, ri->i_log_cluster_size);
>> +		return false;
>> +	}
>> +
>> +	compress_level = le16_to_cpu(ri->i_compress_flag) >> COMPRESS_LEVEL_OFFSET;
> 
> Exceed 80 lines.

Sorry, colunms... out of my mind.

> 
>> +	switch (ri->i_compress_algorithm) {
>> +	case COMPRESS_LZO:
>> +	case COMPRESS_LZORLE:
>> +		if (compress_level)
>> +			goto err;
>> +		break;
>> +	case COMPRESS_LZ4:
>> +		if ((compress_level && compress_level < LZ4HC_MIN_CLEVEL) ||
>> +				compress_level > LZ4HC_MAX_CLEVEL)
>> +			goto err;
>> +		break;
>> +	case COMPRESS_ZSTD:
>> +		if (!compress_level || compress_level > ZSTD_MAX_CLEVEL)
>> +			goto err;
>> +		break;
>> +	default:
>> +		goto err;
>> +	}
>> +
>> +	return true;
>> +
>> +err:
>> +	set_sbi_flag(sbi, SBI_NEED_FSCK);
>> +	f2fs_warn(sbi, "%s: inode (ino=%lx) has unsupported compress level: %u, run fsck to fix",
>> +		  __func__, inode->i_ino, compress_level);
>> +	return false;
>> +}
>> +
>>    static bool sanity_check_inode(struct inode *inode, struct page *node_page)
>>    {
>>    	struct f2fs_sb_info *sbi = F2FS_I_SB(inode);
>> @@ -285,36 +347,8 @@ static bool sanity_check_inode(struct inode *inode, struct page *node_page)
>>    
>>    	if (f2fs_has_extra_attr(inode) && f2fs_sb_has_compression(sbi) &&
>>    			fi->i_flags & F2FS_COMPR_FL &&
>> -			F2FS_FITS_IN_INODE(ri, fi->i_extra_isize,
>> -						i_log_cluster_size)) {
>> -		if (ri->i_compress_algorithm >= COMPRESS_MAX) {
>> -			set_sbi_flag(sbi, SBI_NEED_FSCK);
>> -			f2fs_warn(sbi, "%s: inode (ino=%lx) has unsupported "
>> -				"compress algorithm: %u, run fsck to fix",
>> -				  __func__, inode->i_ino,
>> -				  ri->i_compress_algorithm);
>> -			return false;
>> -		}
>> -		if (le64_to_cpu(ri->i_compr_blocks) >
>> -				SECTOR_TO_BLOCK(inode->i_blocks)) {
>> -			set_sbi_flag(sbi, SBI_NEED_FSCK);
>> -			f2fs_warn(sbi, "%s: inode (ino=%lx) has inconsistent "
>> -				"i_compr_blocks:%llu, i_blocks:%llu, run fsck to fix",
>> -				  __func__, inode->i_ino,
>> -				  le64_to_cpu(ri->i_compr_blocks),
>> -				  SECTOR_TO_BLOCK(inode->i_blocks));
>> -			return false;
>> -		}
>> -		if (ri->i_log_cluster_size < MIN_COMPRESS_LOG_SIZE ||
>> -			ri->i_log_cluster_size > MAX_COMPRESS_LOG_SIZE) {
>> -			set_sbi_flag(sbi, SBI_NEED_FSCK);
>> -			f2fs_warn(sbi, "%s: inode (ino=%lx) has unsupported "
>> -				"log cluster size: %u, run fsck to fix",
>> -				  __func__, inode->i_ino,
>> -				  ri->i_log_cluster_size);
>> -			return false;
>> -		}
>> -	}
>> +			F2FS_FITS_IN_INODE(ri, fi->i_extra_isize, i_log_cluster_size))
> 
> Exceed 80 lines.

Ditto.

Thanks,

> 
>> +		sanity_check_compress_inode(inode, ri);
> 
> Missed to check return value?
> 
>>    
>>    	return true;
>>    }
>> diff --git a/include/linux/zstd_lib.h b/include/linux/zstd_lib.h
>> index 79d55465d5c1..ff55f41c73d3 100644
>> --- a/include/linux/zstd_lib.h
>> +++ b/include/linux/zstd_lib.h
>> @@ -88,6 +88,9 @@ ZSTDLIB_API const char* ZSTD_versionString(void);
>>    #  define ZSTD_CLEVEL_DEFAULT 3
>>    #endif
>>    
>> +/*-=====  Pre-defined compression levels  =====-*/
>> +#define ZSTD_MAX_CLEVEL     22
>> +
>>    /* *************************************
>>     *  Constants
>>     ***************************************/
>> diff --git a/lib/zstd/compress/clevels.h b/lib/zstd/compress/clevels.h
>> index d9a76112ec3a..b040d9d29089 100644
>> --- a/lib/zstd/compress/clevels.h
>> +++ b/lib/zstd/compress/clevels.h
>> @@ -14,10 +14,6 @@
>>    #define ZSTD_STATIC_LINKING_ONLY  /* ZSTD_compressionParameters  */
>>    #include <linux/zstd.h>
>>    
>> -/*-=====  Pre-defined compression levels  =====-*/
>> -
>> -#define ZSTD_MAX_CLEVEL     22
> 
> Why not zstd_max_clevel()?
> 
> Thanks,
> 
>> -
>>    __attribute__((__unused__))
>>    
>>    static const ZSTD_compressionParameters ZSTD_defaultCParameters[4][ZSTD_MAX_CLEVEL+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