Ocfs2 devel - Feb 2009 - [PATCH 0/2] ocfs2: two fixes for xattr

Hi,

For EAs data structure in inode/block are little different from them in
bucket. These two patches try to make them same for the most part.

The first patch set xh_free_start and xh_name_value_len when EAs in
inode/block. xh_free_start is useful to keep the minimum offset of the
xattr name/value. But xh_name_value_len is not very useful because we 
don't have "hole" when EAs in inode/block, we just calculate and
set it
here, maybe it's useful for fsck.

The second patch reserve a blank space between xattr entry and 
name/value when EAs in bucket. This is same as EAs in inode/block and is 
useful in fsck.

Thanks,
tiger

This patch update fields about xh_free_start and
xh_name_value_len when xattr header in inode/block.
Those fields only be used for bucket before.
With xh_free_start, we are free to calculate minimum
offset of xattr name/value.

Signed-off-by: Tiger Yang <tiger.yang at oracle.com>
---
 fs/ocfs2/ocfs2_fs.h |    2 +-
 fs/ocfs2/xattr.c    |  118 ++++++++++++++++++---------------------------------
 2 files changed, 42 insertions(+), 78 deletions(-)

diff --git a/fs/ocfs2/ocfs2_fs.h b/fs/ocfs2/ocfs2_fs.h
index c7ae45a..597d047 100644
--- a/fs/ocfs2/ocfs2_fs.h
+++ b/fs/ocfs2/ocfs2_fs.h
@@ -839,7 +839,7 @@ struct ocfs2_xattr_header {
 	__le16	xh_free_start;                  /* current offset for storing
 						   xattr. */
 	__le16	xh_name_value_len;              /* total length of name/value
-						   length in this bucket. */
+						   length in this object. */
 	__le16	xh_num_buckets;                 /* Number of xattr buckets
 						   in this extent record,
 						   only valid in the first
diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 915039f..ce793af 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -89,6 +89,9 @@ struct ocfs2_xattr_set_ctxt {
 					 - sizeof(struct ocfs2_xattr_block) \
 					 - sizeof(struct ocfs2_xattr_header) \
 					 - sizeof(__u32))
+#define OCFS2_XATTR_HEADER_SIZE(ptr)	(le16_to_cpu((ptr)->xh_count) \
+					 * sizeof(struct ocfs2_xattr_entry) \
+					 + sizeof(struct ocfs2_xattr_header))
 
 static struct ocfs2_xattr_def_value_root def_xv = {
 	.xv.xr_list.l_count = cpu_to_le16(1),
@@ -1365,8 +1368,7 @@ static int ocfs2_xattr_set_value_outside(struct inode
*inode,
 static void ocfs2_xattr_set_entry_local(struct inode *inode,
 					struct ocfs2_xattr_info *xi,
 					struct ocfs2_xattr_search *xs,
-					struct ocfs2_xattr_entry *last,
-					size_t min_offs)
+					struct ocfs2_xattr_entry *last)
 {
 	size_t name_len = strlen(xi->name);
 	int i;
@@ -1382,7 +1384,7 @@ static void ocfs2_xattr_set_entry_local(struct inode
*inode,
 		void *val;
 		size_t offs, size;
 
-		first_val = xs->base + min_offs;
+		first_val = xs->base + le16_to_cpu(xs->header->xh_free_start);
 		offs = le16_to_cpu(xs->here->xe_name_offset);
 		val = xs->base + offs;
 
@@ -1417,7 +1419,8 @@ static void ocfs2_xattr_set_entry_local(struct inode
*inode,
 		ocfs2_xattr_set_local(xs->here, 1);
 		xs->here->xe_value_size = 0;
 
-		min_offs += size;
+		le16_add_cpu(&xs->header->xh_free_start, size);
+		le16_add_cpu(&xs->header->xh_name_value_len, -size);
 
 		/* Adjust all value offsets. */
 		last = xs->header->xh_entries;
@@ -1440,11 +1443,14 @@ static void ocfs2_xattr_set_entry_local(struct inode
*inode,
 	}
 	if (xi->value) {
 		/* Insert the new name+value. */
+		void *val;
 		size_t size = OCFS2_XATTR_SIZE(name_len) +
 				OCFS2_XATTR_SIZE(xi->value_len);
-		void *val = xs->base + min_offs - size;
 
-		xs->here->xe_name_offset = cpu_to_le16(min_offs - size);
+		le16_add_cpu(&xs->header->xh_free_start, -size);
+		le16_add_cpu(&xs->header->xh_name_value_len, size);
+		val = xs->base + le16_to_cpu(xs->header->xh_free_start);
+		xs->here->xe_name_offset = xs->header->xh_free_start;
 		memset(val, 0, size);
 		memcpy(val, xi->name, name_len);
 		memcpy(val + OCFS2_XATTR_SIZE(name_len),
@@ -1476,10 +1482,10 @@ static int ocfs2_xattr_set_entry(struct inode *inode,
 	struct ocfs2_xattr_entry *last;
 	struct ocfs2_inode_info *oi = OCFS2_I(inode);
 	struct ocfs2_dinode *di = (struct ocfs2_dinode *)xs->inode_bh->b_data;
-	size_t min_offs = xs->end - xs->base, name_len = strlen(xi->name);
+	size_t name_len = strlen(xi->name);
 	size_t size_l = 0;
 	handle_t *handle = ctxt->handle;
-	int free, i, ret;
+	int free, ret;
 	struct ocfs2_xattr_info xi_l = {
 		.name_index = xi->name_index,
 		.name = xi->name,
@@ -1497,17 +1503,10 @@ static int ocfs2_xattr_set_entry(struct inode *inode,
 	} else
 		BUG_ON(xs->xattr_bh != xs->inode_bh);
 
-	/* Compute min_offs, last and free space. */
-	last = xs->header->xh_entries;
-
-	for (i = 0 ; i < le16_to_cpu(xs->header->xh_count); i++) {
-		size_t offs = le16_to_cpu(last->xe_name_offset);
-		if (offs < min_offs)
-			min_offs = offs;
-		last += 1;
-	}
-
-	free = min_offs - ((void *)last - xs->base) - sizeof(__u32);
+	/* Compute last entry and free space. */
+	last =
&xs->header->xh_entries[le16_to_cpu(xs->header->xh_count)];
+	free = le16_to_cpu(xs->header->xh_free_start) -
+		OCFS2_XATTR_HEADER_SIZE(xs->header) - sizeof(__u32);
 	if (free < 0)
 		return -EIO;
 
@@ -1522,22 +1521,17 @@ static int ocfs2_xattr_set_entry(struct inode *inode,
 		free += (size + sizeof(struct ocfs2_xattr_entry));
 	}
 	/* Check free space in inode or block */
-	if (xi->value && xi->value_len > OCFS2_XATTR_INLINE_SIZE) {
-		if (free < sizeof(struct ocfs2_xattr_entry) +
-			   OCFS2_XATTR_SIZE(name_len) +
-			   OCFS2_XATTR_ROOT_SIZE) {
+	if (xi->value) {
+		if (ocfs2_xattr_entry_real_size(name_len,
+						xi->value_len) > free) {
 			ret = -ENOSPC;
 			goto out;
 		}
-		size_l = OCFS2_XATTR_SIZE(name_len) + OCFS2_XATTR_ROOT_SIZE;
-		xi_l.value = (void *)&def_xv;
-		xi_l.value_len = OCFS2_XATTR_ROOT_SIZE;
-	} else if (xi->value) {
-		if (free < sizeof(struct ocfs2_xattr_entry) +
-			   OCFS2_XATTR_SIZE(name_len) +
-			   OCFS2_XATTR_SIZE(xi->value_len)) {
-			ret = -ENOSPC;
-			goto out;
+		if (xi->value_len > OCFS2_XATTR_INLINE_SIZE) {
+			size_l = OCFS2_XATTR_SIZE(name_len) +
+				 OCFS2_XATTR_ROOT_SIZE;
+			xi_l.value = (void *)&def_xv;
+			xi_l.value_len = OCFS2_XATTR_ROOT_SIZE;
 		}
 	}
 
@@ -1629,7 +1623,7 @@ static int ocfs2_xattr_set_entry(struct inode *inode,
 	 * Set value in local, include set tree root in local.
 	 * This is the first step for value size >INLINE_SIZE.
 	 */
-	ocfs2_xattr_set_entry_local(inode, &xi_l, xs, last, min_offs);
+	ocfs2_xattr_set_entry_local(inode, &xi_l, xs, last);
 
 	if (!(flag & OCFS2_INLINE_XATTR_FL)) {
 		ret = ocfs2_journal_dirty(handle, xs->xattr_bh);
@@ -1973,9 +1967,12 @@ static int ocfs2_xattr_ibody_find(struct inode *inode,
 	if (oi->ip_dyn_features & OCFS2_INLINE_XATTR_FL)
 		xs->header = (struct ocfs2_xattr_header *)
 			(xs->end - le16_to_cpu(di->i_xattr_inline_size));
-	else
+	else {
 		xs->header = (struct ocfs2_xattr_header *)
 			(xs->end - OCFS2_SB(inode->i_sb)->s_xattr_inline_size);
+		xs->header->xh_free_start = cpu_to_le16(
+				OCFS2_SB(inode->i_sb)->s_xattr_inline_size);
+	}
 	xs->base = (void *)xs->header;
 	xs->here = xs->header->xh_entries;
 
@@ -2138,6 +2135,8 @@ static int ocfs2_xattr_block_set(struct inode *inode,
 		xs->base = (void *)xs->header;
 		xs->end = (void *)xblk + inode->i_sb->s_blocksize;
 		xs->here = xs->header->xh_entries;
+		xblk->xb_attrs.xb_header.xh_free_start +					cpu_to_le16(xs->end -
xs->base);
 
 		ret = ocfs2_journal_dirty(handle, new_bh);
 		if (ret < 0) {
@@ -2168,46 +2167,6 @@ end:
 	return ret;
 }
 
-/* Check whether the new xattr can be inserted into the inode. */
-static int ocfs2_xattr_can_be_in_inode(struct inode *inode,
-				       struct ocfs2_xattr_info *xi,
-				       struct ocfs2_xattr_search *xs)
-{
-	u64 value_size;
-	struct ocfs2_xattr_entry *last;
-	int free, i;
-	size_t min_offs = xs->end - xs->base;
-
-	if (!xs->header)
-		return 0;
-
-	last = xs->header->xh_entries;
-
-	for (i = 0; i < le16_to_cpu(xs->header->xh_count); i++) {
-		size_t offs = le16_to_cpu(last->xe_name_offset);
-		if (offs < min_offs)
-			min_offs = offs;
-		last += 1;
-	}
-
-	free = min_offs - ((void *)last - xs->base) - sizeof(__u32);
-	if (free < 0)
-		return 0;
-
-	BUG_ON(!xs->not_found);
-
-	if (xi->value_len > OCFS2_XATTR_INLINE_SIZE)
-		value_size = OCFS2_XATTR_ROOT_SIZE;
-	else
-		value_size = OCFS2_XATTR_SIZE(xi->value_len);
-
-	if (free >= sizeof(struct ocfs2_xattr_entry) +
-		   OCFS2_XATTR_SIZE(strlen(xi->name)) + value_size)
-		return 1;
-
-	return 0;
-}
-
 static int ocfs2_calc_xattr_set_need(struct inode *inode,
 				     struct ocfs2_dinode *di,
 				     struct ocfs2_xattr_info *xi,
@@ -2303,7 +2262,13 @@ static int ocfs2_calc_xattr_set_need(struct inode *inode,
 		 * one will be removed from the xattr block, and this xattr
 		 * will be inserted into inode as a new xattr in inode.
 		 */
-		if (ocfs2_xattr_can_be_in_inode(inode, xi, xis)) {
+		int free = le16_to_cpu(xis->header->xh_free_start) -
+			   OCFS2_XATTR_HEADER_SIZE(xis->header) -
+			   sizeof(__u32);
+		int size = ocfs2_xattr_entry_real_size(strlen(xi->name),
+						       xi->value_len);
+
+		if (size <= free) {
 			clusters_add += new_clusters;
 			credits += ocfs2_remove_extent_credits(inode->i_sb) +
 				    OCFS2_INODE_UPDATE_CREDITS;
@@ -5058,8 +5023,7 @@ try_again:
 	xh = xs->header;
 	count = le16_to_cpu(xh->xh_count);
 	xh_free_start = le16_to_cpu(xh->xh_free_start);
-	header_size = sizeof(struct ocfs2_xattr_header) +
-			count * sizeof(struct ocfs2_xattr_entry);
+	header_size = OCFS2_XATTR_HEADER_SIZE(xh);
 	max_free = OCFS2_XATTR_BUCKET_SIZE -
 		le16_to_cpu(xh->xh_name_value_len) - header_size;
 
-- 
1.5.4.1

This patch set a gap (4 bytes) between xattr entry and
name/value when xattr in bucket. This gap use to seperate
entry and name/value when a bucket is full. It had already
been set when xattr in inode/block.

Signed-off-by: Tiger Yang <tiger.yang at oracle.com>
---
 fs/ocfs2/xattr.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index ce793af..60b5ca3 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -5024,8 +5024,8 @@ try_again:
 	count = le16_to_cpu(xh->xh_count);
 	xh_free_start = le16_to_cpu(xh->xh_free_start);
 	header_size = OCFS2_XATTR_HEADER_SIZE(xh);
-	max_free = OCFS2_XATTR_BUCKET_SIZE -
-		le16_to_cpu(xh->xh_name_value_len) - header_size;
+	max_free = OCFS2_XATTR_BUCKET_SIZE - header_size -
+		le16_to_cpu(xh->xh_name_value_len) - sizeof(__u32);
 
 	mlog_bug_on_msg(header_size > blocksize, "bucket %llu has header size
"
 			"of %u which exceed block size\n",
@@ -5058,7 +5058,7 @@ try_again:
 			need = 0;
 	}
 
-	free = xh_free_start - header_size;
+	free = xh_free_start - header_size - sizeof(__u32);
 	/*
 	 * We need to make sure the new name/value pair
 	 * can exist in the same block.
@@ -5091,7 +5091,7 @@ try_again:
 			}
 
 			xh_free_start = le16_to_cpu(xh->xh_free_start);
-			free = xh_free_start - header_size;
+			free = xh_free_start - header_size - sizeof(__u32);
 			if (xh_free_start % blocksize < need)
 				free -= xh_free_start % blocksize;
 
-- 
1.5.4.1