On Fri, Nov 30, 2012 at 4:10 PM, Boaz Harrosh <bharrosh@xxxxxxxxxxx> 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@xxxxxxxxxx> > > 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@xxxxxxxxxx> > 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@xxxxxxxxxx> > Signed-off-by: Boaz Harrosh <bharrosh@xxxxxxxxxxx> > --- > 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@xxxxxxxxxxx> wrote: > On 11/30/2012 04:10 PM, Boaz Harrosh wrote: > > I forgot to ask do you need this for stable? > > Boaz nope. -- idank -- To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html