* [PATCH] exofs: clean up the correct page collection on write error @ 2012-11-26 14:49 Idan Kedar 2012-11-30 14:10 ` Boaz Harrosh 0 siblings, 1 reply; 7+ messages in thread From: Idan Kedar @ 2012-11-26 14:49 UTC (permalink / raw) To: bharrosh; +Cc: idank, linux-fsdevel, osd-dev, solo, bhalevy, ilya 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] exofs: clean up the correct page collection on write error 2012-11-26 14:49 [PATCH] exofs: clean up the correct page collection on write error Idan Kedar @ 2012-11-30 14:10 ` Boaz Harrosh 2012-11-30 14:15 ` Boaz Harrosh 2012-12-02 9:57 ` [PATCH] exofs: clean up the correct page collection on write error Idan Kedar 0 siblings, 2 replies; 7+ messages in thread From: Boaz Harrosh @ 2012-11-30 14:10 UTC (permalink / raw) To: Idan Kedar; +Cc: linux-fsdevel, osd-dev, solo, bhalevy, ilya 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] exofs: clean up the correct page collection on write error 2012-11-30 14:10 ` Boaz Harrosh @ 2012-11-30 14:15 ` Boaz Harrosh 2012-11-30 14:26 ` [PATCH] exofs: don't leak io_state and pages on read error Boaz Harrosh 2012-11-30 14:28 ` [PATCH] exofs: Remove dead code in read_exec Boaz Harrosh 2012-12-02 9:57 ` [PATCH] exofs: clean up the correct page collection on write error Idan Kedar 1 sibling, 2 replies; 7+ messages in thread From: Boaz Harrosh @ 2012-11-30 14:15 UTC (permalink / raw) To: Idan Kedar; +Cc: linux-fsdevel, osd-dev, solo, bhalevy, ilya 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] exofs: don't leak io_state and pages on read error 2012-11-30 14:15 ` Boaz Harrosh @ 2012-11-30 14:26 ` Boaz Harrosh 2012-11-30 14:28 ` [PATCH] exofs: Remove dead code in read_exec Boaz Harrosh 1 sibling, 0 replies; 7+ messages in thread From: Boaz Harrosh @ 2012-11-30 14:26 UTC (permalink / raw) To: open-osd; +Cc: Idan Kedar, linux-fsdevel 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH] exofs: Remove dead code in read_exec 2012-11-30 14:15 ` Boaz Harrosh 2012-11-30 14:26 ` [PATCH] exofs: don't leak io_state and pages on read error Boaz Harrosh @ 2012-11-30 14:28 ` Boaz Harrosh 1 sibling, 0 replies; 7+ messages in thread From: Boaz Harrosh @ 2012-11-30 14:28 UTC (permalink / raw) To: osd-dev; +Cc: Idan Kedar, linux-fsdevel 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] exofs: clean up the correct page collection on write error 2012-11-30 14:10 ` Boaz Harrosh 2012-11-30 14:15 ` Boaz Harrosh @ 2012-12-02 9:57 ` Idan Kedar 2012-12-02 12:58 ` Boaz Harrosh 1 sibling, 1 reply; 7+ messages in thread From: Idan Kedar @ 2012-12-02 9:57 UTC (permalink / raw) To: Boaz Harrosh; +Cc: linux-fsdevel, osd-dev, solo, bhalevy, ilya 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] exofs: clean up the correct page collection on write error 2012-12-02 9:57 ` [PATCH] exofs: clean up the correct page collection on write error Idan Kedar @ 2012-12-02 12:58 ` Boaz Harrosh 0 siblings, 0 replies; 7+ messages in thread From: Boaz Harrosh @ 2012-12-02 12:58 UTC (permalink / raw) To: Idan Kedar; +Cc: linux-fsdevel, osd-dev, solo, bhalevy, ilya 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-12-02 12:58 UTC | newest] Thread overview: 7+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-11-26 14:49 [PATCH] exofs: clean up the correct page collection on write error Idan Kedar 2012-11-30 14:10 ` Boaz Harrosh 2012-11-30 14:15 ` Boaz Harrosh 2012-11-30 14:26 ` [PATCH] exofs: don't leak io_state and pages on read error Boaz Harrosh 2012-11-30 14:28 ` [PATCH] exofs: Remove dead code in read_exec Boaz Harrosh 2012-12-02 9:57 ` [PATCH] exofs: clean up the correct page collection on write error Idan Kedar 2012-12-02 12:58 ` Boaz Harrosh
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).