Hi Steven, ---- On Thu, 09 Apr 2026 04:02:56 +0800 Steven Rostedt <[email protected]> wrote --- > On Wed, 8 Apr 2026 19:20:17 +0800 > Li Chen <[email protected]> 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. > > > > [..] > > > @@ -1338,13 +1375,13 @@ static int ext4_fc_perform_commit(journal_t > > *journal) > > if (ret) > > return ret; > > > > - > > ret = ext4_fc_alloc_snapshot_inodes(sb, &inodes, &inodes_size); > > if (ret) > > return ret; > > > > /* Step 4: Mark all inodes as being committed. */ > > jbd2_journal_lock_updates(journal); > > + lock_start = ktime_get(); > > ktime_get() is rather quick but if you care about micro-optimizations, you > could have: > > if (trace_ext4_fc_lock_updates_enabled()) > lock_start = ktime_get(); > else > lock_start = 0; > > > /* > > * The journal is now locked. No more handles can start and all the > > * previous handles are now drained. Snapshotting happens in this > > @@ -1358,8 +1395,15 @@ static int ext4_fc_perform_commit(journal_t > > *journal) > > } > > ext4_fc_unlock(sb, alloc_ctx); > > > > - ret = ext4_fc_snapshot_inodes(journal, inodes, inodes_size); > > + ret = ext4_fc_snapshot_inodes(journal, inodes, inodes_size, > > + &snap_inodes, &snap_ranges, &snap_err); > > jbd2_journal_unlock_updates(journal); > > + if (trace_ext4_fc_lock_updates_enabled()) { > > if (trace_ext4_fc_lock_updates_enabled() && lock_start) { > > But feel free to ignore this if the overhead of always calling ktime_get() > is not an issue. > > > > + locked_ns = ktime_to_ns(ktime_sub(ktime_get(), lock_start)); > > + trace_ext4_fc_lock_updates(sb, commit_tid, locked_ns, > > + snap_inodes, snap_ranges, ret, > > + snap_err); > > + } > > kvfree(inodes); > > if (ret) > > return ret; > > @@ -1564,7 +1608,7 @@ int ext4_fc_commit(journal_t *journal, tid_t > > commit_tid) > > journal_ioprio = EXT4_DEF_JOURNAL_IOPRIO; > > set_task_ioprio(current, journal_ioprio); > > fc_bufs_before = (sbi->s_fc_bytes + bsize - 1) / bsize; > > - ret = ext4_fc_perform_commit(journal); > > + ret = ext4_fc_perform_commit(journal, commit_tid); > > if (ret < 0) { > > if (ret == -EAGAIN || ret == -E2BIG || ret == -ECANCELED) > > status = EXT4_FC_STATUS_INELIGIBLE; > > diff --git a/include/trace/events/ext4.h b/include/trace/events/ext4.h > > index f493642cf121..7028a28316fa 100644 > > --- a/include/trace/events/ext4.h > > +++ b/include/trace/events/ext4.h > > @@ -107,6 +107,26 @@ TRACE_DEFINE_ENUM(EXT4_FC_REASON_VERITY); > > TRACE_DEFINE_ENUM(EXT4_FC_REASON_MOVE_EXT); > > TRACE_DEFINE_ENUM(EXT4_FC_REASON_MAX); > > > > +#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); > > + > > +#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) > > + > > +TRACE_SNAP_ERR > > + > > +#undef EM > > +#undef EMe > > + > > #define show_fc_reason(reason) \ > > __print_symbolic(reason, \ > > { EXT4_FC_REASON_XATTR, "XATTR"}, \ > > @@ -2818,6 +2838,47 @@ TRACE_EVENT(ext4_fc_commit_stop, > > __entry->num_fc_ineligible, __entry->nblks_agg, __entry->tid) > > ); > > > > +#define EM(a) { EXT4_FC_SNAP_ERR_##a, #a }, > > +#define EMe(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 %s", > > + MAJOR(__entry->dev), MINOR(__entry->dev), __entry->tid, > > + __entry->locked_ns, __entry->nr_inodes, __entry->nr_ranges, > > + __entry->err, __print_symbolic(__entry->snap_err, > > + TRACE_SNAP_ERR)) > > +); > > + > > +#undef EM > > +#undef EMe > > +#undef TRACE_SNAP_ERR > > + > > #define FC_REASON_NAME_STAT(reason) \ > > show_fc_reason(reason), \ > > __entry->fc_ineligible_rc[reason] > > As for the rest: > > Reviewed-by: Steven Rostedt (Google) <[email protected]> > > [ Please add this reviewed-by to any new versions so I remember I already > looked at it. ]
Sure, thanks a lot for your thoughtful review! Regards, Li
