Re: [PATCH] exofs: clean up the correct page collection on write error

[Boaz Harrosh <bharrosh@panasas.com>]

On 11/26/2012 04:49 PM, Idan Kedar wrote:
> if ore_write() fails, we would unlock the pages of pcol, which is now
> empty, rather than pcol_copy which owns the pages when ore_write() is
> called. this means that no pages will actually be unlocked
> (pcol.nr_pages == 0) and the writing process (more accurately, the
> syncing process) will hang waiting for a writeback notification that
> never comes.
> 
> moreover, if ore_write() fails, pcol_free() is called for pcol, whereas
> pcol_copy is the object owning the ore_io_state, thus leaking the
> ore_io_state.
> 
> Signed-off-by: Idan Kedar <idank@tonian.com>

Thanks Idan, good catch.

I have simplified your patch a bit, see below. But basically it
is all the same. Please check me out

> ---
>  fs/exofs/inode.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/exofs/inode.c b/fs/exofs/inode.c
> index b561810..5106213 100644
> --- a/fs/exofs/inode.c
> +++ b/fs/exofs/inode.c
> @@ -628,7 +628,7 @@ static int write_exec(struct page_collect *pcol)
>  {
>  	struct exofs_i_info *oi = exofs_i(pcol->inode);
>  	struct ore_io_state *ios;
> -	struct page_collect *pcol_copy = NULL;
> +	struct page_collect *pcol_copy = NULL, *active_pcol = pcol;
>  	int ret;
>  
>  	if (!pcol->pages)
> @@ -658,6 +658,7 @@ static int write_exec(struct page_collect *pcol)
>  
>  	/* pages ownership was passed to pcol_copy */
>  	_pcol_reset(pcol);
> +	active_pcol = pcol_copy;
>  
>  	ret = _maybe_not_all_in_one_io(ios, pcol_copy, pcol);
>  	if (unlikely(ret))
> @@ -676,8 +677,8 @@ static int write_exec(struct page_collect *pcol)
>  	return 0;
>  
>  err:
> -	_unlock_pcol_pages(pcol, ret, WRITE);
> -	pcol_free(pcol);
> +	_unlock_pcol_pages(active_pcol, ret, WRITE);
> +	pcol_free(active_pcol);
>  	kfree(pcol_copy);
>  
>  	return ret;

From: Idan Kedar <idank@tonian.com>
Subject: [PATCH] exofs: clean up the correct page collection on write error

if ore_write() fails, we would unlock the pages of pcol, which is now
empty, rather than pcol_copy which owns the pages when ore_write() is
called. this means that no pages will actually be unlocked
(pcol.nr_pages == 0) and the writing process (more accurately, the
syncing process) will hang waiting for a writeback notification that
never comes.

moreover, if ore_write() fails, pcol_free() is called for pcol, whereas
pcol_copy is the object owning the ore_io_state, thus leaking the
ore_io_state.

[Boaz]
I have simplified Idan's original patch a bit, everything else still
holds

Signed-off-by: Idan Kedar <idank@tonian.com>
Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 fs/exofs/inode.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/exofs/inode.c b/fs/exofs/inode.c
index 19dcc7b..8d82624 100644
--- a/fs/exofs/inode.c
+++ b/fs/exofs/inode.c
@@ -676,8 +676,10 @@ static int write_exec(struct page_collect *pcol)
 	return 0;
 
 err:
-	_unlock_pcol_pages(pcol, ret, WRITE);
-	pcol_free(pcol);
+	if (!pcol_copy) /* Failed before ownership transfer */
+		pcol_copy = pcol;
+	_unlock_pcol_pages(pcol_copy, ret, WRITE);
+	pcol_free(pcol_copy);
 	kfree(pcol_copy);
 
 	return ret;
-- 
1.7.10.2.677.gb6bc67f