[Ocfs2-devel] [PATCH 0/13] More xattr bucket consolidation

[Ocfs2-devel] [PATCH 0/13] More xattr bucket consolidation

[Joel Becker]

There are still places in xattr.c that refer to individual buffer_heads
rather than the ocfs2_xattr_bucket structure.  This series tries to
close all of those.  It also reduces some of the buffer_head games we
play.

While doing this work, I think I've learned some more about the xattr
code.  So I've tried to add some breadcrumbs behind me.

Tao, can we review this quickly?  I'd like to get it into merge_window
ASAP, so Tristan can test it and make sure I didn't break anything.

The patches are on top of merge_window as of today, and are available in
my repo at

git://oss.oracle.com/git/jlbec/linux-2.6.git xattr-buckets-2

Joel

[Ocfs2-devel] [PATCH 01/13] ocfs2: Dirty the entire bucket in ocfs2_bucket_value_truncate()

[Joel Becker]

ocfs2_bucket_value_truncate() currently takes the first bh of the
bucket, and magically plays around with the value bh - even though
the bucket structure in the calling function already has it.

In addition, future code wants to always dirty the entire bucket when it
is changed.  So let's pass the entire bucket into this function, skip
any block reads (we have them), and add the access/dirty logic

Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
 fs/ocfs2/xattr.c |   44 ++++++++++++++++++++++++++------------------
 1 files changed, 26 insertions(+), 18 deletions(-)

diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 7e0d62a..4571df8 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -4619,7 +4619,7 @@ out:
  * Copy the new updated xe and xe_value_root to new_xe and new_xv if needed.
  */
 static int ocfs2_xattr_bucket_value_truncate(struct inode *inode,
-					     struct buffer_head *header_bh,
+					     struct ocfs2_xattr_bucket *bucket,
 					     int xe_off,
 					     int len,
 					     struct ocfs2_xattr_set_ctxt *ctxt)
@@ -4629,8 +4629,7 @@ static int ocfs2_xattr_bucket_value_truncate(struct inode *inode,
 	struct buffer_head *value_bh = NULL;
 	struct ocfs2_xattr_value_root *xv;
 	struct ocfs2_xattr_entry *xe;
-	struct ocfs2_xattr_header *xh =
-			(struct ocfs2_xattr_header *)header_bh->b_data;
+	struct ocfs2_xattr_header *xh = bucket_xh(bucket);
 	size_t blocksize = inode->i_sb->s_blocksize;
 
 	xe = &xh->xh_entries[xe_off];
@@ -4644,34 +4643,44 @@ static int ocfs2_xattr_bucket_value_truncate(struct inode *inode,
 
 	/* We don't allow ocfs2_xattr_value to be stored in different block. */
 	BUG_ON(value_blk != (offset + OCFS2_XATTR_ROOT_SIZE - 1) / blocksize);
-	value_blk += header_bh->b_blocknr;
 
-	ret = ocfs2_read_block(inode, value_blk, &value_bh, NULL);
+	value_bh = bucket->bu_bhs[value_blk];
+	BUG_ON(!value_bh);
+
+	xv = (struct ocfs2_xattr_value_root *)
+		(value_bh->b_data + offset % blocksize);
+
+	ret = ocfs2_xattr_bucket_journal_access(ctxt->handle, bucket,
+						OCFS2_JOURNAL_ACCESS_WRITE);
 	if (ret) {
 		mlog_errno(ret);
 		goto out;
 	}
 
-	xv = (struct ocfs2_xattr_value_root *)
-		(value_bh->b_data + offset % blocksize);
-
+	/*
+	 * From here on out we have to dirty the bucket.  The generic
+	 * value calls only modify one of the bucket's bhs, but we need
+	 * to send the bucket at once.  So if they error, they *could* have
+	 * modified something.  We have to assume they did, and dirty
+	 * the whole bucket.  This leaves us in a consistent state.
+	 */
 	mlog(0, "truncate %u in xattr bucket %llu to %d bytes.\n",
-	     xe_off, (unsigned long long)header_bh->b_blocknr, len);
+	     xe_off, (unsigned long long)bucket_blkno(bucket), len);
 	ret = ocfs2_xattr_value_truncate(inode, value_bh, xv, len, ctxt);
 	if (ret) {
 		mlog_errno(ret);
-		goto out;
+		goto out_dirty;
 	}
 
 	ret = ocfs2_xattr_value_update_size(inode, ctxt->handle,
-					    header_bh, xe, len);
-	if (ret) {
+					    bucket->bu_bhs[0], xe, len);
+	if (ret)
 		mlog_errno(ret);
-		goto out;
-	}
+
+out_dirty:
+	ocfs2_xattr_bucket_journal_dirty(ctxt->handle, bucket);
 
 out:
-	brelse(value_bh);
 	return ret;
 }
 
@@ -4687,7 +4696,7 @@ static int ocfs2_xattr_bucket_value_truncate_xs(struct inode *inode,
 	BUG_ON(!xs->bucket->bu_bhs[0] || !xe || ocfs2_xattr_is_local(xe));
 
 	offset = xe - xh->xh_entries;
-	ret = ocfs2_xattr_bucket_value_truncate(inode, xs->bucket->bu_bhs[0],
+	ret = ocfs2_xattr_bucket_value_truncate(inode, xs->bucket,
 						offset, len, ctxt);
 	if (ret)
 		mlog_errno(ret);
@@ -5129,8 +5138,7 @@ static int ocfs2_delete_xattr_in_bucket(struct inode *inode,
 		if (ocfs2_xattr_is_local(xe))
 			continue;
 
-		ret = ocfs2_xattr_bucket_value_truncate(inode,
-							bucket->bu_bhs[0],
+		ret = ocfs2_xattr_bucket_value_truncate(inode, bucket,
 							i, 0, &ctxt);
 		if (ret) {
 			mlog_errno(ret);
-- 
1.5.6.5

[Ocfs2-devel] [PATCH 01/13] ocfs2: Dirty the entire bucket in ocfs2_bucket_value_truncate()

[Tao Ma]

Joel Becker wrote:
> ocfs2_bucket_value_truncate() currently takes the first bh of the
> bucket, and magically plays around with the value bh - even though
> the bucket structure in the calling function already has it.
> 
> In addition, future code wants to always dirty the entire bucket when it
> is changed.  So let's pass the entire bucket into this function, skip
> any block reads (we have them), and add the access/dirty logic
> 
> Signed-off-by: Joel Becker <joel.becker@oracle.com>
> ---
>  fs/ocfs2/xattr.c |   44 ++++++++++++++++++++++++++------------------
>  1 files changed, 26 insertions(+), 18 deletions(-)
> 
> diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
> index 7e0d62a..4571df8 100644
> --- a/fs/ocfs2/xattr.c
> +++ b/fs/ocfs2/xattr.c
> @@ -4619,7 +4619,7 @@ out:
>   * Copy the new updated xe and xe_value_root to new_xe and new_xv if needed.
>   */
>  static int ocfs2_xattr_bucket_value_truncate(struct inode *inode,
> -					     struct buffer_head *header_bh,
> +					     struct ocfs2_xattr_bucket *bucket,
>  					     int xe_off,
>  					     int len,
>  					     struct ocfs2_xattr_set_ctxt *ctxt)
> @@ -4629,8 +4629,7 @@ static int ocfs2_xattr_bucket_value_truncate(struct inode *inode,
>  	struct buffer_head *value_bh = NULL;
>  	struct ocfs2_xattr_value_root *xv;
>  	struct ocfs2_xattr_entry *xe;
> -	struct ocfs2_xattr_header *xh =
> -			(struct ocfs2_xattr_header *)header_bh->b_data;
> +	struct ocfs2_xattr_header *xh = bucket_xh(bucket);
>  	size_t blocksize = inode->i_sb->s_blocksize;
>  
>  	xe = &xh->xh_entries[xe_off];
> @@ -4644,34 +4643,44 @@ static int ocfs2_xattr_bucket_value_truncate(struct inode *inode,
>  
>  	/* We don't allow ocfs2_xattr_value to be stored in different block. */
>  	BUG_ON(value_blk != (offset + OCFS2_XATTR_ROOT_SIZE - 1) / blocksize);
> -	value_blk += header_bh->b_blocknr;
>  
> -	ret = ocfs2_read_block(inode, value_blk, &value_bh, NULL);
> +	value_bh = bucket->bu_bhs[value_blk];
> +	BUG_ON(!value_bh);
> +
> +	xv = (struct ocfs2_xattr_value_root *)
> +		(value_bh->b_data + offset % blocksize);
> +
> +	ret = ocfs2_xattr_bucket_journal_access(ctxt->handle, bucket,
> +						OCFS2_JOURNAL_ACCESS_WRITE);
>  	if (ret) {
>  		mlog_errno(ret);
>  		goto out;
>  	}
>  
> -	xv = (struct ocfs2_xattr_value_root *)
> -		(value_bh->b_data + offset % blocksize);
> -
> +	/*
> +	 * From here on out we have to dirty the bucket.  The generic
> +	 * value calls only modify one of the bucket's bhs, but we need
> +	 * to send the bucket at once.  So if they error, they *could* have
> +	 * modified something.  We have to assume they did, and dirty
> +	 * the whole bucket.  This leaves us in a consistent state.
> +	 */
>  	mlog(0, "truncate %u in xattr bucket %llu to %d bytes.\n",
> -	     xe_off, (unsigned long long)header_bh->b_blocknr, len);
> +	     xe_off, (unsigned long long)bucket_blkno(bucket), len);
>  	ret = ocfs2_xattr_value_truncate(inode, value_bh, xv, len, ctxt);
>  	if (ret) {
>  		mlog_errno(ret);
> -		goto out;
> +		goto out_dirty;
>  	}
>  
>  	ret = ocfs2_xattr_value_update_size(inode, ctxt->handle,
> -					    header_bh, xe, len);
> -	if (ret) {
> +					    bucket->bu_bhs[0], xe, len);
> +	if (ret)
ocfs2_xattr_value_update_size only access the first_bh, update the size 
and then dirty it. Since you now access and dirty the whole bucket, I 
think maybe you can just remove this function and move the size update here.
>  		mlog_errno(ret);
> -		goto out;
> -	}
> +
> +out_dirty:
> +	ocfs2_xattr_bucket_journal_dirty(ctxt->handle, bucket);
>  
>  out:
> -	brelse(value_bh);
>  	return ret;
>  }

Regards,
Tao

[Ocfs2-devel] [PATCH 01/13] ocfs2: Dirty the entire bucket in ocfs2_bucket_value_truncate()

[Joel Becker]

On Thu, Nov 27, 2008 at 09:23:52AM +0800, Tao Ma wrote:
> ocfs2_xattr_value_update_size only access the first_bh, update the size  
> and then dirty it. Since you now access and dirty the whole bucket, I  
> think maybe you can just remove this function and move the size update 
> here.

	Good point.  Here's an updated version.

diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 7e0d62a..aae4fe2 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -4586,31 +4586,6 @@ out:
 	return ret;
 }
 
-static int ocfs2_xattr_value_update_size(struct inode *inode,
-					 handle_t *handle,
-					 struct buffer_head *xe_bh,
-					 struct ocfs2_xattr_entry *xe,
-					 u64 new_size)
-{
-	int ret;
-
-	ret = ocfs2_journal_access(handle, inode, xe_bh,
-				   OCFS2_JOURNAL_ACCESS_WRITE);
-	if (ret < 0) {
-		mlog_errno(ret);
-		goto out;
-	}
-
-	xe->xe_value_size = cpu_to_le64(new_size);
-
-	ret = ocfs2_journal_dirty(handle, xe_bh);
-	if (ret < 0)
-		mlog_errno(ret);
-
-out:
-	return ret;
-}
-
 /*
  * Truncate the specified xe_off entry in xattr bucket.
  * bucket is indicated by header_bh and len is the new length.
@@ -4619,7 +4594,7 @@ out:
  * Copy the new updated xe and xe_value_root to new_xe and new_xv if needed.
  */
 static int ocfs2_xattr_bucket_value_truncate(struct inode *inode,
-					     struct buffer_head *header_bh,
+					     struct ocfs2_xattr_bucket *bucket,
 					     int xe_off,
 					     int len,
 					     struct ocfs2_xattr_set_ctxt *ctxt)
@@ -4629,8 +4604,7 @@ static int ocfs2_xattr_bucket_value_truncate(struct inode *inode,
 	struct buffer_head *value_bh = NULL;
 	struct ocfs2_xattr_value_root *xv;
 	struct ocfs2_xattr_entry *xe;
-	struct ocfs2_xattr_header *xh =
-			(struct ocfs2_xattr_header *)header_bh->b_data;
+	struct ocfs2_xattr_header *xh = bucket_xh(bucket);
 	size_t blocksize = inode->i_sb->s_blocksize;
 
 	xe = &xh->xh_entries[xe_off];
@@ -4644,34 +4618,41 @@ static int ocfs2_xattr_bucket_value_truncate(struct inode *inode,
 
 	/* We don't allow ocfs2_xattr_value to be stored in different block. */
 	BUG_ON(value_blk != (offset + OCFS2_XATTR_ROOT_SIZE - 1) / blocksize);
-	value_blk += header_bh->b_blocknr;
 
-	ret = ocfs2_read_block(inode, value_blk, &value_bh, NULL);
-	if (ret) {
-		mlog_errno(ret);
-		goto out;
-	}
+	value_bh = bucket->bu_bhs[value_blk];
+	BUG_ON(!value_bh);
 
 	xv = (struct ocfs2_xattr_value_root *)
 		(value_bh->b_data + offset % blocksize);
 
-	mlog(0, "truncate %u in xattr bucket %llu to %d bytes.\n",
-	     xe_off, (unsigned long long)header_bh->b_blocknr, len);
-	ret = ocfs2_xattr_value_truncate(inode, value_bh, xv, len, ctxt);
+	ret = ocfs2_xattr_bucket_journal_access(ctxt->handle, bucket,
+						OCFS2_JOURNAL_ACCESS_WRITE);
 	if (ret) {
 		mlog_errno(ret);
 		goto out;
 	}
 
-	ret = ocfs2_xattr_value_update_size(inode, ctxt->handle,
-					    header_bh, xe, len);
+	/*
+	 * From here on out we have to dirty the bucket.  The generic
+	 * value calls only modify one of the bucket's bhs, but we need
+	 * to send the bucket at once.  So if they error, they *could* have
+	 * modified something.  We have to assume they did, and dirty
+	 * the whole bucket.  This leaves us in a consistent state.
+	 */
+	mlog(0, "truncate %u in xattr bucket %llu to %d bytes.\n",
+	     xe_off, (unsigned long long)bucket_blkno(bucket), len);
+	ret = ocfs2_xattr_value_truncate(inode, value_bh, xv, len, ctxt);
 	if (ret) {
 		mlog_errno(ret);
-		goto out;
+		goto out_dirty;
 	}
 
+	xe->xe_value_size = cpu_to_le64(len);
+
+out_dirty:
+	ocfs2_xattr_bucket_journal_dirty(ctxt->handle, bucket);
+
 out:
-	brelse(value_bh);
 	return ret;
 }
 
@@ -4687,7 +4668,7 @@ static int ocfs2_xattr_bucket_value_truncate_xs(struct inode *inode,
 	BUG_ON(!xs->bucket->bu_bhs[0] || !xe || ocfs2_xattr_is_local(xe));
 
 	offset = xe - xh->xh_entries;
-	ret = ocfs2_xattr_bucket_value_truncate(inode, xs->bucket->bu_bhs[0],
+	ret = ocfs2_xattr_bucket_value_truncate(inode, xs->bucket,
 						offset, len, ctxt);
 	if (ret)
 		mlog_errno(ret);
@@ -5129,8 +5110,7 @@ static int ocfs2_delete_xattr_in_bucket(struct inode *inode,
 		if (ocfs2_xattr_is_local(xe))
 			continue;
 
-		ret = ocfs2_xattr_bucket_value_truncate(inode,
-							bucket->bu_bhs[0],
+		ret = ocfs2_xattr_bucket_value_truncate(inode, bucket,
 							i, 0, &ctxt);
 		if (ret) {
 			mlog_errno(ret);
-- 

Life's Little Instruction Book #347

	"Never waste the oppourtunity to tell someone you love them."

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

[Ocfs2-devel] [PATCH 01/13] ocfs2: Dirty the entire bucket in ocfs2_bucket_value_truncate()

[Joel Becker]

On Wed, Nov 26, 2008 at 07:01:09PM -0800, Joel Becker wrote:
> 	Good point.  Here's an updated version.

	Tristan's testing revealed a bug in the credits calculation in
ocfs2_delete_xattr_in_bucket(), which Tao found the fix for.  oss
bugzilla 1059.  That's been merged into the patch.  
	In addition, Tao added this change which I've added to my
branch.

From a647d0ab5a278b7ddcee5075ef345c40f2783a33 Mon Sep 17 00:00:00 2001
From: Tao Ma <tao.ma@oracle.com>
Date: Thu, 11 Dec 2008 08:54:11 +0800
Subject: [PATCH] ocfs2: Narrow the transaction for deleting xattrs from a bucket.

We move the transaction into the loop because in
ocfs2_remove_extent, we will double the credits in function
ocfs2_extend_rotate_transaction. So if we have a large loop
number, we will soon waste much the journal space.

Signed-off-by: Tao Ma <tao.ma@oracle.com>
Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
 fs/ocfs2/xattr.c |   18 +++++++++---------
 1 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 365b876..93c5668 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -5100,30 +5100,30 @@ static int ocfs2_delete_xattr_in_bucket(struct inode *inode,
 
 	ocfs2_init_dealloc_ctxt(&ctxt.dealloc);
 
-	ctxt.handle = ocfs2_start_trans(osb, credits);
-	if (IS_ERR(ctxt.handle)) {
-		ret = PTR_ERR(ctxt.handle);
-		mlog_errno(ret);
-		goto out;
-	}
-
 	for (i = 0; i < le16_to_cpu(xh->xh_count); i++) {
 		xe = &xh->xh_entries[i];
 		if (ocfs2_xattr_is_local(xe))
 			continue;
 
+		ctxt.handle = ocfs2_start_trans(osb, credits);
+		if (IS_ERR(ctxt.handle)) {
+			ret = PTR_ERR(ctxt.handle);
+			mlog_errno(ret);
+			break;
+		}
+
 		ret = ocfs2_xattr_bucket_value_truncate(inode, bucket,
 							i, 0, &ctxt);
+
+		ocfs2_commit_trans(osb, ctxt.handle);
 		if (ret) {
 			mlog_errno(ret);
 			break;
 		}
 	}
 
-	ret = ocfs2_commit_trans(osb, ctxt.handle);
 	ocfs2_schedule_truncate_log_flush(osb, 1);
 	ocfs2_run_deallocs(osb, &ctxt.dealloc);
-out:
 	return ret;
 }
 
-- 
1.5.6.5


-- 

 One look at the From:
 understanding has blossomed
 .procmailrc grows
	- Alexander Viro

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

[Ocfs2-devel] [PATCH 02/13] ocfs2: Dirty the entire first bucket in ocfs2_extend_xattr_bucket()

[Joel Becker]

ocfs2_extend_xattr_bucket() takes an extent of buckets and shifts some
of them down to make room for a new xattr.  It is passed the first bh of
the first bucket, because that is where we store the number of buckets
in the extent.

However, future code wants to always dirty the entire bucket when it
is changed.  So let's pass the entire bucket into this function, skip
any block reads (we have them), and add the access/dirty logic.  We also
can skip passing in the target bucket bh - we only need its block
number.

Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
 fs/ocfs2/xattr.c |   85 +++++++++++++++++++++++++++++++++++-------------------
 1 files changed, 55 insertions(+), 30 deletions(-)

diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 4571df8..9f2e70b 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -3911,7 +3911,7 @@ static int ocfs2_cp_xattr_bucket(struct inode *inode,
 		mlog_errno(ret);
 		goto out;
 	}
-  
+
 	ret = ocfs2_read_xattr_bucket(s_bucket, s_blkno);
 	if (ret)
 		goto out;
@@ -4238,37 +4238,45 @@ leave:
 }
 
 /*
- * Extend a new xattr bucket and move xattrs to the end one by one until
- * We meet with start_bh. Only move half of the xattrs to the bucket after it.
+ * We are given an extent.  'first' is the bucket at the very front of
+ * the extent.  The extent has space for an additional bucket past
+ * bucket_xh(first)->xh_num_buckets.  'target_blkno' is the block number
+ * of the target bucket.  We wish to shift every bucket past the target
+ * down one, filling in that additional space.  When we get back to the
+ * target, we split the target between itself and the now-empty bucket
+ * at target+1 (aka, target_blkno + blks_per_bucket).
  */
 static int ocfs2_extend_xattr_bucket(struct inode *inode,
 				     handle_t *handle,
-				     struct buffer_head *first_bh,
-				     struct buffer_head *start_bh,
+				     struct ocfs2_xattr_bucket *first,
+				     u64 target_blk,
 				     u32 num_clusters)
 {
 	int ret, credits;
 	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
 	u16 blk_per_bucket = ocfs2_blocks_per_xattr_bucket(inode->i_sb);
-	u64 start_blk = start_bh->b_blocknr, end_blk;
-	u32 num_buckets = num_clusters * ocfs2_xattr_buckets_per_cluster(osb);
-	struct ocfs2_xattr_header *first_xh =
-				(struct ocfs2_xattr_header *)first_bh->b_data;
-	u16 bucket = le16_to_cpu(first_xh->xh_num_buckets);
+	u64 end_blk;
+	u16 new_bucket = le16_to_cpu(bucket_xh(first)->xh_num_buckets);
 
 	mlog(0, "extend xattr bucket in %llu, xattr extend rec starting "
-	     "from %llu, len = %u\n", (unsigned long long)start_blk,
-	     (unsigned long long)first_bh->b_blocknr, num_clusters);
+	     "from %llu, len = %u\n", (unsigned long long)target_blk,
+	     (unsigned long long)bucket_blkno(first), num_clusters);
 
-	BUG_ON(bucket >= num_buckets);
+	/* The extent must have room for an additional bucket */
+	BUG_ON(new_bucket >=
+	       (num_clusters * ocfs2_xattr_buckets_per_cluster(osb)));
 
-	end_blk = first_bh->b_blocknr + (bucket - 1) * blk_per_bucket;
+	/* end_blk points to the last existing bucket */
+	end_blk = bucket_blkno(first) + ((new_bucket - 1) * blk_per_bucket);
 
 	/*
-	 * We will touch all the buckets after the start_bh(include it).
-	 * Then we add one more bucket.
+	 * end_blk is the start of the last existing bucket.
+	 * Thus, (end_blk - target_blk) covers the target bucket and
+	 * every bucket after it up to, but not including, the last
+	 * existing bucket.  Then we add the last existing bucket, the
+	 * new bucket, and the first bucket (3 * blk_per_bucket).
 	 */
-	credits = end_blk - start_blk + 3 * blk_per_bucket + 1 +
+	credits = (end_blk - target_blk) + (3 * blk_per_bucket) +
 		  handle->h_buffer_credits;
 	ret = ocfs2_extend_trans(handle, credits);
 	if (ret) {
@@ -4276,14 +4284,14 @@ static int ocfs2_extend_xattr_bucket(struct inode *inode,
 		goto out;
 	}
 
-	ret = ocfs2_journal_access(handle, inode, first_bh,
-				   OCFS2_JOURNAL_ACCESS_WRITE);
+	ret = ocfs2_xattr_bucket_journal_access(handle, first,
+						OCFS2_JOURNAL_ACCESS_WRITE);
 	if (ret) {
 		mlog_errno(ret);
 		goto out;
 	}
 
-	while (end_blk != start_blk) {
+	while (end_blk != target_blk) {
 		ret = ocfs2_cp_xattr_bucket(inode, handle, end_blk,
 					    end_blk + blk_per_bucket, 0);
 		if (ret)
@@ -4291,12 +4299,12 @@ static int ocfs2_extend_xattr_bucket(struct inode *inode,
 		end_blk -= blk_per_bucket;
 	}
 
-	/* Move half of the xattr in start_blk to the next bucket. */
-	ret = ocfs2_divide_xattr_bucket(inode, handle, start_blk,
-					start_blk + blk_per_bucket, NULL, 0);
+	/* Move half of the xattr in target_blkno to the next bucket. */
+	ret = ocfs2_divide_xattr_bucket(inode, handle, target_blk,
+					target_blk + blk_per_bucket, NULL, 0);
 
-	le16_add_cpu(&first_xh->xh_num_buckets, 1);
-	ocfs2_journal_dirty(handle, first_bh);
+	le16_add_cpu(&bucket_xh(first)->xh_num_buckets, 1);
+	ocfs2_xattr_bucket_journal_dirty(handle, first);
 
 out:
 	return ret;
@@ -4330,10 +4338,19 @@ static int ocfs2_add_new_xattr_bucket(struct inode *inode,
 	int ret, num_buckets, extend = 1;
 	u64 p_blkno;
 	u32 e_cpos, num_clusters;
+	/* The bucket at the front of the extent */
+	struct ocfs2_xattr_bucket *first;
 
 	mlog(0, "Add new xattr bucket starting form %llu\n",
 	     (unsigned long long)header_bh->b_blocknr);
 
+	first = ocfs2_xattr_bucket_new(inode);
+	if (!first) {
+		ret = -ENOMEM;
+		mlog_errno(ret);
+		goto out;
+	}
+
 	/*
 	 * Add refrence for header_bh here because it may be
 	 * changed in ocfs2_add_new_xattr_cluster and we need
@@ -4373,17 +4390,25 @@ static int ocfs2_add_new_xattr_bucket(struct inode *inode,
 		}
 	}
 
-	if (extend)
+	if (extend) {
+		/* These bucket reads should be cached */
+		ret = ocfs2_read_xattr_bucket(first, first_bh->b_blocknr);
+		if (ret) {
+			mlog_errno(ret);
+			goto out;
+		}
 		ret = ocfs2_extend_xattr_bucket(inode,
 						ctxt->handle,
-						first_bh,
-						header_bh,
+						first, header_bh->b_blocknr,
 						num_clusters);
-	if (ret)
-		mlog_errno(ret);
+		if (ret)
+			mlog_errno(ret);
+	}
+
 out:
 	brelse(first_bh);
 	brelse(header_bh);
+	ocfs2_xattr_bucket_free(first);
 	return ret;
 }
 
-- 
1.5.6.5

[Ocfs2-devel] [PATCH 03/13] ocfs2: Dirty the entire first bucket in ocfs2_cp_xattr_cluster().

[Joel Becker]

ocfs2_cp_xattr_cluster() takes the last bucket of a full extent and
copies it over to a new extent.  It then updates the headers of both
extents to reflect the new state.  It is passed the first bh of
the first bucket in order to update that first extent's bucket count.
It reads and dirties the first bh of the new extent for the same reason.

However, future code wants to always dirty the entire bucket when it
is changed.  So it is changed to read the entire bucket it is updating
for both extents.

Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
 fs/ocfs2/xattr.c |   80 ++++++++++++++++++++++++++++++++---------------------
 1 files changed, 48 insertions(+), 32 deletions(-)

diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 9f2e70b..11b518a 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -3942,9 +3942,10 @@ out:
 }
 
 /*
- * Copy one xattr cluster from src_blk to to_blk.
- * The to_blk will become the first bucket header of the cluster, so its
- * xh_num_buckets will be initialized as the bucket num in the cluster.
+ * src_blk points to the last cluster of an existing extent.  to_blk
+ * points to a newly allocated extent.  We copy the cluster over to the
+ * new extent, initializing its xh_num_buckets.  The old extent's
+ * xh_num_buckets shrinks by the same amount.
  */
 static int ocfs2_cp_xattr_cluster(struct inode *inode,
 				  handle_t *handle,
@@ -3956,27 +3957,42 @@ static int ocfs2_cp_xattr_cluster(struct inode *inode,
 	int i, ret, credits;
 	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
 	int bpc = ocfs2_clusters_to_blocks(inode->i_sb, 1);
+	int blks_per_bucket = ocfs2_blocks_per_xattr_bucket(inode->i_sb);
 	int num_buckets = ocfs2_xattr_buckets_per_cluster(osb);
-	struct buffer_head *bh = NULL;
-	struct ocfs2_xattr_header *xh;
-	u64 to_blk_start = to_blk;
+	struct ocfs2_xattr_bucket *old_first, *new_first;
 
 	mlog(0, "cp xattrs from cluster %llu to %llu\n",
 	     (unsigned long long)src_blk, (unsigned long long)to_blk);
 
+	/* The first bucket of the original extent */
+	old_first = ocfs2_xattr_bucket_new(inode);
+	/* The first bucket of the new extent */
+	new_first = ocfs2_xattr_bucket_new(inode);
+	if (!old_first || !new_first) {
+		ret = -ENOMEM;
+		mlog_errno(ret);
+		goto out;
+	}
+
+	ret = ocfs2_read_xattr_bucket(old_first, first_bh->b_blocknr);
+	if (ret) {
+		mlog_errno(ret);
+		goto out;
+	}
+
 	/*
-	 * We need to update the new cluster and 1 more for the update of
-	 * the 1st bucket of the previous extent rec.
+	 * We need to update the first bucket of the old extent and the
+	 * entire first cluster of the new extent.
 	 */
-	credits = bpc + 1 + handle->h_buffer_credits;
+	credits = blks_per_bucket + bpc + handle->h_buffer_credits;
 	ret = ocfs2_extend_trans(handle, credits);
 	if (ret) {
 		mlog_errno(ret);
 		goto out;
 	}
 
-	ret = ocfs2_journal_access(handle, inode, first_bh,
-				   OCFS2_JOURNAL_ACCESS_WRITE);
+	ret = ocfs2_xattr_bucket_journal_access(handle, old_first,
+						OCFS2_JOURNAL_ACCESS_WRITE);
 	if (ret) {
 		mlog_errno(ret);
 		goto out;
@@ -3984,45 +4000,45 @@ static int ocfs2_cp_xattr_cluster(struct inode *inode,
 
 	for (i = 0; i < num_buckets; i++) {
 		ret = ocfs2_cp_xattr_bucket(inode, handle,
-					    src_blk, to_blk, 1);
+					    src_blk + (i * blks_per_bucket),
+					    to_blk + (i * blks_per_bucket),
+					    1);
 		if (ret) {
 			mlog_errno(ret);
 			goto out;
 		}
-
-		src_blk += ocfs2_blocks_per_xattr_bucket(inode->i_sb);
-		to_blk += ocfs2_blocks_per_xattr_bucket(inode->i_sb);
 	}
 
-	/* update the old bucket header. */
-	xh = (struct ocfs2_xattr_header *)first_bh->b_data;
-	le16_add_cpu(&xh->xh_num_buckets, -num_buckets);
-
-	ocfs2_journal_dirty(handle, first_bh);
-
-	/* update the new bucket header. */
-	ret = ocfs2_read_block(inode, to_blk_start, &bh, NULL);
-	if (ret < 0) {
+	/*
+	 * Get the new bucket ready before we dirty anything
+	 * (This actually shouldn't fail, because we already dirtied
+	 * it once in ocfs2_cp_xattr_bucket()).
+	 */
+	ret = ocfs2_read_xattr_bucket(new_first, to_blk);
+	if (ret) {
 		mlog_errno(ret);
 		goto out;
 	}
-
-	ret = ocfs2_journal_access(handle, inode, bh,
-				   OCFS2_JOURNAL_ACCESS_WRITE);
+	ret = ocfs2_xattr_bucket_journal_access(handle, new_first,
+						OCFS2_JOURNAL_ACCESS_WRITE);
 	if (ret) {
 		mlog_errno(ret);
 		goto out;
 	}
 
-	xh = (struct ocfs2_xattr_header *)bh->b_data;
-	xh->xh_num_buckets = cpu_to_le16(num_buckets);
+	/* Now update the headers */
+	le16_add_cpu(&bucket_xh(old_first)->xh_num_buckets, -num_buckets);
+	ocfs2_xattr_bucket_journal_dirty(handle, old_first);
 
-	ocfs2_journal_dirty(handle, bh);
+	bucket_xh(new_first)->xh_num_buckets = cpu_to_le16(num_buckets);
+	ocfs2_xattr_bucket_journal_dirty(handle, new_first);
 
 	if (first_hash)
-		*first_hash = le32_to_cpu(xh->xh_entries[0].xe_name_hash);
+		*first_hash = le32_to_cpu(bucket_xh(new_first)->xh_entries[0].xe_name_hash);
+
 out:
-	brelse(bh);
+	ocfs2_xattr_bucket_free(new_first);
+	ocfs2_xattr_bucket_free(old_first);
 	return ret;
 }
 
-- 
1.5.6.5

[Ocfs2-devel] [PATCH 04/13] ocfs2: Explain t_is_new in ocfs2_cp_xattr_cluster().

[Joel Becker]

I was unsure of the JOURNAL_ACCESS parameters in
ocfs2_cp_xattr_cluster().  They're based on the function argument
't_is_new', but I couldn't quite figure out how t_is_new mapped to
allocation.  ocfs2_cp_xattr_cluster() actually overwrites the target,
regardless of t_is_new.

Well, I just figured it out.  So I'm adding a big fat comment for those
who come after me.  ocfs2_divide_xattr_cluster() has the same behavior.

Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
 fs/ocfs2/xattr.c |   17 +++++++++++++++++
 1 files changed, 17 insertions(+), 0 deletions(-)

diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 11b518a..cb71dcc 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -3753,6 +3753,11 @@ static int ocfs2_divide_xattr_bucket(struct inode *inode,
 		goto out;
 	}
 
+	/*
+	 * Hey, if we're overwriting t_bucket, what difference does
+	 * ACCESS_CREATE vs ACCESS_WRITE make?  See the comment in the
+	 * same part of ocfs2_cp_xattr_bucket().
+	 */
 	ret = ocfs2_xattr_bucket_journal_access(handle, t_bucket,
 						new_bucket_head ?
 						OCFS2_JOURNAL_ACCESS_CREATE :
@@ -3924,6 +3929,18 @@ static int ocfs2_cp_xattr_bucket(struct inode *inode,
 	if (ret)
 		goto out;
 
+	/*
+	 * Hey, if we're overwriting t_bucket, what difference does
+	 * ACCESS_CREATE vs ACCESS_WRITE make?  Well, if we allocated a new
+	 * cluster to fill, we came here from ocfs2_cp_xattr_cluster(), and
+	 * it is really new - ACCESS_CREATE is required.  But we also
+	 * might have moved data out of t_bucket before extending back
+	 * into it.  ocfs2_add_new_xattr_bucket() can do this - its call
+	 * to ocfs2_add_new_xattr_cluster() may have created a new extent
+	 * and copied out the end of the old extent.  Then it re-extends
+	 * the old extent back to create space for new xattrs.  That's
+	 * how we get here, and the bucket isn't really new.
+	 */
 	ret = ocfs2_xattr_bucket_journal_access(handle, t_bucket,
 						t_is_new ?
 						OCFS2_JOURNAL_ACCESS_CREATE :
-- 
1.5.6.5

[Ocfs2-devel] [PATCH 05/13] ocfs2: Use ocfs2_cp_xattr_bucket() in ocfs2_mv_xattr_bucket_cross_cluster().

[Joel Becker]

The buffer copy loop of ocfs2_mv_xattr_bucket_cross_cluster() actually
looks a lot like ocfs2_cp_xattr_bucket().  Let's just use that instead.
We also use bucket operations to update the buckets at the start of each
extent.

Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
 fs/ocfs2/xattr.c |  169 +++++++++++++++++++++++++++++++++---------------------
 1 files changed, 104 insertions(+), 65 deletions(-)

diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index cb71dcc..c9e2f77 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -170,6 +170,11 @@ static int ocfs2_xattr_set_entry_index_block(struct inode *inode,
 
 static int ocfs2_delete_xattr_index_block(struct inode *inode,
 					  struct buffer_head *xb_bh);
+static int ocfs2_cp_xattr_bucket(struct inode *inode,
+				 handle_t *handle,
+				 u64 s_blkno,
+				 u64 t_blkno,
+				 int t_is_new);
 
 static inline u16 ocfs2_xattr_buckets_per_cluster(struct ocfs2_super *osb)
 {
@@ -3532,13 +3537,21 @@ out:
 }
 
 /*
- * Move half nums of the xattr bucket in the previous cluster to this new
- * cluster. We only touch the last cluster of the previous extend record.
+ * prev_blkno points to the start of an existing extent.  new_blkno
+ * points to a newly allocated extent.  Because we know each of our
+ * clusters contains more than bucket, we can easily split one cluster
+ * at a bucket boundary.  So we take the last cluster of the existing
+ * extent and split it down the middle.  We move the last half of the
+ * buckets in the last cluster of the existing extent over to the new
+ * extent.
+ *
+ * first_bh is the buffer at prev_blkno so we can update the existing
+ * extent's bucket count.  header_bh is the bucket were we were hoping
+ * to insert our xattr.  If the bucket move places the target in the new
+ * extent, we'll update first_bh and header_bh after modifying the old
+ * extent.
  *
- * first_bh is the first buffer_head of a series of bucket in the same
- * extent rec and header_bh is the header of one bucket in this cluster.
- * They will be updated if we move the data header_bh contains to the new
- * cluster. first_hash will be set as the 1st xe's name_hash of the new cluster.
+ * first_hash will be set as the 1st xe's name_hash in the new extent.
  */
 static int ocfs2_mv_xattr_bucket_cross_cluster(struct inode *inode,
 					       handle_t *handle,
@@ -3551,105 +3564,131 @@ static int ocfs2_mv_xattr_bucket_cross_cluster(struct inode *inode,
 {
 	int i, ret, credits;
 	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
+	int blks_per_bucket = ocfs2_blocks_per_xattr_bucket(inode->i_sb);
 	int bpc = ocfs2_clusters_to_blocks(inode->i_sb, 1);
 	int num_buckets = ocfs2_xattr_buckets_per_cluster(osb);
-	int blocksize = inode->i_sb->s_blocksize;
-	struct buffer_head *old_bh, *new_bh, *prev_bh, *new_first_bh = NULL;
-	struct ocfs2_xattr_header *new_xh;
+	int to_move = num_buckets / 2;
+	u64 last_cluster_blkno, src_blkno;
 	struct ocfs2_xattr_header *xh =
 			(struct ocfs2_xattr_header *)((*first_bh)->b_data);
+	struct ocfs2_xattr_bucket *old_first, *new_first;
 
 	BUG_ON(le16_to_cpu(xh->xh_num_buckets) < num_buckets);
 	BUG_ON(OCFS2_XATTR_BUCKET_SIZE == osb->s_clustersize);
 
-	prev_bh = *first_bh;
-	get_bh(prev_bh);
-	xh = (struct ocfs2_xattr_header *)prev_bh->b_data;
-
-	prev_blkno += (num_clusters - 1) * bpc + bpc / 2;
+	last_cluster_blkno = prev_blkno + ((num_clusters - 1) * bpc);
+	src_blkno = last_cluster_blkno + (to_move * blks_per_bucket);
 
 	mlog(0, "move half of xattrs in cluster %llu to %llu\n",
 	     (unsigned long long)prev_blkno, (unsigned long long)new_blkno);
 
+	/* The first bucket of the original extent */
+	old_first = ocfs2_xattr_bucket_new(inode);
+	/* The first bucket of the new extent */
+	new_first = ocfs2_xattr_bucket_new(inode);
+	if (!old_first || !new_first) {
+		ret = -ENOMEM;
+		mlog_errno(ret);
+		goto out;
+	}
+
+	ret = ocfs2_read_xattr_bucket(old_first, prev_blkno);
+	if (ret) {
+		mlog_errno(ret);
+		goto out;
+	}
+
 	/*
-	 * We need to update the 1st half of the new cluster and
-	 * 1 more for the update of the 1st bucket of the previous
-	 * extent record.
+	 * We need to update the 1st half of the new extent, and we
+	 * need to update the first bucket of the old extent.
 	 */
-	credits = bpc / 2 + 1 + handle->h_buffer_credits;
+	credits = ((to_move + 1) * blks_per_bucket) + handle->h_buffer_credits;
 	ret = ocfs2_extend_trans(handle, credits);
 	if (ret) {
 		mlog_errno(ret);
 		goto out;
 	}
 
-	ret = ocfs2_journal_access(handle, inode, prev_bh,
-				   OCFS2_JOURNAL_ACCESS_WRITE);
+	ret = ocfs2_xattr_bucket_journal_access(handle, old_first,
+						OCFS2_JOURNAL_ACCESS_WRITE);
 	if (ret) {
 		mlog_errno(ret);
 		goto out;
 	}
 
-	for (i = 0; i < bpc / 2; i++, prev_blkno++, new_blkno++) {
-		old_bh = new_bh = NULL;
-		new_bh = sb_getblk(inode->i_sb, new_blkno);
-		if (!new_bh) {
-			ret = -EIO;
+	for (i = 0; i < to_move; i++) {
+		ret = ocfs2_cp_xattr_bucket(inode, handle,
+					    src_blkno + (i * blks_per_bucket),
+					    new_blkno + (i * blks_per_bucket),
+					    1);
+		if (ret) {
 			mlog_errno(ret);
 			goto out;
 		}
+	}
 
-		ocfs2_set_new_buffer_uptodate(inode, new_bh);
+	/*
+	 * Get the new bucket ready before we dirty anything
+	 * (This actually shouldn't fail, because we already dirtied
+	 * it once in ocfs2_cp_xattr_bucket()).
+	 */
+	ret = ocfs2_read_xattr_bucket(new_first, new_blkno);
+	if (ret) {
+		mlog_errno(ret);
+		goto out;
+	}
+	ret = ocfs2_xattr_bucket_journal_access(handle, new_first,
+						OCFS2_JOURNAL_ACCESS_WRITE);
+	if (ret) {
+		mlog_errno(ret);
+		goto out;
+	}
 
-		ret = ocfs2_journal_access(handle, inode, new_bh,
-					   OCFS2_JOURNAL_ACCESS_CREATE);
-		if (ret < 0) {
-			mlog_errno(ret);
-			brelse(new_bh);
-			goto out;
-		}
+	/* Now update the headers */
+	le16_add_cpu(&bucket_xh(old_first)->xh_num_buckets, -to_move);
+	ocfs2_xattr_bucket_journal_dirty(handle, old_first);
 
-		ret = ocfs2_read_block(inode, prev_blkno, &old_bh, NULL);
-		if (ret < 0) {
-			mlog_errno(ret);
-			brelse(new_bh);
-			goto out;
-		}
+	bucket_xh(new_first)->xh_num_buckets = cpu_to_le16(to_move);
+	ocfs2_xattr_bucket_journal_dirty(handle, new_first);
 
-		memcpy(new_bh->b_data, old_bh->b_data, blocksize);
+	if (first_hash)
+		*first_hash = le32_to_cpu(bucket_xh(new_first)->xh_entries[0].xe_name_hash);
 
-		if (i == 0) {
-			new_xh = (struct ocfs2_xattr_header *)new_bh->b_data;
-			new_xh->xh_num_buckets = cpu_to_le16(num_buckets / 2);
+	/*
+	 * If the target bucket is anywhere past src_blkno, we moved
+	 * it to the new extent.  We need to update first_bh and header_bh.
+	 */
+	if ((*header_bh)->b_blocknr >= src_blkno) {
+		/* We're done with old_first, so we can re-use it. */
+		ocfs2_xattr_bucket_relse(old_first);
 
-			if (first_hash)
-				*first_hash = le32_to_cpu(
-					new_xh->xh_entries[0].xe_name_hash);
-			new_first_bh = new_bh;
-			get_bh(new_first_bh);
-		}
+		/* Find the block for the new target bucket */
+		src_blkno = new_blkno +
+			((*header_bh)->b_blocknr - src_blkno);
 
-		ocfs2_journal_dirty(handle, new_bh);
+		/*
+		 * This shouldn't fail - the buffers are in the
+		 * journal from ocfs2_cp_xattr_bucket().
+		 */
+		ret = ocfs2_read_xattr_bucket(old_first, src_blkno);
+		if (ret) {
+			mlog_errno(ret);
+			goto out;
+		}
 
-		if (*header_bh == old_bh) {
-			brelse(*header_bh);
-			*header_bh = new_bh;
-			get_bh(*header_bh);
+		brelse(*first_bh);
+		*first_bh = new_first->bu_bhs[0];
+		get_bh(*first_bh);
 
-			brelse(*first_bh);
-			*first_bh = new_first_bh;
-			get_bh(*first_bh);
-		}
-		brelse(new_bh);
-		brelse(old_bh);
+		brelse(*header_bh);
+		*header_bh = old_first->bu_bhs[0];
+		get_bh(*header_bh);
 	}
 
-	le16_add_cpu(&xh->xh_num_buckets, -(num_buckets / 2));
-
-	ocfs2_journal_dirty(handle, prev_bh);
 out:
-	brelse(prev_bh);
-	brelse(new_first_bh);
+	ocfs2_xattr_bucket_free(new_first);
+	ocfs2_xattr_bucket_free(old_first);
+
 	return ret;
 }
 
-- 
1.5.6.5

[Ocfs2-devel] [PATCH 06/13] ocfs2: Rename ocfs2_cp_xattr_cluster() to ocfs2_mv_xattr_buckets().

[Joel Becker]

ocfs2_cp_xattr_cluster() takes the last cluster of an xattr extent,
copies its buckets to the front of a new extent, and then shrinks the bucket
count of the original extent.  So it's really moving the data, not
copying it.

While we're here, the function doesn't need a buffer_head for the old
extent, just the block number.

Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
 fs/ocfs2/xattr.c |   42 ++++++++++++++++++++++--------------------
 1 files changed, 22 insertions(+), 20 deletions(-)

diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index c9e2f77..8c0e24b 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -3971,11 +3971,12 @@ static int ocfs2_cp_xattr_bucket(struct inode *inode,
 	/*
 	 * Hey, if we're overwriting t_bucket, what difference does
 	 * ACCESS_CREATE vs ACCESS_WRITE make?  Well, if we allocated a new
-	 * cluster to fill, we came here from ocfs2_cp_xattr_cluster(), and
-	 * it is really new - ACCESS_CREATE is required.  But we also
-	 * might have moved data out of t_bucket before extending back
-	 * into it.  ocfs2_add_new_xattr_bucket() can do this - its call
-	 * to ocfs2_add_new_xattr_cluster() may have created a new extent
+	 * cluster to fill, we came here from
+	 * ocfs2_mv_xattr_buckets(), and it is really new -
+	 * ACCESS_CREATE is required.  But we also might have moved data
+	 * out of t_bucket before extending back into it.
+	 * ocfs2_add_new_xattr_bucket() can do this - its call to
+	 * ocfs2_add_new_xattr_cluster() may have created a new extent
 	 * and copied out the end of the old extent.  Then it re-extends
 	 * the old extent back to create space for new xattrs.  That's
 	 * how we get here, and the bucket isn't really new.
@@ -3998,17 +3999,16 @@ out:
 }
 
 /*
- * src_blk points to the last cluster of an existing extent.  to_blk
- * points to a newly allocated extent.  We copy the cluster over to the
- * new extent, initializing its xh_num_buckets.  The old extent's
- * xh_num_buckets shrinks by the same amount.
+ * src_blk points to the start of an existing extent.  last_blk points to
+ * last cluster in that extent.  to_blk points to a newly allocated
+ * extent.  We copy the buckets from cluster at last_blk to the new extent,
+ * initializing its xh_num_buckets.  The old extent's xh_num_buckets
+ * shrinks by the same amount.
  */
-static int ocfs2_cp_xattr_cluster(struct inode *inode,
+static int ocfs2_mv_xattr_buckets(struct inode *inode,
 				  handle_t *handle,
-				  struct buffer_head *first_bh,
-				  u64 src_blk,
-				  u64 to_blk,
-				  u32 *first_hash)
+				  u64 src_blk, u64 last_blk,
+				  u64 to_blk, u32 *first_hash)
 {
 	int i, ret, credits;
 	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
@@ -4017,8 +4017,8 @@ static int ocfs2_cp_xattr_cluster(struct inode *inode,
 	int num_buckets = ocfs2_xattr_buckets_per_cluster(osb);
 	struct ocfs2_xattr_bucket *old_first, *new_first;
 
-	mlog(0, "cp xattrs from cluster %llu to %llu\n",
-	     (unsigned long long)src_blk, (unsigned long long)to_blk);
+	mlog(0, "mv xattrs from cluster %llu to %llu\n",
+	     (unsigned long long)last_blk, (unsigned long long)to_blk);
 
 	/* The first bucket of the original extent */
 	old_first = ocfs2_xattr_bucket_new(inode);
@@ -4030,7 +4030,7 @@ static int ocfs2_cp_xattr_cluster(struct inode *inode,
 		goto out;
 	}
 
-	ret = ocfs2_read_xattr_bucket(old_first, first_bh->b_blocknr);
+	ret = ocfs2_read_xattr_bucket(old_first, src_blk);
 	if (ret) {
 		mlog_errno(ret);
 		goto out;
@@ -4056,7 +4056,7 @@ static int ocfs2_cp_xattr_cluster(struct inode *inode,
 
 	for (i = 0; i < num_buckets; i++) {
 		ret = ocfs2_cp_xattr_bucket(inode, handle,
-					    src_blk + (i * blks_per_bucket),
+					    last_blk + (i * blks_per_bucket),
 					    to_blk + (i * blks_per_bucket),
 					    1);
 		if (ret) {
@@ -4181,8 +4181,10 @@ static int ocfs2_adjust_xattr_cross_cluster(struct inode *inode,
 		u64 last_blk = prev_blk + bpc * (prev_clusters - 1);
 
 		if (prev_clusters > 1 && (*header_bh)->b_blocknr != last_blk)
-			ret = ocfs2_cp_xattr_cluster(inode, handle, *first_bh,
-						     last_blk, new_blk,
+			ret = ocfs2_mv_xattr_buckets(inode, handle,
+						     (*first_bh)->b_blocknr,
+						     last_blk,
+						     new_blk,
 						     v_start);
 		else {
 			ret = ocfs2_divide_xattr_cluster(inode, handle,
-- 
1.5.6.5

[Ocfs2-devel] [PATCH 07/13] ocfs2: ocfs2_mv_xattr_buckets() can handle a partial cluster now.

[Joel Becker]

If you look at ocfs2_mv_xattr_bucket_cross_cluster(), you'll notice that
two-thirds of the code is almost identical to ocfs2_mv_xattr_buckets().
The only difference is that ocfs2_mv_xattr_buckets() moves a whole
cluster's worth, while ocfs2_mv_xattr_bucket_cross_cluster() moves half
the cluster.

We change ocfs2_mv_xattr_buckets() to allow moving partial clusters.
The original caller of ocfs2_mv_xattr_buckets() still moves the whole
cluster's worth - it just passes a start_bucket of 0.

Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
 fs/ocfs2/xattr.c |   33 ++++++++++++++++++++-------------
 1 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 8c0e24b..652edaf 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -4001,18 +4001,19 @@ out:
 /*
  * src_blk points to the start of an existing extent.  last_blk points to
  * last cluster in that extent.  to_blk points to a newly allocated
- * extent.  We copy the buckets from cluster at last_blk to the new extent,
- * initializing its xh_num_buckets.  The old extent's xh_num_buckets
- * shrinks by the same amount.
+ * extent.  We copy the buckets from the cluster at last_blk to the new
+ * extent.  If start_bucket is non-zero, we skip that many buckets before
+ * we start copying.  The new extent's xh_num_buckets gets set to the
+ * number of buckets we copied.  The old extent's xh_num_buckets shrinks
+ * by the same amount.
  */
-static int ocfs2_mv_xattr_buckets(struct inode *inode,
-				  handle_t *handle,
-				  u64 src_blk, u64 last_blk,
-				  u64 to_blk, u32 *first_hash)
+static int ocfs2_mv_xattr_buckets(struct inode *inode, handle_t *handle,
+				  u64 src_blk, u64 last_blk, u64 to_blk,
+				  unsigned int start_bucket,
+				  u32 *first_hash)
 {
 	int i, ret, credits;
 	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
-	int bpc = ocfs2_clusters_to_blocks(inode->i_sb, 1);
 	int blks_per_bucket = ocfs2_blocks_per_xattr_bucket(inode->i_sb);
 	int num_buckets = ocfs2_xattr_buckets_per_cluster(osb);
 	struct ocfs2_xattr_bucket *old_first, *new_first;
@@ -4020,6 +4021,12 @@ static int ocfs2_mv_xattr_buckets(struct inode *inode,
 	mlog(0, "mv xattrs from cluster %llu to %llu\n",
 	     (unsigned long long)last_blk, (unsigned long long)to_blk);
 
+	BUG_ON(start_bucket >= num_buckets);
+	if (start_bucket) {
+		num_buckets -= start_bucket;
+		last_blk += (start_bucket * blks_per_bucket);
+	}
+
 	/* The first bucket of the original extent */
 	old_first = ocfs2_xattr_bucket_new(inode);
 	/* The first bucket of the new extent */
@@ -4037,10 +4044,11 @@ static int ocfs2_mv_xattr_buckets(struct inode *inode,
 	}
 
 	/*
-	 * We need to update the first bucket of the old extent and the
-	 * entire first cluster of the new extent.
+	 * We need to update the first bucket of the old extent and all
+	 * the buckets going to the new extent.
 	 */
-	credits = blks_per_bucket + bpc + handle->h_buffer_credits;
+	credits = ((num_buckets + 1) * blks_per_bucket) +
+		handle->h_buffer_credits;
 	ret = ocfs2_extend_trans(handle, credits);
 	if (ret) {
 		mlog_errno(ret);
@@ -4183,8 +4191,7 @@ static int ocfs2_adjust_xattr_cross_cluster(struct inode *inode,
 		if (prev_clusters > 1 && (*header_bh)->b_blocknr != last_blk)
 			ret = ocfs2_mv_xattr_buckets(inode, handle,
 						     (*first_bh)->b_blocknr,
-						     last_blk,
-						     new_blk,
+						     last_blk, new_blk, 0,
 						     v_start);
 		else {
 			ret = ocfs2_divide_xattr_cluster(inode, handle,
-- 
1.5.6.5

[Ocfs2-devel] [PATCH 08/13] ocfs2: Use ocfs2_mv_xattr_buckets() in ocfs2_mv_xattr_bucket_cross_cluster().

[Joel Becker]

Now that ocfs2_mv_xattr_buckets() can move a partial cluster's worth of
buckets, ocfs2_mv_xattr_bucket_cross_cluster() can use it.

Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
 fs/ocfs2/xattr.c |  110 ++++++++++++++---------------------------------------
 1 files changed, 29 insertions(+), 81 deletions(-)

diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 652edaf..2c9b673 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -170,11 +170,10 @@ static int ocfs2_xattr_set_entry_index_block(struct inode *inode,
 
 static int ocfs2_delete_xattr_index_block(struct inode *inode,
 					  struct buffer_head *xb_bh);
-static int ocfs2_cp_xattr_bucket(struct inode *inode,
-				 handle_t *handle,
-				 u64 s_blkno,
-				 u64 t_blkno,
-				 int t_is_new);
+static int ocfs2_mv_xattr_buckets(struct inode *inode, handle_t *handle,
+				  u64 src_blk, u64 last_blk, u64 to_blk,
+				  unsigned int start_bucket,
+				  u32 *first_hash);
 
 static inline u16 ocfs2_xattr_buckets_per_cluster(struct ocfs2_super *osb)
 {
@@ -3562,115 +3561,64 @@ static int ocfs2_mv_xattr_bucket_cross_cluster(struct inode *inode,
 					       u32 num_clusters,
 					       u32 *first_hash)
 {
-	int i, ret, credits;
+	int ret;
 	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
 	int blks_per_bucket = ocfs2_blocks_per_xattr_bucket(inode->i_sb);
-	int bpc = ocfs2_clusters_to_blocks(inode->i_sb, 1);
 	int num_buckets = ocfs2_xattr_buckets_per_cluster(osb);
 	int to_move = num_buckets / 2;
-	u64 last_cluster_blkno, src_blkno;
+	u64 src_blkno;
+	u64 last_cluster_blkno = prev_blkno +
+		((num_clusters - 1) * ocfs2_clusters_to_blocks(inode->i_sb, 1));
 	struct ocfs2_xattr_header *xh =
 			(struct ocfs2_xattr_header *)((*first_bh)->b_data);
-	struct ocfs2_xattr_bucket *old_first, *new_first;
+	struct ocfs2_xattr_bucket *new_target, *new_first;
 
 	BUG_ON(le16_to_cpu(xh->xh_num_buckets) < num_buckets);
 	BUG_ON(OCFS2_XATTR_BUCKET_SIZE == osb->s_clustersize);
 
-	last_cluster_blkno = prev_blkno + ((num_clusters - 1) * bpc);
-	src_blkno = last_cluster_blkno + (to_move * blks_per_bucket);
-
 	mlog(0, "move half of xattrs in cluster %llu to %llu\n",
-	     (unsigned long long)prev_blkno, (unsigned long long)new_blkno);
+	     (unsigned long long)last_cluster_blkno, (unsigned long long)new_blkno);
 
-	/* The first bucket of the original extent */
-	old_first = ocfs2_xattr_bucket_new(inode);
 	/* The first bucket of the new extent */
 	new_first = ocfs2_xattr_bucket_new(inode);
-	if (!old_first || !new_first) {
+	/* The target bucket if it was moved to the new extent */
+	new_target = ocfs2_xattr_bucket_new(inode);
+	if (!new_target || !new_first) {
 		ret = -ENOMEM;
 		mlog_errno(ret);
 		goto out;
 	}
 
-	ret = ocfs2_read_xattr_bucket(old_first, prev_blkno);
+	ret = ocfs2_mv_xattr_buckets(inode, handle, prev_blkno,
+				     last_cluster_blkno, new_blkno,
+				     to_move, first_hash);
 	if (ret) {
 		mlog_errno(ret);
 		goto out;
 	}
 
-	/*
-	 * We need to update the 1st half of the new extent, and we
-	 * need to update the first bucket of the old extent.
-	 */
-	credits = ((to_move + 1) * blks_per_bucket) + handle->h_buffer_credits;
-	ret = ocfs2_extend_trans(handle, credits);
-	if (ret) {
-		mlog_errno(ret);
-		goto out;
-	}
-
-	ret = ocfs2_xattr_bucket_journal_access(handle, old_first,
-						OCFS2_JOURNAL_ACCESS_WRITE);
-	if (ret) {
-		mlog_errno(ret);
-		goto out;
-	}
-
-	for (i = 0; i < to_move; i++) {
-		ret = ocfs2_cp_xattr_bucket(inode, handle,
-					    src_blkno + (i * blks_per_bucket),
-					    new_blkno + (i * blks_per_bucket),
-					    1);
-		if (ret) {
-			mlog_errno(ret);
-			goto out;
-		}
-	}
-
-	/*
-	 * Get the new bucket ready before we dirty anything
-	 * (This actually shouldn't fail, because we already dirtied
-	 * it once in ocfs2_cp_xattr_bucket()).
-	 */
-	ret = ocfs2_read_xattr_bucket(new_first, new_blkno);
-	if (ret) {
-		mlog_errno(ret);
-		goto out;
-	}
-	ret = ocfs2_xattr_bucket_journal_access(handle, new_first,
-						OCFS2_JOURNAL_ACCESS_WRITE);
-	if (ret) {
-		mlog_errno(ret);
-		goto out;
-	}
-
-	/* Now update the headers */
-	le16_add_cpu(&bucket_xh(old_first)->xh_num_buckets, -to_move);
-	ocfs2_xattr_bucket_journal_dirty(handle, old_first);
-
-	bucket_xh(new_first)->xh_num_buckets = cpu_to_le16(to_move);
-	ocfs2_xattr_bucket_journal_dirty(handle, new_first);
-
-	if (first_hash)
-		*first_hash = le32_to_cpu(bucket_xh(new_first)->xh_entries[0].xe_name_hash);
+	/* This is the first bucket that got moved */
+	src_blkno = last_cluster_blkno + (to_move * blks_per_bucket);
 
 	/*
-	 * If the target bucket is anywhere past src_blkno, we moved
-	 * it to the new extent.  We need to update first_bh and header_bh.
+	 * If the target bucket was part of the moved buckets, we need to
+	 * update first_bh and header_bh.
 	 */
 	if ((*header_bh)->b_blocknr >= src_blkno) {
-		/* We're done with old_first, so we can re-use it. */
-		ocfs2_xattr_bucket_relse(old_first);
-
 		/* Find the block for the new target bucket */
 		src_blkno = new_blkno +
 			((*header_bh)->b_blocknr - src_blkno);
 
 		/*
-		 * This shouldn't fail - the buffers are in the
+		 * These shouldn't fail - the buffers are in the
 		 * journal from ocfs2_cp_xattr_bucket().
 		 */
-		ret = ocfs2_read_xattr_bucket(old_first, src_blkno);
+		ret = ocfs2_read_xattr_bucket(new_first, src_blkno);
+		if (ret) {
+			mlog_errno(ret);
+			goto out;
+		}
+		ret = ocfs2_read_xattr_bucket(new_target, src_blkno);
 		if (ret) {
 			mlog_errno(ret);
 			goto out;
@@ -3681,13 +3629,13 @@ static int ocfs2_mv_xattr_bucket_cross_cluster(struct inode *inode,
 		get_bh(*first_bh);
 
 		brelse(*header_bh);
-		*header_bh = old_first->bu_bhs[0];
+		*header_bh = new_target->bu_bhs[0];
 		get_bh(*header_bh);
 	}
 
 out:
 	ocfs2_xattr_bucket_free(new_first);
-	ocfs2_xattr_bucket_free(old_first);
+	ocfs2_xattr_bucket_free(new_target);
 
 	return ret;
 }
-- 
1.5.6.5

[Ocfs2-devel] [PATCH 08/13] ocfs2: Use ocfs2_mv_xattr_buckets() in ocfs2_mv_xattr_bucket_cross_cluster().

[Tao Ma]

Joel Becker wrote:
> Now that ocfs2_mv_xattr_buckets() can move a partial cluster's worth of
> buckets, ocfs2_mv_xattr_bucket_cross_cluster() can use it.
> 
> Signed-off-by: Joel Becker <joel.becker@oracle.com>
> ---
>  fs/ocfs2/xattr.c |  110 ++++++++++++++---------------------------------------
>  1 files changed, 29 insertions(+), 81 deletions(-)
> 
> diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
> index 652edaf..2c9b673 100644
> --- a/fs/ocfs2/xattr.c
> +++ b/fs/ocfs2/xattr.c
> @@ -170,11 +170,10 @@ static int ocfs2_xattr_set_entry_index_block(struct inode *inode,
>  
>  static int ocfs2_delete_xattr_index_block(struct inode *inode,
>  					  struct buffer_head *xb_bh);
> -static int ocfs2_cp_xattr_bucket(struct inode *inode,
> -				 handle_t *handle,
> -				 u64 s_blkno,
> -				 u64 t_blkno,
> -				 int t_is_new);
> +static int ocfs2_mv_xattr_buckets(struct inode *inode, handle_t *handle,
> +				  u64 src_blk, u64 last_blk, u64 to_blk,
> +				  unsigned int start_bucket,
> +				  u32 *first_hash);
>  
>  static inline u16 ocfs2_xattr_buckets_per_cluster(struct ocfs2_super *osb)
>  {
> @@ -3562,115 +3561,64 @@ static int ocfs2_mv_xattr_bucket_cross_cluster(struct inode *inode,
>  					       u32 num_clusters,
>  					       u32 *first_hash)
>  {
> -	int i, ret, credits;
> +	int ret;
>  	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
>  	int blks_per_bucket = ocfs2_blocks_per_xattr_bucket(inode->i_sb);
> -	int bpc = ocfs2_clusters_to_blocks(inode->i_sb, 1);
>  	int num_buckets = ocfs2_xattr_buckets_per_cluster(osb);
>  	int to_move = num_buckets / 2;
> -	u64 last_cluster_blkno, src_blkno;
> +	u64 src_blkno;
> +	u64 last_cluster_blkno = prev_blkno +
> +		((num_clusters - 1) * ocfs2_clusters_to_blocks(inode->i_sb, 1));
>  	struct ocfs2_xattr_header *xh =
>  			(struct ocfs2_xattr_header *)((*first_bh)->b_data);
> -	struct ocfs2_xattr_bucket *old_first, *new_first;
> +	struct ocfs2_xattr_bucket *new_target, *new_first;
>  
>  	BUG_ON(le16_to_cpu(xh->xh_num_buckets) < num_buckets);
>  	BUG_ON(OCFS2_XATTR_BUCKET_SIZE == osb->s_clustersize);
>  
> -	last_cluster_blkno = prev_blkno + ((num_clusters - 1) * bpc);
> -	src_blkno = last_cluster_blkno + (to_move * blks_per_bucket);
> -
>  	mlog(0, "move half of xattrs in cluster %llu to %llu\n",
> -	     (unsigned long long)prev_blkno, (unsigned long long)new_blkno);
> +	     (unsigned long long)last_cluster_blkno, (unsigned long long)new_blkno);
>  
> -	/* The first bucket of the original extent */
> -	old_first = ocfs2_xattr_bucket_new(inode);
>  	/* The first bucket of the new extent */
>  	new_first = ocfs2_xattr_bucket_new(inode);
> -	if (!old_first || !new_first) {
> +	/* The target bucket if it was moved to the new extent */
> +	new_target = ocfs2_xattr_bucket_new(inode);
> +	if (!new_target || !new_first) {
>  		ret = -ENOMEM;
>  		mlog_errno(ret);
>  		goto out;
>  	}
>  
> -	ret = ocfs2_read_xattr_bucket(old_first, prev_blkno);
> +	ret = ocfs2_mv_xattr_buckets(inode, handle, prev_blkno,
> +				     last_cluster_blkno, new_blkno,
> +				     to_move, first_hash);
>  	if (ret) {
>  		mlog_errno(ret);
>  		goto out;
>  	}
>  
> -	/*
> -	 * We need to update the 1st half of the new extent, and we
> -	 * need to update the first bucket of the old extent.
> -	 */
> -	credits = ((to_move + 1) * blks_per_bucket) + handle->h_buffer_credits;
> -	ret = ocfs2_extend_trans(handle, credits);
> -	if (ret) {
> -		mlog_errno(ret);
> -		goto out;
> -	}
> -
> -	ret = ocfs2_xattr_bucket_journal_access(handle, old_first,
> -						OCFS2_JOURNAL_ACCESS_WRITE);
> -	if (ret) {
> -		mlog_errno(ret);
> -		goto out;
> -	}
> -
> -	for (i = 0; i < to_move; i++) {
> -		ret = ocfs2_cp_xattr_bucket(inode, handle,
> -					    src_blkno + (i * blks_per_bucket),
> -					    new_blkno + (i * blks_per_bucket),
> -					    1);
> -		if (ret) {
> -			mlog_errno(ret);
> -			goto out;
> -		}
> -	}
> -
> -	/*
> -	 * Get the new bucket ready before we dirty anything
> -	 * (This actually shouldn't fail, because we already dirtied
> -	 * it once in ocfs2_cp_xattr_bucket()).
> -	 */
> -	ret = ocfs2_read_xattr_bucket(new_first, new_blkno);
> -	if (ret) {
> -		mlog_errno(ret);
> -		goto out;
> -	}
> -	ret = ocfs2_xattr_bucket_journal_access(handle, new_first,
> -						OCFS2_JOURNAL_ACCESS_WRITE);
> -	if (ret) {
> -		mlog_errno(ret);
> -		goto out;
> -	}
> -
> -	/* Now update the headers */
> -	le16_add_cpu(&bucket_xh(old_first)->xh_num_buckets, -to_move);
> -	ocfs2_xattr_bucket_journal_dirty(handle, old_first);
> -
> -	bucket_xh(new_first)->xh_num_buckets = cpu_to_le16(to_move);
> -	ocfs2_xattr_bucket_journal_dirty(handle, new_first);
> -
> -	if (first_hash)
> -		*first_hash = le32_to_cpu(bucket_xh(new_first)->xh_entries[0].xe_name_hash);
> +	/* This is the first bucket that got moved */
> +	src_blkno = last_cluster_blkno + (to_move * blks_per_bucket);
>  
>  	/*
> -	 * If the target bucket is anywhere past src_blkno, we moved
> -	 * it to the new extent.  We need to update first_bh and header_bh.
> +	 * If the target bucket was part of the moved buckets, we need to
> +	 * update first_bh and header_bh.
>  	 */
>  	if ((*header_bh)->b_blocknr >= src_blkno) {
> -		/* We're done with old_first, so we can re-use it. */
> -		ocfs2_xattr_bucket_relse(old_first);
> -
>  		/* Find the block for the new target bucket */
>  		src_blkno = new_blkno +
>  			((*header_bh)->b_blocknr - src_blkno);
>  
>  		/*
> -		 * This shouldn't fail - the buffers are in the
> +		 * These shouldn't fail - the buffers are in the
>  		 * journal from ocfs2_cp_xattr_bucket().
>  		 */
> -		ret = ocfs2_read_xattr_bucket(old_first, src_blkno);
> +		ret = ocfs2_read_xattr_bucket(new_first, src_blkno);
> +		if (ret) {
> +			mlog_errno(ret);
> +			goto out;
> +		}
here should read new_blkno since "new_first" means the first bucket of 
the new cluster(I guess that from the code you removed).

Regards,
Tao

[Ocfs2-devel] [PATCH 08/13] ocfs2: Use ocfs2_mv_xattr_buckets() in ocfs2_mv_xattr_bucket_cross_cluster().

[Joel Becker]

On Thu, Nov 27, 2008 at 10:10:38AM +0800, Tao Ma wrote:
> Joel Becker wrote:
>> Now that ocfs2_mv_xattr_buckets() can move a partial cluster's worth of
>> buckets, ocfs2_mv_xattr_bucket_cross_cluster() can use it.

<snip>

>> -		ret = ocfs2_read_xattr_bucket(old_first, src_blkno);
>> +		ret = ocfs2_read_xattr_bucket(new_first, src_blkno);
>> +		if (ret) {
>> +			mlog_errno(ret);
>> +			goto out;
>> +		}
> here should read new_blkno since "new_first" means the first bucket of  
> the new cluster(I guess that from the code you removed).

	I was hoping you'd notice that :-)  I saw it when reviewing
the emailed patches, and I've already fixed it in my git tree.

Joel

-- 

"People with narrow minds usually have broad tongues."

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

[Ocfs2-devel] [PATCH 09/13] ocfs2: Start using buckets in ocfs2_adjust_xattr_cross_cluster().

[Joel Becker]

We want to be passing around buckets instead of buffer_heads.  Let's get
them into ocfs2_adjust_xattr_cross_cluster.

Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
 fs/ocfs2/xattr.c |   46 ++++++++++++++++++++++++++++++++++++++--------
 1 files changed, 38 insertions(+), 8 deletions(-)

diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 2c9b673..4dcbc21 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -3613,7 +3613,7 @@ static int ocfs2_mv_xattr_bucket_cross_cluster(struct inode *inode,
 		 * These shouldn't fail - the buffers are in the
 		 * journal from ocfs2_cp_xattr_bucket().
 		 */
-		ret = ocfs2_read_xattr_bucket(new_first, src_blkno);
+		ret = ocfs2_read_xattr_bucket(new_first, new_blkno);
 		if (ret) {
 			mlog_errno(ret);
 			goto out;
@@ -4117,28 +4117,54 @@ static int ocfs2_adjust_xattr_cross_cluster(struct inode *inode,
 					    u32 *v_start,
 					    int *extend)
 {
-	int ret = 0;
-	int bpc = ocfs2_clusters_to_blocks(inode->i_sb, 1);
+	int ret;
+	struct ocfs2_xattr_bucket *first, *target;
 
 	mlog(0, "adjust xattrs from cluster %llu len %u to %llu\n",
 	     (unsigned long long)prev_blk, prev_clusters,
 	     (unsigned long long)new_blk);
 
+	/* The first bucket of the original extent */
+	first = ocfs2_xattr_bucket_new(inode);
+	/* The target bucket for insert */
+	target = ocfs2_xattr_bucket_new(inode);
+	if (!first || !target) {
+		ret = -ENOMEM;
+		mlog_errno(ret);
+		goto out;
+	}
+
+	BUG_ON(prev_blk != (*first_bh)->b_blocknr);
+	ret = ocfs2_read_xattr_bucket(first, prev_blk);
+	if (ret) {
+		mlog_errno(ret);
+		goto out;
+	}
+
+	ret = ocfs2_read_xattr_bucket(target, (*header_bh)->b_blocknr);
+	if (ret) {
+		mlog_errno(ret);
+		goto out;
+	}
+
 	if (ocfs2_xattr_buckets_per_cluster(OCFS2_SB(inode->i_sb)) > 1)
 		ret = ocfs2_mv_xattr_bucket_cross_cluster(inode,
 							  handle,
 							  first_bh,
 							  header_bh,
 							  new_blk,
-							  prev_blk,
+							  bucket_blkno(first),
 							  prev_clusters,
 							  v_start);
 	else {
-		u64 last_blk = prev_blk + bpc * (prev_clusters - 1);
+		/* The start of the last cluster in the first extent */
+		u64 last_blk = bucket_blkno(first) +
+			((prev_clusters - 1) *
+			 ocfs2_clusters_to_blocks(inode->i_sb, 1));
 
-		if (prev_clusters > 1 && (*header_bh)->b_blocknr != last_blk)
+		if (prev_clusters > 1 && bucket_blkno(target) != last_blk)
 			ret = ocfs2_mv_xattr_buckets(inode, handle,
-						     (*first_bh)->b_blocknr,
+						     bucket_blkno(first),
 						     last_blk, new_blk, 0,
 						     v_start);
 		else {
@@ -4146,11 +4172,15 @@ static int ocfs2_adjust_xattr_cross_cluster(struct inode *inode,
 							 last_blk, new_blk,
 							 v_start);
 
-			if ((*header_bh)->b_blocknr == last_blk && extend)
+			if ((bucket_blkno(target) == last_blk) && extend)
 				*extend = 0;
 		}
 	}
 
+out:
+	ocfs2_xattr_bucket_free(first);
+	ocfs2_xattr_bucket_free(target);
+
 	return ret;
 }
 
-- 
1.5.6.5

[Ocfs2-devel] [PATCH 09/13] ocfs2: Start using buckets in ocfs2_adjust_xattr_cross_cluster().

[Tao Ma]

Joel Becker wrote:
> We want to be passing around buckets instead of buffer_heads.  Let's get
> them into ocfs2_adjust_xattr_cross_cluster.
> 
> Signed-off-by: Joel Becker <joel.becker@oracle.com>
> ---
>  fs/ocfs2/xattr.c |   46 ++++++++++++++++++++++++++++++++++++++--------
>  1 files changed, 38 insertions(+), 8 deletions(-)
> 
> diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
> index 2c9b673..4dcbc21 100644
> --- a/fs/ocfs2/xattr.c
> +++ b/fs/ocfs2/xattr.c
> @@ -3613,7 +3613,7 @@ static int ocfs2_mv_xattr_bucket_cross_cluster(struct inode *inode,
>  		 * These shouldn't fail - the buffers are in the
>  		 * journal from ocfs2_cp_xattr_bucket().
>  		 */
> -		ret = ocfs2_read_xattr_bucket(new_first, src_blkno);
> +		ret = ocfs2_read_xattr_bucket(new_first, new_blkno);
>  		if (ret) {
>  			mlog_errno(ret);
>  			goto out;
sorry, I didn't see you change it here, but it may be merged with the 
previous patch.

Regards,
Tao

[Ocfs2-devel] [PATCH 09/13] ocfs2: Start using buckets in ocfs2_adjust_xattr_cross_cluster().

[Joel Becker]

On Thu, Nov 27, 2008 at 10:16:05AM +0800, Tao Ma wrote:
>> -		ret = ocfs2_read_xattr_bucket(new_first, src_blkno);
>> +		ret = ocfs2_read_xattr_bucket(new_first, new_blkno);
>>  		if (ret) {
>>  			mlog_errno(ret);
>>  			goto out;
> sorry, I didn't see you change it here, but it may be merged with the  
> previous patch.

	I did move it to the previous patch in my git tree.

Joel

-- 

Life's Little Instruction Book #356

	"Be there when people need you."

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

[Ocfs2-devel] [PATCH 10/13] ocfs2: Pass buckets into ocfs2_mv_xattr_bucket_cross_cluster().

[Joel Becker]

Now that ocfs2_adjust_xattr_cross_cluster() has buckets, it can pass
them into ocfs2_mv_xattr_bucket_cross_cluster().  It no longer has to
care about buffer_heads.  The manipulation of first_bh and header_bh
moves up to ocfs2_adjust_xattr_cross_cluster().

Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
 fs/ocfs2/xattr.c |   84 +++++++++++++++++++++++------------------------------
 1 files changed, 37 insertions(+), 47 deletions(-)

diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 4dcbc21..8ea4032 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -3554,42 +3554,28 @@ out:
  */
 static int ocfs2_mv_xattr_bucket_cross_cluster(struct inode *inode,
 					       handle_t *handle,
-					       struct buffer_head **first_bh,
-					       struct buffer_head **header_bh,
+					       struct ocfs2_xattr_bucket *first,
+					       struct ocfs2_xattr_bucket *target,
 					       u64 new_blkno,
-					       u64 prev_blkno,
 					       u32 num_clusters,
 					       u32 *first_hash)
 {
 	int ret;
-	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
-	int blks_per_bucket = ocfs2_blocks_per_xattr_bucket(inode->i_sb);
-	int num_buckets = ocfs2_xattr_buckets_per_cluster(osb);
+	struct super_block *sb = inode->i_sb;
+	int blks_per_bucket = ocfs2_blocks_per_xattr_bucket(sb);
+	int num_buckets = ocfs2_xattr_buckets_per_cluster(OCFS2_SB(sb));
 	int to_move = num_buckets / 2;
 	u64 src_blkno;
-	u64 last_cluster_blkno = prev_blkno +
-		((num_clusters - 1) * ocfs2_clusters_to_blocks(inode->i_sb, 1));
-	struct ocfs2_xattr_header *xh =
-			(struct ocfs2_xattr_header *)((*first_bh)->b_data);
-	struct ocfs2_xattr_bucket *new_target, *new_first;
+	u64 last_cluster_blkno = bucket_blkno(first) +
+		((num_clusters - 1) * ocfs2_clusters_to_blocks(sb, 1));
 
-	BUG_ON(le16_to_cpu(xh->xh_num_buckets) < num_buckets);
-	BUG_ON(OCFS2_XATTR_BUCKET_SIZE == osb->s_clustersize);
+	BUG_ON(le16_to_cpu(bucket_xh(first)->xh_num_buckets) < num_buckets);
+	BUG_ON(OCFS2_XATTR_BUCKET_SIZE == OCFS2_SB(sb)->s_clustersize);
 
 	mlog(0, "move half of xattrs in cluster %llu to %llu\n",
 	     (unsigned long long)last_cluster_blkno, (unsigned long long)new_blkno);
 
-	/* The first bucket of the new extent */
-	new_first = ocfs2_xattr_bucket_new(inode);
-	/* The target bucket if it was moved to the new extent */
-	new_target = ocfs2_xattr_bucket_new(inode);
-	if (!new_target || !new_first) {
-		ret = -ENOMEM;
-		mlog_errno(ret);
-		goto out;
-	}
-
-	ret = ocfs2_mv_xattr_buckets(inode, handle, prev_blkno,
+	ret = ocfs2_mv_xattr_buckets(inode, handle, bucket_blkno(first),
 				     last_cluster_blkno, new_blkno,
 				     to_move, first_hash);
 	if (ret) {
@@ -3602,41 +3588,32 @@ static int ocfs2_mv_xattr_bucket_cross_cluster(struct inode *inode,
 
 	/*
 	 * If the target bucket was part of the moved buckets, we need to
-	 * update first_bh and header_bh.
+	 * update first and target.
 	 */
-	if ((*header_bh)->b_blocknr >= src_blkno) {
+	if (bucket_blkno(target) >= src_blkno) {
 		/* Find the block for the new target bucket */
 		src_blkno = new_blkno +
-			((*header_bh)->b_blocknr - src_blkno);
+			(bucket_blkno(target) - src_blkno);
+
+		ocfs2_xattr_bucket_relse(first);
+		ocfs2_xattr_bucket_relse(target);
 
 		/*
 		 * These shouldn't fail - the buffers are in the
 		 * journal from ocfs2_cp_xattr_bucket().
 		 */
-		ret = ocfs2_read_xattr_bucket(new_first, new_blkno);
+		ret = ocfs2_read_xattr_bucket(first, new_blkno);
 		if (ret) {
 			mlog_errno(ret);
 			goto out;
 		}
-		ret = ocfs2_read_xattr_bucket(new_target, src_blkno);
-		if (ret) {
+		ret = ocfs2_read_xattr_bucket(target, src_blkno);
+		if (ret)
 			mlog_errno(ret);
-			goto out;
-		}
 
-		brelse(*first_bh);
-		*first_bh = new_first->bu_bhs[0];
-		get_bh(*first_bh);
-
-		brelse(*header_bh);
-		*header_bh = new_target->bu_bhs[0];
-		get_bh(*header_bh);
 	}
 
 out:
-	ocfs2_xattr_bucket_free(new_first);
-	ocfs2_xattr_bucket_free(new_target);
-
 	return ret;
 }
 
@@ -4147,16 +4124,29 @@ static int ocfs2_adjust_xattr_cross_cluster(struct inode *inode,
 		goto out;
 	}
 
-	if (ocfs2_xattr_buckets_per_cluster(OCFS2_SB(inode->i_sb)) > 1)
+	if (ocfs2_xattr_buckets_per_cluster(OCFS2_SB(inode->i_sb)) > 1) {
 		ret = ocfs2_mv_xattr_bucket_cross_cluster(inode,
 							  handle,
-							  first_bh,
-							  header_bh,
+							  first, target,
 							  new_blk,
-							  bucket_blkno(first),
 							  prev_clusters,
 							  v_start);
-	else {
+		if (ret) {
+			mlog_errno(ret);
+			goto out;
+		}
+
+		/* Did first+target get moved? */
+		if (prev_blk != bucket_blkno(first)) {
+			brelse(*first_bh);
+			*first_bh = first->bu_bhs[0];
+			get_bh(*first_bh);
+
+			brelse(*header_bh);
+			*header_bh = target->bu_bhs[0];
+			get_bh(*header_bh);
+		}
+	} else {
 		/* The start of the last cluster in the first extent */
 		u64 last_blk = bucket_blkno(first) +
 			((prev_clusters - 1) *
-- 
1.5.6.5

[Ocfs2-devel] [PATCH 11/13] ocfs2: Move buckets up into ocfs2_add_new_xattr_cluster().

[Joel Becker]

Lift the buckets from ocfs2_adjust_xattr_cross_cluster() up into
ocfs2_add_new_xattr_cluster().  Now ocfs2_adjust_xattr_cross_cluster()
doesn't deal with buffer_heads.

Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
 fs/ocfs2/xattr.c |  100 ++++++++++++++++++++++++++---------------------------
 1 files changed, 49 insertions(+), 51 deletions(-)

diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 8ea4032..b9128aa 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -4086,44 +4086,19 @@ static int ocfs2_divide_xattr_cluster(struct inode *inode,
  */
 static int ocfs2_adjust_xattr_cross_cluster(struct inode *inode,
 					    handle_t *handle,
-					    struct buffer_head **first_bh,
-					    struct buffer_head **header_bh,
+					    struct ocfs2_xattr_bucket *first,
+					    struct ocfs2_xattr_bucket *target,
 					    u64 new_blk,
-					    u64 prev_blk,
 					    u32 prev_clusters,
 					    u32 *v_start,
 					    int *extend)
 {
 	int ret;
-	struct ocfs2_xattr_bucket *first, *target;
 
 	mlog(0, "adjust xattrs from cluster %llu len %u to %llu\n",
-	     (unsigned long long)prev_blk, prev_clusters,
+	     (unsigned long long)bucket_blkno(first), prev_clusters,
 	     (unsigned long long)new_blk);
 
-	/* The first bucket of the original extent */
-	first = ocfs2_xattr_bucket_new(inode);
-	/* The target bucket for insert */
-	target = ocfs2_xattr_bucket_new(inode);
-	if (!first || !target) {
-		ret = -ENOMEM;
-		mlog_errno(ret);
-		goto out;
-	}
-
-	BUG_ON(prev_blk != (*first_bh)->b_blocknr);
-	ret = ocfs2_read_xattr_bucket(first, prev_blk);
-	if (ret) {
-		mlog_errno(ret);
-		goto out;
-	}
-
-	ret = ocfs2_read_xattr_bucket(target, (*header_bh)->b_blocknr);
-	if (ret) {
-		mlog_errno(ret);
-		goto out;
-	}
-
 	if (ocfs2_xattr_buckets_per_cluster(OCFS2_SB(inode->i_sb)) > 1) {
 		ret = ocfs2_mv_xattr_bucket_cross_cluster(inode,
 							  handle,
@@ -4131,46 +4106,33 @@ static int ocfs2_adjust_xattr_cross_cluster(struct inode *inode,
 							  new_blk,
 							  prev_clusters,
 							  v_start);
-		if (ret) {
+		if (ret)
 			mlog_errno(ret);
-			goto out;
-		}
-
-		/* Did first+target get moved? */
-		if (prev_blk != bucket_blkno(first)) {
-			brelse(*first_bh);
-			*first_bh = first->bu_bhs[0];
-			get_bh(*first_bh);
-
-			brelse(*header_bh);
-			*header_bh = target->bu_bhs[0];
-			get_bh(*header_bh);
-		}
 	} else {
 		/* The start of the last cluster in the first extent */
 		u64 last_blk = bucket_blkno(first) +
 			((prev_clusters - 1) *
 			 ocfs2_clusters_to_blocks(inode->i_sb, 1));
 
-		if (prev_clusters > 1 && bucket_blkno(target) != last_blk)
+		if (prev_clusters > 1 && bucket_blkno(target) != last_blk) {
 			ret = ocfs2_mv_xattr_buckets(inode, handle,
 						     bucket_blkno(first),
 						     last_blk, new_blk, 0,
 						     v_start);
-		else {
+			if (ret)
+				mlog_errno(ret);
+		} else {
 			ret = ocfs2_divide_xattr_cluster(inode, handle,
 							 last_blk, new_blk,
 							 v_start);
+			if (ret)
+				mlog_errno(ret);
 
 			if ((bucket_blkno(target) == last_blk) && extend)
 				*extend = 0;
 		}
 	}
 
-out:
-	ocfs2_xattr_bucket_free(first);
-	ocfs2_xattr_bucket_free(target);
-
 	return ret;
 }
 
@@ -4208,6 +4170,7 @@ static int ocfs2_add_new_xattr_cluster(struct inode *inode,
 	handle_t *handle = ctxt->handle;
 	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
 	struct ocfs2_extent_tree et;
+	struct ocfs2_xattr_bucket *first, *target;
 
 	mlog(0, "Add new xattr cluster for %llu, previous xattr hash = %u, "
 	     "previous xattr blkno = %llu\n",
@@ -4216,6 +4179,29 @@ static int ocfs2_add_new_xattr_cluster(struct inode *inode,
 
 	ocfs2_init_xattr_tree_extent_tree(&et, inode, root_bh);
 
+	/* The first bucket of the original extent */
+	first = ocfs2_xattr_bucket_new(inode);
+	/* The target bucket for insert */
+	target = ocfs2_xattr_bucket_new(inode);
+	if (!first || !target) {
+		ret = -ENOMEM;
+		mlog_errno(ret);
+		goto leave;
+	}
+
+	BUG_ON(prev_blkno != (*first_bh)->b_blocknr);
+	ret = ocfs2_read_xattr_bucket(first, prev_blkno);
+	if (ret) {
+		mlog_errno(ret);
+		goto leave;
+	}
+
+	ret = ocfs2_read_xattr_bucket(target, (*header_bh)->b_blocknr);
+	if (ret) {
+		mlog_errno(ret);
+		goto leave;
+	}
+
 	ret = ocfs2_journal_access(handle, inode, root_bh,
 				   OCFS2_JOURNAL_ACCESS_WRITE);
 	if (ret < 0) {
@@ -4256,10 +4242,9 @@ static int ocfs2_add_new_xattr_cluster(struct inode *inode,
 	} else {
 		ret = ocfs2_adjust_xattr_cross_cluster(inode,
 						       handle,
-						       first_bh,
-						       header_bh,
+						       first,
+						       target,
 						       block,
-						       prev_blkno,
 						       prev_clusters,
 						       &v_start,
 						       extend);
@@ -4267,6 +4252,17 @@ static int ocfs2_add_new_xattr_cluster(struct inode *inode,
 			mlog_errno(ret);
 			goto leave;
 		}
+
+		/* Did first+target get moved? */
+		if (prev_blkno != bucket_blkno(first)) {
+			brelse(*first_bh);
+			*first_bh = first->bu_bhs[0];
+			get_bh(*first_bh);
+
+			brelse(*header_bh);
+			*header_bh = target->bu_bhs[0];
+			get_bh(*header_bh);
+		}
 	}
 
 	mlog(0, "Insert %u clusters at block %llu for xattr at %u\n",
@@ -4283,6 +4279,8 @@ static int ocfs2_add_new_xattr_cluster(struct inode *inode,
 		mlog_errno(ret);
 
 leave:
+	ocfs2_xattr_bucket_free(first);
+	ocfs2_xattr_bucket_free(target);
 	return ret;
 }
 
-- 
1.5.6.5

[Ocfs2-devel] [PATCH 12/13] ocfs2: Move buckets up into ocfs2_add_new_xattr_bucket().

[Joel Becker]

Lift the buckets from ocfs2_add_new_xattr_cluster() up into
ocfs2_add_new_xattr_bucket().  Now ocfs2_add_new_xattr_cluster()
doesn't deal with buffer_heads.  In fact, we no longer have to play
get_bh() tricks at all.

Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
 fs/ocfs2/xattr.c |  103 +++++++++++++++++-------------------------------------
 1 files changed, 32 insertions(+), 71 deletions(-)

diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index b9128aa..57171d9 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -4154,11 +4154,10 @@ static int ocfs2_adjust_xattr_cross_cluster(struct inode *inode,
  */
 static int ocfs2_add_new_xattr_cluster(struct inode *inode,
 				       struct buffer_head *root_bh,
-				       struct buffer_head **first_bh,
-				       struct buffer_head **header_bh,
+				       struct ocfs2_xattr_bucket *first,
+				       struct ocfs2_xattr_bucket *target,
 				       u32 *num_clusters,
 				       u32 prev_cpos,
-				       u64 prev_blkno,
 				       int *extend,
 				       struct ocfs2_xattr_set_ctxt *ctxt)
 {
@@ -4170,38 +4169,14 @@ static int ocfs2_add_new_xattr_cluster(struct inode *inode,
 	handle_t *handle = ctxt->handle;
 	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
 	struct ocfs2_extent_tree et;
-	struct ocfs2_xattr_bucket *first, *target;
 
 	mlog(0, "Add new xattr cluster for %llu, previous xattr hash = %u, "
 	     "previous xattr blkno = %llu\n",
 	     (unsigned long long)OCFS2_I(inode)->ip_blkno,
-	     prev_cpos, (unsigned long long)prev_blkno);
+	     prev_cpos, (unsigned long long)bucket_blkno(first));
 
 	ocfs2_init_xattr_tree_extent_tree(&et, inode, root_bh);
 
-	/* The first bucket of the original extent */
-	first = ocfs2_xattr_bucket_new(inode);
-	/* The target bucket for insert */
-	target = ocfs2_xattr_bucket_new(inode);
-	if (!first || !target) {
-		ret = -ENOMEM;
-		mlog_errno(ret);
-		goto leave;
-	}
-
-	BUG_ON(prev_blkno != (*first_bh)->b_blocknr);
-	ret = ocfs2_read_xattr_bucket(first, prev_blkno);
-	if (ret) {
-		mlog_errno(ret);
-		goto leave;
-	}
-
-	ret = ocfs2_read_xattr_bucket(target, (*header_bh)->b_blocknr);
-	if (ret) {
-		mlog_errno(ret);
-		goto leave;
-	}
-
 	ret = ocfs2_journal_access(handle, inode, root_bh,
 				   OCFS2_JOURNAL_ACCESS_WRITE);
 	if (ret < 0) {
@@ -4223,7 +4198,7 @@ static int ocfs2_add_new_xattr_cluster(struct inode *inode,
 	mlog(0, "Allocating %u clusters@block %u for xattr in inode %llu\n",
 	     num_bits, bit_off, (unsigned long long)OCFS2_I(inode)->ip_blkno);
 
-	if (prev_blkno + prev_clusters * bpc == block &&
+	if (bucket_blkno(first) + (prev_clusters * bpc) == block &&
 	    (prev_clusters + num_bits) << osb->s_clustersize_bits <=
 	     OCFS2_MAX_XATTR_TREE_LEAF_SIZE) {
 		/*
@@ -4252,17 +4227,6 @@ static int ocfs2_add_new_xattr_cluster(struct inode *inode,
 			mlog_errno(ret);
 			goto leave;
 		}
-
-		/* Did first+target get moved? */
-		if (prev_blkno != bucket_blkno(first)) {
-			brelse(*first_bh);
-			*first_bh = first->bu_bhs[0];
-			get_bh(*first_bh);
-
-			brelse(*header_bh);
-			*header_bh = target->bu_bhs[0];
-			get_bh(*header_bh);
-		}
 	}
 
 	mlog(0, "Insert %u clusters at block %llu for xattr at %u\n",
@@ -4363,16 +4327,16 @@ out:
  * We will move all the buckets starting from header_bh to the next place. As
  * for this one, half num of its xattrs will be moved to the next one.
  *
- * We will allocate a new cluster if current cluster is full and adjust
- * header_bh and first_bh if the insert place is moved to the new cluster.
+ * We will allocate a new cluster if current cluster is full.  The
+ * underlying calls will make sure that there is space at the target
+ * bucket, shifting buckets around if necessary.  'target' may be updated
+ * by those calls.
  */
 static int ocfs2_add_new_xattr_bucket(struct inode *inode,
 				      struct buffer_head *xb_bh,
 				      struct buffer_head *header_bh,
 				      struct ocfs2_xattr_set_ctxt *ctxt)
 {
-	struct ocfs2_xattr_header *first_xh = NULL;
-	struct buffer_head *first_bh = NULL;
 	struct ocfs2_xattr_block *xb =
 			(struct ocfs2_xattr_block *)xb_bh->b_data;
 	struct ocfs2_xattr_tree_root *xb_root = &xb->xb_attrs.xb_root;
@@ -4380,31 +4344,26 @@ static int ocfs2_add_new_xattr_bucket(struct inode *inode,
 	struct ocfs2_xattr_header *xh =
 			(struct ocfs2_xattr_header *)header_bh->b_data;
 	u32 name_hash = le32_to_cpu(xh->xh_entries[0].xe_name_hash);
-	struct super_block *sb = inode->i_sb;
-	struct ocfs2_super *osb = OCFS2_SB(sb);
+	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
 	int ret, num_buckets, extend = 1;
 	u64 p_blkno;
 	u32 e_cpos, num_clusters;
 	/* The bucket at the front of the extent */
-	struct ocfs2_xattr_bucket *first;
+	struct ocfs2_xattr_bucket *first, *target;
 
 	mlog(0, "Add new xattr bucket starting form %llu\n",
 	     (unsigned long long)header_bh->b_blocknr);
 
+	/* The first bucket of the original extent */
 	first = ocfs2_xattr_bucket_new(inode);
-	if (!first) {
+	/* The target bucket for insert */
+	target = ocfs2_xattr_bucket_new(inode);
+	if (!first || !target) {
 		ret = -ENOMEM;
 		mlog_errno(ret);
 		goto out;
 	}
 
-	/*
-	 * Add refrence for header_bh here because it may be
-	 * changed in ocfs2_add_new_xattr_cluster and we need
-	 * to free it in the end.
-	 */
-	get_bh(header_bh);
-
 	ret = ocfs2_xattr_get_rec(inode, name_hash, &p_blkno, &e_cpos,
 				  &num_clusters, el);
 	if (ret) {
@@ -4412,23 +4371,30 @@ static int ocfs2_add_new_xattr_bucket(struct inode *inode,
 		goto out;
 	}
 
-	ret = ocfs2_read_block(inode, p_blkno, &first_bh, NULL);
+	ret = ocfs2_read_xattr_bucket(first, p_blkno);
 	if (ret) {
 		mlog_errno(ret);
 		goto out;
 	}
 
-	num_buckets = ocfs2_xattr_buckets_per_cluster(osb) * num_clusters;
-	first_xh = (struct ocfs2_xattr_header *)first_bh->b_data;
+	ret = ocfs2_read_xattr_bucket(target, header_bh->b_blocknr);
+	if (ret) {
+		mlog_errno(ret);
+		goto out;
+	}
 
-	if (num_buckets == le16_to_cpu(first_xh->xh_num_buckets)) {
+	num_buckets = ocfs2_xattr_buckets_per_cluster(osb) * num_clusters;
+	if (num_buckets == le16_to_cpu(bucket_xh(first)->xh_num_buckets)) {
+		/*
+		 * This can move first+target if the target bucket moves
+		 * to the new extent.
+		 */
 		ret = ocfs2_add_new_xattr_cluster(inode,
 						  xb_bh,
-						  &first_bh,
-						  &header_bh,
+						  first,
+						  target,
 						  &num_clusters,
 						  e_cpos,
-						  p_blkno,
 						  &extend,
 						  ctxt);
 		if (ret) {
@@ -4438,24 +4404,19 @@ static int ocfs2_add_new_xattr_bucket(struct inode *inode,
 	}
 
 	if (extend) {
-		/* These bucket reads should be cached */
-		ret = ocfs2_read_xattr_bucket(first, first_bh->b_blocknr);
-		if (ret) {
-			mlog_errno(ret);
-			goto out;
-		}
 		ret = ocfs2_extend_xattr_bucket(inode,
 						ctxt->handle,
-						first, header_bh->b_blocknr,
+						first,
+						bucket_blkno(target),
 						num_clusters);
 		if (ret)
 			mlog_errno(ret);
 	}
 
 out:
-	brelse(first_bh);
-	brelse(header_bh);
 	ocfs2_xattr_bucket_free(first);
+	ocfs2_xattr_bucket_free(target);
+
 	return ret;
 }
 
-- 
1.5.6.5

[Ocfs2-devel] [PATCH 12/13] ocfs2: Move buckets up into ocfs2_add_new_xattr_bucket().

[Joel Becker]

On Wed, Nov 26, 2008 at 04:06:25PM -0800, Joel Becker wrote:
> Lift the buckets from ocfs2_add_new_xattr_cluster() up into
> ocfs2_add_new_xattr_bucket().  Now ocfs2_add_new_xattr_cluster()
> doesn't deal with buffer_heads.  In fact, we no longer have to play
> get_bh() tricks at all.

	Tristan found a bug here, filed as
http://oss.oracle.com/bugzilla/show_bug.cgi?id=1057.
Here's the fix, attached to the bugzilla entry.  I will be rebasing my
tree to include this in the original patch (the xattr-buckets-2 branch).

From 61d88e85c741430c15e4176abc99a5cf11435ff2 Mon Sep 17 00:00:00 2001
From: Joel Becker <joel.becker@oracle.com>
Date: Wed, 10 Dec 2008 14:11:34 -0800
Subject: [PATCH] ocfs2: Don't free buckets in ocfs2_add_new_xattr_cluster()

When I moved the buckets to the parent of ocfs2_add_new_xattr_cluster(),
I forgot to remove the ocfs2_xattr_bucket_free() calls at the bottom.
This causes garbage.

Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
 fs/ocfs2/xattr.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index b75522e..f732e62 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -4288,8 +4288,6 @@ static int ocfs2_add_new_xattr_cluster(struct inode *inode,
 		mlog_errno(ret);
 
 leave:
-	ocfs2_xattr_bucket_free(first);
-	ocfs2_xattr_bucket_free(target);
 	return ret;
 }
 
-- 
1.5.6.5

-- 

"I always thought the hardest questions were those I could not answer.
 Now I know they are the ones I can never ask."
			- Charlie Watkins

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

[Ocfs2-devel] [PATCH 13/13] ocfs2: Pass xs->bucket into ocfs2_add_new_xattr_bucket().

[Joel Becker]

Pass the actual target bucket for insert through to
ocfs2_add_new_xattr_bucket().  Now growing a bucket has no buffer_head
knowledge.

ocfs2_add_new_xattr_bucket() leavs xs->bucket in the proper state for
insert.  However, it doesn't update the rest of the search fields in xs,
so we still have to relse() and re-find.  That's OK, because everything
is cached.

Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
 fs/ocfs2/xattr.c |   52 +++++++++++++++++++++++++---------------------------
 1 files changed, 25 insertions(+), 27 deletions(-)

diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 57171d9..cb9617d 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -4322,43 +4322,42 @@ out:
 }
 
 /*
- * Add new xattr bucket in an extent record and adjust the buckets accordingly.
- * xb_bh is the ocfs2_xattr_block.
- * We will move all the buckets starting from header_bh to the next place. As
- * for this one, half num of its xattrs will be moved to the next one.
+ * Add new xattr bucket in an extent record and adjust the buckets
+ * accordingly.  xb_bh is the ocfs2_xattr_block, and target is the
+ * bucket we want to insert into.
  *
- * We will allocate a new cluster if current cluster is full.  The
- * underlying calls will make sure that there is space at the target
- * bucket, shifting buckets around if necessary.  'target' may be updated
- * by those calls.
+ * In the easy case, we will move all the buckets after target down by
+ * one. Half of target's xattrs will be moved to the next bucket.
+ *
+ * If current cluster is full, we'll allocate a new one.  This may not
+ * be contiguous.  The underlying calls will make sure that there is
+ * space for the insert, shifting buckets around if necessary.
+ * 'target' may be moved by those calls.
  */
 static int ocfs2_add_new_xattr_bucket(struct inode *inode,
 				      struct buffer_head *xb_bh,
-				      struct buffer_head *header_bh,
+				      struct ocfs2_xattr_bucket *target,
 				      struct ocfs2_xattr_set_ctxt *ctxt)
 {
 	struct ocfs2_xattr_block *xb =
 			(struct ocfs2_xattr_block *)xb_bh->b_data;
 	struct ocfs2_xattr_tree_root *xb_root = &xb->xb_attrs.xb_root;
 	struct ocfs2_extent_list *el = &xb_root->xt_list;
-	struct ocfs2_xattr_header *xh =
-			(struct ocfs2_xattr_header *)header_bh->b_data;
-	u32 name_hash = le32_to_cpu(xh->xh_entries[0].xe_name_hash);
+	u32 name_hash =
+		le32_to_cpu(bucket_xh(target)->xh_entries[0].xe_name_hash);
 	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
 	int ret, num_buckets, extend = 1;
 	u64 p_blkno;
 	u32 e_cpos, num_clusters;
 	/* The bucket at the front of the extent */
-	struct ocfs2_xattr_bucket *first, *target;
+	struct ocfs2_xattr_bucket *first;
 
-	mlog(0, "Add new xattr bucket starting form %llu\n",
-	     (unsigned long long)header_bh->b_blocknr);
+	mlog(0, "Add new xattr bucket starting from %llu\n",
+	     (unsigned long long)bucket_blkno(target));
 
 	/* The first bucket of the original extent */
 	first = ocfs2_xattr_bucket_new(inode);
-	/* The target bucket for insert */
-	target = ocfs2_xattr_bucket_new(inode);
-	if (!first || !target) {
+	if (!first) {
 		ret = -ENOMEM;
 		mlog_errno(ret);
 		goto out;
@@ -4377,12 +4376,6 @@ static int ocfs2_add_new_xattr_bucket(struct inode *inode,
 		goto out;
 	}
 
-	ret = ocfs2_read_xattr_bucket(target, header_bh->b_blocknr);
-	if (ret) {
-		mlog_errno(ret);
-		goto out;
-	}
-
 	num_buckets = ocfs2_xattr_buckets_per_cluster(osb) * num_clusters;
 	if (num_buckets == le16_to_cpu(bucket_xh(first)->xh_num_buckets)) {
 		/*
@@ -4415,7 +4408,6 @@ static int ocfs2_add_new_xattr_bucket(struct inode *inode,
 
 out:
 	ocfs2_xattr_bucket_free(first);
-	ocfs2_xattr_bucket_free(target);
 
 	return ret;
 }
@@ -5119,15 +5111,21 @@ try_again:
 
 		ret = ocfs2_add_new_xattr_bucket(inode,
 						 xs->xattr_bh,
-						 xs->bucket->bu_bhs[0],
+						 xs->bucket,
 						 ctxt);
 		if (ret) {
 			mlog_errno(ret);
 			goto out;
 		}
 
+		/*
+		 * ocfs2_add_new_xattr_bucket() will have updated
+		 * xs->bucket if it moved, but it will not have updated
+		 * any of the other search fields.  Thus, we drop it and
+		 * re-search.  Everything should be cached, so it'll be
+		 * quick.
+		 */
 		ocfs2_xattr_bucket_relse(xs->bucket);
-
 		ret = ocfs2_xattr_index_block_find(inode, xs->xattr_bh,
 						   xi->name_index,
 						   xi->name, xs);
-- 
1.5.6.5

[Ocfs2-devel] [PATCH 13/13] ocfs2: Pass xs->bucket into ocfs2_add_new_xattr_bucket().

[Tao Ma]

Joel/Mark,
	OK, I have finally finished the review. Except the mail I have sent, 
all the other parts look good.

Regards,
Tao

Joel Becker wrote:
> Pass the actual target bucket for insert through to
> ocfs2_add_new_xattr_bucket().  Now growing a bucket has no buffer_head
> knowledge.
> 
> ocfs2_add_new_xattr_bucket() leavs xs->bucket in the proper state for
> insert.  However, it doesn't update the rest of the search fields in xs,
> so we still have to relse() and re-find.  That's OK, because everything
> is cached.
> 
> Signed-off-by: Joel Becker <joel.becker@oracle.com>
> ---
>  fs/ocfs2/xattr.c |   52 +++++++++++++++++++++++++---------------------------
>  1 files changed, 25 insertions(+), 27 deletions(-)
> 
> diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
> index 57171d9..cb9617d 100644
> --- a/fs/ocfs2/xattr.c
> +++ b/fs/ocfs2/xattr.c
> @@ -4322,43 +4322,42 @@ out:
>  }
>  
>  /*
> - * Add new xattr bucket in an extent record and adjust the buckets accordingly.
> - * xb_bh is the ocfs2_xattr_block.
> - * We will move all the buckets starting from header_bh to the next place. As
> - * for this one, half num of its xattrs will be moved to the next one.
> + * Add new xattr bucket in an extent record and adjust the buckets
> + * accordingly.  xb_bh is the ocfs2_xattr_block, and target is the
> + * bucket we want to insert into.
>   *
> - * We will allocate a new cluster if current cluster is full.  The
> - * underlying calls will make sure that there is space at the target
> - * bucket, shifting buckets around if necessary.  'target' may be updated
> - * by those calls.
> + * In the easy case, we will move all the buckets after target down by
> + * one. Half of target's xattrs will be moved to the next bucket.
> + *
> + * If current cluster is full, we'll allocate a new one.  This may not
> + * be contiguous.  The underlying calls will make sure that there is
> + * space for the insert, shifting buckets around if necessary.
> + * 'target' may be moved by those calls.
>   */
>  static int ocfs2_add_new_xattr_bucket(struct inode *inode,
>  				      struct buffer_head *xb_bh,
> -				      struct buffer_head *header_bh,
> +				      struct ocfs2_xattr_bucket *target,
>  				      struct ocfs2_xattr_set_ctxt *ctxt)
>  {
>  	struct ocfs2_xattr_block *xb =
>  			(struct ocfs2_xattr_block *)xb_bh->b_data;
>  	struct ocfs2_xattr_tree_root *xb_root = &xb->xb_attrs.xb_root;
>  	struct ocfs2_extent_list *el = &xb_root->xt_list;
> -	struct ocfs2_xattr_header *xh =
> -			(struct ocfs2_xattr_header *)header_bh->b_data;
> -	u32 name_hash = le32_to_cpu(xh->xh_entries[0].xe_name_hash);
> +	u32 name_hash =
> +		le32_to_cpu(bucket_xh(target)->xh_entries[0].xe_name_hash);
>  	struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
>  	int ret, num_buckets, extend = 1;
>  	u64 p_blkno;
>  	u32 e_cpos, num_clusters;
>  	/* The bucket at the front of the extent */
> -	struct ocfs2_xattr_bucket *first, *target;
> +	struct ocfs2_xattr_bucket *first;
>  
> -	mlog(0, "Add new xattr bucket starting form %llu\n",
> -	     (unsigned long long)header_bh->b_blocknr);
> +	mlog(0, "Add new xattr bucket starting from %llu\n",
> +	     (unsigned long long)bucket_blkno(target));
>  
>  	/* The first bucket of the original extent */
>  	first = ocfs2_xattr_bucket_new(inode);
> -	/* The target bucket for insert */
> -	target = ocfs2_xattr_bucket_new(inode);
> -	if (!first || !target) {
> +	if (!first) {
>  		ret = -ENOMEM;
>  		mlog_errno(ret);
>  		goto out;
> @@ -4377,12 +4376,6 @@ static int ocfs2_add_new_xattr_bucket(struct inode *inode,
>  		goto out;
>  	}
>  
> -	ret = ocfs2_read_xattr_bucket(target, header_bh->b_blocknr);
> -	if (ret) {
> -		mlog_errno(ret);
> -		goto out;
> -	}
> -
>  	num_buckets = ocfs2_xattr_buckets_per_cluster(osb) * num_clusters;
>  	if (num_buckets == le16_to_cpu(bucket_xh(first)->xh_num_buckets)) {
>  		/*
> @@ -4415,7 +4408,6 @@ static int ocfs2_add_new_xattr_bucket(struct inode *inode,
>  
>  out:
>  	ocfs2_xattr_bucket_free(first);
> -	ocfs2_xattr_bucket_free(target);
>  
>  	return ret;
>  }
> @@ -5119,15 +5111,21 @@ try_again:
>  
>  		ret = ocfs2_add_new_xattr_bucket(inode,
>  						 xs->xattr_bh,
> -						 xs->bucket->bu_bhs[0],
> +						 xs->bucket,
>  						 ctxt);
>  		if (ret) {
>  			mlog_errno(ret);
>  			goto out;
>  		}
>  
> +		/*
> +		 * ocfs2_add_new_xattr_bucket() will have updated
> +		 * xs->bucket if it moved, but it will not have updated
> +		 * any of the other search fields.  Thus, we drop it and
> +		 * re-search.  Everything should be cached, so it'll be
> +		 * quick.
> +		 */
>  		ocfs2_xattr_bucket_relse(xs->bucket);
> -
>  		ret = ocfs2_xattr_index_block_find(inode, xs->xattr_bh,
>  						   xi->name_index,
>  						   xi->name, xs);

[Ocfs2-devel] [PATCH 13/13] ocfs2: Pass xs->bucket into ocfs2_add_new_xattr_bucket().

[Joel Becker]

On Thu, Nov 27, 2008 at 10:39:05AM +0800, Tao Ma wrote:
> 	OK, I have finally finished the review. Except the mail I have sent,  
> all the other parts look good.

Tao,
	Thanks!  That was fast!

Mark,
	The latest versions are in my xattr-buckets-2 branch, honoring
all of Tao's comments.

Joel

-- 

"I'm so tired of being tired,
 Sure as night will follow day.
 Most things I worry about
 Never happen anyway."

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