Re: [PATCH v2 3/5] btrfs: Add lockdep models for the transaction states wait events

[Ioannis Angelakopoulos <iangelak@fb.com>]

On 7/20/22 7:47 AM, Sweet Tea Dorminy wrote:
> 
> 
> On 7/19/22 00:09, Ioannis Angelakopoulos wrote:
>> Add a lockdep annotation for the transaction states that have wait
>> events; 1) TRANS_STATE_COMMIT_START, 2) TRANS_STATE_UNBLOCKED, 3)
>> TRANS_STATE_SUPER_COMMITTED, and 4) TRANS_STATE_COMPLETED in
>> fs/btrfs/transaction.c.
>>
>> With the exception of the lockdep annotation for TRANS_STATE_COMMIT_START
>> the transaction thread has to acquire the lockdep maps for the 
>> transaction
>> states as reader after the lockdep map for num_writers is released so 
>> that
>> lockdep does not complain.
> 
> "acquire the lockdep maps for the transaction states as reader ..." took 
> a couple readings for me to grasp. Maybe "get a notional read lock on 
> the transaction state only after releasing the notional lock on 
> num_writers, as lockdep requires locks to be acquired/released in 
> [somehow]". (I don't know how lockdep complains, but I think saying why 
> in this commit message would help me understand the 'why' better.)
>
I will make the comment here or the one below more detailed. Yes it has 
to do with the lock ordering. As it is, the thread has first acquired 
the btrfs_trans_num_writers lock. In case it acquired the state locks 
before releasing the btrfs_trans_num_writers lock, it would first 
release the btrfs_trans_num_writers lock and then the state locks. Thus 
lockdep would complain about a potential deadlock, since locks should be 
released in the reverse order they are acquired.
>> @@ -2323,6 +2344,15 @@ int btrfs_commit_transaction(struct 
>> btrfs_trans_handle *trans)
>>       wait_event(cur_trans->writer_wait,
>>              atomic_read(&cur_trans->num_writers) == 1);
>> +    /*
>> +     * Make lockdep happy by acquiring the state locks after
>> +     * btrfs_trans_num_writers is released.
>> +     */
> 
> Same sort of comment here: elaborating on why it makes lockdep happy 
> would help me understand this better.