Re: [PATCH v4] f2fs: add sysfs nodes to get runtime compression stat

[Greg KH <gregkh@linuxfoundation.org>]

On Fri, Mar 12, 2021 at 09:25:31PM +0900, Daeho Jeong wrote:
> From: Daeho Jeong <daehojeong@google.com>
> 
> I've added new sysfs nodes to show runtime compression stat since mount.
> compr_written_block - show the block count written after compression
> compr_saved_block - show the saved block count with compression
> compr_new_inode - show the count of inode newly enabled for compression
> 
> Signed-off-by: Daeho Jeong <daehojeong@google.com>
> ---
> v2: thanks to kernel test robot <lkp@intel.com>, fixed compile issue
>     related to kernel config
> v3: changed sysfs nodes' names and made them runtime stat, not
>     persistent on disk
> v4: changed sysfs nodes' desctiption
> ---
>  Documentation/ABI/testing/sysfs-fs-f2fs | 24 ++++++++++
>  fs/f2fs/compress.c                      |  1 +
>  fs/f2fs/f2fs.h                          | 19 ++++++++
>  fs/f2fs/super.c                         |  7 +++
>  fs/f2fs/sysfs.c                         | 58 +++++++++++++++++++++++++
>  5 files changed, 109 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-fs-f2fs b/Documentation/ABI/testing/sysfs-fs-f2fs
> index cbeac1bebe2f..ddd4bd6116fc 100644
> --- a/Documentation/ABI/testing/sysfs-fs-f2fs
> +++ b/Documentation/ABI/testing/sysfs-fs-f2fs
> @@ -409,3 +409,27 @@ Description:	Give a way to change checkpoint merge daemon's io priority.
>  		I/O priority "3". We can select the class between "rt" and "be",
>  		and set the I/O priority within valid range of it. "," delimiter
>  		is necessary in between I/O class and priority number.
> +
> +What:		/sys/fs/f2fs/<disk>/compr_written_block
> +Date:		March 2021
> +Contact:	"Daeho Jeong" <daehojeong@google.com>
> +Description:	Show the block count written after compression since mount. Note
> +		that when the compressed blocks are deleted, this count doesn't
> +		decrease. If you write "0" here, you can initialize
> +		compr_written_block and compr_saved_block to "0".
> +
> +What:		/sys/fs/f2fs/<disk>/compr_saved_block
> +Date:		March 2021
> +Contact:	"Daeho Jeong" <daehojeong@google.com>
> +Description:	Show the saved block count with compression since mount. Note
> +		that when the compressed blocks are deleted, this count doesn't
> +		decrease. If you write "0" here, you can initialize
> +		compr_written_block and compr_saved_block to "0".
> +
> +What:		/sys/fs/f2fs/<disk>/compr_new_inode
> +Date:		March 2021
> +Contact:	"Daeho Jeong" <daehojeong@google.com>
> +Description:	Show the count of inode newly enabled for compression since mount.
> +		Note that when the compression is disabled for the files, this count
> +		doesn't decrease. If you write "0" here, you can initialize
> +		compr_new_inode to "0".
> diff --git a/fs/f2fs/compress.c b/fs/f2fs/compress.c
> index 77fa342de38f..3c9d797dbdd6 100644
> --- a/fs/f2fs/compress.c
> +++ b/fs/f2fs/compress.c
> @@ -1353,6 +1353,7 @@ static int f2fs_write_compressed_pages(struct compress_ctx *cc,
>  	if (fio.compr_blocks)
>  		f2fs_i_compr_blocks_update(inode, fio.compr_blocks - 1, false);
>  	f2fs_i_compr_blocks_update(inode, cc->nr_cpages, true);
> +	add_compr_block_stat(inode, cc->nr_cpages);
>  
>  	set_inode_flag(cc->inode, FI_APPEND_WRITE);
>  	if (cc->cluster_idx == 0)
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index e2d302ae3a46..2c989f8caf05 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -1623,6 +1623,11 @@ struct f2fs_sb_info {
>  #ifdef CONFIG_F2FS_FS_COMPRESSION
>  	struct kmem_cache *page_array_slab;	/* page array entry */
>  	unsigned int page_array_slab_size;	/* default page array slab size */
> +
> +	/* For runtime compression statistics */
> +	atomic64_t compr_written_block;
> +	atomic64_t compr_saved_block;
> +	atomic_t compr_new_inode;

Why do you need these to be atomic?  What requires this?

> +#ifdef CONFIG_F2FS_FS_COMPRESSION
> +	if (!strcmp(a->attr.name, "compr_written_block")) {
> +		u64 bcount;
> +		int len;
> +
> +		bcount = atomic64_read(&sbi->compr_written_block);
> +
> +		len = scnprintf(buf, PAGE_SIZE, "%llu\n", bcount);

Please use sysfs_emit() for new sysfs entries like these.  Makes it much
simpler.

And look, you really do not need an atomic value as this is just a
random number you are sending to userspace that could be stale the
minute you read from it.

Please just use a normal u64 and save the cpu sync for stuff like this.

thanks,

greg k-h