[Ocfs2-tools-devel] [PATCH 1/2] fsck: supporting fixing inode alloc group desc

[piaojun]

Hi Eric,

On 2018/1/31 14:41, Eric Ren wrote:
> Jun,
> 
>>> I mean if path == NULL, &path will crash?
>>>
>>  From my test, it won't cause crash, as '&path' is also a NULL pointer.
> 
> Yes, get the address of path that is NULL, returns NULL, no crash.
> 
> 
> """
> errcode_t ocfs2_free(void *ptr)
> {
>         void **pp = (void **)ptr;
> 
>         free(*pp);
>         *pp = NULL;
>         return 0;
> }
> """
> 
> But, pass NULL to ocfs2_free, pp will be NULL, *pp can crash.
> Look at other places in the code, some are using ocfs2_free() under "if"
> condition, but some are not.
> 
This won't cause crash, as '*pp = NULL' is just 'ptr = NULL'.

>>
>>>>>> +
>>>>>> +    return reti;
>>>>>> +}
>>>>>> +
>>>>>> +static errcode_t verify_group_desc(o2fsck_state *ost,
>>>>>> +                     struct ocfs2_dinode *di, int type)
>>>>>> +{
>>>>>> +    uint16_t bits;
>>>>>> +    uint64_t blkno;
>>>>>> +    errcode_t ret = 0;
>>>>>> +    int corrupted_bgs = 0, i;
>>>>>> +    struct ocfs2_chain_list *cl = &di->id2.i_chain;
>>>>>> +    struct ocfs2_chain_rec *rec;
>>>>>> +    struct ocfs2_group_desc *bgs = NULL;
>>>>>> +
>>>>>> +    ret = ocfs2_malloc_blocks(ost->ost_fs->fs_io,
>>>>>> +            cl->cl_next_free_rec, &bgs);
>>>>>> +    if (ret) {
>>>>>> +        com_err(whoami, ret, "while allocating block group descriptors");
>>>>>> +        goto out;
>>>>>> +    }
>>>>>> +    memset(bgs, 0, ost->ost_fs->fs_blocksize * cl->cl_next_free_rec);
>>>>>> +
>>>>>> +    for (i = 0; i < cl->cl_next_free_rec; i++) {
>>>>>> +        rec = &cl->cl_recs[i];
>>>>> One thing I doubt about:
>>>>>
>>>>> In this loop, it checks every first group descriptor of each chain, right? How about the
>>>>> rest gd on the chain?
>>>>>
>>>> Currently we could only fix the situation that there is one gd in each
>>>> chain, because we can hardly rebuild the gd far from chain header. For
>>>> example, when gd4 is corrupted, we can find its son gd247.
>>> I guess you meant "we cannot find its son"?
>> Yes, I made a mistake.
>>
>>> I know it's not easy, but it seems doable. I am thinking if it's possible to iterate the chain list
>>> in bottom-up order? If you simply fix gd1 by initializing it, you will loose the link to the rest gds,
>>> it's still a big problem, right?
>>>
>> The key problem is that we could not trust gd anymore as they have
>> been corrupted. So We must relay on the ocfs2_chain_list struct to
>> restore all gds. BTW the gds of the first level can hold
>> '1024 * 243 = 248832' files, and that's enough for most cases.
> 
> Hmm, could you put comments explaining this limitation both in commit log and above the function in code?
OK, I will add them later.

> 
> And, during iterating all files, if one file doesn't belong to any block group, it is a sign of problem, can we prompt
> a warning for user to make a decision?
We only iterate the gds in first level of gd chains, so probably there
will be some files not belong to any of the gds. That is to say, we
can't distinguish them from the bad files. And I think this patch
should not be responsible for the bad files.

thanks,
Jun

> 
> """
> 
> +    /* set group desc bitmap */
> +    for (i = 0; i < wp->corrupted_bgs; i++) {
> +        bg = &wp->bgs[i];
> +        bg_blkno = bg->bg_blkno;
> +        if (inode > bg_blkno && inode <= bg_blkno + bg->bg_bits) {     ====> if the inode doesn't fit into any bg?
> +            ocfs2_set_bit(inode - bg_blkno, bg->bg_bitmap);
> +            bg->bg_free_bits_count--;
> +        }
> +    }
> 
> 
> """
> 
> Otherwise, people may consider it will work in any situation.
> 
>>
>>>> rec  rec  rec  rec ... rec
>>>> gd1  gd2  gd3  gd4 ... gd243
>>>> |    |    |    |   ...
>>>> gd   gd   gd   gd247
>>>>
>>>>>> +        blkno = rec->c_blkno;
>>>>>> +        bits = rec->c_total;
>>>>>> +
>>>>>> +        ret = ocfs2_read_group_desc(ost->ost_fs, blkno,
>>>>>> +                (char *)&bgs[corrupted_bgs]);
>>>>>> +        if ((ret == OCFS2_ET_BAD_GROUP_DESC_MAGIC) ||
>>>>>> +            (!ret && bgs[corrupted_bgs].bg_generation != ost->ost_fs_generation)) {
>>>>>> +            if (!prompt(ost, PY, PR_GROUP_EXPECTED_DESC,
>>>>>> +                "Block %"PRIu64" should be a group "
>>>>>> +                "descriptor for the bitmap chain allocator "
>>>>>> +                "but it was corrupted.  Reinitialize it as "
>>>>>> +                "a group desc and link it into the bitmap "
>>>>>> +                "allocator?", blkno))
>>>>>> +                continue;
>>>>>> +            ocfs2_init_group_desc(ost->ost_fs,
>>>>>> +                    &bgs[corrupted_bgs],
>>>>>> +                    blkno, ost->ost_fs_generation,
>>>>>> +                    di->i_blkno, bits, i, 1);
>>>>> In the last review:
>>>>> """
>>>>>
>>>>> Hmm, regarding the last parameter of ocfs2_init_group_desc(...,
>>>>> suballoc), is it correct to always
>>>>> set 0 no matter it's global_inode_alloc or inode_alloc?
>>>>> """
>>>>>
>>>>> global_inode_alloc is also sub-alloctor? oh, I think so.
>>>>>
>>>>>
>>>>> Can fsck already fix global bitmap group desc?
>>>>>
>>>> Yes, it could.
>>> Is global_bitmap also a chain allocator? If so, I am curious about how global bitmap groups descs are fixed.
>> Yes, it is.
> 
> So, I guess it will share the similar issue as here. Any good points we can learn from there? Let me check a bit there.
> 
> Thanks,
> Eric
> .
>