* [PATCH 1/3] f2fs: disable the discard option when device does not support @ 2015-05-12 3:48 Jaegeuk Kim 2015-05-12 3:48 ` [PATCH 2/3] f2fs: do not issue next dnode discard redundantly Jaegeuk Kim 2015-05-12 3:48 ` [PATCH 3/3] f2fs: get rid of buggy function Jaegeuk Kim 0 siblings, 2 replies; 5+ messages in thread From: Jaegeuk Kim @ 2015-05-12 3:48 UTC (permalink / raw) To: linux-kernel, linux-fsdevel, linux-f2fs-devel; +Cc: 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 2/3] f2fs: do not issue next dnode discard redundantly 2015-05-12 3:48 [PATCH 1/3] f2fs: disable the discard option when device does not support Jaegeuk Kim @ 2015-05-12 3:48 ` Jaegeuk Kim 2015-05-12 3:48 ` [PATCH 3/3] f2fs: get rid of buggy function Jaegeuk Kim 1 sibling, 0 replies; 5+ messages in thread From: Jaegeuk Kim @ 2015-05-12 3:48 UTC (permalink / raw) To: linux-kernel, linux-fsdevel, linux-f2fs-devel; +Cc: 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH 3/3] f2fs: get rid of buggy function 2015-05-12 3:48 [PATCH 1/3] f2fs: disable the discard option when device does not support Jaegeuk Kim 2015-05-12 3:48 ` [PATCH 2/3] f2fs: do not issue next dnode discard redundantly Jaegeuk Kim @ 2015-05-12 3:48 ` Jaegeuk Kim 2015-05-12 4:10 ` Nicholas Krause 1 sibling, 1 reply; 5+ messages in thread From: Jaegeuk Kim @ 2015-05-12 3:48 UTC (permalink / raw) To: linux-kernel, linux-fsdevel, linux-f2fs-devel; +Cc: 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 ^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH 3/3] f2fs: get rid of buggy function 2015-05-12 3:48 ` [PATCH 3/3] f2fs: get rid of buggy function Jaegeuk Kim @ 2015-05-12 4:10 ` Nicholas Krause 2015-05-12 6:00 ` [f2fs-dev] " Jaegeuk Kim 0 siblings, 1 reply; 5+ messages in thread From: Nicholas Krause @ 2015-05-12 4:10 UTC (permalink / raw) To: Jaegeuk Kim, linux-kernel, linux-fsdevel, linux-f2fs-devel 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 ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [f2fs-dev] [PATCH 3/3] f2fs: get rid of buggy function 2015-05-12 4:10 ` Nicholas Krause @ 2015-05-12 6:00 ` Jaegeuk Kim 0 siblings, 0 replies; 5+ messages in thread From: Jaegeuk Kim @ 2015-05-12 6:00 UTC (permalink / raw) To: Nicholas Krause; +Cc: linux-kernel, linux-fsdevel, linux-f2fs-devel 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. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-05-12 6:00 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2015-05-12 3:48 [PATCH 1/3] f2fs: disable the discard option when device does not support Jaegeuk Kim 2015-05-12 3:48 ` [PATCH 2/3] f2fs: do not issue next dnode discard redundantly Jaegeuk Kim 2015-05-12 3:48 ` [PATCH 3/3] f2fs: get rid of buggy function Jaegeuk Kim 2015-05-12 4:10 ` Nicholas Krause 2015-05-12 6:00 ` [f2fs-dev] " Jaegeuk Kim
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).