[Ocfs2-tools-devel] [PATCH 39/50] fsck.ocfs2: Check refcount of clusters.

[Joel Becker]

On Mon, Jan 11, 2010 at 11:31:25PM +0800, Tao Ma wrote:
> +		assert(extent->p_cpos >= cpos);

	There are many asserts throughout this code.  I think asserting
to trust your code is good, but I want to be sure they are true asserts.
That is, these conditions can't happen because of corrupt disk data.
They only happen if you messed up building data structures in earlier
passes.  I think that's the case, but I just wanted to make sure ;-)

> +static errcode_t o2fsck_check_clusters_in_refcount(o2fsck_state *ost,
> +						   struct refcount_tree *tree,
> +						   uint64_t p_cpos,
> +						   uint32_t clusters,
> +						   uint32_t refcount)
> +{

	Huge function, lots of gotos.  Is there any way you can make
some sub-functions so that this is much more readable?  Even just break
some of the checks out

> +	errcode_t ret = 0;
> +	uint32_t rec_len;
> +	struct ocfs2_refcount_rec tmp_rec, *rec = &tree->rec;
> +	uint64_t cpos, range;
> +	unsigned int len;
> +	int index;
> +
> +	if (!clusters)
> +		return 0;
> +
> +	/*
> +	 * the previous check ended at tree->p_cend, and now we get
> +	 * p_cpos, so any refcount record between p_cend and p_cpos
> +	 * should be considered as redundant.
> +	 */
> +	cpos = tree->p_cend;
> +	range = p_cpos - cpos;
> +	while (range) {
> +		len = range > UINT_MAX ? UINT_MAX : range;
> +
> +		ret = ocfs2_get_refcount_rec(ost->ost_fs, tree->root_buf,
> +				     cpos, len, &tmp_rec,
> +				     &index, tree->leaf_buf);
> +		if (ret) {
> +			com_err(whoami, ret, "while getting refcount rec at "
> +				"%"PRIu64" in tree %"PRIu64,
> +				cpos, tree->rf_blkno);
> +			goto out;
> +		}
> +
> +		if ((tmp_rec.r_refcount || tmp_rec.r_clusters != len) &&
> +		    prompt(ost, PY, PR_REFCOUNT_REC_REDUNDANT,
> +			   "refcount records among clusters (%"PRIu64
> +			   ", %u) are found with no physical clusters "
> +			   "corresponding to them. Remove them?", cpos, len)) {
> +			ret = o2fsck_refcount_punch_hole(ost, tree, cpos, len);
> +			if (ret) {
> +				com_err(whoami, ret, "while punching "
> +					"hole in (%"PRIu64", %u) in refcount "
> +					"tree %"PRIu64,
> +					cpos, len, tree->rf_blkno);
> +				goto out;
> +			}
> +
> +			/*
> +			 * We have punch a hole at (cpos, len),
> +			 * so fix the recorded index and r_offset in the tree
> +			 * if we have read them.
> +			 */
> +			if (tree->index != -1) {
> +				if (cpos + len >=
> +				    rec->r_cpos + rec->r_clusters) {
> +					tree->r_offset = 0;
> +					tree->index = -1;
> +				} else
> +					tree->r_offset =
> +						cpos + len - rec->r_cpos;
> +			}
> +		}
> +
> +		cpos += len;
> +		range -= len;
> +	}

	Like, can this be a sub-function?

> +again:
> +	if (tree->index == -1) {
> +		ret = ocfs2_get_refcount_rec(ost->ost_fs, tree->root_buf,
> +					     p_cpos, clusters, rec,
> +					     &tree->index, tree->leaf_buf);
> +		if (ret) {
> +			com_err(whoami, ret, "while getting refcount rec at "
> +				"%"PRIu64" in tree %"PRIu64,
> +				p_cpos, tree->rf_blkno);
> +			goto out;
> +		}
> +	}
> +
> +	/*
> +	 * Actually ocfs2_get_refcount_rec will fake some refcount record
> +	 * in case it can't find p_cpos in the refcount tree. So we really
> +	 * shouldn't meet with a case rec->r_cpos + tree->r_offset > p_cpos.
> +	 * And as for rec->r_cpos + tree->r_offset < p_pcos, it should already
> +	 * be resolved by the previous punch_hole.
> +	 */
> +	assert(rec->r_cpos + tree->r_offset == p_cpos);
> +
> +	rec_len = ocfs2_min(p_cpos + clusters,
> +			    (uint64_t)rec->r_cpos + rec->r_clusters) - p_cpos;
> +	if (rec->r_refcount != refcount) {
> +		if (prompt(ost, PY, PR_REFCOUNT_COUNT_INVALID,
> +			   "clusters %"PRIu64 " with len %u have %u refcount "
> +			   "while there are %u files point to them. "
> +			   "Correct the refcount value?",
> +			   p_cpos, rec_len, rec->r_refcount, refcount)) {
> +			ret = o2fsck_change_refcount(ost, tree,
> +						     p_cpos, rec_len, refcount);
> +			if (ret) {
> +				com_err(whoami, ret, "while updating refcount "
> +					"%u at %"PRIu64" len %u in tree "
> +					"%"PRIu64, refcount, p_cpos,
> +					rec_len, tree->rf_blkno);
> +				goto out;
> +			}
> +		} else {
> +			/*
> +			 * XXX:
> +			 * Do we need to ask user for adding them to dup?
> +			 */
> +			o2fsck_mark_clusters_allocated(ost, p_cpos, rec_len);
> +			ret = o2fsck_refcount_punch_hole(ost, tree,
> +							 p_cpos, rec_len);
> +			if (ret) {
> +				com_err(whoami, ret, "while punching "
> +					"hole at %"PRIu64"in refcount "
> +					"tree %"PRIu64, p_cpos,
> +					tree->rf_blkno);
> +				goto out;
> +			}
> +
> +			ret = o2fsck_clear_refcount(ost, tree, p_cpos, rec_len);
> +			if (ret) {
> +				com_err(whoami, ret,
> +					"while clearing refcount for "
> +					"cluster %"PRIu64" len %u in %"PRIu64,
> +					p_cpos, rec_len, tree->rf_blkno);
> +				goto out;
> +			}
> +		}
> +	}

	And this if block?  You get the idea.

> +/*
> + * Given a refcount tree, check the refcounted clusters and their refcount.
> + */
> +static errcode_t o2fsck_check_refcount(o2fsck_state *ost,
> +				       struct refcount_tree *tree)
> +{

	And let's try the same with this big function.

Joel

-- 

Life's Little Instruction Book #232

	"Keep your promises."

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