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

[Tao Ma]

Hi Coly,

Coly Li wrote:
> This patch adds more library support for indexed dirs,
> - dx_root alloc/delete
> - dx_leaf alloc/delete
> - dx_root read/write
> - dx_leaf read/write
> - indexed tree insert/truncate
> - dx_root extent tree operations
> - indexed dir support in ocfs2_lookup_system_inode(),
>   ocfs2_init_dir(), ocfs2_link(), ocfs2_unlink(),
>   ocfs2_lookup().
> With this patch, indexed dirs support in fsck.ocfs2 is possible.
> 
> Signed-off-by: Coly Li <coly.li at suse.de>
> Cc: Mark Fasheh <mfasheh at suse.com>
> ---
<snip>
> +errcode_t ocfs2_new_dx_root(ocfs2_filesys *fs,
> +				struct ocfs2_dinode *di,
> +				uint64_t *dr_blkno)
> +{
> +	errcode_t ret;
> +	char *buf = NULL;
> +	uint64_t gd_blkno;
> +	struct ocfs2_dx_root_block *dx_root;
> +	int slot;
> +
> +	ret = ocfs2_malloc_block(fs->fs_io, &buf);
> +	if (ret)
> +		goto out;
> +
> +	slot = di->i_suballoc_slot;
> +	ret = ocfs2_load_allocator(fs, EXTENT_ALLOC_SYSTEM_INODE,
> +				slot, &fs->fs_eb_allocs[slot]);
why you want to use the suballoc_slot of the inode? the i_suballoc_slot 
means the slot for inode_alloc while you load the allocators from 
extent_alloc. I pointed it out because here is a bug. For "/", it is 
allocated from global_alloc, so i_suballoc_slot is INVALID_SLOT, so 
there is no corresponding extent alloc, and &fs->fs_eb_allocs[slot] will 
overflow.
> +	if (ret)
> +		goto out;
> +
> +	ret = ocfs2_chain_alloc_with_io(fs, fs->fs_eb_allocs[slot],
> +	    				&gd_blkno, dr_blkno);
> +	if (ret == OCFS2_ET_BIT_NOT_FOUND) {
> +		ret = ocfs2_chain_add_group(fs, fs->fs_eb_allocs[slot]);
> +		if (ret)
> +			goto out;
> +		ret = ocfs2_chain_alloc_with_io(fs, fs->fs_eb_allocs[slot],
> +						&gd_blkno, dr_blkno);
> +		if (ret)
> +			goto out;
> +	} else if (ret)
> +		goto out;
> +
> +	dx_root = (struct ocfs2_dx_root_block *)buf;
> +	init_dx_root(fs, dx_root, slot, gd_blkno, *dr_blkno);
> +
> +	ret = ocfs2_write_dx_root(fs, *dr_blkno, (char *)dx_root);
> +out:
> +	if (buf)
> +		ocfs2_free(&buf);
> +	return ret;
<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;
> +
> +	/* here we don't trust trailer, cannot use
> +	 * ocfs2_read_dir_block() */
> +	ret = ocfs2_read_blocks(fs, blkno, 1, blk);
> +	if (ret)
> +		goto out;
> +	ret = ocfs2_check_dir_trailer_space(fs, di, blkno, blk);
> +	if (ret)
> +		goto out;
> +	ocfs2_init_dir_trailer(fs, di, blkno, blk);
> +	max_rec_len = ocfs2_find_max_rec_len(fs, blk);
> +	trailer = ocfs2_dir_trailer_from_block(fs, blk);
> +	trailer->db_free_rec_len = max_rec_len;
> +
> +	if (max_rec_len) {
> +		trailer->db_free_next = dx_root->dr_free_blk;
> +		dx_root->dr_free_blk = blkno;
> +	}
> +
> +	/* comput trailer->db_check here, after writes out,
> +	 * trailer is trustable */
> +	ret = ocfs2_write_dir_block(fs, di, blkno, blk);
I am just curious about one thing. We have added the trailer to the end 
of the dir block. Do we need to update the rec_len of the last record of 
the dir block? Or am I missing something here?
> +static errcode_t ocfs2_read_dx_leaves(ocfs2_filesys *fs,
> +				uint64_t start,
> +				int num,
> +				struct ocfs2_dx_leaf **dx_leaves)
> +{
> +	errcode_t ret;
> +	int i;
> +	struct ocfs2_dx_leaf *dx_leaf;
> +	for (i = 0; i < num; i++) {
> +		assert(!dx_leaves[i]);
> +		ret = ocfs2_malloc_block(fs->fs_io, (char **)&dx_leaf);
> +		if (ret)
> +			goto bail;
> +		ret = ocfs2_read_dx_leaf(fs, start + i, (char *)dx_leaf);
> +		if (ret)
> +			goto bail;
> +		dx_leaves[i] = dx_leaf;
> +	}
> +	goto out;
> +
> +bail:
> +	for (; i >= 0; i--) {
> +		if (dx_leaves[i])
> +			ocfs2_free(&dx_leaves[i]);
We need to set dx_leaves[i] to NULL here. Otherwise the callers of 
ocfs2_read_dx_leaves will double free them. At least I see the caller 
ocfs2_dx_dir_rebalance call ocfs2_dx_dir_free_leaves which free 
dx_leaves[i] again.
> +	}
> +out:
> +	return ret;
> +}
> +

Regards,
Tao