Re: [PATCH] ocfs2: fix journal protection issues

[Joseph Qi <joseph.qi@linux.alibaba.com>]

On 3/15/24 10:25 AM, Heming Zhao wrote:
> On 3/15/24 09:09, Joseph Qi wrote:
>> Hi,
>>
>> On 3/14/24 3:33 PM, Heming Zhao wrote:
>>> This patch resolves 3 journal-related issues:
>>> 1. ocfs2_rollback_alloc_dinode_counts() forgets to provide journal
>>>     protection for modifying dinode.
>>> 2. ocfs2_block_group_alloc() & ocfs2_group_add() forget to provide
>>>     journal protection for writing fe->i_size.
>>> 3. adjusted journal_dirty scope for ocfs2_update_last_group_and_inode().
>>>     This adjustment ensures that only content requiring protection is
>>>     included in the journal.
>>>
>>
>> Any real issue you've found for above cases?
> 
> No. I just found these issues when I was reading code.
>>
>> Take ocfs2_rollback_alloc_dinode_counts() for example, it will always be
>> pair used with ocfs2_alloc_dinode_update_counts(), more precisely, in
>> the error case of ocfs2_block_group_set_bits(). And the 'di_bh' is
>> already dirtied before. Since they are in the same transaction, I don't
>> see any problem here.
> 
> Yes, they share the same transaction.
> However:
> In ocfs2_alloc_dinode_update_counts(), there already uses a jbd2 access/dirty
> pair to protect metadata. In my view, jbd2_journal_access starts the
> protection stage, jbd2_journal_dirty closes the protection stage. the rollback
> function should start a new access/dirty pair to info jbd2 the di_bh had been
> modified again.
> 
Refer jbd2_journal_dirty_metadata():

/*
 * fastpath, to avoid expensive locking.  If this buffer is already
 * on the running transaction's metadata list there is nothing to do.
 * Nobody can take it off again because there is a handle open.
 * I _think_ we're OK here with SMP barriers - a mistaken decision will
 * result in this test being false, so we go in and take the locks.
 */

So IMO, we don't have to access/dirty again since the buffer head is
already added before.

Joseph