[Ocfs2-tools-devel] [PATCH 2/3] fsck.ocfs2: Add extent list check for discontig bg.

[Tao Ma]

Hi Joel,

On 07/22/2010 10:28 AM, Joel Becker wrote:
> On Wed, Jul 21, 2010 at 03:31:37PM +0800, Tao Ma wrote:
<snip>
>> +	for (i = 0; i<  next_free; i++) {
>> +		rec =&bg->bg_list.l_recs[i];
>> +
>> +		/*
>> +		 * We treat e_blkno = 0 and e_leaf_cluster = 0 as the
>> +		 * end of the extent list so that we can find the proper
>> +		 * l_next_free_rec.
>> +		 */
>> +		if (!rec->e_blkno&&  !rec->e_leaf_clusters)
>> +			break;
>> +
>> +		if (ocfs2_block_out_of_range(ost->ost_fs, rec->e_blkno)) {
>
> 	Shouldn't we be checking the last block of the extent, not the
> first one?
I am just worried about the case of e_blkno = 0 while the end may be 
valid. but you are also right, so we need to check both e_blkno and the 
last block of the extent.
>
>> +			if (prompt(ost, PY, PR_DISCONTIG_BG_REC_RANGE,
>> +				   "Discontig block group %"PRIu64" in chain "
>> +				   "%d at inode %"PRIu64" contains a reference "
>> +				   "block %"PRIu64" which is out "
>> +				   "of range. Drop this group?",
>
> 				"Discontiguous block group %"PRIu64" in"
> 				   "chain %d of inode %"PRIu64" claims "
> 				   "clusters beyond the end of the volume. "
> 				   "Drop this group?"
>
>> +				   blkno, bg->bg_chain,
>> +				   (uint64_t)bg->bg_parent_dinode,
>> +				   (uint64_t)rec->e_blkno))  {
>> +				*clear_ref = 1;
>> +			}
>> +			goto out;
>> +		}
>> +
>> +		if (rec->e_leaf_clusters>  cpg) {
>> +			if (fix_leaf_clusters) {
>> +				if (prompt(ost, PY,
>> +					   PR_DISCONTIG_BG_CORRUPT_LEAFS,
>
> 					PR_DISCONTIG_BG_CORRUPT_LEAVES,
>
>> +					   "Discontig block group %"PRIu64" "
>> +					   "in chain %d at inode %"PRIu64" "
>> +					   "have too many errors in "
>> +					   "e_leaf_clusters. "
>> +					   "Drop this group?",
>> +					   blkno, bg->bg_chain,
>> +					   (uint64_t)bg->bg_parent_dinode))
>
> 					"Discontiguous block group %"PRIu64
> 					   " in chain %d of inode %"PRIu64" "
> 					   "has errors in more than one extent "
> 					   "record.  Record %d claims %u "
> 					   "clusters and record %d claims %u "
> 					   "clusters, but a group does not "
> 					   "contain more than %u clusters. "
> 					   "Drop this group?", blkno,
> 					   bg->bg_chain,
> 					   (uint64_t)bg->bg_parent_dinode,
> 					   fix_pos,
> 					   bg->bg_list.l_recs[fix_pos].e_leaf_clusters;
> 					   i, rec->e_leaf_clusters, cpg))
>
>> +					*clear_ref = 1;
>> +				goto out;
>> +			}
>> +			fix_pos = i;
>> +			fix_leaf_clusters = 1;
>> +		} else
>> +			total_clusters += rec->e_leaf_clusters;
>
> 		} else if ((total_clusters + rec->e_leaf_clusters)>  cpg) {
> 			if (total_clusters == cpg) {
> 				Ask to truncate l_next_free_rec at i.
> 			} else {
> 				Ask to truncate rec->e_leaf_clusters at
> 				cpg -. total_clusters.  Or perhaps drop
> 				the group if we don't think that is safe
> 				to do.
I think we should drop the group here.
> 			}
> 		} else
> 			total_clusters += rec->e_leaf_clusters;
>
>> +	}
>> +
>> +	if (total_clusters>  cpg) {
>> +		if (prompt(ost, PY, PR_DISCONTIG_BG_CLUSTERS,
>> +		    "Discontig Group descriptor at block %"PRIu64" has "
>> +		    "a extent list which has %u clusters allocated, which "
>> +		    "is greater than %u. Drop the group?",
>> +		    blkno, total_clusters, cpg))
>> +			*clear_ref = 1;
>> +		goto out;
>> +	}
>
> 	If you check total_clusters as I describe above, you can drop
> this check, right?
yes.

Thanks for the review. I will send a v2 based on it.

Regards,
Tao