[Ocfs2-devel] [PATCH] ocfs2: Avoid livelock in ocfs2_readpage()

[Ocfs2-devel] [PATCH] ocfs2: Avoid livelock in ocfs2_readpage()

[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 <jack@suse.cz>
---
 fs/ocfs2/aops.c |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
index ac97bca..0919e8f 100644
--- a/fs/ocfs2/aops.c
+++ b/fs/ocfs2/aops.c
@@ -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;
 	}
 
-- 
1.7.1

[Ocfs2-devel] [PATCH] ocfs2: Avoid livelock in ocfs2_readpage()

[Sunil Mushran]

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

On 06/23/2011 01:51 PM, Jan Kara wrote:
> 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<jack@suse.cz>
> ---
>   fs/ocfs2/aops.c |    8 ++++++++
>   1 files changed, 8 insertions(+), 0 deletions(-)
>
> diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
> index ac97bca..0919e8f 100644
> --- a/fs/ocfs2/aops.c
> +++ b/fs/ocfs2/aops.c
> @@ -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;
>   	}
>

[Ocfs2-devel] [PATCH] ocfs2: Avoid livelock in ocfs2_readpage()

[Jan Kara]

On Fri 24-06-11 13:55:28, Sunil Mushran wrote:
> 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
> On 06/23/2011 01:51 PM, Jan Kara wrote:
> >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<jack@suse.cz>
> >---
> >  fs/ocfs2/aops.c |    8 ++++++++
> >  1 files changed, 8 insertions(+), 0 deletions(-)
> >
> >diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
> >index ac97bca..0919e8f 100644
> >--- a/fs/ocfs2/aops.c
> >+++ b/fs/ocfs2/aops.c
> >@@ -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;
> >  	}
> >
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

[Ocfs2-devel] [PATCH] ocfs2: Avoid livelock in ocfs2_readpage()

[Joel Becker]

On Thu, Jun 23, 2011 at 10:51:47PM +0200, Jan Kara wrote:
> 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 <jack@suse.cz>
> ---
>  fs/ocfs2/aops.c |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
> index ac97bca..0919e8f 100644
> --- a/fs/ocfs2/aops.c
> +++ b/fs/ocfs2/aops.c
> @@ -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;
>  	}

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

-- 

"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

[Ocfs2-devel] [PATCH] ocfs2: Avoid livelock in ocfs2_readpage()

[Jan Kara]

On Sun 26-06-11 00:26:44, Joel Becker wrote:
> On Thu, Jun 23, 2011 at 10:51:47PM +0200, Jan Kara wrote:
> > 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 <jack@suse.cz>
> > ---
> >  fs/ocfs2/aops.c |    8 ++++++++
> >  1 files changed, 8 insertions(+), 0 deletions(-)
> > 
> > diff --git a/fs/ocfs2/aops.c b/fs/ocfs2/aops.c
> > index ac97bca..0919e8f 100644
> > --- a/fs/ocfs2/aops.c
> > +++ b/fs/ocfs2/aops.c
> > @@ -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;
> >  	}
> 
> 	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
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

[Ocfs2-devel] [PATCH] ocfs2: Avoid livelock in ocfs2_readpage()

[Joel Becker]

On Thu, Jun 23, 2011 at 10:51:47PM +0200, Jan Kara wrote:
> 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 <jack@suse.cz>

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

Joel

-- 

Life's Little Instruction Book #30

	"Never buy a house without a fireplace."

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