From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boaz Harrosh Subject: Re: [PATCH] exofs: clean up the correct page collection on write error Date: Fri, 30 Nov 2012 16:10:40 +0200 Message-ID: <50B8BE60.6090009@panasas.com> References: <1353941346-20598-1-git-send-email-idank@tonian.com> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: 7bit Cc: , , , , To: Idan Kedar Return-path: Received: from natasha.panasas.com ([67.152.220.90]:35654 "EHLO natasha.panasas.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755148Ab2K3OKx (ORCPT ); Fri, 30 Nov 2012 09:10:53 -0500 In-Reply-To: <1353941346-20598-1-git-send-email-idank@tonian.com> Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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 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 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 Signed-off-by: Boaz Harrosh --- 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