[Ocfs2-tools-devel] [PATCH 2/5] Add "list-sparse" option in tunefs.ocfs2, take 2

[Mark Fasheh]

This is looking much better... A few comments below.

On Thu, Oct 25, 2007 at 03:36:25PM +0800, tao.ma wrote:
> +/*
> + * Iterate a file and call "func" when we meet with a hole.
> + */
> +static errcode_t iterate_file(ocfs2_filesys *fs,

A better name might be "iterate_file_holes"


> +			      struct ocfs2_dinode *di,
> +			      void (*func)(void *priv_data,
> +					   uint32_t hole_start,
> +					   uint32_t hole_len),
> +			      void *priv_data)
> +{
> +	errcode_t ret;
> +	uint32_t clusters, v_cluster = 0, p_cluster, num_clusters;
> +	uint16_t extent_flags;
> +	ocfs2_cached_inode *ci = NULL;
> +
> +	clusters = (di->i_size + fs->fs_clustersize -1 ) /
> +			fs->fs_clustersize;
> +
> +	ret = ocfs2_read_cached_inode(fs, di->i_blkno, &ci);
> +	if (ret)
> +		goto bail;
> +
> +	while (v_cluster < clusters) {
> +		ret = ocfs2_get_clusters(ci,
> +					 v_cluster, &p_cluster,
> +					 &num_clusters, &extent_flags);
> +		if (ret)
> +			goto bail;
> +
> +		/*
> +		 * If the tail of the file is a hole, let the
> +		 * hole length only cover the last i_size.
> +		 */
> +		if (v_cluster + num_clusters == UINT32_MAX)
> +			num_clusters = clusters - v_cluster;

I think you want: 

	if (!p_cluster && (v_cluster + num_clusters == UINT32_MAX))
		num_clusters = cluster - v_cluster;

Otherwise, we might concatenate the last extent length in a very large file.

Really, we could then just combine both the if statements into something
like this:

	if (!p_cluster) {
		if (v_cluster + num_clusters == UINT32_MAX)
			num_clusters = cluster - v_cluster;

		if (func)
			func(priv_data, v_cluster, num_clusters);
	}


> +		if (!p_cluster && func)
> +			func(priv_data, v_cluster, num_clusters);
> +
> +		v_cluster += num_clusters;
> +	}
> +
> +bail:
> +	if (ci)
> +		ocfs2_free_cached_inode(fs, ci);
> +	return ret;
> +}
> +
> +/*
> + * For a regular file, we will iterate it and calculate all the
> + * hole in it and store the information in ctxt->file_hole_len.
> + *
> + * for the file which has i_links_count > 1, we only iterate it
> + * when we meet it the first time and record it into multi_link_file
> + * tree, so the next time we will just search the tree and set
> + * file_hole_len accordingly.
> + */

Oh, great - I like that we're properly counting hard linked files.


> +static errcode_t list_sparse_file(ocfs2_filesys *fs,
> +				  struct ocfs2_dinode *di,
> +				  struct list_ctxt *ctxt)
> +{
> +	errcode_t ret = 0;
> +	struct multi_link_file *file = NULL;
> +
> +	assert(S_ISREG(di->i_mode));
> +
> +	ctxt->file_hole_len = 0;
> +
> +	if (di->i_links_count > 1) {
> +		file = multi_link_file_lookup(ctxt, di->i_blkno);
> +
> +		if (file) {
> +			ctxt->file_hole_len = file->clusters;
> +			ctxt->duplicated = 1;
> +			goto print;

Shouldn't we skip printing the same inode number twice?


> +static int list_sparse_func(struct ocfs2_dir_entry *dirent,
> +			    int offset, int blocksize,
> +			    char *buf, void *priv_data)
> +{
> +	errcode_t ret;
> +	char *di_buf = NULL;
> +	struct ocfs2_dinode *di = NULL;
> +	char file_name[OCFS2_MAX_FILENAME_LEN];
> +	int file_name_len = 0;
> +	struct list_ctxt *ctxt = (struct list_ctxt *)priv_data;
> +	ocfs2_filesys *fs = ctxt->fs;
> +
> +	ret = ocfs2_malloc_block(fs->fs_io, &di_buf);
> +	if (ret)
> +		goto bail;
> +
> +	ret = ocfs2_read_inode(fs, (uint64_t)dirent->inode, di_buf);
> +	if (ret)
> +		goto bail;
> +
> +	di = (struct ocfs2_dinode *)di_buf;
> +
> +	/* currently, we only handle directories and regular files. */
> +	if (!S_ISDIR(di->i_mode) && !S_ISREG(di->i_mode))
> +		return 0;
> +
> +	strcpy(file_name, ctxt->file_name);
> +	file_name_len = ctxt->file_name_len;
> +
> +	if (dirent->name_len + ctxt->file_name_len + 1 >=
> +		OCFS2_MAX_FILENAME_LEN) {
> +		goto bail;
> +	}

Can you explain this check to me? Should we be really comparing against
PATH_MAX instead?


> +/*
> + * list_sparse will iterate from "/" and all the orphan_dirs recursively
> + * and print out all the hole information on the screen.
> + *
> + * We will use ocfs2_dir_iterate to iterate from the very beginning and
> + * tunefs_iterate_func will handle every dir entry.
> + */
> +errcode_t list_sparse(ocfs2_filesys *fs)
> +{
> +	int i;
> +	errcode_t ret;
> +	uint64_t blkno;
> +	char file_name[OCFS2_MAX_FILENAME_LEN];
> +	struct list_ctxt ctxt;
> +	struct ocfs2_super_block *sb = OCFS2_RAW_SB(fs->fs_super);
> +	uint32_t total_holes = 0, free_clusters = 0;
> +
> +	/*
> +	 * We will use block cache in io, so that the extent block
> +	 * information can be cached to speed up our extent iteration.
> +	 * If io_init_cache failed, we will go on the list work without
> +	 * the io_cache, so there is no check here.
> +	 */
> +	io_init_cache(fs->fs_io, ocfs2_extent_recs_per_eb(fs->fs_blocksize));

I think it's best if we just do this up in the main body of tunefs.c (after
the cluster checks) so that all operations get the benefit of caching. It
should also be in a seperate patch.
	--Mark

--
Mark Fasheh
Senior Software Developer, Oracle
mark.fasheh at oracle.com