ocfs2: free up write context when direct IO failed
diff mbox series

Message ID 20181101211531.21036-1-wen.gang.wang@oracle.com
State New
Headers show
Series
  • ocfs2: free up write context when direct IO failed
Related show

Commit Message

Wengang Nov. 1, 2018, 9:15 p.m. UTC
The write context should also be freed even when direct IO failed.
Otherwise a memory leak is introduced and entries remain in
oi->ip_unwritten_list causing the following BUG later in unlink path:

ERROR: bug expression: !list_empty(&oi->ip_unwritten_list)
ERROR: Clear inode of 215043, inode has unwritten extents
...
Call Trace:
? __set_current_blocked+0x42/0x68
ocfs2_evict_inode+0x91/0x6a0 [ocfs2]
? bit_waitqueue+0x40/0x33
evict+0xdb/0x1af
iput+0x1a2/0x1f7
do_unlinkat+0x194/0x28f
SyS_unlinkat+0x1b/0x2f
do_syscall_64+0x79/0x1ae
entry_SYSCALL_64_after_hwframe+0x151/0x0

Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
---
 fs/ocfs2/aops.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Comments

Changwei Ge Nov. 2, 2018, 12:51 a.m. UTC | #1
Hello Wengang,

The fix looks sane to me.

On 2018/11/2 5:17, Wengang Wang wrote:
> The write context should also be freed even when direct IO failed.
> Otherwise a memory leak is introduced and entries remain in
> oi->ip_unwritten_list causing the following BUG later in unlink path:
> 
> ERROR: bug expression: !list_empty(&oi->ip_unwritten_list)
> ERROR: Clear inode of 215043, inode has unwritten extents
> ...
> Call Trace:
> ? __set_current_blocked+0x42/0x68
> ocfs2_evict_inode+0x91/0x6a0 [ocfs2]
> ? bit_waitqueue+0x40/0x33
> evict+0xdb/0x1af
> iput+0x1a2/0x1f7
> do_unlinkat+0x194/0x28f
> SyS_unlinkat+0x1b/0x2f
> do_syscall_64+0x79/0x1ae
> entry_SYSCALL_64_after_hwframe+0x151/0x0
> 
> Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>

Reviewed-by: Changwei Ge <ge.changwei@h3c.com>

> ---
>   fs/ocfs2/aops.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
> index 302cd7c..a87ca42d 100644
> --- a/fs/ocfs2/aops.c
> +++ b/fs/ocfs2/aops.c
> @@ -2412,8 +2412,15 @@ static int ocfs2_dio_end_io(struct kiocb *iocb,
>   	/* this io's submitter should not have unlocked this before we could */
>   	BUG_ON(!ocfs2_iocb_is_rw_locked(iocb));
>   
> -	if (bytes > 0 && private)
> -		ret = ocfs2_dio_end_io_write(inode, private, offset, bytes);
> +	if (private) {
> +		if (bytes > 0) {
> +			ret = ocfs2_dio_end_io_write(inode, private, offset,
> +						     bytes);
> +		} else {
> +			mlog(ML_ERROR, "Direct IO failed, free write ctx");
> +			ocfs2_dio_free_write_ctx(inode, private);
> +		}
> +	}
>   
>   	ocfs2_iocb_clear_rw_locked(iocb);
>   
>
Joseph Qi Nov. 2, 2018, 1:35 a.m. UTC | #2
On 18/11/2 05:15, Wengang Wang wrote:
> The write context should also be freed even when direct IO failed.
> Otherwise a memory leak is introduced and entries remain in
> oi->ip_unwritten_list causing the following BUG later in unlink path:
> 
> ERROR: bug expression: !list_empty(&oi->ip_unwritten_list)
> ERROR: Clear inode of 215043, inode has unwritten extents
> ...
> Call Trace:
> ? __set_current_blocked+0x42/0x68
> ocfs2_evict_inode+0x91/0x6a0 [ocfs2]
> ? bit_waitqueue+0x40/0x33
> evict+0xdb/0x1af
> iput+0x1a2/0x1f7
> do_unlinkat+0x194/0x28f
> SyS_unlinkat+0x1b/0x2f
> do_syscall_64+0x79/0x1ae
> entry_SYSCALL_64_after_hwframe+0x151/0x0
> 
> Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>

Looks good.
We'd better cc stable as well (4.10+ ?).

Acked-by: Joseph Qi <jiangqi903@gmail.com>

> ---
>  fs/ocfs2/aops.c | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
> index 302cd7c..a87ca42d 100644
> --- a/fs/ocfs2/aops.c
> +++ b/fs/ocfs2/aops.c
> @@ -2412,8 +2412,15 @@ static int ocfs2_dio_end_io(struct kiocb *iocb,
>  	/* this io's submitter should not have unlocked this before we could */
>  	BUG_ON(!ocfs2_iocb_is_rw_locked(iocb));
>  
> -	if (bytes > 0 && private)
> -		ret = ocfs2_dio_end_io_write(inode, private, offset, bytes);
> +	if (private) {
> +		if (bytes > 0) {
> +			ret = ocfs2_dio_end_io_write(inode, private, offset,
> +						     bytes);
> +		} else {
> +			mlog(ML_ERROR, "Direct IO failed, free write ctx");
> +			ocfs2_dio_free_write_ctx(inode, private);
> +		}
> +	}
>  
>  	ocfs2_iocb_clear_rw_locked(iocb);
>  
>
Junxiao Bi Nov. 2, 2018, 2:51 a.m. UTC | #3
On 11/02/2018 05:15 AM, Wengang Wang wrote:

> The write context should also be freed even when direct IO failed.
> Otherwise a memory leak is introduced and entries remain in
> oi->ip_unwritten_list causing the following BUG later in unlink path:
>
> ERROR: bug expression: !list_empty(&oi->ip_unwritten_list)
> ERROR: Clear inode of 215043, inode has unwritten extents
> ...
> Call Trace:
> ? __set_current_blocked+0x42/0x68
> ocfs2_evict_inode+0x91/0x6a0 [ocfs2]
> ? bit_waitqueue+0x40/0x33
> evict+0xdb/0x1af
> iput+0x1a2/0x1f7
> do_unlinkat+0x194/0x28f
> SyS_unlinkat+0x1b/0x2f
> do_syscall_64+0x79/0x1ae
> entry_SYSCALL_64_after_hwframe+0x151/0x0
>
> Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
> ---
>   fs/ocfs2/aops.c | 11 +++++++++--
>   1 file changed, 9 insertions(+), 2 deletions(-)
>
> diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
> index 302cd7c..a87ca42d 100644
> --- a/fs/ocfs2/aops.c
> +++ b/fs/ocfs2/aops.c
> @@ -2412,8 +2412,15 @@ static int ocfs2_dio_end_io(struct kiocb *iocb,
>   	/* this io's submitter should not have unlocked this before we could */
>   	BUG_ON(!ocfs2_iocb_is_rw_locked(iocb));
>   
> -	if (bytes > 0 && private)
> -		ret = ocfs2_dio_end_io_write(inode, private, offset, bytes);
> +	if (private) {
> +		if (bytes > 0) {
> +			ret = ocfs2_dio_end_io_write(inode, private, offset,
> +						     bytes);
> +		} else {
> +			mlog(ML_ERROR, "Direct IO failed, free write ctx");
Is it useful to leave the "bytes" value here? It could be some error code.

Thanks,
Junxiao.
> +			ocfs2_dio_free_write_ctx(inode, private);
> +		}
> +	}
>   
>   	ocfs2_iocb_clear_rw_locked(iocb);
>
Wengang Nov. 2, 2018, 3:57 p.m. UTC | #4
On 2018/11/1 19:51, Junxiao Bi wrote:
> On 11/02/2018 05:15 AM, Wengang Wang wrote:
>
>> The write context should also be freed even when direct IO failed.
>> Otherwise a memory leak is introduced and entries remain in
>> oi->ip_unwritten_list causing the following BUG later in unlink path:
>>
>> ERROR: bug expression: !list_empty(&oi->ip_unwritten_list)
>> ERROR: Clear inode of 215043, inode has unwritten extents
>> ...
>> Call Trace:
>> ? __set_current_blocked+0x42/0x68
>> ocfs2_evict_inode+0x91/0x6a0 [ocfs2]
>> ? bit_waitqueue+0x40/0x33
>> evict+0xdb/0x1af
>> iput+0x1a2/0x1f7
>> do_unlinkat+0x194/0x28f
>> SyS_unlinkat+0x1b/0x2f
>> do_syscall_64+0x79/0x1ae
>> entry_SYSCALL_64_after_hwframe+0x151/0x0
>>
>> Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
>> ---
>>   fs/ocfs2/aops.c | 11 +++++++++--
>>   1 file changed, 9 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
>> index 302cd7c..a87ca42d 100644
>> --- a/fs/ocfs2/aops.c
>> +++ b/fs/ocfs2/aops.c
>> @@ -2412,8 +2412,15 @@ static int ocfs2_dio_end_io(struct kiocb *iocb,
>>       /* this io's submitter should not have unlocked this before we 
>> could */
>>       BUG_ON(!ocfs2_iocb_is_rw_locked(iocb));
>>   -    if (bytes > 0 && private)
>> -        ret = ocfs2_dio_end_io_write(inode, private, offset, bytes);
>> +    if (private) {
>> +        if (bytes > 0) {
>> +            ret = ocfs2_dio_end_io_write(inode, private, offset,
>> +                             bytes);
>> +        } else {
>> +            mlog(ML_ERROR, "Direct IO failed, free write ctx");
> Is it useful to leave the "bytes" value here? It could be some error 
> code.
>
That would be error come from below file system, but we can log it.
Will send v2 to add "bytes".

thanks,
wengang
> Thanks,
> Junxiao.
>> + ocfs2_dio_free_write_ctx(inode, private);
>> +        }
>> +    }
>>         ocfs2_iocb_clear_rw_locked(iocb);
>

Patch
diff mbox series

diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
index 302cd7c..a87ca42d 100644
--- a/fs/ocfs2/aops.c
+++ b/fs/ocfs2/aops.c
@@ -2412,8 +2412,15 @@  static int ocfs2_dio_end_io(struct kiocb *iocb,
 	/* this io's submitter should not have unlocked this before we could */
 	BUG_ON(!ocfs2_iocb_is_rw_locked(iocb));
 
-	if (bytes > 0 && private)
-		ret = ocfs2_dio_end_io_write(inode, private, offset, bytes);
+	if (private) {
+		if (bytes > 0) {
+			ret = ocfs2_dio_end_io_write(inode, private, offset,
+						     bytes);
+		} else {
+			mlog(ML_ERROR, "Direct IO failed, free write ctx");
+			ocfs2_dio_free_write_ctx(inode, private);
+		}
+	}
 
 	ocfs2_iocb_clear_rw_locked(iocb);