exofs: clean up the correct page collection on write error

PATCH In review Idan Kedar 2 participants 7 messages Nov 26, 2012 – Dec 2, 2012 VFS

Share & tools

Idan Kedar

author

2012-11-26 14:49 UTC #0 lore raw

PATCH 1/1 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.

Signed-off-by Idan Kedar <[email protected]>

---
 fs/exofs/inode.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

fs/exofs/inode.c +4 -3

index b561810..5106213 100644
@@ -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;

Signature · 1 line

1.7.6.5

Boaz Harrosh

2012-11-30 14:10 UTC #1 lore raw

Quoted 7 lines · 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.

Quoted 5 lines

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 <[email protected]>

Thanks Idan, good catch.

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

Quoted 26 lines

---
 fs/exofs/inode.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

fs/exofs/inode.c +2 -1

index b561810..5106213 100644
@@ -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))

fs/exofs/inode.c static int write_exec(struct page_collect *pcol) · line 684

Quoted 10 lines

fs/exofs/inode.c +2 -2

@@ -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 <[email protected]>
Subject: [PATCH] exofs: clean up the correct page collection on write error

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 <[email protected]>

Signed-off-by Boaz Harrosh <[email protected]>

---
 fs/exofs/inode.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

fs/exofs/inode.c +4 -2

index 19dcc7b..8d82624 100644
@@ -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;

Signature · 1 line

1.7.10.2.677.gb6bc67f

Boaz Harrosh

2012-11-30 14:15 UTC #2 lore raw

Quoted 12 lines · Boaz Harrosh

Quoted 11 lines · 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.

Quoted 7 lines

Quoted 1 line
Signed-off-by Idan Kedar <[email protected]>
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

Boaz Harrosh

2012-11-30 14:26 UTC #3 lore raw

PATCH 1/1 exofs: don't leak io_state and pages on read error

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 <[email protected]>

Signed-off-by Boaz Harrosh <[email protected]>

---
 fs/exofs/inode.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

fs/exofs/inode.c +5 -3

index 8d82624..87f05d9 100644
@@ -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;
 }

Signature · 1 line

1.7.10.2.677.gb6bc67f

Boaz Harrosh

2012-11-30 14:28 UTC #4 lore raw

PATCH 1/1 exofs: Remove dead code in read_exec

In reply to 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 <[email protected]>

---
 fs/exofs/inode.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

fs/exofs/inode.c +1 -3

index 87f05d9..449da05 100644
@@ -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;

Signature · 1 line

1.7.10.2.677.gb6bc67f

Idan Kedar

author

2012-12-02 09:58 UTC #5 lore raw

In reply to Boaz Harrosh

Quoted 98 lines · Boaz Harrosh

Quoted 7 lines · 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.
Quoted 5 lines
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 <[email protected]>
Thanks Idan, good catch.
I have simplified your patch a bit, see below. But basically it is all the same. Please check me out
Quoted 26 lines
---
 fs/exofs/inode.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)
fs/exofs/inode.c +2 -1
index b561810..5106213 100644
@@ -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))
fs/exofs/inode.c static int write_exec(struct page_collect *pcol) · line 684
Quoted 10 lines
fs/exofs/inode.c +2 -2
@@ -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 <[email protected]>
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 <[email protected]>
Signed-off-by Boaz Harrosh <[email protected]>
---
 fs/exofs/inode.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)
fs/exofs/inode.c +4 -3
index 19dcc7b..8d82624 100644
@@ -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;
--

Quoted 2 lines

1.7.10.2.677.gb6bc67f

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

Quoted 5 lines · Boaz Harrosh

On 11/30/2012 04:10 PM, Boaz Harrosh wrote:
I forgot to ask do you need this for stable?
Boaz

nope.

Signature · 1 line

idank

Boaz Harrosh

2012-12-02 12:58 UTC #6 lore raw

Quoted 1 line · Idan Kedar

On Fri, Nov 30, 2012 at 4:10 PM, Boaz Harrosh <[email protected]> wrote:

Quoted 9 lines

err:

fs/exofs/inode.c +4 -2

-       _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);

Quoted 7 lines

Quoted 4 lines
        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)

Quoted 9 lines · Boaz Harrosh

Quoted 5 lines · Boaz Harrosh
I forgot to ask do you need this for stable?
Boaz
nope.

Thanks
Boaz