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

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

[Daeho Jeong]

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;
 #endif
 };
 
@@ -3955,6 +3960,18 @@ int f2fs_init_page_array_cache(struct f2fs_sb_info *sbi);
 void f2fs_destroy_page_array_cache(struct f2fs_sb_info *sbi);
 int __init f2fs_init_compress_cache(void);
 void f2fs_destroy_compress_cache(void);
+#define inc_compr_inode_stat(inode)					\
+	do {								\
+		struct f2fs_sb_info *sbi = F2FS_I_SB(inode);		\
+		atomic_inc(&sbi->compr_new_inode);			\
+	} while (0)
+#define add_compr_block_stat(inode, blocks)				\
+	do {								\
+		struct f2fs_sb_info *sbi = F2FS_I_SB(inode);		\
+		int diff = F2FS_I(inode)->i_cluster_size - blocks;	\
+		atomic64_add(blocks, &sbi->compr_written_block);	\
+		atomic64_add(diff, &sbi->compr_saved_block);		\
+	} while (0)
 #else
 static inline bool f2fs_is_compressed_page(struct page *page) { return false; }
 static inline bool f2fs_is_compress_backend_ready(struct inode *inode)
@@ -3983,6 +4000,7 @@ static inline int f2fs_init_page_array_cache(struct f2fs_sb_info *sbi) { return
 static inline void f2fs_destroy_page_array_cache(struct f2fs_sb_info *sbi) { }
 static inline int __init f2fs_init_compress_cache(void) { return 0; }
 static inline void f2fs_destroy_compress_cache(void) { }
+#define inc_compr_inode_stat(inode)		do { } while (0)
 #endif
 
 static inline void set_compress_context(struct inode *inode)
@@ -4006,6 +4024,7 @@ static inline void set_compress_context(struct inode *inode)
 	F2FS_I(inode)->i_flags |= F2FS_COMPR_FL;
 	set_inode_flag(inode, FI_COMPRESSED_FILE);
 	stat_inc_compr_inode(inode);
+	inc_compr_inode_stat(inode);
 	f2fs_mark_inode_dirty_sync(inode, true);
 }
 
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 7069793752f1..88d9ecdee8d3 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -3260,6 +3260,13 @@ static void init_sb_info(struct f2fs_sb_info *sbi)
 
 	init_rwsem(&sbi->sb_lock);
 	init_rwsem(&sbi->pin_sem);
+
+#ifdef CONFIG_F2FS_FS_COMPRESSION
+	/* For runtime compression statistics */
+	atomic64_set(&sbi->compr_written_block, 0);
+	atomic64_set(&sbi->compr_saved_block, 0);
+	atomic_set(&sbi->compr_new_inode, 0);
+#endif
 }
 
 static int init_percpu_info(struct f2fs_sb_info *sbi)
diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
index e38a7f6921dd..2b6e5e6e1286 100644
--- a/fs/f2fs/sysfs.c
+++ b/fs/f2fs/sysfs.c
@@ -282,6 +282,38 @@ static ssize_t f2fs_sbi_show(struct f2fs_attr *a,
 		return len;
 	}
 
+#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);
+		return len;
+	}
+
+	if (!strcmp(a->attr.name, "compr_saved_block")) {
+		u64 bcount;
+		int len;
+
+		bcount = atomic64_read(&sbi->compr_saved_block);
+
+		len = scnprintf(buf, PAGE_SIZE, "%llu\n", bcount);
+		return len;
+	}
+
+	if (!strcmp(a->attr.name, "compr_new_inode")) {
+		u32 icount;
+		int len;
+
+		icount = atomic_read(&sbi->compr_new_inode);
+
+		len = scnprintf(buf, PAGE_SIZE, "%u\n", icount);
+		return len;
+	}
+#endif
+
 	ui = (unsigned int *)(ptr + a->offset);
 
 	return sprintf(buf, "%u\n", *ui);
@@ -458,6 +490,24 @@ static ssize_t __sbi_store(struct f2fs_attr *a,
 		return count;
 	}
 
+#ifdef CONFIG_F2FS_FS_COMPRESSION
+	if (!strcmp(a->attr.name, "compr_written_block") ||
+		!strcmp(a->attr.name, "compr_saved_block")) {
+		if (t != 0)
+			return -EINVAL;
+		atomic64_set(&sbi->compr_written_block, 0);
+		atomic64_set(&sbi->compr_saved_block, 0);
+		return count;
+	}
+
+	if (!strcmp(a->attr.name, "compr_new_inode")) {
+		if (t != 0)
+			return -EINVAL;
+		atomic_set(&sbi->compr_new_inode, 0);
+		return count;
+	}
+#endif
+
 	*ui = (unsigned int)t;
 
 	return count;
@@ -668,6 +718,9 @@ F2FS_FEATURE_RO_ATTR(sb_checksum, FEAT_SB_CHECKSUM);
 F2FS_FEATURE_RO_ATTR(casefold, FEAT_CASEFOLD);
 #ifdef CONFIG_F2FS_FS_COMPRESSION
 F2FS_FEATURE_RO_ATTR(compression, FEAT_COMPRESSION);
+F2FS_RW_ATTR(F2FS_SBI, f2fs_sb_info, compr_written_block, compr_written_block);
+F2FS_RW_ATTR(F2FS_SBI, f2fs_sb_info, compr_saved_block, compr_saved_block);
+F2FS_RW_ATTR(F2FS_SBI, f2fs_sb_info, compr_new_inode, compr_new_inode);
 #endif
 
 #define ATTR_LIST(name) (&f2fs_attr_##name.attr)
@@ -730,6 +783,11 @@ static struct attribute *f2fs_attrs[] = {
 	ATTR_LIST(moved_blocks_foreground),
 	ATTR_LIST(moved_blocks_background),
 	ATTR_LIST(avg_vblocks),
+#endif
+#ifdef CONFIG_F2FS_FS_COMPRESSION
+	ATTR_LIST(compr_written_block),
+	ATTR_LIST(compr_saved_block),
+	ATTR_LIST(compr_new_inode),
 #endif
 	NULL,
 };
-- 
2.31.0.rc2.261.g7f71774620-goog

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

[Greg KH]

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

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

[Daeho Jeong]

Thanks for suggesting me sysfs_emit().

For atomic values, actually, those are needed for writer part, not reader.

+#define add_compr_block_stat(inode, blocks)                            \
+       do {                                                            \
+               struct f2fs_sb_info *sbi = F2FS_I_SB(inode);            \
+               int diff = F2FS_I(inode)->i_cluster_size - blocks;      \
+               atomic64_add(blocks, &sbi->compr_written_block);        \
+               atomic64_add(diff, &sbi->compr_saved_block);            \
+       } while (0)

I needed a protection here, because they might be updated in the race condition.

2021년 3월 12일 (금) 오후 9:39, 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

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

[Greg KH]

On Fri, Mar 12, 2021 at 10:56:13PM +0900, Daeho Jeong wrote:
> Thanks for suggesting me sysfs_emit().
> 
> For atomic values, actually, those are needed for writer part, not reader.
> 
> +#define add_compr_block_stat(inode, blocks)                            \
> +       do {                                                            \
> +               struct f2fs_sb_info *sbi = F2FS_I_SB(inode);            \
> +               int diff = F2FS_I(inode)->i_cluster_size - blocks;      \
> +               atomic64_add(blocks, &sbi->compr_written_block);        \
> +               atomic64_add(diff, &sbi->compr_saved_block);            \
> +       } while (0)
> 
> I needed a protection here, because they might be updated in the race condition.

Why?  What are you trying to protect from "racing" here?

thanks,

greg k-h

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

[Daeho Jeong]

As you can see, if we're doing like the below.

sbi->compr_written_block += blocks;

Let's assume the initial value as 0.

<thread A>                                             <thread B>
sbi->compr_written_block = 0;

sbi->compr_written_block = 0;
+blocks(3);
                                                               + blocks(2);
sbi->compr_written_block = 3;

sbi->compr_written_block = 2;

Finally, we end up with 2, not 5.

As more threads are participating it, we might miss more counting.

2021년 3월 12일 (금) 오후 11:04, Greg KH <gregkh@linuxfoundation.org>님이 작성:
>
> On Fri, Mar 12, 2021 at 10:56:13PM +0900, Daeho Jeong wrote:
> > Thanks for suggesting me sysfs_emit().
> >
> > For atomic values, actually, those are needed for writer part, not reader.
> >
> > +#define add_compr_block_stat(inode, blocks)                            \
> > +       do {                                                            \
> > +               struct f2fs_sb_info *sbi = F2FS_I_SB(inode);            \
> > +               int diff = F2FS_I(inode)->i_cluster_size - blocks;      \
> > +               atomic64_add(blocks, &sbi->compr_written_block);        \
> > +               atomic64_add(diff, &sbi->compr_saved_block);            \
> > +       } while (0)
> >
> > I needed a protection here, because they might be updated in the race condition.
>
> Why?  What are you trying to protect from "racing" here?
>
> thanks,
>
> greg k-h

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

[Daeho Jeong]

So, do you want we protect the values here with spin_lock and just
read without spin_lock in sysfs read part?

2021년 3월 12일 (금) 오후 11:37, Daeho Jeong <daeho43@gmail.com>님이 작성:
>
> As you can see, if we're doing like the below.
>
> sbi->compr_written_block += blocks;
>
> Let's assume the initial value as 0.
>
> <thread A>                                             <thread B>
> sbi->compr_written_block = 0;
>
> sbi->compr_written_block = 0;
> +blocks(3);
>                                                                + blocks(2);
> sbi->compr_written_block = 3;
>
> sbi->compr_written_block = 2;
>
> Finally, we end up with 2, not 5.
>
> As more threads are participating it, we might miss more counting.
>
> 2021년 3월 12일 (금) 오후 11:04, Greg KH <gregkh@linuxfoundation.org>님이 작성:
> >
> > On Fri, Mar 12, 2021 at 10:56:13PM +0900, Daeho Jeong wrote:
> > > Thanks for suggesting me sysfs_emit().
> > >
> > > For atomic values, actually, those are needed for writer part, not reader.
> > >
> > > +#define add_compr_block_stat(inode, blocks)                            \
> > > +       do {                                                            \
> > > +               struct f2fs_sb_info *sbi = F2FS_I_SB(inode);            \
> > > +               int diff = F2FS_I(inode)->i_cluster_size - blocks;      \
> > > +               atomic64_add(blocks, &sbi->compr_written_block);        \
> > > +               atomic64_add(diff, &sbi->compr_saved_block);            \
> > > +       } while (0)
> > >
> > > I needed a protection here, because they might be updated in the race condition.
> >
> > Why?  What are you trying to protect from "racing" here?
> >
> > thanks,
> >
> > greg k-h

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

[Greg KH]

A: http://en.wikipedia.org/wiki/Top_post
Q: Were do I find info about this thing called top-posting?
A: Because it messes up the order in which people normally read text.
Q: Why is top-posting such a bad thing?
A: Top-posting.
Q: What is the most annoying thing in e-mail?

A: No.
Q: Should I include quotations after my reply?

http://daringfireball.net/2007/07/on_top


On Fri, Mar 12, 2021 at 11:37:29PM +0900, Daeho Jeong wrote:
> As you can see, if we're doing like the below.
> 
> sbi->compr_written_block += blocks;
> 
> Let's assume the initial value as 0.
> 
> <thread A>                                             <thread B>
> sbi->compr_written_block = 0;
> 
> sbi->compr_written_block = 0;
> +blocks(3);
>                                                                + blocks(2);
> sbi->compr_written_block = 3;
> 
> sbi->compr_written_block = 2;
> 
> Finally, we end up with 2, not 5.
> 
> As more threads are participating it, we might miss more counting.

Are you sure?  Isn't adding a number something that should happen in a
"safe" way?

And if you miss 2 blocks, who cares?  What is so critical about these
things that you take the cache flush of 2 atomic writes just for a
debugging statistic?

Why not just take 1 lock for everything if it's so important to get
these "correct"?

What is the performance throughput degradation of adding 2 atomic writes
to each time you write a block?

But really, will you ever notice missing a few, even if that could be
possible on your cpu (and I strongly doubt most modern cpus will miss
this...)

But this isn't my code, I just hate seeing atomic variables used for
silly things like debugging stats when they do not seem to be really
needed.  So if you want to keep them, go ahead, but realize that the
number you are reading has nothing to do with being "atomic" at all.

thanks,

greg k-h

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

[Greg KH]

On Fri, Mar 12, 2021 at 11:42:04PM +0900, Daeho Jeong wrote:
> So, do you want we protect the values here with spin_lock and just
> read without spin_lock in sysfs read part?

I would not use any lock at all if this were my code.  Remember, this is
for debugging/information only, right?  And the data is always "stale"
so what is it going to be used for?

thanks,

greg k-h

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

[Daeho Jeong]

2021년 3월 12일 (금) 오후 11:45, Greg KH <gregkh@linuxfoundation.org>님이 작성:
>
> A: http://en.wikipedia.org/wiki/Top_post
> Q: Were do I find info about this thing called top-posting?
> A: Because it messes up the order in which people normally read text.
> Q: Why is top-posting such a bad thing?
> A: Top-posting.
> Q: What is the most annoying thing in e-mail?
>
> A: No.
> Q: Should I include quotations after my reply?
>
> http://daringfireball.net/2007/07/on_top
>

Thanks for letting me know this!

>
> On Fri, Mar 12, 2021 at 11:37:29PM +0900, Daeho Jeong wrote:
> > As you can see, if we're doing like the below.
> >
> > sbi->compr_written_block += blocks;
> >
> > Let's assume the initial value as 0.
> >
> > <thread A>                                             <thread B>
> > sbi->compr_written_block = 0;
> >
> > sbi->compr_written_block = 0;
> > +blocks(3);
> >                                                                + blocks(2);
> > sbi->compr_written_block = 3;
> >
> > sbi->compr_written_block = 2;
> >
> > Finally, we end up with 2, not 5.
> >
> > As more threads are participating it, we might miss more counting.
>
> Are you sure?  Isn't adding a number something that should happen in a
> "safe" way?
>
> And if you miss 2 blocks, who cares?  What is so critical about these
> things that you take the cache flush of 2 atomic writes just for a
> debugging statistic?
>
> Why not just take 1 lock for everything if it's so important to get
> these "correct"?
>
> What is the performance throughput degradation of adding 2 atomic writes
> to each time you write a block?
>
> But really, will you ever notice missing a few, even if that could be
> possible on your cpu (and I strongly doubt most modern cpus will miss
> this...)
>
> But this isn't my code, I just hate seeing atomic variables used for
> silly things like debugging stats when they do not seem to be really
> needed.  So if you want to keep them, go ahead, but realize that the
> number you are reading has nothing to do with being "atomic" at all.
>
> thanks,
>

I agree that missing number would be extremely few and the overhead of
updating the numbers would be quite bad.

Thanks for your valuable comments. :)

> greg k-h