On 11/27/2015 04:45 PM, Jan Kara wrote:
> On Fri 27-11-15 09:57:21, Junxiao Bi wrote:
>> commit de92c8c ("jbd2: speedup jbd2_journal_get_[write|undo]_access()")
>> introduced jbd2_write_access_granted() to improve write|undo_access
>> speed, but missed to check the status of b_committed_data which caused
>> a kernel panic on ocfs2.
>
> Thanks for debugging this! Just a few minor nits:
>
> 1) Please use at least 12 characters of the commit hash - i.e. de92c8caf16c
> - that is pretty much guaranteed to stay unique for the lifetime of Linux.
> Using just 7 characters will become non-unique with high probability rather
> soon.
I saw this report from checkpatch.pl. Since there have "patch subject",
so i didn't modify it. I will update it in v2 patch.
>
> 2) Send this patch to Ted Tso as well as he is the one merging jbd2
> patches.
OK.
>
> 3) The fact that OCFS2 hit this problem means that it is mixing
> jbd2_journal_get_write_access() and jbd2_journal_get_undo_access() for the
> same buffer. That is slightly suspicious to me. Not that it would be
> outright bug but you have to account for the fact that someone can be
> modifying the buffer data while you are getting undo access...
I am not sure about this. Mark involved this in commit b4414eea0e
("ocfs2: Clear undo bits when local alloc is freed").
Mark, is mixing using undo/write access a bug?
>
> 4) Some other comments below in the code.
>
>> diff --git a/fs/jbd2/transaction.c b/fs/jbd2/transaction.c
>> index 6b8338e..4750bda 100644
>> --- a/fs/jbd2/transaction.c
>> +++ b/fs/jbd2/transaction.c
>> @@ -1009,7 +1009,8 @@ out:
>> }
>>
>> /* Fast check whether buffer is already attached to the required transaction */
>> -static bool jbd2_write_access_granted(handle_t *handle, struct buffer_head *bh)
>> +static bool jbd2_write_access_granted(handle_t *handle, struct buffer_head *bh,
>> + int undo)
> ^^^
> Use 'bool' please. The function is already using bools so it is good for
> consistency.
Will fix this in v2.
Thanks,
Junxiao.
>
>> {
>> struct journal_head *jh;
>> bool ret = false;
>> @@ -1036,6 +1037,8 @@ static bool jbd2_write_access_granted(handle_t *handle, struct buffer_head *bh)
>> jh = READ_ONCE(bh->b_private);
>> if (!jh)
>> goto out;
>
> Short comment here please. Like:
>
> /* For undo access buffer must have data copied */
>
>> + if (undo && !jh->b_committed_data)
>> + goto out;
>> if (jh->b_transaction != handle->h_transaction &&
>> jh->b_next_transaction != handle->h_transaction)
>> goto out;
>> @@ -1073,7 +1076,7 @@ int jbd2_journal_get_write_access(handle_t *handle, struct buffer_head *bh)
>> struct journal_head *jh;
>> int rc;
>>
>> - if (jbd2_write_access_granted(handle, bh))
>> + if (jbd2_write_access_granted(handle, bh, 0))
>
> Here 'false' instead of 0.
>
>> JBUFFER_TRACE(jh, "entry");
>> - if (jbd2_write_access_granted(handle, bh))
>> + if (jbd2_write_access_granted(handle, bh, 1))
>
> Here 'true' instead of 1.
>
> Thanks.
>
> Honza
>
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
