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

[Coly Li]

On 04/23/2010 10:26 PM, Tao Ma Wrote:
> Hi coly,
>     the 2nd part of review for this really large patch. ;)
> 
> Coly Li wrote:
[snip]
>> +static int dir_trailer_func(ocfs2_filesys *fs,
>> +                uint64_t blkno,
>> +                uint64_t bcount,
>> +                uint16_t ext_flags,
>> +                void *priv_data)
>> +{
>> +    struct trailer_ctxt *ctxt = (struct trailer_ctxt *)priv_data;
>> +    struct ocfs2_dinode *di = ctxt->di;
>> +    struct ocfs2_dx_root_block *dx_root = ctxt->dx_root;
>> +    struct ocfs2_dir_block_trailer *trailer;
>> +    int max_rec_len = 0;
>> +    errcode_t ret = 0;
>> +    char *blk = NULL;
>> +
>> +    ret = ocfs2_malloc_block(fs->fs_io, &blk);
>> +    if (ret)
>> +        goto out;
> This function has the same problem for extent_iterate_el. We may need a
> errcode_t member in trailer_ctxt. And in case of any error in this
> function, we just need to set OCFS2_EXTENT_ERROR. And the iteration
> caller, ocfs2_init_dir_trailers in this case, should check the errcode
> by itself.

I see. Fixed similar issues in this function in next post.

>> +/*
>> + * This function overwite the indexed dir attribute of
>> + * the given inode. The caller should make sure the dir's
>> + * indexed tree is truncated.
>> + * Currently tunefs.ocfs2 is the only user, before calling
>> + * this function, tunefs.ocfs2 makes sure there is space
>> + * for directory trailer. So directory entry moves here.
>> + */
>> +errcode_t ocfs2_dx_dir_build(ocfs2_filesys *fs,
>> +            uint64_t dir)
>> +{
[snip]
>> +
>> +    di->i_dx_root = dr_blkno;
>> +    di->i_dyn_features |= OCFS2_INDEXED_DIR_FL;
>> +
>> +    ret = ocfs2_write_dx_root(fs, dr_blkno, dx_buf);
>> +    if (ret)
>> +        goto out;
>> +    ret = ocfs2_write_inode(fs, dir, di_buf);
>> +    if (ret)
>> +        goto out;
> Can we really update the inode information so earlier? If we set the
> indexed dir flag, we will search indexed dir first in ocfs2_lookup. But
> now we only update the trailer actually. So if the following
> ocfs2_dx_dir_insert has some error, the dir entries will be lost from
> the user's perspective.

Nice catching. The fix will be posted soon.

[snip]
>>  
>> -static void ocfs2_swap_dx_entry_list_to_cpu(struct
>> ocfs2_dx_entry_list *dl_list)
>> +static void ocfs2_swap_dx_entry_list(struct ocfs2_dx_entry_list
>> *dl_list)
>>  {
>>      int i;
>>  
>> -    if (cpu_is_little_endian)
>> -        return;
>> -
>>      dl_list->de_count    = bswap_16(dl_list->de_count);
>>      dl_list->de_num_used    = bswap_16(dl_list->de_num_used);
>>  
>>      for (i = 0; i < dl_list->de_count; i++)
>> -        ocfs2_swap_dx_entry_to_cpu(&dl_list->de_entries[i]);
>> +        ocfs2_swap_dx_entry(&dl_list->de_entries[i]);
>> +}
>> +
>> +static void ocfs2_swap_dx_entry_list_to_cpu(struct
>> ocfs2_dx_entry_list *dl_list)
>> +{
>> +    if (cpu_is_little_endian)
>> +        return;
>> +    ocfs2_swap_dx_entry_list(dl_list);
>> +}
>> +
>> +static void ocfs2_swap_dx_entry_list_from_cpu(struct
>> ocfs2_dx_entry_list *dl_list)
>> +{
>> +    if (cpu_is_little_endian)
>> +        return;
>> +    ocfs2_swap_dx_entry_list(dl_list);
>>  }
> Hey, why do you change the original one? Your code is buggy now. When

I change the original one, just want to avoid another ocfs2_swap_dx_entry_from_cpu().

> swap_entry_list_to_cpu you need to swap de_count/de_num_used first.
> While if we want to swap_entry_list_from_cpu we neet to swap the
> dx_entry first.

It should be safe to call ocfs2_swap_dx_entry_list() for both to_cpu and from_cpu. And the code is tested, it works here.

Thank you for the review :-)

-- 
Coly Li
SuSE Labs