[Ocfs2-devel] [PATCH 0/2] ocfs2: make xattr work with new local alloc reservation.

[Ocfs2-devel] [PATCH 0/2] ocfs2: make xattr work with new local alloc reservation.

[Tao Ma]

Hi all,
	The old xattr codes deem that we can allocate enough contiguous 
clusters from the allocator, it works with the old local alloc 
allocator. But as Mark has added reservation code to ocfs2, now it isn't 
the good hypothesis any more. So these 2 patches try to let xattr work 
with it.

Regards,
Tao

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

[Tao Ma]

New local alloc reservation can give us less clusters than
what we want and ask us to restart the transaction, so let
ocfs2_xattr_extend_allocation restart the transaction in
this case.

Signed-off-by: Tao Ma <tao.ma@oracle.com>
---
 fs/ocfs2/xattr.c |   33 +++++++++++++++++++++++++--------
 1 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index e97b348..894734b 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -709,7 +709,7 @@ static int ocfs2_xattr_extend_allocation(struct inode *inode,
 					 struct ocfs2_xattr_value_buf *vb,
 					 struct ocfs2_xattr_set_ctxt *ctxt)
 {
-	int status = 0;
+	int status = 0, credits;
 	handle_t *handle = ctxt->handle;
 	enum ocfs2_alloc_restarted why;
 	u32 prev_clusters, logical_start = le32_to_cpu(vb->vb_xv->xr_clusters);
@@ -719,6 +719,7 @@ static int ocfs2_xattr_extend_allocation(struct inode *inode,
 
 	ocfs2_init_xattr_value_extent_tree(&et, INODE_CACHE(inode), vb);
 
+restarted_transaction:
 	status = vb->vb_access(handle, INODE_CACHE(inode), vb->vb_bh,
 			      OCFS2_JOURNAL_ACCESS_WRITE);
 	if (status < 0) {
@@ -735,8 +736,9 @@ static int ocfs2_xattr_extend_allocation(struct inode *inode,
 					     ctxt->data_ac,
 					     ctxt->meta_ac,
 					     &why);
-	if (status < 0) {
-		mlog_errno(status);
+	if ((status < 0) && (status != -EAGAIN)) {
+		if (status != -ENOSPC)
+			mlog_errno(status);
 		goto leave;
 	}
 
@@ -744,11 +746,26 @@ static int ocfs2_xattr_extend_allocation(struct inode *inode,
 
 	clusters_to_add -= le32_to_cpu(vb->vb_xv->xr_clusters) - prev_clusters;
 
-	/*
-	 * We should have already allocated enough space before the transaction,
-	 * so no need to restart.
-	 */
-	BUG_ON(why != RESTART_NONE || clusters_to_add);
+	if (why != RESTART_NONE && clusters_to_add) {
+		/*
+		 * We can only fail in case the alloc file doesn't give up
+		 * enough clusters.
+		 */
+		BUG_ON(why == RESTART_META);
+
+		mlog(0, "restarting xattr value extension for %u clusters,.\n",
+		     clusters_to_add);
+		credits = ocfs2_calc_extend_credits(inode->i_sb,
+						    &vb->vb_xv->xr_list,
+						    clusters_to_add);
+		status = ocfs2_extend_trans(handle, credits);
+		if (status < 0) {
+			status = -ENOMEM;
+			mlog_errno(status);
+			goto leave;
+		}
+		goto restarted_transaction;
+	}
 
 leave:
 
-- 
1.5.5

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

[Tao Ma]

The new reservation code in local alloc has add the limitation
that the caller should handle the case that the local alloc
doesn't give use enough contiguous clusters. It make the old
xattr reflink code broken.

So this patch udpate the xattr reflink code so that it can
handle the case that local alloc give us one cluster at a time.

Signed-off-by: Tao Ma <tao.ma@oralce.com>
---
 fs/ocfs2/xattr.c |  125 ++++++++++++++++++++++++++++++++++++++---------------
 1 files changed, 89 insertions(+), 36 deletions(-)

diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 894734b..bc52840 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -6805,16 +6805,15 @@ out:
 	return ret;
 }
 
-static int ocfs2_reflink_xattr_buckets(handle_t *handle,
+static int ocfs2_reflink_xattr_bucket(handle_t *handle,
 				u64 blkno, u64 new_blkno, u32 clusters,
+				u32 *cpos, int num_buckets,
 				struct ocfs2_alloc_context *meta_ac,
 				struct ocfs2_alloc_context *data_ac,
 				struct ocfs2_reflink_xattr_tree_args *args)
 {
 	int i, j, ret = 0;
 	struct super_block *sb = args->reflink->old_inode->i_sb;
-	u32 bpc = ocfs2_xattr_buckets_per_cluster(OCFS2_SB(sb));
-	u32 num_buckets = clusters * bpc;
 	int bpb = args->old_bucket->bu_blocks;
 	struct ocfs2_xattr_value_buf vb = {
 		.vb_access = ocfs2_journal_access,
@@ -6833,14 +6832,6 @@ static int ocfs2_reflink_xattr_buckets(handle_t *handle,
 			break;
 		}
 
-		/*
-		 * The real bucket num in this series of blocks is stored
-		 * in the 1st bucket.
-		 */
-		if (i == 0)
-			num_buckets = le16_to_cpu(
-				bucket_xh(args->old_bucket)->xh_num_buckets);
-
 		ret = ocfs2_xattr_bucket_journal_access(handle,
 						args->new_bucket,
 						OCFS2_JOURNAL_ACCESS_CREATE);
@@ -6854,6 +6845,18 @@ static int ocfs2_reflink_xattr_buckets(handle_t *handle,
 			       bucket_block(args->old_bucket, j),
 			       sb->s_blocksize);
 
+		/*
+		 * Record the start cpos so that we can use it to initialize
+		 * our xattr tree we also set the xh_num_bucket for the new
+		 * bucket.
+		 */
+		if (i == 0) {
+			*cpos = le32_to_cpu(bucket_xh(args->new_bucket)->
+					    xh_entries[0].xe_name_hash);
+			bucket_xh(args->new_bucket)->xh_num_buckets =
+				cpu_to_le16(num_buckets);
+		}
+
 		ocfs2_xattr_bucket_journal_dirty(handle, args->new_bucket);
 
 		ret = ocfs2_reflink_xattr_header(handle, args->reflink,
@@ -6883,6 +6886,7 @@ static int ocfs2_reflink_xattr_buckets(handle_t *handle,
 		}
 
 		ocfs2_xattr_bucket_journal_dirty(handle, args->new_bucket);
+
 		ocfs2_xattr_bucket_relse(args->old_bucket);
 		ocfs2_xattr_bucket_relse(args->new_bucket);
 	}
@@ -6891,6 +6895,74 @@ static int ocfs2_reflink_xattr_buckets(handle_t *handle,
 	ocfs2_xattr_bucket_relse(args->new_bucket);
 	return ret;
 }
+
+static int ocfs2_reflink_xattr_buckets(handle_t *handle,
+				struct inode *inode,
+				struct ocfs2_reflink_xattr_tree_args *args,
+				struct ocfs2_extent_tree *et,
+				struct ocfs2_alloc_context *meta_ac,
+				struct ocfs2_alloc_context *data_ac,
+				u64 blkno, u32 cpos, u32 len)
+{
+	int ret, num_buckets, reflink_buckets, first_inserted = 0;
+	u32 p_cluster, num_clusters, reflink_cpos = 0;
+	u64 new_blkno;
+	int bpc = ocfs2_xattr_buckets_per_cluster(OCFS2_SB(inode->i_sb));
+
+	ret = ocfs2_read_xattr_bucket(args->old_bucket, blkno);
+	if (ret) {
+		mlog_errno(ret);
+		goto out;
+	}
+	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;
+
+		ret = ocfs2_reflink_xattr_bucket(handle, blkno,
+						 new_blkno, num_clusters,
+						 &reflink_cpos, reflink_buckets,
+						 meta_ac, data_ac, args);
+		if (ret) {
+			mlog_errno(ret);
+			goto out;
+		}
+
+		/*
+		 * For the 1st allocated cluster, we make it use the same cpos
+		 * so that the xattr tree looks the same as the original one
+		 * in the most case.
+		 */
+		if (!first_inserted) {
+			reflink_cpos = cpos;
+			first_inserted = 1;
+		}
+		ret = ocfs2_insert_extent(handle, et, reflink_cpos, new_blkno,
+					  num_clusters, 0, meta_ac);
+		if (ret)
+			mlog_errno(ret);
+
+		mlog(0, "insert new xattr extent rec start %llu len %u to %u\n",
+		     (unsigned long long)new_blkno, num_clusters, reflink_cpos);
+
+		len -= num_clusters;
+		blkno += ocfs2_clusters_to_blocks(inode->i_sb, num_clusters);
+		num_buckets -= reflink_buckets;
+	}
+out:
+	return ret;
+}
+
 /*
  * Create the same xattr extent record in the new inode's xattr tree.
  */
@@ -6902,8 +6974,6 @@ static int ocfs2_reflink_xattr_rec(struct inode *inode,
 				   void *para)
 {
 	int ret, credits = 0;
-	u32 p_cluster, num_clusters;
-	u64 new_blkno;
 	handle_t *handle;
 	struct ocfs2_reflink_xattr_tree_args *args =
 			(struct ocfs2_reflink_xattr_tree_args *)para;
@@ -6912,6 +6982,9 @@ static int ocfs2_reflink_xattr_rec(struct inode *inode,
 	struct ocfs2_alloc_context *data_ac = NULL;
 	struct ocfs2_extent_tree et;
 
+	mlog(0, "reflink xattr buckets %llu len %u\n",
+	     (unsigned long long)blkno, len);
+
 	ocfs2_init_xattr_tree_extent_tree(&et,
 					  INODE_CACHE(args->reflink->new_inode),
 					  args->new_blk_bh);
@@ -6931,32 +7004,12 @@ static int ocfs2_reflink_xattr_rec(struct inode *inode,
 		goto out;
 	}
 
-	ret = ocfs2_claim_clusters(handle, data_ac,
-				   len, &p_cluster, &num_clusters);
-	if (ret) {
-		mlog_errno(ret);
-		goto out_commit;
-	}
-
-	new_blkno = ocfs2_clusters_to_blocks(osb->sb, p_cluster);
-
-	mlog(0, "reflink xattr buckets %llu to %llu, len %u\n",
-	     (unsigned long long)blkno, (unsigned long long)new_blkno, len);
-	ret = ocfs2_reflink_xattr_buckets(handle, blkno, new_blkno, len,
-					  meta_ac, data_ac, args);
-	if (ret) {
-		mlog_errno(ret);
-		goto out_commit;
-	}
-
-	mlog(0, "insert new xattr extent rec start %llu len %u to %u\n",
-	     (unsigned long long)new_blkno, len, cpos);
-	ret = ocfs2_insert_extent(handle, &et, cpos, new_blkno,
-				  len, 0, meta_ac);
+	ret = ocfs2_reflink_xattr_buckets(handle, inode, args, &et,
+					  meta_ac, data_ac,
+					  blkno, cpos, len);
 	if (ret)
 		mlog_errno(ret);
 
-out_commit:
 	ocfs2_commit_trans(osb, handle);
 
 out:
-- 
1.5.5

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

[Tao Ma]

On 06/18/2010 11:02 AM, Tao Ma wrote:
> The new reservation code in local alloc has add the limitation
> that the caller should handle the case that the local alloc
> doesn't give use enough contiguous clusters. It make the old
> xattr reflink code broken.
> 
> So this patch udpate the xattr reflink code so that it can
> handle the case that local alloc give us one cluster at a time.
> 
> Signed-off-by: Tao Ma<tao.ma@oralce.com>
oh, I used the wrong sob here. ;) Here is the updated one.

Regards,
Tao

From 5b714e5a16c6ff912fcd720b916d4324f6151d16 Mon Sep 17 00:00:00 2001
From: Tao Ma <tao.ma@oracle.com>
Date: Fri, 18 Jun 2010 10:38:48 +0800
Subject: [PATCH v2] ocfs2: Make xattr reflink work with new local alloc reservation.

The new reservation code in local alloc has add the limitation
that the caller should handle the case that the local alloc
doesn't give use enough contiguous clusters. It make the old
xattr reflink code broken.

So this patch udpate the xattr reflink code so that it can
handle the case that local alloc give us one cluster at a time.

Signed-off-by: Tao Ma <tao.ma@oracle.com>
---
 fs/ocfs2/xattr.c |  125 ++++++++++++++++++++++++++++++++++++++---------------
 1 files changed, 89 insertions(+), 36 deletions(-)

diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 894734b..bc52840 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -6805,16 +6805,15 @@ out:
 	return ret;
 }
 
-static int ocfs2_reflink_xattr_buckets(handle_t *handle,
+static int ocfs2_reflink_xattr_bucket(handle_t *handle,
 				u64 blkno, u64 new_blkno, u32 clusters,
+				u32 *cpos, int num_buckets,
 				struct ocfs2_alloc_context *meta_ac,
 				struct ocfs2_alloc_context *data_ac,
 				struct ocfs2_reflink_xattr_tree_args *args)
 {
 	int i, j, ret = 0;
 	struct super_block *sb = args->reflink->old_inode->i_sb;
-	u32 bpc = ocfs2_xattr_buckets_per_cluster(OCFS2_SB(sb));
-	u32 num_buckets = clusters * bpc;
 	int bpb = args->old_bucket->bu_blocks;
 	struct ocfs2_xattr_value_buf vb = {
 		.vb_access = ocfs2_journal_access,
@@ -6833,14 +6832,6 @@ static int ocfs2_reflink_xattr_buckets(handle_t *handle,
 			break;
 		}
 
-		/*
-		 * The real bucket num in this series of blocks is stored
-		 * in the 1st bucket.
-		 */
-		if (i == 0)
-			num_buckets = le16_to_cpu(
-				bucket_xh(args->old_bucket)->xh_num_buckets);
-
 		ret = ocfs2_xattr_bucket_journal_access(handle,
 						args->new_bucket,
 						OCFS2_JOURNAL_ACCESS_CREATE);
@@ -6854,6 +6845,18 @@ static int ocfs2_reflink_xattr_buckets(handle_t *handle,
 			       bucket_block(args->old_bucket, j),
 			       sb->s_blocksize);
 
+		/*
+		 * Record the start cpos so that we can use it to initialize
+		 * our xattr tree we also set the xh_num_bucket for the new
+		 * bucket.
+		 */
+		if (i == 0) {
+			*cpos = le32_to_cpu(bucket_xh(args->new_bucket)->
+					    xh_entries[0].xe_name_hash);
+			bucket_xh(args->new_bucket)->xh_num_buckets =
+				cpu_to_le16(num_buckets);
+		}
+
 		ocfs2_xattr_bucket_journal_dirty(handle, args->new_bucket);
 
 		ret = ocfs2_reflink_xattr_header(handle, args->reflink,
@@ -6883,6 +6886,7 @@ static int ocfs2_reflink_xattr_buckets(handle_t *handle,
 		}
 
 		ocfs2_xattr_bucket_journal_dirty(handle, args->new_bucket);
+
 		ocfs2_xattr_bucket_relse(args->old_bucket);
 		ocfs2_xattr_bucket_relse(args->new_bucket);
 	}
@@ -6891,6 +6895,74 @@ static int ocfs2_reflink_xattr_buckets(handle_t *handle,
 	ocfs2_xattr_bucket_relse(args->new_bucket);
 	return ret;
 }
+
+static int ocfs2_reflink_xattr_buckets(handle_t *handle,
+				struct inode *inode,
+				struct ocfs2_reflink_xattr_tree_args *args,
+				struct ocfs2_extent_tree *et,
+				struct ocfs2_alloc_context *meta_ac,
+				struct ocfs2_alloc_context *data_ac,
+				u64 blkno, u32 cpos, u32 len)
+{
+	int ret, num_buckets, reflink_buckets, first_inserted = 0;
+	u32 p_cluster, num_clusters, reflink_cpos = 0;
+	u64 new_blkno;
+	int bpc = ocfs2_xattr_buckets_per_cluster(OCFS2_SB(inode->i_sb));
+
+	ret = ocfs2_read_xattr_bucket(args->old_bucket, blkno);
+	if (ret) {
+		mlog_errno(ret);
+		goto out;
+	}
+	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;
+
+		ret = ocfs2_reflink_xattr_bucket(handle, blkno,
+						 new_blkno, num_clusters,
+						 &reflink_cpos, reflink_buckets,
+						 meta_ac, data_ac, args);
+		if (ret) {
+			mlog_errno(ret);
+			goto out;
+		}
+
+		/*
+		 * For the 1st allocated cluster, we make it use the same cpos
+		 * so that the xattr tree looks the same as the original one
+		 * in the most case.
+		 */
+		if (!first_inserted) {
+			reflink_cpos = cpos;
+			first_inserted = 1;
+		}
+		ret = ocfs2_insert_extent(handle, et, reflink_cpos, new_blkno,
+					  num_clusters, 0, meta_ac);
+		if (ret)
+			mlog_errno(ret);
+
+		mlog(0, "insert new xattr extent rec start %llu len %u to %u\n",
+		     (unsigned long long)new_blkno, num_clusters, reflink_cpos);
+
+		len -= num_clusters;
+		blkno += ocfs2_clusters_to_blocks(inode->i_sb, num_clusters);
+		num_buckets -= reflink_buckets;
+	}
+out:
+	return ret;
+}
+
 /*
  * Create the same xattr extent record in the new inode's xattr tree.
  */
@@ -6902,8 +6974,6 @@ static int ocfs2_reflink_xattr_rec(struct inode *inode,
 				   void *para)
 {
 	int ret, credits = 0;
-	u32 p_cluster, num_clusters;
-	u64 new_blkno;
 	handle_t *handle;
 	struct ocfs2_reflink_xattr_tree_args *args =
 			(struct ocfs2_reflink_xattr_tree_args *)para;
@@ -6912,6 +6982,9 @@ static int ocfs2_reflink_xattr_rec(struct inode *inode,
 	struct ocfs2_alloc_context *data_ac = NULL;
 	struct ocfs2_extent_tree et;
 
+	mlog(0, "reflink xattr buckets %llu len %u\n",
+	     (unsigned long long)blkno, len);
+
 	ocfs2_init_xattr_tree_extent_tree(&et,
 					  INODE_CACHE(args->reflink->new_inode),
 					  args->new_blk_bh);
@@ -6931,32 +7004,12 @@ static int ocfs2_reflink_xattr_rec(struct inode *inode,
 		goto out;
 	}
 
-	ret = ocfs2_claim_clusters(handle, data_ac,
-				   len, &p_cluster, &num_clusters);
-	if (ret) {
-		mlog_errno(ret);
-		goto out_commit;
-	}
-
-	new_blkno = ocfs2_clusters_to_blocks(osb->sb, p_cluster);
-
-	mlog(0, "reflink xattr buckets %llu to %llu, len %u\n",
-	     (unsigned long long)blkno, (unsigned long long)new_blkno, len);
-	ret = ocfs2_reflink_xattr_buckets(handle, blkno, new_blkno, len,
-					  meta_ac, data_ac, args);
-	if (ret) {
-		mlog_errno(ret);
-		goto out_commit;
-	}
-
-	mlog(0, "insert new xattr extent rec start %llu len %u to %u\n",
-	     (unsigned long long)new_blkno, len, cpos);
-	ret = ocfs2_insert_extent(handle, &et, cpos, new_blkno,
-				  len, 0, meta_ac);
+	ret = ocfs2_reflink_xattr_buckets(handle, inode, args, &et,
+					  meta_ac, data_ac,
+					  blkno, cpos, len);
 	if (ret)
 		mlog_errno(ret);
 
-out_commit:
 	ocfs2_commit_trans(osb, handle);
 
 out:
-- 
1.5.5

[Ocfs2-devel] [PATCH 0/2] ocfs2: make xattr work with new local alloc reservation.

[Joel Becker]

On Fri, Jun 18, 2010 at 11:00:17AM +0800, Tao Ma wrote:
> 	The old xattr codes deem that we can allocate enough contiguous
> clusters from the allocator, it works with the old local alloc
> allocator. But as Mark has added reservation code to ocfs2, now it
> isn't the good hypothesis any more. So these 2 patches try to let
> xattr work with it.

	Am I right that the xattr code is broken without these?

Joel

-- 

"The whole principle is wrong; it's like demanding that grown men 
 live on skim milk because the baby can't eat steak."
        - author Robert A. Heinlein on censorship

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

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

[Joel Becker]

On Fri, Jun 18, 2010 at 11:02:50AM +0800, Tao Ma wrote:
> +		goto restarted_transaction;

	There's no need to confuse the issue with gotos here.  Just make
it a loop, while (clusters_to_add).  Make the vb->vb_access through
ocfs2_journal_dirty() parts a subfunction so that the loop is readable.
Have the subfunction return the number of clusters added, or -errno on
error.  Then process the why in this function.

Joel

-- 

"Reader, suppose you were and idiot.  And suppose you were a member of
 Congress.  But I repeat myself."
	- Mark Twain

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

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

[Joel Becker]

On Fri, Jun 18, 2010 at 11:02:50AM +0800, Tao Ma wrote:
> @@ -735,8 +736,9 @@ static int ocfs2_xattr_extend_allocation(struct inode *inode,
>  					     ctxt->data_ac,
>  					     ctxt->meta_ac,
>  					     &why);

	Btw, this code was already buggy.
ocfs2_xattr_extend_allocation() calls ocfs2_add_clusters_in_btree(),
which can return with RESTART_TRANS just because the filesystem is
fragmented.  We would just fail with EAGAIN in that case, which makes no
sense to a user.
	So this fix actually matters to older kernels and non-reflink
operations too.  Would you agree it should go to the stable tree?  If
so, add the Cc: to your commit message.

Joel

-- 

"There is shadow under this red rock.
 (Come in under the shadow of this red rock)
 And I will show you something different from either
 Your shadow at morning striding behind you
 Or your shadow at evening rising to meet you.
 I will show you fear in a handful of dust."

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

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

[Joel Becker]

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.
	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);

Actually, I think that's the most readable.

Joel

-- 

Life's Little Instruction Book #337

	"Reread your favorite book."

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

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

[Tao Ma]

On 07/09/2010 02:51 AM, Joel Becker wrote:
> On Fri, Jun 18, 2010 at 11:02:50AM +0800, Tao Ma wrote:
>> @@ -735,8 +736,9 @@ static int ocfs2_xattr_extend_allocation(struct inode *inode,
>>   					     ctxt->data_ac,
>>   					     ctxt->meta_ac,
>>   					&why);
>
> 	Btw, this code was already buggy.
> ocfs2_xattr_extend_allocation() calls ocfs2_add_clusters_in_btree(),
> which can return with RESTART_TRANS just because the filesystem is
> fragmented.  We would just fail with EAGAIN in that case, which makes no
> sense to a user.
> 	So this fix actually matters to older kernels and non-reflink
> operations too.  Would you agree it should go to the stable tree?  If
> so, add the Cc: to your commit message.
yes, the old kernel should be affected by this and we should have it in 
stable tree.

But I just have one concern: should I add stable to cc list the first 
time I create the patch? I am afraid not since it may be changed may 
times and have many revisions.

So my suggestion is that we go on the normal process, and when my patch 
get acked, I will resend it, add your ack and cc to stable. Agree? The 
same goes to other's patches.

Regards,
Tao

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

[Tao Ma]

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

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

[Joel Becker]

On Fri, Jul 09, 2010 at 11:38:33AM +0800, Tao Ma wrote:
> But I just have one concern: should I add stable to cc list the
> first time I create the patch? I am afraid not since it may be
> changed may times and have many revisions.

	You can add it before the final pull request.  You don't need to
add my ack - I'll have signed off on the way through my tree.  I always
use --suppress-cc=all on git-send-email, so I never worry about this ;-)

Joel

-- 

"Maybe the time has drawn the faces I recall.
 But things in this life change very slowly,
 If they ever change at all."

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