[Ocfs2-tools-devel] [PATCH 01/15] dx_dirs v11: Add library support for directory indexing

[Coly Li]

On 04/26/2010 09:46 AM, Tao Ma Wrote:
> Hi coly,
> 
> Coly Li wrote:
>>
>> On 04/23/2010 02:47 PM, Tao Ma Wrote:
>>> Hi Coly/Mark,
>>>     Sorry for the delay.
>>>
>>> Coly Li wrote:
>>>> diff --git a/libocfs2/dir_iterate.c b/libocfs2/dir_iterate.c
>> [snip]
>>>> +static int dx_iterator(ocfs2_filesys *fs,
>>>> +               struct ocfs2_extent_rec *rec,
>>>> +               int tree_depth,
>>>> +               uint32_t ccount,
>>>> +               uint64_t ref_blkno,
>>>> +               int ref_recno,
>>>> +               void *priv_data)
>>>> +{
>>>> +    int ret, i;
>>>> +    struct ocfs2_dx_leaf *dx_leaf;
>>>> +    struct dx_iterator_data *iter = priv_data;
>>>> +    uint64_t blkno, count;
>>>> +
>>>> +    count = ocfs2_clusters_to_blocks(fs, rec->e_leaf_clusters);
>>>> +
>>>> +    blkno = rec->e_blkno;
>>>> +    for (i = 0; i < count; i++) {
>>>> +        ret = ocfs2_read_dx_leaf(fs, blkno, iter->leaf_buf);
>>>> +        if (ret)
>>>> +            return ret;
>>> I noticed that this function is actually called by extent_iterate_el. So
>>> I guess we'd better store 'ret' here in dx_iterator_data and return
>>> OCFS2_EXTENT_ERROR. That way we can stop iteration immediately. Return
>>> an errcode_t here can't work.
>>
>> Nice catch in extent related operations! The fix will be posted soon.
>>
>>>> +
>>>> +        dx_leaf = (struct ocfs2_dx_leaf *)iter->leaf_buf;
>>>> +        iter->dx_func(fs, &dx_leaf->dl_list, iter->dx_root, dx_leaf,
>>>> +                  iter->dx_priv_data);
>>> We will not take the return value into consideration here. Do we have a
>>> plan that we will support some return values like OCFS2_CHAIN_ABORT so
>>> that we can stop the iteration by the given callback functions?
>>
>> Great suggestion ! My fix looks like this,
>>
>>                 }
>>
>>                 dx_leaf = (struct ocfs2_dx_leaf *)iter->leaf_buf;
>> -               iter->dx_func(fs, &dx_leaf->dl_list, iter->dx_root,
>> dx_leaf,
>> +               err = iter->dx_func(fs, &dx_leaf->dl_list,
>> iter->dx_root, dx_leaf,
>>                               iter->dx_priv_data);
>> +               /* callback dx_func() is defined by users, the return
>> value does not
>> +                * follow libocfs2 error codes. Don't touch iter->err
>> and just stop
>> +                * the iteration here.*/
>> +               if (err)
>> +                       return OCFS2_EXTENT_ERROR;
>>
>>                 blkno++;
>>         }
>>
>> The idea is explained in the comments. How about this ?
> I mean we can do like we do for extent iteration.
> #define OCFS2_DX_CHANGED    0x01
> #define OCFS2_DX_ABORT      0x02
> #define OCFS2_DX_ERROR      0x04
> 
> And
>         if (err & (OCFS2_DX_ABORT | OCFS2_DX_ERROR))
>             return OCFS2_EXTENT_ABORT;
> 
> And in ocfs2_extent_iterate_dx_root, we don't check both the return
> value of ocfs2_extent_el and ctxt.err.

Hmm, I suggest not to do this. Condition here is a little bit different, dx_func is passed in from an application, here
the application is debugfs.ocfs2. This callback is defined in debugfs.ocfs2/dump.c, using an internal libocfs2 macro in
this place will be confused.

-- 
Coly Li
SuSE Labs