Hi,I think we don't need to put the invalid page that get_node_page returned.
So I add " idx = i--" based on your version.Following is the patch:
---
fs/f2fs/node.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 304d5ce..fd5f721 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -631,19 +631,19 @@ static int truncate_partial_nodes(struct dnode_of_data
*dn,
return 0;
/* get indirect nodes in the path */
- for (i = 0; i < depth - 1; i++) {
+ for (i = 0; i < idx + 1; i++) {
/* refernece count'll be increased */
pages[i] = get_node_page(sbi, nid[i]);
if (IS_ERR(pages[i])) {
- depth = i + 1;
err = PTR_ERR(pages[i]);
+ idx = i--;
goto fail;
}
nid[i + 1] = get_nid(pages[i], offset[i + 1], false);
}
/* free direct nodes linked to a partial indirect node */
- for (i = offset[depth - 1]; i < NIDS_PER_BLOCK; i++) {
+ for (i = offset[idx + 1]; i < NIDS_PER_BLOCK; i++) {
child_nid = get_nid(pages[idx], i, false);
if (!child_nid)
continue;
@@ -654,7 +654,7 @@ static int truncate_partial_nodes(struct dnode_of_data *dn,
set_nid(pages[idx], i, 0, false);
}
- if (offset[depth - 1] == 0) {
+ if (offset[idx + 1] == 0) {
dn->node_page = pages[idx];
dn->nid = nid[idx];
truncate_node(dn);
@@ -662,9 +662,10 @@ static int truncate_partial_nodes(struct dnode_of_data *dn,
f2fs_put_page(pages[idx], 1);
}
offset[idx]++;
- offset[depth - 1] = 0;
+ offset[idx + 1] = 0;
+ idx--;
fail:
- for (i = depth - 3; i >= 0; i--)
+ for (i = idx; i >= 0; i--)
f2fs_put_page(pages[i], 1);
trace_f2fs_truncate_partial_nodes(dn->inode, nid, depth, err);
--
1.7.9.5
> -----Original Message-----
> From: Jaegeuk Kim [mailto:jaegeuk....@samsung.com]
> Sent: Monday, October 28, 2013 6:22 PM
> To: shifei10...@samsung.com
> Cc: ??; linux-fsde...@vger.kernel.org; linux-ker...@vger.kernel.org; linux-
> f2fs-de...@lists.sourceforge.net
> Subject: Re: [f2fs-dev] [PATCH] f2fs:fix truncate_partial_nodes bug
>
> Hi,
>
> 2013-10-28 (월), 08:15 +0000, Shifei Ge:
> > truncate_partial_nodes puts pages incorrectly in two cases.note that the
> value for argc 'depth' can only be 2 or 3(see truncate_inode_blocks ---
> >truncate_partial_nodes ).
> > first case:err happened in the first 'for' loop
> > assume depth is 2,when err happened,pages[0] is invalid,so this page
> don't need to be put when func return,there is no problem.
> > but when depth is 3 ,pages isn't put correctly when pages[0] is valid
> and pages[1] is invalid.in this case,depth is set to 2(ref to statemnt depth =
> i + 1) ,and then 'goto fail'.
> > in label 'fail', for (i = depth - 3; i >= 0; i--) cann't meet the
> condition because i=-1,so pages[0] cann't be put when func return.
> > second case:err happened in the second 'for' loop
> > now,we've got pages[0] with argc 'depth' is 2 ,or we've got pages[0]
> and pages[1] with depth is 3.when err happend,we need 'goto fail' to put pages
> we've got.
> > when depth is 2,in label 'fail',for (i = depth - 3; i >= 0; i--) cann't
> meet the condition because i=-1,so pages[0] cann't be put.
> > when depth is 3,in label 'fail',for (i = depth - 3; i >= 0; i--) can
> only put pages[0],pages[1] also cann't be put.
> > notes 'depth' has been changed before first 'goto fail'(ref to statemnt
> depth = i + 1),so pass this modified 'depth' to
> trace_f2fs_truncate_partial_nodes is also incorrectly.
>
> Please write a description not to exceed 80 columns.
> It's very hard to understand.
>
> >
> > From 9de8efa31759ce86a032f5ad092d337b686ede06 Mon Sep 17 00:00:00 2001
> > Signed-off-by: Shifei Ge <shifei10...@samsung.com>
> > ---
> > fs/f2fs/node.c | 6 ++++--
> > 1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c index 304d5ce..9986930
> > 100644
> > --- a/fs/f2fs/node.c
> > +++ b/fs/f2fs/node.c
> > @@ -625,6 +625,7 @@ static int truncate_partial_nodes(struct dnode_of_data
> *dn,
> > int err = 0;
> > int i;
> > int idx = depth - 2;
> > + int dep = depth;
>
> I think we need to avoid using a local variable only for the use of
> tracepoints below.
> How about this?
>
> ---
> fs/f2fs/node.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c index 304d5ce..8578719 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -631,11 +631,10 @@ static int truncate_partial_nodes(struct dnode_of_data
> *dn,
> return 0;
>
> /* get indirect nodes in the path */
> - for (i = 0; i < depth - 1; i++) {
> + for (i = 0; i < idx + 1; i++) {
> /* refernece count'll be increased */
> pages[i] = get_node_page(sbi, nid[i]);
> if (IS_ERR(pages[i])) {
> - depth = i + 1;
> err = PTR_ERR(pages[i]);
> goto fail;
> }
> @@ -643,7 +642,7 @@ static int truncate_partial_nodes(struct dnode_of_data
> *dn,
> }
>
> /* free direct nodes linked to a partial indirect node */
> - for (i = offset[depth - 1]; i < NIDS_PER_BLOCK; i++) {
> + for (i = offset[idx + 1]; i < NIDS_PER_BLOCK; i++) {
> child_nid = get_nid(pages[idx], i, false);
> if (!child_nid)
> continue;
> @@ -654,7 +653,7 @@ static int truncate_partial_nodes(struct dnode_of_data
> *dn,
> set_nid(pages[idx], i, 0, false);
> }
>
> - if (offset[depth - 1] == 0) {
> + if (offset[idx + 1] == 0) {
> dn->node_page = pages[idx];
> dn->nid = nid[idx];
> truncate_node(dn);
> @@ -662,9 +661,10 @@ static int truncate_partial_nodes(struct dnode_of_data
> *dn,
> f2fs_put_page(pages[idx], 1);
> }
> offset[idx]++;
> - offset[depth - 1] = 0;
> + offset[idx + 1] = 0;
> + idx--;
> fail:
> - for (i = depth - 3; i >= 0; i--)
> + for (i = idx; i >= 0; i--)
> f2fs_put_page(pages[i], 1);
>
> trace_f2fs_truncate_partial_nodes(dn->inode, nid, depth, err);
> --
> 1.8.4.474.g128a96c
>
>
>
>
> --
> Jaegeuk Kim
> Samsung
------------------------------------------------------------------------------
Android is increasing in popularity, but the open development platform that
developers love is also attractive to malware creators. Download this white
paper to learn more about secure code signing practices that can help keep
Android apps secure.
http://pubads.g.doubleclick.net/gampad/clk?id=65839951&iu=/4140/ostg.clktrk
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
