[Ocfs2-tools-devel] [PATCH 5/5] Add "clear sparse" feature for ocfs2-tools, take 2

[Mark Fasheh]

On Thu, Oct 25, 2007 at 03:38:14PM +0800, tao.ma wrote:

> @@ -500,3 +553,254 @@ errcode_t set_sparse_file_flag(ocfs2_filesys *fs, char *progname)
>  bail:
>  	return ret;
>  }
> +
> +static void add_hole(void *priv_data, uint32_t hole_start, uint32_t hole_len)
> +{
> +	struct hole_list *hole = NULL;
> +	struct clear_hole_unwritten_ctxt *ctxt =
> +			(struct clear_hole_unwritten_ctxt *)priv_data;
> +
> +	ocfs2_malloc0(sizeof(struct hole_list), &hole);
> +	assert(hole);

As unlikely as they might be, you need to handle memory allocation failures
in a graceful fasion...

You fail to handle this in add_unwritten too.

If it's a major pain to return an int from those function callbacks, just
stick a status member on your ctxt and check that after the call from
iterate_file().


> +static errcode_t calc_hole_and_unwritten(ocfs2_filesys *fs,
> +					 struct ocfs2_dinode *di)
> +{
> +	errcode_t ret = 0;
> +	uint64_t blk_num;
> +	uint32_t clusters;
> +	struct sparse_file *file = NULL, *old_files = NULL;
> +	uint32_t recs_per_eb = ocfs2_extent_recs_per_eb(fs->fs_blocksize);
> +
> +	assert(S_ISREG(di->i_mode));
> +
> +	ret = ocfs2_malloc0(sizeof(struct sparse_file), &file);
> +	if (ret)
> +		goto bail;
> +
> +	memset(file, 0, sizeof(struct sparse_file));

Doesn't ocfs2_malloc0() handle the memset for you?


> +	file->blkno = di->i_blkno;
> +	old_files = clear_ctxt.files;
> +	clear_ctxt.files = file;
> +	ret = iterate_file(fs, di, add_hole, add_unwritten, &clear_ctxt);
> +	if (ret) {
> +		clear_ctxt.files = old_files;
> +		goto bail;
> +	}

Is it possible to optimize by freeing your 'file' structure for any inodes
which don't have holes/unwritten extents? I'm a little bit concerned with
allocating structure for every inode in the file system and keeping those in
memory for the duration of the operation. In general, an inode with holes is
a rare case so it'd be far less likely that we'd run into problems if it was
only sparse inodes which were remembered.


> +	/*
> +	 * Iterate all the unwritten extents, empty its content and
> +	 * mark it written.
> +	 */
> +	file = clear_ctxt.files;
> +	while (file) {
> +		if (!file->unwritten)
> +			goto next_file;
> +
> +		ret = ocfs2_read_inode(fs, file->blkno, buf);
> +		if (ret)
> +			goto bail;
> +		di = (struct ocfs2_dinode *)buf;
> +
> +		unwritten = file->unwritten;
> +		while (unwritten) {
> +			start = unwritten->start;
> +			len = unwritten->len;
> +			p_start = unwritten->p_start;
> +
> +			ret = empty_clusters(fs, p_start, len);
> +			if (ret)
> +				goto bail;
> +
> +			ret = ocfs2_mark_extent_written(fs, di, start,
> +							len, p_start);

You're missing a return code check here.
	--Mark

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