[Ocfs2-tools-devel] [PATCH 01/15] dx_dirs v11: Add library support for directory indexing
Coly Li
coly.li at suse.de
Sun Apr 25 11:03:09 PDT 2010
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 ?
>> +
>> + blkno++;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +extern errcode_t ocfs2_dx_entries_iterate(ocfs2_filesys *fs,
[snip]
> No need to set ret here.
[snip]
>> +extern errcode_t ocfs2_dx_frees_iterate(ocfs2_filesys *fs,
[snip]
>> + if (!S_ISDIR(dir->i_mode) || !(ocfs2_dir_indexed(dir))) {
>> + ret = 0;
> No need to set 'ret' here.
>> + goto out;
>> + }
>> +
>> + if (dx_root->dr_flags & OCFS2_DX_FLAG_INLINE) {
>> + ret = 0;
> ditto.
Thanks, removed all unnecessary return value assignment.
[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 ?
>> + {
>> + .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.
--
Coly Li
SuSE Labs
More information about the Ocfs2-tools-devel
mailing list