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

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

[Idan Kedar]

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>
---
 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;
-- 
1.7.6.5

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

[Boaz Harrosh]

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

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

[Boaz Harrosh]

On 11/30/2012 04:10 PM, Boaz Harrosh wrote:
> 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
> 

I forgot to ask do you need this for stable?

Boaz

[PATCH] exofs: don't leak io_state and pages on read error

[Boaz Harrosh]

Same bug as fixed by Idan for write_exec was in read_exec.
Fix the io_state leak and pages state on read error.

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

diff --git a/fs/exofs/inode.c b/fs/exofs/inode.c
index 8d82624..87f05d9 100644
--- a/fs/exofs/inode.c
+++ b/fs/exofs/inode.c
@@ -361,11 +361,13 @@ static int read_exec(struct page_collect *pcol)
 	return 0;
 
 err:
-	if (!pcol->read_4_write)
-		_unlock_pcol_pages(pcol, ret, READ);
+	if (!pcol_copy) /* Failed before ownership transfer */
+		pcol_copy = pcol;
 
-	pcol_free(pcol);
+	if (!pcol->read_4_write)
+		_unlock_pcol_pages(pcol_copy, ret, READ);
 
+	pcol_free(pcol_copy);
 	kfree(pcol_copy);
 	return ret;
 }
-- 
1.7.10.2.677.gb6bc67f

[PATCH] exofs: Remove dead code in read_exec

[Boaz Harrosh]

The if (!pcol->read_4_write) at the error path is redundant
because all goto err; are after the if (pcol->read_4_write)
bale out.

Signed-off-by: Boaz Harrosh <bharrosh@panasas.com>
---
 fs/exofs/inode.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/exofs/inode.c b/fs/exofs/inode.c
index 87f05d9..449da05 100644
--- a/fs/exofs/inode.c
+++ b/fs/exofs/inode.c
@@ -364,9 +364,7 @@ err:
 	if (!pcol_copy) /* Failed before ownership transfer */
 		pcol_copy = pcol;
 
-	if (!pcol->read_4_write)
-		_unlock_pcol_pages(pcol_copy, ret, READ);
-
+	_unlock_pcol_pages(pcol_copy, ret, READ);
 	pcol_free(pcol_copy);
 	kfree(pcol_copy);
 	return ret;
-- 
1.7.10.2.677.gb6bc67f

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

[Idan Kedar]

On Fri, Nov 30, 2012 at 4:10 PM, Boaz Harrosh <bharrosh@panasas.com> wrote:
> 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
>

I started with that implementation, but it seemed less readable to me.

On Fri, Nov 30, 2012 at 4:15 PM, Boaz Harrosh <bharrosh@panasas.com> wrote:
> On 11/30/2012 04:10 PM, Boaz Harrosh wrote:
>
> I forgot to ask do you need this for stable?
>
> Boaz

nope.

-- 
idank

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

[Boaz Harrosh]

On 12/02/2012 11:57 AM, Idan Kedar wrote:
> On Fri, Nov 30, 2012 at 4:10 PM, Boaz Harrosh <bharrosh@panasas.com> wrote:
<>
>>  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
>>
> 
> I started with that implementation, but it seemed less readable to me.
> 

I don't mind the readability, it is not that bad. But I do mind the extra
local variable I'd rather not have it. (I hate it when two places have the
same info, they get out of sync fast, I'd rather do with one)

> On Fri, Nov 30, 2012 at 4:15 PM, Boaz Harrosh <bharrosh@panasas.com> wrote:
>> On 11/30/2012 04:10 PM, Boaz Harrosh wrote:
>>
>> I forgot to ask do you need this for stable?
>>
>> Boaz
> 
> nope.
> 

Thanks
Boaz