Re: [f2fs-dev] [PATCH] f2fs: introduce "strict_fsync" for posix standard fsync
On 2018/2/2 11:28, Junling Zheng wrote:
> Commit "0a007b97aad6"(f2fs: recover directory operations by fsync)
> fixed xfstest generic/342 case, but it also increased the written
> data and caused the performance degradation. In most cases, there's
> no need to do so heavily fsync actually.
>
> So we introduce a new mount option "strict_fsync" to control the
> policy of fsync. It's set by default, and means that fsync follows
> POSIX semantics. And "nostrict_fsync" means that the behaviour is
> in line with xfs, ext4 and btrfs, on which generic/342 will be passed.
Need to document {,no}strict_fsync in f2fs.txt.
Otherwise, you can add:
Reviewed-by: Chao Yu
Thanks,
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
[f2fs-dev] [PATCH] f2fs: introduce "strict_fsync" for posix standard fsync
Commit "0a007b97aad6"(f2fs: recover directory operations by fsync)
fixed xfstest generic/342 case, but it also increased the written
data and caused the performance degradation. In most cases, there's
no need to do so heavily fsync actually.
So we introduce a new mount option "strict_fsync" to control the
policy of fsync. It's set by default, and means that fsync follows
POSIX semantics. And "nostrict_fsync" means that the behaviour is
in line with xfs, ext4 and btrfs, on which generic/342 will be passed.
Signed-off-by: Junling Zheng
---
fs/f2fs/dir.c | 3 ++-
fs/f2fs/f2fs.h | 1 +
fs/f2fs/file.c | 3 ++-
fs/f2fs/namei.c | 9 ++---
fs/f2fs/super.c | 13 +
5 files changed, 24 insertions(+), 5 deletions(-)
diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
index f00b5ed8c011..7487b7e77a36 100644
--- a/fs/f2fs/dir.c
+++ b/fs/f2fs/dir.c
@@ -713,7 +713,8 @@ void f2fs_delete_entry(struct f2fs_dir_entry *dentry,
struct page *page,
f2fs_update_time(F2FS_I_SB(dir), REQ_TIME);
- add_ino_entry(F2FS_I_SB(dir), dir->i_ino, TRANS_DIR_INO);
+ if (!test_opt(F2FS_I_SB(dir), STRICT_FSYNC))
+ add_ino_entry(F2FS_I_SB(dir), dir->i_ino, TRANS_DIR_INO);
if (f2fs_has_inline_dentry(dir))
return f2fs_delete_inline_entry(dentry, page, dir, inode);
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index dbe87c7a266e..8cf914d12f17 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -97,6 +97,7 @@ extern char *fault_name[FAULT_MAX];
#define F2FS_MOUNT_QUOTA 0x0040
#define F2FS_MOUNT_INLINE_XATTR_SIZE 0x0080
#define F2FS_MOUNT_RESERVE_ROOT0x0100
+#define F2FS_MOUNT_STRICT_FSYNC0x0200
#define clear_opt(sbi, option) ((sbi)->mount_opt.opt &= ~F2FS_MOUNT_##option)
#define set_opt(sbi, option) ((sbi)->mount_opt.opt |= F2FS_MOUNT_##option)
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index 672a542e5464..9b39254f5b48 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -165,7 +165,8 @@ static inline enum cp_reason_type need_do_checkpoint(struct
inode *inode)
cp_reason = CP_FASTBOOT_MODE;
else if (sbi->active_logs == 2)
cp_reason = CP_SPEC_LOG_NUM;
- else if (need_dentry_mark(sbi, inode->i_ino) &&
+ else if (!test_opt(sbi, STRICT_FSYNC) &&
+ need_dentry_mark(sbi, inode->i_ino) &&
exist_written_data(sbi, F2FS_I(inode)->i_pino, TRANS_DIR_INO))
cp_reason = CP_RECOVER_DIR;
diff --git a/fs/f2fs/namei.c b/fs/f2fs/namei.c
index c4c94c7e9f4f..ef86ae327f91 100644
--- a/fs/f2fs/namei.c
+++ b/fs/f2fs/namei.c
@@ -936,7 +936,8 @@ static int f2fs_rename(struct inode *old_dir, struct dentry
*old_dentry,
}
f2fs_i_links_write(old_dir, false);
}
- add_ino_entry(sbi, new_dir->i_ino, TRANS_DIR_INO);
+ if (!test_opt(sbi, STRICT_FSYNC))
+ add_ino_entry(sbi, new_dir->i_ino, TRANS_DIR_INO);
f2fs_unlock_op(sbi);
@@ -1091,8 +1092,10 @@ static int f2fs_cross_rename(struct inode *old_dir,
struct dentry *old_dentry,
}
f2fs_mark_inode_dirty_sync(new_dir, false);
- add_ino_entry(sbi, old_dir->i_ino, TRANS_DIR_INO);
- add_ino_entry(sbi, new_dir->i_ino, TRANS_DIR_INO);
+ if (!test_opt(sbi, STRICT_FSYNC)) {
+ add_ino_entry(sbi, old_dir->i_ino, TRANS_DIR_INO);
+ add_ino_entry(sbi, new_dir->i_ino, TRANS_DIR_INO);
+ }
f2fs_unlock_op(sbi);
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 7966cf7bfb8e..3066fc9d8985 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -130,6 +130,8 @@ enum {
Opt_jqfmt_vfsv0,
Opt_jqfmt_vfsv1,
Opt_whint,
+ Opt_strict_fsync,
+ Opt_nostrict_fsync,
Opt_err,
};
@@ -184,6 +186,8 @@ static match_table_t f2fs_tokens = {
{Opt_jqfmt_vfsv0, "jqfmt=vfsv0"},
{Opt_jqfmt_vfsv1, "jqfmt=vfsv1"},
{Opt_whint, "whint_mode=%s"},
+ {Opt_strict_fsync, "strict_fsync"},
+ {Opt_nostrict_fsync, "nostrict_fsync"},
{Opt_err, NULL},
};
@@ -700,6 +704,12 @@ static int parse_options(struct super_block *sb, char
*options)
}
kfree(name);
break;
+ case Opt_strict_fsync:
+ set_opt(sbi, STRICT_FSYNC);
+ break;
+ case Opt_nostrict_fsync:
+ clear_opt(sbi, STRICT_FSYNC);
+ break;
default:
f2fs_msg(sb, KERN_ERR,
"Unrecognized mount option \"%s\" or missing
value",
@@ -1296,6 +1306,9 @@ static void default_options(struct f2fs_sb_info *sbi)
set_opt(sbi, POSIX_ACL);
#endif
+ /* POSIX standard fsync */
+ set_opt(sbi, STRICT_FSYNC);
+
#ifdef CONFIG_F2FS_FAULT_INJECTION
f2fs_build_fault_attr(sbi, 0);
#e
Re: [f2fs-dev] [PATCH] f2fs: use crc ^ cp_ver instead of crc | cp_ver for recovery
Hi Chao, On 2018/2/1 22:30, Chao Yu wrote: We can use this calculation since cp_ver is complete 64-bits random number now. Sorry, I meant we can't. Alright, I think it is an unimportant proposal for now. We could fix now, sometime later or never :) Just saw by chance and a little suggestion to the community... Thanks, Thanks, -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] f2fs: use crc ^ cp_ver instead of crc | cp_ver for recovery
> We can use this calculation since cp_ver is complete 64-bits random number > now. Sorry, I meant we can't. Thanks, -- Check out the vibrant tech community on one of the world's most engaging tech sites, Slashdot.org! http://sdm.link/slashdot ___ Linux-f2fs-devel mailing list Linux-f2fs-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] f2fs: use crc ^ cp_ver instead of crc | cp_ver for recovery
On 2018/2/1 22:06, Gao Xiang wrote: > > > On 2018/2/1 21:57, Gao Xiang wrote: >> Hi Chao, >> >> On 2018/2/1 21:27, Chao Yu wrote: >>> Hi Xiang, >>> >>> On 2018/2/1 0:16, Gaoxiang (OS) wrote: This patch add a flag CP_CRC_RECOVERY_XOR_FLAG to use XORed crc ^ cp_ver since crc | cp_ver is more likely to get a collision or become 11.. | cp_ver. >>> >>> FYI, we have discussed about this before: >>> >>> https://patchwork.kernel.org/patch/9342639/ >>> >>> At that time, cp_ver will always be initialized with 1, so there is almost a >>> little chance to use high 32 bits, result in less collision, so I think it >>> will be OK. >>> >>> But now, cp_ver will be initialized with a random 64 bits value, then the >>> collision will be increased, I agree that xor method will be better, but I'm >>> not sure we should use this, since layout change makes complicated handling >>> in codes for back compatibility. >>> >>> And do you encounter any incorrect recovery, or is there any following >>> feature >>> rely on this? >> > > > >> No... I just looked at node_footer because of another work and saw this part >> of code by chance. >> I think XOR-ed calculation is much better in mathematics, and typical method >> for mixing two values (eg. XOR encryption) is also XOR-ed calculation rather >> than OR-ed... Yes, I think so. >> >> Thanks, >> > > BTW, > "The crc is already random enough, but has 32bits only. > The cp_ver is not easy to use over 32bits, so we don't need to keep the other > 32bits untouched in most of life." > > I observe cp_ver(if the high 32 bits are 0) ^ crc == cp_ver | crc, but We can use this calculation since cp_ver is complete 64-bits random number now. Thanks, > if cp_ver is over 32bits, ... > > ...hmmm... > > Thanks, > >>> >>> Thanks, >>> Signed-off-by: Gao Xiang --- fs/f2fs/checkpoint.c | 4 ++-- fs/f2fs/node.h | 16 +++- fs/f2fs/segment.c | 3 ++- include/linux/f2fs_fs.h | 1 + 4 files changed, 16 insertions(+), 8 deletions(-) diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c index 8b0945b..9e7e63b 100644 --- a/fs/f2fs/checkpoint.c +++ b/fs/f2fs/checkpoint.c @@ -1157,8 +1157,8 @@ static void update_ckpt_flags(struct f2fs_sb_info *sbi, struct cp_control *cpc) if (is_sbi_flag_set(sbi, SBI_NEED_FSCK)) __set_ckpt_flags(ckpt, CP_FSCK_FLAG); - /* set this flag to activate crc|cp_ver for recovery */ - __set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG); + /* set this flag to activate crc^cp_ver for recovery */ + __set_ckpt_flags(ckpt, CP_CRC_RECOVERY_XOR_FLAG); __clear_ckpt_flags(ckpt, CP_NOCRC_RECOVERY_FLAG); spin_unlock_irqrestore(&sbi->cp_lock, flags); diff --git a/fs/f2fs/node.h b/fs/f2fs/node.h index 081ef0d..7b9489f 100644 --- a/fs/f2fs/node.h +++ b/fs/f2fs/node.h @@ -293,8 +293,11 @@ static inline void fill_node_footer_blkaddr(struct page *page, block_t blkaddr) struct f2fs_node *rn = F2FS_NODE(page); __u64 cp_ver = cur_cp_version(ckpt); - if (__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG)) - cp_ver |= (cur_cp_crc(ckpt) << 32); + if (__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_XOR_FLAG)) + cp_ver ^= cur_cp_crc(ckpt) << 32; + /* for backward compatibility */ + else if (unlikely(__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG))) + cp_ver |= cur_cp_crc(ckpt) << 32; rn->footer.cp_ver = cpu_to_le64(cp_ver); rn->footer.next_blkaddr = cpu_to_le32(blkaddr); @@ -307,10 +310,13 @@ static inline bool is_recoverable_dnode(struct page *page) /* Don't care crc part, if fsck.f2fs sets it. */ if (__is_set_ckpt_flags(ckpt, CP_NOCRC_RECOVERY_FLAG)) - return (cp_ver << 32) == (cpver_of_node(page) << 32); + return (__u32)cp_ver == (__u32)cpver_of_node(page); - if (__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG)) - cp_ver |= (cur_cp_crc(ckpt) << 32); + if (__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_XOR_FLAG)) + cp_ver ^= cur_cp_crc(ckpt) << 32; + /* for backward compatibility */ + else if (unlikely(__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG))) + cp_ver |= cur_cp_crc(ckpt) << 32; return cp_ver == cpver_of_node(page); } diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c index 205b0d9..64d0c1f 100644 --- a/fs/f2fs/segment.c +++ b/fs/f2fs/segment.c @@ -2316,7 +2316,8 @@ static void allocate_segment_by_default(struct f2fs_sb_info *sbi, if (force) new_curseg(sbi, type, true); - else if (!is_set_ckpt_flags(sbi, CP_CRC_RECOVERY_FLAG) && + else if (!(is_set_ckpt_flags(sbi, CP_CRC_RECOVERY_FLAG) || + is_set_ckpt_flags(sbi, CP_CRC_RECOVERY_XOR_FL
Re: [f2fs-dev] [PATCH 2/2] f2fs: support {d,id,did,x}node checksum
On 2018/2/1 6:15, Jaegeuk Kim wrote:
> On 01/31, Chao Yu wrote:
>> On 2018/1/31 10:02, Jaegeuk Kim wrote:
>>> What if we want to add more entries in addition to node_checksum? Do we have
>>> to add a new feature flag at every time? How about adding a layout value
>>> instead
>>
>> Hmm.. for previous implementation, IMO, we'd better add a new feature flag at
>> every time, otherwise, w/ extra_nsize only, in current image, we can know a
>> valid range of extended area in node block, but we don't know which
>> fields/features are valid/enabled or not.
>>
>> One more thing is that if we can add one feature flag for each field, we got
>> one
>> more chance to disable it dynamically.
>>
>>> of extra_nsize? For example, layout #1 means node_checksum with
>>> extra_nsize=X?
>>>
>>>
>>> What does 1017 mean? We need to make this structure more flexibly for new
>>
>> Yes, using raw 1017 is not appropriate here.
>>
>>> entries. Like this?
>>> union {
>>> struct node_v1;
>>> struct node_v2;
>>> struct node_v3;
>>> ...
>>> struct direct_node dn;
>>> struct indirect_node in;
>>> };
>>> };
>>>
>>> struct node_v1 {
>>> __le32 data[DEF_ADDRS_PER_BLOCK - V1_NSIZE=1];
>>> __le32 node_checksum;
>>> }
>>>
>>> struct node_v2 {
>>> __le32 data[DEF_ADDRS_PER_BLOCK - V2_NSIZE=500];
>>
>> Hmm.. If we only need to add one more 4 bytes field in struct node_v2, but
>> V2_NSIZE is defined as fixed 500, there must be 492 bytes wasted.
>>
>> Or we can define V2_NSIZE as 8, but if there comes more and more extended
>> fields, node version count can be a large number, it results in complicated
>> version recognization and handling.
>>
>> One more question is how can we control which fields are valid or not in
>> comp[Vx_NSIZE]?
>>
>>
>> Anyway, what I'm thinking is maybe we can restructure layout of node block
>> like
>> the one used by f2fs_inode:
>>
>> struct f2fs_node {
>> union {
>> struct f2fs_inode i;
>> union {
>> struct {
>> __le32 node_checksum;
>> __le32 feature_field_1;
>> __le32 feature_field_2;
>>
>> __le32 addr[];
>>
>> };
>> struct direct_node dn;
>> struct indirect_node in;
>> };
>> };
>> struct node_footer footer;
>> } __packed;
>>
>> Moving all extended fields to the head of f2fs_node, so we don't have to use
>> macro to indicate actual size of addr.
>
> Thinking what'd be the best way. My concern is, once getting more entries, we
OK, I think we need more discussion.. ;)
> can't set each of features individually. Like the second entry should have
Oh, that will be hard. If we have to avoid that, we have to tag in somewhere
e.g. f2fs_inode::i_flags2 to indicate which new field in f2fs_node is valid, for
example:
#define F2FS_NODE_CHECKSUM 0x0001
#define F2FS_NODE_FIELD10x0002
#define F2FS_NODE_FIELD20x0004
union {
struct {
__le32 node_checksum;
__le32 field_1;
__le32 field_2;
__le32 addr[];
};
struct direct_node dn;
struct indirect_node in;
};
f2fs_inode::i_flags2 = F2FS_NODE_CHECKSUM | F2FS_NODE_FIELD1
indicates that f2fs_node::node_checksum and f2fs_node::field_1 are valid;
f2fs_inode::i_flags2 = F2FS_NODE_FIELD1 | F2FS_NODE_FIELD2
indicates that f2fs_node::field_1 and f2fs_node::field_2 are valid.
Any thoughts?
Thanks,
> enabled node_checksum, which we may not want to do.
>
>>
>> Thanks,
>>
>>> __le32 comp[V2_NSIZE];
>>> }
>>> ...
>>>
+ };
+ struct direct_node dn;
+ struct indirect_node in;
+ };
};
struct node_footer footer;
} __packed;
--
2.15.0.55.gc2ece9dc4de6
>>>
>>> .
>>>
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
Re: [f2fs-dev] [PATCH] f2fs: use crc ^ cp_ver instead of crc | cp_ver for recovery
On 2018/2/1 21:57, Gao Xiang wrote:
Hi Chao,
On 2018/2/1 21:27, Chao Yu wrote:
Hi Xiang,
On 2018/2/1 0:16, Gaoxiang (OS) wrote:
This patch add a flag CP_CRC_RECOVERY_XOR_FLAG to use XORed crc ^ cp_ver
since crc | cp_ver is more likely to get a collision or become
11.. | cp_ver.
FYI, we have discussed about this before:
https://patchwork.kernel.org/patch/9342639/
At that time, cp_ver will always be initialized with 1, so there is
almost a
little chance to use high 32 bits, result in less collision, so I
think it
will be OK.
But now, cp_ver will be initialized with a random 64 bits value, then the
collision will be increased, I agree that xor method will be better,
but I'm
not sure we should use this, since layout change makes complicated
handling
in codes for back compatibility.
And do you encounter any incorrect recovery, or is there any following
feature
rely on this?
No... I just looked at node_footer because of another work and saw this
part of code by chance.
I think XOR-ed calculation is much better in mathematics, and typical
method for mixing two values (eg. XOR encryption) is also XOR-ed
calculation rather than OR-ed...
Thanks,
BTW,
"The crc is already random enough, but has 32bits only.
The cp_ver is not easy to use over 32bits, so we don't need to keep the
other
32bits untouched in most of life."
I observe cp_ver(if the high 32 bits are 0) ^ crc == cp_ver | crc, but
if cp_ver is over 32bits, ...
...hmmm...
Thanks,
Thanks,
Signed-off-by: Gao Xiang
---
fs/f2fs/checkpoint.c | 4 ++--
fs/f2fs/node.h | 16 +++-
fs/f2fs/segment.c | 3 ++-
include/linux/f2fs_fs.h | 1 +
4 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 8b0945b..9e7e63b 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -1157,8 +1157,8 @@ static void update_ckpt_flags(struct
f2fs_sb_info *sbi, struct cp_control *cpc)
if (is_sbi_flag_set(sbi, SBI_NEED_FSCK))
__set_ckpt_flags(ckpt, CP_FSCK_FLAG);
- /* set this flag to activate crc|cp_ver for recovery */
- __set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG);
+ /* set this flag to activate crc^cp_ver for recovery */
+ __set_ckpt_flags(ckpt, CP_CRC_RECOVERY_XOR_FLAG);
__clear_ckpt_flags(ckpt, CP_NOCRC_RECOVERY_FLAG);
spin_unlock_irqrestore(&sbi->cp_lock, flags);
diff --git a/fs/f2fs/node.h b/fs/f2fs/node.h
index 081ef0d..7b9489f 100644
--- a/fs/f2fs/node.h
+++ b/fs/f2fs/node.h
@@ -293,8 +293,11 @@ static inline void
fill_node_footer_blkaddr(struct page *page, block_t blkaddr)
struct f2fs_node *rn = F2FS_NODE(page);
__u64 cp_ver = cur_cp_version(ckpt);
- if (__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG))
- cp_ver |= (cur_cp_crc(ckpt) << 32);
+ if (__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_XOR_FLAG))
+ cp_ver ^= cur_cp_crc(ckpt) << 32;
+ /* for backward compatibility */
+ else if (unlikely(__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG)))
+ cp_ver |= cur_cp_crc(ckpt) << 32;
rn->footer.cp_ver = cpu_to_le64(cp_ver);
rn->footer.next_blkaddr = cpu_to_le32(blkaddr);
@@ -307,10 +310,13 @@ static inline bool is_recoverable_dnode(struct
page *page)
/* Don't care crc part, if fsck.f2fs sets it. */
if (__is_set_ckpt_flags(ckpt, CP_NOCRC_RECOVERY_FLAG))
- return (cp_ver << 32) == (cpver_of_node(page) << 32);
+ return (__u32)cp_ver == (__u32)cpver_of_node(page);
- if (__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG))
- cp_ver |= (cur_cp_crc(ckpt) << 32);
+ if (__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_XOR_FLAG))
+ cp_ver ^= cur_cp_crc(ckpt) << 32;
+ /* for backward compatibility */
+ else if (unlikely(__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG)))
+ cp_ver |= cur_cp_crc(ckpt) << 32;
return cp_ver == cpver_of_node(page);
}
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 205b0d9..64d0c1f 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -2316,7 +2316,8 @@ static void allocate_segment_by_default(struct
f2fs_sb_info *sbi,
if (force)
new_curseg(sbi, type, true);
- else if (!is_set_ckpt_flags(sbi, CP_CRC_RECOVERY_FLAG) &&
+ else if (!(is_set_ckpt_flags(sbi, CP_CRC_RECOVERY_FLAG) ||
+ is_set_ckpt_flags(sbi, CP_CRC_RECOVERY_XOR_FLAG)) &&
type == CURSEG_WARM_NODE)
new_curseg(sbi, type, false);
else if (curseg->alloc_type == LFS && is_next_segment_free(sbi,
type))
diff --git a/include/linux/f2fs_fs.h b/include/linux/f2fs_fs.h
index f7f0990..07ddf4b 100644
--- a/include/linux/f2fs_fs.h
+++ b/include/linux/f2fs_fs.h
@@ -117,6 +117,7 @@ struct f2fs_super_block {
/*
* For checkpoint
*/
+#define CP_CRC_RECOVERY_XOR_FLAG 0x0800
#define CP_LARGE_NAT_BITMAP_FLAG 0x0400
#define CP_NOCRC_RECOVERY_FLAG 0x0200
#define CP_TRIMMED_FLAG 0x0100
---
Re: [f2fs-dev] [PATCH] f2fs: use crc ^ cp_ver instead of crc | cp_ver for recovery
Hi Chao,
On 2018/2/1 21:27, Chao Yu wrote:
Hi Xiang,
On 2018/2/1 0:16, Gaoxiang (OS) wrote:
This patch add a flag CP_CRC_RECOVERY_XOR_FLAG to use XORed crc ^ cp_ver
since crc | cp_ver is more likely to get a collision or become 11.. |
cp_ver.
FYI, we have discussed about this before:
https://patchwork.kernel.org/patch/9342639/
At that time, cp_ver will always be initialized with 1, so there is almost a
little chance to use high 32 bits, result in less collision, so I think it
will be OK.
But now, cp_ver will be initialized with a random 64 bits value, then the
collision will be increased, I agree that xor method will be better, but I'm
not sure we should use this, since layout change makes complicated handling
in codes for back compatibility.
And do you encounter any incorrect recovery, or is there any following feature
rely on this?
No... I just looked at node_footer because of another work and saw this
part of code by chance.
I think XOR-ed calculation is much better in mathematics, and typical
method for mixing two values (eg. XOR encryption) is also XOR-ed
calculation rather than OR-ed...
Thanks,
Thanks,
Signed-off-by: Gao Xiang
---
fs/f2fs/checkpoint.c| 4 ++--
fs/f2fs/node.h | 16 +++-
fs/f2fs/segment.c | 3 ++-
include/linux/f2fs_fs.h | 1 +
4 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 8b0945b..9e7e63b 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -1157,8 +1157,8 @@ static void update_ckpt_flags(struct f2fs_sb_info *sbi,
struct cp_control *cpc)
if (is_sbi_flag_set(sbi, SBI_NEED_FSCK))
__set_ckpt_flags(ckpt, CP_FSCK_FLAG);
- /* set this flag to activate crc|cp_ver for recovery */
- __set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG);
+ /* set this flag to activate crc^cp_ver for recovery */
+ __set_ckpt_flags(ckpt, CP_CRC_RECOVERY_XOR_FLAG);
__clear_ckpt_flags(ckpt, CP_NOCRC_RECOVERY_FLAG);
spin_unlock_irqrestore(&sbi->cp_lock, flags);
diff --git a/fs/f2fs/node.h b/fs/f2fs/node.h
index 081ef0d..7b9489f 100644
--- a/fs/f2fs/node.h
+++ b/fs/f2fs/node.h
@@ -293,8 +293,11 @@ static inline void fill_node_footer_blkaddr(struct page
*page, block_t blkaddr)
struct f2fs_node *rn = F2FS_NODE(page);
__u64 cp_ver = cur_cp_version(ckpt);
- if (__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG))
- cp_ver |= (cur_cp_crc(ckpt) << 32);
+ if (__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_XOR_FLAG))
+ cp_ver ^= cur_cp_crc(ckpt) << 32;
+ /* for backward compatibility */
+ else if (unlikely(__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG)))
+ cp_ver |= cur_cp_crc(ckpt) << 32;
rn->footer.cp_ver = cpu_to_le64(cp_ver);
rn->footer.next_blkaddr = cpu_to_le32(blkaddr);
@@ -307,10 +310,13 @@ static inline bool is_recoverable_dnode(struct page *page)
/* Don't care crc part, if fsck.f2fs sets it. */
if (__is_set_ckpt_flags(ckpt, CP_NOCRC_RECOVERY_FLAG))
- return (cp_ver << 32) == (cpver_of_node(page) << 32);
+ return (__u32)cp_ver == (__u32)cpver_of_node(page);
- if (__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG))
- cp_ver |= (cur_cp_crc(ckpt) << 32);
+ if (__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_XOR_FLAG))
+ cp_ver ^= cur_cp_crc(ckpt) << 32;
+ /* for backward compatibility */
+ else if (unlikely(__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG)))
+ cp_ver |= cur_cp_crc(ckpt) << 32;
return cp_ver == cpver_of_node(page);
}
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index 205b0d9..64d0c1f 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -2316,7 +2316,8 @@ static void allocate_segment_by_default(struct
f2fs_sb_info *sbi,
if (force)
new_curseg(sbi, type, true);
- else if (!is_set_ckpt_flags(sbi, CP_CRC_RECOVERY_FLAG) &&
+ else if (!(is_set_ckpt_flags(sbi, CP_CRC_RECOVERY_FLAG) ||
+ is_set_ckpt_flags(sbi, CP_CRC_RECOVERY_XOR_FLAG)) &&
type == CURSEG_WARM_NODE)
new_curseg(sbi, type, false);
else if (curseg->alloc_type == LFS && is_next_segment_free(sbi, type))
diff --git a/include/linux/f2fs_fs.h b/include/linux/f2fs_fs.h
index f7f0990..07ddf4b 100644
--- a/include/linux/f2fs_fs.h
+++ b/include/linux/f2fs_fs.h
@@ -117,6 +117,7 @@ struct f2fs_super_block {
/*
* For checkpoint
*/
+#define CP_CRC_RECOVERY_XOR_FLAG 0x0800
#define CP_LARGE_NAT_BITMAP_FLAG 0x0400
#define CP_NOCRC_RECOVERY_FLAG0x0200
#define CP_TRIMMED_FLAG 0x0100
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
__
Re: [f2fs-dev] [PATCH RFC v4] f2fs: flush cp pack except cp pack 2 page at first
On 2018/2/1 6:28, Jaegeuk Kim wrote:
> On 01/31, Chao Yu wrote:
>> On 2018/1/31 14:39, Gaoxiang (OS) wrote:
>>> Previously, we attempt to flush the whole cp pack in a single bio,
>>> however, when suddenly powering off at this time, we could get into
>>> an extreme scenario that cp pack 1 page and cp pack 2 page are updated
>>> and latest, but payload or current summaries are still partially
>>> outdated. (see reliable write in the UFS specification)
>>>
>>> This patch submits the whole cp pack except cp pack 2 page at first,
>>> and then writes the cp pack 2 page with an extra independent
>>> bio with pre-io barrier.
>>>
>>> Signed-off-by: Gao Xiang
>>> Reviewed-by: Chao Yu
>>> ---
>>> Change log from v3:
>>> - further review comments are applied from Jaegeuk and Chao
>>> - Tested on this patch (without multiple-device): mount, boot Android
>>> with f2fs userdata and make fragment
>>> - If any problem with this patch or I miss something, please kindly share
>>> your comments, thanks :)
>>> Change log from v2:
>>> - Apply the review comments from Chao
>>> Change log from v1:
>>> - Apply the review comments from Chao
>>> - time data from "finish block_ops" to " finish checkpoint" (tested on
>>> ARM64 with TOSHIBA 128GB UFS):
>>> Before patch: 0.002273 0.001973 0.002789 0.005159 0.002050
>>> After patch: 0.002502 0.001624 0.002487 0.003049 0.002696
>>> fs/f2fs/checkpoint.c | 67
>>>
>>> 1 file changed, 46 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
>>> index 14d2fed..916dc72 100644
>>> --- a/fs/f2fs/checkpoint.c
>>> +++ b/fs/f2fs/checkpoint.c
>>> @@ -1158,6 +1158,39 @@ static void update_ckpt_flags(struct f2fs_sb_info
>>> *sbi, struct cp_control *cpc)
>>> spin_unlock_irqrestore(&sbi->cp_lock, flags);
>>> }
>>>
>>> +static void commit_checkpoint(struct f2fs_sb_info *sbi,
>>> + void *src, block_t blk_addr)
>>> +{
>>> + struct writeback_control wbc = {
>>> + .for_reclaim = 0,
>>> + };
>>> +
>>> + /*
>>> +* pagevec_lookup_tag and lock_page again will take
>>> +* some extra time. Therefore, update_meta_pages and
>>> +* sync_meta_pages are combined in this function.
>>> +*/
>>> + struct page *page = grab_meta_page(sbi, blk_addr);
>>> + int err;
>>> +
>>> + memcpy(page_address(page), src, PAGE_SIZE);
>>> + set_page_dirty(page);
>>> +
>>> + f2fs_wait_on_page_writeback(page, META, true);
>>> + f2fs_bug_on(sbi, PageWriteback(page));
>>> + if (unlikely(!clear_page_dirty_for_io(page)))
>>> + f2fs_bug_on(sbi, 1);
>>> +
>>> + /* writeout cp pack 2 page */
>>> + err = __f2fs_write_meta_page(page, &wbc, FS_CP_META_IO);
>>> + f2fs_bug_on(sbi, err);
>>> +
>>> + f2fs_put_page(page, 0);
>>> +
>>> + /* submit checkpoint (with barrier if NOBARRIER is not set) */
>>> + f2fs_submit_merged_write(sbi, META_FLUSH);
>>> +}
>>> +
>>> static int do_checkpoint(struct f2fs_sb_info *sbi, struct cp_control *cpc)
>>> {
>>> struct f2fs_checkpoint *ckpt = F2FS_CKPT(sbi);
>>> @@ -1260,16 +1293,6 @@ static int do_checkpoint(struct f2fs_sb_info *sbi,
>>> struct cp_control *cpc)
>>> }
>>> }
>>>
>>> - /* need to wait for end_io results */
>>> - wait_on_all_pages_writeback(sbi);
>>> - if (unlikely(f2fs_cp_error(sbi)))
>>> - return -EIO;
>>> -
>>> - /* flush all device cache */
>>> - err = f2fs_flush_device_cache(sbi);
>>> - if (err)
>>> - return err;
>>> -
>>> /* write out checkpoint buffer at block 0 */
>>> update_meta_page(sbi, ckpt, start_blk++);
>>>
>>> @@ -1297,15 +1320,6 @@ static int do_checkpoint(struct f2fs_sb_info *sbi,
>>> struct cp_control *cpc)
>>> start_blk += NR_CURSEG_NODE_TYPE;
>>> }
>>>
>>> - /* writeout checkpoint block */
>>> - update_meta_page(sbi, ckpt, start_blk);
>>> -
>>> - /* wait for previous submitted node/meta pages writeback */
>>> - wait_on_all_pages_writeback(sbi);
>>> -
>>> - if (unlikely(f2fs_cp_error(sbi)))
>>> - return -EIO;
>>> -
>>> filemap_fdatawait_range(NODE_MAPPING(sbi), 0, LLONG_MAX);
>>> filemap_fdatawait_range(META_MAPPING(sbi), 0, LLONG_MAX);
>
> - remove
Agreed.
>
>>>
>>> @@ -1313,12 +1327,23 @@ static int do_checkpoint(struct f2fs_sb_info *sbi,
>>> struct cp_control *cpc)
>>> sbi->last_valid_block_count = sbi->total_valid_block_count;
>>> percpu_counter_set(&sbi->alloc_valid_block_count, 0);
>>>
>>> - /* Here, we only have one bio having CP pack */
>>> - sync_meta_pages(sbi, META_FLUSH, LONG_MAX, FS_CP_META_IO);
>>> + /* Here, we have one bio having CP pack except cp pack 2 page */
>>> + sync_meta_pages(sbi, META, LONG_MAX, FS_CP_META_IO);
>>> +
>>> + /* flush all device cache */
>>> + err = f2fs_flush_device_cache(sbi);
>>> + if (err)
>>> + return err;
>>>
>>> /* wait for previous submitted meta pages writeback */
>>> wait_
Re: [f2fs-dev] [PATCH] f2fs: add sanity check for quota sysfile ino
On 2018/2/1 5:57, Jaegeuk Kim wrote:
> On 01/31, Chao Yu wrote:
>> On 2018/1/31 11:49, Jaegeuk Kim wrote:
>>> On 01/31, Chao Yu wrote:
On 2018/1/31 10:36, Jaegeuk Kim wrote:
> On 01/31, Chao Yu wrote:
>> On 2018/1/31 9:35, Jaegeuk Kim wrote:
>>> On 01/29, Chao Yu wrote:
Add missing sanity check for quota sysfile ino.
>>>
>>> We don't need to limit the specific inode numbers for quota files, since
>>> we may be able to set any inode numbers later. How about checkint the
>>> numbers are allocated as quota?
>
> If quota_ino is set, f2fs_enable_quotas() will be failed, if its ino is
> invalid.
> It'd be also fine, if we have no inode numbers there. What I mean was we
> may need
> to consider reallocating or deallocating inode numbers later.
Oh, so that it will be better to do more check & repair in fsck tools,
right?
>>>
>>> What is the rule that you're considering? We're already checking quota
>>> inodes
>>> in fsck.f2fs.
>>
>> The case that qf_ino[] is corrupted, I didn't see it can be relinked to
>> corresponding quota inode by fsck, am I missing something?
>
> We can check its basic metadata such as i_mode? Or, in order to do that
Agreed, how about:
if (i_mode == 0x8180 && i_flags == FS_IMMUTABLE_FL &&
i_name[0] == 0x00 && i_namelen == 0)
relink super_block::qf_ino to quote inode;
> entirely, we need to add file truncation and creationg in fsck.f2fs, which
Yes, I think we can rebuild sys quote file if original one is missing or
corrupted.
Thanks,
> was too complicated to add before.
> Any idea?
>
> Thanks,
>
>>
>> Thanks,
>>
>>>
Thanks,
>
>>
>> Do you mean:
>>
>> if (raw_super->feature & cpu_to_le32(F2FS_FEATURE_QUOTA_INO)) {
>> if (!le32_to_cpu(raw_super->qf_ino[0]) ||
>> !le32_to_cpu(raw_super->qf_ino[1])) {
>> f2fs_msg(sb, KERN_INFO,
>> "Invalid Quota Ino: user_ino(%u), grp_ino(%u)",
>> le32_to_cpu(raw_super->qf_ino[0]),
>> le32_to_cpu(raw_super->qf_ino[1]));
>> return 1;
>> }
>> if (raw_super->feature & cpu_to_le32(F2FS_FEATURE_PRJQUOTA) &&
>> !le32_to_cpu(raw_super->qf_ino[2])) {
>> f2fs_msg(sb, KERN_INFO,
>> "Invalid Quota Ino: prj_ino(%u)",
>> le32_to_cpu(raw_super->qf_ino[2]));
>> return 1;
>> }
>> }
>>
>> I think it's equal to:
>>
>> if (raw_super->feature & cpu_to_le32(F2FS_FEATURE_QUOTA_INO)) {
>> unsigned int qf_cnt = 0;
>> if (qf_ino[0])
>> qf_cnt++;
>> if (qf_ino[1])
>> qf_cnt++;
>> if (qf_cnt != 2) {
>> f2fs_msg();
>> return 1;
>> }
>> if (raw_super->feature & cpu_to_le32(F2FS_FEATURE_PRJQUOTA)) {
>> if (qf_ino[2])
>> qf_cnt++;
>> if (qf_cnt != 3) {
>> f2fs_msg();
>> return 1;
>> }
>> }
>> }
>>
>> Thanks,
>>
>>>
Signed-off-by: Chao Yu
---
fs/f2fs/super.c | 19 +++
1 file changed, 19 insertions(+)
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 368f63d7bad2..6011071688ca 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -2116,6 +2116,25 @@ static int sanity_check_raw_super(struct
f2fs_sb_info *sbi,
return 1;
}
+ /* check quota sysfile ino info */
+ if (raw_super->feature & cpu_to_le32(F2FS_FEATURE_QUOTA_INO)) {
+ if (le32_to_cpu(raw_super->qf_ino[0]) != 4 ||
+ le32_to_cpu(raw_super->qf_ino[1]) != 5) {
+ f2fs_msg(sb, KERN_INFO,
+ "Invalid Quota Ino: user_ino(%u),
grp_ino(%u)",
+ le32_to_cpu(raw_super->qf_ino[0]),
+ le32_to_cpu(raw_super->qf_ino[1]));
+ return 1;
+ }
+ if (raw_super->feature &
cpu_to_le32(F2FS_FEATURE_PRJQUOTA) &&
+ le32_to_cpu(raw_super->qf_ino[2]) != 6) {
+ f2fs_msg(sb, KERN_INFO,
+ "Invalid Quota Ino: prj_ino(%u)",
+ le32_to_cpu(raw_super->qf_ino[2]));
+ return 1;
+ }
>
Re: [f2fs-dev] [PATCH] f2fs: use crc ^ cp_ver instead of crc | cp_ver for recovery
Hi Xiang,
On 2018/2/1 0:16, Gaoxiang (OS) wrote:
> This patch add a flag CP_CRC_RECOVERY_XOR_FLAG to use XORed crc ^ cp_ver
> since crc | cp_ver is more likely to get a collision or become 11.. |
> cp_ver.
FYI, we have discussed about this before:
https://patchwork.kernel.org/patch/9342639/
At that time, cp_ver will always be initialized with 1, so there is almost a
little chance to use high 32 bits, result in less collision, so I think it
will be OK.
But now, cp_ver will be initialized with a random 64 bits value, then the
collision will be increased, I agree that xor method will be better, but I'm
not sure we should use this, since layout change makes complicated handling
in codes for back compatibility.
And do you encounter any incorrect recovery, or is there any following feature
rely on this?
Thanks,
>
> Signed-off-by: Gao Xiang
> ---
> fs/f2fs/checkpoint.c| 4 ++--
> fs/f2fs/node.h | 16 +++-
> fs/f2fs/segment.c | 3 ++-
> include/linux/f2fs_fs.h | 1 +
> 4 files changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
> index 8b0945b..9e7e63b 100644
> --- a/fs/f2fs/checkpoint.c
> +++ b/fs/f2fs/checkpoint.c
> @@ -1157,8 +1157,8 @@ static void update_ckpt_flags(struct f2fs_sb_info *sbi,
> struct cp_control *cpc)
> if (is_sbi_flag_set(sbi, SBI_NEED_FSCK))
> __set_ckpt_flags(ckpt, CP_FSCK_FLAG);
>
> - /* set this flag to activate crc|cp_ver for recovery */
> - __set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG);
> + /* set this flag to activate crc^cp_ver for recovery */
> + __set_ckpt_flags(ckpt, CP_CRC_RECOVERY_XOR_FLAG);
> __clear_ckpt_flags(ckpt, CP_NOCRC_RECOVERY_FLAG);
>
> spin_unlock_irqrestore(&sbi->cp_lock, flags);
> diff --git a/fs/f2fs/node.h b/fs/f2fs/node.h
> index 081ef0d..7b9489f 100644
> --- a/fs/f2fs/node.h
> +++ b/fs/f2fs/node.h
> @@ -293,8 +293,11 @@ static inline void fill_node_footer_blkaddr(struct page
> *page, block_t blkaddr)
> struct f2fs_node *rn = F2FS_NODE(page);
> __u64 cp_ver = cur_cp_version(ckpt);
>
> - if (__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG))
> - cp_ver |= (cur_cp_crc(ckpt) << 32);
> + if (__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_XOR_FLAG))
> + cp_ver ^= cur_cp_crc(ckpt) << 32;
> + /* for backward compatibility */
> + else if (unlikely(__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG)))
> + cp_ver |= cur_cp_crc(ckpt) << 32;
>
> rn->footer.cp_ver = cpu_to_le64(cp_ver);
> rn->footer.next_blkaddr = cpu_to_le32(blkaddr);
> @@ -307,10 +310,13 @@ static inline bool is_recoverable_dnode(struct page
> *page)
>
> /* Don't care crc part, if fsck.f2fs sets it. */
> if (__is_set_ckpt_flags(ckpt, CP_NOCRC_RECOVERY_FLAG))
> - return (cp_ver << 32) == (cpver_of_node(page) << 32);
> + return (__u32)cp_ver == (__u32)cpver_of_node(page);
>
> - if (__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG))
> - cp_ver |= (cur_cp_crc(ckpt) << 32);
> + if (__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_XOR_FLAG))
> + cp_ver ^= cur_cp_crc(ckpt) << 32;
> + /* for backward compatibility */
> + else if (unlikely(__is_set_ckpt_flags(ckpt, CP_CRC_RECOVERY_FLAG)))
> + cp_ver |= cur_cp_crc(ckpt) << 32;
>
> return cp_ver == cpver_of_node(page);
> }
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index 205b0d9..64d0c1f 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -2316,7 +2316,8 @@ static void allocate_segment_by_default(struct
> f2fs_sb_info *sbi,
>
> if (force)
> new_curseg(sbi, type, true);
> - else if (!is_set_ckpt_flags(sbi, CP_CRC_RECOVERY_FLAG) &&
> + else if (!(is_set_ckpt_flags(sbi, CP_CRC_RECOVERY_FLAG) ||
> + is_set_ckpt_flags(sbi, CP_CRC_RECOVERY_XOR_FLAG)) &&
> type == CURSEG_WARM_NODE)
> new_curseg(sbi, type, false);
> else if (curseg->alloc_type == LFS && is_next_segment_free(sbi, type))
> diff --git a/include/linux/f2fs_fs.h b/include/linux/f2fs_fs.h
> index f7f0990..07ddf4b 100644
> --- a/include/linux/f2fs_fs.h
> +++ b/include/linux/f2fs_fs.h
> @@ -117,6 +117,7 @@ struct f2fs_super_block {
> /*
> * For checkpoint
> */
> +#define CP_CRC_RECOVERY_XOR_FLAG 0x0800
> #define CP_LARGE_NAT_BITMAP_FLAG 0x0400
> #define CP_NOCRC_RECOVERY_FLAG 0x0200
> #define CP_TRIMMED_FLAG 0x0100
>
--
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
___
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
