[Ocfs2-devel] [PATCH] ocfs2: Make xattr reflink work with new local alloc reservation.
Tao Ma
tao.ma at oracle.com
Thu Jul 8 21:03:15 PDT 2010
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
More information about the Ocfs2-devel
mailing list