[Ocfs2-tools-devel] [PATCH 01/15] dx_dirs v11: Add library support for directory indexing
Tao Ma
tao.ma at oracle.com
Sun Apr 25 18:46:47 PDT 2010
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.
> [snip]
>>> +errcode_t ocfs2_extent_iterate_dx_root(ocfs2_filesys *fs,
>>> + struct ocfs2_dx_root_block *dx_root,
>>> + int flags,
>>> + char *block_buf,
>>> + int (*func)(ocfs2_filesys *fs,
>>> + struct ocfs2_extent_rec *rec,
>>> + int tree_depth,
>>> + uint32_t ccount,
>>> + uint64_t ref_blkno,
>>> + int ref_recno,
>>> + void *priv_data),
>>> + void *priv_data)
>>> +{
>>> + int i;
>>> + int iret = 0;
>>> + struct ocfs2_extent_list *el;
>>> + errcode_t ret;
>>> + struct extent_context ctxt;
>>> +
>>> + if (dx_root->dr_flags & OCFS2_DX_FLAG_INLINE)
>>> + return OCFS2_ET_INODE_CANNOT_BE_ITERATED;
>>> +
>>> + el = &dx_root->dr_list;
>>> + if (el->l_tree_depth) {
>>> + ret = ocfs2_malloc0(sizeof(char *) * el->l_tree_depth,
>>> + &ctxt.eb_bufs);
>>> + if (ret)
>>> + goto out;
>>> +
>>> + if (block_buf) {
>>> + ctxt.eb_bufs[0] = block_buf;
>>> + } else {
>>> + ret = ocfs2_malloc0(fs->fs_blocksize *
>>> + el->l_tree_depth,
>>> + &ctxt.eb_bufs[0]);
>>> + if (ret)
>>> + goto out_eb_bufs;
>>> + }
>>> +
>>> + for (i = 1; i < el->l_tree_depth; i++) {
>>> + ctxt.eb_bufs[i] = ctxt.eb_bufs[0] +
>>> + i * fs->fs_blocksize;
>>> + }
>>> + }
>>> + else
>>> + ctxt.eb_bufs = NULL;
>>> +
>>> + ctxt.fs = fs;
>>> + ctxt.func = func;
>>> + ctxt.priv_data = priv_data;
>>> + ctxt.flags = flags;
>>> + ctxt.ccount = 0;
>>> + ctxt.last_eb_blkno = 0;
>>> + ctxt.last_eb_cpos = 0;
>>> +
>>> + ret = 0;
>>> + iret |= extent_iterate_el(el, 0, &ctxt);
>>> + if (iret & OCFS2_EXTENT_ERROR)
>>> + ret = ctxt.errcode;
>>> +
>>> + if (iret & OCFS2_EXTENT_ABORT)
>>> + goto out_abort;
>>> +
>>> + /* we can only trust ctxt.last_eb_blkno if we walked the whole
>>> tree */
>>> + if (dx_root->dr_last_eb_blk != ctxt.last_eb_blkno) {
>>> + dx_root->dr_last_eb_blk = ctxt.last_eb_blkno;
>>> + iret |= OCFS2_EXTENT_CHANGED;
>>> + }
>>> +
>>> +out_abort:
>>> +#if 0
>>> + /*
>>> + * This block needs to be fixed up for write support.
>>> + */
>>> + if (!ret && (iret & OCFS2_EXTENT_CHANGED))
>>> + ret = ocfs2_write_inode(fs, inode->i_blkno, (char *)inode);
>>> +#endif
>>> +
>>> +out_eb_bufs:
>>> + if (ctxt.eb_bufs) {
>> hey, this field isn't set to NULL at the beginning.
>
> IMHO, the code here is correct.
> ctxt.eb_bufs only be freed only when, 1) el->l_tree_depth is not 0, 2) ctxt.eb_bufs is allocated.
> Could you give me more hint why you feel it's buggy here ?
sorry, I read your code wrongly.
>
>
>>> + {
>>> + .fn_name = "IndexedDirs",
>>> + .fn_flag = {0, OCFS2_FEATURE_INCOMPAT_INDEXED_DIRS, 0},
>> I remembered that now we required both featurn strings should have the
>> same name. Here one is "indexed-dirs", the other is "IndexedDirs".
>
> Hmm, IMHO, this should be fixed. Here is my proposal for the fix, I will post it as a separated patch for more review.
> diff --git a/libocfs2/feature_string.c b/libocfs2/feature_string.c
> index 9f395c6..83fec9a 100644
> --- a/libocfs2/feature_string.c
> +++ b/libocfs2/feature_string.c
> @@ -254,7 +254,7 @@ static struct feature_name ocfs2_feature_names[] = {
> .fn_flag = {0, OCFS2_FEATURE_INCOMPAT_XATTR, 0},
> },
> {
> - .fn_name = "IndexedDirs",
> + .fn_name = "indexed-dirs",
> .fn_flag = {0, OCFS2_FEATURE_INCOMPAT_INDEXED_DIRS, 0},
> },
> {
>
> By this modification, we can have unified feature string name for 1) debugfs.ocfs2 output 2) .fn_name string of
> ocfs2_feature_names[] 3) fs_features parameter in mkfs.ocfs2 and tunefs.ocfs2.
looks good to me.
Regards,
Tao
More information about the Ocfs2-tools-devel
mailing list