* [PATCH v3 0/2] jbd2: audit and convert legacy J_ASSERT usage
@ 2026-03-03 0:55 Milos Nikic
2026-03-03 0:55 ` [PATCH v3 1/2] jbd2: gracefully abort instead of panicking on unlocked buffer Milos Nikic
` (2 more replies)
0 siblings, 3 replies; 6+ messages in thread
From: Milos Nikic @ 2026-03-03 0:55 UTC (permalink / raw)
To: jack; +Cc: tytso, linux-ext4, linux-kernel, Milos Nikic
Hello Jan and the ext4 team,
This patch series follows up on the previous discussion regarding
converting hard J_ASSERT panics into graceful journal aborts.
In v1, we addressed a specific panic on unlock. Per Jan's suggestion,
I have audited fs/jbd2/transaction.c for other low-hanging fruit
where state machine invariants are enforced by J_ASSERT inside
functions that natively support error returns.
Changes in v3:
Patch 2: Added pr_err() statements inside the ambiguous WARN_ON_ONCE()
blocks (where multiple conditions are checked via logical OR/AND) to
explicitly dump the b_transaction, b_next_transaction, and
j_committing_transaction pointers. This provides necessary context for
debugging state machine corruptions from the dmesg stack trace.
Changes in v2:
Patch 1: Unmodified from v1. Collected Reviewed-by tags.
Patch 2: New patch resulting from the broader audit. Systematically
replaces J_ASSERTs with WARN_ON_ONCE and graceful -EINVAL returns
across 6 core transaction lifecycle functions. Careful attention was
paid to ensuring spinlocks are safely dropped before triggering
jbd2_journal_abort(), and no memory is leaked on the error paths.
Milos Nikic (2):
jbd2: gracefully abort instead of panicking on unlocked buffer
jbd2: gracefully abort on transaction state corruptions
fs/jbd2/transaction.c | 115 +++++++++++++++++++++++++++++++++---------
1 file changed, 91 insertions(+), 24 deletions(-)
--
2.53.0
^ permalink raw reply [flat|nested] 6+ messages in thread* [PATCH v3 1/2] jbd2: gracefully abort instead of panicking on unlocked buffer 2026-03-03 0:55 [PATCH v3 0/2] jbd2: audit and convert legacy J_ASSERT usage Milos Nikic @ 2026-03-03 0:55 ` Milos Nikic 2026-03-03 1:05 ` Andreas Dilger 2026-03-03 0:55 ` [PATCH v3 2/2] jbd2: gracefully abort on transaction state corruptions Milos Nikic 2026-03-03 2:11 ` [PATCH v3 0/2] jbd2: audit and convert legacy J_ASSERT usage yebin (H) 2 siblings, 1 reply; 6+ messages in thread From: Milos Nikic @ 2026-03-03 0:55 UTC (permalink / raw) To: jack; +Cc: tytso, linux-ext4, linux-kernel, Milos Nikic, Zhang Yi In jbd2_journal_get_create_access(), if the caller passes an unlocked buffer, the code currently triggers a fatal J_ASSERT. While an unlocked buffer here is a clear API violation and a bug in the caller, crashing the entire system is an overly severe response. It brings down the whole machine for a localized filesystem inconsistency. Replace the J_ASSERT with a WARN_ON_ONCE to capture the offending caller's stack trace, and return an error (-EINVAL). This allows the journal to gracefully abort the transaction, protecting data integrity without causing a kernel panic. Signed-off-by: Milos Nikic <nikic.milos@gmail.com> Reviewed-by: Zhang Yi <yi.zhang@huawei.com> Reviewed-by: Jan Kara <jack@suse.cz> --- fs/jbd2/transaction.c | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c index dca4b5d8aaaa..04d17a5f2a82 100644 --- a/fs/jbd2/transaction.c +++ b/fs/jbd2/transaction.c @@ -1302,7 +1302,12 @@ int jbd2_journal_get_create_access(handle_t *handle, struct buffer_head *bh) goto out; } - J_ASSERT_JH(jh, buffer_locked(jh2bh(jh))); + if (WARN_ON_ONCE(!buffer_locked(jh2bh(jh)))) { + err = -EINVAL; + spin_unlock(&jh->b_state_lock); + jbd2_journal_abort(journal, err); + goto out; + } if (jh->b_transaction == NULL) { /* -- 2.53.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 1/2] jbd2: gracefully abort instead of panicking on unlocked buffer 2026-03-03 0:55 ` [PATCH v3 1/2] jbd2: gracefully abort instead of panicking on unlocked buffer Milos Nikic @ 2026-03-03 1:05 ` Andreas Dilger 0 siblings, 0 replies; 6+ messages in thread From: Andreas Dilger @ 2026-03-03 1:05 UTC (permalink / raw) To: Milos Nikic; +Cc: jack, tytso, linux-ext4, linux-kernel, Zhang Yi On Mar 2, 2026, at 17:55, Milos Nikic <nikic.milos@gmail.com> wrote: > > In jbd2_journal_get_create_access(), if the caller passes an unlocked > buffer, the code currently triggers a fatal J_ASSERT. > > While an unlocked buffer here is a clear API violation and a bug in the > caller, crashing the entire system is an overly severe response. It brings > down the whole machine for a localized filesystem inconsistency. > > Replace the J_ASSERT with a WARN_ON_ONCE to capture the offending caller's > stack trace, and return an error (-EINVAL). This allows the journal to > gracefully abort the transaction, protecting data integrity without > causing a kernel panic. > > Signed-off-by: Milos Nikic <nikic.milos@gmail.com> > Reviewed-by: Zhang Yi <yi.zhang@huawei.com> > Reviewed-by: Jan Kara <jack@suse.cz> Reviewed-by: Andreas Dilger <adilger@dilger.ca <mailto:adilger@dilger.ca>> > --- > fs/jbd2/transaction.c | 7 ++++++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c > index dca4b5d8aaaa..04d17a5f2a82 100644 > --- a/fs/jbd2/transaction.c > +++ b/fs/jbd2/transaction.c > @@ -1302,7 +1302,12 @@ int jbd2_journal_get_create_access(handle_t *handle, struct buffer_head *bh) > goto out; > } > > - J_ASSERT_JH(jh, buffer_locked(jh2bh(jh))); > + if (WARN_ON_ONCE(!buffer_locked(jh2bh(jh)))) { > + err = -EINVAL; > + spin_unlock(&jh->b_state_lock); > + jbd2_journal_abort(journal, err); > + goto out; > + } > > if (jh->b_transaction == NULL) { > /* > -- > 2.53.0 > > ^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v3 2/2] jbd2: gracefully abort on transaction state corruptions 2026-03-03 0:55 [PATCH v3 0/2] jbd2: audit and convert legacy J_ASSERT usage Milos Nikic 2026-03-03 0:55 ` [PATCH v3 1/2] jbd2: gracefully abort instead of panicking on unlocked buffer Milos Nikic @ 2026-03-03 0:55 ` Milos Nikic 2026-03-03 6:16 ` kernel test robot 2026-03-03 2:11 ` [PATCH v3 0/2] jbd2: audit and convert legacy J_ASSERT usage yebin (H) 2 siblings, 1 reply; 6+ messages in thread From: Milos Nikic @ 2026-03-03 0:55 UTC (permalink / raw) To: jack; +Cc: tytso, linux-ext4, linux-kernel, Milos Nikic Auditing the jbd2 codebase reveals several legacy J_ASSERT calls that enforce internal state machine invariants (e.g., verifying jh->b_transaction or jh->b_next_transaction pointers). When these invariants are broken, the journal is in a corrupted state. However, triggering a fatal panic brings down the entire system for a localized filesystem error. This patch targets a specific class of these asserts: those residing inside functions that natively return integer error codes, booleans, or error pointers. It replaces the hard J_ASSERTs with WARN_ON_ONCE to capture the offending stack trace, safely drops any held locks, gracefully aborts the journal, and returns -EINVAL. This prevents a catastrophic kernel panic while ensuring the corrupted journal state is safely contained and upstream callers (like ext4 or ocfs2) can gracefully handle the aborted handle. Functions modified in fs/jbd2/transaction.c: - jbd2__journal_start() - do_get_write_access() - jbd2_journal_dirty_metadata() - jbd2_journal_forget() - jbd2_journal_try_to_free_buffers() - jbd2_journal_file_inode() Signed-off-by: Milos Nikic <nikic.milos@gmail.com> --- fs/jbd2/transaction.c | 108 +++++++++++++++++++++++++++++++++--------- 1 file changed, 85 insertions(+), 23 deletions(-) diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c index 04d17a5f2a82..3cb4e524e0a6 100644 --- a/fs/jbd2/transaction.c +++ b/fs/jbd2/transaction.c @@ -474,7 +474,8 @@ handle_t *jbd2__journal_start(journal_t *journal, int nblocks, int rsv_blocks, return ERR_PTR(-EROFS); if (handle) { - J_ASSERT(handle->h_transaction->t_journal == journal); + if (WARN_ON_ONCE(handle->h_transaction->t_journal != journal)) + return ERR_PTR(-EINVAL); handle->h_ref++; return handle; } @@ -1036,7 +1037,13 @@ do_get_write_access(handle_t *handle, struct journal_head *jh, */ if (!jh->b_transaction) { JBUFFER_TRACE(jh, "no transaction"); - J_ASSERT_JH(jh, !jh->b_next_transaction); + if (WARN_ON_ONCE(jh->b_next_transaction)) { + spin_unlock(&jh->b_state_lock); + unlock_buffer(bh); + error = -EINVAL; + jbd2_journal_abort(journal, error); + goto out; + } JBUFFER_TRACE(jh, "file as BJ_Reserved"); /* * Make sure all stores to jh (b_modified, b_frozen_data) are @@ -1069,13 +1076,27 @@ do_get_write_access(handle_t *handle, struct journal_head *jh, */ if (jh->b_frozen_data) { JBUFFER_TRACE(jh, "has frozen data"); - J_ASSERT_JH(jh, jh->b_next_transaction == NULL); + if (WARN_ON_ONCE(jh->b_next_transaction)) { + spin_unlock(&jh->b_state_lock); + error = -EINVAL; + jbd2_journal_abort(journal, error); + goto out; + } goto attach_next; } JBUFFER_TRACE(jh, "owned by older transaction"); - J_ASSERT_JH(jh, jh->b_next_transaction == NULL); - J_ASSERT_JH(jh, jh->b_transaction == journal->j_committing_transaction); + if (WARN_ON_ONCE(jh->b_next_transaction || + jh->b_transaction != + journal->j_committing_transaction)) { + pr_err("JBD2: %s: assertion failure: b_next_transaction=%p b_transaction=%p j_committing_transaction=%p\n", + journal->j_devname, jh->b_next_transaction, + jh->b_transaction, journal->j_committing_transaction); + spin_unlock(&jh->b_state_lock); + error = -EINVAL; + jbd2_journal_abort(journal, error); + goto out; + } /* * There is one case we have to be very careful about. If the @@ -1520,8 +1541,14 @@ int jbd2_journal_dirty_metadata(handle_t *handle, struct buffer_head *bh) if (data_race(jh->b_transaction != transaction && jh->b_next_transaction != transaction)) { spin_lock(&jh->b_state_lock); - J_ASSERT_JH(jh, jh->b_transaction == transaction || - jh->b_next_transaction == transaction); + if (WARN_ON_ONCE(jh->b_transaction != transaction && + jh->b_next_transaction != transaction)) { + pr_err("JBD2: %s: assertion failure: b_transaction=%p transaction=%p b_next_transaction=%p\n", + journal->j_devname, jh->b_transaction, + transaction, jh->b_next_transaction); + ret = -EINVAL; + goto out_unlock_bh; + } spin_unlock(&jh->b_state_lock); } if (data_race(jh->b_modified == 1)) { @@ -1531,13 +1558,15 @@ int jbd2_journal_dirty_metadata(handle_t *handle, struct buffer_head *bh) spin_lock(&jh->b_state_lock); if (jh->b_transaction == transaction && jh->b_jlist != BJ_Metadata) - pr_err("JBD2: assertion failure: h_type=%u " - "h_line_no=%u block_no=%llu jlist=%u\n", + pr_err("JBD2: assertion failure: h_type=%u h_line_no=%u block_no=%llu jlist=%u\n", handle->h_type, handle->h_line_no, (unsigned long long) bh->b_blocknr, jh->b_jlist); - J_ASSERT_JH(jh, jh->b_transaction != transaction || - jh->b_jlist == BJ_Metadata); + if (WARN_ON_ONCE(jh->b_transaction == transaction && + jh->b_jlist != BJ_Metadata)) { + ret = -EINVAL; + goto out_unlock_bh; + } spin_unlock(&jh->b_state_lock); } goto out; @@ -1636,7 +1665,10 @@ int jbd2_journal_dirty_metadata(handle_t *handle, struct buffer_head *bh) } /* That test should have eliminated the following case: */ - J_ASSERT_JH(jh, jh->b_frozen_data == NULL); + if (WARN_ON_ONCE(jh->b_frozen_data)) { + ret = -EINVAL; + goto out_unlock_bh; + } JBUFFER_TRACE(jh, "file as BJ_Metadata"); spin_lock(&journal->j_list_lock); @@ -1675,6 +1707,7 @@ int jbd2_journal_forget(handle_t *handle, struct buffer_head *bh) int err = 0; int was_modified = 0; int wait_for_writeback = 0; + int abort_journal = 0; if (is_handle_aborted(handle)) return -EROFS; @@ -1708,7 +1741,11 @@ int jbd2_journal_forget(handle_t *handle, struct buffer_head *bh) jh->b_modified = 0; if (jh->b_transaction == transaction) { - J_ASSERT_JH(jh, !jh->b_frozen_data); + if (WARN_ON_ONCE(jh->b_frozen_data)) { + err = -EINVAL; + abort_journal = 1; + goto drop; + } /* If we are forgetting a buffer which is already part * of this transaction, then we can just drop it from @@ -1747,8 +1784,11 @@ int jbd2_journal_forget(handle_t *handle, struct buffer_head *bh) } spin_unlock(&journal->j_list_lock); } else if (jh->b_transaction) { - J_ASSERT_JH(jh, (jh->b_transaction == - journal->j_committing_transaction)); + if (WARN_ON_ONCE(jh->b_transaction != journal->j_committing_transaction)) { + err = -EINVAL; + abort_journal = 1; + goto drop; + } /* However, if the buffer is still owned by a prior * (committing) transaction, we can't drop it yet... */ JBUFFER_TRACE(jh, "belongs to older transaction"); @@ -1766,7 +1806,11 @@ int jbd2_journal_forget(handle_t *handle, struct buffer_head *bh) jh->b_next_transaction = transaction; spin_unlock(&journal->j_list_lock); } else { - J_ASSERT(jh->b_next_transaction == transaction); + if (WARN_ON_ONCE(jh->b_next_transaction != transaction)) { + err = -EINVAL; + abort_journal = 1; + goto drop; + } /* * only drop a reference if this transaction modified @@ -1812,6 +1856,8 @@ int jbd2_journal_forget(handle_t *handle, struct buffer_head *bh) drop: __brelse(bh); spin_unlock(&jh->b_state_lock); + if (abort_journal) + jbd2_journal_abort(journal, err); if (wait_for_writeback) wait_on_buffer(bh); jbd2_journal_put_journal_head(jh); @@ -2136,7 +2182,8 @@ bool jbd2_journal_try_to_free_buffers(journal_t *journal, struct folio *folio) struct buffer_head *bh; bool ret = false; - J_ASSERT(folio_test_locked(folio)); + if (WARN_ON_ONCE(!folio_test_locked(folio))) + return false; head = folio_buffers(folio); bh = head; @@ -2651,6 +2698,8 @@ static int jbd2_journal_file_inode(handle_t *handle, struct jbd2_inode *jinode, { transaction_t *transaction = handle->h_transaction; journal_t *journal; + int err = 0; + int abort_transaction = 0; if (is_handle_aborted(handle)) return -EROFS; @@ -2685,20 +2734,33 @@ static int jbd2_journal_file_inode(handle_t *handle, struct jbd2_inode *jinode, /* On some different transaction's list - should be * the committing one */ if (jinode->i_transaction) { - J_ASSERT(jinode->i_next_transaction == NULL); - J_ASSERT(jinode->i_transaction == - journal->j_committing_transaction); + if (WARN_ON_ONCE(jinode->i_next_transaction || + jinode->i_transaction != + journal->j_committing_transaction)) { + pr_err("JBD2: %s: assertion failure: i_next_transaction=%p i_transaction=%p j_committing_transaction=%p\n", + journal->j_devname, jinode->i_next_transaction, + jinode->i_transaction, + journal->j_committing_transaction); + err = -EINVAL; + abort_transaction = 1; + goto done; + } jinode->i_next_transaction = transaction; goto done; } /* Not on any transaction list... */ - J_ASSERT(!jinode->i_next_transaction); + if (WARN_ON_ONCE(jinode->i_next_transaction)) { + err = -EINVAL; + abort_transaction = 1; + goto done; + } jinode->i_transaction = transaction; list_add(&jinode->i_list, &transaction->t_inode_list); done: spin_unlock(&journal->j_list_lock); - - return 0; + if (abort_transaction) + jbd2_journal_abort(journal, err); + return err; } int jbd2_journal_inode_ranged_write(handle_t *handle, -- 2.53.0 ^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH v3 2/2] jbd2: gracefully abort on transaction state corruptions 2026-03-03 0:55 ` [PATCH v3 2/2] jbd2: gracefully abort on transaction state corruptions Milos Nikic @ 2026-03-03 6:16 ` kernel test robot 0 siblings, 0 replies; 6+ messages in thread From: kernel test robot @ 2026-03-03 6:16 UTC (permalink / raw) To: Milos Nikic, jack Cc: llvm, oe-kbuild-all, tytso, linux-ext4, linux-kernel, Milos Nikic Hi Milos, kernel test robot noticed the following build warnings: [auto build test WARNING on brauner-vfs/vfs.all] [also build test WARNING on tytso-ext4/dev next-20260302] [cannot apply to linus/master v6.16-rc1] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Milos-Nikic/jbd2-gracefully-abort-instead-of-panicking-on-unlocked-buffer/20260303-085946 base: https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git vfs.all patch link: https://lore.kernel.org/r/20260303005502.337108-3-nikic.milos%40gmail.com patch subject: [PATCH v3 2/2] jbd2: gracefully abort on transaction state corruptions config: x86_64-kexec (https://download.01.org/0day-ci/archive/20260303/202603030706.NpyA7pqe-lkp@intel.com/config) compiler: clang version 20.1.8 (https://github.com/llvm/llvm-project 87f0227cb60147a26a1eeb4fb06e3b505e9c7261) reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20260303/202603030706.NpyA7pqe-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202603030706.NpyA7pqe-lkp@intel.com/ All warnings (new ones prefixed by >>): >> fs/jbd2/transaction.c:1547:11: warning: variable 'journal' is uninitialized when used here [-Wuninitialized] 1547 | journal->j_devname, jh->b_transaction, | ^~~~~~~ include/linux/printk.h:554:33: note: expanded from macro 'pr_err' 554 | printk(KERN_ERR pr_fmt(fmt), ##__VA_ARGS__) | ^~~~~~~~~~~ include/linux/printk.h:511:60: note: expanded from macro 'printk' 511 | #define printk(fmt, ...) printk_index_wrap(_printk, fmt, ##__VA_ARGS__) | ^~~~~~~~~~~ include/linux/printk.h:483:19: note: expanded from macro 'printk_index_wrap' 483 | _p_func(_fmt, ##__VA_ARGS__); \ | ^~~~~~~~~~~ fs/jbd2/transaction.c:1520:20: note: initialize the variable 'journal' to silence this warning 1520 | journal_t *journal; | ^ | = NULL 1 warning generated. vim +/journal +1547 fs/jbd2/transaction.c 1493 1494 /** 1495 * jbd2_journal_dirty_metadata() - mark a buffer as containing dirty metadata 1496 * @handle: transaction to add buffer to. 1497 * @bh: buffer to mark 1498 * 1499 * mark dirty metadata which needs to be journaled as part of the current 1500 * transaction. 1501 * 1502 * The buffer must have previously had jbd2_journal_get_write_access() 1503 * called so that it has a valid journal_head attached to the buffer 1504 * head. 1505 * 1506 * The buffer is placed on the transaction's metadata list and is marked 1507 * as belonging to the transaction. 1508 * 1509 * Returns error number or 0 on success. 1510 * 1511 * Special care needs to be taken if the buffer already belongs to the 1512 * current committing transaction (in which case we should have frozen 1513 * data present for that commit). In that case, we don't relink the 1514 * buffer: that only gets done when the old transaction finally 1515 * completes its commit. 1516 */ 1517 int jbd2_journal_dirty_metadata(handle_t *handle, struct buffer_head *bh) 1518 { 1519 transaction_t *transaction = handle->h_transaction; 1520 journal_t *journal; 1521 struct journal_head *jh; 1522 int ret = 0; 1523 1524 if (!buffer_jbd(bh)) 1525 return -EUCLEAN; 1526 1527 /* 1528 * We don't grab jh reference here since the buffer must be part 1529 * of the running transaction. 1530 */ 1531 jh = bh2jh(bh); 1532 jbd2_debug(5, "journal_head %p\n", jh); 1533 JBUFFER_TRACE(jh, "entry"); 1534 1535 /* 1536 * This and the following assertions are unreliable since we may see jh 1537 * in inconsistent state unless we grab bh_state lock. But this is 1538 * crucial to catch bugs so let's do a reliable check until the 1539 * lockless handling is fully proven. 1540 */ 1541 if (data_race(jh->b_transaction != transaction && 1542 jh->b_next_transaction != transaction)) { 1543 spin_lock(&jh->b_state_lock); 1544 if (WARN_ON_ONCE(jh->b_transaction != transaction && 1545 jh->b_next_transaction != transaction)) { 1546 pr_err("JBD2: %s: assertion failure: b_transaction=%p transaction=%p b_next_transaction=%p\n", > 1547 journal->j_devname, jh->b_transaction, 1548 transaction, jh->b_next_transaction); 1549 ret = -EINVAL; 1550 goto out_unlock_bh; 1551 } 1552 spin_unlock(&jh->b_state_lock); 1553 } 1554 if (data_race(jh->b_modified == 1)) { 1555 /* If it's in our transaction it must be in BJ_Metadata list. */ 1556 if (data_race(jh->b_transaction == transaction && 1557 jh->b_jlist != BJ_Metadata)) { 1558 spin_lock(&jh->b_state_lock); 1559 if (jh->b_transaction == transaction && 1560 jh->b_jlist != BJ_Metadata) 1561 pr_err("JBD2: assertion failure: h_type=%u h_line_no=%u block_no=%llu jlist=%u\n", 1562 handle->h_type, handle->h_line_no, 1563 (unsigned long long) bh->b_blocknr, 1564 jh->b_jlist); 1565 if (WARN_ON_ONCE(jh->b_transaction == transaction && 1566 jh->b_jlist != BJ_Metadata)) { 1567 ret = -EINVAL; 1568 goto out_unlock_bh; 1569 } 1570 spin_unlock(&jh->b_state_lock); 1571 } 1572 goto out; 1573 } 1574 1575 spin_lock(&jh->b_state_lock); 1576 1577 if (is_handle_aborted(handle)) { 1578 /* 1579 * Check journal aborting with @jh->b_state_lock locked, 1580 * since 'jh->b_transaction' could be replaced with 1581 * 'jh->b_next_transaction' during old transaction 1582 * committing if journal aborted, which may fail 1583 * assertion on 'jh->b_frozen_data == NULL'. 1584 */ 1585 ret = -EROFS; 1586 goto out_unlock_bh; 1587 } 1588 1589 journal = transaction->t_journal; 1590 1591 if (jh->b_modified == 0) { 1592 /* 1593 * This buffer's got modified and becoming part 1594 * of the transaction. This needs to be done 1595 * once a transaction -bzzz 1596 */ 1597 if (WARN_ON_ONCE(jbd2_handle_buffer_credits(handle) <= 0)) { 1598 ret = -ENOSPC; 1599 goto out_unlock_bh; 1600 } 1601 jh->b_modified = 1; 1602 handle->h_total_credits--; 1603 } 1604 1605 /* 1606 * fastpath, to avoid expensive locking. If this buffer is already 1607 * on the running transaction's metadata list there is nothing to do. 1608 * Nobody can take it off again because there is a handle open. 1609 * I _think_ we're OK here with SMP barriers - a mistaken decision will 1610 * result in this test being false, so we go in and take the locks. 1611 */ 1612 if (jh->b_transaction == transaction && jh->b_jlist == BJ_Metadata) { 1613 JBUFFER_TRACE(jh, "fastpath"); 1614 if (unlikely(jh->b_transaction != 1615 journal->j_running_transaction)) { 1616 printk(KERN_ERR "JBD2: %s: " 1617 "jh->b_transaction (%llu, %p, %u) != " 1618 "journal->j_running_transaction (%p, %u)\n", 1619 journal->j_devname, 1620 (unsigned long long) bh->b_blocknr, 1621 jh->b_transaction, 1622 jh->b_transaction ? jh->b_transaction->t_tid : 0, 1623 journal->j_running_transaction, 1624 journal->j_running_transaction ? 1625 journal->j_running_transaction->t_tid : 0); 1626 ret = -EINVAL; 1627 } 1628 goto out_unlock_bh; 1629 } 1630 1631 set_buffer_jbddirty(bh); 1632 1633 /* 1634 * Metadata already on the current transaction list doesn't 1635 * need to be filed. Metadata on another transaction's list must 1636 * be committing, and will be refiled once the commit completes: 1637 * leave it alone for now. 1638 */ 1639 if (jh->b_transaction != transaction) { 1640 JBUFFER_TRACE(jh, "already on other transaction"); 1641 if (unlikely(((jh->b_transaction != 1642 journal->j_committing_transaction)) || 1643 (jh->b_next_transaction != transaction))) { 1644 printk(KERN_ERR "jbd2_journal_dirty_metadata: %s: " 1645 "bad jh for block %llu: " 1646 "transaction (%p, %u), " 1647 "jh->b_transaction (%p, %u), " 1648 "jh->b_next_transaction (%p, %u), jlist %u\n", 1649 journal->j_devname, 1650 (unsigned long long) bh->b_blocknr, 1651 transaction, transaction->t_tid, 1652 jh->b_transaction, 1653 jh->b_transaction ? 1654 jh->b_transaction->t_tid : 0, 1655 jh->b_next_transaction, 1656 jh->b_next_transaction ? 1657 jh->b_next_transaction->t_tid : 0, 1658 jh->b_jlist); 1659 WARN_ON(1); 1660 ret = -EINVAL; 1661 } 1662 /* And this case is illegal: we can't reuse another 1663 * transaction's data buffer, ever. */ 1664 goto out_unlock_bh; 1665 } 1666 1667 /* That test should have eliminated the following case: */ 1668 if (WARN_ON_ONCE(jh->b_frozen_data)) { 1669 ret = -EINVAL; 1670 goto out_unlock_bh; 1671 } 1672 1673 JBUFFER_TRACE(jh, "file as BJ_Metadata"); 1674 spin_lock(&journal->j_list_lock); 1675 __jbd2_journal_file_buffer(jh, transaction, BJ_Metadata); 1676 spin_unlock(&journal->j_list_lock); 1677 out_unlock_bh: 1678 spin_unlock(&jh->b_state_lock); 1679 out: 1680 JBUFFER_TRACE(jh, "exit"); 1681 return ret; 1682 } 1683 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH v3 0/2] jbd2: audit and convert legacy J_ASSERT usage 2026-03-03 0:55 [PATCH v3 0/2] jbd2: audit and convert legacy J_ASSERT usage Milos Nikic 2026-03-03 0:55 ` [PATCH v3 1/2] jbd2: gracefully abort instead of panicking on unlocked buffer Milos Nikic 2026-03-03 0:55 ` [PATCH v3 2/2] jbd2: gracefully abort on transaction state corruptions Milos Nikic @ 2026-03-03 2:11 ` yebin (H) 2 siblings, 0 replies; 6+ messages in thread From: yebin (H) @ 2026-03-03 2:11 UTC (permalink / raw) To: jack; +Cc: tytso, linux-ext4, linux-kernel The macro `J_ASSERT_JH` is a rather troublesome implementation. There are numerous calls to `J_ASSERT_JH` within `jbd2_journal_commit_transaction()`, and after compilation, these may all jump to the same address for execution, making it difficult to determine exactly where the assertion is being triggered. If there is a functional issue in just a single file system, using `BUG_ON` to handle it seems a bit too aggressive. I wonder if you all have any good ideas or suggestions. On 2026/3/3 8:55, Milos Nikic wrote: > Hello Jan and the ext4 team, > > This patch series follows up on the previous discussion regarding > converting hard J_ASSERT panics into graceful journal aborts. > > In v1, we addressed a specific panic on unlock. Per Jan's suggestion, > I have audited fs/jbd2/transaction.c for other low-hanging fruit > where state machine invariants are enforced by J_ASSERT inside > functions that natively support error returns. > > Changes in v3: > > Patch 2: Added pr_err() statements inside the ambiguous WARN_ON_ONCE() > blocks (where multiple conditions are checked via logical OR/AND) to > explicitly dump the b_transaction, b_next_transaction, and > j_committing_transaction pointers. This provides necessary context for > debugging state machine corruptions from the dmesg stack trace. > > Changes in v2: > > Patch 1: Unmodified from v1. Collected Reviewed-by tags. > > Patch 2: New patch resulting from the broader audit. Systematically > replaces J_ASSERTs with WARN_ON_ONCE and graceful -EINVAL returns > across 6 core transaction lifecycle functions. Careful attention was > paid to ensuring spinlocks are safely dropped before triggering > jbd2_journal_abort(), and no memory is leaked on the error paths. > > Milos Nikic (2): > jbd2: gracefully abort instead of panicking on unlocked buffer > jbd2: gracefully abort on transaction state corruptions > > fs/jbd2/transaction.c | 115 +++++++++++++++++++++++++++++++++--------- > 1 file changed, 91 insertions(+), 24 deletions(-) > ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2026-03-03 6:17 UTC | newest] Thread overview: 6+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2026-03-03 0:55 [PATCH v3 0/2] jbd2: audit and convert legacy J_ASSERT usage Milos Nikic 2026-03-03 0:55 ` [PATCH v3 1/2] jbd2: gracefully abort instead of panicking on unlocked buffer Milos Nikic 2026-03-03 1:05 ` Andreas Dilger 2026-03-03 0:55 ` [PATCH v3 2/2] jbd2: gracefully abort on transaction state corruptions Milos Nikic 2026-03-03 6:16 ` kernel test robot 2026-03-03 2:11 ` [PATCH v3 0/2] jbd2: audit and convert legacy J_ASSERT usage yebin (H)
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.