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

[Tao Ma]

Joel Becker wrote:
> 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 ;-)
I will refactor the code so that it is more readable.

Regards,
Tao
> 
>> +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
>