ocfs2: don't use iocb when EIOCBQUEUED returns

PATCH New Changwei Ge 5 participants 9 messages Apr 10, 2018 – May 9, 2018 OCFS2

Share & tools

Changwei Ge

author

2018-04-10 12:00 UTC #0 lore raw

PATCH 1/1 ocfs2: don't use iocb when EIOCBQUEUED returns

When -EIOCBQUEUED returns, it means that aio_complete() will be called from dio_complete(), which is an asynchronous progress against write_iter. Generally, IO is a very slow progress than executing instruction, but we still can't take the risk to access a freed iocb.

And we do face a BUG crash issue.
From crash tool, iocb is obviously freed already.
crash> struct -x kiocb ffff881a350f5900
struct kiocb {
ki_filp = 0xffff881a350f5a80,
ki_pos = 0x0,
ki_complete = 0x0,
private = 0x0,
ki_flags = 0x0
}

And the backtrace shows:

ocfs2_file_write_iter+0xcaa/0xd00 [ocfs2]
? ocfs2_check_range_for_refcount+0x150/0x150 [ocfs2]
aio_run_iocb+0x229/0x2f0
? try_to_wake_up+0x380/0x380
do_io_submit+0x291/0x540
? syscall_trace_leave+0xad/0x130
SyS_io_submit+0x10/0x20
system_call_fastpath+0x16/0x75

Signed-off-by Changwei Ge <[email protected]>

---
 fs/ocfs2/file.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

fs/ocfs2/file.c +2 -2

index 5d1784a..1393ff2 100644
@@ -2343,7 +2343,7 @@ static ssize_t ocfs2_file_write_iter(struct kiocb *iocb,
 
 	written = __generic_file_write_iter(iocb, from);
 	/* buffered aio wouldn't have proper lock coverage today */
-	BUG_ON(written == -EIOCBQUEUED && !(iocb->ki_flags & IOCB_DIRECT));
+	BUG_ON(written == -EIOCBQUEUED && !direct_io);
 
 	/*
 	 * deep in g_f_a_w_n()->ocfs2_direct_IO we pass in a ocfs2_dio_end_io

@@ -2463,7 +2463,7 @@ static ssize_t ocfs2_file_read_iter(struct kiocb *iocb,
 	trace_generic_file_aio_read_ret(ret);
 
 	/* buffered aio wouldn't have proper lock coverage today */
-	BUG_ON(ret == -EIOCBQUEUED && !(iocb->ki_flags & IOCB_DIRECT));
+	BUG_ON(ret == -EIOCBQUEUED && !direct_io);
 
 	/* see ocfs2_file_write_iter */
 	if (ret == -EIOCBQUEUED || !ocfs2_iocb_is_rw_locked(iocb)) {

Signature · 1 line

2.7.4

piaojun

2018-04-11 00:51 UTC #1 lore raw

PATCH 1/1 ocfs2: don't use iocb when EIOCBQUEUED returns

Hi Changwei,

It looks like a code bug, and 'iocb' should not be freed at this place. Could this BUG reproduced easily?

thanks,
Jun

Quoted 54 lines · Changwei Ge

When -EIOCBQUEUED returns, it means that aio_complete() will be called from dio_complete(), which is an asynchronous progress against write_iter. Generally, IO is a very slow progress than executing instruction, but we still can't take the risk to access a freed iocb.
And we do face a BUG crash issue.
Quoted 1 line
From crash tool, iocb is obviously freed already.
crash> struct -x kiocb ffff881a350f5900
struct kiocb {
ki_filp = 0xffff881a350f5a80,
ki_pos = 0x0,
ki_complete = 0x0,
private = 0x0,
ki_flags = 0x0
}
And the backtrace shows:
ocfs2_file_write_iter+0xcaa/0xd00 [ocfs2]
? ocfs2_check_range_for_refcount+0x150/0x150 [ocfs2]
aio_run_iocb+0x229/0x2f0
? try_to_wake_up+0x380/0x380
do_io_submit+0x291/0x540
? syscall_trace_leave+0xad/0x130
SyS_io_submit+0x10/0x20
system_call_fastpath+0x16/0x75
Signed-off-by Changwei Ge <[email protected]>
---
 fs/ocfs2/file.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
fs/ocfs2/file.c +2 -2
index 5d1784a..1393ff2 100644
@@ -2343,7 +2343,7 @@ static ssize_t ocfs2_file_write_iter(struct kiocb *iocb,
 
 	written = __generic_file_write_iter(iocb, from);
 	/* buffered aio wouldn't have proper lock coverage today */
-	BUG_ON(written == -EIOCBQUEUED && !(iocb->ki_flags & IOCB_DIRECT));
+	BUG_ON(written == -EIOCBQUEUED && !direct_io);
 
 	/*
 	 * deep in g_f_a_w_n()->ocfs2_direct_IO we pass in a ocfs2_dio_end_io
@@ -2463,7 +2463,7 @@ static ssize_t ocfs2_file_read_iter(struct kiocb *iocb,
 	trace_generic_file_aio_read_ret(ret);
 
 	/* buffered aio wouldn't have proper lock coverage today */
-	BUG_ON(ret == -EIOCBQUEUED && !(iocb->ki_flags & IOCB_DIRECT));
+	BUG_ON(ret == -EIOCBQUEUED && !direct_io);
 
 	/* see ocfs2_file_write_iter */
 	if (ret == -EIOCBQUEUED || !ocfs2_iocb_is_rw_locked(iocb)) {

Changwei Ge

author

2018-04-11 01:07 UTC #2 lore raw

PATCH 1/1 ocfs2: don't use iocb when EIOCBQUEUED returns

Hi Jun,

Quoted 4 lines · piaojun

Hi Changwei,
It looks like a code bug, and 'iocb' should not be freed at this place. Could this BUG reproduced easily?

Actually, it's not easy to be reproduced since IO is much slower than CPU executing instructions. But the logic here is broken, we'd better fix this.

Thanks,
Changwei

Quoted 60 lines

thanks,
Jun

Quoted 54 lines · Changwei Ge

And we do face a BUG crash issue.

Quoted 1 line

From crash tool, iocb is obviously freed already.

crash> struct -x kiocb ffff881a350f5900

struct kiocb {
   ki_filp = 0xffff881a350f5a80,
   ki_pos = 0x0,
   ki_complete = 0x0,
   private = 0x0,
   ki_flags = 0x0
}

And the backtrace shows:

ocfs2_file_write_iter+0xcaa/0xd00 [ocfs2]
? ocfs2_check_range_for_refcount+0x150/0x150 [ocfs2]
aio_run_iocb+0x229/0x2f0
? try_to_wake_up+0x380/0x380
do_io_submit+0x291/0x540
? syscall_trace_leave+0xad/0x130
SyS_io_submit+0x10/0x20
system_call_fastpath+0x16/0x75

Signed-off-by Changwei Ge <[email protected]>

---
  fs/ocfs2/file.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

fs/ocfs2/file.c +2 -2

index 5d1784a..1393ff2 100644
@@ -2343,7 +2343,7 @@ static ssize_t ocfs2_file_write_iter(struct kiocb *iocb,
 
  	written = __generic_file_write_iter(iocb, from);
  	/* buffered aio wouldn't have proper lock coverage today */
-	BUG_ON(written == -EIOCBQUEUED && !(iocb->ki_flags & IOCB_DIRECT));
+	BUG_ON(written == -EIOCBQUEUED && !direct_io);
 
  	/*
  	 * deep in g_f_a_w_n()->ocfs2_direct_IO we pass in a ocfs2_dio_end_io

@@ -2463,7 +2463,7 @@ static ssize_t ocfs2_file_read_iter(struct kiocb *iocb,
  	trace_generic_file_aio_read_ret(ret);
 
  	/* buffered aio wouldn't have proper lock coverage today */
-	BUG_ON(ret == -EIOCBQUEUED && !(iocb->ki_flags & IOCB_DIRECT));
+	BUG_ON(ret == -EIOCBQUEUED && !direct_io);
 
  	/* see ocfs2_file_write_iter */
  	if (ret == -EIOCBQUEUED || !ocfs2_iocb_is_rw_locked(iocb)) {

piaojun

2018-04-11 01:51 UTC #3 lore raw

PATCH 1/1 ocfs2: don't use iocb when EIOCBQUEUED returns

Hi Changwei,

It seems other codes which try to access 'iocb' will also cause error, right? I think we should find the reason why 'iocb' is freed first.

thanks,
Jun

Quoted 76 lines · Changwei Ge

Hi Jun,
Quoted 4 lines · piaojun
Hi Changwei,
It looks like a code bug, and 'iocb' should not be freed at this place. Could this BUG reproduced easily?
Actually, it's not easy to be reproduced since IO is much slower than CPU executing instructions. But the logic here is broken, we'd better fix this.
Thanks,
Changwei
Quoted 52 lines
thanks,
Jun
Quoted 47 lines · Changwei Ge
When -EIOCBQUEUED returns, it means that aio_complete() will be called from dio_complete(), which is an asynchronous progress against write_iter. Generally, IO is a very slow progress than executing instruction, but we still can't take the risk to access a freed iocb.
And we do face a BUG crash issue.
Quoted 1 line
From crash tool, iocb is obviously freed already.
crash> struct -x kiocb ffff881a350f5900
struct kiocb {
   ki_filp = 0xffff881a350f5a80,
   ki_pos = 0x0,
   ki_complete = 0x0,
   private = 0x0,
   ki_flags = 0x0
}
And the backtrace shows:
ocfs2_file_write_iter+0xcaa/0xd00 [ocfs2]
? ocfs2_check_range_for_refcount+0x150/0x150 [ocfs2]
aio_run_iocb+0x229/0x2f0
? try_to_wake_up+0x380/0x380
do_io_submit+0x291/0x540
? syscall_trace_leave+0xad/0x130
SyS_io_submit+0x10/0x20
system_call_fastpath+0x16/0x75
Signed-off-by Changwei Ge <[email protected]>
---
  fs/ocfs2/file.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
fs/ocfs2/file.c +1 -1
index 5d1784a..1393ff2 100644
@@ -2343,7 +2343,7 @@ static ssize_t ocfs2_file_write_iter(struct kiocb *iocb,
 
  	written = __generic_file_write_iter(iocb, from);
  	/* buffered aio wouldn't have proper lock coverage today */
-	BUG_ON(written == -EIOCBQUEUED && !(iocb->ki_flags & IOCB_DIRECT));
+	BUG_ON(written == -EIOCBQUEUED && !direct_io);
 
  	/*
  	 * deep in g_f_a_w_n()->ocfs2_direct_IO we pass in a ocfs2_dio_end_io
@@ -2463,7 +2463,7 @@ static ssize_t ocfs2_file_read_iter(struct kiocb *iocb,
  	trace_generic_file_aio_read_ret(ret);
 
Quoted 8 lines
fs/ocfs2/file.c static ssize_t ocfs2_file_read_iter(struct kiocb *iocb, · line 2469
Quoted 7 lines
fs/ocfs2/file.c +1 -1
  	/* buffered aio wouldn't have proper lock coverage today */
-	BUG_ON(ret == -EIOCBQUEUED && !(iocb->ki_flags & IOCB_DIRECT));
+	BUG_ON(ret == -EIOCBQUEUED && !direct_io);
 
  	/* see ocfs2_file_write_iter */
  	if (ret == -EIOCBQUEUED || !ocfs2_iocb_is_rw_locked(iocb)) {
.

Changwei Ge

author

2018-04-11 02:01 UTC #4 lore raw

PATCH 1/1 ocfs2: don't use iocb when EIOCBQUEUED returns

Hi Jun,

Quoted 4 lines · piaojun

Hi Changwei,
It seems other codes which try to access 'iocb' will also cause error, right? I think we should find the reason why 'iocb' is freed first.

Which code snippet do you mean? Actually, I have checked most of other parts in write_iter() and read_iter(). I don't see any risk accessing freed iocb yet.

The root cause is clear for this issue.
iocb is freed by aio_complete().

You can refer to path:

dio_complete
aio_complete
kiocb_free

Thanks,
Changwei

Quoted 82 lines

thanks,
Jun
Quoted 76 lines · Changwei Ge
Hi Jun,
Quoted 4 lines · piaojun
Hi Changwei,
It looks like a code bug, and 'iocb' should not be freed at this place. Could this BUG reproduced easily?
Actually, it's not easy to be reproduced since IO is much slower than CPU executing instructions. But the logic here is broken, we'd better fix this.
Thanks,
Changwei
Quoted 52 lines
thanks,
Jun
Quoted 47 lines · Changwei Ge
When -EIOCBQUEUED returns, it means that aio_complete() will be called from dio_complete(), which is an asynchronous progress against write_iter. Generally, IO is a very slow progress than executing instruction, but we still can't take the risk to access a freed iocb.
And we do face a BUG crash issue.
Quoted 1 line
From crash tool, iocb is obviously freed already.
crash> struct -x kiocb ffff881a350f5900
struct kiocb {
    ki_filp = 0xffff881a350f5a80,
    ki_pos = 0x0,
    ki_complete = 0x0,
    private = 0x0,
    ki_flags = 0x0
}
And the backtrace shows:
ocfs2_file_write_iter+0xcaa/0xd00 [ocfs2]
? ocfs2_check_range_for_refcount+0x150/0x150 [ocfs2]
aio_run_iocb+0x229/0x2f0
? try_to_wake_up+0x380/0x380
do_io_submit+0x291/0x540
? syscall_trace_leave+0xad/0x130
SyS_io_submit+0x10/0x20
system_call_fastpath+0x16/0x75
Signed-off-by Changwei Ge <[email protected]>
---
   fs/ocfs2/file.c | 4 ++--
   1 file changed, 2 insertions(+), 2 deletions(-)
fs/ocfs2/file.c +1 -1
index 5d1784a..1393ff2 100644
@@ -2343,7 +2343,7 @@ static ssize_t ocfs2_file_write_iter(struct kiocb *iocb,
 
   	written = __generic_file_write_iter(iocb, from);
   	/* buffered aio wouldn't have proper lock coverage today */
-	BUG_ON(written == -EIOCBQUEUED && !(iocb->ki_flags & IOCB_DIRECT));
+	BUG_ON(written == -EIOCBQUEUED && !direct_io);
 
   	/*
   	 * deep in g_f_a_w_n()->ocfs2_direct_IO we pass in a ocfs2_dio_end_io
@@ -2463,7 +2463,7 @@ static ssize_t ocfs2_file_read_iter(struct kiocb *iocb,
   	trace_generic_file_aio_read_ret(ret);
 
Quoted 8 lines
fs/ocfs2/file.c static ssize_t ocfs2_file_read_iter(struct kiocb *iocb, · line 2469
Quoted 7 lines
fs/ocfs2/file.c +1 -1
   	/* buffered aio wouldn't have proper lock coverage today */
-	BUG_ON(ret == -EIOCBQUEUED && !(iocb->ki_flags & IOCB_DIRECT));
+	BUG_ON(ret == -EIOCBQUEUED && !direct_io);
 
   	/* see ocfs2_file_write_iter */
   	if (ret == -EIOCBQUEUED || !ocfs2_iocb_is_rw_locked(iocb)) {
.

Gang He

2018-04-11 02:51 UTC #5 lore raw

PATCH 1/1 ocfs2: don't use iocb when EIOCBQUEUED returns

In reply to Changwei Ge

Hi Changwei,

The code change just works around the problem, but theoretically the IOCB object should not be freed before which is handled. Anyway, if we can find the root cause behind via some way (e.g. inject delay in some place), the result is more perfect.

Thanks
Gang

Quoted 81 lines

Quoted 1 line
Quoted 1 line
Hi Jun,
Quoted 4 lines · piaojun
Hi Changwei,
It looks like a code bug, and 'iocb' should not be freed at this place. Could this BUG reproduced easily?
Actually, it's not easy to be reproduced since IO is much slower than CPU executing instructions. But the logic here is broken, we'd better fix this.
Thanks,
Changwei
Quoted 36 lines
thanks,
Jun
Quoted 31 lines · Changwei Ge
When -EIOCBQUEUED returns, it means that aio_complete() will be called from dio_complete(), which is an asynchronous progress against write_iter. Generally, IO is a very slow progress than executing instruction, but we still can't take the risk to access a freed iocb.
And we do face a BUG crash issue.
Quoted 1 line
From crash tool, iocb is obviously freed already.
crash> struct -x kiocb ffff881a350f5900
struct kiocb {
   ki_filp = 0xffff881a350f5a80,
   ki_pos = 0x0,
   ki_complete = 0x0,
   private = 0x0,
   ki_flags = 0x0
}
And the backtrace shows:
ocfs2_file_write_iter+0xcaa/0xd00 [ocfs2]
? ocfs2_check_range_for_refcount+0x150/0x150 [ocfs2]
aio_run_iocb+0x229/0x2f0
? try_to_wake_up+0x380/0x380
do_io_submit+0x291/0x540
? syscall_trace_leave+0xad/0x130
SyS_io_submit+0x10/0x20
system_call_fastpath+0x16/0x75
Signed-off-by Changwei Ge <[email protected]>
---
  fs/ocfs2/file.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
Quoted 5 lines
fs/ocfs2/file.c static ssize_t ocfs2_file_write_iter(struct kiocb
Quoted 5 lines
fs/ocfs2/file.c +0 -0
index 5d1784a..1393ff2 100644
@@ -2343,7 +2343,7 @@ static ssize_t ocfs2_file_write_iter(struct kiocb 
*iocb,
Quoted 11 lines
Quoted 11 lines
fs/ocfs2/file.c +1 -1
  	written = __generic_file_write_iter(iocb, from);
  	/* buffered aio wouldn't have proper lock coverage today */
-	BUG_ON(written == -EIOCBQUEUED && !(iocb->ki_flags & IOCB_DIRECT));
+	BUG_ON(written == -EIOCBQUEUED && !direct_io);
 
  	/*
  	 * deep in g_f_a_w_n()->ocfs2_direct_IO we pass in a ocfs2_dio_end_io
@@ -2463,7 +2463,7 @@ static ssize_t ocfs2_file_read_iter(struct kiocb *iocb,
  	trace_generic_file_aio_read_ret(ret);
 
Quoted 8 lines
fs/ocfs2/file.c static ssize_t ocfs2_file_read_iter(struct kiocb *iocb, · line 2469
Quoted 7 lines
fs/ocfs2/file.c +1 -1
  	/* buffered aio wouldn't have proper lock coverage today */
-	BUG_ON(ret == -EIOCBQUEUED && !(iocb->ki_flags & IOCB_DIRECT));
+	BUG_ON(ret == -EIOCBQUEUED && !direct_io);
 
  	/* see ocfs2_file_write_iter */
  	if (ret == -EIOCBQUEUED || !ocfs2_iocb_is_rw_locked(iocb)) {
_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

Changwei Ge

2018-05-08 15:23 UTC #6 lore raw

PATCH 1/1 ocfs2: don't use iocb when EIOCBQUEUED returns

Hi Gang,

I don't think this patch is a workaround trick.

We do face the risk using freed iocb although it is actually indeed hard to encounter, it still exists.

So I propose to fix it making ocfs2 more reliable.

Moreover, this patch has been kept in -mm tree for one month. Can anyone help review it with ack or nack? So I can do some improvement for it. :-)

Thanks,

Changwei

Quoted 91 lines · Gang He

Hi Changwei,
The code change just works around the problem, but theoretically the IOCB object should not be freed before which is handled. Anyway, if we can find the root cause behind via some way (e.g. inject delay in some place), the result is more perfect.
Thanks
Gang
Quoted 69 lines
Hi Jun,
Quoted 4 lines · piaojun
Hi Changwei,
It looks like a code bug, and 'iocb' should not be freed at this place. Could this BUG reproduced easily?
Actually, it's not easy to be reproduced since IO is much slower than CPU executing instructions. But the logic here is broken, we'd better fix this.
Thanks,
Changwei
Quoted 35 lines
thanks,
Jun
Quoted 31 lines · Changwei Ge
When -EIOCBQUEUED returns, it means that aio_complete() will be called from dio_complete(), which is an asynchronous progress against write_iter. Generally, IO is a very slow progress than executing instruction, but we still can't take the risk to access a freed iocb.
And we do face a BUG crash issue.
Quoted 1 line
From crash tool, iocb is obviously freed already.
crash> struct -x kiocb ffff881a350f5900
struct kiocb {
    ki_filp = 0xffff881a350f5a80,
    ki_pos = 0x0,
    ki_complete = 0x0,
    private = 0x0,
    ki_flags = 0x0
}
And the backtrace shows:
ocfs2_file_write_iter+0xcaa/0xd00 [ocfs2]
? ocfs2_check_range_for_refcount+0x150/0x150 [ocfs2]
aio_run_iocb+0x229/0x2f0
? try_to_wake_up+0x380/0x380
do_io_submit+0x291/0x540
? syscall_trace_leave+0xad/0x130
SyS_io_submit+0x10/0x20
system_call_fastpath+0x16/0x75
Signed-off-by Changwei Ge <[email protected]>
---
   fs/ocfs2/file.c | 4 ++--
   1 file changed, 2 insertions(+), 2 deletions(-)
Quoted 5 lines
fs/ocfs2/file.c static ssize_t ocfs2_file_write_iter(struct kiocb
Quoted 5 lines
fs/ocfs2/file.c +0 -0
index 5d1784a..1393ff2 100644
@@ -2343,7 +2343,7 @@ static ssize_t ocfs2_file_write_iter(struct kiocb
*iocb,
Quoted 15 lines
Quoted 15 lines
fs/ocfs2/file.c +2 -2
   	written = __generic_file_write_iter(iocb, from);
   	/* buffered aio wouldn't have proper lock coverage today */
-	BUG_ON(written == -EIOCBQUEUED && !(iocb->ki_flags & IOCB_DIRECT));
+	BUG_ON(written == -EIOCBQUEUED && !direct_io);
 
   	/*
   	 * deep in g_f_a_w_n()->ocfs2_direct_IO we pass in a ocfs2_dio_end_io
@@ -2463,7 +2463,7 @@ static ssize_t ocfs2_file_read_iter(struct kiocb *iocb,
   	trace_generic_file_aio_read_ret(ret);
 
   	/* buffered aio wouldn't have proper lock coverage today */
-	BUG_ON(ret == -EIOCBQUEUED && !(iocb->ki_flags & IOCB_DIRECT));
+	BUG_ON(ret == -EIOCBQUEUED && !direct_io);
 
Quoted 7 lines
Quoted 3 lines
fs/ocfs2/file.c static ssize_t ocfs2_file_read_iter(struct kiocb *iocb, · line 2469
Quoted 3 lines
fs/ocfs2/file.c +0 -0
   	/* see ocfs2_file_write_iter */
   	if (ret == -EIOCBQUEUED || !ocfs2_iocb_is_rw_locked(iocb)) {
_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel
_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

Joseph Qi

2018-05-09 01:41 UTC #7 lore raw

PATCH 1/1 ocfs2: don't use iocb when EIOCBQUEUED returns

Hi Changwei,

I agree with Gang that currently we still haven't figured out why iocb was freed. Though you fix won't bring any side effect, it looks like a workaround.
That means, the freed iocb may still have risk in other place.

Thanks,
Joseph

Quoted 112 lines · Changwei Ge

Hi Gang,
I don't think this patch is a workaround trick.
We do face the risk using freed iocb although it is actually indeed hard to encounter, it still exists.
So I propose to fix it making ocfs2 more reliable.
Moreover, this patch has been kept in -mm tree for one month. Can anyone help review it with ack or nack? So I can do some improvement for it. :-)
Thanks,
Changwei
Quoted 91 lines · Gang He
Hi Changwei,
The code change just works around the problem, but theoretically the IOCB object should not be freed before which is handled. Anyway, if we can find the root cause behind via some way (e.g. inject delay in some place), the result is more perfect.
Thanks
Gang
Quoted 69 lines
Hi Jun,
Quoted 4 lines · piaojun
Hi Changwei,
It looks like a code bug, and 'iocb' should not be freed at this place. Could this BUG reproduced easily?
Actually, it's not easy to be reproduced since IO is much slower than CPU executing instructions. But the logic here is broken, we'd better fix this.
Thanks,
Changwei
Quoted 35 lines
thanks,
Jun
Quoted 31 lines · Changwei Ge
When -EIOCBQUEUED returns, it means that aio_complete() will be called from dio_complete(), which is an asynchronous progress against write_iter. Generally, IO is a very slow progress than executing instruction, but we still can't take the risk to access a freed iocb.
And we do face a BUG crash issue.
Quoted 1 line
From crash tool, iocb is obviously freed already.
crash> struct -x kiocb ffff881a350f5900
struct kiocb {
    ki_filp = 0xffff881a350f5a80,
    ki_pos = 0x0,
    ki_complete = 0x0,
    private = 0x0,
    ki_flags = 0x0
}
And the backtrace shows:
ocfs2_file_write_iter+0xcaa/0xd00 [ocfs2]
? ocfs2_check_range_for_refcount+0x150/0x150 [ocfs2]
aio_run_iocb+0x229/0x2f0
? try_to_wake_up+0x380/0x380
do_io_submit+0x291/0x540
? syscall_trace_leave+0xad/0x130
SyS_io_submit+0x10/0x20
system_call_fastpath+0x16/0x75
Signed-off-by Changwei Ge <[email protected]>
---
   fs/ocfs2/file.c | 4 ++--
   1 file changed, 2 insertions(+), 2 deletions(-)
Quoted 5 lines
fs/ocfs2/file.c static ssize_t ocfs2_file_write_iter(struct kiocb
Quoted 5 lines
fs/ocfs2/file.c +0 -0
index 5d1784a..1393ff2 100644
@@ -2343,7 +2343,7 @@ static ssize_t ocfs2_file_write_iter(struct kiocb
*iocb,
Quoted 15 lines
Quoted 15 lines
fs/ocfs2/file.c +2 -2
   	written = __generic_file_write_iter(iocb, from);
   	/* buffered aio wouldn't have proper lock coverage today */
-	BUG_ON(written == -EIOCBQUEUED && !(iocb->ki_flags & IOCB_DIRECT));
+	BUG_ON(written == -EIOCBQUEUED && !direct_io);
 
   	/*
   	 * deep in g_f_a_w_n()->ocfs2_direct_IO we pass in a ocfs2_dio_end_io
@@ -2463,7 +2463,7 @@ static ssize_t ocfs2_file_read_iter(struct kiocb *iocb,
   	trace_generic_file_aio_read_ret(ret);
 
   	/* buffered aio wouldn't have proper lock coverage today */
-	BUG_ON(ret == -EIOCBQUEUED && !(iocb->ki_flags & IOCB_DIRECT));
+	BUG_ON(ret == -EIOCBQUEUED && !direct_io);
 
Quoted 7 lines
Quoted 3 lines
fs/ocfs2/file.c static ssize_t ocfs2_file_read_iter(struct kiocb *iocb, · line 2469
Quoted 3 lines
fs/ocfs2/file.c +0 -0
   	/* see ocfs2_file_write_iter */
   	if (ret == -EIOCBQUEUED || !ocfs2_iocb_is_rw_locked(iocb)) {
_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel
_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

Changwei Ge

2018-05-09 02:14 UTC #8 lore raw

PATCH 1/1 ocfs2: don't use iocb when EIOCBQUEUED returns

Hi Joseph,

Thanks a lot for your comments.
Actually, I think I have found the root cause why the iocb is freed.

As __generic_file_write_iter() returns -EIOCBQUEUED, aio_complete() will be invoked asynchronously
against write_iter()->ocfs2_file_write_iter().

If -EIOCBQUEUED is not returned, aio_complete() should be invoked after return from ocfs2_file_write_iter(), which is a synchronous progress.

If I still don't make my point clear, please let me know.

Thanks,
Changwei

Quoted 121 lines · Joseph Qi

Hi Changwei,
I agree with Gang that currently we still haven't figured out why iocb was freed. Though you fix won't bring any side effect, it looks like a workaround.
That means, the freed iocb may still have risk in other place.
Thanks,
Joseph
Quoted 110 lines · Changwei Ge
Hi Gang,
I don't think this patch is a workaround trick.
We do face the risk using freed iocb although it is actually indeed hard to encounter, it still exists.
So I propose to fix it making ocfs2 more reliable.
Moreover, this patch has been kept in -mm tree for one month. Can anyone help review it with ack or nack? So I can do some improvement for it. :-)
Thanks,
Changwei
Quoted 90 lines · Gang He
Hi Changwei,
The code change just works around the problem, but theoretically the IOCB object should not be freed before which is handled. Anyway, if we can find the root cause behind via some way (e.g. inject delay in some place), the result is more perfect.
Thanks
Gang
Quoted 69 lines
Hi Jun,
Quoted 4 lines · piaojun
Hi Changwei,
It looks like a code bug, and 'iocb' should not be freed at this place. Could this BUG reproduced easily?
Actually, it's not easy to be reproduced since IO is much slower than CPU executing instructions. But the logic here is broken, we'd better fix this.
Thanks,
Changwei
Quoted 35 lines
thanks,
Jun
Quoted 31 lines · Changwei Ge
When -EIOCBQUEUED returns, it means that aio_complete() will be called from dio_complete(), which is an asynchronous progress against write_iter. Generally, IO is a very slow progress than executing instruction, but we still can't take the risk to access a freed iocb.
And we do face a BUG crash issue.
Quoted 1 line
From crash tool, iocb is obviously freed already.
crash> struct -x kiocb ffff881a350f5900
struct kiocb {
     ki_filp = 0xffff881a350f5a80,
     ki_pos = 0x0,
     ki_complete = 0x0,
     private = 0x0,
     ki_flags = 0x0
}
And the backtrace shows:
ocfs2_file_write_iter+0xcaa/0xd00 [ocfs2]
? ocfs2_check_range_for_refcount+0x150/0x150 [ocfs2]
aio_run_iocb+0x229/0x2f0
? try_to_wake_up+0x380/0x380
do_io_submit+0x291/0x540
? syscall_trace_leave+0xad/0x130
SyS_io_submit+0x10/0x20
system_call_fastpath+0x16/0x75
Signed-off-by Changwei Ge <[email protected]>
---
    fs/ocfs2/file.c | 4 ++--
    1 file changed, 2 insertions(+), 2 deletions(-)
Quoted 5 lines
fs/ocfs2/file.c static ssize_t ocfs2_file_write_iter(struct kiocb
Quoted 5 lines
fs/ocfs2/file.c +0 -0
index 5d1784a..1393ff2 100644
@@ -2343,7 +2343,7 @@ static ssize_t ocfs2_file_write_iter(struct kiocb
*iocb,
Quoted 15 lines
Quoted 15 lines
fs/ocfs2/file.c +2 -2
    	written = __generic_file_write_iter(iocb, from);
    	/* buffered aio wouldn't have proper lock coverage today */
-	BUG_ON(written == -EIOCBQUEUED && !(iocb->ki_flags & IOCB_DIRECT));
+	BUG_ON(written == -EIOCBQUEUED && !direct_io);
 
    	/*
    	 * deep in g_f_a_w_n()->ocfs2_direct_IO we pass in a ocfs2_dio_end_io
@@ -2463,7 +2463,7 @@ static ssize_t ocfs2_file_read_iter(struct kiocb *iocb,
    	trace_generic_file_aio_read_ret(ret);
 
    	/* buffered aio wouldn't have proper lock coverage today */
-	BUG_ON(ret == -EIOCBQUEUED && !(iocb->ki_flags & IOCB_DIRECT));
+	BUG_ON(ret == -EIOCBQUEUED && !direct_io);
 
Quoted 7 lines
Quoted 3 lines
fs/ocfs2/file.c static ssize_t ocfs2_file_read_iter(struct kiocb *iocb, · line 2469
Quoted 3 lines
fs/ocfs2/file.c +0 -0
    	/* see ocfs2_file_write_iter */
    	if (ret == -EIOCBQUEUED || !ocfs2_iocb_is_rw_locked(iocb)) {
_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel
_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel