[PATCH] erofs: fix erofs_module_init & exit
Gao Xiang
gaoxiang25 at huawei.com
Fri Jul 6 12:09:15 AEST 2018
Hi Chao,
On 2018/7/6 9:30, Chao Yu wrote:
> Hi Xiang,
>
> On 2018/7/6 0:39, Gao Xiang wrote:
>> Hi Chao,
>>
>> On 2018/7/6 0:19, Chao Yu wrote:
>>> Hi Xiang,
>>>
>>> On 2018/7/5 13:03, Gao Xiang wrote:
>>>> This patch adds missing parts and EROFS ondisk
>>>> layout check as well.
>>>>
>>>> Signed-off-by: Gao Xiang <gaoxiang25 at huawei.com>
>>>> ---
>>>> fs/erofs/erofs_fs.h | 13 +++++++++++++
>>>> fs/erofs/super.c | 28 +++++++++++++++++++++-------
>>>> 2 files changed, 34 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/fs/erofs/erofs_fs.h b/fs/erofs/erofs_fs.h
>>>> index ed943d4..0dbf08d 100644
>>>> --- a/fs/erofs/erofs_fs.h
>>>> +++ b/fs/erofs/erofs_fs.h
>>>> @@ -253,5 +253,18 @@ enum {
>>>>
>>>> #define EROFS_NAME_LEN 255
>>>>
>>>> +/* check the EROFS on-disk layout strictly at compile time */
>>>> +static inline void erofs_check_ondisk_layout(void)
>>>> +{
>>>> + BUILD_BUG_ON(sizeof(struct erofs_super_block) != 128);
>>>> + BUILD_BUG_ON(sizeof(struct erofs_inode_v1) != 32);
>>>> + BUILD_BUG_ON(sizeof(struct erofs_inode_v2) != 64);
>>>> + BUILD_BUG_ON(sizeof(struct erofs_xattr_ibody_header) != 12);
>>>> + BUILD_BUG_ON(sizeof(struct erofs_xattr_entry) != 4);
>>>> + BUILD_BUG_ON(sizeof(struct erofs_extent_header) != 16);
>>>> + BUILD_BUG_ON(sizeof(struct erofs_decompressed_index_vle) != 8);
>>>> + BUILD_BUG_ON(sizeof(struct erofs_dirent) != 12);
>>> We'd better define some macros include above constants.
>>>
>>> Other parts look good to me.
>>>
>>> Thanks,
>>>
>> I was also thinking of wrapping in macros, but I thought over again,
>> it seems not too necessary because:
>>
>> 1) the above code is already well straight-forward to view and change if needed...
>>
>> BUILD_BUG_ON(sizeof(struct erofs_super_block) != 128);
>> <==> #define EROFS_SUPER_BLOCK_ONDISK_SIZE 128
>>
>> EROFS_SUPER_BLOCK_ONDISK_SIZE is not really needed since
>> we could use sizeof(struct erofs_super_block) directly in code.
>>
>> 2) Besides, I also observed the other file systems, such as xfs, sysv, minix, and ubifs, eg.
>>
>> fs/jffs2/super.c:
>> 366 /* Paranoia checks for on-medium structures. If we ask GCC
>> 367 to pack them with __attribute__((packed)) then it _also_
>> 368 assumes that they're not aligned -- so it emits crappy
>> 369 code on some architectures. Ideally we want an attribute
>> 370 which means just 'no padding', without the alignment
>> 371 thing. But GCC doesn't have that -- we have to just
>> 372 hope the structs are the right sizes, instead. */
>> 373 BUILD_BUG_ON(sizeof(struct jffs2_unknown_node) != 12);
>> 374 BUILD_BUG_ON(sizeof(struct jffs2_raw_dirent) != 40);
>> 375 BUILD_BUG_ON(sizeof(struct jffs2_raw_inode) != 68);
>> 376 BUILD_BUG_ON(sizeof(struct jffs2_raw_summary) != 32);
>>
>> fs/minix/inode.c:
>> 174 BUILD_BUG_ON(32 != sizeof (struct minix_inode));
>> 175 BUILD_BUG_ON(64 != sizeof(struct minix2_inode));
>>
>> fs/ubifs/super.c:
>> 359 BUILD_BUG_ON(1024 != sizeof (struct xenix_super_block));
>> 360 BUILD_BUG_ON(512 != sizeof (struct sysv4_super_block));
>> 361 BUILD_BUG_ON(512 != sizeof (struct sysv2_super_block));
>> 362 BUILD_BUG_ON(500 != sizeof (struct coh_super_block));
>> 363 BUILD_BUG_ON(64 != sizeof (struct sysv_inode));
>>
>> fs/ubifs/super.c
>> 2211 BUILD_BUG_ON(UBIFS_MAX_NODE_SZ & 7);
>> 2212 BUILD_BUG_ON(MIN_WRITE_SZ & 7);
>> 2213
>> 2214 /* Check min. node size */
>> 2215 BUILD_BUG_ON(UBIFS_INO_NODE_SZ < MIN_WRITE_SZ);
>> 2216 BUILD_BUG_ON(UBIFS_DENT_NODE_SZ < MIN_WRITE_SZ);
>> 2217 BUILD_BUG_ON(UBIFS_XENT_NODE_SZ < MIN_WRITE_SZ);
>> 2218 BUILD_BUG_ON(UBIFS_TRUN_NODE_SZ < MIN_WRITE_SZ);
>> 2219
>> 2220 BUILD_BUG_ON(UBIFS_MAX_DENT_NODE_SZ > UBIFS_MAX_NODE_SZ);
>> 2221 BUILD_BUG_ON(UBIFS_MAX_XENT_NODE_SZ > UBIFS_MAX_NODE_SZ);
>> 2222 BUILD_BUG_ON(UBIFS_MAX_DATA_NODE_SZ > UBIFS_MAX_NODE_SZ);
>> 2223 BUILD_BUG_ON(UBIFS_MAX_INO_NODE_SZ > UBIFS_MAX_NODE_SZ);
>> 2224
>> 2225 /* Defined node sizes */
>> 2226 BUILD_BUG_ON(UBIFS_SB_NODE_SZ != 4096);
>> 2227 BUILD_BUG_ON(UBIFS_MST_NODE_SZ != 512);
>> 2228 BUILD_BUG_ON(UBIFS_INO_NODE_SZ != 160);
>> 2229 BUILD_BUG_ON(UBIFS_REF_NODE_SZ != 64);
>>
>> how do you think? :) If you think it is necessary, I can change into marcos immediately~
> Alright, it's trivial, let's keep the code as original state. :)
>
> Reviewed-by: Chao Yu <yuchao0 at huawei.com>
>
:), Thanks for the review~
It seems that 'erofs_check_ondisk_layout_definations' is more proper than 'erofs_check_ondisk_layout',
which seems used for the runtime sanity check..
Let me send the next version :)
Thanks,
Gao Xiang
> Thanks,
>
>> Thanks,
>> Gao Xiang
>>
>>>> +}
>>>> +
>>>> #endif
>>>>
>>>> diff --git a/fs/erofs/super.c b/fs/erofs/super.c
>>>> index a1826b9..d104d88 100644
>>>> --- a/fs/erofs/super.c
>>>> +++ b/fs/erofs/super.c
>>>> @@ -37,6 +37,12 @@ static int erofs_init_inode_cache(void)
>>>> return erofs_inode_cachep != NULL ? 0 : -ENOMEM;
>>>> }
>>>>
>>>> +static void erofs_exit_inode_cache(void)
>>>> +{
>>>> + BUG_ON(erofs_inode_cachep == NULL);
>>>> + kmem_cache_destroy(erofs_inode_cachep);
>>>> +}
>>>> +
>>>> static struct inode *alloc_inode(struct super_block *sb)
>>>> {
>>>> struct erofs_vnode *vi =
>>>> @@ -398,22 +404,30 @@ int __init erofs_module_init(void)
>>>> {
>>>> int err;
>>>>
>>>> + erofs_check_ondisk_layout();
>>>> infoln("Initializing erofs " EROFS_VERSION);
>>>>
>>>> err = erofs_init_inode_cache();
>>>> - if (!err) {
>>>> - err = register_filesystem(&erofs_fs_type);
>>>> - if (!err) {
>>>> - infoln("Successfully to initialize erofs");
>>>> - return 0;
>>>> - }
>>>> - }
>>>> + if (err)
>>>> + goto icache_err;
>>>> +
>>>> + err = register_filesystem(&erofs_fs_type);
>>>> + if (err)
>>>> + goto fs_err;
>>>> +
>>>> + infoln("Successfully to initialize erofs");
>>>> + return 0;
>>>> +
>>>> +fs_err:
>>>> + erofs_exit_inode_cache();
>>>> +icache_err:
>>>> return err;
>>>> }
>>>>
>>>> void __exit erofs_module_exit(void)
>>>> {
>>>> unregister_filesystem(&erofs_fs_type);
>>>> + erofs_exit_inode_cache();
>>>> infoln("Successfully finalize erofs");
>>>> }
>>>>
>>>>
>> .
>>
More information about the Linux-erofs
mailing list