Skip to contentSkip to conversation

ocfs2: free up write context when direct IO failed

PATCH Applied Wengang Wang 5 participants 7 messages Nov 2, 2018 – Nov 6, 2018

Subsystems (2)
Stable branch OCFS2
lore
Share & tools
lore

Wengang Wang

author
#0 lore raw

PATCH v2 1/1 ocfs2: free up write context when direct IO failed

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

This patch also logs, with frequency limit, direct IO failures.

Signed-off-by Wengang Wang <[email protected]>

---
v1 -> v2: log direct IO failures

---
 fs/ocfs2/aops.c            | 12 ++++++++++--
 fs/ocfs2/cluster/masklog.h |  9 +++++++++
 2 files changed, 19 insertions(+), 2 deletions(-)
fs/ocfs2/aops.c +10 -2
index 302cd7c..7578bd5 100644
@@ -2412,8 +2412,16 @@ 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 (bytes <= 0)
+		mlog_ratelimited(ML_ERROR, "Direct IO failed, bytes = %lld",
+				 (long long)bytes);
+	if (private) {
+		if (bytes > 0)
+			ret = ocfs2_dio_end_io_write(inode, private, offset,
+						     bytes);
+		else
+			ocfs2_dio_free_write_ctx(inode, private);
+	}
 
 	ocfs2_iocb_clear_rw_locked(iocb);
fs/ocfs2/cluster/masklog.h +9 -0
index 308ea0e..a396096 100644
@@ -178,6 +178,15 @@ do {									\
 			      ##__VA_ARGS__);				\
 } while (0)
 
+#define mlog_ratelimited(mask, fmt, ...)				\
+do {									\
+	static DEFINE_RATELIMIT_STATE(_rs,				\
+				      DEFAULT_RATELIMIT_INTERVAL,	\
+				      DEFAULT_RATELIMIT_BURST);		\
+	if (__ratelimit(&_rs))						\
+		mlog(mask, fmt, ##__VA_ARGS__);				\
+} while (0)
+
 #define mlog_errno(st) ({						\
 	int _st = (st);							\
 	if (_st != -ERESTARTSYS && _st != -EINTR &&			\
Signature · 1 line
2.9.5
author
#1 lore raw

PATCH v2 1/1 ocfs2: free up write context when direct IO failed

Hi Guys,

Thanks for your reviews.
As suggested by Junxiao, logging direct IO failures is added in v2. Please review the v2.

thanks,
wengang

Quoted 71 lines · Wengang Wang

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

This patch also logs, with frequency limit, direct IO failures.

Signed-off-by Wengang Wang <[email protected]>

---
v1 -> v2: log direct IO failures

---
  fs/ocfs2/aops.c            | 12 ++++++++++--
  fs/ocfs2/cluster/masklog.h |  9 +++++++++
  2 files changed, 19 insertions(+), 2 deletions(-)
fs/ocfs2/aops.c +10 -2
index 302cd7c..7578bd5 100644
@@ -2412,8 +2412,16 @@ 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 (bytes <= 0)
+		mlog_ratelimited(ML_ERROR, "Direct IO failed, bytes = %lld",
+				 (long long)bytes);
+	if (private) {
+		if (bytes > 0)
+			ret = ocfs2_dio_end_io_write(inode, private, offset,
+						     bytes);
+		else
+			ocfs2_dio_free_write_ctx(inode, private);
+	}
 
  	ocfs2_iocb_clear_rw_locked(iocb);
fs/ocfs2/cluster/masklog.h +9 -0
index 308ea0e..a396096 100644
@@ -178,6 +178,15 @@ do {									\
  			      ##__VA_ARGS__);				\
  } while (0)
 
+#define mlog_ratelimited(mask, fmt, ...)				\
+do {									\
+	static DEFINE_RATELIMIT_STATE(_rs,				\
+				      DEFAULT_RATELIMIT_INTERVAL,	\
+				      DEFAULT_RATELIMIT_BURST);		\
+	if (__ratelimit(&_rs))						\
+		mlog(mask, fmt, ##__VA_ARGS__);				\
+} while (0)
+
  #define mlog_errno(st) ({						\
  	int _st = (st);							\
  	if (_st != -ERESTARTSYS && _st != -EINTR &&			\
maintainer
#2 lore raw

PATCH v2 1/1 ocfs2: free up write context when direct IO failed

In reply to Wengang Wang
Quoted 20 lines · Wengang Wang

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

This patch also logs, with frequency limit, direct IO failures.

Quoted 7 lines
Signed-off-by Wengang Wang <[email protected]>

---
v1 -> v2: log direct IO failures

---
 fs/ocfs2/aops.c            | 12 ++++++++++--
 fs/ocfs2/cluster/masklog.h |  9 +++++++++
 2 files changed, 19 insertions(+), 2 deletions(-)

<formletter>

This is not the correct way to submit patches for inclusion in the stable kernel tree. Please read:
https://urldefense.proofpoint.com/v2/url?u=https-3A__www.kernel.org_doc_html_latest_process_stable-2Dkernel-2Drules.html&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=C7gAd4uDxlAvTdc0vmU6X8CMk6L2iDY8-HD0qT6Fo7Y&m=P2I-XIsUfJ0h1jlymjMe7ziTkrs4pJ-sCZYaijMZpWw&s=ygRPYcgBDv1LNZjnBgPzukjEYvJ_sE8jlBo7R_3_b5c&e=
for how to do this properly.

</formletter>

#3 lore raw

PATCH v2 1/1 ocfs2: free up write context when direct IO failed

In reply to Wengang Wang
Quoted 18 lines · Wengang Wang

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
Quoted 3 lines

This patch also logs, with frequency limit, direct IO failures.

Signed-off-by Wengang Wang <[email protected]>
Reviewed-by Junxiao Bi <[email protected]>
Quoted 50 lines

---
v1 -> v2: log direct IO failures

---
  fs/ocfs2/aops.c            | 12 ++++++++++--
  fs/ocfs2/cluster/masklog.h |  9 +++++++++
  2 files changed, 19 insertions(+), 2 deletions(-)
fs/ocfs2/aops.c +10 -2
index 302cd7c..7578bd5 100644
@@ -2412,8 +2412,16 @@ 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 (bytes <= 0)
+		mlog_ratelimited(ML_ERROR, "Direct IO failed, bytes = %lld",
+				 (long long)bytes);
+	if (private) {
+		if (bytes > 0)
+			ret = ocfs2_dio_end_io_write(inode, private, offset,
+						     bytes);
+		else
+			ocfs2_dio_free_write_ctx(inode, private);
+	}
 
  	ocfs2_iocb_clear_rw_locked(iocb);
fs/ocfs2/cluster/masklog.h +9 -0
index 308ea0e..a396096 100644
@@ -178,6 +178,15 @@ do {									\
  			      ##__VA_ARGS__);				\
  } while (0)
 
+#define mlog_ratelimited(mask, fmt, ...)				\
+do {									\
+	static DEFINE_RATELIMIT_STATE(_rs,				\
+				      DEFAULT_RATELIMIT_INTERVAL,	\
+				      DEFAULT_RATELIMIT_BURST);		\
+	if (__ratelimit(&_rs))						\
+		mlog(mask, fmt, ##__VA_ARGS__);				\
+} while (0)
+
  #define mlog_errno(st) ({						\
  	int _st = (st);							\
  	if (_st != -ERESTARTSYS && _st != -EINTR &&			\
#4 lore raw

PATCH v2 1/1 ocfs2: free up write context when direct IO failed

In reply to Wengang Wang

This version also added a nice mlog infrastructure.

Looks good to me.

Thanks,
Changwei

Quoted 18 lines · Wengang Wang

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
Quoted 3 lines

This patch also logs, with frequency limit, direct IO failures.

Signed-off-by Wengang Wang <[email protected]>
Reviewed-by Changwei Ge <[email protected]>
Quoted 51 lines

---
v1 -> v2: log direct IO failures

---
  fs/ocfs2/aops.c            | 12 ++++++++++--
  fs/ocfs2/cluster/masklog.h |  9 +++++++++
  2 files changed, 19 insertions(+), 2 deletions(-)
fs/ocfs2/aops.c +10 -2
index 302cd7c..7578bd5 100644
@@ -2412,8 +2412,16 @@ 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 (bytes <= 0)
+		mlog_ratelimited(ML_ERROR, "Direct IO failed, bytes = %lld",
+				 (long long)bytes);
+	if (private) {
+		if (bytes > 0)
+			ret = ocfs2_dio_end_io_write(inode, private, offset,
+						     bytes);
+		else
+			ocfs2_dio_free_write_ctx(inode, private);
+	}
 
  	ocfs2_iocb_clear_rw_locked(iocb);
fs/ocfs2/cluster/masklog.h +9 -0
index 308ea0e..a396096 100644
@@ -178,6 +178,15 @@ do {									\
  			      ##__VA_ARGS__);				\
  } while (0)
 
+#define mlog_ratelimited(mask, fmt, ...)				\
+do {									\
+	static DEFINE_RATELIMIT_STATE(_rs,				\
+				      DEFAULT_RATELIMIT_INTERVAL,	\
+				      DEFAULT_RATELIMIT_BURST);		\
+	if (__ratelimit(&_rs))						\
+		mlog(mask, fmt, ##__VA_ARGS__);				\
+} while (0)
+
  #define mlog_errno(st) ({						\
  	int _st = (st);							\
  	if (_st != -ERESTARTSYS && _st != -EINTR &&			\
#5 lore raw

PATCH v2 1/1 ocfs2: free up write context when direct IO failed

In reply to Wengang Wang
Quoted 18 lines · Wengang Wang

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
Quoted 3 lines

This patch also logs, with frequency limit, direct IO failures.

Signed-off-by Wengang Wang <[email protected]>
Reviewed-by Joseph Qi <[email protected]>
Quoted 51 lines

---
v1 -> v2: log direct IO failures

---
 fs/ocfs2/aops.c            | 12 ++++++++++--
 fs/ocfs2/cluster/masklog.h |  9 +++++++++
 2 files changed, 19 insertions(+), 2 deletions(-)
fs/ocfs2/aops.c +10 -2
index 302cd7c..7578bd5 100644
@@ -2412,8 +2412,16 @@ 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 (bytes <= 0)
+		mlog_ratelimited(ML_ERROR, "Direct IO failed, bytes = %lld",
+				 (long long)bytes);
+	if (private) {
+		if (bytes > 0)
+			ret = ocfs2_dio_end_io_write(inode, private, offset,
+						     bytes);
+		else
+			ocfs2_dio_free_write_ctx(inode, private);
+	}
 
 	ocfs2_iocb_clear_rw_locked(iocb);
fs/ocfs2/cluster/masklog.h +9 -0
index 308ea0e..a396096 100644
@@ -178,6 +178,15 @@ do {									\
 			      ##__VA_ARGS__);				\
 } while (0)
 
+#define mlog_ratelimited(mask, fmt, ...)				\
+do {									\
+	static DEFINE_RATELIMIT_STATE(_rs,				\
+				      DEFAULT_RATELIMIT_INTERVAL,	\
+				      DEFAULT_RATELIMIT_BURST);		\
+	if (__ratelimit(&_rs))						\
+		mlog(mask, fmt, ##__VA_ARGS__);				\
+} while (0)
+
 #define mlog_errno(st) ({						\
 	int _st = (st);							\
 	if (_st != -ERESTARTSYS && _st != -EINTR &&			\
author
#6 lore raw

PATCH v2 1/1 ocfs2: free up write context when direct IO failed

Hi AKPM,

Now we have three reviewed-bys for this patch:

Reviewed-by Junxiao Bi <[email protected]>
Reviewed-by Changwei Ge <[email protected]>
Reviewed-by Joseph Qi <[email protected]>

Could you please merge it to mainline code?

Also thanks Junxiao, Changwei and Joseph for the reviews!

Thanks,
Wengang

Quoted 76 lines · Joseph Qi
Quoted 18 lines · Wengang Wang

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
Quoted 3 lines

This patch also logs, with frequency limit, direct IO failures.

Signed-off-by Wengang Wang <[email protected]>
Reviewed-by Joseph Qi <[email protected]>
Quoted 51 lines

---
v1 -> v2: log direct IO failures

---
  fs/ocfs2/aops.c            | 12 ++++++++++--
  fs/ocfs2/cluster/masklog.h |  9 +++++++++
  2 files changed, 19 insertions(+), 2 deletions(-)
fs/ocfs2/aops.c +10 -2
index 302cd7c..7578bd5 100644
@@ -2412,8 +2412,16 @@ 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 (bytes <= 0)
+		mlog_ratelimited(ML_ERROR, "Direct IO failed, bytes = %lld",
+				 (long long)bytes);
+	if (private) {
+		if (bytes > 0)
+			ret = ocfs2_dio_end_io_write(inode, private, offset,
+						     bytes);
+		else
+			ocfs2_dio_free_write_ctx(inode, private);
+	}
 
  	ocfs2_iocb_clear_rw_locked(iocb);
fs/ocfs2/cluster/masklog.h +9 -0
index 308ea0e..a396096 100644
@@ -178,6 +178,15 @@ do {									\
  			      ##__VA_ARGS__);				\
  } while (0)
 
+#define mlog_ratelimited(mask, fmt, ...)				\
+do {									\
+	static DEFINE_RATELIMIT_STATE(_rs,				\
+				      DEFAULT_RATELIMIT_INTERVAL,	\
+				      DEFAULT_RATELIMIT_BURST);		\
+	if (__ratelimit(&_rs))						\
+		mlog(mask, fmt, ##__VA_ARGS__);				\
+} while (0)
+
  #define mlog_errno(st) ({						\
  	int _st = (st);							\
  	if (_st != -ERESTARTSYS && _st != -EINTR &&			\