[PATCH 1/1] No need to do recovery if there is no available CP

[PATCH 1/1] No need to do recovery if there is no available CP

[Huajun Li]

From: Huajun Li <huajun.li@intel.com>

Normally we expect an empty partition after formatting by
mkfs.f2fs. But in this case, when we format a dirty partition and mount
it again. The former file will be recovered and available again! and
kernel log shows a recovery procedure is evoked.
This patch adds a new flag CP_EXIST_FLAG to indicate whether is a
available CP, and do recovery only when this flag is set.

You can reproduce the bug by following script:
===================================
	TEST_DEV=/dev/sdb1
	umount $TEST_DEV
	mkfs.f2fs $TEST_DEV > /dev/null
	mount -t f2fs $TEST_DEV /mnt
	dmesg -c > /dev/null

	echo
	echo "Small Vector Sync"
	echo "abcdefghijklmnopqrstuvwxyz" > /mnt/small_vector_async

	xfs_io -F -f -s -c "pread -v 0 1"\
	    -c "pwrite -S 0x61 4090 1"\
	    /mnt/small_vector_async

	umount $TEST_DEV

	mkfs.f2fs $TEST_DEV
	#After we format a partition, there should be nothing but root in it.
	#But in this case, when we format a used partition and mount it again,
	#the former created file small_vector_async will be recover!

	mount -t f2fs $TEST_DEV /mnt
	#We expect nothing after mkfs, but in this case small_vector_async will
	#be recovered when we mount the partition.

	ls /mnt
	dmesg


Signed-off-by: Huajun Li <huajun.li@intel.com>
Reported-and-tested-by: Weihong Xu <weihong.xu@intel.com>
---
 fs/f2fs/checkpoint.c    |    2 ++
 fs/f2fs/super.c         |    3 ++-
 include/linux/f2fs_fs.h |    1 +
 3 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index d430157..22b3972 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -767,6 +767,8 @@ static void do_checkpoint(struct f2fs_sb_info *sbi, bool is_umount)
 		clear_prefree_segments(sbi);
 		F2FS_RESET_SB_DIRT(sbi);
 	}
+	if (!is_set_ckpt_flags(ckpt, CP_EXIST_FLAG))
+		set_ckpt_flags(ckpt, CP_EXIST_FLAG);
 }
 
 /*
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index e42351c..963da7d 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -959,7 +959,8 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
 	}
 
 	/* recover fsynced data */
-	if (!test_opt(sbi, DISABLE_ROLL_FORWARD)) {
+	if (!test_opt(sbi, DISABLE_ROLL_FORWARD) &&
+	    is_set_ckpt_flags(F2FS_CKPT(sbi), CP_EXIST_FLAG)) {
 		err = recover_fsync_data(sbi);
 		if (err)
 			f2fs_msg(sb, KERN_ERR,
diff --git a/include/linux/f2fs_fs.h b/include/linux/f2fs_fs.h
index bb942f6..6e48f22 100644
--- a/include/linux/f2fs_fs.h
+++ b/include/linux/f2fs_fs.h
@@ -80,6 +80,7 @@ struct f2fs_super_block {
 /*
  * For checkpoint
  */
+#define CP_EXIST_FLAG		0x00000010
 #define CP_ERROR_FLAG		0x00000008
 #define CP_COMPACT_SUM_FLAG	0x00000004
 #define CP_ORPHAN_PRESENT_FLAG	0x00000002
-- 
1.7.9.5

Re: [PATCH 1/1] No need to do recovery if there is no available CP

[Jaegeuk Kim]

2013-11-03 (일), 23:08 +0800, Huajun Li:
> From: Huajun Li <huajun.li@intel.com>
> 
> Normally we expect an empty partition after formatting by
> mkfs.f2fs. But in this case, when we format a dirty partition and mount
> it again. The former file will be recovered and available again! and
> kernel log shows a recovery procedure is evoked.
> This patch adds a new flag CP_EXIST_FLAG to indicate whether is a
> available CP, and do recovery only when this flag is set.

IMO, mkfs.f2fs should do the right thing to avoid this.
If storage does not support discard, mkfs.f2fs can simply address the
problem by writing one node block with zeros to prevent this.

And, if you introduce a new flag, mkfs.f2fs should do the same thing,
which means that mkfs.f2fs also needs to set the CP_EXIST_FLAG.
So then, it could not fix the bug.

How do you think? 

-- 
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

Re: [PATCH 1/1] No need to do recovery if there is no available CP

[Huajun Li]

Hi Jaegeuk,

On Mon, Nov 4, 2013 at 9:24 AM, Jaegeuk Kim <jaegeuk.kim@samsung.com> wrote:
> 2013-11-03 (일), 23:08 +0800, Huajun Li:
>> From: Huajun Li <huajun.li@intel.com>
>>
>> Normally we expect an empty partition after formatting by
>> mkfs.f2fs. But in this case, when we format a dirty partition and mount
>> it again. The former file will be recovered and available again! and
>> kernel log shows a recovery procedure is evoked.
>> This patch adds a new flag CP_EXIST_FLAG to indicate whether is a
>> available CP, and do recovery only when this flag is set.
>
> IMO, mkfs.f2fs should do the right thing to avoid this.
> If storage does not support discard, mkfs.f2fs can simply address the
> problem by writing one node block with zeros to prevent this.
>
Yes, mkfs.f2fs should do this definitely. :)

> And, if you introduce a new flag, mkfs.f2fs should do the same thing,
> which means that mkfs.f2fs also needs to set the CP_EXIST_FLAG.
> So then, it could not fix the bug.
>
> How do you think?

IMO, mkfs.f2fs doesn't need to set CP_EXIST_FLAG. This flag indicates
there exists an available CP, so an new formatted partition don't need
set the flag since there is no CP on it yet.

On the other hand, while mounting a partition, we need to do recovery
only if there is CP on the partition. So this flag may be helpful,
right?

>
> --
> Jaegeuk Kim
> Samsung
>
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] No need to do recovery if there is no available CP

[Jaegeuk Kim]

Hi Huajun,

2013-11-04 (월), 23:40 +0800, Huajun Li:
> Hi Jaegeuk,
> 
> On Mon, Nov 4, 2013 at 9:24 AM, Jaegeuk Kim <jaegeuk.kim@samsung.com> wrote:
> > 2013-11-03 (일), 23:08 +0800, Huajun Li:
> >> From: Huajun Li <huajun.li@intel.com>
> >>
> >> Normally we expect an empty partition after formatting by
> >> mkfs.f2fs. But in this case, when we format a dirty partition and mount
> >> it again. The former file will be recovered and available again! and
> >> kernel log shows a recovery procedure is evoked.
> >> This patch adds a new flag CP_EXIST_FLAG to indicate whether is a
> >> available CP, and do recovery only when this flag is set.
> >
> > IMO, mkfs.f2fs should do the right thing to avoid this.
> > If storage does not support discard, mkfs.f2fs can simply address the
> > problem by writing one node block with zeros to prevent this.
> >
> Yes, mkfs.f2fs should do this definitely. :)
> 
> > And, if you introduce a new flag, mkfs.f2fs should do the same thing,
> > which means that mkfs.f2fs also needs to set the CP_EXIST_FLAG.
> > So then, it could not fix the bug.
> >
> > How do you think?
> 
> IMO, mkfs.f2fs doesn't need to set CP_EXIST_FLAG. This flag indicates
> there exists an available CP, so an new formatted partition don't need
> set the flag since there is no CP on it yet.


Ah, what I concern is that mkfs.f2fs writes a valid CP so that it should
set the flag.


> On the other hand, while mounting a partition, we need to do recovery
> only if there is CP on the partition. So this flag may be helpful,
> right?

If there is no valid CP, f2fs never goes to recover path.
So, I doubt the necessity of that flag. :)

Thanks,

-- 
Jaegeuk Kim
Samsung

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] No need to do recovery if there is no available CP

[Huajun Li]

Hi Jaegeuk,

Got it, and nice to fix it in mkfs.f2fs.

Thanks,
--Huajun
On Tue, Nov 5, 2013 at 12:48 PM, Jaegeuk Kim <jaegeuk.kim@samsung.com> wrote:
> Hi Huajun,
>
> 2013-11-04 (월), 23:40 +0800, Huajun Li:
>> Hi Jaegeuk,
>>
>> On Mon, Nov 4, 2013 at 9:24 AM, Jaegeuk Kim <jaegeuk.kim@samsung.com> wrote:
>> > 2013-11-03 (일), 23:08 +0800, Huajun Li:
>> >> From: Huajun Li <huajun.li@intel.com>
>> >>
>> >> Normally we expect an empty partition after formatting by
>> >> mkfs.f2fs. But in this case, when we format a dirty partition and mount
>> >> it again. The former file will be recovered and available again! and
>> >> kernel log shows a recovery procedure is evoked.
>> >> This patch adds a new flag CP_EXIST_FLAG to indicate whether is a
>> >> available CP, and do recovery only when this flag is set.
>> >
>> > IMO, mkfs.f2fs should do the right thing to avoid this.
>> > If storage does not support discard, mkfs.f2fs can simply address the
>> > problem by writing one node block with zeros to prevent this.
>> >
>> Yes, mkfs.f2fs should do this definitely. :)
>>
>> > And, if you introduce a new flag, mkfs.f2fs should do the same thing,
>> > which means that mkfs.f2fs also needs to set the CP_EXIST_FLAG.
>> > So then, it could not fix the bug.
>> >
>> > How do you think?
>>
>> IMO, mkfs.f2fs doesn't need to set CP_EXIST_FLAG. This flag indicates
>> there exists an available CP, so an new formatted partition don't need
>> set the flag since there is no CP on it yet.
>
>
> Ah, what I concern is that mkfs.f2fs writes a valid CP so that it should
> set the flag.
>
>
>> On the other hand, while mounting a partition, we need to do recovery
>> only if there is CP on the partition. So this flag may be helpful,
>> right?
>
> If there is no valid CP, f2fs never goes to recover path.
> So, I doubt the necessity of that flag. :)
>
> Thanks,
>
> --
> Jaegeuk Kim
> Samsung
>
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] No need to do recovery if there is no available CP

[Jaegeuk Kim]

Hi Huajun,

2013-11-05 (화), 21:28 +0800, Huajun Li:
> Hi Jaegeuk,
> 
> Got it, and nice to fix it in mkfs.f2fs.
> 
> Thanks,
> --Huajun
> On Tue, Nov 5, 2013 at 12:48 PM, Jaegeuk Kim <jaegeuk.kim@samsung.com> wrote:
> > Hi Huajun,
> >
> > 2013-11-04 (월), 23:40 +0800, Huajun Li:
> >> Hi Jaegeuk,
> >>
> >> On Mon, Nov 4, 2013 at 9:24 AM, Jaegeuk Kim <jaegeuk.kim@samsung.com> wrote:
> >> > 2013-11-03 (일), 23:08 +0800, Huajun Li:
> >> >> From: Huajun Li <huajun.li@intel.com>
> >> >>
> >> >> Normally we expect an empty partition after formatting by
> >> >> mkfs.f2fs. But in this case, when we format a dirty partition and mount
> >> >> it again. The former file will be recovered and available again! and
> >> >> kernel log shows a recovery procedure is evoked.
> >> >> This patch adds a new flag CP_EXIST_FLAG to indicate whether is a
> >> >> available CP, and do recovery only when this flag is set.
> >> >
> >> > IMO, mkfs.f2fs should do the right thing to avoid this.
> >> > If storage does not support discard, mkfs.f2fs can simply address the
> >> > problem by writing one node block with zeros to prevent this.
> >> >

WRT the below issue, I made a patch for mkfs.f2fs.
If possible, could you test and write a valid patch?
Thanks,

---
 mkfs/f2fs_format.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/mkfs/f2fs_format.c b/mkfs/f2fs_format.c
index 8234b00..18ded79 100644
--- a/mkfs/f2fs_format.c
+++ b/mkfs/f2fs_format.c
@@ -788,7 +788,12 @@ static int f2fs_write_root_inode(void)
 
 	memset(raw_node, 0xff, sizeof(struct f2fs_node));
 
-	main_area_node_seg_blk_offset += F2FS_BLKSIZE;
+	/* avoid power-off-recovery based on roll-forward policy */
+	main_area_node_seg_blk_offset = le32_to_cpu(super_block.main_blkaddr);
+	main_area_node_seg_blk_offset += config.cur_seg[CURSEG_WARM_NODE] *
+					config.blks_per_seg;
+        main_area_node_seg_blk_offset *= blk_size_bytes;
+
 	if (dev_write(raw_node, main_area_node_seg_blk_offset, F2FS_BLKSIZE))
{
 		MSG(1, "\tError: While writing the raw_node to disk!!!\n");
 		return -1;
-- 
1.8.4.474.g128a96c


-- 
Jaegeuk Kim
Samsung

--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/1] No need to do recovery if there is no available CP

[Huajun Li]

On Mon, Nov 11, 2013 at 12:33 PM, Jaegeuk Kim <jaegeuk.kim@samsung.com> wrote:
> Hi Huajun,
>
> 2013-11-05 (화), 21:28 +0800, Huajun Li:
>> Hi Jaegeuk,
>>
>> Got it, and nice to fix it in mkfs.f2fs.
>>
>> Thanks,
>> --Huajun
>> On Tue, Nov 5, 2013 at 12:48 PM, Jaegeuk Kim <jaegeuk.kim@samsung.com> wrote:
>> > Hi Huajun,
>> >
>> > 2013-11-04 (월), 23:40 +0800, Huajun Li:
>> >> Hi Jaegeuk,
>> >>
>> >> On Mon, Nov 4, 2013 at 9:24 AM, Jaegeuk Kim <jaegeuk.kim@samsung.com> wrote:
>> >> > 2013-11-03 (일), 23:08 +0800, Huajun Li:
>> >> >> From: Huajun Li <huajun.li@intel.com>
>> >> >>
>> >> >> Normally we expect an empty partition after formatting by
>> >> >> mkfs.f2fs. But in this case, when we format a dirty partition and mount
>> >> >> it again. The former file will be recovered and available again! and
>> >> >> kernel log shows a recovery procedure is evoked.
>> >> >> This patch adds a new flag CP_EXIST_FLAG to indicate whether is a
>> >> >> available CP, and do recovery only when this flag is set.
>> >> >
>> >> > IMO, mkfs.f2fs should do the right thing to avoid this.
>> >> > If storage does not support discard, mkfs.f2fs can simply address the
>> >> > problem by writing one node block with zeros to prevent this.
>> >> >
>
> WRT the below issue, I made a patch for mkfs.f2fs.
> If possible, could you test and write a valid patch?
> Thanks,
>
Hi Jaegeuk,

Test your following fix just now on the same test machine, it can work fine.
This is new implementation to fix the issue, maybe you can
submit/integrate your patch directly.   ;)

> ---
>  mkfs/f2fs_format.c | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/mkfs/f2fs_format.c b/mkfs/f2fs_format.c
> index 8234b00..18ded79 100644
> --- a/mkfs/f2fs_format.c
> +++ b/mkfs/f2fs_format.c
> @@ -788,7 +788,12 @@ static int f2fs_write_root_inode(void)
>
>         memset(raw_node, 0xff, sizeof(struct f2fs_node));
>
> -       main_area_node_seg_blk_offset += F2FS_BLKSIZE;
> +       /* avoid power-off-recovery based on roll-forward policy */
> +       main_area_node_seg_blk_offset = le32_to_cpu(super_block.main_blkaddr);
> +       main_area_node_seg_blk_offset += config.cur_seg[CURSEG_WARM_NODE] *
> +                                       config.blks_per_seg;
> +        main_area_node_seg_blk_offset *= blk_size_bytes;
> +
>         if (dev_write(raw_node, main_area_node_seg_blk_offset, F2FS_BLKSIZE))
> {
>                 MSG(1, "\tError: While writing the raw_node to disk!!!\n");
>                 return -1;
> --
> 1.8.4.474.g128a96c
>
>
> --
> Jaegeuk Kim
> Samsung
>

------------------------------------------------------------------------------
November Webinars for C, C++, Fortran Developers
Accelerate application performance with scalable programming models. Explore
techniques for threading, error checking, porting, and tuning. Get the most 
from the latest Intel processors and coprocessors. See abstracts and register
http://pubads.g.doubleclick.net/gampad/clk?id=60136231&iu=/4140/ostg.clktrk
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel