Re: [PATCH] bcachefs: fix io_read/write counter with wrong unit

[Hongbo Li]

On 2024/11/7 5:26, Kent Overstreet wrote:
On Tue, Nov 05, 2024 at 10:22:24PM +0800, Hongbo Li wrote:
In bcachefs, io_read and io_write counter record the amount
of data which has been read and written. They increase in
unit of sector, so to display correctly, they need to be
shifted to the left by the size of a sector.

Fixes: 1c6fdbd8f246 ("bcachefs: Initial commit")
Signed-off-by: Hongbo Li <lihongb...@huawei.com>

Nice, I'd been meaning to fix that

---
When I read one 15.7M file, the io_read sysfs interface shows:
- Before this patch
   - original:
     since mount:                   0 B
     since filesystem creation:     5.11 KiB

   - after read:
     since mount:                   31.4 KiB
     since filesystem creation:     36.5 KiB

- After this patch:
   - original:
     since mount:                   0 B
     since filesystem creation:     18.3 MiB

   - after read:
     since mount:                   15.7 MiB
     since filesystem creation:     34.0 MiB
---
  fs/bcachefs/sysfs.c | 5 +++++
  1 file changed, 5 insertions(+)

diff --git a/fs/bcachefs/sysfs.c b/fs/bcachefs/sysfs.c
index 97733c766948..aa46b69da4d8 100644
--- a/fs/bcachefs/sysfs.c
+++ b/fs/bcachefs/sysfs.c
@@ -513,6 +513,11 @@ SHOW(bch2_fs_counters)
                if (attr == &sysfs_##t) {                                   \
                        counter             = 
percpu_u64_get(&c->counters[BCH_COUNTER_##t]);\
                        counter_since_mount = counter - 
c->counters_on_mount[BCH_COUNTER_##t];\
+                       if (attr == &sysfs_io_read || attr == &sysfs_io_write) 
{\
+                               counter <<= 9;                                  
  \
+                               counter_since_mount <<= 9;                      
  \
+                       }                                                       
\
+                                                                               
\

But, I really try to avoid this kind of "do something special for one or
two cases" in generic code, this kind of thing proliferates and then it
becomes really hard to track down and see where behaviour is coming from
later.

We've already got a table of counters - BCH_PERSISTENT_COUNTERS(); it
would be better to add a "type" column there, and then it'll make it
easy to spot later if any other counters have the wrong type or need
different behaviour.

Yeah, I finish it in [1]. For counters like io_read, io_write, io_move 
and move_extent_{read, write, finish} should multiple 512 when they are 
displayed to user.

[1] 
https://lore.kernel.org/linux-bcachefs/20241107123145.1003801-1-lihongb...@huawei.com/T/#u

Thanks,
Hongbo