[Ocfs2-tools-devel] [PATCH 2/4] defrag.ocfs2: Pass 1: Defrag individual files and directories

[Joel Becker]

On Tue, May 11, 2010 at 11:02:34PM -0500, Goldwyn Rodrigues wrote:
> Defragging file -
> Allocate the maximum possible extent and copy the file into the
> newly allocated extent. If the allocated extent is smaller than
> the extent read from the file, write the collected data, and
> re-use the file extent. At any point, if the number of extents
> in the new inode increases than in the existing inode, abort.
> 
> Defragging directory -
> Allocate an extent, and copy dirents to the new extent, skipping
> holes and empty dirents. For each dirent, the dirent length
> is recalculated to optimize on space.
> 
> TODO: Find the optimum extent size, perhaps by reading a
> group_desc
> 
> Signed-off-by: Goldwyn Rodrigues <rgoldwyn at suse.de>

	Can you break this up into three patches?  The first adds
defrag.c, which has a main function that parses options but doesn't
actually do anything.  The next patch adds file defrag, and the third
does dir defrag.
	It's just that this is a lot of code to review in one email.
It's easier if broken up, when there is a logical boundary to break it
up.  I'm going to review in multiple emails to take each piece of
functionality one at a time.

> diff --git a/defrag.ocfs2/defrag.c b/defrag.ocfs2/defrag.c
> new file mode 100644
> index 0000000..65c1a13
> --- /dev/null
> +++ b/defrag.ocfs2/defrag.c
> @@ -0,0 +1,401 @@
> +/* -*- mode: c; c-basic-offset: 8; -*-
> +   * vim: noexpandtab sw=8 ts=8 sts=0:
> +   *
> +   * defrag.c
> +   *
> +   * Copyright (C) 2010 Oracle. All rights reserved.

	Make sure you set the copyright to the company that employs you.
Oracle can't claim ownership on code not written by an Oracle employee.
Well, at least not without copyright assignment, and we don't really
need to involve the lawyers, do we? ;-)

> +static errcode_t check_superblock(struct defrag_state *dst)
> +{
> +	struct ocfs2_dinode *di = dst->dst_fs->fs_super;
> +	struct ocfs2_super_block *sb = OCFS2_RAW_SB(di);
> +	errcode_t ret = 0;
> +
> +	if (sb->s_max_slots == 0) {
> +		printf("The superblock max_slots field is set to 0.\n");
> +		ret = OCFS2_ET_CORRUPT_SUPERBLOCK;
> +	}
> +	dst->fs_generation = di->i_fs_generation;
> +
> +	return ret;
> +}

	I don't get this.  This should never happen, so why are you
checking for it?

> +static int fs_clean(struct ocfs2_super_block *sb, char *filename)
> +{
> +	time_t now = time(NULL);
> +	time_t next = sb->s_lastcheck + sb->s_checkinterval;
> +
> +	if (sb->s_checkinterval > 0 && now >= next) {
> +		unsigned scaled_time;
> +		char *scaled_units;
> +
> +		scale_time(now - sb->s_lastcheck, &scaled_time, &scaled_units);
> +		fprintf(stderr, "Filesystem has gone %u %s without"
> +				"being checked\n", scaled_time, scaled_units);
> +		return 0;
> +	}
> +
> +	if (sb->s_mnt_count > 0) {
> +		fprintf(stderr, "Filesystem mounted for %u mounts "
> +				"without fsck. ", sb->s_mnt_count);
> +		return 0;
> +	}
> +
> +	return 1;
> +}

	And why are you detecting and printing the fsck counts?

> +static errcode_t recover_cluster_info(struct defrag_state *dst)
> +{
> +	errcode_t ret = 0;
> +	struct o2cb_cluster_desc disk = {NULL, NULL}, running = {NULL, NULL};
> +
> +	ret = o2cb_running_cluster_desc(&running);
> +	if (ret)
> +		goto bail;
> +
> +	ret = ocfs2_fill_cluster_desc(dst->dst_fs, &disk);
> +	if (ret)
> +		goto bail;
> +
> +	/*
> +	 * If the disk matches the running cluster, there is nothing we
> +	 * can fix.
> +	 */
> +	if ((!running.c_stack && !disk.c_stack) ||
> +			(running.c_stack && running.c_cluster &&
> +			 disk.c_stack && disk.c_cluster &&
> +			 !strcmp(running.c_stack, disk.c_stack) &&
> +			 !strcmp(running.c_cluster, disk.c_cluster)))
> +		goto bail;
> +
> +bail:
> +	o2cb_free_cluster_desc(&running);
> +	o2cb_free_cluster_desc(&disk);
> +
> +	return ret;
> +}

	I don't get this function either.  It doesn't do anything.
But...

> +	if (!dst->skip_o2cb && !ocfs2_mount_local(dst->dst_fs)) {
> +		ret = o2cb_init();
> +		if (ret) {
> +			com_err(whoami, ret, "while initializing the cluster");
> +			goto close;
> +		}
> +
> +		block_signals(SIG_BLOCK);
> +		ret = ocfs2_initialize_dlm(dst->dst_fs, whoami);
> +		if (ret == O2CB_ET_INVALID_STACK_NAME) {
> +			block_signals(SIG_UNBLOCK);
> +			ret = recover_cluster_info(dst);

	No, no, no, no, no.  If the cluster stack is invalid, you just
error.  They need to run 'tunfes.ocfs2 --update-cluster-stack' if they
want to adjust the disk's stack information.  defrag.ocfs2 shouldn't be
touching this stuff.

Joel

-- 

Viro's Razor:
	Any race condition, no matter how unlikely, will occur just
	often enough to bite you.

Joel Becker
Consulting Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127