Re: [PATCH 2/2] f2fs: support {d,id,did,x}node checksum

From: Chao Yu
Date: Wed Jan 31 2018 - 02:15:27 EST


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