Skip to contentSkip to conversation

ocfs2: Avoid livelock in ocfs2_readpage()

PATCH New Jan Kara 3 participants 6 messages Jun 23, 2011 – Jul 28, 2011 OCFS2

lore
Share & tools
lore

Jan Kara

author
#0 lore raw

PATCH 1/1 ocfs2: Avoid livelock in ocfs2_readpage()

When someone writes to an inode, readers accessing the same inode via ocfs2_readpage() just busyloop trying to get ip_alloc_sem because do_generic_file_read() looks up the page again and retries ->readpage() when previous attempt failed with AOP_TRUNCATED_PAGE. When there are enough readers, they can occupy all CPUs and in non-preempt kernel the system is deadlocked because writer holding ip_alloc_sem is never run to release the semaphore. Fix the problem by making reader block on ip_alloc_sem to break the busy loop.

Signed-off-by Jan Kara <[email protected]>
---
 fs/ocfs2/aops.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)
fs/ocfs2/aops.c +8 -0
index ac97bca..0919e8f 100644
@@ -290,7 +290,15 @@ static int ocfs2_readpage(struct file *file, struct page *page)
 	}
 
 	if (down_read_trylock(&oi->ip_alloc_sem) == 0) {
+		/*
+		 * Unlock the page and cycle ip_alloc_sem so that we don't
+		 * busyloop waiting for ip_alloc_sem to unlock
+		 */
 		ret = AOP_TRUNCATED_PAGE;
+		unlock_page(page);
+		unlock = 0;
+		down_read(&oi->ip_alloc_sem);
+		up_read(&oi->ip_alloc_sem);
 		goto out_inode_unlock;
 	}
Signature · 1 line
1.7.1
#1 lore raw

PATCH 1/1 ocfs2: Avoid livelock in ocfs2_readpage()

Looks reasonable. Was this something someone ran into or was this reproduced only via a test workload?

Quoted 34 lines · Jan Kara

When someone writes to an inode, readers accessing the same inode via ocfs2_readpage() just busyloop trying to get ip_alloc_sem because do_generic_file_read() looks up the page again and retries ->readpage() when previous attempt failed with AOP_TRUNCATED_PAGE. When there are enough readers, they can occupy all CPUs and in non-preempt kernel the system is deadlocked because writer holding ip_alloc_sem is never run to release the semaphore. Fix the problem by making reader block on ip_alloc_sem to break the busy loop.

Signed-off-by Jan Kara<[email protected]>
---
  fs/ocfs2/aops.c |    8 ++++++++
  1 files changed, 8 insertions(+), 0 deletions(-)
fs/ocfs2/aops.c +8 -0
index ac97bca..0919e8f 100644
@@ -290,7 +290,15 @@ static int ocfs2_readpage(struct file *file, struct page *page)
  	}
 
  	if (down_read_trylock(&oi->ip_alloc_sem) == 0) {
+		/*
+		 * Unlock the page and cycle ip_alloc_sem so that we don't
+		 * busyloop waiting for ip_alloc_sem to unlock
+		 */
  		ret = AOP_TRUNCATED_PAGE;
+		unlock_page(page);
+		unlock = 0;
+		down_read(&oi->ip_alloc_sem);
+		up_read(&oi->ip_alloc_sem);
  		goto out_inode_unlock;
  	}
author
#2 lore raw

PATCH 1/1 ocfs2: Avoid livelock in ocfs2_readpage()

Quoted 2 lines · Sunil Mushran

Looks reasonable. Was this something someone ran into or was this reproduced only via a test workload?

One of our customers hit this when running installation in KVM whose
filesystem image was stored on OCFS2... Generally loading KVM guest with IO seemed to trigger this fairly regulary.

Honza

Quoted 36 lines · Jan Kara
Quoted 34 lines

When someone writes to an inode, readers accessing the same inode via ocfs2_readpage() just busyloop trying to get ip_alloc_sem because do_generic_file_read() looks up the page again and retries ->readpage() when previous attempt failed with AOP_TRUNCATED_PAGE. When there are enough readers, they can occupy all CPUs and in non-preempt kernel the system is deadlocked because writer holding ip_alloc_sem is never run to release the semaphore. Fix the problem by making reader block on ip_alloc_sem to break the busy loop.

Signed-off-by Jan Kara<[email protected]>
---
 fs/ocfs2/aops.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)
fs/ocfs2/aops.c +8 -0
index ac97bca..0919e8f 100644
@@ -290,7 +290,15 @@ static int ocfs2_readpage(struct file *file, struct page *page)
 	}
 
 	if (down_read_trylock(&oi->ip_alloc_sem) == 0) {
+		/*
+		 * Unlock the page and cycle ip_alloc_sem so that we don't
+		 * busyloop waiting for ip_alloc_sem to unlock
+		 */
 		ret = AOP_TRUNCATED_PAGE;
+		unlock_page(page);
+		unlock = 0;
+		down_read(&oi->ip_alloc_sem);
+		up_read(&oi->ip_alloc_sem);
 		goto out_inode_unlock;
 	}
Signature · 2 lines
Jan Kara <[email protected]>
SUSE Labs, CR
maintainer
#3 lore raw

PATCH 1/1 ocfs2: Avoid livelock in ocfs2_readpage()

In reply to Jan Kara
Quoted 23 lines · Jan Kara

When someone writes to an inode, readers accessing the same inode via ocfs2_readpage() just busyloop trying to get ip_alloc_sem because do_generic_file_read() looks up the page again and retries ->readpage() when previous attempt failed with AOP_TRUNCATED_PAGE. When there are enough readers, they can occupy all CPUs and in non-preempt kernel the system is deadlocked because writer holding ip_alloc_sem is never run to release the semaphore. Fix the problem by making reader block on ip_alloc_sem to break the busy loop.

Signed-off-by Jan Kara <[email protected]>
---
 fs/ocfs2/aops.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)
fs/ocfs2/aops.c +1 -0
index ac97bca..0919e8f 100644
@@ -290,7 +290,15 @@ static int ocfs2_readpage(struct file *file, struct page *page)
 	}
 
 	if (down_read_trylock(&oi->ip_alloc_sem) == 0) {
+		/*

fs/ocfs2/aops.c static int ocfs2_readpage(struct file *file, struct page *page) · line 303

Quoted 10 lines
fs/ocfs2/aops.c +7 -0
+		 * Unlock the page and cycle ip_alloc_sem so that we don't
+		 * busyloop waiting for ip_alloc_sem to unlock
+		 */
 		ret = AOP_TRUNCATED_PAGE;
+		unlock_page(page);
+		unlock = 0;
+		down_read(&oi->ip_alloc_sem);
+		up_read(&oi->ip_alloc_sem);
 		goto out_inode_unlock;
 	}

Question: First, is it safe to drop the page lock here?
Can all callers of readpage (not just g_f_a_r()) handle that?>

Signature · 7 lines
"But all my words come back to me
 In shades of mediocrity.
 Like emptiness in harmony
 I need someone to comfort me."

			http://www.jlbec.org/
			jlbec at evilplan.org
author
#4 lore raw

PATCH 1/1 ocfs2: Avoid livelock in ocfs2_readpage()

Quoted 22 lines · Joel Becker
Quoted 21 lines · Jan Kara

When someone writes to an inode, readers accessing the same inode via ocfs2_readpage() just busyloop trying to get ip_alloc_sem because do_generic_file_read() looks up the page again and retries ->readpage() when previous attempt failed with AOP_TRUNCATED_PAGE. When there are enough readers, they can occupy all CPUs and in non-preempt kernel the system is deadlocked because writer holding ip_alloc_sem is never run to release the semaphore. Fix the problem by making reader block on ip_alloc_sem to break the busy loop.

Signed-off-by Jan Kara <[email protected]>
---
 fs/ocfs2/aops.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)
fs/ocfs2/aops.c +0 -0
index ac97bca..0919e8f 100644
@@ -290,7 +290,15 @@ static int ocfs2_readpage(struct file *file, struct page *page)
 	}
Quoted 15 lines

fs/ocfs2/aops.c static int ocfs2_readpage(struct file *file, struct page *page) · line 303

Quoted 12 lines
fs/ocfs2/aops.c +8 -0
 	if (down_read_trylock(&oi->ip_alloc_sem) == 0) {
+		/*
+		 * Unlock the page and cycle ip_alloc_sem so that we don't
+		 * busyloop waiting for ip_alloc_sem to unlock
+		 */
 		ret = AOP_TRUNCATED_PAGE;
+		unlock_page(page);
+		unlock = 0;
+		down_read(&oi->ip_alloc_sem);
+		up_read(&oi->ip_alloc_sem);
 		goto out_inode_unlock;
 	}

Question: First, is it safe to drop the page lock here?
Can all callers of readpage (not just g_f_a_r()) handle that?

We do exactly the same thing in ocfs2_inode_lock_with_page() so we
definitely don't introduce a new problem. And every caller of readpage() is *supposed* to handle AOP_TRUNCATED_PAGE return value (because page reading can race with truncate) in which case page must be returned unlocked. So it should be safe. Admittedly I did not bother to check the call sites.

Honza

Signature · 2 lines
Jan Kara <[email protected]>
SUSE Labs, CR
maintainer
#5 lore raw

PATCH 1/1 ocfs2: Avoid livelock in ocfs2_readpage()

In reply to Jan Kara
Quoted 10 lines · Jan Kara

When someone writes to an inode, readers accessing the same inode via ocfs2_readpage() just busyloop trying to get ip_alloc_sem because do_generic_file_read() looks up the page again and retries ->readpage() when previous attempt failed with AOP_TRUNCATED_PAGE. When there are enough readers, they can occupy all CPUs and in non-preempt kernel the system is deadlocked because writer holding ip_alloc_sem is never run to release the semaphore. Fix the problem by making reader block on ip_alloc_sem to break the busy loop.

Signed-off-by Jan Kara <[email protected]>

This patch is now in the fixes branch of ocfs2.git.

Joel

Signature · 6 lines
Life's Little Instruction Book #30

	"Never buy a house without a fireplace."

			http://www.jlbec.org/
			jlbec at evilplan.org