Skip to contentSkip to conversation

ocfs2: fix deadlock on mmapped page in ocfs2_write_begin_nolock()

PATCH New Eric Ren 3 participants 10 messages Sep 10, 2016 – Sep 14, 2016 OCFS2

lore
Share & tools
lore

Eric Ren

author
#0 lore raw

PATCH 1/1 ocfs2: fix deadlock on mmapped page in ocfs2_write_begin_nolock()

The testcase "mmaptruncate" of ocfs2-test deadlocked occasionally.

In this testcase, we create a 2*CLUSTER_SIZE file and mmap() on it; there are 2 process repeatedly performing the following operations
respectively: one is doing memset(mmaped_addr + 2*CLUSTER_SIZE - 1, 'a', 1), while the another is playing ftruncate(fd, 2*CLUSTER_SIZE) and then ftruncate(fd, CLUSTER_SIZE) again and again.

This is the backtrace when the deadlock happens:

[<ffffffff817054f0>] __wait_on_bit_lock+0x50/0xa0
[<ffffffff81199bd7>] __lock_page+0xb7/0xc0
[<ffffffff810c4de0>] ? autoremove_wake_function+0x40/0x40
[<ffffffffa0440f4f>] ocfs2_write_begin_nolock+0x163f/0x1790 [ocfs2]
[<ffffffffa0462a50>] ? ocfs2_allocate_extend_trans+0x180/0x180 [ocfs2]
[<ffffffffa0467b47>] ocfs2_page_mkwrite+0x1c7/0x2a0 [ocfs2]
[<ffffffff811cf286>] do_page_mkwrite+0x66/0xc0
[<ffffffff811d3635>] handle_mm_fault+0x685/0x1350
[<ffffffff81039dc0>] ? __fpu__restore_sig+0x70/0x530
[<ffffffff810694c8>] __do_page_fault+0x1d8/0x4d0
[<ffffffff81069827>] trace_do_page_fault+0x37/0xf0
[<ffffffff81061e69>] do_async_page_fault+0x19/0x70
[<ffffffff8170ac98>] async_page_fault+0x28/0x30

In ocfs2_write_begin_nolock(), we first grab the pages and then allocate disk space for this write; ocfs2_try_to_free_truncate_log() will be called if ENOSPC is turned; if we're lucky to get enough clusters, which is usually the case, we start over again. But in ocfs2_free_write_ctxt() the target page isn't unlocked, so we will deadlock when trying to grab the target page again.

Fix this issue by unlocking the target page after we fail to allocate enough space at the first time.

Jan Kara helps me clear out the JBD2 part, and suggest the hint for root cause.

---
 fs/ocfs2/aops.c | 7 +++++++
 1 file changed, 7 insertions(+)
fs/ocfs2/aops.c +7 -0
index 98d3654..78d1d67 100644
@@ -1860,6 +1860,13 @@ out:
 		 */
 		try_free = 0;
 
+		/*
+		 * Unlock mmap_page because the page has been locked when we
+		 * are here.
+		 */
+		if (mmap_page)
+			unlock_page(mmap_page);
+
 		ret1 = ocfs2_try_to_free_truncate_log(osb, clusters_need);
 		if (ret1 == 1)
 			goto try_again;
Signature · 1 line
2.6.6
#1 lore raw

PATCH 1/1 ocfs2: fix deadlock on mmapped page in ocfs2_write_begin_nolock()

Hi Eric,

Quoted 23 lines · Eric Ren

The testcase "mmaptruncate" of ocfs2-test deadlocked occasionally.

In this testcase, we create a 2*CLUSTER_SIZE file and mmap() on it; there are 2 process repeatedly performing the following operations
respectively: one is doing memset(mmaped_addr + 2*CLUSTER_SIZE - 1, 'a', 1), while the another is playing ftruncate(fd, 2*CLUSTER_SIZE) and then ftruncate(fd, CLUSTER_SIZE) again and again.

This is the backtrace when the deadlock happens:

[<ffffffff817054f0>] __wait_on_bit_lock+0x50/0xa0
[<ffffffff81199bd7>] __lock_page+0xb7/0xc0
[<ffffffff810c4de0>] ? autoremove_wake_function+0x40/0x40
[<ffffffffa0440f4f>] ocfs2_write_begin_nolock+0x163f/0x1790 [ocfs2]
[<ffffffffa0462a50>] ? ocfs2_allocate_extend_trans+0x180/0x180 [ocfs2]
[<ffffffffa0467b47>] ocfs2_page_mkwrite+0x1c7/0x2a0 [ocfs2]
[<ffffffff811cf286>] do_page_mkwrite+0x66/0xc0
[<ffffffff811d3635>] handle_mm_fault+0x685/0x1350
[<ffffffff81039dc0>] ? __fpu__restore_sig+0x70/0x530
[<ffffffff810694c8>] __do_page_fault+0x1d8/0x4d0
[<ffffffff81069827>] trace_do_page_fault+0x37/0xf0
[<ffffffff81061e69>] do_async_page_fault+0x19/0x70
[<ffffffff8170ac98>] async_page_fault+0x28/0x30
Quoted 6 lines

In ocfs2_write_begin_nolock(), we first grab the pages and then allocate disk space for this write; ocfs2_try_to_free_truncate_log() will be called if ENOSPC is turned; if we're lucky to get enough clusters, which is usually the case, we start over again. But in ocfs2_free_write_ctxt() the target page isn't unlocked, so we will deadlock when trying to grab the target page again.

IMO, in ocfs2_grab_pages_for_write, mmap_page is mapping to w_pages and w_target_locked is set to true, and then will be unlocked by ocfs2_unlock_pages in ocfs2_free_write_ctxt.
So I'm not getting the case "page isn't unlock". Could you please explain it in more detail?

Thanks,
Joseph

Quoted 30 lines

Fix this issue by unlocking the target page after we fail to allocate enough space at the first time.

Jan Kara helps me clear out the JBD2 part, and suggest the hint for root cause.

---
 fs/ocfs2/aops.c | 7 +++++++
 1 file changed, 7 insertions(+)
fs/ocfs2/aops.c +7 -0
index 98d3654..78d1d67 100644
@@ -1860,6 +1860,13 @@ out:
 		 */
 		try_free = 0;
 
+		/*
+		 * Unlock mmap_page because the page has been locked when we
+		 * are here.
+		 */
+		if (mmap_page)
+			unlock_page(mmap_page);
+
 		ret1 = ocfs2_try_to_free_truncate_log(osb, clusters_need);
 		if (ret1 == 1)
 			goto try_again;
author
#2 lore raw

PATCH 1/1 ocfs2: fix deadlock on mmapped page in ocfs2_write_begin_nolock()

Hi Joseph,

Quoted 32 lines · Joseph Qi

Hi Eric,

Quoted 23 lines · Eric Ren

The testcase "mmaptruncate" of ocfs2-test deadlocked occasionally.

In this testcase, we create a 2*CLUSTER_SIZE file and mmap() on it; there are 2 process repeatedly performing the following operations
respectively: one is doing memset(mmaped_addr + 2*CLUSTER_SIZE - 1, 'a', 1), while the another is playing ftruncate(fd, 2*CLUSTER_SIZE) and then ftruncate(fd, CLUSTER_SIZE) again and again.

This is the backtrace when the deadlock happens:

[<ffffffff817054f0>] __wait_on_bit_lock+0x50/0xa0
[<ffffffff81199bd7>] __lock_page+0xb7/0xc0
[<ffffffff810c4de0>] ? autoremove_wake_function+0x40/0x40
[<ffffffffa0440f4f>] ocfs2_write_begin_nolock+0x163f/0x1790 [ocfs2]
[<ffffffffa0462a50>] ? ocfs2_allocate_extend_trans+0x180/0x180 [ocfs2]
[<ffffffffa0467b47>] ocfs2_page_mkwrite+0x1c7/0x2a0 [ocfs2]
[<ffffffff811cf286>] do_page_mkwrite+0x66/0xc0
[<ffffffff811d3635>] handle_mm_fault+0x685/0x1350
[<ffffffff81039dc0>] ? __fpu__restore_sig+0x70/0x530
[<ffffffff810694c8>] __do_page_fault+0x1d8/0x4d0
[<ffffffff81069827>] trace_do_page_fault+0x37/0xf0
[<ffffffff81061e69>] do_async_page_fault+0x19/0x70
[<ffffffff8170ac98>] async_page_fault+0x28/0x30
Quoted 6 lines

In ocfs2_write_begin_nolock(), we first grab the pages and then allocate disk space for this write; ocfs2_try_to_free_truncate_log() will be called if ENOSPC is turned; if we're lucky to get enough clusters, which is usually the case, we start over again. But in ocfs2_free_write_ctxt() the target page isn't unlocked, so we will deadlock when trying to grab the target page again.

Quoted 5 lines

IMO, in ocfs2_grab_pages_for_write, mmap_page is mapping to w_pages and w_target_locked is set to true, and then will be unlocked by ocfs2_unlock_pages in ocfs2_free_write_ctxt.
So I'm not getting the case "page isn't unlock". Could you please explain it in more detail?

Thanks for review;-) Follow up the calling chain:

ocfs2_free_write_ctxt()
->ocfs2_unlock_pages()

in ocfs2_unlock_pages (https://github.com/torvalds/linux/blob/master/fs/ocfs2/aops.c#L793), we can see the code just put_page(target_page), but not unlock it.

Yeah, I will think this a bit more like:
why not unlock the target_page there? Is there other potential problems if the "ret" is not "-ENOSPC" but
other possible error code?

Thanks,
Eric

Quoted 22 lines

Thanks,
Joseph

Quoted 18 lines

Fix this issue by unlocking the target page after we fail to allocate enough space at the first time.

Jan Kara helps me clear out the JBD2 part, and suggest the hint for root cause.

---
  fs/ocfs2/aops.c | 7 +++++++
  1 file changed, 7 insertions(+)
fs/ocfs2/aops.c +0 -0
index 98d3654..78d1d67 100644
@@ -1860,6 +1860,13 @@ out:
  		 */
  		try_free = 0;
 
Quoted 13 lines

fs/ocfs2/aops.c out: · line 1872

Quoted 11 lines
fs/ocfs2/aops.c +7 -0
+		/*
+		 * Unlock mmap_page because the page has been locked when we
+		 * are here.
+		 */
+		if (mmap_page)
+			unlock_page(mmap_page);
+
  		ret1 = ocfs2_try_to_free_truncate_log(osb, clusters_need);
  		if (ret1 == 1)
  			goto try_again;

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://oss.oracle.com/pipermail/ocfs2-devel/attachments/20160912/73d4d6f7/attachment.html

author
#3 lore raw

PATCH 1/1 ocfs2: fix deadlock on mmapped page in ocfs2_write_begin_nolock()

Hi,

Quoted 10 lines
Quoted 5 lines

IMO, in ocfs2_grab_pages_for_write, mmap_page is mapping to w_pages and w_target_locked is set to true, and then will be unlocked by ocfs2_unlock_pages in ocfs2_free_write_ctxt.
So I'm not getting the case "page isn't unlock". Could you please explain it in more detail?

Thanks for review;-) Follow up the calling chain:

ocfs2_free_write_ctxt()
->ocfs2_unlock_pages()

Quoted 8 lines

in ocfs2_unlock_pages
(https://github.com/torvalds/linux/blob/master/fs/ocfs2/aops.c#L793), we can see the code just put_page(target_page), but not unlock it.

Yeah, I will think this a bit more like:
why not unlock the target_page there? Is there other potential problems if the "ret" is not "-ENOSPC" but
other possible error code?

1. ocfs2_unlock_pages() will be called in ocfs2_write_end_nolock(), in this case, we definitely want to return a locked mmaped page
to VM code (do_page_mkwrite) when VM_FAULT_LOCKED is set.

2. But there's indeed a potential existing deadlock situation:

                                ocfs2_grab_pages_for_write()  ==> return -ENOMEM and with 
the mmaped page locked
                                ocfs2_free_write_ctxt() ==> leave the mmapped page locked
                          ocfs2_write_begin_nolock() ==> return -ENOMEM
                    __ocfs2_page_mkwrite() ==> return VM_FAULT_OMM
              __do_page_mkwrite() ==> deadlock here 

(https://github.com/torvalds/linux/blob/master/mm/memory.c#L2054)
This is another corner case, right?

Anyway, I think this patch is good for the -ENOSPC case. And another patch should be proposed for -ENOMEM case?

Thanks,
Eric

Quoted 40 lines

Thanks,
Eric

Quoted 34 lines

Thanks,
Joseph

Quoted 28 lines

Fix this issue by unlocking the target page after we fail to allocate enough space at the first time.

Jan Kara helps me clear out the JBD2 part, and suggest the hint for root cause.

---
  fs/ocfs2/aops.c | 7 +++++++
  1 file changed, 7 insertions(+)
fs/ocfs2/aops.c +6 -0
index 98d3654..78d1d67 100644
@@ -1860,6 +1860,13 @@ out:
           */
          try_free = 0;
  +        /*
+         * Unlock mmap_page because the page has been locked when we
+         * are here.
+         */
+        if (mmap_page)
+            unlock_page(mmap_page);
+
          ret1 = ocfs2_try_to_free_truncate_log(osb, clusters_need);
          if (ret1 == 1)
              goto try_again;
author
#4 lore raw

PATCH 1/1 ocfs2: fix deadlock on mmapped page in ocfs2_write_begin_nolock()

Hi,

Quoted 30 lines · Eric Ren

Hi,

Quoted 10 lines
Quoted 5 lines

IMO, in ocfs2_grab_pages_for_write, mmap_page is mapping to w_pages and w_target_locked is set to true, and then will be unlocked by ocfs2_unlock_pages in ocfs2_free_write_ctxt.
So I'm not getting the case "page isn't unlock". Could you please explain it in more detail?

Thanks for review;-) Follow up the calling chain:

ocfs2_free_write_ctxt()
->ocfs2_unlock_pages()

Quoted 8 lines

in ocfs2_unlock_pages
(https://github.com/torvalds/linux/blob/master/fs/ocfs2/aops.c#L793), we can see the code just put_page(target_page), but not unlock it.

Yeah, I will think this a bit more like:
why not unlock the target_page there? Is there other potential problems if the "ret" is not "-ENOSPC" but
other possible error code?

1. ocfs2_unlock_pages() will be called in ocfs2_write_end_nolock(), in this case, we definitely want to return a locked mmaped page
to VM code (do_page_mkwrite) when VM_FAULT_LOCKED is set.

2. But there's indeed a potential existing deadlock situation:

                                 ocfs2_grab_pages_for_write()  ==> return -ENOMEM and with
the mmaped page locked
                                 ocfs2_free_write_ctxt() ==> leave the mmapped page locked
                           ocfs2_write_begin_nolock() ==> return -ENOMEM
                     __ocfs2_page_mkwrite() ==> return VM_FAULT_OMM
               __do_page_mkwrite() ==> deadlock here
Quoted 5 lines

(https://github.com/torvalds/linux/blob/master/mm/memory.c#L2054)
This is another corner case, right?

Anyway, I think this patch is good for the -ENOSPC case. And another patch should be proposed for -ENOMEM case?

Yes, I think we can catch both -ENOSPC and -ENOMEM cases in the failure path by unlocking the mmaped page after ocfs2_free_write_ctx(), right?

Eric

Quoted 46 lines

Thanks,
Eric

Quoted 36 lines

Thanks,
Eric

Quoted 32 lines

Thanks,
Joseph

Quoted 28 lines

Fix this issue by unlocking the target page after we fail to allocate enough space at the first time.

Jan Kara helps me clear out the JBD2 part, and suggest the hint for root cause.

---
   fs/ocfs2/aops.c | 7 +++++++
   1 file changed, 7 insertions(+)
fs/ocfs2/aops.c +6 -0
index 98d3654..78d1d67 100644
@@ -1860,6 +1860,13 @@ out:
            */
           try_free = 0;
   +        /*
+         * Unlock mmap_page because the page has been locked when we
+         * are here.
+         */
+        if (mmap_page)
+            unlock_page(mmap_page);
+
           ret1 = ocfs2_try_to_free_truncate_log(osb, clusters_need);
           if (ret1 == 1)
               goto try_again;

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

author
#5 lore raw

PATCH 1/1 ocfs2: fix deadlock on mmapped page in ocfs2_write_begin_nolock()

In reply to Eric Ren

Hi Joseph,

Quoted 11 lines
Quoted 11 lines
Quoted 6 lines

In ocfs2_write_begin_nolock(), we first grab the pages and then allocate disk space for this write; ocfs2_try_to_free_truncate_log() will be called if ENOSPC is turned; if we're lucky to get enough clusters, which is usually the case, we start over again. But in ocfs2_free_write_ctxt() the target page isn't unlocked, so we will deadlock when trying to grab the target page again.

IMO, in ocfs2_grab_pages_for_write, mmap_page is mapping to w_pages and w_target_locked is set to true, and then will be unlocked by ocfs2_unlock_pages in ocfs2_free_write_ctxt.
So I'm not getting the case "page isn't unlock". Could you please explain it in more detail?

Quoted 8 lines

Thanks for review;-) Follow up the calling chain:

ocfs2_free_write_ctxt()
->ocfs2_unlock_pages()

in ocfs2_unlock_pages
(https://github.com/torvalds/linux/blob/master/fs/ocfs2/aops.c#L793), we can see the code just put_page(target_page), but not unlock it.

Did this answer your question?

Thanks,
Eric

Quoted 47 lines

Yeah, I will think this a bit more like:
why not unlock the target_page there? Is there other potential problems if the "ret" is not "-ENOSPC" but
other possible error code?

Thanks,
Eric

Quoted 24 lines

Thanks,
Joseph

Quoted 20 lines

Fix this issue by unlocking the target page after we fail to allocate enough space at the first time.

Jan Kara helps me clear out the JBD2 part, and suggest the hint for root cause.

---
  fs/ocfs2/aops.c | 7 +++++++
  1 file changed, 7 insertions(+)
fs/ocfs2/aops.c +2 -0
index 98d3654..78d1d67 100644
@@ -1860,6 +1860,13 @@ out:
           */
          try_free = 0;
  +        /*
+         * Unlock mmap_page because the page has been locked when we
+         * are here.
Quoted 10 lines

fs/ocfs2/aops.c out: · line 1872

Quoted 8 lines
fs/ocfs2/aops.c +4 -0
+         */
+        if (mmap_page)
+            unlock_page(mmap_page);
+
          ret1 = ocfs2_try_to_free_truncate_log(osb, clusters_need);
          if (ret1 == 1)
              goto try_again;
Quoted 4 lines

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://oss.oracle.com/pipermail/ocfs2-devel/attachments/20160914/f04177a5/attachment.html

#6 lore raw

PATCH 1/1 ocfs2: fix deadlock on mmapped page in ocfs2_write_begin_nolock()

Hi Eric,
Sorry for the delayed response.
I have got your explanation. So we have to unlock the page only in case of retry, right?
If so, I think the unlock should be right before "goto try_again".

Thanks,
Joseph

Quoted 75 lines · Eric Ren

Hi Joseph,

Quoted 11 lines
Quoted 11 lines
Quoted 6 lines

In ocfs2_write_begin_nolock(), we first grab the pages and then allocate disk space for this write; ocfs2_try_to_free_truncate_log() will be called if ENOSPC is turned; if we're lucky to get enough clusters, which is usually the case, we start over again. But in ocfs2_free_write_ctxt() the target page isn't unlocked, so we will deadlock when trying to grab the target page again.

IMO, in ocfs2_grab_pages_for_write, mmap_page is mapping to w_pages and w_target_locked is set to true, and then will be unlocked by ocfs2_unlock_pages in ocfs2_free_write_ctxt.
So I'm not getting the case "page isn't unlock". Could you please explain it in more detail?

Quoted 7 lines

Thanks for review;-) Follow up the calling chain:

ocfs2_free_write_ctxt()
->ocfs2_unlock_pages()

in ocfs2_unlock_pages (https://github.com/torvalds/linux/blob/master/fs/ocfs2/aops.c#L793), we can see the code just put_page(target_page), but not unlock it.

Did this answer your question?

Thanks,
Eric

Quoted 50 lines

Yeah, I will think this a bit more like:
why not unlock the target_page there? Is there other potential problems if the "ret" is not "-ENOSPC" but other possible error code?

Thanks,
Eric

Quoted 24 lines

Thanks,
Joseph

Quoted 20 lines

Fix this issue by unlocking the target page after we fail to allocate enough space at the first time.

Jan Kara helps me clear out the JBD2 part, and suggest the hint for root cause.

---
  fs/ocfs2/aops.c | 7 +++++++
  1 file changed, 7 insertions(+)
fs/ocfs2/aops.c +2 -0
index 98d3654..78d1d67 100644
@@ -1860,6 +1860,13 @@ out:
           */
          try_free = 0;
  +        /*
+         * Unlock mmap_page because the page has been locked when we
+         * are here.
Quoted 10 lines

fs/ocfs2/aops.c out: · line 1872

Quoted 8 lines
fs/ocfs2/aops.c +4 -0
+         */
+        if (mmap_page)
+            unlock_page(mmap_page);
+
          ret1 = ocfs2_try_to_free_truncate_log(osb, clusters_need);
          if (ret1 == 1)
              goto try_again;

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

author
#7 lore raw

PATCH 1/1 ocfs2: fix deadlock on mmapped page in ocfs2_write_begin_nolock()

Hi Joseph,

Quoted 5 lines · Joseph Qi

Hi Eric,
Sorry for the delayed response.
I have got your explanation. So we have to unlock the page only in case of retry, right?
If so, I think the unlock should be right before "goto try_again".

No, the mmapped page should be unlocked as long as we cannot return VM_FAULT_LOCKED to do_page_mkpage(). Otherwise, the deadlock will happen in do_page_mkpage(). Please see the recent 2 mails;-)

Eric

Quoted 77 lines

Thanks,
Joseph

Quoted 70 lines · Eric Ren

Hi Joseph,

Quoted 11 lines
Quoted 11 lines
Quoted 6 lines

In ocfs2_write_begin_nolock(), we first grab the pages and then allocate disk space for this write; ocfs2_try_to_free_truncate_log() will be called if ENOSPC is turned; if we're lucky to get enough clusters, which is usually the case, we start over again. But in ocfs2_free_write_ctxt() the target page isn't unlocked, so we will deadlock when trying to grab the target page again.

IMO, in ocfs2_grab_pages_for_write, mmap_page is mapping to w_pages and w_target_locked is set to true, and then will be unlocked by ocfs2_unlock_pages in ocfs2_free_write_ctxt.
So I'm not getting the case "page isn't unlock". Could you please explain it in more detail?

Quoted 7 lines

Thanks for review;-) Follow up the calling chain:

ocfs2_free_write_ctxt()
->ocfs2_unlock_pages()

in ocfs2_unlock_pages (https://github.com/torvalds/linux/blob/master/fs/ocfs2/aops.c#L793), we can see the code just put_page(target_page), but not unlock it.

Did this answer your question?

Thanks,
Eric

Quoted 46 lines

Yeah, I will think this a bit more like:
why not unlock the target_page there? Is there other potential problems if the "ret" is not "-ENOSPC" but other possible error code?

Thanks,
Eric

Quoted 23 lines

Thanks,
Joseph

Quoted 20 lines

Fix this issue by unlocking the target page after we fail to allocate enough space at the first time.

Jan Kara helps me clear out the JBD2 part, and suggest the hint for root cause.

---
   fs/ocfs2/aops.c | 7 +++++++
   1 file changed, 7 insertions(+)
fs/ocfs2/aops.c +2 -0
index 98d3654..78d1d67 100644
@@ -1860,6 +1860,13 @@ out:
            */
           try_free = 0;
   +        /*
+         * Unlock mmap_page because the page has been locked when we
+         * are here.
Quoted 9 lines

fs/ocfs2/aops.c out: · line 1872

Quoted 8 lines
fs/ocfs2/aops.c +4 -0
+         */
+        if (mmap_page)
+            unlock_page(mmap_page);
+
           ret1 = ocfs2_try_to_free_truncate_log(osb, clusters_need);
           if (ret1 == 1)
               goto try_again;

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

#8 lore raw

PATCH 1/1 ocfs2: fix deadlock on mmapped page in ocfs2_write_begin_nolock()

Okay, IC.
So we have to take care of all errors for ocfs2_write_begin_nolock.

Quoted 94 lines · Eric Ren

Hi Joseph,

Quoted 5 lines · Joseph Qi

Hi Eric,
Sorry for the delayed response.
I have got your explanation. So we have to unlock the page only in case of retry, right?
If so, I think the unlock should be right before "goto try_again".

No, the mmapped page should be unlocked as long as we cannot return VM_FAULT_LOCKED to do_page_mkpage(). Otherwise, the deadlock will happen in do_page_mkpage(). Please see the recent 2 mails;-)

Eric

Quoted 77 lines

Thanks,
Joseph

Quoted 70 lines · Eric Ren

Hi Joseph,

Quoted 11 lines
Quoted 11 lines
Quoted 6 lines

In ocfs2_write_begin_nolock(), we first grab the pages and then allocate disk space for this write; ocfs2_try_to_free_truncate_log() will be called if ENOSPC is turned; if we're lucky to get enough clusters, which is usually the case, we start over again. But in ocfs2_free_write_ctxt() the target page isn't unlocked, so we will deadlock when trying to grab the target page again.

IMO, in ocfs2_grab_pages_for_write, mmap_page is mapping to w_pages and w_target_locked is set to true, and then will be unlocked by ocfs2_unlock_pages in ocfs2_free_write_ctxt.
So I'm not getting the case "page isn't unlock". Could you please explain it in more detail?

Quoted 7 lines

Thanks for review;-) Follow up the calling chain:

ocfs2_free_write_ctxt()
->ocfs2_unlock_pages()

in ocfs2_unlock_pages (https://github.com/torvalds/linux/blob/master/fs/ocfs2/aops.c#L793), we can see the code just put_page(target_page), but not unlock it.

Did this answer your question?

Thanks,
Eric

Quoted 46 lines

Yeah, I will think this a bit more like:
why not unlock the target_page there? Is there other potential problems if the "ret" is not "-ENOSPC" but other possible error code?

Thanks,
Eric

Quoted 23 lines

Thanks,
Joseph

Quoted 20 lines

Fix this issue by unlocking the target page after we fail to allocate enough space at the first time.

Jan Kara helps me clear out the JBD2 part, and suggest the hint for root cause.

---
   fs/ocfs2/aops.c | 7 +++++++
   1 file changed, 7 insertions(+)
fs/ocfs2/aops.c +2 -0
index 98d3654..78d1d67 100644
@@ -1860,6 +1860,13 @@ out:
            */
           try_free = 0;
   +        /*
+         * Unlock mmap_page because the page has been locked when we
+         * are here.
Quoted 9 lines

fs/ocfs2/aops.c out: · line 1872

Quoted 8 lines
fs/ocfs2/aops.c +4 -0
+         */
+        if (mmap_page)
+            unlock_page(mmap_page);
+
           ret1 = ocfs2_try_to_free_truncate_log(osb, clusters_need);
           if (ret1 == 1)
               goto try_again;

_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com
https://oss.oracle.com/mailman/listinfo/ocfs2-devel

.

#9 lore raw

PATCH 1/1 ocfs2: fix deadlock on mmapped page in ocfs2_write_begin_nolock()

In reply to Eric Ren

Thanks
Gang

Quoted 69 lines
Quoted 1 line
Quoted 1 line

The testcase "mmaptruncate" of ocfs2-test deadlocked occasionally.

In this testcase, we create a 2*CLUSTER_SIZE file and mmap() on it; there are 2 process repeatedly performing the following operations
respectively: one is doing memset(mmaped_addr + 2*CLUSTER_SIZE - 1, 'a', 1), while the another is playing ftruncate(fd, 2*CLUSTER_SIZE) and then ftruncate(fd, CLUSTER_SIZE) again and again.

This is the backtrace when the deadlock happens:

[<ffffffff817054f0>] __wait_on_bit_lock+0x50/0xa0
[<ffffffff81199bd7>] __lock_page+0xb7/0xc0
[<ffffffff810c4de0>] ? autoremove_wake_function+0x40/0x40
[<ffffffffa0440f4f>] ocfs2_write_begin_nolock+0x163f/0x1790 [ocfs2]
[<ffffffffa0462a50>] ? ocfs2_allocate_extend_trans+0x180/0x180 [ocfs2]
[<ffffffffa0467b47>] ocfs2_page_mkwrite+0x1c7/0x2a0 [ocfs2]
[<ffffffff811cf286>] do_page_mkwrite+0x66/0xc0
[<ffffffff811d3635>] handle_mm_fault+0x685/0x1350
[<ffffffff81039dc0>] ? __fpu__restore_sig+0x70/0x530
[<ffffffff810694c8>] __do_page_fault+0x1d8/0x4d0
[<ffffffff81069827>] trace_do_page_fault+0x37/0xf0
[<ffffffff81061e69>] do_async_page_fault+0x19/0x70
[<ffffffff8170ac98>] async_page_fault+0x28/0x30

In ocfs2_write_begin_nolock(), we first grab the pages and then allocate disk space for this write; ocfs2_try_to_free_truncate_log() will be called if ENOSPC is turned; if we're lucky to get enough clusters, which is usually the case, we start over again. But in ocfs2_free_write_ctxt()
the target page isn't unlocked, so we will deadlock when trying to grab the target page again.

Fix this issue by unlocking the target page after we fail to allocate enough space at the first time.

Jan Kara helps me clear out the JBD2 part, and suggest the hint for root cause.

---
 fs/ocfs2/aops.c | 7 +++++++
 1 file changed, 7 insertions(+)
fs/ocfs2/aops.c +7 -0
index 98d3654..78d1d67 100644
@@ -1860,6 +1860,13 @@ out:
 		 */
 		try_free = 0;
 
+		/*
+		 * Unlock mmap_page because the page has been locked when we
+		 * are here.
+		 */
+		if (mmap_page)
+			unlock_page(mmap_page);
+
 		ret1 = ocfs2_try_to_free_truncate_log(osb, clusters_need);
 		if (ret1 == 1)
 			goto try_again;
Signature · 7 lines
2.6.6


_______________________________________________
Ocfs2-devel mailing list
Ocfs2-devel at oss.oracle.com 
https://oss.oracle.com/mailman/listinfo/ocfs2-devel