On Tue, 2025-07-22 at 15:50 +0800, Yangtao Li wrote: > Hi Slava, > > 在 2025/7/12 01:24, Viacheslav Dubeyko 写道: > > On Fri, 2025-07-11 at 10:41 +0800, Yangtao Li wrote: > > > Hi Slava, > > > > > > 在 2025/7/11 06:16, Viacheslav Dubeyko 写道: > > > > Currently, HFS/HFS+ has very obsolete and inconvenient > > > > debug output subsystem. Also, the code is duplicated > > > > in HFS and HFS+ driver. This patch introduces > > > > linux/hfs_common.h for gathering common declarations, > > > > inline functions, and common short methods. Currently, > > > > this file contains only hfs_dbg() function that > > > > employs pr_debug() with the goal to print a debug-level > > > > messages conditionally. > > > > > > > > So, now, it is possible to enable the debug output > > > > by means of: > > > > > > > > echo 'file extent.c +p' > /proc/dynamic_debug/control > > > > echo 'func hfsplus_evict_inode +p' > > > > > /proc/dynamic_debug/control > > > > > > > > And debug output looks like this: > > > > > > > > hfs: pid 5831:fs/hfs/catalog.c:228 hfs_cat_delete(): > > > > delete_cat: > > > > m00,48 > > > > hfs: pid 5831:fs/hfs/extent.c:484 hfs_file_truncate(): > > > > truncate: > > > > 48, 409600 -> 0 > > > > hfs: pid 5831:fs/hfs/extent.c:212 hfs_dump_extent(): > > > > hfs: pid 5831:fs/hfs/extent.c:214 hfs_dump_extent(): 78:4 > > > > hfs: pid 5831:fs/hfs/extent.c:214 hfs_dump_extent(): 0:0 > > > > hfs: pid 5831:fs/hfs/extent.c:214 hfs_dump_extent(): 0:0 > > > > > > > > Signed-off-by: Viacheslav Dubeyko <slava@xxxxxxxxxxx> > > > > cc: John Paul Adrian Glaubitz <glaubitz@xxxxxxxxxxxxxxxxxxx> > > > > cc: Yangtao Li <frank.li@xxxxxxxx> > > > > cc: linux-fsdevel@xxxxxxxxxxxxxxx > > > > cc: Johannes Thumshirn <Johannes.Thumshirn@xxxxxxx> > > > > --- > > > > fs/hfs/bfind.c | 4 ++-- > > > > fs/hfs/bitmap.c | 4 ++-- > > > > fs/hfs/bnode.c | 28 ++++++++++++++-------------- > > > > fs/hfs/brec.c | 8 ++++---- > > > > fs/hfs/btree.c | 2 +- > > > > fs/hfs/catalog.c | 6 +++--- > > > > fs/hfs/extent.c | 18 +++++++++--------- > > > > fs/hfs/hfs_fs.h | 33 +--------------------------- > > > > ----- > > > > fs/hfs/inode.c | 4 ++-- > > > > fs/hfsplus/attributes.c | 8 ++++---- > > > > fs/hfsplus/bfind.c | 4 ++-- > > > > fs/hfsplus/bitmap.c | 10 +++++----- > > > > fs/hfsplus/bnode.c | 28 ++++++++++++++-------------- > > > > fs/hfsplus/brec.c | 10 +++++----- > > > > fs/hfsplus/btree.c | 4 ++-- > > > > fs/hfsplus/catalog.c | 6 +++--- > > > > fs/hfsplus/extents.c | 24 ++++++++++++------------ > > > > fs/hfsplus/hfsplus_fs.h | 35 +--------------------------- > > > > ----- > > > > -- > > > > fs/hfsplus/super.c | 8 ++++---- > > > > fs/hfsplus/xattr.c | 4 ++-- > > > > include/linux/hfs_common.h | 20 ++++++++++++++++++++ > > > > > > For include/linux/hfs_common.h, it seems like to be a good start > > > to > > > seperate common stuff for hfs&hfsplus. > > > > > > Colud we rework msg to add value description? > > > There're too much values to identify what it is. > > > > > > > What do you mean by value description? > > For example: > > hfs_dbg(BNODE_MOD, "%d, %d, %d, %d, %d\n", > be32_to_cpu(desc.next), be32_to_cpu(desc.prev), > desc.type, desc.height, be16_to_cpu(desc.num_recs)); > > There are 5 %d. It's hard to recognize what it is. Changing it to > following style w/ description might be a bit more clear? > > hfs_dbg(BNODE_MOD, "next:%d prev:%d, type:%s, > height:%d > num_recs:%d\n", be32_to_cpu(desc.next), be32_to_cpu(desc.prev), > hfs_node_type(desc.type), desc.height, be16_to_cpu(desc.num_recs)); > We can rework it step by step. First of all, the reworking of all debug messages at once is too much for one patch. Secondly, the style of messages is history of HFS implementation. I suggest to make this first step and, then, we can rework the debugging messages in the background of bug fix. Does it make sense to you? > > > > > You ignore those msg type, maybe we don't need it? > > > > Could you please explain what do you mean here? :) > > -#define DBG_BNODE_REFS 0x00000001 > -#define DBG_BNODE_MOD 0x00000002 > -#define DBG_CAT_MOD 0x00000004 > -#define DBG_INODE 0x00000008 > -#define DBG_SUPER 0x00000010 > -#define DBG_EXTENT 0x00000020 > -#define DBG_BITMAP 0x00000040 > > I'm not sure whether we should keep those dbg type. > > I have removed all these types because with dynamic debug this stuff doesn't make sense. If you would like to enable the debug output from different parts of driver, then you can use commands [1]: (1) enable all the messages in HFS module: echo -n 'module hfs +p' > <debugfs>/dynamic_debug/control (2) enable all the messages in file: echo -n 'file inode.c +p' > <debugfs>/dynamic_debug/control (3) enable all messages in the function hfs_dump_extent: echo -n 'func hfs_dump_extent +p' > <debugfs>/dynamic_debug/control (4) disable debug output: echo -n 'module hfs -p' > <debugfs>/dynamic_debug/control echo -n 'file inode.c -p' > <debugfs>/dynamic_debug/control echo -n 'func hfs_dump_extent -p' > <debugfs>/dynamic_debug/control So, the dynamic debug is flexible enough and you don't need to recompile the kernel. So, why do we need to keep these dbg types? Thanks, Slava. [1] https://www.kernel.org/doc/html/v4.14/admin-guide/dynamic-debug-howto.html
