Hi Joseph,
On 2018/3/30 10:04, Joseph Qi wrote:
>
>
> On 18/3/30 09:31, Changwei Ge wrote:
>> Hi Joseph,
>>
>> On 2018/3/30 9:27, Joseph Qi wrote:
>>>
>>>
>>> On 18/3/29 10:06, Changwei Ge wrote:
>>>> ocfs2_read_blocks() is used to read several blocks from disk.
>>>> Currently, the input argument *bhs* can be NULL or NOT. It depends on
>>>> the caller's behavior. If the function fails in reading blocks from
>>>> disk, the corresponding bh will be assigned to NULL and put.
>>>>
>>>> Obviously, above process for non-NULL input bh is not appropriate.
>>>> Because the caller doesn't even know its bhs are put and re-assigned.
>>>>
>>>> If buffer head is managed by caller, ocfs2_read_blocks should not
>>>> evaluate it to NULL. It will cause caller accessing illegal memory,
>>>> thus crash.
>>>>
>>>> Signed-off-by: Changwei Ge <ge.chang...@h3c.com>
>>>> ---
>>>> fs/ocfs2/buffer_head_io.c | 31 +++++++++++++++++++++++++------
>>>> 1 file changed, 25 insertions(+), 6 deletions(-)
>>>>
>>>> diff --git a/fs/ocfs2/buffer_head_io.c b/fs/ocfs2/buffer_head_io.c
>>>> index d9ebe11..17329b6 100644
>>>> --- a/fs/ocfs2/buffer_head_io.c
>>>> +++ b/fs/ocfs2/buffer_head_io.c
>>>> @@ -188,6 +188,7 @@ int ocfs2_read_blocks(struct ocfs2_caching_info *ci,
>>>> u64 block, int nr,
>>>> int i, ignore_cache = 0;
>>>> struct buffer_head *bh;
>>>> struct super_block *sb = ocfs2_metadata_cache_get_super(ci);
>>>> + int new_bh = 0;
>>>>
>>>> trace_ocfs2_read_blocks_begin(ci, (unsigned long long)block,
>>>> nr, flags);
>>>>
>>>> @@ -213,6 +214,18 @@ int ocfs2_read_blocks(struct ocfs2_caching_info *ci,
>>>> u64 block, int nr,
>>>> goto bail;
>>>> }
>>>>
>>>> + /* Use below trick to check if all bhs are NULL or assigned.
>>>> + * Basically, we hope all bhs are consistent so that we can
>>>> + * handle exception easily.
>>>> + */
>>>> + new_bh = (bhs[0] == NULL);
>>>> + for (i = 1 ; i < nr ; i++) {
>>>> + if ((new_bh && bhs[i]) || (!new_bh && !bhs[i])) {
>>>> + WARN(1, "Not all bhs are consistent\n");
>>>> + break;
>>>> + }
>>>> + }
>>>> +
>>>> ocfs2_metadata_cache_io_lock(ci);
>>>> for (i = 0 ; i < nr ; i++) {
>>>> if (bhs[i] == NULL) {
>>>> @@ -324,8 +337,10 @@ int ocfs2_read_blocks(struct ocfs2_caching_info *ci,
>>>> u64 block, int nr,
>>>> if (!(flags & OCFS2_BH_READAHEAD)) {
>>>> if (status) {
>>>> /* Clear the rest of the buffers on
>>>> error */
>>>> - put_bh(bh);
>>>> - bhs[i] = NULL;
>>>> + if (new_bh) {
>>>> + put_bh(bh);
>>>> + bhs[i] = NULL;
>>>> + }
>>>
>>> Since we assume caller has to pass either all NULL or all non-NULL,
>>> here we will only put bh internal allocated. Am I missing something?
>>
>> Thanks for your review.
>> Yes, we will only put bh internally allocated.
>> If bh is reserved in advance, we will not put it and re-assign it to NULL.
>>
>
> So this branch won't have risk, right?
Sorry... I'm not sure if I understand you correctly.
This branch will be walked through when previous part of bhs[] faces a read
failure in order to put bh allocated in ocfs2_read_blocks().
And we assume all bh should be NULL or non-NULL, if new_bh is set, the back
part
should also be put to release those buffer heads.
If I made a mistake or misunderstand you, please let me know.
Thanks,
Changwei
>
> Thanks,
> Joseph
>
_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel@oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel
