[Ocfs2-devel] [PATCH] ocfs2: Make xattr reflink work with new local alloc reservation.

[Tao Ma <tao.ma@oracle.com>]

On 07/09/2010 03:42 AM, Joel Becker wrote:
> On Fri, Jun 18, 2010 at 11:02:51AM +0800, Tao Ma wrote:
>> +	num_buckets = le16_to_cpu(bucket_xh(args->old_bucket)->xh_num_buckets);
>> +	ocfs2_xattr_bucket_relse(args->old_bucket);
>> +
>> +	while (len&&  num_buckets) {
>> +		ret = ocfs2_claim_clusters(handle, data_ac,
>> +					   1,&p_cluster,&num_clusters);
>> +		if (ret) {
>> +			mlog_errno(ret);
>> +			goto out;
>> +		}
>> +
>> +		new_blkno = ocfs2_clusters_to_blocks(inode->i_sb, p_cluster);
>> +		reflink_buckets = num_buckets<  bpc * num_clusters ?
>> +				  num_buckets : bpc * num_clusters;
>
> 	I don't get this check.  In
> ocfs2_lock_reflink_xattr_rec_allocators(), we've reserved exactly enough
> clusters for the xattr rec.  ocfs2_claim_clusters() may return less than
> that, but in the optimal case it will return the entire len.  So...
> 	Ok, I think I get it.  I was trying to figure out if you were
> leaking clusters.  This check reflects that a cluster may contain
> multiple buckets, but not all buckets may be used yet.  Correct me if
> I'm wrong about the following limits:
>
> 	BUG_ON(num_buckets<  (bpc * (len - 1)));
> 	BUG_ON(num_buckets>  (bpc * len));
>
> You don't have to add those BUG_ON() calls to the code; I'm just
> confirming the range as I understand it.
yes, you are right. so num_buckets is originally set to the total bucket 
number for the clusters. But maybe there are several unused, and maybe 
we can't allocate enough contiguous clusters at a time. So we just CoW 
the minimal value of buckets.
> 	With that knowledge, your code is saying: "I have num_buckets to
> reflink, did I get enough clusters to do them all, or do I just use the
> entire range I got?"
> 	I think we should be more explicit.  Something like:
>
> 		reflink_buckets = num_buckets;
> 		if (reflink_buckets<  (bpc * num_clusters))
> 			reflink_buckets = bpc * num_clusters;
>
> This way speaks the intention.  I'd also be fine with:
>
> 		reflink_buckets = min(num_buckets, bpc * num_clusters);
I will use this line. thanks for the review.

Regards,
Tao