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

[Tao Ma]

Hi coly,
	the 2nd part of review for this really large patch. ;)

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>
> +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.
> +
> +	/* 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);
> +out:
> +	if (blk)
> +		ocfs2_free(&blk);
> +	return ret;
> +}
<snip>
> +/*
> + * 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)
> +{
> +	errcode_t ret = 0, err;
> +	uint64_t dr_blkno;
> +	char *dx_buf = NULL, *di_buf = NULL;
> +	struct ocfs2_dinode *di;
> +	struct ocfs2_dx_root_block *dx_root;
> +	struct dx_insert_ctxt ctxt;
> +	ocfs2_quota_hash *usrhash = NULL, *grphash = NULL;
> +	uint32_t uid, gid;
> +	long long change;
> +
> +	ret = ocfs2_load_fs_quota_info(fs);
> +	if (ret)
> +		goto out;
> +
> +	ret = ocfs2_init_quota_change(fs, &usrhash, &grphash);
> +	if (ret)
> +		goto out;
> +
> +	ret = ocfs2_malloc_block(fs->fs_io, &di_buf);
> +	if (ret)
> +		goto out;
> +
> +	ret = ocfs2_read_inode(fs, dir, di_buf);
> +	if (ret)
> +		goto out;
> +	di = (struct ocfs2_dinode *)di_buf;
> +
> +	if ((ocfs2_dir_indexed(di)) ||
> +	    (di->i_dyn_features & OCFS2_INLINE_DATA_FL))
> +		goto out;
> +
> +	ret = ocfs2_new_dx_root(fs, di, &dr_blkno);
> +	if (ret)
> +		goto out;
> +
> +	ret = ocfs2_malloc_block(fs->fs_io, &dx_buf);
> +	if (ret)
> +		goto out;
> +
> +	ret = ocfs2_read_dx_root(fs, dr_blkno, dx_buf);
> +	if (ret)
> +		goto out;
> +	dx_root = (struct ocfs2_dx_root_block *)dx_buf;
> +
> +	ret = ocfs2_init_dir_trailers(fs, di, dx_root);
> +	if (ret)
> +		goto out;
> +
> +	dx_root->dr_dir_blkno = di->i_blkno;
> +	dx_root->dr_num_entries = 0;
> +	dx_root->dr_entries.de_count = ocfs2_dx_entries_per_root(fs->fs_blocksize);
> +
> +	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.
> +
> +	ctxt.dir_blkno = dir;
> +	ctxt.dx_root_blkno = dr_blkno;
> +	ctxt.fs = fs;
> +	ret = ocfs2_dir_iterate(fs, dir, 0, NULL,
> +				ocfs2_dx_dir_insert,  &ctxt);
> +
> +	/* check quota for dx_leaf */
> +	ret = ocfs2_read_dx_root(fs, dr_blkno, dx_buf);
> +	if (ret)
> +		goto out;
> +	ret = ocfs2_read_inode(fs, dir, di_buf);
> +	if (ret)
> +		goto out;
> +
> +	change = ocfs2_clusters_to_bytes(fs,
> +				dx_root->dr_clusters);
> +	uid = di->i_uid;
> +	gid = di->i_gid;
> +
> +	ret = ocfs2_apply_quota_change(fs, usrhash, grphash,
> +					uid, gid, change, 0);
> +	if (ret) {
> +		/* exceed quota, truncate the indexed tree */
> +		ret = ocfs2_dx_dir_truncate(fs, dir);
> +	}
> +
> +out:
> +	err = ocfs2_finish_quota_change(fs, usrhash, grphash);
> +	if (!ret)
> +		ret = err;
> +
> +	if (di_buf)
> +		ocfs2_free(&di_buf);
> +	if (dx_buf)
> +		ocfs2_free(&dx_buf);
> +
> +	return ret;
> +}
<snip>
> -static void ocfs2_swap_dx_entry_to_cpu(struct ocfs2_dx_entry *dx_entry)
> +static void ocfs2_swap_dx_entry(struct ocfs2_dx_entry *dx_entry)
>  {
> -	if (cpu_is_little_endian)
> -		return;
> -
>  	dx_entry->dx_major_hash		= bswap_32(dx_entry->dx_major_hash);
>  	dx_entry->dx_minor_hash		= bswap_32(dx_entry->dx_minor_hash);
>  	dx_entry->dx_dirent_blk		= bswap_64(dx_entry->dx_dirent_blk);
>  }
>  
> -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 
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.
>  
>  static void ocfs2_swap_dx_root_to_cpu(ocfs2_filesys *fs,

Regards,
Tao