[PATCH 1/3] f2fs: disable the discard option when device does not support

[PATCH 1/3] f2fs: disable the discard option when device does not support

[Jaegeuk Kim]

This patch disables given discard option when device does not support it.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/super.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index bd8a405..19438f2 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1186,6 +1186,7 @@ try_onemore:
 			f2fs_msg(sb, KERN_WARNING,
 					"mounting with \"discard\" option, but "
 					"the device does not support discard");
+		clear_opt(sbi, DISCARD);
 	}
 
 	sbi->s_kobj.kset = f2fs_kset;
-- 
2.1.1

[PATCH 2/3] f2fs: do not issue next dnode discard redundantly

[Jaegeuk Kim]

We have a discard map, so that we can avoid redundant discard issues.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/segment.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 2c40ce1..342e0f7 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -486,7 +486,20 @@ static int f2fs_issue_discard(struct f2fs_sb_info *sbi,
 
 void discard_next_dnode(struct f2fs_sb_info *sbi, block_t blkaddr)
 {
-	if (f2fs_issue_discard(sbi, blkaddr, 1)) {
+	int err = -ENOTSUPP;
+
+	if (test_opt(sbi, DISCARD)) {
+		struct seg_entry *se = get_seg_entry(sbi,
+				GET_SEGNO(sbi, blkaddr));
+		unsigned int offset = GET_BLKOFF_FROM_SEG0(sbi, blkaddr);
+
+		if (f2fs_test_bit(offset, se->discard_map))
+			return;
+
+		err = f2fs_issue_discard(sbi, blkaddr, 1);
+	}
+
+	if (err) {
 		struct page *page = grab_meta_page(sbi, blkaddr);
 		/* zero-filled page */
 		set_page_dirty(page);
-- 
2.1.1


------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud 
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y

[PATCH 3/3] f2fs: get rid of buggy function

[Jaegeuk Kim]

This patch avoids to use a buggy function for now.
It needs to fix it later.

Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
---
 fs/f2fs/segment.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 342e0f7..17e89ba 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -64,6 +64,8 @@ static inline unsigned long __reverse_ffs(unsigned long word)
 	return num;
 }
 
+/* FIXME: Do not use this due to a subtle bug */
+#if 0
 /*
  * __find_rev_next(_zero)_bit is copied from lib/find_next_bit.c because
  * f2fs_set_bit makes MSB and LSB reversed in a byte.
@@ -122,6 +124,7 @@ found_first:
 found_middle:
 	return result + __reverse_ffs(tmp);
 }
+#endif
 
 static unsigned long __find_rev_next_zero_bit(const unsigned long *addr,
 			unsigned long size, unsigned long offset)
@@ -542,7 +545,7 @@ static void add_discard_addrs(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 	unsigned long *ckpt_map = (unsigned long *)se->ckpt_valid_map;
 	unsigned long *discard_map = (unsigned long *)se->discard_map;
 	unsigned long *dmap = SIT_I(sbi)->tmp_map;
-	unsigned int start = 0, end = -1;
+	unsigned int start = -1, end = 0;
 	bool force = (cpc->reason == CP_DISCARD);
 	int i;
 
@@ -561,12 +564,14 @@ static void add_discard_addrs(struct f2fs_sb_info *sbi, struct cp_control *cpc)
 				(cur_map[i] ^ ckpt_map[i]) & ckpt_map[i];
 
 	while (force || SM_I(sbi)->nr_discards <= SM_I(sbi)->max_discards) {
-		start = __find_rev_next_bit(dmap, max_blocks, end + 1);
-		if (start >= max_blocks)
-			break;
 
 		end = __find_rev_next_zero_bit(dmap, max_blocks, start + 1);
-		__add_discard_entry(sbi, cpc, se, start, end);
+
+		__add_discard_entry(sbi, cpc, se, start + 1, end);
+
+		if (end >= max_blocks)
+			break;
+		start = end;
 	}
 }
 
-- 
2.1.1


------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud 
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y

Re: [PATCH 3/3] f2fs: get rid of buggy function

[Nicholas Krause]

On May 11, 2015 11:48:48 PM EDT, Jaegeuk Kim <jaegeuk@kernel.org> wrote:
>This patch avoids to use a buggy function for now.
>It needs to fix it later.
>
>Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
>---
> fs/f2fs/segment.c | 15 ++++++++++-----
> 1 file changed, 10 insertions(+), 5 deletions(-)
>
>diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
>index 342e0f7..17e89ba 100644
>--- a/fs/f2fs/segment.c
>+++ b/fs/f2fs/segment.c
>@@ -64,6 +64,8 @@ static inline unsigned long __reverse_ffs(unsigned
>long word)
> 	return num;
> }
> 
>+/* FIXME: Do not use this due to a subtle bug */
>+#if 0
> /*
>* __find_rev_next(_zero)_bit is copied from lib/find_next_bit.c because
>  * f2fs_set_bit makes MSB and LSB reversed in a byte.
>@@ -122,6 +124,7 @@ found_first:
> found_middle:
> 	return result + __reverse_ffs(tmp);
> }
>+#endif
> 
>static unsigned long __find_rev_next_zero_bit(const unsigned long
>*addr,
> 			unsigned long size, unsigned long offset)
>@@ -542,7 +545,7 @@ static void add_discard_addrs(struct f2fs_sb_info
>*sbi, struct cp_control *cpc)
> 	unsigned long *ckpt_map = (unsigned long *)se->ckpt_valid_map;
> 	unsigned long *discard_map = (unsigned long *)se->discard_map;
> 	unsigned long *dmap = SIT_I(sbi)->tmp_map;
>-	unsigned int start = 0, end = -1;
>+	unsigned int start = -1, end = 0;
> 	bool force = (cpc->reason == CP_DISCARD);
> 	int i;
> 
>@@ -561,12 +564,14 @@ static void add_discard_addrs(struct f2fs_sb_info
>*sbi, struct cp_control *cpc)
> 				(cur_map[i] ^ ckpt_map[i]) & ckpt_map[i];
> 
> 	while (force || SM_I(sbi)->nr_discards <= SM_I(sbi)->max_discards) {
>-		start = __find_rev_next_bit(dmap, max_blocks, end + 1);
>-		if (start >= max_blocks)
>-			break;
> 
> 		end = __find_rev_next_zero_bit(dmap, max_blocks, start + 1);
>-		__add_discard_entry(sbi, cpc, se, start, end);
>+
>+		__add_discard_entry(sbi, cpc, se, start + 1, end);
>+
>+		if (end >= max_blocks)
>+			break;
>+		start = end;
> 	}
> }
> 
Rather then avoid that  function, why not fix it.  This seems to add more work in the future and due to this I would like recommend fixing the function,__find_rev_next_zero now. 
IMHO, 
Nick 

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

------------------------------------------------------------------------------
One dashboard for servers and applications across Physical-Virtual-Cloud 
Widest out-of-the-box monitoring support with 50+ applications
Performance metrics, stats and reports that give you Actionable Insights
Deep dive visibility with transaction tracing using APM Insight.
http://ad.doubleclick.net/ddm/clk/290420510;117567292;y

Re: [f2fs-dev] [PATCH 3/3] f2fs: get rid of buggy function

[Jaegeuk Kim]

On Tue, May 12, 2015 at 12:10:16AM -0400, Nicholas Krause wrote:
> 
> 
> On May 11, 2015 11:48:48 PM EDT, Jaegeuk Kim <jaegeuk@kernel.org> wrote:
> >This patch avoids to use a buggy function for now.
> >It needs to fix it later.
> >
> >Signed-off-by: Jaegeuk Kim <jaegeuk@kernel.org>
> >---
> > fs/f2fs/segment.c | 15 ++++++++++-----
> > 1 file changed, 10 insertions(+), 5 deletions(-)
> >
> >diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> >index 342e0f7..17e89ba 100644
> >--- a/fs/f2fs/segment.c
> >+++ b/fs/f2fs/segment.c
> >@@ -64,6 +64,8 @@ static inline unsigned long __reverse_ffs(unsigned
> >long word)
> > 	return num;
> > }
> > 
> >+/* FIXME: Do not use this due to a subtle bug */
> >+#if 0
> > /*
> >* __find_rev_next(_zero)_bit is copied from lib/find_next_bit.c because
> >  * f2fs_set_bit makes MSB and LSB reversed in a byte.
> >@@ -122,6 +124,7 @@ found_first:
> > found_middle:
> > 	return result + __reverse_ffs(tmp);
> > }
> >+#endif
> > 
> >static unsigned long __find_rev_next_zero_bit(const unsigned long
> >*addr,
> > 			unsigned long size, unsigned long offset)
> >@@ -542,7 +545,7 @@ static void add_discard_addrs(struct f2fs_sb_info
> >*sbi, struct cp_control *cpc)
> > 	unsigned long *ckpt_map = (unsigned long *)se->ckpt_valid_map;
> > 	unsigned long *discard_map = (unsigned long *)se->discard_map;
> > 	unsigned long *dmap = SIT_I(sbi)->tmp_map;
> >-	unsigned int start = 0, end = -1;
> >+	unsigned int start = -1, end = 0;
> > 	bool force = (cpc->reason == CP_DISCARD);
> > 	int i;
> > 
> >@@ -561,12 +564,14 @@ static void add_discard_addrs(struct f2fs_sb_info
> >*sbi, struct cp_control *cpc)
> > 				(cur_map[i] ^ ckpt_map[i]) & ckpt_map[i];
> > 
> > 	while (force || SM_I(sbi)->nr_discards <= SM_I(sbi)->max_discards) {
> >-		start = __find_rev_next_bit(dmap, max_blocks, end + 1);
> >-		if (start >= max_blocks)
> >-			break;
> > 
> > 		end = __find_rev_next_zero_bit(dmap, max_blocks, start + 1);
> >-		__add_discard_entry(sbi, cpc, se, start, end);
> >+
> >+		__add_discard_entry(sbi, cpc, se, start + 1, end);
> >+
> >+		if (end >= max_blocks)
> >+			break;
> >+		start = end;
> > 	}
> > }
> > 
> Rather then avoid that  function, why not fix it.  This seems to add more work in the future and due to this I would like recommend fixing the function,__find_rev_next_zero now. 
> IMHO, 

Agreed.
But, in the mean time, it'd be necessary to avoid the bug.
And, I think this will not cause any additional work, since this is a somewhat
f2fs-only function used in very corner cases.

Thanks,

> Nick 
> 
> -- 
> Sent from my Android device with K-9 Mail. Please excuse my brevity.