* [PATCH 1/2] f2fs: enhance scalibility of {d, id, did, x}node disk layout
@ 2018-01-27 9:43 Chao Yu
2018-01-27 9:43 ` [PATCH 2/2] f2fs: support {d,id,did,x}node checksum Chao Yu
0 siblings, 1 reply; 16+ messages in thread
From: Chao Yu @ 2018-01-27 9:43 UTC (permalink / raw)
To: jaegeuk; +Cc: linux-kernel, linux-f2fs-devel
Scalibility of non-inode disk layout is very bad, it's hard to add or reuse
any fields in current structure, so, for new feature like node checksum
which wants to add 4 bytes field in node structure, the bad scaliblity
becomes a obstacle for its implementation.
In order to enhance scalibility, we introduce a new filesystem feature
'extended_node' which can be enabled via mkfs.f2fs, once this feature is
set, we will add and configure f2fs_super_block::extra_nsize to indicate
extended space used for storing new attribution, accordingly, it needs
to recalculate space of original .addr/.nid/xattr space in node block.
dnode, idnode, didnode, xnode disk layout:
+----------------------+
| .addr or .nid, .xatt |
+----------------------+<----+
| ...... | |
| .epoch | |
| .transaction_id | +------ f2fs_super_block::extra_nsize
| .node_checksum | |
+----------------------+<----+
| node_footer |
| (nid, ino, offset) |
+----------------------+
Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
fs/f2fs/f2fs.h | 23 +++++++++++++++++++++++
fs/f2fs/file.c | 7 ++++---
fs/f2fs/gc.c | 9 +++++----
fs/f2fs/node.c | 46 +++++++++++++++++++++++++---------------------
fs/f2fs/node.h | 13 ++++++++-----
fs/f2fs/super.c | 16 +++++++++++-----
fs/f2fs/sysfs.c | 7 +++++++
fs/f2fs/xattr.c | 10 ++++++----
fs/f2fs/xattr.h | 6 ++++--
include/linux/f2fs_fs.h | 18 ++++++++++--------
10 files changed, 103 insertions(+), 52 deletions(-)
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 6ac1c09419e2..6f5e41657c62 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -127,6 +127,7 @@ struct f2fs_mount_info {
#define F2FS_FEATURE_FLEXIBLE_INLINE_XATTR 0x0040
#define F2FS_FEATURE_QUOTA_INO 0x0080
#define F2FS_FEATURE_INODE_CRTIME 0x0100
+#define F2FS_FEATURE_EXTENDED_NODE 0x0200
#define F2FS_HAS_FEATURE(sb, mask) \
((F2FS_SB(sb)->raw_super->feature & cpu_to_le32(mask)) != 0)
@@ -416,6 +417,7 @@ struct f2fs_flush_device {
#define DEF_INLINE_RESERVED_SIZE 1
#define DEF_MIN_INLINE_SIZE 1
static inline int get_extra_isize(struct inode *inode);
+static inline int get_extra_nsize(struct super_block *sb);
static inline int get_inline_xattr_addrs(struct inode *inode);
#define MAX_INLINE_DATA(inode) (sizeof(__le32) * \
(CUR_ADDRS_PER_INODE(inode) - \
@@ -1123,6 +1125,7 @@ struct f2fs_sb_info {
int inline_xattr_size; /* inline xattr size */
unsigned int trigger_ssr_threshold; /* threshold to trigger ssr */
int readdir_ra; /* readahead inode in readdir */
+ unsigned int extra_nsize; /* extra attr size in {d,id,did,x}node */
block_t user_block_count; /* # of user blocks */
block_t total_valid_block_count; /* # of valid blocks */
@@ -2344,6 +2347,16 @@ static inline unsigned int addrs_per_inode(struct inode *inode)
return CUR_ADDRS_PER_INODE(inode) - get_inline_xattr_addrs(inode);
}
+static inline unsigned int addrs_per_dnode(struct inode *inode)
+{
+ return DEF_ADDRS_PER_BLOCK - get_extra_nsize(inode->i_sb);
+}
+
+static inline unsigned int nids_per_idnode(struct inode *inode)
+{
+ return DEF_NIDS_PER_BLOCK - get_extra_nsize(inode->i_sb);
+}
+
static inline void *inline_xattr_addr(struct inode *inode, struct page *page)
{
struct f2fs_inode *ri = F2FS_INODE(page);
@@ -2533,6 +2546,11 @@ static inline int get_extra_isize(struct inode *inode)
return F2FS_I(inode)->i_extra_isize / sizeof(__le32);
}
+static inline int get_extra_nsize(struct super_block *sb)
+{
+ return F2FS_SB(sb)->extra_nsize / sizeof(__le32);
+}
+
static inline int get_inline_xattr_addrs(struct inode *inode)
{
return F2FS_I(inode)->i_inline_xattr_size;
@@ -3229,6 +3247,11 @@ static inline int f2fs_sb_has_inode_crtime(struct super_block *sb)
return F2FS_HAS_FEATURE(sb, F2FS_FEATURE_INODE_CRTIME);
}
+static inline int f2fs_sb_has_extended_node(struct super_block *sb)
+{
+ return F2FS_HAS_FEATURE(sb, F2FS_FEATURE_EXTENDED_NODE);
+}
+
#ifdef CONFIG_BLK_DEV_ZONED
static inline int get_blkz_type(struct f2fs_sb_info *sbi,
struct block_device *bdev, block_t blkaddr)
diff --git a/fs/f2fs/file.c b/fs/f2fs/file.c
index d56c3b7a8ba1..b8810843dc21 100644
--- a/fs/f2fs/file.c
+++ b/fs/f2fs/file.c
@@ -540,7 +540,7 @@ void truncate_data_blocks_range(struct dnode_of_data *dn, int count)
void truncate_data_blocks(struct dnode_of_data *dn)
{
- truncate_data_blocks_range(dn, ADDRS_PER_BLOCK);
+ truncate_data_blocks_range(dn, ADDRS_PER_BLOCK(dn->inode));
}
static int truncate_partial_data_page(struct inode *inode, u64 from,
@@ -978,7 +978,8 @@ static int __read_out_blkaddrs(struct inode *inode, block_t *blkaddr,
} else if (ret == -ENOENT) {
if (dn.max_level == 0)
return -ENOENT;
- done = min((pgoff_t)ADDRS_PER_BLOCK - dn.ofs_in_node, len);
+ done = min((pgoff_t)ADDRS_PER_BLOCK(inode) - dn.ofs_in_node,
+ len);
blkaddr += done;
do_replace += done;
goto next;
@@ -1122,7 +1123,7 @@ static int __exchange_data_block(struct inode *src_inode,
int ret;
while (len) {
- olen = min((pgoff_t)4 * ADDRS_PER_BLOCK, len);
+ olen = min((pgoff_t)4 * ADDRS_PER_BLOCK(src_inode), len);
src_blkaddr = f2fs_kvzalloc(F2FS_I_SB(src_inode),
sizeof(block_t) * olen, GFP_KERNEL);
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index 3b26aa19430b..0da1e218a1ec 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -538,7 +538,7 @@ static void gc_node_segment(struct f2fs_sb_info *sbi,
*/
block_t start_bidx_of_node(unsigned int node_ofs, struct inode *inode)
{
- unsigned int indirect_blks = 2 * NIDS_PER_BLOCK + 4;
+ unsigned int indirect_blks = 2 * NIDS_PER_BLOCK(inode) + 4;
unsigned int bidx;
if (node_ofs == 0)
@@ -547,13 +547,14 @@ block_t start_bidx_of_node(unsigned int node_ofs, struct inode *inode)
if (node_ofs <= 2) {
bidx = node_ofs - 1;
} else if (node_ofs <= indirect_blks) {
- int dec = (node_ofs - 4) / (NIDS_PER_BLOCK + 1);
+ int dec = (node_ofs - 4) / (NIDS_PER_BLOCK(inode) + 1);
bidx = node_ofs - 2 - dec;
} else {
- int dec = (node_ofs - indirect_blks - 3) / (NIDS_PER_BLOCK + 1);
+ int dec = (node_ofs - indirect_blks - 3) /
+ (NIDS_PER_BLOCK(inode) + 1);
bidx = node_ofs - 5 - dec;
}
- return bidx * ADDRS_PER_BLOCK + ADDRS_PER_INODE(inode);
+ return bidx * ADDRS_PER_BLOCK(inode) + ADDRS_PER_INODE(inode);
}
static bool is_alive(struct f2fs_sb_info *sbi, struct f2fs_summary *sum,
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index 7cded843cf18..bc8424babf36 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -461,7 +461,8 @@ static void ra_node_pages(struct page *parent, int start, int n)
/* Then, try readahead for siblings of the desired node */
end = start + n;
- end = min(end, NIDS_PER_BLOCK);
+ end = min_t(unsigned int, end, DEF_NIDS_PER_BLOCK -
+ get_extra_nsize(sbi->sb));
for (i = start; i < end; i++) {
nid = get_nid(parent, i, false);
ra_node_page(sbi, nid);
@@ -473,9 +474,10 @@ static void ra_node_pages(struct page *parent, int start, int n)
pgoff_t get_next_page_offset(struct dnode_of_data *dn, pgoff_t pgofs)
{
const long direct_index = ADDRS_PER_INODE(dn->inode);
- const long direct_blks = ADDRS_PER_BLOCK;
- const long indirect_blks = ADDRS_PER_BLOCK * NIDS_PER_BLOCK;
- unsigned int skipped_unit = ADDRS_PER_BLOCK;
+ const long direct_blks = ADDRS_PER_BLOCK(dn->inode);
+ const long indirect_blks = ADDRS_PER_BLOCK(dn->inode) *
+ NIDS_PER_BLOCK(dn->inode);
+ unsigned int skipped_unit = ADDRS_PER_BLOCK(dn->inode);
int cur_level = dn->cur_level;
int max_level = dn->max_level;
pgoff_t base = 0;
@@ -484,7 +486,7 @@ pgoff_t get_next_page_offset(struct dnode_of_data *dn, pgoff_t pgofs)
return pgofs + 1;
while (max_level-- > cur_level)
- skipped_unit *= NIDS_PER_BLOCK;
+ skipped_unit *= NIDS_PER_BLOCK(dn->inode);
switch (dn->max_level) {
case 3:
@@ -509,10 +511,11 @@ static int get_node_path(struct inode *inode, long block,
int offset[4], unsigned int noffset[4])
{
const long direct_index = ADDRS_PER_INODE(inode);
- const long direct_blks = ADDRS_PER_BLOCK;
- const long dptrs_per_blk = NIDS_PER_BLOCK;
- const long indirect_blks = ADDRS_PER_BLOCK * NIDS_PER_BLOCK;
- const long dindirect_blks = indirect_blks * NIDS_PER_BLOCK;
+ const long direct_blks = ADDRS_PER_BLOCK(inode);
+ const long dptrs_per_blk = NIDS_PER_BLOCK(inode);
+ const long indirect_blks = ADDRS_PER_BLOCK(inode) *
+ NIDS_PER_BLOCK(inode);
+ const long dindirect_blks = indirect_blks * NIDS_PER_BLOCK(inode);
int n = 0;
int level = 0;
@@ -758,7 +761,7 @@ static int truncate_nodes(struct dnode_of_data *dn, unsigned int nofs,
int i, ret;
if (dn->nid == 0)
- return NIDS_PER_BLOCK + 1;
+ return NIDS_PER_BLOCK(dn->inode) + 1;
trace_f2fs_truncate_nodes_enter(dn->inode, dn->nid, dn->data_blkaddr);
@@ -768,11 +771,11 @@ static int truncate_nodes(struct dnode_of_data *dn, unsigned int nofs,
return PTR_ERR(page);
}
- ra_node_pages(page, ofs, NIDS_PER_BLOCK);
+ ra_node_pages(page, ofs, NIDS_PER_BLOCK(dn->inode));
rn = F2FS_NODE(page);
if (depth < 3) {
- for (i = ofs; i < NIDS_PER_BLOCK; i++, freed++) {
+ for (i = ofs; i < NIDS_PER_BLOCK(dn->inode); i++, freed++) {
child_nid = le32_to_cpu(rn->in.nid[i]);
if (child_nid == 0)
continue;
@@ -784,16 +787,16 @@ static int truncate_nodes(struct dnode_of_data *dn, unsigned int nofs,
dn->node_changed = true;
}
} else {
- child_nofs = nofs + ofs * (NIDS_PER_BLOCK + 1) + 1;
- for (i = ofs; i < NIDS_PER_BLOCK; i++) {
+ child_nofs = nofs + ofs * (NIDS_PER_BLOCK(dn->inode) + 1) + 1;
+ for (i = ofs; i < NIDS_PER_BLOCK(dn->inode); i++) {
child_nid = le32_to_cpu(rn->in.nid[i]);
if (child_nid == 0) {
- child_nofs += NIDS_PER_BLOCK + 1;
+ child_nofs += NIDS_PER_BLOCK(dn->inode) + 1;
continue;
}
rdn.nid = child_nid;
ret = truncate_nodes(&rdn, child_nofs, 0, depth - 1);
- if (ret == (NIDS_PER_BLOCK + 1)) {
+ if (ret == (NIDS_PER_BLOCK(dn->inode) + 1)) {
if (set_nid(page, i, 0, false))
dn->node_changed = true;
child_nofs += ret;
@@ -847,10 +850,10 @@ static int truncate_partial_nodes(struct dnode_of_data *dn,
nid[i + 1] = get_nid(pages[i], offset[i + 1], false);
}
- ra_node_pages(pages[idx], offset[idx + 1], NIDS_PER_BLOCK);
+ ra_node_pages(pages[idx], offset[idx + 1], NIDS_PER_BLOCK(dn->inode));
/* free direct nodes linked to a partial indirect node */
- for (i = offset[idx + 1]; i < NIDS_PER_BLOCK; i++) {
+ for (i = offset[idx + 1]; i < NIDS_PER_BLOCK(dn->inode); i++) {
child_nid = get_nid(pages[idx], i, false);
if (!child_nid)
continue;
@@ -922,10 +925,10 @@ int truncate_inode_blocks(struct inode *inode, pgoff_t from)
err = truncate_partial_nodes(&dn, ri, offset, level);
if (err < 0 && err != -ENOENT)
goto fail;
- nofs += 1 + NIDS_PER_BLOCK;
+ nofs += 1 + NIDS_PER_BLOCK(inode);
break;
case 3:
- nofs = 5 + 2 * NIDS_PER_BLOCK;
+ nofs = 5 + 2 * NIDS_PER_BLOCK(inode);
if (!offset[level - 1])
goto skip_partial;
err = truncate_partial_nodes(&dn, ri, offset, level);
@@ -2278,7 +2281,8 @@ int recover_xattr_data(struct inode *inode, struct page *page)
update_inode_page(inode);
/* 3: update and set xattr node page dirty */
- memcpy(F2FS_NODE(xpage), F2FS_NODE(page), VALID_XATTR_BLOCK_SIZE);
+ memcpy(F2FS_NODE(xpage), F2FS_NODE(page),
+ VALID_XATTR_BLOCK_SIZE(inode));
set_page_dirty(xpage);
f2fs_put_page(xpage, 1);
diff --git a/fs/f2fs/node.h b/fs/f2fs/node.h
index 081ef0d672bf..b051196b5d5b 100644
--- a/fs/f2fs/node.h
+++ b/fs/f2fs/node.h
@@ -338,17 +338,20 @@ static inline bool is_recoverable_dnode(struct page *page)
*/
static inline bool IS_DNODE(struct page *node_page)
{
+ struct f2fs_sb_info *sbi = F2FS_P_SB(node_page);
unsigned int ofs = ofs_of_node(node_page);
+ unsigned int nids_per_block = DEF_NIDS_PER_BLOCK -
+ get_extra_nsize(sbi->sb);
if (f2fs_has_xattr_block(ofs))
return true;
- if (ofs == 3 || ofs == 4 + NIDS_PER_BLOCK ||
- ofs == 5 + 2 * NIDS_PER_BLOCK)
+ if (ofs == 3 || ofs == 4 + nids_per_block ||
+ ofs == 5 + 2 * nids_per_block)
return false;
- if (ofs >= 6 + 2 * NIDS_PER_BLOCK) {
- ofs -= 6 + 2 * NIDS_PER_BLOCK;
- if (!((long int)ofs % (NIDS_PER_BLOCK + 1)))
+ if (ofs >= 6 + 2 * nids_per_block) {
+ ofs -= 6 + 2 * nids_per_block;
+ if (!((long int)ofs % (nids_per_block + 1)))
return false;
}
return true;
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index f6fb8d9928bc..368f63d7bad2 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -1901,10 +1901,12 @@ static const struct export_operations f2fs_export_ops = {
.get_parent = f2fs_get_parent,
};
-static loff_t max_file_blocks(void)
+static loff_t max_file_blocks(struct super_block *sb)
{
+ unsigned int nids_per_block = DEF_NIDS_PER_BLOCK -
+ get_extra_nsize(sb);
+ loff_t leaf_count = DEF_ADDRS_PER_BLOCK - get_extra_nsize(sb);
loff_t result = 0;
- loff_t leaf_count = ADDRS_PER_BLOCK;
/*
* note: previously, result is equal to (DEF_ADDRS_PER_INODE -
@@ -1917,11 +1919,11 @@ static loff_t max_file_blocks(void)
result += (leaf_count * 2);
/* two indirect node blocks */
- leaf_count *= NIDS_PER_BLOCK;
+ leaf_count *= nids_per_block;
result += (leaf_count * 2);
/* one double indirect node block */
- leaf_count *= NIDS_PER_BLOCK;
+ leaf_count *= nids_per_block;
result += leaf_count;
return result;
@@ -2554,6 +2556,10 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
goto free_sb_buf;
}
#endif
+
+ if (f2fs_sb_has_extended_node(sb))
+ sbi->extra_nsize = le16_to_cpu(raw_super->extra_nsize);
+
default_options(sbi);
/* parse mount options */
options = kstrdup((const char *)data, GFP_KERNEL);
@@ -2566,7 +2572,7 @@ static int f2fs_fill_super(struct super_block *sb, void *data, int silent)
if (err)
goto free_options;
- sbi->max_file_blocks = max_file_blocks();
+ sbi->max_file_blocks = max_file_blocks(sb);
sb->s_maxbytes = sbi->max_file_blocks <<
le32_to_cpu(raw_super->log_blocksize);
sb->s_max_links = F2FS_LINK_MAX;
diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
index d978c7b6ea04..f25a6fc0a17e 100644
--- a/fs/f2fs/sysfs.c
+++ b/fs/f2fs/sysfs.c
@@ -116,6 +116,9 @@ static ssize_t features_show(struct f2fs_attr *a,
if (f2fs_sb_has_inode_crtime(sb))
len += snprintf(buf + len, PAGE_SIZE - len, "%s%s",
len ? ", " : "", "inode_crtime");
+ if (f2fs_sb_has_extended_node(sb))
+ len += snprintf(buf + len, PAGE_SIZE - len, "%s%s",
+ len ? ", " : "", "extended_node");
len += snprintf(buf + len, PAGE_SIZE - len, "\n");
return len;
}
@@ -236,6 +239,7 @@ enum feat_id {
FEAT_FLEXIBLE_INLINE_XATTR,
FEAT_QUOTA_INO,
FEAT_INODE_CRTIME,
+ FEAT_EXTENDED_NODE,
};
static ssize_t f2fs_feature_show(struct f2fs_attr *a,
@@ -251,6 +255,7 @@ static ssize_t f2fs_feature_show(struct f2fs_attr *a,
case FEAT_FLEXIBLE_INLINE_XATTR:
case FEAT_QUOTA_INO:
case FEAT_INODE_CRTIME:
+ case FEAT_EXTENDED_NODE:
return snprintf(buf, PAGE_SIZE, "supported\n");
}
return 0;
@@ -329,6 +334,7 @@ F2FS_FEATURE_RO_ATTR(inode_checksum, FEAT_INODE_CHECKSUM);
F2FS_FEATURE_RO_ATTR(flexible_inline_xattr, FEAT_FLEXIBLE_INLINE_XATTR);
F2FS_FEATURE_RO_ATTR(quota_ino, FEAT_QUOTA_INO);
F2FS_FEATURE_RO_ATTR(inode_crtime, FEAT_INODE_CRTIME);
+F2FS_FEATURE_RO_ATTR(extended_node, FEAT_EXTENDED_NODE);
#define ATTR_LIST(name) (&f2fs_attr_##name.attr)
static struct attribute *f2fs_attrs[] = {
@@ -383,6 +389,7 @@ static struct attribute *f2fs_feat_attrs[] = {
ATTR_LIST(flexible_inline_xattr),
ATTR_LIST(quota_ino),
ATTR_LIST(inode_crtime),
+ ATTR_LIST(extended_node),
NULL,
};
diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
index ae2dfa709f5d..d847b2b11659 100644
--- a/fs/f2fs/xattr.c
+++ b/fs/f2fs/xattr.c
@@ -278,7 +278,8 @@ static int read_xattr_block(struct inode *inode, void *txattr_addr)
return PTR_ERR(xpage);
xattr_addr = page_address(xpage);
- memcpy(txattr_addr + inline_size, xattr_addr, VALID_XATTR_BLOCK_SIZE);
+ memcpy(txattr_addr + inline_size, xattr_addr,
+ VALID_XATTR_BLOCK_SIZE(inode));
f2fs_put_page(xpage, 1);
return 0;
@@ -291,7 +292,7 @@ static int lookup_all_xattrs(struct inode *inode, struct page *ipage,
{
void *cur_addr, *txattr_addr, *last_addr = NULL;
nid_t xnid = F2FS_I(inode)->i_xattr_nid;
- unsigned int size = xnid ? VALID_XATTR_BLOCK_SIZE : 0;
+ unsigned int size = xnid ? VALID_XATTR_BLOCK_SIZE(inode) : 0;
unsigned int inline_size = inline_xattr_size(inode);
int err = 0;
@@ -346,7 +347,7 @@ static int read_all_xattrs(struct inode *inode, struct page *ipage,
{
struct f2fs_xattr_header *header;
nid_t xnid = F2FS_I(inode)->i_xattr_nid;
- unsigned int size = VALID_XATTR_BLOCK_SIZE;
+ unsigned int size = VALID_XATTR_BLOCK_SIZE(inode);
unsigned int inline_size = inline_xattr_size(inode);
void *txattr_addr;
int err;
@@ -454,7 +455,8 @@ static inline int write_all_xattrs(struct inode *inode, __u32 hsize,
if (inline_size)
memcpy(inline_addr, txattr_addr, inline_size);
- memcpy(xattr_addr, txattr_addr + inline_size, VALID_XATTR_BLOCK_SIZE);
+ memcpy(xattr_addr, txattr_addr + inline_size,
+ VALID_XATTR_BLOCK_SIZE(inode));
if (inline_size)
set_page_dirty(ipage ? ipage : in_page);
diff --git a/fs/f2fs/xattr.h b/fs/f2fs/xattr.h
index dbcd1d16e669..8ddc94ea5d00 100644
--- a/fs/f2fs/xattr.h
+++ b/fs/f2fs/xattr.h
@@ -72,10 +72,12 @@ struct f2fs_xattr_entry {
for (entry = XATTR_FIRST_ENTRY(addr);\
!IS_XATTR_LAST_ENTRY(entry);\
entry = XATTR_NEXT_ENTRY(entry))
-#define VALID_XATTR_BLOCK_SIZE (PAGE_SIZE - sizeof(struct node_footer))
+#define VALID_XATTR_BLOCK_SIZE(i) (PAGE_SIZE - \
+ sizeof(struct node_footer) - \
+ F2FS_I_SB(i)->extra_nsize)
#define XATTR_PADDING_SIZE (sizeof(__u32))
#define MIN_OFFSET(i) XATTR_ALIGN(inline_xattr_size(i) + \
- VALID_XATTR_BLOCK_SIZE)
+ VALID_XATTR_BLOCK_SIZE(i))
#define MAX_VALUE_LEN(i) (MIN_OFFSET(i) - \
sizeof(struct f2fs_xattr_header) - \
diff --git a/include/linux/f2fs_fs.h b/include/linux/f2fs_fs.h
index f7f09907e69d..a6bacfdd378d 100644
--- a/include/linux/f2fs_fs.h
+++ b/include/linux/f2fs_fs.h
@@ -111,7 +111,8 @@ struct f2fs_super_block {
__u8 encrypt_pw_salt[16]; /* Salt used for string2key algorithm */
struct f2fs_device devs[MAX_DEVICES]; /* device list */
__le32 qf_ino[F2FS_MAX_QUOTAS]; /* quota inode numbers */
- __u8 reserved[315]; /* valid reserved region */
+ __le16 extra_nsize; /* extra node size */
+ __u8 reserved[313]; /* valid reserved region */
} __packed;
/*
@@ -192,15 +193,16 @@ struct f2fs_extent {
/* 200 bytes for inline xattrs by default */
#define DEFAULT_INLINE_XATTR_ADDRS 50
#define DEF_ADDRS_PER_INODE 923 /* Address Pointers in an Inode */
+#define DEF_ADDRS_PER_BLOCK 1018 /* Address Pointers in a Direct Block */
+#define DEF_NIDS_PER_BLOCK 1018 /* Node IDs in an Indirect Block */
+#define DEF_NIDS_PER_INODE 5 /* Node IDs in an Inode */
#define CUR_ADDRS_PER_INODE(inode) (DEF_ADDRS_PER_INODE - \
get_extra_isize(inode))
-#define DEF_NIDS_PER_INODE 5 /* Node IDs in an Inode */
#define ADDRS_PER_INODE(inode) addrs_per_inode(inode)
-#define ADDRS_PER_BLOCK 1018 /* Address Pointers in a Direct Block */
-#define NIDS_PER_BLOCK 1018 /* Node IDs in an Indirect Block */
-
+#define ADDRS_PER_BLOCK(inode) addrs_per_dnode(inode)
+#define NIDS_PER_BLOCK(inode) nids_per_idnode(inode)
#define ADDRS_PER_PAGE(page, inode) \
- (IS_INODE(page) ? ADDRS_PER_INODE(inode) : ADDRS_PER_BLOCK)
+ (IS_INODE(page) ? ADDRS_PER_INODE(inode) : ADDRS_PER_BLOCK(inode))
#define NODE_DIR1_BLOCK (DEF_ADDRS_PER_INODE + 1)
#define NODE_DIR2_BLOCK (DEF_ADDRS_PER_INODE + 2)
@@ -265,11 +267,11 @@ struct f2fs_inode {
} __packed;
struct direct_node {
- __le32 addr[ADDRS_PER_BLOCK]; /* array of data block address */
+ __le32 addr[DEF_ADDRS_PER_BLOCK]; /* array of data block address */
} __packed;
struct indirect_node {
- __le32 nid[NIDS_PER_BLOCK]; /* array of data block address */
+ __le32 nid[DEF_NIDS_PER_BLOCK]; /* array of data block address */
} __packed;
enum {
--
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
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH 2/2] f2fs: support {d,id,did,x}node checksum
2018-01-27 9:43 [PATCH 1/2] f2fs: enhance scalibility of {d, id, did, x}node disk layout Chao Yu
@ 2018-01-27 9:43 ` Chao Yu
2018-01-31 2:02 ` Jaegeuk Kim
2020-02-14 2:32 ` [f2fs-dev] " Chao Yu
0 siblings, 2 replies; 16+ messages in thread
From: Chao Yu @ 2018-01-27 9:43 UTC (permalink / raw)
To: jaegeuk; +Cc: linux-f2fs-devel, linux-kernel, chao, Chao Yu
This patch adds to support {d,id,did,x}node checksum in kernel side.
Signed-off-by: Chao Yu <yuchao0@huawei.com>
---
fs/f2fs/f2fs.h | 15 +++++++-
fs/f2fs/inode.c | 98 +++++++++++++++++++++++++++++++------------------
fs/f2fs/node.c | 2 +-
fs/f2fs/segment.c | 2 +-
fs/f2fs/sysfs.c | 7 ++++
include/linux/f2fs_fs.h | 10 ++++-
6 files changed, 93 insertions(+), 41 deletions(-)
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 6f5e41657c62..ad48d389fea4 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -128,6 +128,7 @@ struct f2fs_mount_info {
#define F2FS_FEATURE_QUOTA_INO 0x0080
#define F2FS_FEATURE_INODE_CRTIME 0x0100
#define F2FS_FEATURE_EXTENDED_NODE 0x0200
+#define F2FS_FEATURE_NODE_CHECKSUM 0x0400
#define F2FS_HAS_FEATURE(sb, mask) \
((F2FS_SB(sb)->raw_super->feature & cpu_to_le32(mask)) != 0)
@@ -2570,6 +2571,11 @@ static inline int get_inline_xattr_addrs(struct inode *inode)
sizeof((f2fs_inode)->field)) \
<= (F2FS_OLD_ATTRIBUTE_SIZE + extra_isize)) \
+#define F2FS_FITS_IN_NODE(extra_nsize, field) \
+ ((offsetof(struct f2fs_node, footer) - \
+ offsetof(struct f2fs_node, field)) \
+ <= (extra_nsize)) \
+
static inline void f2fs_reset_iostat(struct f2fs_sb_info *sbi)
{
int i;
@@ -2616,8 +2622,8 @@ int f2fs_pin_file_control(struct inode *inode, bool inc);
* inode.c
*/
void f2fs_set_inode_flags(struct inode *inode);
-bool f2fs_inode_chksum_verify(struct f2fs_sb_info *sbi, struct page *page);
-void f2fs_inode_chksum_set(struct f2fs_sb_info *sbi, struct page *page);
+bool f2fs_node_chksum_verify(struct f2fs_sb_info *sbi, struct page *page);
+void f2fs_node_chksum_set(struct f2fs_sb_info *sbi, struct page *page);
struct inode *f2fs_iget(struct super_block *sb, unsigned long ino);
struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino);
int try_to_free_nats(struct f2fs_sb_info *sbi, int nr_shrink);
@@ -3252,6 +3258,11 @@ static inline int f2fs_sb_has_extended_node(struct super_block *sb)
return F2FS_HAS_FEATURE(sb, F2FS_FEATURE_EXTENDED_NODE);
}
+static inline int f2fs_sb_has_node_checksum(struct super_block *sb)
+{
+ return F2FS_HAS_FEATURE(sb, F2FS_FEATURE_NODE_CHECKSUM);
+}
+
#ifdef CONFIG_BLK_DEV_ZONED
static inline int get_blkz_type(struct f2fs_sb_info *sbi,
struct block_device *bdev, block_t blkaddr)
diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
index f5b5606ca273..f879f0a65986 100644
--- a/fs/f2fs/inode.c
+++ b/fs/f2fs/inode.c
@@ -111,75 +111,103 @@ static void __recover_inline_status(struct inode *inode, struct page *ipage)
return;
}
-static bool f2fs_enable_inode_chksum(struct f2fs_sb_info *sbi, struct page *page)
+static bool f2fs_enable_node_chksum(struct f2fs_sb_info *sbi, struct page *page)
{
- struct f2fs_inode *ri = &F2FS_NODE(page)->i;
- int extra_isize = le32_to_cpu(ri->i_extra_isize);
-
- if (!f2fs_sb_has_inode_chksum(sbi->sb))
- return false;
-
- if (!RAW_IS_INODE(F2FS_NODE(page)) || !(ri->i_inline & F2FS_EXTRA_ATTR))
- return false;
-
- if (!F2FS_FITS_IN_INODE(ri, extra_isize, i_inode_checksum))
- return false;
+ if (IS_INODE(page)) {
+ struct f2fs_inode *ri = &F2FS_NODE(page)->i;
+ int extra_isize = le32_to_cpu(ri->i_extra_isize);
+
+ if (!(ri->i_inline & F2FS_EXTRA_ATTR))
+ return false;
+ if (!f2fs_sb_has_inode_chksum(sbi->sb))
+ return false;
+ if (!F2FS_FITS_IN_INODE(ri, extra_isize, i_inode_checksum))
+ return false;
+ } else {
+ if (!f2fs_sb_has_extended_node(sbi->sb))
+ return false;
+ if (!f2fs_sb_has_node_checksum(sbi->sb))
+ return false;
+ if (!F2FS_FITS_IN_NODE(sbi->extra_nsize, node_checksum))
+ return false;
+ }
return true;
}
-static __u32 f2fs_inode_chksum(struct f2fs_sb_info *sbi, struct page *page)
+static __u32 f2fs_node_chksum(struct f2fs_sb_info *sbi, struct page *page)
{
struct f2fs_node *node = F2FS_NODE(page);
- struct f2fs_inode *ri = &node->i;
- __le32 ino = node->footer.ino;
- __le32 gen = ri->i_generation;
- __u32 chksum, chksum_seed;
- __u32 dummy_cs = 0;
- unsigned int offset = offsetof(struct f2fs_inode, i_inode_checksum);
- unsigned int cs_size = sizeof(dummy_cs);
+ __le32 append, ino = node->footer.ino;
+ __u32 chksum, chksum_seed, dummy_cs = 0;
+ unsigned int offset, cs_size = sizeof(dummy_cs);
+ bool is_inode = RAW_IS_INODE(node);
+
+ if (is_inode) {
+ append = node->i.i_generation;
+ offset = offsetof(struct f2fs_inode, i_inode_checksum);
+ } else {
+ append = node->footer.nid;
+ offset = offsetof(struct f2fs_node, node_checksum);
+ }
chksum = f2fs_chksum(sbi, sbi->s_chksum_seed, (__u8 *)&ino,
sizeof(ino));
- chksum_seed = f2fs_chksum(sbi, chksum, (__u8 *)&gen, sizeof(gen));
+ chksum_seed = f2fs_chksum(sbi, chksum, (__u8 *)&append, sizeof(append));
- chksum = f2fs_chksum(sbi, chksum_seed, (__u8 *)ri, offset);
+ chksum = f2fs_chksum(sbi, chksum_seed, (__u8 *)node, offset);
chksum = f2fs_chksum(sbi, chksum, (__u8 *)&dummy_cs, cs_size);
offset += cs_size;
- chksum = f2fs_chksum(sbi, chksum, (__u8 *)ri + offset,
+ chksum = f2fs_chksum(sbi, chksum, (__u8 *)node + offset,
F2FS_BLKSIZE - offset);
return chksum;
}
-bool f2fs_inode_chksum_verify(struct f2fs_sb_info *sbi, struct page *page)
+bool f2fs_node_chksum_verify(struct f2fs_sb_info *sbi, struct page *page)
{
- struct f2fs_inode *ri;
+ struct f2fs_node *rn;
__u32 provided, calculated;
- if (!f2fs_enable_inode_chksum(sbi, page) ||
+ if (!f2fs_enable_node_chksum(sbi, page) ||
PageDirty(page) || PageWriteback(page))
return true;
- ri = &F2FS_NODE(page)->i;
- provided = le32_to_cpu(ri->i_inode_checksum);
- calculated = f2fs_inode_chksum(sbi, page);
+ rn = F2FS_NODE(page);
+ if (RAW_IS_INODE(rn))
+ provided = le32_to_cpu(rn->i.i_inode_checksum);
+ else
+ provided = le32_to_cpu(rn->node_checksum);
+ calculated = f2fs_node_chksum(sbi, page);
if (provided != calculated)
f2fs_msg(sbi->sb, KERN_WARNING,
- "checksum invalid, ino = %x, %x vs. %x",
- ino_of_node(page), provided, calculated);
+ "checksum invalid, ino = %u, nid = %u, %x vs. %x",
+ ino_of_node(page), nid_of_node(page),
+ provided, calculated);
+
+ else
+ f2fs_msg(sbi->sb, KERN_WARNING,
+ "checksum is valid, ino = %u, nid = %u, %x vs. %x",
+ ino_of_node(page), nid_of_node(page),
+ provided, calculated);
return provided == calculated;
}
-void f2fs_inode_chksum_set(struct f2fs_sb_info *sbi, struct page *page)
+void f2fs_node_chksum_set(struct f2fs_sb_info *sbi, struct page *page)
{
- struct f2fs_inode *ri = &F2FS_NODE(page)->i;
+ struct f2fs_node *rn = F2FS_NODE(page);
+ __le32 chksum;
- if (!f2fs_enable_inode_chksum(sbi, page))
+ if (!f2fs_enable_node_chksum(sbi, page))
return;
- ri->i_inode_checksum = cpu_to_le32(f2fs_inode_chksum(sbi, page));
+ chksum = cpu_to_le32(f2fs_node_chksum(sbi, page));
+
+ if (RAW_IS_INODE(rn))
+ rn->i.i_inode_checksum = chksum;
+ else
+ rn->node_checksum = chksum;
}
static int do_read_inode(struct inode *inode)
diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
index bc8424babf36..613e6bc65f9c 100644
--- a/fs/f2fs/node.c
+++ b/fs/f2fs/node.c
@@ -1194,7 +1194,7 @@ static struct page *__get_node_page(struct f2fs_sb_info *sbi, pgoff_t nid,
goto out_err;
}
- if (!f2fs_inode_chksum_verify(sbi, page)) {
+ if (!f2fs_node_chksum_verify(sbi, page)) {
err = -EBADMSG;
goto out_err;
}
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index ba8cd4e5ad75..5aa2c24cdcd8 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -2678,7 +2678,7 @@ void allocate_data_block(struct f2fs_sb_info *sbi, struct page *page,
if (page && IS_NODESEG(type)) {
fill_node_footer_blkaddr(page, NEXT_FREE_BLKADDR(sbi, curseg));
- f2fs_inode_chksum_set(sbi, page);
+ f2fs_node_chksum_set(sbi, page);
}
if (add_list) {
diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
index f25a6fc0a17e..24d3b82c7bf0 100644
--- a/fs/f2fs/sysfs.c
+++ b/fs/f2fs/sysfs.c
@@ -119,6 +119,9 @@ static ssize_t features_show(struct f2fs_attr *a,
if (f2fs_sb_has_extended_node(sb))
len += snprintf(buf + len, PAGE_SIZE - len, "%s%s",
len ? ", " : "", "extended_node");
+ if (f2fs_sb_has_node_checksum(sb))
+ len += snprintf(buf + len, PAGE_SIZE - len, "%s%s",
+ len ? ", " : "", "node_checksum");
len += snprintf(buf + len, PAGE_SIZE - len, "\n");
return len;
}
@@ -240,6 +243,7 @@ enum feat_id {
FEAT_QUOTA_INO,
FEAT_INODE_CRTIME,
FEAT_EXTENDED_NODE,
+ FEAT_NODE_CHECKSUM,
};
static ssize_t f2fs_feature_show(struct f2fs_attr *a,
@@ -256,6 +260,7 @@ static ssize_t f2fs_feature_show(struct f2fs_attr *a,
case FEAT_QUOTA_INO:
case FEAT_INODE_CRTIME:
case FEAT_EXTENDED_NODE:
+ case FEAT_NODE_CHECKSUM:
return snprintf(buf, PAGE_SIZE, "supported\n");
}
return 0;
@@ -335,6 +340,7 @@ F2FS_FEATURE_RO_ATTR(flexible_inline_xattr, FEAT_FLEXIBLE_INLINE_XATTR);
F2FS_FEATURE_RO_ATTR(quota_ino, FEAT_QUOTA_INO);
F2FS_FEATURE_RO_ATTR(inode_crtime, FEAT_INODE_CRTIME);
F2FS_FEATURE_RO_ATTR(extended_node, FEAT_EXTENDED_NODE);
+F2FS_FEATURE_RO_ATTR(node_checksum, FEAT_NODE_CHECKSUM);
#define ATTR_LIST(name) (&f2fs_attr_##name.attr)
static struct attribute *f2fs_attrs[] = {
@@ -390,6 +396,7 @@ static struct attribute *f2fs_feat_attrs[] = {
ATTR_LIST(quota_ino),
ATTR_LIST(inode_crtime),
ATTR_LIST(extended_node),
+ ATTR_LIST(node_checksum),
NULL,
};
diff --git a/include/linux/f2fs_fs.h b/include/linux/f2fs_fs.h
index a6bacfdd378d..941b5330c504 100644
--- a/include/linux/f2fs_fs.h
+++ b/include/linux/f2fs_fs.h
@@ -295,8 +295,14 @@ struct f2fs_node {
/* can be one of three types: inode, direct, and indirect types */
union {
struct f2fs_inode i;
- struct direct_node dn;
- struct indirect_node in;
+ union {
+ struct {
+ __le32 data[1017];
+ __le32 node_checksum;
+ };
+ struct direct_node dn;
+ struct indirect_node in;
+ };
};
struct node_footer footer;
} __packed;
--
2.15.0.55.gc2ece9dc4de6
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] f2fs: support {d,id,did,x}node checksum
2018-01-27 9:43 ` [PATCH 2/2] f2fs: support {d,id,did,x}node checksum Chao Yu
@ 2018-01-31 2:02 ` Jaegeuk Kim
2018-01-31 7:14 ` Chao Yu
2020-02-14 2:32 ` [f2fs-dev] " Chao Yu
1 sibling, 1 reply; 16+ messages in thread
From: Jaegeuk Kim @ 2018-01-31 2:02 UTC (permalink / raw)
To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel
On 01/27, Chao Yu wrote:
> This patch adds to support {d,id,did,x}node checksum in kernel side.
>
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> ---
> fs/f2fs/f2fs.h | 15 +++++++-
> fs/f2fs/inode.c | 98 +++++++++++++++++++++++++++++++------------------
> fs/f2fs/node.c | 2 +-
> fs/f2fs/segment.c | 2 +-
> fs/f2fs/sysfs.c | 7 ++++
> include/linux/f2fs_fs.h | 10 ++++-
> 6 files changed, 93 insertions(+), 41 deletions(-)
>
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 6f5e41657c62..ad48d389fea4 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -128,6 +128,7 @@ struct f2fs_mount_info {
> #define F2FS_FEATURE_QUOTA_INO 0x0080
> #define F2FS_FEATURE_INODE_CRTIME 0x0100
> #define F2FS_FEATURE_EXTENDED_NODE 0x0200
> +#define F2FS_FEATURE_NODE_CHECKSUM 0x0400
>
> #define F2FS_HAS_FEATURE(sb, mask) \
> ((F2FS_SB(sb)->raw_super->feature & cpu_to_le32(mask)) != 0)
> @@ -2570,6 +2571,11 @@ static inline int get_inline_xattr_addrs(struct inode *inode)
> sizeof((f2fs_inode)->field)) \
> <= (F2FS_OLD_ATTRIBUTE_SIZE + extra_isize)) \
>
> +#define F2FS_FITS_IN_NODE(extra_nsize, field) \
> + ((offsetof(struct f2fs_node, footer) - \
> + offsetof(struct f2fs_node, field)) \
> + <= (extra_nsize)) \
> +
> static inline void f2fs_reset_iostat(struct f2fs_sb_info *sbi)
> {
> int i;
> @@ -2616,8 +2622,8 @@ int f2fs_pin_file_control(struct inode *inode, bool inc);
> * inode.c
> */
> void f2fs_set_inode_flags(struct inode *inode);
> -bool f2fs_inode_chksum_verify(struct f2fs_sb_info *sbi, struct page *page);
> -void f2fs_inode_chksum_set(struct f2fs_sb_info *sbi, struct page *page);
> +bool f2fs_node_chksum_verify(struct f2fs_sb_info *sbi, struct page *page);
> +void f2fs_node_chksum_set(struct f2fs_sb_info *sbi, struct page *page);
> struct inode *f2fs_iget(struct super_block *sb, unsigned long ino);
> struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino);
> int try_to_free_nats(struct f2fs_sb_info *sbi, int nr_shrink);
> @@ -3252,6 +3258,11 @@ static inline int f2fs_sb_has_extended_node(struct super_block *sb)
> return F2FS_HAS_FEATURE(sb, F2FS_FEATURE_EXTENDED_NODE);
> }
>
> +static inline int f2fs_sb_has_node_checksum(struct super_block *sb)
> +{
> + return F2FS_HAS_FEATURE(sb, F2FS_FEATURE_NODE_CHECKSUM);
> +}
> +
> #ifdef CONFIG_BLK_DEV_ZONED
> static inline int get_blkz_type(struct f2fs_sb_info *sbi,
> struct block_device *bdev, block_t blkaddr)
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index f5b5606ca273..f879f0a65986 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -111,75 +111,103 @@ static void __recover_inline_status(struct inode *inode, struct page *ipage)
> return;
> }
>
> -static bool f2fs_enable_inode_chksum(struct f2fs_sb_info *sbi, struct page *page)
> +static bool f2fs_enable_node_chksum(struct f2fs_sb_info *sbi, struct page *page)
> {
> - struct f2fs_inode *ri = &F2FS_NODE(page)->i;
> - int extra_isize = le32_to_cpu(ri->i_extra_isize);
> -
> - if (!f2fs_sb_has_inode_chksum(sbi->sb))
> - return false;
> -
> - if (!RAW_IS_INODE(F2FS_NODE(page)) || !(ri->i_inline & F2FS_EXTRA_ATTR))
> - return false;
> -
> - if (!F2FS_FITS_IN_INODE(ri, extra_isize, i_inode_checksum))
> - return false;
> + if (IS_INODE(page)) {
> + struct f2fs_inode *ri = &F2FS_NODE(page)->i;
> + int extra_isize = le32_to_cpu(ri->i_extra_isize);
> +
> + if (!(ri->i_inline & F2FS_EXTRA_ATTR))
> + return false;
> + if (!f2fs_sb_has_inode_chksum(sbi->sb))
> + return false;
> + if (!F2FS_FITS_IN_INODE(ri, extra_isize, i_inode_checksum))
> + return false;
> + } else {
> + if (!f2fs_sb_has_extended_node(sbi->sb))
> + return false;
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
of extra_nsize? For example, layout #1 means node_checksum with extra_nsize=X?
> + if (!f2fs_sb_has_node_checksum(sbi->sb))
> + return false;
> + if (!F2FS_FITS_IN_NODE(sbi->extra_nsize, node_checksum))
> + return false;
> + }
>
> return true;
> }
>
> -static __u32 f2fs_inode_chksum(struct f2fs_sb_info *sbi, struct page *page)
> +static __u32 f2fs_node_chksum(struct f2fs_sb_info *sbi, struct page *page)
> {
> struct f2fs_node *node = F2FS_NODE(page);
> - struct f2fs_inode *ri = &node->i;
> - __le32 ino = node->footer.ino;
> - __le32 gen = ri->i_generation;
> - __u32 chksum, chksum_seed;
> - __u32 dummy_cs = 0;
> - unsigned int offset = offsetof(struct f2fs_inode, i_inode_checksum);
> - unsigned int cs_size = sizeof(dummy_cs);
> + __le32 append, ino = node->footer.ino;
> + __u32 chksum, chksum_seed, dummy_cs = 0;
> + unsigned int offset, cs_size = sizeof(dummy_cs);
> + bool is_inode = RAW_IS_INODE(node);
> +
> + if (is_inode) {
> + append = node->i.i_generation;
> + offset = offsetof(struct f2fs_inode, i_inode_checksum);
> + } else {
> + append = node->footer.nid;
> + offset = offsetof(struct f2fs_node, node_checksum);
> + }
>
> chksum = f2fs_chksum(sbi, sbi->s_chksum_seed, (__u8 *)&ino,
> sizeof(ino));
> - chksum_seed = f2fs_chksum(sbi, chksum, (__u8 *)&gen, sizeof(gen));
> + chksum_seed = f2fs_chksum(sbi, chksum, (__u8 *)&append, sizeof(append));
>
> - chksum = f2fs_chksum(sbi, chksum_seed, (__u8 *)ri, offset);
> + chksum = f2fs_chksum(sbi, chksum_seed, (__u8 *)node, offset);
> chksum = f2fs_chksum(sbi, chksum, (__u8 *)&dummy_cs, cs_size);
> offset += cs_size;
> - chksum = f2fs_chksum(sbi, chksum, (__u8 *)ri + offset,
> + chksum = f2fs_chksum(sbi, chksum, (__u8 *)node + offset,
> F2FS_BLKSIZE - offset);
> return chksum;
> }
>
> -bool f2fs_inode_chksum_verify(struct f2fs_sb_info *sbi, struct page *page)
> +bool f2fs_node_chksum_verify(struct f2fs_sb_info *sbi, struct page *page)
> {
> - struct f2fs_inode *ri;
> + struct f2fs_node *rn;
> __u32 provided, calculated;
>
> - if (!f2fs_enable_inode_chksum(sbi, page) ||
> + if (!f2fs_enable_node_chksum(sbi, page) ||
> PageDirty(page) || PageWriteback(page))
> return true;
>
> - ri = &F2FS_NODE(page)->i;
> - provided = le32_to_cpu(ri->i_inode_checksum);
> - calculated = f2fs_inode_chksum(sbi, page);
> + rn = F2FS_NODE(page);
> + if (RAW_IS_INODE(rn))
> + provided = le32_to_cpu(rn->i.i_inode_checksum);
> + else
> + provided = le32_to_cpu(rn->node_checksum);
> + calculated = f2fs_node_chksum(sbi, page);
>
> if (provided != calculated)
> f2fs_msg(sbi->sb, KERN_WARNING,
> - "checksum invalid, ino = %x, %x vs. %x",
> - ino_of_node(page), provided, calculated);
> + "checksum invalid, ino = %u, nid = %u, %x vs. %x",
> + ino_of_node(page), nid_of_node(page),
> + provided, calculated);
> +
> + else
> + f2fs_msg(sbi->sb, KERN_WARNING,
> + "checksum is valid, ino = %u, nid = %u, %x vs. %x",
> + ino_of_node(page), nid_of_node(page),
> + provided, calculated);
Need to clean up.
>
> return provided == calculated;
> }
>
> -void f2fs_inode_chksum_set(struct f2fs_sb_info *sbi, struct page *page)
> +void f2fs_node_chksum_set(struct f2fs_sb_info *sbi, struct page *page)
> {
> - struct f2fs_inode *ri = &F2FS_NODE(page)->i;
> + struct f2fs_node *rn = F2FS_NODE(page);
> + __le32 chksum;
>
> - if (!f2fs_enable_inode_chksum(sbi, page))
> + if (!f2fs_enable_node_chksum(sbi, page))
> return;
>
> - ri->i_inode_checksum = cpu_to_le32(f2fs_inode_chksum(sbi, page));
> + chksum = cpu_to_le32(f2fs_node_chksum(sbi, page));
> +
> + if (RAW_IS_INODE(rn))
> + rn->i.i_inode_checksum = chksum;
> + else
> + rn->node_checksum = chksum;
> }
>
> static int do_read_inode(struct inode *inode)
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index bc8424babf36..613e6bc65f9c 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -1194,7 +1194,7 @@ static struct page *__get_node_page(struct f2fs_sb_info *sbi, pgoff_t nid,
> goto out_err;
> }
>
> - if (!f2fs_inode_chksum_verify(sbi, page)) {
> + if (!f2fs_node_chksum_verify(sbi, page)) {
> err = -EBADMSG;
> goto out_err;
> }
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index ba8cd4e5ad75..5aa2c24cdcd8 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -2678,7 +2678,7 @@ void allocate_data_block(struct f2fs_sb_info *sbi, struct page *page,
> if (page && IS_NODESEG(type)) {
> fill_node_footer_blkaddr(page, NEXT_FREE_BLKADDR(sbi, curseg));
>
> - f2fs_inode_chksum_set(sbi, page);
> + f2fs_node_chksum_set(sbi, page);
> }
>
> if (add_list) {
> diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
> index f25a6fc0a17e..24d3b82c7bf0 100644
> --- a/fs/f2fs/sysfs.c
> +++ b/fs/f2fs/sysfs.c
> @@ -119,6 +119,9 @@ static ssize_t features_show(struct f2fs_attr *a,
> if (f2fs_sb_has_extended_node(sb))
> len += snprintf(buf + len, PAGE_SIZE - len, "%s%s",
> len ? ", " : "", "extended_node");
> + if (f2fs_sb_has_node_checksum(sb))
> + len += snprintf(buf + len, PAGE_SIZE - len, "%s%s",
> + len ? ", " : "", "node_checksum");
> len += snprintf(buf + len, PAGE_SIZE - len, "\n");
> return len;
> }
> @@ -240,6 +243,7 @@ enum feat_id {
> FEAT_QUOTA_INO,
> FEAT_INODE_CRTIME,
> FEAT_EXTENDED_NODE,
> + FEAT_NODE_CHECKSUM,
> };
>
> static ssize_t f2fs_feature_show(struct f2fs_attr *a,
> @@ -256,6 +260,7 @@ static ssize_t f2fs_feature_show(struct f2fs_attr *a,
> case FEAT_QUOTA_INO:
> case FEAT_INODE_CRTIME:
> case FEAT_EXTENDED_NODE:
> + case FEAT_NODE_CHECKSUM:
> return snprintf(buf, PAGE_SIZE, "supported\n");
> }
> return 0;
> @@ -335,6 +340,7 @@ F2FS_FEATURE_RO_ATTR(flexible_inline_xattr, FEAT_FLEXIBLE_INLINE_XATTR);
> F2FS_FEATURE_RO_ATTR(quota_ino, FEAT_QUOTA_INO);
> F2FS_FEATURE_RO_ATTR(inode_crtime, FEAT_INODE_CRTIME);
> F2FS_FEATURE_RO_ATTR(extended_node, FEAT_EXTENDED_NODE);
> +F2FS_FEATURE_RO_ATTR(node_checksum, FEAT_NODE_CHECKSUM);
>
> #define ATTR_LIST(name) (&f2fs_attr_##name.attr)
> static struct attribute *f2fs_attrs[] = {
> @@ -390,6 +396,7 @@ static struct attribute *f2fs_feat_attrs[] = {
> ATTR_LIST(quota_ino),
> ATTR_LIST(inode_crtime),
> ATTR_LIST(extended_node),
> + ATTR_LIST(node_checksum),
> NULL,
> };
>
> diff --git a/include/linux/f2fs_fs.h b/include/linux/f2fs_fs.h
> index a6bacfdd378d..941b5330c504 100644
> --- a/include/linux/f2fs_fs.h
> +++ b/include/linux/f2fs_fs.h
> @@ -295,8 +295,14 @@ struct f2fs_node {
> /* can be one of three types: inode, direct, and indirect types */
> union {
> struct f2fs_inode i;
> - struct direct_node dn;
> - struct indirect_node in;
> + union {
> + struct {
> + __le32 data[1017];
> + __le32 node_checksum;
What does 1017 mean? We need to make this structure more flexibly for new
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];
__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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] f2fs: support {d,id,did,x}node checksum
2018-01-31 2:02 ` Jaegeuk Kim
@ 2018-01-31 7:14 ` Chao Yu
2018-01-31 22:15 ` Jaegeuk Kim
0 siblings, 1 reply; 16+ messages in thread
From: Chao Yu @ 2018-01-31 7:14 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: linux-f2fs-devel, linux-kernel, chao
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.
Thanks,
> __le32 comp[V2_NSIZE];
> }
> ...
>
>> + };
>> + struct direct_node dn;
>> + struct indirect_node in;
>> + };
>> };
>> struct node_footer footer;
>> } __packed;
>> --
>> 2.15.0.55.gc2ece9dc4de6
>
> .
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] f2fs: support {d,id,did,x}node checksum
2018-01-31 7:14 ` Chao Yu
@ 2018-01-31 22:15 ` Jaegeuk Kim
2018-02-01 14:20 ` Chao Yu
0 siblings, 1 reply; 16+ messages in thread
From: Jaegeuk Kim @ 2018-01-31 22:15 UTC (permalink / raw)
To: Chao Yu; +Cc: linux-kernel, linux-f2fs-devel
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
can't set each of features individually. Like the second entry should have
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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] f2fs: support {d,id,did,x}node checksum
2018-01-31 22:15 ` Jaegeuk Kim
@ 2018-02-01 14:20 ` Chao Yu
2018-02-10 1:41 ` Jaegeuk Kim
0 siblings, 1 reply; 16+ messages in thread
From: Chao Yu @ 2018-02-01 14:20 UTC (permalink / raw)
To: Jaegeuk Kim, Chao Yu; +Cc: linux-f2fs-devel, linux-kernel
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_FIELD1 0x0002
#define F2FS_NODE_FIELD2 0x0004
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
>>>
>>> .
>>>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] f2fs: support {d,id,did,x}node checksum
2018-02-01 14:20 ` Chao Yu
@ 2018-02-10 1:41 ` Jaegeuk Kim
2018-02-10 2:52 ` Chao Yu
0 siblings, 1 reply; 16+ messages in thread
From: Jaegeuk Kim @ 2018-02-10 1:41 UTC (permalink / raw)
To: Chao Yu; +Cc: Chao Yu, linux-f2fs-devel, linux-kernel
On 02/01, Chao Yu wrote:
>
>
> 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_FIELD1 0x0002
> #define F2FS_NODE_FIELD2 0x0004
>
> 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.
So, that's why I thought we may need a sort of each formats.
>
> 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
> >>>
> >>> .
> >>>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] f2fs: support {d,id,did,x}node checksum
2018-02-10 1:41 ` Jaegeuk Kim
@ 2018-02-10 2:52 ` Chao Yu
2018-02-13 7:34 ` Chao Yu
0 siblings, 1 reply; 16+ messages in thread
From: Chao Yu @ 2018-02-10 2:52 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: linux-kernel, linux-f2fs-devel
On 2018/2/10 9:41, Jaegeuk Kim wrote:
> On 02/01, Chao Yu wrote:
>>
>>
>> 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_FIELD1 0x0002
>> #define F2FS_NODE_FIELD2 0x0004
>>
>> 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.
>
> So, that's why I thought we may need a sort of each formats.
Hmm.. if we have two new added fields, there are (2 << 2) combinations
of all formats, as:
struct original {
__le32 data[DEF_ADDRS_PER_BLOCK];
}
struct node_v1 {
__le32 data[DEF_ADDRS_PER_BLOCK - V1_NSIZE=1];
__le32 field_1;
}
struct node_v2 {
__le32 data[DEF_ADDRS_PER_BLOCK - V2_NSIZE=1];
__le32 field_2;
}
struct node_v2 {
__le32 data[DEF_ADDRS_PER_BLOCK - V3_NSIZE=2];
__le32 field_1;
__le32 field_2;
}
If we add more new fields, the node version will increase sharply due
to there is (n << 2) combination with n fields. Right? Any thoughts to
reduce maintaining overhead on those node versions structures?
Thanks,
>
>>
>> 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] f2fs: support {d,id,did,x}node checksum
2018-02-10 2:52 ` Chao Yu
@ 2018-02-13 7:34 ` Chao Yu
2018-02-27 14:16 ` Chao Yu
0 siblings, 1 reply; 16+ messages in thread
From: Chao Yu @ 2018-02-13 7:34 UTC (permalink / raw)
To: Chao Yu, Jaegeuk Kim; +Cc: linux-kernel, linux-f2fs-devel
Hi Jaegeuk,
On 2018/2/10 10:52, Chao Yu wrote:
> On 2018/2/10 9:41, Jaegeuk Kim wrote:
>> On 02/01, Chao Yu wrote:
>>>
>>>
>>> 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_FIELD1 0x0002
>>> #define F2FS_NODE_FIELD2 0x0004
>>>
>>> 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.
>>
>> So, that's why I thought we may need a sort of each formats.
>
> Hmm.. if we have two new added fields, there are (2 << 2) combinations
> of all formats, as:
>
> struct original {
> __le32 data[DEF_ADDRS_PER_BLOCK];
> }
>
> struct node_v1 {
> __le32 data[DEF_ADDRS_PER_BLOCK - V1_NSIZE=1];
> __le32 field_1;
> }
>
> struct node_v2 {
> __le32 data[DEF_ADDRS_PER_BLOCK - V2_NSIZE=1];
> __le32 field_2;
> }
>
> struct node_v2 {
> __le32 data[DEF_ADDRS_PER_BLOCK - V3_NSIZE=2];
> __le32 field_1;
> __le32 field_2;
> }
>
> If we add more new fields, the node version will increase sharply due
> to there is (n << 2) combination with n fields. Right? Any thoughts to
> reduce maintaining overhead on those node versions structures?
Do you have time to explain more about the design of multiple version structure
for node block, I'm still be confused about two things:
1. what will we do if we want to add one new field in node structure.
2. how can we recognize which fields are valid and which ones are invalid.
Thanks,
>
> Thanks,
>
>>
>>>
>>> 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
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] f2fs: support {d,id,did,x}node checksum
2018-02-13 7:34 ` Chao Yu
@ 2018-02-27 14:16 ` Chao Yu
2018-02-28 5:34 ` Jaegeuk Kim
2018-04-13 8:40 ` Chao Yu
0 siblings, 2 replies; 16+ messages in thread
From: Chao Yu @ 2018-02-27 14:16 UTC (permalink / raw)
To: Chao Yu, Jaegeuk Kim; +Cc: linux-f2fs-devel, linux-kernel
Ping,
On 2018/2/13 15:34, Chao Yu wrote:
> Hi Jaegeuk,
>
> On 2018/2/10 10:52, Chao Yu wrote:
>> On 2018/2/10 9:41, Jaegeuk Kim wrote:
>>> On 02/01, Chao Yu wrote:
>>>>
>>>>
>>>> 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_FIELD1 0x0002
>>>> #define F2FS_NODE_FIELD2 0x0004
>>>>
>>>> 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.
>>>
>>> So, that's why I thought we may need a sort of each formats.
>>
>> Hmm.. if we have two new added fields, there are (2 << 2) combinations
>> of all formats, as:
>>
>> struct original {
>> __le32 data[DEF_ADDRS_PER_BLOCK];
>> }
>>
>> struct node_v1 {
>> __le32 data[DEF_ADDRS_PER_BLOCK - V1_NSIZE=1];
>> __le32 field_1;
>> }
>>
>> struct node_v2 {
>> __le32 data[DEF_ADDRS_PER_BLOCK - V2_NSIZE=1];
>> __le32 field_2;
>> }
>>
>> struct node_v2 {
>> __le32 data[DEF_ADDRS_PER_BLOCK - V3_NSIZE=2];
>> __le32 field_1;
>> __le32 field_2;
>> }
>>
>> If we add more new fields, the node version will increase sharply due
>> to there is (n << 2) combination with n fields. Right? Any thoughts to
>> reduce maintaining overhead on those node versions structures?
>
> Do you have time to explain more about the design of multiple version structure
> for node block, I'm still be confused about two things:
> 1. what will we do if we want to add one new field in node structure.
> 2. how can we recognize which fields are valid and which ones are invalid.
>
> Thanks,
>
>>
>> Thanks,
>>
>>>
>>>>
>>>> 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
>>>>>>>
>>>>>>> .
>>>>>>>
>>
>> .
>>
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] f2fs: support {d,id,did,x}node checksum
2018-02-27 14:16 ` Chao Yu
@ 2018-02-28 5:34 ` Jaegeuk Kim
2018-02-28 9:46 ` Chao Yu
2018-04-13 8:40 ` Chao Yu
1 sibling, 1 reply; 16+ messages in thread
From: Jaegeuk Kim @ 2018-02-28 5:34 UTC (permalink / raw)
To: Chao Yu; +Cc: Chao Yu, linux-f2fs-devel, linux-kernel
On 02/27, Chao Yu wrote:
> Ping,
>
> On 2018/2/13 15:34, Chao Yu wrote:
> > Hi Jaegeuk,
> >
> > On 2018/2/10 10:52, Chao Yu wrote:
> >> On 2018/2/10 9:41, Jaegeuk Kim wrote:
> >>> On 02/01, Chao Yu wrote:
> >>>>
> >>>>
> >>>> 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_FIELD1 0x0002
> >>>> #define F2FS_NODE_FIELD2 0x0004
> >>>>
> >>>> 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.
> >>>
> >>> So, that's why I thought we may need a sort of each formats.
> >>
> >> Hmm.. if we have two new added fields, there are (2 << 2) combinations
> >> of all formats, as:
> >>
> >> struct original {
> >> __le32 data[DEF_ADDRS_PER_BLOCK];
> >> }
> >>
> >> struct node_v1 {
> >> __le32 data[DEF_ADDRS_PER_BLOCK - V1_NSIZE=1];
> >> __le32 field_1;
> >> }
> >>
> >> struct node_v2 {
> >> __le32 data[DEF_ADDRS_PER_BLOCK - V2_NSIZE=1];
> >> __le32 field_2;
> >> }
> >>
> >> struct node_v2 {
> >> __le32 data[DEF_ADDRS_PER_BLOCK - V3_NSIZE=2];
> >> __le32 field_1;
> >> __le32 field_2;
> >> }
> >>
> >> If we add more new fields, the node version will increase sharply due
> >> to there is (n << 2) combination with n fields. Right? Any thoughts to
> >> reduce maintaining overhead on those node versions structures?
> >
> > Do you have time to explain more about the design of multiple version structure
> > for node block, I'm still be confused about two things:
> > 1. what will we do if we want to add one new field in node structure.
> > 2. how can we recognize which fields are valid and which ones are invalid.
Can we discuss this in LSF/MM, if we get an invitation letter? :P
> >
> > Thanks,
> >
> >>
> >> Thanks,
> >>
> >>>
> >>>>
> >>>> 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
> >>>>>>>
> >>>>>>> .
> >>>>>>>
> >>
> >> .
> >>
> >
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] f2fs: support {d,id,did,x}node checksum
2018-02-28 5:34 ` Jaegeuk Kim
@ 2018-02-28 9:46 ` Chao Yu
0 siblings, 0 replies; 16+ messages in thread
From: Chao Yu @ 2018-02-28 9:46 UTC (permalink / raw)
To: Jaegeuk Kim, Chao Yu; +Cc: linux-f2fs-devel, linux-kernel
On 2018/2/28 13:34, Jaegeuk Kim wrote:
> On 02/27, Chao Yu wrote:
>> Ping,
>>
>> On 2018/2/13 15:34, Chao Yu wrote:
>>> Hi Jaegeuk,
>>>
>>> On 2018/2/10 10:52, Chao Yu wrote:
>>>> On 2018/2/10 9:41, Jaegeuk Kim wrote:
>>>>> On 02/01, Chao Yu wrote:
>>>>>>
>>>>>>
>>>>>> 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_FIELD1 0x0002
>>>>>> #define F2FS_NODE_FIELD2 0x0004
>>>>>>
>>>>>> 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.
>>>>>
>>>>> So, that's why I thought we may need a sort of each formats.
>>>>
>>>> Hmm.. if we have two new added fields, there are (2 << 2) combinations
>>>> of all formats, as:
>>>>
>>>> struct original {
>>>> __le32 data[DEF_ADDRS_PER_BLOCK];
>>>> }
>>>>
>>>> struct node_v1 {
>>>> __le32 data[DEF_ADDRS_PER_BLOCK - V1_NSIZE=1];
>>>> __le32 field_1;
>>>> }
>>>>
>>>> struct node_v2 {
>>>> __le32 data[DEF_ADDRS_PER_BLOCK - V2_NSIZE=1];
>>>> __le32 field_2;
>>>> }
>>>>
>>>> struct node_v2 {
>>>> __le32 data[DEF_ADDRS_PER_BLOCK - V3_NSIZE=2];
>>>> __le32 field_1;
>>>> __le32 field_2;
>>>> }
>>>>
>>>> If we add more new fields, the node version will increase sharply due
>>>> to there is (n << 2) combination with n fields. Right? Any thoughts to
>>>> reduce maintaining overhead on those node versions structures?
>>>
>>> Do you have time to explain more about the design of multiple version structure
>>> for node block, I'm still be confused about two things:
>>> 1. what will we do if we want to add one new field in node structure.
>>> 2. how can we recognize which fields are valid and which ones are invalid.
>
> Can we discuss this in LSF/MM, if we get an invitation letter? :P
I'm OK, I hope we can get the invitation and reach an agreement about node
extension format, so I can add checksum for node block as soon as possible,
since during development our guys suffer node block inconsistence occasionally,
I hope checksum can relief us from hard debug work on fs. ;)
Thanks,
>
>>>
>>> Thanks,
>>>
>>>>
>>>> Thanks,
>>>>
>>>>>
>>>>>>
>>>>>> 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
>>>>>>>>>
>>>>>>>>> .
>>>>>>>>>
>>>>
>>>> .
>>>>
>>>
>
> .
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] f2fs: support {d,id,did,x}node checksum
2018-02-27 14:16 ` Chao Yu
2018-02-28 5:34 ` Jaegeuk Kim
@ 2018-04-13 8:40 ` Chao Yu
2018-04-17 3:38 ` Jaegeuk Kim
1 sibling, 1 reply; 16+ messages in thread
From: Chao Yu @ 2018-04-13 8:40 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: Chao Yu, linux-f2fs-devel, linux-kernel
Ping again..
Do you have time to discuss this?
On 2018/2/27 22:16, Chao Yu wrote:
> Ping,
>
> On 2018/2/13 15:34, Chao Yu wrote:
>> Hi Jaegeuk,
>>
>> On 2018/2/10 10:52, Chao Yu wrote:
>>> On 2018/2/10 9:41, Jaegeuk Kim wrote:
>>>> On 02/01, Chao Yu wrote:
>>>>>
>>>>>
>>>>> 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_FIELD1 0x0002
>>>>> #define F2FS_NODE_FIELD2 0x0004
>>>>>
>>>>> 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.
>>>>
>>>> So, that's why I thought we may need a sort of each formats.
>>>
>>> Hmm.. if we have two new added fields, there are (2 << 2) combinations
>>> of all formats, as:
>>>
>>> struct original {
>>> __le32 data[DEF_ADDRS_PER_BLOCK];
>>> }
>>>
>>> struct node_v1 {
>>> __le32 data[DEF_ADDRS_PER_BLOCK - V1_NSIZE=1];
>>> __le32 field_1;
>>> }
>>>
>>> struct node_v2 {
>>> __le32 data[DEF_ADDRS_PER_BLOCK - V2_NSIZE=1];
>>> __le32 field_2;
>>> }
>>>
>>> struct node_v2 {
>>> __le32 data[DEF_ADDRS_PER_BLOCK - V3_NSIZE=2];
>>> __le32 field_1;
>>> __le32 field_2;
>>> }
>>>
>>> If we add more new fields, the node version will increase sharply due
>>> to there is (n << 2) combination with n fields. Right? Any thoughts to
>>> reduce maintaining overhead on those node versions structures?
>>
>> Do you have time to explain more about the design of multiple version structure
>> for node block, I'm still be confused about two things:
>> 1. what will we do if we want to add one new field in node structure.
>> 2. how can we recognize which fields are valid and which ones are invalid.
>>
>> Thanks,
>>
>>>
>>> Thanks,
>>>
>>>>
>>>>>
>>>>> 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
>>>>>>>>
>>>>>>>> .
>>>>>>>>
>>>
>>> .
>>>
>>
>
> .
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] f2fs: support {d,id,did,x}node checksum
2018-04-13 8:40 ` Chao Yu
@ 2018-04-17 3:38 ` Jaegeuk Kim
2018-04-17 7:11 ` Chao Yu
0 siblings, 1 reply; 16+ messages in thread
From: Jaegeuk Kim @ 2018-04-17 3:38 UTC (permalink / raw)
To: Chao Yu; +Cc: Chao Yu, linux-f2fs-devel, linux-kernel
On 04/13, Chao Yu wrote:
> Ping again..
>
> Do you have time to discuss this?
We may need a time to have a chat in person. Do you have any chance to visit
US?
>
> On 2018/2/27 22:16, Chao Yu wrote:
> > Ping,
> >
> > On 2018/2/13 15:34, Chao Yu wrote:
> >> Hi Jaegeuk,
> >>
> >> On 2018/2/10 10:52, Chao Yu wrote:
> >>> On 2018/2/10 9:41, Jaegeuk Kim wrote:
> >>>> On 02/01, Chao Yu wrote:
> >>>>>
> >>>>>
> >>>>> 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_FIELD1 0x0002
> >>>>> #define F2FS_NODE_FIELD2 0x0004
> >>>>>
> >>>>> 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.
> >>>>
> >>>> So, that's why I thought we may need a sort of each formats.
> >>>
> >>> Hmm.. if we have two new added fields, there are (2 << 2) combinations
> >>> of all formats, as:
> >>>
> >>> struct original {
> >>> __le32 data[DEF_ADDRS_PER_BLOCK];
> >>> }
> >>>
> >>> struct node_v1 {
> >>> __le32 data[DEF_ADDRS_PER_BLOCK - V1_NSIZE=1];
> >>> __le32 field_1;
> >>> }
> >>>
> >>> struct node_v2 {
> >>> __le32 data[DEF_ADDRS_PER_BLOCK - V2_NSIZE=1];
> >>> __le32 field_2;
> >>> }
> >>>
> >>> struct node_v2 {
> >>> __le32 data[DEF_ADDRS_PER_BLOCK - V3_NSIZE=2];
> >>> __le32 field_1;
> >>> __le32 field_2;
> >>> }
> >>>
> >>> If we add more new fields, the node version will increase sharply due
> >>> to there is (n << 2) combination with n fields. Right? Any thoughts to
> >>> reduce maintaining overhead on those node versions structures?
> >>
> >> Do you have time to explain more about the design of multiple version structure
> >> for node block, I'm still be confused about two things:
> >> 1. what will we do if we want to add one new field in node structure.
> >> 2. how can we recognize which fields are valid and which ones are invalid.
> >>
> >> Thanks,
> >>
> >>>
> >>> Thanks,
> >>>
> >>>>
> >>>>>
> >>>>> 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
> >>>>>>>>
> >>>>>>>> .
> >>>>>>>>
> >>>
> >>> .
> >>>
> >>
> >
> > .
> >
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 2/2] f2fs: support {d,id,did,x}node checksum
2018-04-17 3:38 ` Jaegeuk Kim
@ 2018-04-17 7:11 ` Chao Yu
0 siblings, 0 replies; 16+ messages in thread
From: Chao Yu @ 2018-04-17 7:11 UTC (permalink / raw)
To: Jaegeuk Kim; +Cc: Chao Yu, linux-f2fs-devel, linux-kernel
On 2018/4/17 11:38, Jaegeuk Kim wrote:
> On 04/13, Chao Yu wrote:
>> Ping again..
>>
>> Do you have time to discuss this?
>
> We may need a time to have a chat in person. Do you have any chance to visit
> US?
I prefer to, just count on LSF, but...
I think I need to find a conference which is opened in US first.
Just checked events.linuxfoundation.org, and didn't find any suitable conference
I could attend recently in US.
Location: US
Apr 18-20 Boston, Could Foundry Summit
Apr 23-25 Park City, LSF
Sep 24-26 Nashville, API strategy & practice
Oct 10-11 New York, Open FinTech Forum
Dec 11-13 Seattle, KubeCon & CloudNativeCon
Any other conferences?
Thanks,
>
>>
>> On 2018/2/27 22:16, Chao Yu wrote:
>>> Ping,
>>>
>>> On 2018/2/13 15:34, Chao Yu wrote:
>>>> Hi Jaegeuk,
>>>>
>>>> On 2018/2/10 10:52, Chao Yu wrote:
>>>>> On 2018/2/10 9:41, Jaegeuk Kim wrote:
>>>>>> On 02/01, Chao Yu wrote:
>>>>>>>
>>>>>>>
>>>>>>> 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_FIELD1 0x0002
>>>>>>> #define F2FS_NODE_FIELD2 0x0004
>>>>>>>
>>>>>>> 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.
>>>>>>
>>>>>> So, that's why I thought we may need a sort of each formats.
>>>>>
>>>>> Hmm.. if we have two new added fields, there are (2 << 2) combinations
>>>>> of all formats, as:
>>>>>
>>>>> struct original {
>>>>> __le32 data[DEF_ADDRS_PER_BLOCK];
>>>>> }
>>>>>
>>>>> struct node_v1 {
>>>>> __le32 data[DEF_ADDRS_PER_BLOCK - V1_NSIZE=1];
>>>>> __le32 field_1;
>>>>> }
>>>>>
>>>>> struct node_v2 {
>>>>> __le32 data[DEF_ADDRS_PER_BLOCK - V2_NSIZE=1];
>>>>> __le32 field_2;
>>>>> }
>>>>>
>>>>> struct node_v2 {
>>>>> __le32 data[DEF_ADDRS_PER_BLOCK - V3_NSIZE=2];
>>>>> __le32 field_1;
>>>>> __le32 field_2;
>>>>> }
>>>>>
>>>>> If we add more new fields, the node version will increase sharply due
>>>>> to there is (n << 2) combination with n fields. Right? Any thoughts to
>>>>> reduce maintaining overhead on those node versions structures?
>>>>
>>>> Do you have time to explain more about the design of multiple version structure
>>>> for node block, I'm still be confused about two things:
>>>> 1. what will we do if we want to add one new field in node structure.
>>>> 2. how can we recognize which fields are valid and which ones are invalid.
>>>>
>>>> Thanks,
>>>>
>>>>>
>>>>> Thanks,
>>>>>
>>>>>>
>>>>>>>
>>>>>>> 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
>>>>>>>>>>
>>>>>>>>>> .
>>>>>>>>>>
>>>>>
>>>>> .
>>>>>
>>>>
>>>
>>> .
>>>
>
> .
>
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [f2fs-dev] [PATCH 2/2] f2fs: support {d,id,did,x}node checksum
2018-01-27 9:43 ` [PATCH 2/2] f2fs: support {d,id,did,x}node checksum Chao Yu
2018-01-31 2:02 ` Jaegeuk Kim
@ 2020-02-14 2:32 ` Chao Yu
1 sibling, 0 replies; 16+ messages in thread
From: Chao Yu @ 2020-02-14 2:32 UTC (permalink / raw)
To: jaegeuk; +Cc: linux-kernel, linux-f2fs-devel
Hi Jaegeuk,
Is it time to consider this metadata chksum feature? Maybe for 5.7 version.
Thanks,
On 2018/1/27 17:43, Chao Yu wrote:
> This patch adds to support {d,id,did,x}node checksum in kernel side.
>
> Signed-off-by: Chao Yu <yuchao0@huawei.com>
> ---
> fs/f2fs/f2fs.h | 15 +++++++-
> fs/f2fs/inode.c | 98 +++++++++++++++++++++++++++++++------------------
> fs/f2fs/node.c | 2 +-
> fs/f2fs/segment.c | 2 +-
> fs/f2fs/sysfs.c | 7 ++++
> include/linux/f2fs_fs.h | 10 ++++-
> 6 files changed, 93 insertions(+), 41 deletions(-)
>
> diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
> index 6f5e41657c62..ad48d389fea4 100644
> --- a/fs/f2fs/f2fs.h
> +++ b/fs/f2fs/f2fs.h
> @@ -128,6 +128,7 @@ struct f2fs_mount_info {
> #define F2FS_FEATURE_QUOTA_INO 0x0080
> #define F2FS_FEATURE_INODE_CRTIME 0x0100
> #define F2FS_FEATURE_EXTENDED_NODE 0x0200
> +#define F2FS_FEATURE_NODE_CHECKSUM 0x0400
>
> #define F2FS_HAS_FEATURE(sb, mask) \
> ((F2FS_SB(sb)->raw_super->feature & cpu_to_le32(mask)) != 0)
> @@ -2570,6 +2571,11 @@ static inline int get_inline_xattr_addrs(struct inode *inode)
> sizeof((f2fs_inode)->field)) \
> <= (F2FS_OLD_ATTRIBUTE_SIZE + extra_isize)) \
>
> +#define F2FS_FITS_IN_NODE(extra_nsize, field) \
> + ((offsetof(struct f2fs_node, footer) - \
> + offsetof(struct f2fs_node, field)) \
> + <= (extra_nsize)) \
> +
> static inline void f2fs_reset_iostat(struct f2fs_sb_info *sbi)
> {
> int i;
> @@ -2616,8 +2622,8 @@ int f2fs_pin_file_control(struct inode *inode, bool inc);
> * inode.c
> */
> void f2fs_set_inode_flags(struct inode *inode);
> -bool f2fs_inode_chksum_verify(struct f2fs_sb_info *sbi, struct page *page);
> -void f2fs_inode_chksum_set(struct f2fs_sb_info *sbi, struct page *page);
> +bool f2fs_node_chksum_verify(struct f2fs_sb_info *sbi, struct page *page);
> +void f2fs_node_chksum_set(struct f2fs_sb_info *sbi, struct page *page);
> struct inode *f2fs_iget(struct super_block *sb, unsigned long ino);
> struct inode *f2fs_iget_retry(struct super_block *sb, unsigned long ino);
> int try_to_free_nats(struct f2fs_sb_info *sbi, int nr_shrink);
> @@ -3252,6 +3258,11 @@ static inline int f2fs_sb_has_extended_node(struct super_block *sb)
> return F2FS_HAS_FEATURE(sb, F2FS_FEATURE_EXTENDED_NODE);
> }
>
> +static inline int f2fs_sb_has_node_checksum(struct super_block *sb)
> +{
> + return F2FS_HAS_FEATURE(sb, F2FS_FEATURE_NODE_CHECKSUM);
> +}
> +
> #ifdef CONFIG_BLK_DEV_ZONED
> static inline int get_blkz_type(struct f2fs_sb_info *sbi,
> struct block_device *bdev, block_t blkaddr)
> diff --git a/fs/f2fs/inode.c b/fs/f2fs/inode.c
> index f5b5606ca273..f879f0a65986 100644
> --- a/fs/f2fs/inode.c
> +++ b/fs/f2fs/inode.c
> @@ -111,75 +111,103 @@ static void __recover_inline_status(struct inode *inode, struct page *ipage)
> return;
> }
>
> -static bool f2fs_enable_inode_chksum(struct f2fs_sb_info *sbi, struct page *page)
> +static bool f2fs_enable_node_chksum(struct f2fs_sb_info *sbi, struct page *page)
> {
> - struct f2fs_inode *ri = &F2FS_NODE(page)->i;
> - int extra_isize = le32_to_cpu(ri->i_extra_isize);
> -
> - if (!f2fs_sb_has_inode_chksum(sbi->sb))
> - return false;
> -
> - if (!RAW_IS_INODE(F2FS_NODE(page)) || !(ri->i_inline & F2FS_EXTRA_ATTR))
> - return false;
> -
> - if (!F2FS_FITS_IN_INODE(ri, extra_isize, i_inode_checksum))
> - return false;
> + if (IS_INODE(page)) {
> + struct f2fs_inode *ri = &F2FS_NODE(page)->i;
> + int extra_isize = le32_to_cpu(ri->i_extra_isize);
> +
> + if (!(ri->i_inline & F2FS_EXTRA_ATTR))
> + return false;
> + if (!f2fs_sb_has_inode_chksum(sbi->sb))
> + return false;
> + if (!F2FS_FITS_IN_INODE(ri, extra_isize, i_inode_checksum))
> + return false;
> + } else {
> + if (!f2fs_sb_has_extended_node(sbi->sb))
> + return false;
> + if (!f2fs_sb_has_node_checksum(sbi->sb))
> + return false;
> + if (!F2FS_FITS_IN_NODE(sbi->extra_nsize, node_checksum))
> + return false;
> + }
>
> return true;
> }
>
> -static __u32 f2fs_inode_chksum(struct f2fs_sb_info *sbi, struct page *page)
> +static __u32 f2fs_node_chksum(struct f2fs_sb_info *sbi, struct page *page)
> {
> struct f2fs_node *node = F2FS_NODE(page);
> - struct f2fs_inode *ri = &node->i;
> - __le32 ino = node->footer.ino;
> - __le32 gen = ri->i_generation;
> - __u32 chksum, chksum_seed;
> - __u32 dummy_cs = 0;
> - unsigned int offset = offsetof(struct f2fs_inode, i_inode_checksum);
> - unsigned int cs_size = sizeof(dummy_cs);
> + __le32 append, ino = node->footer.ino;
> + __u32 chksum, chksum_seed, dummy_cs = 0;
> + unsigned int offset, cs_size = sizeof(dummy_cs);
> + bool is_inode = RAW_IS_INODE(node);
> +
> + if (is_inode) {
> + append = node->i.i_generation;
> + offset = offsetof(struct f2fs_inode, i_inode_checksum);
> + } else {
> + append = node->footer.nid;
> + offset = offsetof(struct f2fs_node, node_checksum);
> + }
>
> chksum = f2fs_chksum(sbi, sbi->s_chksum_seed, (__u8 *)&ino,
> sizeof(ino));
> - chksum_seed = f2fs_chksum(sbi, chksum, (__u8 *)&gen, sizeof(gen));
> + chksum_seed = f2fs_chksum(sbi, chksum, (__u8 *)&append, sizeof(append));
>
> - chksum = f2fs_chksum(sbi, chksum_seed, (__u8 *)ri, offset);
> + chksum = f2fs_chksum(sbi, chksum_seed, (__u8 *)node, offset);
> chksum = f2fs_chksum(sbi, chksum, (__u8 *)&dummy_cs, cs_size);
> offset += cs_size;
> - chksum = f2fs_chksum(sbi, chksum, (__u8 *)ri + offset,
> + chksum = f2fs_chksum(sbi, chksum, (__u8 *)node + offset,
> F2FS_BLKSIZE - offset);
> return chksum;
> }
>
> -bool f2fs_inode_chksum_verify(struct f2fs_sb_info *sbi, struct page *page)
> +bool f2fs_node_chksum_verify(struct f2fs_sb_info *sbi, struct page *page)
> {
> - struct f2fs_inode *ri;
> + struct f2fs_node *rn;
> __u32 provided, calculated;
>
> - if (!f2fs_enable_inode_chksum(sbi, page) ||
> + if (!f2fs_enable_node_chksum(sbi, page) ||
> PageDirty(page) || PageWriteback(page))
> return true;
>
> - ri = &F2FS_NODE(page)->i;
> - provided = le32_to_cpu(ri->i_inode_checksum);
> - calculated = f2fs_inode_chksum(sbi, page);
> + rn = F2FS_NODE(page);
> + if (RAW_IS_INODE(rn))
> + provided = le32_to_cpu(rn->i.i_inode_checksum);
> + else
> + provided = le32_to_cpu(rn->node_checksum);
> + calculated = f2fs_node_chksum(sbi, page);
>
> if (provided != calculated)
> f2fs_msg(sbi->sb, KERN_WARNING,
> - "checksum invalid, ino = %x, %x vs. %x",
> - ino_of_node(page), provided, calculated);
> + "checksum invalid, ino = %u, nid = %u, %x vs. %x",
> + ino_of_node(page), nid_of_node(page),
> + provided, calculated);
> +
> + else
> + f2fs_msg(sbi->sb, KERN_WARNING,
> + "checksum is valid, ino = %u, nid = %u, %x vs. %x",
> + ino_of_node(page), nid_of_node(page),
> + provided, calculated);
>
> return provided == calculated;
> }
>
> -void f2fs_inode_chksum_set(struct f2fs_sb_info *sbi, struct page *page)
> +void f2fs_node_chksum_set(struct f2fs_sb_info *sbi, struct page *page)
> {
> - struct f2fs_inode *ri = &F2FS_NODE(page)->i;
> + struct f2fs_node *rn = F2FS_NODE(page);
> + __le32 chksum;
>
> - if (!f2fs_enable_inode_chksum(sbi, page))
> + if (!f2fs_enable_node_chksum(sbi, page))
> return;
>
> - ri->i_inode_checksum = cpu_to_le32(f2fs_inode_chksum(sbi, page));
> + chksum = cpu_to_le32(f2fs_node_chksum(sbi, page));
> +
> + if (RAW_IS_INODE(rn))
> + rn->i.i_inode_checksum = chksum;
> + else
> + rn->node_checksum = chksum;
> }
>
> static int do_read_inode(struct inode *inode)
> diff --git a/fs/f2fs/node.c b/fs/f2fs/node.c
> index bc8424babf36..613e6bc65f9c 100644
> --- a/fs/f2fs/node.c
> +++ b/fs/f2fs/node.c
> @@ -1194,7 +1194,7 @@ static struct page *__get_node_page(struct f2fs_sb_info *sbi, pgoff_t nid,
> goto out_err;
> }
>
> - if (!f2fs_inode_chksum_verify(sbi, page)) {
> + if (!f2fs_node_chksum_verify(sbi, page)) {
> err = -EBADMSG;
> goto out_err;
> }
> diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
> index ba8cd4e5ad75..5aa2c24cdcd8 100644
> --- a/fs/f2fs/segment.c
> +++ b/fs/f2fs/segment.c
> @@ -2678,7 +2678,7 @@ void allocate_data_block(struct f2fs_sb_info *sbi, struct page *page,
> if (page && IS_NODESEG(type)) {
> fill_node_footer_blkaddr(page, NEXT_FREE_BLKADDR(sbi, curseg));
>
> - f2fs_inode_chksum_set(sbi, page);
> + f2fs_node_chksum_set(sbi, page);
> }
>
> if (add_list) {
> diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
> index f25a6fc0a17e..24d3b82c7bf0 100644
> --- a/fs/f2fs/sysfs.c
> +++ b/fs/f2fs/sysfs.c
> @@ -119,6 +119,9 @@ static ssize_t features_show(struct f2fs_attr *a,
> if (f2fs_sb_has_extended_node(sb))
> len += snprintf(buf + len, PAGE_SIZE - len, "%s%s",
> len ? ", " : "", "extended_node");
> + if (f2fs_sb_has_node_checksum(sb))
> + len += snprintf(buf + len, PAGE_SIZE - len, "%s%s",
> + len ? ", " : "", "node_checksum");
> len += snprintf(buf + len, PAGE_SIZE - len, "\n");
> return len;
> }
> @@ -240,6 +243,7 @@ enum feat_id {
> FEAT_QUOTA_INO,
> FEAT_INODE_CRTIME,
> FEAT_EXTENDED_NODE,
> + FEAT_NODE_CHECKSUM,
> };
>
> static ssize_t f2fs_feature_show(struct f2fs_attr *a,
> @@ -256,6 +260,7 @@ static ssize_t f2fs_feature_show(struct f2fs_attr *a,
> case FEAT_QUOTA_INO:
> case FEAT_INODE_CRTIME:
> case FEAT_EXTENDED_NODE:
> + case FEAT_NODE_CHECKSUM:
> return snprintf(buf, PAGE_SIZE, "supported\n");
> }
> return 0;
> @@ -335,6 +340,7 @@ F2FS_FEATURE_RO_ATTR(flexible_inline_xattr, FEAT_FLEXIBLE_INLINE_XATTR);
> F2FS_FEATURE_RO_ATTR(quota_ino, FEAT_QUOTA_INO);
> F2FS_FEATURE_RO_ATTR(inode_crtime, FEAT_INODE_CRTIME);
> F2FS_FEATURE_RO_ATTR(extended_node, FEAT_EXTENDED_NODE);
> +F2FS_FEATURE_RO_ATTR(node_checksum, FEAT_NODE_CHECKSUM);
>
> #define ATTR_LIST(name) (&f2fs_attr_##name.attr)
> static struct attribute *f2fs_attrs[] = {
> @@ -390,6 +396,7 @@ static struct attribute *f2fs_feat_attrs[] = {
> ATTR_LIST(quota_ino),
> ATTR_LIST(inode_crtime),
> ATTR_LIST(extended_node),
> + ATTR_LIST(node_checksum),
> NULL,
> };
>
> diff --git a/include/linux/f2fs_fs.h b/include/linux/f2fs_fs.h
> index a6bacfdd378d..941b5330c504 100644
> --- a/include/linux/f2fs_fs.h
> +++ b/include/linux/f2fs_fs.h
> @@ -295,8 +295,14 @@ struct f2fs_node {
> /* can be one of three types: inode, direct, and indirect types */
> union {
> struct f2fs_inode i;
> - struct direct_node dn;
> - struct indirect_node in;
> + union {
> + struct {
> + __le32 data[1017];
> + __le32 node_checksum;
> + };
> + struct direct_node dn;
> + struct indirect_node in;
> + };
> };
> struct node_footer footer;
> } __packed;
>
_______________________________________________
Linux-f2fs-devel mailing list
Linux-f2fs-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-f2fs-devel
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2020-02-14 2:43 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-01-27 9:43 [PATCH 1/2] f2fs: enhance scalibility of {d, id, did, x}node disk layout Chao Yu
2018-01-27 9:43 ` [PATCH 2/2] f2fs: support {d,id,did,x}node checksum Chao Yu
2018-01-31 2:02 ` Jaegeuk Kim
2018-01-31 7:14 ` Chao Yu
2018-01-31 22:15 ` Jaegeuk Kim
2018-02-01 14:20 ` Chao Yu
2018-02-10 1:41 ` Jaegeuk Kim
2018-02-10 2:52 ` Chao Yu
2018-02-13 7:34 ` Chao Yu
2018-02-27 14:16 ` Chao Yu
2018-02-28 5:34 ` Jaegeuk Kim
2018-02-28 9:46 ` Chao Yu
2018-04-13 8:40 ` Chao Yu
2018-04-17 3:38 ` Jaegeuk Kim
2018-04-17 7:11 ` Chao Yu
2020-02-14 2:32 ` [f2fs-dev] " Chao Yu
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).