diff mbox

ocfs2: do not put bh when buffer_uptodate failed

Message ID 53312AD6.4080607@huawei.com
State New, archived
Headers show

Commit Message

AlexChen March 25, 2014, 7:05 a.m. UTC
Do not put bh when buffer_uptodate failed in ocfs2_write_block and
ocfs2_write_super_or_backup, because it will put bh in b_end_io.
Otherwise it will hit a warning "VFS: brelse: Trying to free free
buffer".

Signed-off-by: Alex Chen <alex.chen@huawei.com>
Reviewed-by: Joseph Qi <joseph.qi@huawei.com>
---
 fs/ocfs2/buffer_head_io.c | 2 --
 1 file changed, 2 deletions(-)

Comments

Srinivas Eeda March 25, 2014, 6:45 p.m. UTC | #1
These changes looks good to me. However ocfs2_read_blocks and 
ocfs2_read_blocks_sync needs the same fix ? :)

On 03/25/2014 12:05 AM, alex chen wrote:
> Do not put bh when buffer_uptodate failed in ocfs2_write_block and
> ocfs2_write_super_or_backup, because it will put bh in b_end_io.
> Otherwise it will hit a warning "VFS: brelse: Trying to free free
> buffer".
>
> Signed-off-by: Alex Chen <alex.chen@huawei.com>
> Reviewed-by: Joseph Qi <joseph.qi@huawei.com>
> ---
>   fs/ocfs2/buffer_head_io.c | 2 --
>   1 file changed, 2 deletions(-)
>
> diff --git a/fs/ocfs2/buffer_head_io.c b/fs/ocfs2/buffer_head_io.c
> index 5b704c6..1edcb14 100644
> --- a/fs/ocfs2/buffer_head_io.c
> +++ b/fs/ocfs2/buffer_head_io.c
> @@ -90,7 +90,6 @@ int ocfs2_write_block(struct ocfs2_super *osb, struct buffer_head *bh,
>   		 * information for this bh as it's not marked locally
>   		 * uptodate. */
>   		ret = -EIO;
> -		put_bh(bh);
>   		mlog_errno(ret);
>   	}
>
> @@ -420,7 +419,6 @@ int ocfs2_write_super_or_backup(struct ocfs2_super *osb,
>
>   	if (!buffer_uptodate(bh)) {
>   		ret = -EIO;
> -		put_bh(bh);
>   		mlog_errno(ret);
>   	}
>
AlexChen March 26, 2014, 1:25 a.m. UTC | #2
On 2014/3/26 2:45, Srinivas Eeda wrote:
> These changes looks good to me. However ocfs2_read_blocks and 
> ocfs2_read_blocks_sync needs the same fix ? :)

There is no need to do this in ocfs2_read_blocks and
ocfs2_read_blocks_sync, because bh will be set to NULL after put_bh,
and brelse will handle it.

> 
> On 03/25/2014 12:05 AM, alex chen wrote:
>> Do not put bh when buffer_uptodate failed in ocfs2_write_block and
>> ocfs2_write_super_or_backup, because it will put bh in b_end_io.
>> Otherwise it will hit a warning "VFS: brelse: Trying to free free
>> buffer".
>>
>> Signed-off-by: Alex Chen <alex.chen@huawei.com>
>> Reviewed-by: Joseph Qi <joseph.qi@huawei.com>
>> ---
>>   fs/ocfs2/buffer_head_io.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
>> diff --git a/fs/ocfs2/buffer_head_io.c b/fs/ocfs2/buffer_head_io.c
>> index 5b704c6..1edcb14 100644
>> --- a/fs/ocfs2/buffer_head_io.c
>> +++ b/fs/ocfs2/buffer_head_io.c
>> @@ -90,7 +90,6 @@ int ocfs2_write_block(struct ocfs2_super *osb, struct buffer_head *bh,
>>   		 * information for this bh as it's not marked locally
>>   		 * uptodate. */
>>   		ret = -EIO;
>> -		put_bh(bh);
>>   		mlog_errno(ret);
>>   	}
>>
>> @@ -420,7 +419,6 @@ int ocfs2_write_super_or_backup(struct ocfs2_super *osb,
>>
>>   	if (!buffer_uptodate(bh)) {
>>   		ret = -EIO;
>> -		put_bh(bh);
>>   		mlog_errno(ret);
>>   	}
>>
> 
> 
> _______________________________________________
> Ocfs2-devel mailing list
> Ocfs2-devel@oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/ocfs2-devel
> 
>
Srinivas Eeda March 26, 2014, 5:10 p.m. UTC | #3
Thanks for explaining
Reviewed-by: Srinivas Eeda <srinivas.eeda@oracle.com>

On 03/25/2014 06:25 PM, alex chen wrote:
> On 2014/3/26 2:45, Srinivas Eeda wrote:
>> These changes looks good to me. However ocfs2_read_blocks and
>> ocfs2_read_blocks_sync needs the same fix ? :)
> There is no need to do this in ocfs2_read_blocks and
> ocfs2_read_blocks_sync, because bh will be set to NULL after put_bh,
> and brelse will handle it.
>
>> On 03/25/2014 12:05 AM, alex chen wrote:
>>> Do not put bh when buffer_uptodate failed in ocfs2_write_block and
>>> ocfs2_write_super_or_backup, because it will put bh in b_end_io.
>>> Otherwise it will hit a warning "VFS: brelse: Trying to free free
>>> buffer".
>>>
>>> Signed-off-by: Alex Chen <alex.chen@huawei.com>
>>> Reviewed-by: Joseph Qi <joseph.qi@huawei.com>
>>> ---
>>>    fs/ocfs2/buffer_head_io.c | 2 --
>>>    1 file changed, 2 deletions(-)
>>>
>>> diff --git a/fs/ocfs2/buffer_head_io.c b/fs/ocfs2/buffer_head_io.c
>>> index 5b704c6..1edcb14 100644
>>> --- a/fs/ocfs2/buffer_head_io.c
>>> +++ b/fs/ocfs2/buffer_head_io.c
>>> @@ -90,7 +90,6 @@ int ocfs2_write_block(struct ocfs2_super *osb, struct buffer_head *bh,
>>>    		 * information for this bh as it's not marked locally
>>>    		 * uptodate. */
>>>    		ret = -EIO;
>>> -		put_bh(bh);
>>>    		mlog_errno(ret);
>>>    	}
>>>
>>> @@ -420,7 +419,6 @@ int ocfs2_write_super_or_backup(struct ocfs2_super *osb,
>>>
>>>    	if (!buffer_uptodate(bh)) {
>>>    		ret = -EIO;
>>> -		put_bh(bh);
>>>    		mlog_errno(ret);
>>>    	}
>>>
>>
>> _______________________________________________
>> Ocfs2-devel mailing list
>> Ocfs2-devel@oss.oracle.com
>> https://oss.oracle.com/mailman/listinfo/ocfs2-devel
>>
>>
Joel Becker March 27, 2014, 2:22 a.m. UTC | #4
On Tue, Mar 25, 2014 at 03:05:58PM +0800, alex chen wrote:
> Do not put bh when buffer_uptodate failed in ocfs2_write_block and
> ocfs2_write_super_or_backup, because it will put bh in b_end_io.
> Otherwise it will hit a warning "VFS: brelse: Trying to free free
> buffer".
> 
> Signed-off-by: Alex Chen <alex.chen@huawei.com>
> Reviewed-by: Joseph Qi <joseph.qi@huawei.com>

Good catch.  Can you tell me what testing or workload found this issue?
Just for future reference.

Acked-by: Joel Becker <jlbec@evilplan.org>

> ---
>  fs/ocfs2/buffer_head_io.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/fs/ocfs2/buffer_head_io.c b/fs/ocfs2/buffer_head_io.c
> index 5b704c6..1edcb14 100644
> --- a/fs/ocfs2/buffer_head_io.c
> +++ b/fs/ocfs2/buffer_head_io.c
> @@ -90,7 +90,6 @@ int ocfs2_write_block(struct ocfs2_super *osb, struct buffer_head *bh,
>  		 * information for this bh as it's not marked locally
>  		 * uptodate. */
>  		ret = -EIO;
> -		put_bh(bh);
>  		mlog_errno(ret);
>  	}
> 
> @@ -420,7 +419,6 @@ int ocfs2_write_super_or_backup(struct ocfs2_super *osb,
> 
>  	if (!buffer_uptodate(bh)) {
>  		ret = -EIO;
> -		put_bh(bh);
>  		mlog_errno(ret);
>  	}
> 
> -- 
> 1.8.4.3
> 
> 
> _______________________________________________
> Ocfs2-devel mailing list
> Ocfs2-devel@oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/ocfs2-devel
AlexChen March 28, 2014, 1:33 a.m. UTC | #5
On 2014/3/27 10:22, Joel Becker wrote:
> On Tue, Mar 25, 2014 at 03:05:58PM +0800, alex chen wrote:
>> Do not put bh when buffer_uptodate failed in ocfs2_write_block and
>> ocfs2_write_super_or_backup, because it will put bh in b_end_io.
>> Otherwise it will hit a warning "VFS: brelse: Trying to free free
>> buffer".
>>
>> Signed-off-by: Alex Chen <alex.chen@huawei.com>
>> Reviewed-by: Joseph Qi <joseph.qi@huawei.com>
> 
> Good catch.  Can you tell me what testing or workload found this issue?
> Just for future reference.
> 
> Acked-by: Joel Becker <jlbec@evilplan.org>
> 

This issue was found when resizing volume. During the resize, storage
link was not steady and continuing up and down. Uptodate buffer failed
because of EIO in ocfs2_write_super_or_backup and then this warning
occurs.

>> ---
>>  fs/ocfs2/buffer_head_io.c | 2 --
>>  1 file changed, 2 deletions(-)
>>
>> diff --git a/fs/ocfs2/buffer_head_io.c b/fs/ocfs2/buffer_head_io.c
>> index 5b704c6..1edcb14 100644
>> --- a/fs/ocfs2/buffer_head_io.c
>> +++ b/fs/ocfs2/buffer_head_io.c
>> @@ -90,7 +90,6 @@ int ocfs2_write_block(struct ocfs2_super *osb, struct buffer_head *bh,
>>  		 * information for this bh as it's not marked locally
>>  		 * uptodate. */
>>  		ret = -EIO;
>> -		put_bh(bh);
>>  		mlog_errno(ret);
>>  	}
>>
>> @@ -420,7 +419,6 @@ int ocfs2_write_super_or_backup(struct ocfs2_super *osb,
>>
>>  	if (!buffer_uptodate(bh)) {
>>  		ret = -EIO;
>> -		put_bh(bh);
>>  		mlog_errno(ret);
>>  	}
>>
>> -- 
>> 1.8.4.3
>>
>>
>> _______________________________________________
>> Ocfs2-devel mailing list
>> Ocfs2-devel@oss.oracle.com
>> https://oss.oracle.com/mailman/listinfo/ocfs2-devel
>
diff mbox

Patch

diff --git a/fs/ocfs2/buffer_head_io.c b/fs/ocfs2/buffer_head_io.c
index 5b704c6..1edcb14 100644
--- a/fs/ocfs2/buffer_head_io.c
+++ b/fs/ocfs2/buffer_head_io.c
@@ -90,7 +90,6 @@  int ocfs2_write_block(struct ocfs2_super *osb, struct buffer_head *bh,
 		 * information for this bh as it's not marked locally
 		 * uptodate. */
 		ret = -EIO;
-		put_bh(bh);
 		mlog_errno(ret);
 	}

@@ -420,7 +419,6 @@  int ocfs2_write_super_or_backup(struct ocfs2_super *osb,

 	if (!buffer_uptodate(bh)) {
 		ret = -EIO;
-		put_bh(bh);
 		mlog_errno(ret);
 	}