linux-ext4 - Re: [RFC v4 6/7] ext4: fast commit: add lock

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite for Android: free password hash cracker in your pocket
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <19bff57d732.675dda83304172.1851572061101730667@linux.beauty>
Date: Tue, 27 Jan 2026 20:05:05 +0800
From: Li Chen <me@...ux.beauty>
To: "Steven Rostedt" <rostedt@...dmis.org>
Cc: "Zhang Yi" <yi.zhang@...weicloud.com>, "Theodore Ts'o" <tytso@....edu>,
	"Andreas Dilger" <adilger.kernel@...ger.ca>,
	"Masami Hiramatsu" <mhiramat@...nel.org>,
	"Mathieu Desnoyers" <mathieu.desnoyers@...icios.com>,
	"linux-ext4" <linux-ext4@...r.kernel.org>,
	"linux-kernel" <linux-kernel@...r.kernel.org>,
	"linux-trace-kernel" <linux-trace-kernel@...r.kernel.org>
Subject: Re: [RFC v4 6/7] ext4: fast commit: add lock_updates tracepoint

Hi Steven,

 > On Tue, 20 Jan 2026 19:25:35 +0800
 > Li Chen <me@...ux.beauty> wrote:
 > 
 > > Commit-time fast commit snapshots run under jbd2_journal_lock_updates(),
 > > so it is useful to quantify the time spent with updates locked and to
 > > understand why snapshotting can fail.
 > > 
 > > Add a new tracepoint, ext4_fc_lock_updates, reporting the time spent in
 > > the updates-locked window along with the number of snapshotted inodes
 > > and ranges. Record the first snapshot failure reason in a stable snap_err
 > > field for tooling.
 > > 
 > > Signed-off-by: Li Chen <me@...ux.beauty>
 > > ---
 > >  fs/ext4/fast_commit.c       | 86 ++++++++++++++++++++++++++++++-------
 > >  include/trace/events/ext4.h | 33 ++++++++++++++
 > >  2 files changed, 104 insertions(+), 15 deletions(-)
 > > 
 > > diff --git a/fs/ext4/fast_commit.c b/fs/ext4/fast_commit.c
 > > index d1eefee60912..d266eb2a4219 100644
 > > --- a/fs/ext4/fast_commit.c
 > > +++ b/fs/ext4/fast_commit.c
 > > @@ -193,6 +193,27 @@ static struct kmem_cache *ext4_fc_range_cachep;
 > >  #define EXT4_FC_SNAPSHOT_MAX_INODES    1024
 > >  #define EXT4_FC_SNAPSHOT_MAX_RANGES    2048
 > >  
 > > +/*
 > > + * Snapshot failure reasons for ext4_fc_lock_updates tracepoint.
 > > + * Keep these stable for tooling.
 > > + */
 > > +enum ext4_fc_snap_err {
 > > +    EXT4_FC_SNAP_ERR_NONE = 0,
 > > +    EXT4_FC_SNAP_ERR_ES_MISS,
 > > +    EXT4_FC_SNAP_ERR_ES_DELAYED,
 > > +    EXT4_FC_SNAP_ERR_ES_OTHER,
 > > +    EXT4_FC_SNAP_ERR_INODES_CAP,
 > > +    EXT4_FC_SNAP_ERR_RANGES_CAP,
 > > +    EXT4_FC_SNAP_ERR_NOMEM,
 > > +    EXT4_FC_SNAP_ERR_INODE_LOC,
 > > +};
 > > +
 > > +static inline void ext4_fc_set_snap_err(int *snap_err, int err)
 > > +{
 > > +    if (snap_err && *snap_err == EXT4_FC_SNAP_ERR_NONE)
 > > +        *snap_err = err;
 > > +}
 > > +
 > 
 > 
 > 
 > > diff --git a/include/trace/events/ext4.h b/include/trace/events/ext4.h
 > > index fd76d14c2776..a1493971821d 100644
 > > --- a/include/trace/events/ext4.h
 > > +++ b/include/trace/events/ext4.h
 > > @@ -2812,6 +2812,39 @@ TRACE_EVENT(ext4_fc_commit_stop,
 > >            __entry->num_fc_ineligible, __entry->nblks_agg, __entry->tid)
 > >  );
 > >  
 > 
 > Why not make the snap_err into a human readable format?
 > 
 > #define TRACE_SNAP_ERR        \
 >     EM(NONE)        \
 >     EM(ES_MISS)        \
 >     EM(ES_DELAYED)        \
 >     EM(ES_OTHER)        \
 >     EM(INODES_CAP)        \
 >     EM(RANGES_CAP)        \
 >     EM(NOMEM)        \
 >     EMe(INODE_LOC)        \
 > 
 > #undef EM
 > #undef EMe
 > 
 > #define EM(a)    TRACE_DEFINE_ENUM(EXT4_FC_SNAP_ERR_##a);
 > #define EMe(a)    TRACE_DEFINE_ENUM(EXT4_FC_SNAP_ERR_##a);
 > 
 > TRACE_SNAP_ERR
 > 
 > #undef EM
 > #undef EMe
 > 
 > #define EM(a)    { EXT4_FC_SNAP_ERR_##a, #a },
 > #define EM(a)    { EXT4_FC_SNAP_ERR_##a, #a }
 > 
 > 
 > > +TRACE_EVENT(ext4_fc_lock_updates,
 > > +        TP_PROTO(struct super_block *sb, tid_t commit_tid, u64 locked_ns,
 > > +             unsigned int nr_inodes, unsigned int nr_ranges, int err,
 > > +             int snap_err),
 > > +
 > > +    TP_ARGS(sb, commit_tid, locked_ns, nr_inodes, nr_ranges, err, snap_err),
 > > +
 > > +    TP_STRUCT__entry(/* entry */
 > > +        __field(dev_t, dev)
 > > +        __field(tid_t, tid)
 > > +        __field(u64, locked_ns)
 > > +        __field(unsigned int, nr_inodes)
 > > +        __field(unsigned int, nr_ranges)
 > > +        __field(int, err)
 > > +        __field(int, snap_err)
 > > +    ),
 > > +
 > > +    TP_fast_assign(/* assign */
 > > +        __entry->dev = sb->s_dev;
 > > +        __entry->tid = commit_tid;
 > > +        __entry->locked_ns = locked_ns;
 > > +        __entry->nr_inodes = nr_inodes;
 > > +        __entry->nr_ranges = nr_ranges;
 > > +        __entry->err = err;
 > > +        __entry->snap_err = snap_err;
 > > +    ),
 > > +
 > > +    TP_printk("dev %d,%d tid %u locked_ns %llu nr_inodes %u nr_ranges %u err %d snap_err %d",
 > > +          MAJOR(__entry->dev), MINOR(__entry->dev), __entry->tid,
 > > +          __entry->locked_ns, __entry->nr_inodes, __entry->nr_ranges,
 > > +          __entry->err, __entry->snap_err)
 > 
 > And instead of having the raw value of __entry->snap_err, use:
 > 
 >         __entry->err, __print_symbolic(__entry->snap_err, TRACE_SNAP_ERR))

Good point. I'll switch snap_err to __print_symbolic() in v5.

Regards,
Li