From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932123AbbELGAg (ORCPT ); Tue, 12 May 2015 02:00:36 -0400 Received: from mail.kernel.org ([198.145.29.136]:35546 "EHLO mail.kernel.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752211AbbELGAd (ORCPT ); Tue, 12 May 2015 02:00:33 -0400 Date: Mon, 11 May 2015 23:00:30 -0700 From: Jaegeuk Kim To: Nicholas Krause Cc: linux-kernel@vger.kernel.org, linux-fsdevel@vger.kernel.org, linux-f2fs-devel@lists.sourceforge.net Subject: Re: [f2fs-dev] [PATCH 3/3] f2fs: get rid of buggy function Message-ID: <20150512060030.GA25143@jaegeuk-mac02.hsd1.ca.comcast.net> References: <1431402528-24156-1-git-send-email-jaegeuk@kernel.org> <1431402528-24156-3-git-send-email-jaegeuk@kernel.org> <0DE36CEE-3086-4727-9B7B-58D784941567@gmail.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <0DE36CEE-3086-4727-9B7B-58D784941567@gmail.com> User-Agent: Mutt/1.5.21 (2010-09-15) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, May 12, 2015 at 12:10:16AM -0400, Nicholas Krause wrote: > > > On May 11, 2015 11:48:48 PM EDT, Jaegeuk Kim wrote: > >This patch avoids to use a buggy function for now. > >It needs to fix it later. > > > >Signed-off-by: Jaegeuk Kim > >--- > > 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.