| Message ID | 20181101211531.21036-1-wen.gang.wang@oracle.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ocfs2: free up write context when direct IO failed | expand |
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); > >
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); > >
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); >
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); >
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);
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(-)