* [Ocfs2-devel] [PATCH 0/14] ocfs2: Unify the setting of extended attributes
@ 2009-08-28 8:35 Joel Becker
2009-08-28 8:35 ` [Ocfs2-devel] [PATCH 01/14] ocfs2: Introduce ocfs2_xa_loc Joel Becker
` (13 more replies)
0 siblings, 14 replies; 45+ messages in thread
From: Joel Becker @ 2009-08-28 8:35 UTC (permalink / raw)
To: ocfs2-devel
[PATCH 0/14] ocfs2: Unify the setting of extended attributes
ocfs2 can store extended attributes in many ways. They can live inside
the inode's inline data area, they can be stored in a single external
block, or they can be in a "bucket" hanging off of a lookup tree. There
are differences in how each storage type manages the attributes, and the
current ocfs2 code reflects this.
There are two entirely separate code paths for setting extended
attributes. The first code path handles "block" storage - the inline
inode or the external block. The second handles "bucket" storage. They
do very similar things, but they go about them in very different ways.
This makes reading and understanding the ocfs2 xattr code harder than it
needs to be. Worse, updating the xattr code requires understanding and
modifying both paths.
This patch series unifies the xattr set code such that inodes, block,
and buckets make the same call. It removes most of the redundant code,
like the repeated implementations of truncating externally stored values.
The core of the implementation is the ocfs2_xa_loc structure. This is
an in-memory representation of an ocfs2_xattr_entry. It has operations
that allow blocks and buckets to behave differently during the xattr
modification process. Blocks and buckets are different enough that
ocfs2_xa_loc_operations has ten different ops! But with these
differences encapsulated, we can have a single code path to modify an
entry.
The changes don't reduce the size of the source code perceptibly (~10
lines of actual code without comments or blanks), but to my mind they
greatly improve the readability and maintainability.
I've rebased on top of 'merge-window', so they should play well with
cacheme and reflink. So it's ready to go, unless you all find a problem
or two.
Tao and Tiger, I'd still really appreciate you going over the changes
with a find-toothed comb.
Joel
[Pull]
git://git.kernel.org/pub/scm/linux/kernel/git/jlbec/ocfs2.git xa_loc
[View]
http://git.kernel.org/?p=linux/kernel/git/jlbec/ocfs2.git;a=shortlog;h=xa_loc
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 01/14] ocfs2: Introduce ocfs2_xa_loc
2009-08-28 8:35 [Ocfs2-devel] [PATCH 0/14] ocfs2: Unify the setting of extended attributes Joel Becker
@ 2009-08-28 8:35 ` Joel Becker
2009-09-01 5:32 ` Tao Ma
2009-08-28 8:35 ` [Ocfs2-devel] [PATCH 02/14] ocfs2: Remove xattrs via ocfs2_xa_loc Joel Becker
` (12 subsequent siblings)
13 siblings, 1 reply; 45+ messages in thread
From: Joel Becker @ 2009-08-28 8:35 UTC (permalink / raw)
To: ocfs2-devel
The ocfs2 extended attribute (xattr) code is very flexible. It can
store xattrs in the inode itself, in an external block, or in a tree of
data structures. This allows the number of xattrs to be bounded by the
filesystem size.
However, the code that manages each possible storage location is
different. Maintaining the ocfs2 xattr code requires changing each hunk
separately.
This patch is the start of a series introducing the ocfs2_xa_loc
structure. This structure wraps the on-disk details of an xattr
entry. The goal is that the generic xattr routines can use
ocfs2_xa_loc without knowing the underlying storage location.
This first pass merely implements the basic structure, initializing it,
and wiping the name+value pair of the entry.
Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
fs/ocfs2/xattr.c | 242 ++++++++++++++++++++++++++++++++++++++++++++++++++----
1 files changed, 227 insertions(+), 15 deletions(-)
diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index a3eed49..848f24e 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -141,6 +141,51 @@ struct ocfs2_xattr_search {
int not_found;
};
+/* Operations on struct ocfs2_xa_entry */
+struct ocfs2_xa_loc;
+struct ocfs2_xa_loc_operations {
+ /*
+ * Return a pointer to the appropriate buffer in loc->xl_storage
+ * at the given offset from loc->xl_header.
+ */
+ void *(*xlo_offset_pointer)(struct ocfs2_xa_loc *loc, int offset);
+
+ /*
+ * Remove the name+value at this location. Do whatever is
+ * appropriate with the remaining name+value pairs.
+ */
+ void (*xlo_wipe_namevalue)(struct ocfs2_xa_loc *loc);
+};
+
+/*
+ * Describes an xattr entry location. This is a memory structure
+ * tracking the on-disk structure.
+ */
+struct ocfs2_xa_loc {
+ /* The ocfs2_xattr_header inside the on-disk storage. Not NULL. */
+ struct ocfs2_xattr_header *xl_header;
+
+ /* Bytes from xl_header to the end of the storage */
+ int xl_size;
+
+ /*
+ * The ocfs2_xattr_entry this location describes. If this is
+ * NULL, this location describes the on-disk structure where it
+ * would have been.
+ */
+ struct ocfs2_xattr_entry *xl_entry;
+
+ /*
+ * Internal housekeeping
+ */
+
+ /* Buffer(s) containing this entry */
+ void *xl_storage;
+
+ /* Operations on the storage backing this location */
+ const struct ocfs2_xa_loc_operations *xl_ops;
+};
+
static int ocfs2_xattr_bucket_get_name_value(struct super_block *sb,
struct ocfs2_xattr_header *xh,
int index,
@@ -1424,6 +1469,171 @@ static int ocfs2_xattr_set_value_outside(struct inode *inode,
}
/*
+ * Wipe the name+value pair and allow the storage to reclaim it. This
+ * must be followed by either removal of the entry or a call to
+ * ocfs2_xa_add_namevalue().
+ */
+static void ocfs2_xa_wipe_namevalue(struct ocfs2_xa_loc *loc)
+{
+ loc->xl_ops->xlo_wipe_namevalue(loc);
+}
+
+static void *ocfs2_xa_block_offset_pointer(struct ocfs2_xa_loc *loc,
+ int offset)
+{
+ struct buffer_head *bh = loc->xl_storage;
+
+ BUG_ON(offset >= bh->b_size);
+ return bh->b_data + offset;
+}
+
+/*
+ * Block storage for xattrs keeps the name+value pairs compacted. When
+ * we remove one, we have to shift any that preceded it towards the end.
+ */
+static void ocfs2_xa_block_wipe_namevalue(struct ocfs2_xa_loc *loc)
+{
+ int i, offset;
+ int namevalue_offset, first_namevalue_offset, namevalue_size;
+ struct ocfs2_xattr_entry *entry = loc->xl_entry;
+ struct ocfs2_xattr_header *xh = loc->xl_header;
+ u64 value_size = le64_to_cpu(entry->xe_value_size);
+ int count = le16_to_cpu(xh->xh_count);
+
+ namevalue_offset = le16_to_cpu(entry->xe_name_offset);
+ namevalue_size = OCFS2_XATTR_SIZE(entry->xe_name_len);
+ if (value_size > OCFS2_XATTR_INLINE_SIZE)
+ namevalue_size += OCFS2_XATTR_ROOT_SIZE;
+ else
+ namevalue_size += OCFS2_XATTR_SIZE(value_size);
+
+ for (i = 0, first_namevalue_offset = loc->xl_size;
+ i < count; i++) {
+ offset = le16_to_cpu(xh->xh_entries[i].xe_name_offset);
+ if (offset < first_namevalue_offset)
+ first_namevalue_offset = offset;
+ }
+
+ /* Shift the name+value pairs */
+ memmove((char *)xh + first_namevalue_offset + namevalue_size,
+ (char *)xh + first_namevalue_offset,
+ namevalue_offset - first_namevalue_offset);
+ memset((char *)xh + first_namevalue_offset, 0, namevalue_size);
+
+ /* Now tell xh->xh_entries about it */
+ for (i = 0; i < count; i++) {
+ offset = le16_to_cpu(xh->xh_entries[i].xe_name_offset);
+ if (offset < namevalue_offset)
+ le16_add_cpu(&xh->xh_entries[i].xe_name_offset,
+ namevalue_size);
+ }
+
+ /*
+ * Note that we don't update xh_free_start or xh_name_value_len
+ * because they're not used in block-stored xattrs.
+ */
+}
+
+/*
+ * Operations for xattrs stored in blocks. This includes inline inode
+ * storage and unindexed ocfs2_xattr_blocks.
+ */
+static const struct ocfs2_xa_loc_operations ocfs2_xa_block_loc_ops = {
+ .xlo_offset_pointer = ocfs2_xa_block_offset_pointer,
+ .xlo_wipe_namevalue = ocfs2_xa_block_wipe_namevalue,
+};
+
+static void *ocfs2_xa_bucket_offset_pointer(struct ocfs2_xa_loc *loc,
+ int offset)
+{
+ struct ocfs2_xattr_bucket *bucket = loc->xl_storage;
+ int block, block_offset;
+
+ BUG_ON(offset >= OCFS2_XATTR_BUCKET_SIZE);
+
+ block = offset >> bucket->bu_inode->i_sb->s_blocksize_bits;
+ block_offset = offset % bucket->bu_inode->i_sb->s_blocksize;
+
+ return bucket_block(bucket, block) + block_offset;
+}
+
+static void ocfs2_xa_bucket_wipe_namevalue(struct ocfs2_xa_loc *loc)
+{
+ int namevalue_size;
+ struct ocfs2_xattr_entry *entry = loc->xl_entry;
+ u64 value_size = le64_to_cpu(entry->xe_value_size);
+
+ namevalue_size = OCFS2_XATTR_SIZE(entry->xe_name_len);
+ if (value_size > OCFS2_XATTR_INLINE_SIZE)
+ namevalue_size += OCFS2_XATTR_ROOT_SIZE;
+ else
+ namevalue_size += OCFS2_XATTR_SIZE(value_size);
+
+ le16_add_cpu(&loc->xl_header->xh_name_value_len, -namevalue_size);
+}
+
+/* Operations for xattrs stored in buckets. */
+static const struct ocfs2_xa_loc_operations ocfs2_xa_bucket_loc_ops = {
+ .xlo_offset_pointer = ocfs2_xa_bucket_offset_pointer,
+ .xlo_wipe_namevalue = ocfs2_xa_bucket_wipe_namevalue,
+};
+
+static void ocfs2_xa_remove_entry(struct ocfs2_xa_loc *loc)
+{
+ ocfs2_xa_wipe_namevalue(loc);
+}
+
+static void ocfs2_init_dinode_xa_loc(struct ocfs2_xa_loc *loc,
+ struct inode *inode,
+ struct buffer_head *bh,
+ struct ocfs2_xattr_entry *entry)
+{
+ struct ocfs2_dinode *di = (struct ocfs2_dinode *)bh->b_data;
+
+ loc->xl_ops = &ocfs2_xa_block_loc_ops;
+ loc->xl_storage = bh;
+ loc->xl_entry = entry;
+
+ if (OCFS2_I(inode)->ip_dyn_features & OCFS2_INLINE_XATTR_FL)
+ loc->xl_size = le16_to_cpu(di->i_xattr_inline_size);
+ else {
+ BUG_ON(entry);
+ loc->xl_size = OCFS2_SB(inode->i_sb)->s_xattr_inline_size;
+ }
+ loc->xl_header =
+ (struct ocfs2_xattr_header *)(bh->b_data + bh->b_size -
+ loc->xl_size);
+}
+
+static void ocfs2_init_xattr_block_xa_loc(struct ocfs2_xa_loc *loc,
+ struct buffer_head *bh,
+ struct ocfs2_xattr_entry *entry)
+{
+ struct ocfs2_xattr_block *xb =
+ (struct ocfs2_xattr_block *)bh->b_data;
+
+ BUG_ON(le16_to_cpu(xb->xb_flags) & OCFS2_XATTR_INDEXED);
+
+ loc->xl_ops = &ocfs2_xa_block_loc_ops;
+ loc->xl_storage = bh;
+ loc->xl_header = &(xb->xb_attrs.xb_header);
+ loc->xl_entry = entry;
+ loc->xl_size = bh->b_size - offsetof(struct ocfs2_xattr_block,
+ xb_attrs.xb_header);
+}
+
+static void ocfs2_init_xattr_bucket_xa_loc(struct ocfs2_xa_loc *loc,
+ struct ocfs2_xattr_bucket *bucket,
+ struct ocfs2_xattr_entry *entry)
+{
+ loc->xl_ops = &ocfs2_xa_bucket_loc_ops;
+ loc->xl_storage = bucket;
+ loc->xl_header = bucket_xh(bucket);
+ loc->xl_entry = entry;
+ loc->xl_size = OCFS2_XATTR_BUCKET_SIZE;
+}
+
+/*
* ocfs2_xattr_set_entry_local()
*
* Set, replace or remove extended attribute in local.
@@ -1436,7 +1646,14 @@ static void ocfs2_xattr_set_entry_local(struct inode *inode,
{
size_t name_len = strlen(xi->name);
int i;
+ struct ocfs2_xa_loc loc;
+ if (xs->xattr_bh == xs->inode_bh)
+ ocfs2_init_dinode_xa_loc(&loc, inode, xs->inode_bh,
+ xs->not_found ? NULL : xs->here);
+ else
+ ocfs2_init_xattr_block_xa_loc(&loc, xs->xattr_bh,
+ xs->not_found ? NULL : xs->here);
if (xi->value && xs->not_found) {
/* Insert the new xattr entry. */
le16_add_cpu(&xs->header->xh_count, 1);
@@ -1475,9 +1692,9 @@ static void ocfs2_xattr_set_entry_local(struct inode *inode,
xi->value_len);
return;
}
+
/* Remove the old name+value. */
- memmove(first_val + size, first_val, val - first_val);
- memset(first_val, 0, size);
+ ocfs2_xa_wipe_namevalue(&loc);
xs->here->xe_name_hash = 0;
xs->here->xe_name_offset = 0;
ocfs2_xattr_set_local(xs->here, 1);
@@ -1485,23 +1702,15 @@ static void ocfs2_xattr_set_entry_local(struct inode *inode,
min_offs += size;
- /* Adjust all value offsets. */
- last = xs->header->xh_entries;
- for (i = 0 ; i < le16_to_cpu(xs->header->xh_count); i++) {
- size_t o = le16_to_cpu(last->xe_name_offset);
-
- if (o < offs)
- last->xe_name_offset = cpu_to_le16(o + size);
- last += 1;
- }
-
if (!xi->value) {
/* Remove the old entry. */
- last -= 1;
+ i = le16_to_cpu(xs->header->xh_count) - 1;
+ last = &xs->header->xh_entries[i];
+ xs->header->xh_count = cpu_to_le16(i);
+
memmove(xs->here, xs->here + 1,
(void *)last - (void *)xs->here);
memset(last, 0, sizeof(struct ocfs2_xattr_entry));
- le16_add_cpu(&xs->header->xh_count, -1);
}
}
if (xi->value) {
@@ -4775,7 +4984,10 @@ static void ocfs2_xattr_set_entry_normal(struct inode *inode,
size_t blocksize = inode->i_sb->s_blocksize;
char *val;
size_t offs, size, new_size;
+ struct ocfs2_xa_loc loc;
+ ocfs2_init_xattr_bucket_xa_loc(&loc, xs->bucket,
+ xs->not_found ? NULL : xs->here);
last = &xh->xh_entries[count];
if (!xs->not_found) {
xe = xs->here;
@@ -4796,7 +5008,7 @@ static void ocfs2_xattr_set_entry_normal(struct inode *inode,
new_size = OCFS2_XATTR_SIZE(name_len) +
OCFS2_XATTR_SIZE(xi->value_len);
- le16_add_cpu(&xh->xh_name_value_len, -size);
+ ocfs2_xa_wipe_namevalue(&loc);
if (xi->value) {
if (new_size > size)
goto set_new_name_value;
--
1.6.3.3
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 02/14] ocfs2: Remove xattrs via ocfs2_xa_loc
2009-08-28 8:35 [Ocfs2-devel] [PATCH 0/14] ocfs2: Unify the setting of extended attributes Joel Becker
2009-08-28 8:35 ` [Ocfs2-devel] [PATCH 01/14] ocfs2: Introduce ocfs2_xa_loc Joel Becker
@ 2009-08-28 8:35 ` Joel Becker
2009-09-01 6:03 ` Tao Ma
2009-08-28 8:35 ` [Ocfs2-devel] [PATCH 03/14] ocfs2: Prefix the member fields of struct ocfs2_xattr_info Joel Becker
` (11 subsequent siblings)
13 siblings, 1 reply; 45+ messages in thread
From: Joel Becker @ 2009-08-28 8:35 UTC (permalink / raw)
To: ocfs2-devel
Add ocfs2_xa_remove_entry(), which will remove an xattr entry from its
storage via the ocfs2_xa_loc descriptor.
Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
fs/ocfs2/xattr.c | 61 ++++++++++++++++++++++++-----------------------------
1 files changed, 28 insertions(+), 33 deletions(-)
diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 848f24e..333d384 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -1580,7 +1580,28 @@ static const struct ocfs2_xa_loc_operations ocfs2_xa_bucket_loc_ops = {
static void ocfs2_xa_remove_entry(struct ocfs2_xa_loc *loc)
{
+ int index, count;
+ struct ocfs2_xattr_header *xh = loc->xl_header;
+ struct ocfs2_xattr_entry *entry = loc->xl_entry;
+
ocfs2_xa_wipe_namevalue(loc);
+
+ le16_add_cpu(&xh->xh_count, -1);
+ count = le16_to_cpu(xh->xh_count);
+
+ /*
+ * Only zero out the entry if there are more remaining. This is
+ * important for an empty bucket, as it keeps track of the
+ * bucket's hash value. It doesn't hurt empty block storage.
+ */
+ if (count) {
+ index = ((char *)entry - (char *)&xh->xh_entries) /
+ sizeof(struct ocfs2_xattr_entry);
+ memmove(&xh->xh_entries[index], &xh->xh_entries[index + 1],
+ (count - index) * sizeof(struct ocfs2_xattr_entry));
+ memset(&xh->xh_entries[count], 0,
+ sizeof(struct ocfs2_xattr_entry));
+ }
}
static void ocfs2_init_dinode_xa_loc(struct ocfs2_xa_loc *loc,
@@ -1645,7 +1666,6 @@ static void ocfs2_xattr_set_entry_local(struct inode *inode,
size_t min_offs)
{
size_t name_len = strlen(xi->name);
- int i;
struct ocfs2_xa_loc loc;
if (xs->xattr_bh == xs->inode_bh)
@@ -1693,25 +1713,12 @@ static void ocfs2_xattr_set_entry_local(struct inode *inode,
return;
}
- /* Remove the old name+value. */
- ocfs2_xa_wipe_namevalue(&loc);
- xs->here->xe_name_hash = 0;
- xs->here->xe_name_offset = 0;
- ocfs2_xattr_set_local(xs->here, 1);
- xs->here->xe_value_size = 0;
+ if (!xi->value)
+ ocfs2_xa_remove_entry(&loc);
+ else
+ ocfs2_xa_wipe_namevalue(&loc);
min_offs += size;
-
- if (!xi->value) {
- /* Remove the old entry. */
- i = le16_to_cpu(xs->header->xh_count) - 1;
- last = &xs->header->xh_entries[i];
- xs->header->xh_count = cpu_to_le16(i);
-
- memmove(xs->here, xs->here + 1,
- (void *)last - (void *)xs->here);
- memset(last, 0, sizeof(struct ocfs2_xattr_entry));
- }
}
if (xi->value) {
/* Insert the new name+value. */
@@ -5008,8 +5015,8 @@ static void ocfs2_xattr_set_entry_normal(struct inode *inode,
new_size = OCFS2_XATTR_SIZE(name_len) +
OCFS2_XATTR_SIZE(xi->value_len);
- ocfs2_xa_wipe_namevalue(&loc);
if (xi->value) {
+ ocfs2_xa_wipe_namevalue(&loc);
if (new_size > size)
goto set_new_name_value;
@@ -5031,20 +5038,8 @@ static void ocfs2_xattr_set_entry_normal(struct inode *inode,
ocfs2_xattr_set_local(xe, local);
return;
} else {
- /*
- * Remove the old entry if there is more than one.
- * We don't remove the last entry so that we can
- * use it to indicate the hash value of the empty
- * bucket.
- */
- last -= 1;
- le16_add_cpu(&xh->xh_count, -1);
- if (xh->xh_count) {
- memmove(xe, xe + 1,
- (void *)last - (void *)xe);
- memset(last, 0,
- sizeof(struct ocfs2_xattr_entry));
- } else
+ ocfs2_xa_remove_entry(&loc);
+ if (!xh->xh_count)
xh->xh_free_start =
cpu_to_le16(OCFS2_XATTR_BUCKET_SIZE);
--
1.6.3.3
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 03/14] ocfs2: Prefix the member fields of struct ocfs2_xattr_info.
2009-08-28 8:35 [Ocfs2-devel] [PATCH 0/14] ocfs2: Unify the setting of extended attributes Joel Becker
2009-08-28 8:35 ` [Ocfs2-devel] [PATCH 01/14] ocfs2: Introduce ocfs2_xa_loc Joel Becker
2009-08-28 8:35 ` [Ocfs2-devel] [PATCH 02/14] ocfs2: Remove xattrs via ocfs2_xa_loc Joel Becker
@ 2009-08-28 8:35 ` Joel Becker
2009-08-28 8:35 ` [Ocfs2-devel] [PATCH 04/14] ocfs2: Add a name_len field to ocfs2_xattr_info Joel Becker
` (10 subsequent siblings)
13 siblings, 0 replies; 45+ messages in thread
From: Joel Becker @ 2009-08-28 8:35 UTC (permalink / raw)
To: ocfs2-devel
struct ocfs2_xattr_info is a useful structure describing an xattr
you'd like to set. Let's put prefixes on the member fields so it's
easier to read and use.
Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
fs/ocfs2/xattr.c | 212 +++++++++++++++++++++++++++--------------------------
1 files changed, 108 insertions(+), 104 deletions(-)
diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 333d384..2fdf809 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -120,10 +120,10 @@ static struct xattr_handler *ocfs2_xattr_handler_map[OCFS2_XATTR_MAX] = {
};
struct ocfs2_xattr_info {
- int name_index;
- const char *name;
- const void *value;
- size_t value_len;
+ int xi_name_index;
+ const char *xi_name;
+ const void *xi_value;
+ size_t xi_value_len;
};
struct ocfs2_xattr_search {
@@ -1367,7 +1367,7 @@ static int ocfs2_xattr_cleanup(struct inode *inode,
size_t offs)
{
int ret = 0;
- size_t name_len = strlen(xi->name);
+ size_t name_len = strlen(xi->xi_name);
void *val = xs->base + offs;
size_t size = OCFS2_XATTR_SIZE(name_len) + OCFS2_XATTR_ROOT_SIZE;
@@ -1407,8 +1407,8 @@ static int ocfs2_xattr_update_entry(struct inode *inode,
}
xs->here->xe_name_offset = cpu_to_le16(offs);
- xs->here->xe_value_size = cpu_to_le64(xi->value_len);
- if (xi->value_len <= OCFS2_XATTR_INLINE_SIZE)
+ xs->here->xe_value_size = cpu_to_le64(xi->xi_value_len);
+ if (xi->xi_value_len <= OCFS2_XATTR_INLINE_SIZE)
ocfs2_xattr_set_local(xs->here, 1);
else
ocfs2_xattr_set_local(xs->here, 0);
@@ -1433,14 +1433,14 @@ static int ocfs2_xattr_set_value_outside(struct inode *inode,
struct ocfs2_xattr_value_buf *vb,
size_t offs)
{
- size_t name_len = strlen(xi->name);
+ size_t name_len = strlen(xi->xi_name);
void *val = xs->base + offs;
struct ocfs2_xattr_value_root *xv = NULL;
size_t size = OCFS2_XATTR_SIZE(name_len) + OCFS2_XATTR_ROOT_SIZE;
int ret = 0;
memset(val, 0, size);
- memcpy(val, xi->name, name_len);
+ memcpy(val, xi->xi_name, name_len);
xv = (struct ocfs2_xattr_value_root *)
(val + OCFS2_XATTR_SIZE(name_len));
xv->xr_clusters = 0;
@@ -1450,7 +1450,7 @@ static int ocfs2_xattr_set_value_outside(struct inode *inode,
xv->xr_list.l_next_free_rec = 0;
vb->vb_xv = xv;
- ret = ocfs2_xattr_value_truncate(inode, vb, xi->value_len, ctxt);
+ ret = ocfs2_xattr_value_truncate(inode, vb, xi->xi_value_len, ctxt);
if (ret < 0) {
mlog_errno(ret);
return ret;
@@ -1461,7 +1461,7 @@ static int ocfs2_xattr_set_value_outside(struct inode *inode,
return ret;
}
ret = __ocfs2_xattr_set_value_outside(inode, ctxt->handle, vb,
- xi->value, xi->value_len);
+ xi->xi_value, xi->xi_value_len);
if (ret < 0)
mlog_errno(ret);
@@ -1665,7 +1665,7 @@ static void ocfs2_xattr_set_entry_local(struct inode *inode,
struct ocfs2_xattr_entry *last,
size_t min_offs)
{
- size_t name_len = strlen(xi->name);
+ size_t name_len = strlen(xi->xi_name);
struct ocfs2_xa_loc loc;
if (xs->xattr_bh == xs->inode_bh)
@@ -1674,10 +1674,10 @@ static void ocfs2_xattr_set_entry_local(struct inode *inode,
else
ocfs2_init_xattr_block_xa_loc(&loc, xs->xattr_bh,
xs->not_found ? NULL : xs->here);
- if (xi->value && xs->not_found) {
+ if (xi->xi_value && xs->not_found) {
/* Insert the new xattr entry. */
le16_add_cpu(&xs->header->xh_count, 1);
- ocfs2_xattr_set_type(last, xi->name_index);
+ ocfs2_xattr_set_type(last, xi->xi_name_index);
ocfs2_xattr_set_local(last, 1);
last->xe_name_len = name_len;
} else {
@@ -1697,42 +1697,42 @@ static void ocfs2_xattr_set_entry_local(struct inode *inode,
size = OCFS2_XATTR_SIZE(name_len) +
OCFS2_XATTR_SIZE(le64_to_cpu(xs->here->xe_value_size));
- if (xi->value && size == OCFS2_XATTR_SIZE(name_len) +
- OCFS2_XATTR_SIZE(xi->value_len)) {
+ if (xi->xi_value && size == OCFS2_XATTR_SIZE(name_len) +
+ OCFS2_XATTR_SIZE(xi->xi_value_len)) {
/* The old and the new value have the
same size. Just replace the value. */
ocfs2_xattr_set_local(xs->here, 1);
- xs->here->xe_value_size = cpu_to_le64(xi->value_len);
+ xs->here->xe_value_size = cpu_to_le64(xi->xi_value_len);
/* Clear value bytes. */
memset(val + OCFS2_XATTR_SIZE(name_len),
0,
- OCFS2_XATTR_SIZE(xi->value_len));
+ OCFS2_XATTR_SIZE(xi->xi_value_len));
memcpy(val + OCFS2_XATTR_SIZE(name_len),
- xi->value,
- xi->value_len);
+ xi->xi_value,
+ xi->xi_value_len);
return;
}
- if (!xi->value)
+ if (!xi->xi_value)
ocfs2_xa_remove_entry(&loc);
else
ocfs2_xa_wipe_namevalue(&loc);
min_offs += size;
}
- if (xi->value) {
+ if (xi->xi_value) {
/* Insert the new name+value. */
size_t size = OCFS2_XATTR_SIZE(name_len) +
- OCFS2_XATTR_SIZE(xi->value_len);
+ OCFS2_XATTR_SIZE(xi->xi_value_len);
void *val = xs->base + min_offs - size;
xs->here->xe_name_offset = cpu_to_le16(min_offs - size);
memset(val, 0, size);
- memcpy(val, xi->name, name_len);
+ memcpy(val, xi->xi_name, name_len);
memcpy(val + OCFS2_XATTR_SIZE(name_len),
- xi->value,
- xi->value_len);
- xs->here->xe_value_size = cpu_to_le64(xi->value_len);
+ xi->xi_value,
+ xi->xi_value_len);
+ xs->here->xe_value_size = cpu_to_le64(xi->xi_value_len);
ocfs2_xattr_set_local(xs->here, 1);
ocfs2_xattr_hash_entry(inode, xs->header, xs->here);
}
@@ -1758,15 +1758,15 @@ 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 min_offs = xs->end - xs->base, name_len = strlen(xi->xi_name);
size_t size_l = 0;
handle_t *handle = ctxt->handle;
int free, i, ret;
struct ocfs2_xattr_info xi_l = {
- .name_index = xi->name_index,
- .name = xi->name,
- .value = xi->value,
- .value_len = xi->value_len,
+ .xi_name_index = xi->xi_name_index,
+ .xi_name = xi->xi_name,
+ .xi_value = xi->xi_value,
+ .xi_value_len = xi->xi_value_len,
};
struct ocfs2_xattr_value_buf vb = {
.vb_bh = xs->xattr_bh,
@@ -1804,7 +1804,7 @@ 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 (xi->xi_value && xi->xi_value_len > OCFS2_XATTR_INLINE_SIZE) {
if (free < sizeof(struct ocfs2_xattr_entry) +
OCFS2_XATTR_SIZE(name_len) +
OCFS2_XATTR_ROOT_SIZE) {
@@ -1812,12 +1812,12 @@ static int ocfs2_xattr_set_entry(struct inode *inode,
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) {
+ xi_l.xi_value = (void *)&def_xv;
+ xi_l.xi_value_len = OCFS2_XATTR_ROOT_SIZE;
+ } else if (xi->xi_value) {
if (free < sizeof(struct ocfs2_xattr_entry) +
OCFS2_XATTR_SIZE(name_len) +
- OCFS2_XATTR_SIZE(xi->value_len)) {
+ OCFS2_XATTR_SIZE(xi->xi_value_len)) {
ret = -ENOSPC;
goto out;
}
@@ -1842,16 +1842,16 @@ static int ocfs2_xattr_set_entry(struct inode *inode,
vb.vb_xv = (struct ocfs2_xattr_value_root *)
(val + OCFS2_XATTR_SIZE(name_len));
- if (xi->value_len > OCFS2_XATTR_INLINE_SIZE) {
+ if (xi->xi_value_len > OCFS2_XATTR_INLINE_SIZE) {
/*
* If new value need set outside also,
* first truncate old value to new value,
* then set new value with set_value_outside().
*/
ret = ocfs2_xattr_value_truncate(inode,
- &vb,
- xi->value_len,
- ctxt);
+ &vb,
+ xi->xi_value_len,
+ ctxt);
if (ret < 0) {
mlog_errno(ret);
goto out;
@@ -1869,10 +1869,10 @@ static int ocfs2_xattr_set_entry(struct inode *inode,
}
ret = __ocfs2_xattr_set_value_outside(inode,
- handle,
- &vb,
- xi->value,
- xi->value_len);
+ handle,
+ &vb,
+ xi->xi_value,
+ xi->xi_value_len);
if (ret < 0)
mlog_errno(ret);
goto out;
@@ -1950,7 +1950,7 @@ static int ocfs2_xattr_set_entry(struct inode *inode,
if (ret < 0)
mlog_errno(ret);
- if (!ret && xi->value_len > OCFS2_XATTR_INLINE_SIZE) {
+ if (!ret && xi->xi_value_len > OCFS2_XATTR_INLINE_SIZE) {
/*
* Set value outside in B tree.
* This is the second step for value size > INLINE_SIZE.
@@ -2616,13 +2616,13 @@ static int ocfs2_xattr_can_be_in_inode(struct inode *inode,
BUG_ON(!xs->not_found);
- if (xi->value_len > OCFS2_XATTR_INLINE_SIZE)
+ if (xi->xi_value_len > OCFS2_XATTR_INLINE_SIZE)
value_size = OCFS2_XATTR_ROOT_SIZE;
else
- value_size = OCFS2_XATTR_SIZE(xi->value_len);
+ value_size = OCFS2_XATTR_SIZE(xi->xi_value_len);
if (free >= sizeof(struct ocfs2_xattr_entry) +
- OCFS2_XATTR_SIZE(strlen(xi->name)) + value_size)
+ OCFS2_XATTR_SIZE(strlen(xi->xi_name)) + value_size)
return 1;
return 0;
@@ -2646,7 +2646,7 @@ static int ocfs2_calc_xattr_set_need(struct inode *inode,
char *base = NULL;
int name_offset, name_len = 0;
u32 new_clusters = ocfs2_clusters_for_bytes(inode->i_sb,
- xi->value_len);
+ xi->xi_value_len);
u64 value_size;
/*
@@ -2654,14 +2654,14 @@ static int ocfs2_calc_xattr_set_need(struct inode *inode,
* No matter whether we replace an old one or add a new one,
* we need this for writing.
*/
- if (xi->value_len > OCFS2_XATTR_INLINE_SIZE)
+ if (xi->xi_value_len > OCFS2_XATTR_INLINE_SIZE)
credits += new_clusters *
ocfs2_clusters_to_blocks(inode->i_sb, 1);
if (xis->not_found && xbs->not_found) {
credits += ocfs2_blocks_per_xattr_bucket(inode->i_sb);
- if (xi->value_len > OCFS2_XATTR_INLINE_SIZE) {
+ if (xi->xi_value_len > OCFS2_XATTR_INLINE_SIZE) {
clusters_add += new_clusters;
credits += ocfs2_calc_extend_credits(inode->i_sb,
&def_xv.xv.xr_list,
@@ -2706,7 +2706,7 @@ static int ocfs2_calc_xattr_set_need(struct inode *inode,
* The credits for removing the value tree will be extended
* by ocfs2_remove_extent itself.
*/
- if (!xi->value) {
+ if (!xi->xi_value) {
if (!ocfs2_xattr_is_local(xe))
credits += ocfs2_remove_extent_credits(inode->i_sb);
@@ -2736,7 +2736,7 @@ static int ocfs2_calc_xattr_set_need(struct inode *inode,
}
}
- if (xi->value_len > OCFS2_XATTR_INLINE_SIZE) {
+ if (xi->xi_value_len > OCFS2_XATTR_INLINE_SIZE) {
/* the new values will be stored outside. */
u32 old_clusters = 0;
@@ -2769,9 +2769,10 @@ static int ocfs2_calc_xattr_set_need(struct inode *inode,
* value, we don't need any allocation, otherwise we have
* to guess metadata allocation.
*/
- if ((ocfs2_xattr_is_local(xe) && value_size >= xi->value_len) ||
+ if ((ocfs2_xattr_is_local(xe) &&
+ (value_size >= xi->xi_value_len)) ||
(!ocfs2_xattr_is_local(xe) &&
- OCFS2_XATTR_ROOT_SIZE >= xi->value_len))
+ OCFS2_XATTR_ROOT_SIZE >= xi->xi_value_len))
goto out;
}
@@ -2861,7 +2862,7 @@ static int ocfs2_init_xattr_set_ctxt(struct inode *inode,
meta_add += extra_meta;
mlog(0, "Set xattr %s, reserve meta blocks = %d, clusters = %d, "
- "credits = %d\n", xi->name, meta_add, clusters_add, *credits);
+ "credits = %d\n", xi->xi_name, meta_add, clusters_add, *credits);
if (meta_add) {
ret = ocfs2_reserve_new_metadata_blocks(osb, meta_add,
@@ -2901,7 +2902,7 @@ static int __ocfs2_xattr_set_handle(struct inode *inode,
{
int ret = 0, credits, old_found;
- if (!xi->value) {
+ if (!xi->xi_value) {
/* Remove existing extended attribute */
if (!xis->not_found)
ret = ocfs2_xattr_ibody_set(inode, xi, xis, ctxt);
@@ -2915,8 +2916,8 @@ static int __ocfs2_xattr_set_handle(struct inode *inode,
* If succeed and that extended attribute existing in
* external block, then we will remove it.
*/
- xi->value = NULL;
- xi->value_len = 0;
+ xi->xi_value = NULL;
+ xi->xi_value_len = 0;
old_found = xis->not_found;
xis->not_found = -ENODATA;
@@ -2944,8 +2945,8 @@ static int __ocfs2_xattr_set_handle(struct inode *inode,
} else if (ret == -ENOSPC) {
if (di->i_xattr_loc && !xbs->xattr_bh) {
ret = ocfs2_xattr_block_find(inode,
- xi->name_index,
- xi->name, xbs);
+ xi->xi_name_index,
+ xi->xi_name, xbs);
if (ret)
goto out;
@@ -2984,8 +2985,8 @@ static int __ocfs2_xattr_set_handle(struct inode *inode,
* If succeed and that extended attribute
* existing in inode, we will remove it.
*/
- xi->value = NULL;
- xi->value_len = 0;
+ xi->xi_value = NULL;
+ xi->xi_value_len = 0;
xbs->not_found = -ENODATA;
ret = ocfs2_calc_xattr_set_need(inode,
di,
@@ -3051,10 +3052,10 @@ int ocfs2_xattr_set_handle(handle_t *handle,
int ret;
struct ocfs2_xattr_info xi = {
- .name_index = name_index,
- .name = name,
- .value = value,
- .value_len = value_len,
+ .xi_name_index = name_index,
+ .xi_name = name,
+ .xi_value = value,
+ .xi_value_len = value_len,
};
struct ocfs2_xattr_search xis = {
@@ -3134,10 +3135,10 @@ int ocfs2_xattr_set(struct inode *inode,
struct ocfs2_refcount_tree *ref_tree = NULL;
struct ocfs2_xattr_info xi = {
- .name_index = name_index,
- .name = name,
- .value = value,
- .value_len = value_len,
+ .xi_name_index = name_index,
+ .xi_name = name,
+ .xi_value = value,
+ .xi_value_len = value_len,
};
struct ocfs2_xattr_search xis = {
@@ -4985,7 +4986,7 @@ static void ocfs2_xattr_set_entry_normal(struct inode *inode,
int local)
{
struct ocfs2_xattr_entry *last, *xe;
- int name_len = strlen(xi->name);
+ int name_len = strlen(xi->xi_name);
struct ocfs2_xattr_header *xh = xs->header;
u16 count = le16_to_cpu(xh->xh_count), start;
size_t blocksize = inode->i_sb->s_blocksize;
@@ -5007,22 +5008,24 @@ static void ocfs2_xattr_set_entry_normal(struct inode *inode,
OCFS2_XATTR_SIZE(OCFS2_XATTR_ROOT_SIZE);
/*
- * If the new value will be stored outside, xi->value has been
- * initalized as an empty ocfs2_xattr_value_root, and the same
- * goes with xi->value_len, so we can set new_size safely here.
+ * If the new value will be stored outside, xi->xi_value has
+ * been initalized as an empty ocfs2_xattr_value_root, and
+ * the same goes with xi->xi_value_len, so we can set
+ * new_size safely here.
* See ocfs2_xattr_set_in_bucket.
*/
new_size = OCFS2_XATTR_SIZE(name_len) +
- OCFS2_XATTR_SIZE(xi->value_len);
+ OCFS2_XATTR_SIZE(xi->xi_value_len);
- if (xi->value) {
+ if (xi->xi_value) {
ocfs2_xa_wipe_namevalue(&loc);
if (new_size > size)
goto set_new_name_value;
/* Now replace the old value with new one. */
if (local)
- xe->xe_value_size = cpu_to_le64(xi->value_len);
+ xe->xe_value_size =
+ cpu_to_le64(xi->xi_value_len);
else
xe->xe_value_size = 0;
@@ -5030,9 +5033,9 @@ static void ocfs2_xattr_set_entry_normal(struct inode *inode,
xs->bucket, offs);
memset(val + OCFS2_XATTR_SIZE(name_len), 0,
size - OCFS2_XATTR_SIZE(name_len));
- if (OCFS2_XATTR_SIZE(xi->value_len) > 0)
+ if (OCFS2_XATTR_SIZE(xi->xi_value_len) > 0)
memcpy(val + OCFS2_XATTR_SIZE(name_len),
- xi->value, xi->value_len);
+ xi->xi_value, xi->xi_value_len);
le16_add_cpu(&xh->xh_name_value_len, new_size);
ocfs2_xattr_set_local(xe, local);
@@ -5073,12 +5076,12 @@ static void ocfs2_xattr_set_entry_normal(struct inode *inode,
memset(xe, 0, sizeof(struct ocfs2_xattr_entry));
xe->xe_name_hash = cpu_to_le32(name_hash);
xe->xe_name_len = name_len;
- ocfs2_xattr_set_type(xe, xi->name_index);
+ ocfs2_xattr_set_type(xe, xi->xi_name_index);
}
set_new_name_value:
/* Insert the new name+value. */
- size = OCFS2_XATTR_SIZE(name_len) + OCFS2_XATTR_SIZE(xi->value_len);
+ size = OCFS2_XATTR_SIZE(name_len) + OCFS2_XATTR_SIZE(xi->xi_value_len);
/*
* We must make sure that the name/value pair
@@ -5097,10 +5100,11 @@ set_new_name_value:
xe->xe_name_offset = cpu_to_le16(offs - size);
memset(val, 0, size);
- memcpy(val, xi->name, name_len);
- memcpy(val + OCFS2_XATTR_SIZE(name_len), xi->value, xi->value_len);
+ memcpy(val, xi->xi_name, name_len);
+ memcpy(val + OCFS2_XATTR_SIZE(name_len), xi->xi_value,
+ xi->xi_value_len);
- xe->xe_value_size = cpu_to_le64(xi->value_len);
+ xe->xe_value_size = cpu_to_le64(xi->xi_value_len);
ocfs2_xattr_set_local(xe, local);
xs->here = xe;
le16_add_cpu(&xh->xh_free_start, -size);
@@ -5125,7 +5129,7 @@ static int ocfs2_xattr_set_entry_in_bucket(struct inode *inode,
u64 blkno;
mlog(0, "Set xattr entry len = %lu index = %d in bucket %llu\n",
- (unsigned long)xi->value_len, xi->name_index,
+ (unsigned long)xi->xi_value_len, xi->xi_name_index,
(unsigned long long)bucket_blkno(xs->bucket));
if (!xs->bucket->bu_bhs[1]) {
@@ -5423,10 +5427,10 @@ static int ocfs2_xattr_set_in_bucket(struct inode *inode,
{
int ret, local = 1;
size_t value_len;
- char *val = (char *)xi->value;
+ char *val = (char *)xi->xi_value;
struct ocfs2_xattr_entry *xe = xs->here;
- u32 name_hash = ocfs2_xattr_name_hash(inode, xi->name,
- strlen(xi->name));
+ u32 name_hash = ocfs2_xattr_name_hash(inode, xi->xi_name,
+ strlen(xi->xi_name));
if (!xs->not_found && !ocfs2_xattr_is_local(xe)) {
/*
@@ -5441,8 +5445,8 @@ static int ocfs2_xattr_set_in_bucket(struct inode *inode,
* the modification to the xattr block will be done
* by following steps.
*/
- if (xi->value_len > OCFS2_XATTR_INLINE_SIZE)
- value_len = xi->value_len;
+ if (xi->xi_value_len > OCFS2_XATTR_INLINE_SIZE)
+ value_len = xi->xi_value_len;
else
value_len = 0;
@@ -5456,7 +5460,7 @@ static int ocfs2_xattr_set_in_bucket(struct inode *inode,
goto set_value_outside;
}
- value_len = xi->value_len;
+ value_len = xi->xi_value_len;
/* So we have to handle the inside block change now. */
if (value_len > OCFS2_XATTR_INLINE_SIZE) {
/*
@@ -5464,8 +5468,8 @@ static int ocfs2_xattr_set_in_bucket(struct inode *inode,
* initalize a new empty value root and insert it first.
*/
local = 0;
- xi->value = &def_xv;
- xi->value_len = OCFS2_XATTR_ROOT_SIZE;
+ xi->xi_value = &def_xv;
+ xi->xi_value_len = OCFS2_XATTR_ROOT_SIZE;
}
ret = ocfs2_xattr_set_entry_in_bucket(inode, ctxt->handle, xi, xs,
@@ -5539,11 +5543,11 @@ static int ocfs2_xattr_set_entry_index_block(struct inode *inode,
struct ocfs2_xattr_entry *xe;
u16 count, header_size, xh_free_start;
int free, max_free, need, old;
- size_t value_size = 0, name_len = strlen(xi->name);
+ size_t value_size = 0, name_len = strlen(xi->xi_name);
size_t blocksize = inode->i_sb->s_blocksize;
int ret, allocation = 0;
- mlog_entry("Set xattr %s in xattr index block\n", xi->name);
+ mlog_entry("Set xattr %s in xattr index block\n", xi->xi_name);
try_again:
xh = xs->header;
@@ -5559,10 +5563,10 @@ try_again:
(unsigned long long)bucket_blkno(xs->bucket),
header_size);
- if (xi->value && xi->value_len > OCFS2_XATTR_INLINE_SIZE)
+ if (xi->xi_value && xi->xi_value_len > OCFS2_XATTR_INLINE_SIZE)
value_size = OCFS2_XATTR_ROOT_SIZE;
- else if (xi->value)
- value_size = OCFS2_XATTR_SIZE(xi->value_len);
+ else if (xi->xi_value)
+ value_size = OCFS2_XATTR_SIZE(xi->xi_value_len);
if (xs->not_found)
need = sizeof(struct ocfs2_xattr_entry) +
@@ -5645,7 +5649,7 @@ try_again:
*/
ret = ocfs2_check_xattr_bucket_collision(inode,
xs->bucket,
- xi->name);
+ xi->xi_name);
if (ret) {
mlog_errno(ret);
goto out;
@@ -5669,8 +5673,8 @@ try_again:
*/
ocfs2_xattr_bucket_relse(xs->bucket);
ret = ocfs2_xattr_index_block_find(inode, xs->xattr_bh,
- xi->name_index,
- xi->name, xs);
+ xi->xi_name_index,
+ xi->xi_name, xs);
if (ret && ret != -ENODATA)
goto out;
xs->not_found = ret;
@@ -5891,7 +5895,7 @@ static int ocfs2_prepare_refcount_xattr(struct inode *inode,
* refcount tree, and make the original extent become 3. So we will need
* 2 * cluster more extent recs at most.
*/
- if (!xi->value || xi->value_len <= OCFS2_XATTR_INLINE_SIZE) {
+ if (!xi->xi_value || xi->xi_value_len <= OCFS2_XATTR_INLINE_SIZE) {
ret = ocfs2_refcounted_xattr_delete_need(inode,
&(*ref_tree)->rf_ci,
--
1.6.3.3
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 04/14] ocfs2: Add a name_len field to ocfs2_xattr_info.
2009-08-28 8:35 [Ocfs2-devel] [PATCH 0/14] ocfs2: Unify the setting of extended attributes Joel Becker
` (2 preceding siblings ...)
2009-08-28 8:35 ` [Ocfs2-devel] [PATCH 03/14] ocfs2: Prefix the member fields of struct ocfs2_xattr_info Joel Becker
@ 2009-08-28 8:35 ` Joel Becker
2009-08-28 8:35 ` [Ocfs2-devel] [PATCH 05/14] ocfs2: Wrap calculation of name+value pair size Joel Becker
` (9 subsequent siblings)
13 siblings, 0 replies; 45+ messages in thread
From: Joel Becker @ 2009-08-28 8:35 UTC (permalink / raw)
To: ocfs2-devel
Rather than calculating strlen all over the place, let's store the
name length directly on ocfs2_xattr_info.
Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
fs/ocfs2/xattr.c | 84 ++++++++++++++++++++++++++++-------------------------
1 files changed, 44 insertions(+), 40 deletions(-)
diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 2fdf809..b92c775 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -122,6 +122,7 @@ static struct xattr_handler *ocfs2_xattr_handler_map[OCFS2_XATTR_MAX] = {
struct ocfs2_xattr_info {
int xi_name_index;
const char *xi_name;
+ int xi_name_len;
const void *xi_value;
size_t xi_value_len;
};
@@ -1367,9 +1368,9 @@ static int ocfs2_xattr_cleanup(struct inode *inode,
size_t offs)
{
int ret = 0;
- size_t name_len = strlen(xi->xi_name);
void *val = xs->base + offs;
- size_t size = OCFS2_XATTR_SIZE(name_len) + OCFS2_XATTR_ROOT_SIZE;
+ size_t size = OCFS2_XATTR_SIZE(xi->xi_name_len) +
+ OCFS2_XATTR_ROOT_SIZE;
ret = vb->vb_access(handle, INODE_CACHE(inode), vb->vb_bh,
OCFS2_JOURNAL_ACCESS_WRITE);
@@ -1433,16 +1434,16 @@ static int ocfs2_xattr_set_value_outside(struct inode *inode,
struct ocfs2_xattr_value_buf *vb,
size_t offs)
{
- size_t name_len = strlen(xi->xi_name);
void *val = xs->base + offs;
struct ocfs2_xattr_value_root *xv = NULL;
- size_t size = OCFS2_XATTR_SIZE(name_len) + OCFS2_XATTR_ROOT_SIZE;
+ size_t size = OCFS2_XATTR_SIZE(xi->xi_name_len) +
+ OCFS2_XATTR_ROOT_SIZE;
int ret = 0;
memset(val, 0, size);
- memcpy(val, xi->xi_name, name_len);
+ memcpy(val, xi->xi_name, xi->xi_name_len);
xv = (struct ocfs2_xattr_value_root *)
- (val + OCFS2_XATTR_SIZE(name_len));
+ (val + OCFS2_XATTR_SIZE(xi->xi_name_len));
xv->xr_clusters = 0;
xv->xr_last_eb_blk = 0;
xv->xr_list.l_tree_depth = 0;
@@ -1665,7 +1666,6 @@ static void ocfs2_xattr_set_entry_local(struct inode *inode,
struct ocfs2_xattr_entry *last,
size_t min_offs)
{
- size_t name_len = strlen(xi->xi_name);
struct ocfs2_xa_loc loc;
if (xs->xattr_bh == xs->inode_bh)
@@ -1679,7 +1679,7 @@ static void ocfs2_xattr_set_entry_local(struct inode *inode,
le16_add_cpu(&xs->header->xh_count, 1);
ocfs2_xattr_set_type(last, xi->xi_name_index);
ocfs2_xattr_set_local(last, 1);
- last->xe_name_len = name_len;
+ last->xe_name_len = xi->xi_name_len;
} else {
void *first_val;
void *val;
@@ -1691,23 +1691,23 @@ static void ocfs2_xattr_set_entry_local(struct inode *inode,
if (le64_to_cpu(xs->here->xe_value_size) >
OCFS2_XATTR_INLINE_SIZE)
- size = OCFS2_XATTR_SIZE(name_len) +
+ size = OCFS2_XATTR_SIZE(xi->xi_name_len) +
OCFS2_XATTR_ROOT_SIZE;
else
- size = OCFS2_XATTR_SIZE(name_len) +
+ size = OCFS2_XATTR_SIZE(xi->xi_name_len) +
OCFS2_XATTR_SIZE(le64_to_cpu(xs->here->xe_value_size));
- if (xi->xi_value && size == OCFS2_XATTR_SIZE(name_len) +
+ if (xi->xi_value && size == OCFS2_XATTR_SIZE(xi->xi_name_len) +
OCFS2_XATTR_SIZE(xi->xi_value_len)) {
/* The old and the new value have the
same size. Just replace the value. */
ocfs2_xattr_set_local(xs->here, 1);
xs->here->xe_value_size = cpu_to_le64(xi->xi_value_len);
/* Clear value bytes. */
- memset(val + OCFS2_XATTR_SIZE(name_len),
+ memset(val + OCFS2_XATTR_SIZE(xi->xi_name_len),
0,
OCFS2_XATTR_SIZE(xi->xi_value_len));
- memcpy(val + OCFS2_XATTR_SIZE(name_len),
+ memcpy(val + OCFS2_XATTR_SIZE(xi->xi_name_len),
xi->xi_value,
xi->xi_value_len);
return;
@@ -1722,14 +1722,14 @@ static void ocfs2_xattr_set_entry_local(struct inode *inode,
}
if (xi->xi_value) {
/* Insert the new name+value. */
- size_t size = OCFS2_XATTR_SIZE(name_len) +
+ size_t size = OCFS2_XATTR_SIZE(xi->xi_name_len) +
OCFS2_XATTR_SIZE(xi->xi_value_len);
void *val = xs->base + min_offs - size;
xs->here->xe_name_offset = cpu_to_le16(min_offs - size);
memset(val, 0, size);
- memcpy(val, xi->xi_name, name_len);
- memcpy(val + OCFS2_XATTR_SIZE(name_len),
+ memcpy(val, xi->xi_name, xi->xi_name_len);
+ memcpy(val + OCFS2_XATTR_SIZE(xi->xi_name_len),
xi->xi_value,
xi->xi_value_len);
xs->here->xe_value_size = cpu_to_le64(xi->xi_value_len);
@@ -1758,13 +1758,14 @@ 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->xi_name);
+ size_t min_offs = xs->end - xs->base;
size_t size_l = 0;
handle_t *handle = ctxt->handle;
int free, i, ret;
struct ocfs2_xattr_info xi_l = {
.xi_name_index = xi->xi_name_index,
.xi_name = xi->xi_name,
+ .xi_name_len = xi->xi_name_len,
.xi_value = xi->xi_value,
.xi_value_len = xi->xi_value_len,
};
@@ -1796,27 +1797,28 @@ static int ocfs2_xattr_set_entry(struct inode *inode,
if (!xs->not_found) {
size_t size = 0;
if (ocfs2_xattr_is_local(xs->here))
- size = OCFS2_XATTR_SIZE(name_len) +
+ size = OCFS2_XATTR_SIZE(xi->xi_name_len) +
OCFS2_XATTR_SIZE(le64_to_cpu(xs->here->xe_value_size));
else
- size = OCFS2_XATTR_SIZE(name_len) +
+ size = OCFS2_XATTR_SIZE(xi->xi_name_len) +
OCFS2_XATTR_ROOT_SIZE;
free += (size + sizeof(struct ocfs2_xattr_entry));
}
/* Check free space in inode or block */
if (xi->xi_value && xi->xi_value_len > OCFS2_XATTR_INLINE_SIZE) {
if (free < sizeof(struct ocfs2_xattr_entry) +
- OCFS2_XATTR_SIZE(name_len) +
+ OCFS2_XATTR_SIZE(xi->xi_name_len) +
OCFS2_XATTR_ROOT_SIZE) {
ret = -ENOSPC;
goto out;
}
- size_l = OCFS2_XATTR_SIZE(name_len) + OCFS2_XATTR_ROOT_SIZE;
+ size_l = OCFS2_XATTR_SIZE(xi->xi_name_len) +
+ OCFS2_XATTR_ROOT_SIZE;
xi_l.xi_value = (void *)&def_xv;
xi_l.xi_value_len = OCFS2_XATTR_ROOT_SIZE;
} else if (xi->xi_value) {
if (free < sizeof(struct ocfs2_xattr_entry) +
- OCFS2_XATTR_SIZE(name_len) +
+ OCFS2_XATTR_SIZE(xi->xi_name_len) +
OCFS2_XATTR_SIZE(xi->xi_value_len)) {
ret = -ENOSPC;
goto out;
@@ -1825,7 +1827,7 @@ static int ocfs2_xattr_set_entry(struct inode *inode,
if (!xs->not_found) {
/* For existing extended attribute */
- size_t size = OCFS2_XATTR_SIZE(name_len) +
+ size_t size = OCFS2_XATTR_SIZE(xi->xi_name_len) +
OCFS2_XATTR_SIZE(le64_to_cpu(xs->here->xe_value_size));
size_t offs = le16_to_cpu(xs->here->xe_name_offset);
void *val = xs->base + offs;
@@ -1840,7 +1842,7 @@ static int ocfs2_xattr_set_entry(struct inode *inode,
} else if (!ocfs2_xattr_is_local(xs->here)) {
/* For existing xattr which has value outside */
vb.vb_xv = (struct ocfs2_xattr_value_root *)
- (val + OCFS2_XATTR_SIZE(name_len));
+ (val + OCFS2_XATTR_SIZE(xi->xi_name_len));
if (xi->xi_value_len > OCFS2_XATTR_INLINE_SIZE) {
/*
@@ -2622,7 +2624,7 @@ static int ocfs2_xattr_can_be_in_inode(struct inode *inode,
value_size = OCFS2_XATTR_SIZE(xi->xi_value_len);
if (free >= sizeof(struct ocfs2_xattr_entry) +
- OCFS2_XATTR_SIZE(strlen(xi->xi_name)) + value_size)
+ OCFS2_XATTR_SIZE(xi->xi_name_len) + value_size)
return 1;
return 0;
@@ -3054,6 +3056,7 @@ int ocfs2_xattr_set_handle(handle_t *handle,
struct ocfs2_xattr_info xi = {
.xi_name_index = name_index,
.xi_name = name,
+ .xi_name_len = strlen(name),
.xi_value = value,
.xi_value_len = value_len,
};
@@ -3137,6 +3140,7 @@ int ocfs2_xattr_set(struct inode *inode,
struct ocfs2_xattr_info xi = {
.xi_name_index = name_index,
.xi_name = name,
+ .xi_name_len = strlen(name),
.xi_value = value,
.xi_value_len = value_len,
};
@@ -4986,7 +4990,6 @@ static void ocfs2_xattr_set_entry_normal(struct inode *inode,
int local)
{
struct ocfs2_xattr_entry *last, *xe;
- int name_len = strlen(xi->xi_name);
struct ocfs2_xattr_header *xh = xs->header;
u16 count = le16_to_cpu(xh->xh_count), start;
size_t blocksize = inode->i_sb->s_blocksize;
@@ -5001,10 +5004,10 @@ static void ocfs2_xattr_set_entry_normal(struct inode *inode,
xe = xs->here;
offs = le16_to_cpu(xe->xe_name_offset);
if (ocfs2_xattr_is_local(xe))
- size = OCFS2_XATTR_SIZE(name_len) +
+ size = OCFS2_XATTR_SIZE(xi->xi_name_len) +
OCFS2_XATTR_SIZE(le64_to_cpu(xe->xe_value_size));
else
- size = OCFS2_XATTR_SIZE(name_len) +
+ size = OCFS2_XATTR_SIZE(xi->xi_name_len) +
OCFS2_XATTR_SIZE(OCFS2_XATTR_ROOT_SIZE);
/*
@@ -5014,7 +5017,7 @@ static void ocfs2_xattr_set_entry_normal(struct inode *inode,
* new_size safely here.
* See ocfs2_xattr_set_in_bucket.
*/
- new_size = OCFS2_XATTR_SIZE(name_len) +
+ new_size = OCFS2_XATTR_SIZE(xi->xi_name_len) +
OCFS2_XATTR_SIZE(xi->xi_value_len);
if (xi->xi_value) {
@@ -5031,10 +5034,10 @@ static void ocfs2_xattr_set_entry_normal(struct inode *inode,
val = ocfs2_xattr_bucket_get_val(inode,
xs->bucket, offs);
- memset(val + OCFS2_XATTR_SIZE(name_len), 0,
- size - OCFS2_XATTR_SIZE(name_len));
+ memset(val + OCFS2_XATTR_SIZE(xi->xi_name_len), 0,
+ size - OCFS2_XATTR_SIZE(xi->xi_name_len));
if (OCFS2_XATTR_SIZE(xi->xi_value_len) > 0)
- memcpy(val + OCFS2_XATTR_SIZE(name_len),
+ memcpy(val + OCFS2_XATTR_SIZE(xi->xi_name_len),
xi->xi_value, xi->xi_value_len);
le16_add_cpu(&xh->xh_name_value_len, new_size);
@@ -5075,13 +5078,14 @@ static void ocfs2_xattr_set_entry_normal(struct inode *inode,
le16_add_cpu(&xh->xh_count, 1);
memset(xe, 0, sizeof(struct ocfs2_xattr_entry));
xe->xe_name_hash = cpu_to_le32(name_hash);
- xe->xe_name_len = name_len;
+ xe->xe_name_len = xi->xi_name_len;
ocfs2_xattr_set_type(xe, xi->xi_name_index);
}
set_new_name_value:
/* Insert the new name+value. */
- size = OCFS2_XATTR_SIZE(name_len) + OCFS2_XATTR_SIZE(xi->xi_value_len);
+ size = OCFS2_XATTR_SIZE(xi->xi_name_len) +
+ OCFS2_XATTR_SIZE(xi->xi_value_len);
/*
* We must make sure that the name/value pair
@@ -5100,8 +5104,8 @@ set_new_name_value:
xe->xe_name_offset = cpu_to_le16(offs - size);
memset(val, 0, size);
- memcpy(val, xi->xi_name, name_len);
- memcpy(val + OCFS2_XATTR_SIZE(name_len), xi->xi_value,
+ memcpy(val, xi->xi_name, xi->xi_name_len);
+ memcpy(val + OCFS2_XATTR_SIZE(xi->xi_name_len), xi->xi_value,
xi->xi_value_len);
xe->xe_value_size = cpu_to_le64(xi->xi_value_len);
@@ -5430,7 +5434,7 @@ static int ocfs2_xattr_set_in_bucket(struct inode *inode,
char *val = (char *)xi->xi_value;
struct ocfs2_xattr_entry *xe = xs->here;
u32 name_hash = ocfs2_xattr_name_hash(inode, xi->xi_name,
- strlen(xi->xi_name));
+ xi->xi_name_len);
if (!xs->not_found && !ocfs2_xattr_is_local(xe)) {
/*
@@ -5543,7 +5547,7 @@ static int ocfs2_xattr_set_entry_index_block(struct inode *inode,
struct ocfs2_xattr_entry *xe;
u16 count, header_size, xh_free_start;
int free, max_free, need, old;
- size_t value_size = 0, name_len = strlen(xi->xi_name);
+ size_t value_size = 0;
size_t blocksize = inode->i_sb->s_blocksize;
int ret, allocation = 0;
@@ -5570,9 +5574,9 @@ try_again:
if (xs->not_found)
need = sizeof(struct ocfs2_xattr_entry) +
- OCFS2_XATTR_SIZE(name_len) + value_size;
+ OCFS2_XATTR_SIZE(xi->xi_name_len) + value_size;
else {
- need = value_size + OCFS2_XATTR_SIZE(name_len);
+ need = value_size + OCFS2_XATTR_SIZE(xi->xi_name_len);
/*
* We only replace the old value if the new length is smaller
--
1.6.3.3
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 05/14] ocfs2: Wrap calculation of name+value pair size.
2009-08-28 8:35 [Ocfs2-devel] [PATCH 0/14] ocfs2: Unify the setting of extended attributes Joel Becker
` (3 preceding siblings ...)
2009-08-28 8:35 ` [Ocfs2-devel] [PATCH 04/14] ocfs2: Add a name_len field to ocfs2_xattr_info Joel Becker
@ 2009-08-28 8:35 ` Joel Becker
2009-08-28 8:35 ` [Ocfs2-devel] [PATCH 06/14] ocfs2: Set the xattr name+value pair in one place Joel Becker
` (8 subsequent siblings)
13 siblings, 0 replies; 45+ messages in thread
From: Joel Becker @ 2009-08-28 8:35 UTC (permalink / raw)
To: ocfs2-devel
An ocfs2 xattr entry stores the text name and value as a pair in the
storage area. Obviously names and values can be variable-sized. If a
value is too large for the entry storage, a tree root is stored instead.
The name+value pair is also padded.
Because of this, there are a million places in the code that do:
if (needs_external_tree(value_size)
namevalue_size = pad(name_size) + tree_root_size;
else
namevalue_size = pad(name_size) + pad(value_size);
Let's create some convenience functions to make the code more readable.
There are three forms. The first takes the raw sizes. The second takes
an ocfs2_xattr_info structure. The third takes an existing
ocfs2_xattr_entry.
Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
fs/ocfs2/xattr.c | 170 +++++++++++++++++++++---------------------------------
1 files changed, 65 insertions(+), 105 deletions(-)
diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index b92c775..4c5c566 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -187,6 +187,33 @@ struct ocfs2_xa_loc {
const struct ocfs2_xa_loc_operations *xl_ops;
};
+/*
+ * Convenience functions to calculate how much space is needed for a
+ * given name+value pair
+ */
+static int namevalue_size(int name_len, uint64_t value_len)
+{
+ if (value_len > OCFS2_XATTR_INLINE_SIZE)
+ return OCFS2_XATTR_SIZE(name_len) + OCFS2_XATTR_ROOT_SIZE;
+ else
+ return OCFS2_XATTR_SIZE(name_len) + OCFS2_XATTR_SIZE(value_len);
+}
+
+static int namevalue_size_xi(struct ocfs2_xattr_info *xi)
+{
+ return namevalue_size(xi->xi_name_len, xi->xi_value_len);
+}
+
+static int namevalue_size_xe(struct ocfs2_xattr_entry *xe)
+{
+ u64 value_len = le64_to_cpu(xe->xe_value_size);
+
+ BUG_ON((value_len > OCFS2_XATTR_INLINE_SIZE) &&
+ ocfs2_xattr_is_local(xe));
+ return namevalue_size(xe->xe_name_len, value_len);
+}
+
+
static int ocfs2_xattr_bucket_get_name_value(struct super_block *sb,
struct ocfs2_xattr_header *xh,
int index,
@@ -535,15 +562,20 @@ static void ocfs2_xattr_hash_entry(struct inode *inode,
static int ocfs2_xattr_entry_real_size(int name_len, size_t value_len)
{
- int size = 0;
+ return namevalue_size(name_len, value_len) +
+ sizeof(struct ocfs2_xattr_entry);
+}
- if (value_len <= OCFS2_XATTR_INLINE_SIZE)
- size = OCFS2_XATTR_SIZE(name_len) + OCFS2_XATTR_SIZE(value_len);
- else
- size = OCFS2_XATTR_SIZE(name_len) + OCFS2_XATTR_ROOT_SIZE;
- size += sizeof(struct ocfs2_xattr_entry);
+static int ocfs2_xi_entry_usage(struct ocfs2_xattr_info *xi)
+{
+ return namevalue_size_xi(xi) +
+ sizeof(struct ocfs2_xattr_entry);
+}
- return size;
+static int ocfs2_xe_entry_usage(struct ocfs2_xattr_entry *xe)
+{
+ return namevalue_size_xe(xe) +
+ sizeof(struct ocfs2_xattr_entry);
}
int ocfs2_calc_security_init(struct inode *dir,
@@ -1369,8 +1401,7 @@ static int ocfs2_xattr_cleanup(struct inode *inode,
{
int ret = 0;
void *val = xs->base + offs;
- size_t size = OCFS2_XATTR_SIZE(xi->xi_name_len) +
- OCFS2_XATTR_ROOT_SIZE;
+ size_t size = namevalue_size_xi(xi);
ret = vb->vb_access(handle, INODE_CACHE(inode), vb->vb_bh,
OCFS2_JOURNAL_ACCESS_WRITE);
@@ -1436,8 +1467,7 @@ static int ocfs2_xattr_set_value_outside(struct inode *inode,
{
void *val = xs->base + offs;
struct ocfs2_xattr_value_root *xv = NULL;
- size_t size = OCFS2_XATTR_SIZE(xi->xi_name_len) +
- OCFS2_XATTR_ROOT_SIZE;
+ size_t size = namevalue_size_xi(xi);
int ret = 0;
memset(val, 0, size);
@@ -1498,15 +1528,10 @@ static void ocfs2_xa_block_wipe_namevalue(struct ocfs2_xa_loc *loc)
int namevalue_offset, first_namevalue_offset, namevalue_size;
struct ocfs2_xattr_entry *entry = loc->xl_entry;
struct ocfs2_xattr_header *xh = loc->xl_header;
- u64 value_size = le64_to_cpu(entry->xe_value_size);
int count = le16_to_cpu(xh->xh_count);
namevalue_offset = le16_to_cpu(entry->xe_name_offset);
- namevalue_size = OCFS2_XATTR_SIZE(entry->xe_name_len);
- if (value_size > OCFS2_XATTR_INLINE_SIZE)
- namevalue_size += OCFS2_XATTR_ROOT_SIZE;
- else
- namevalue_size += OCFS2_XATTR_SIZE(value_size);
+ namevalue_size = namevalue_size_xe(entry);
for (i = 0, first_namevalue_offset = loc->xl_size;
i < count; i++) {
@@ -1560,17 +1585,8 @@ static void *ocfs2_xa_bucket_offset_pointer(struct ocfs2_xa_loc *loc,
static void ocfs2_xa_bucket_wipe_namevalue(struct ocfs2_xa_loc *loc)
{
- int namevalue_size;
- struct ocfs2_xattr_entry *entry = loc->xl_entry;
- u64 value_size = le64_to_cpu(entry->xe_value_size);
-
- namevalue_size = OCFS2_XATTR_SIZE(entry->xe_name_len);
- if (value_size > OCFS2_XATTR_INLINE_SIZE)
- namevalue_size += OCFS2_XATTR_ROOT_SIZE;
- else
- namevalue_size += OCFS2_XATTR_SIZE(value_size);
-
- le16_add_cpu(&loc->xl_header->xh_name_value_len, -namevalue_size);
+ le16_add_cpu(&loc->xl_header->xh_name_value_len,
+ -namevalue_size_xe(loc->xl_entry));
}
/* Operations for xattrs stored in buckets. */
@@ -1689,16 +1705,8 @@ static void ocfs2_xattr_set_entry_local(struct inode *inode,
offs = le16_to_cpu(xs->here->xe_name_offset);
val = xs->base + offs;
- if (le64_to_cpu(xs->here->xe_value_size) >
- OCFS2_XATTR_INLINE_SIZE)
- size = OCFS2_XATTR_SIZE(xi->xi_name_len) +
- OCFS2_XATTR_ROOT_SIZE;
- else
- size = OCFS2_XATTR_SIZE(xi->xi_name_len) +
- OCFS2_XATTR_SIZE(le64_to_cpu(xs->here->xe_value_size));
-
- if (xi->xi_value && size == OCFS2_XATTR_SIZE(xi->xi_name_len) +
- OCFS2_XATTR_SIZE(xi->xi_value_len)) {
+ size = namevalue_size_xe(xs->here);
+ if (xi->xi_value && (size == namevalue_size_xi(xi))) {
/* The old and the new value have the
same size. Just replace the value. */
ocfs2_xattr_set_local(xs->here, 1);
@@ -1722,8 +1730,7 @@ static void ocfs2_xattr_set_entry_local(struct inode *inode,
}
if (xi->xi_value) {
/* Insert the new name+value. */
- size_t size = OCFS2_XATTR_SIZE(xi->xi_name_len) +
- OCFS2_XATTR_SIZE(xi->xi_value_len);
+ size_t size = namevalue_size_xi(xi);
void *val = xs->base + min_offs - size;
xs->here->xe_name_offset = cpu_to_le16(min_offs - size);
@@ -1794,41 +1801,25 @@ static int ocfs2_xattr_set_entry(struct inode *inode,
if (free < 0)
return -EIO;
- if (!xs->not_found) {
- size_t size = 0;
- if (ocfs2_xattr_is_local(xs->here))
- size = OCFS2_XATTR_SIZE(xi->xi_name_len) +
- OCFS2_XATTR_SIZE(le64_to_cpu(xs->here->xe_value_size));
- else
- size = OCFS2_XATTR_SIZE(xi->xi_name_len) +
- OCFS2_XATTR_ROOT_SIZE;
- free += (size + sizeof(struct ocfs2_xattr_entry));
- }
+ if (!xs->not_found)
+ free += ocfs2_xe_entry_usage(xs->here);
+
/* Check free space in inode or block */
- if (xi->xi_value && xi->xi_value_len > OCFS2_XATTR_INLINE_SIZE) {
- if (free < sizeof(struct ocfs2_xattr_entry) +
- OCFS2_XATTR_SIZE(xi->xi_name_len) +
- OCFS2_XATTR_ROOT_SIZE) {
+ if (xi->xi_value) {
+ if (free < ocfs2_xi_entry_usage(xi)) {
ret = -ENOSPC;
goto out;
}
- size_l = OCFS2_XATTR_SIZE(xi->xi_name_len) +
- OCFS2_XATTR_ROOT_SIZE;
- xi_l.xi_value = (void *)&def_xv;
- xi_l.xi_value_len = OCFS2_XATTR_ROOT_SIZE;
- } else if (xi->xi_value) {
- if (free < sizeof(struct ocfs2_xattr_entry) +
- OCFS2_XATTR_SIZE(xi->xi_name_len) +
- OCFS2_XATTR_SIZE(xi->xi_value_len)) {
- ret = -ENOSPC;
- goto out;
+ if (xi->xi_value_len > OCFS2_XATTR_INLINE_SIZE) {
+ size_l = namevalue_size_xi(xi);
+ xi_l.xi_value = (void *)&def_xv;
+ xi_l.xi_value_len = OCFS2_XATTR_ROOT_SIZE;
}
}
if (!xs->not_found) {
/* For existing extended attribute */
- size_t size = OCFS2_XATTR_SIZE(xi->xi_name_len) +
- OCFS2_XATTR_SIZE(le64_to_cpu(xs->here->xe_value_size));
+ size_t size = namevalue_size_xe(xs->here);
size_t offs = le16_to_cpu(xs->here->xe_name_offset);
void *val = xs->base + offs;
@@ -2595,7 +2586,6 @@ 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;
@@ -2618,13 +2608,7 @@ static int ocfs2_xattr_can_be_in_inode(struct inode *inode,
BUG_ON(!xs->not_found);
- if (xi->xi_value_len > OCFS2_XATTR_INLINE_SIZE)
- value_size = OCFS2_XATTR_ROOT_SIZE;
- else
- value_size = OCFS2_XATTR_SIZE(xi->xi_value_len);
-
- if (free >= sizeof(struct ocfs2_xattr_entry) +
- OCFS2_XATTR_SIZE(xi->xi_name_len) + value_size)
+ if (free >= (sizeof(struct ocfs2_xattr_entry) + namevalue_size_xi(xi)))
return 1;
return 0;
@@ -3986,7 +3970,7 @@ static int ocfs2_defrag_xattr_bucket(struct inode *inode,
struct ocfs2_xattr_bucket *bucket)
{
int ret, i;
- size_t end, offset, len, value_len;
+ size_t end, offset, len;
struct ocfs2_xattr_header *xh;
char *entries, *buf, *bucket_buf = NULL;
u64 blkno = bucket_blkno(bucket);
@@ -4040,12 +4024,7 @@ static int ocfs2_defrag_xattr_bucket(struct inode *inode,
end = OCFS2_XATTR_BUCKET_SIZE;
for (i = 0; i < le16_to_cpu(xh->xh_count); i++, xe++) {
offset = le16_to_cpu(xe->xe_name_offset);
- if (ocfs2_xattr_is_local(xe))
- value_len = OCFS2_XATTR_SIZE(
- le64_to_cpu(xe->xe_value_size));
- else
- value_len = OCFS2_XATTR_ROOT_SIZE;
- len = OCFS2_XATTR_SIZE(xe->xe_name_len) + value_len;
+ len = namevalue_size_xe(xe);
/*
* We must make sure that the name/value pair
@@ -4234,7 +4213,7 @@ static int ocfs2_divide_xattr_bucket(struct inode *inode,
int new_bucket_head)
{
int ret, i;
- int count, start, len, name_value_len = 0, xe_len, name_offset = 0;
+ int count, start, len, name_value_len = 0, name_offset = 0;
struct ocfs2_xattr_bucket *s_bucket = NULL, *t_bucket = NULL;
struct ocfs2_xattr_header *xh;
struct ocfs2_xattr_entry *xe;
@@ -4325,13 +4304,7 @@ static int ocfs2_divide_xattr_bucket(struct inode *inode,
name_value_len = 0;
for (i = 0; i < start; i++) {
xe = &xh->xh_entries[i];
- xe_len = OCFS2_XATTR_SIZE(xe->xe_name_len);
- if (ocfs2_xattr_is_local(xe))
- xe_len +=
- OCFS2_XATTR_SIZE(le64_to_cpu(xe->xe_value_size));
- else
- xe_len += OCFS2_XATTR_ROOT_SIZE;
- name_value_len += xe_len;
+ name_value_len += namevalue_size_xe(xe);
if (le16_to_cpu(xe->xe_name_offset) < name_offset)
name_offset = le16_to_cpu(xe->xe_name_offset);
}
@@ -4361,12 +4334,6 @@ static int ocfs2_divide_xattr_bucket(struct inode *inode,
xh->xh_free_start = cpu_to_le16(OCFS2_XATTR_BUCKET_SIZE);
for (i = 0; i < le16_to_cpu(xh->xh_count); i++) {
xe = &xh->xh_entries[i];
- xe_len = OCFS2_XATTR_SIZE(xe->xe_name_len);
- if (ocfs2_xattr_is_local(xe))
- xe_len +=
- OCFS2_XATTR_SIZE(le64_to_cpu(xe->xe_value_size));
- else
- xe_len += OCFS2_XATTR_ROOT_SIZE;
if (le16_to_cpu(xe->xe_name_offset) <
le16_to_cpu(xh->xh_free_start))
xh->xh_free_start = xe->xe_name_offset;
@@ -5003,12 +4970,7 @@ static void ocfs2_xattr_set_entry_normal(struct inode *inode,
if (!xs->not_found) {
xe = xs->here;
offs = le16_to_cpu(xe->xe_name_offset);
- if (ocfs2_xattr_is_local(xe))
- size = OCFS2_XATTR_SIZE(xi->xi_name_len) +
- OCFS2_XATTR_SIZE(le64_to_cpu(xe->xe_value_size));
- else
- size = OCFS2_XATTR_SIZE(xi->xi_name_len) +
- OCFS2_XATTR_SIZE(OCFS2_XATTR_ROOT_SIZE);
+ size = namevalue_size_xe(xe);
/*
* If the new value will be stored outside, xi->xi_value has
@@ -5017,8 +4979,7 @@ static void ocfs2_xattr_set_entry_normal(struct inode *inode,
* new_size safely here.
* See ocfs2_xattr_set_in_bucket.
*/
- new_size = OCFS2_XATTR_SIZE(xi->xi_name_len) +
- OCFS2_XATTR_SIZE(xi->xi_value_len);
+ new_size = namevalue_size_xi(xi);
if (xi->xi_value) {
ocfs2_xa_wipe_namevalue(&loc);
@@ -5084,8 +5045,7 @@ static void ocfs2_xattr_set_entry_normal(struct inode *inode,
set_new_name_value:
/* Insert the new name+value. */
- size = OCFS2_XATTR_SIZE(xi->xi_name_len) +
- OCFS2_XATTR_SIZE(xi->xi_value_len);
+ size = namevalue_size_xi(xi);
/*
* We must make sure that the name/value pair
--
1.6.3.3
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 06/14] ocfs2: Set the xattr name+value pair in one place
2009-08-28 8:35 [Ocfs2-devel] [PATCH 0/14] ocfs2: Unify the setting of extended attributes Joel Becker
` (4 preceding siblings ...)
2009-08-28 8:35 ` [Ocfs2-devel] [PATCH 05/14] ocfs2: Wrap calculation of name+value pair size Joel Becker
@ 2009-08-28 8:35 ` Joel Becker
2009-09-01 7:33 ` Tao Ma
2009-08-28 8:35 ` [Ocfs2-devel] [PATCH 07/14] ocfs2: Handle value tree roots in ocfs2_xa_set_inline_value() Joel Becker
` (7 subsequent siblings)
13 siblings, 1 reply; 45+ messages in thread
From: Joel Becker @ 2009-08-28 8:35 UTC (permalink / raw)
To: ocfs2-devel
We create two new functions on ocfs2_xa_loc, ocfs2_xa_prepare_entry()
and ocfs2_xa_store_inline_value().
ocfs2_xa_prepare_entry() makes sure that the xl_entry field of
ocfs2_xa_loc is ready to receive an xattr. The entry will point to an
appropriately sized name+value region in storage. If an existing entry
can be reused, it will be. If no entry already exists, it will be
allocated. If there isn't space to allocate it, -ENOSPC will be
returned.
ocfs2_xa_store_inline_value() stores the data that goes into the 'value'
part of the name+value pair. For values that don't fit directly, this
stores the value tree root.
A number of operations are added to ocfs2_xa_loc_operations to support
these functions. This reflects the disparate behaviors of xattr blocks
and buckets.
With these functions, the overlapping ocfs2_xattr_set_entry_local() and
ocfs2_xattr_set_entry_normal() can be replaced with a single call
scheme.
Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
fs/ocfs2/xattr.c | 606 ++++++++++++++++++++++++++++++++++--------------------
1 files changed, 386 insertions(+), 220 deletions(-)
diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 4c5c566..3d14c1c 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -151,11 +151,31 @@ struct ocfs2_xa_loc_operations {
*/
void *(*xlo_offset_pointer)(struct ocfs2_xa_loc *loc, int offset);
+ /* Can we reuse the existing entry for the new value? */
+ int (*xlo_can_reuse)(struct ocfs2_xa_loc *loc,
+ struct ocfs2_xattr_info *xi);
+
+ /* How much space is needed for the new value? */
+ int (*xlo_has_space)(struct ocfs2_xa_loc *loc,
+ struct ocfs2_xattr_info *xi);
+
+ /*
+ * Return the offset of the first name+value pair. This is
+ * the start of our downward-filling free space.
+ */
+ int (*xlo_get_free_start)(struct ocfs2_xa_loc *loc);
+
/*
* Remove the name+value at this location. Do whatever is
* appropriate with the remaining name+value pairs.
*/
void (*xlo_wipe_namevalue)(struct ocfs2_xa_loc *loc);
+
+ /* Fill xl_entry with a new entry */
+ void (*xlo_add_entry)(struct ocfs2_xa_loc *loc, u32 name_hash);
+
+ /* Add name+value storage to an entry */
+ void (*xlo_add_namevalue)(struct ocfs2_xa_loc *loc, int size);
};
/*
@@ -1499,6 +1519,29 @@ static int ocfs2_xattr_set_value_outside(struct inode *inode,
return ret;
}
+static int ocfs2_xa_has_space_helper(int needed_space, int free_start,
+ int num_entries)
+{
+ int free_space;
+
+ free_space = free_start -
+ sizeof(struct ocfs2_xattr_header) -
+ (num_entries * sizeof(struct ocfs2_xattr_entry)) -
+ OCFS2_XATTR_HEADER_GAP;
+ if (free_space < 0)
+ return -EIO;
+ if (free_space < needed_space)
+ return -ENOSPC;
+
+ return 0;
+}
+
+/* Give a pointer into the storage for the given offset */
+static void *ocfs2_xa_offset_pointer(struct ocfs2_xa_loc *loc, int offset)
+{
+ return loc->xl_ops->xlo_offset_pointer(loc, offset);
+}
+
/*
* Wipe the name+value pair and allow the storage to reclaim it. This
* must be followed by either removal of the entry or a call to
@@ -1509,6 +1552,60 @@ static void ocfs2_xa_wipe_namevalue(struct ocfs2_xa_loc *loc)
loc->xl_ops->xlo_wipe_namevalue(loc);
}
+/*
+ * Find lowest offset to a name+value pair. This is the start of our
+ * downward-growing free space.
+ */
+static int ocfs2_xa_get_free_start(struct ocfs2_xa_loc *loc)
+{
+ return loc->xl_ops->xlo_get_free_start(loc);
+}
+
+/* Can we reuse loc->xl_entry for xi? */
+static int ocfs2_xa_can_reuse_entry(struct ocfs2_xa_loc *loc,
+ struct ocfs2_xattr_info *xi)
+{
+ return loc->xl_ops->xlo_can_reuse(loc, xi);
+}
+
+/* How much free space is needed to set the new value */
+static int ocfs2_xa_has_space(struct ocfs2_xa_loc *loc,
+ struct ocfs2_xattr_info *xi)
+{
+ return loc->xl_ops->xlo_has_space(loc, xi);
+}
+
+static void ocfs2_xa_add_entry(struct ocfs2_xa_loc *loc, u32 name_hash)
+{
+ loc->xl_ops->xlo_add_entry(loc, name_hash);
+ loc->xl_entry->xe_name_hash = cpu_to_le32(name_hash);
+ /*
+ * We can't leave the new entry's xe_name_offset at zero or
+ * add_namevalue() will go nuts. We set it to the size of our
+ * storage so that it can never be less than any other entry.
+ */
+ loc->xl_entry->xe_name_offset = cpu_to_le16(loc->xl_size);
+}
+
+static void ocfs2_xa_add_namevalue(struct ocfs2_xa_loc *loc,
+ struct ocfs2_xattr_info *xi)
+{
+ int size = namevalue_size_xi(xi);
+ int nameval_offset;
+ char *nameval_buf;
+
+ loc->xl_ops->xlo_add_namevalue(loc, size);
+ loc->xl_entry->xe_value_size = cpu_to_le64(xi->xi_value_len);
+ ocfs2_xattr_set_type(loc->xl_entry, xi->xi_name_index);
+ ocfs2_xattr_set_local(loc->xl_entry,
+ xi->xi_value_len <= OCFS2_XATTR_INLINE_SIZE);
+
+ nameval_offset = le16_to_cpu(loc->xl_entry->xe_name_offset);
+ nameval_buf = ocfs2_xa_offset_pointer(loc, nameval_offset);
+ memset(nameval_buf, 0, size);
+ memcpy(nameval_buf, xi->xi_name, xi->xi_name_len);
+}
+
static void *ocfs2_xa_block_offset_pointer(struct ocfs2_xa_loc *loc,
int offset)
{
@@ -1518,6 +1615,56 @@ static void *ocfs2_xa_block_offset_pointer(struct ocfs2_xa_loc *loc,
return bh->b_data + offset;
}
+static int ocfs2_xa_block_can_reuse(struct ocfs2_xa_loc *loc,
+ struct ocfs2_xattr_info *xi)
+{
+ /*
+ * Block storage is strict. If the sizes aren't exact, we will
+ * remove the old one and reinsert the new.
+ */
+ return namevalue_size_xe(loc->xl_entry) ==
+ namevalue_size_xi(xi);
+}
+
+static int ocfs2_xa_block_get_free_start(struct ocfs2_xa_loc *loc)
+{
+ struct ocfs2_xattr_header *xh = loc->xl_header;
+ int i, count = le16_to_cpu(xh->xh_count);
+ int offset, free_start = loc->xl_size;
+
+ for (i = 0; i < count; i++) {
+ offset = le16_to_cpu(xh->xh_entries[i].xe_name_offset);
+ if (offset < free_start)
+ free_start = offset;
+ }
+
+ return free_start;
+}
+
+static int ocfs2_xa_block_has_space(struct ocfs2_xa_loc *loc,
+ struct ocfs2_xattr_info *xi)
+{
+ int count = le16_to_cpu(loc->xl_header->xh_count);
+ int free_start = ocfs2_xa_get_free_start(loc);
+ int needed_space = ocfs2_xi_entry_usage(xi);
+
+ /*
+ * Block storage will reclaim the original entry before inserting
+ * the new value, so we only need the difference. If the new
+ * entry is smaller than the old one, we don't need anything.
+ */
+ if (loc->xl_entry) {
+ /* Don't need space if we're reusing! */
+ if (ocfs2_xa_can_reuse_entry(loc, xi))
+ needed_space = 0;
+ else
+ needed_space -= ocfs2_xe_entry_usage(loc->xl_entry);
+ }
+ if (needed_space < 0)
+ needed_space = 0;
+ return ocfs2_xa_has_space_helper(needed_space, free_start, count);
+}
+
/*
* Block storage for xattrs keeps the name+value pairs compacted. When
* we remove one, we have to shift any that preceded it towards the end.
@@ -1532,13 +1679,7 @@ static void ocfs2_xa_block_wipe_namevalue(struct ocfs2_xa_loc *loc)
namevalue_offset = le16_to_cpu(entry->xe_name_offset);
namevalue_size = namevalue_size_xe(entry);
-
- for (i = 0, first_namevalue_offset = loc->xl_size;
- i < count; i++) {
- offset = le16_to_cpu(xh->xh_entries[i].xe_name_offset);
- if (offset < first_namevalue_offset)
- first_namevalue_offset = offset;
- }
+ first_namevalue_offset = ocfs2_xa_get_free_start(loc);
/* Shift the name+value pairs */
memmove((char *)xh + first_namevalue_offset + namevalue_size,
@@ -1560,13 +1701,33 @@ static void ocfs2_xa_block_wipe_namevalue(struct ocfs2_xa_loc *loc)
*/
}
+static void ocfs2_xa_block_add_entry(struct ocfs2_xa_loc *loc, u32 name_hash)
+{
+ int count = le16_to_cpu(loc->xl_header->xh_count);
+ loc->xl_entry = &(loc->xl_header->xh_entries[count]);
+ le16_add_cpu(&loc->xl_header->xh_count, 1);
+ memset(loc->xl_entry, 0, sizeof(struct ocfs2_xattr_entry));
+}
+
+static void ocfs2_xa_block_add_namevalue(struct ocfs2_xa_loc *loc, int size)
+{
+ int free_start = ocfs2_xa_get_free_start(loc);
+
+ loc->xl_entry->xe_name_offset = cpu_to_le16(free_start - size);
+}
+
/*
* Operations for xattrs stored in blocks. This includes inline inode
* storage and unindexed ocfs2_xattr_blocks.
*/
static const struct ocfs2_xa_loc_operations ocfs2_xa_block_loc_ops = {
.xlo_offset_pointer = ocfs2_xa_block_offset_pointer,
+ .xlo_has_space = ocfs2_xa_block_has_space,
+ .xlo_can_reuse = ocfs2_xa_block_can_reuse,
+ .xlo_get_free_start = ocfs2_xa_block_get_free_start,
.xlo_wipe_namevalue = ocfs2_xa_block_wipe_namevalue,
+ .xlo_add_entry = ocfs2_xa_block_add_entry,
+ .xlo_add_namevalue = ocfs2_xa_block_add_namevalue,
};
static void *ocfs2_xa_bucket_offset_pointer(struct ocfs2_xa_loc *loc,
@@ -1583,16 +1744,128 @@ static void *ocfs2_xa_bucket_offset_pointer(struct ocfs2_xa_loc *loc,
return bucket_block(bucket, block) + block_offset;
}
+static int ocfs2_xa_bucket_can_reuse(struct ocfs2_xa_loc *loc,
+ struct ocfs2_xattr_info *xi)
+{
+ return namevalue_size_xe(loc->xl_entry) >=
+ namevalue_size_xi(xi);
+}
+
+static int ocfs2_xa_bucket_get_free_start(struct ocfs2_xa_loc *loc)
+{
+ struct ocfs2_xattr_bucket *bucket = loc->xl_storage;
+ return le16_to_cpu(bucket_xh(bucket)->xh_free_start);
+}
+
+static int ocfs2_bucket_align_free_start(struct super_block *sb,
+ int free_start, int size)
+{
+ /*
+ * We need to make sure that the name+value pair fits within
+ * one block.
+ */
+ if (((free_start - size) >> sb->s_blocksize_bits) !=
+ ((free_start - 1) >> sb->s_blocksize_bits))
+ free_start -= free_start % sb->s_blocksize;
+
+ return free_start;
+}
+
+static int ocfs2_xa_bucket_has_space(struct ocfs2_xa_loc *loc,
+ struct ocfs2_xattr_info *xi)
+{
+ int count = le16_to_cpu(loc->xl_header->xh_count);
+ int free_start = ocfs2_xa_get_free_start(loc);
+ int needed_space = ocfs2_xi_entry_usage(xi);
+ int size = namevalue_size_xi(xi);
+ struct ocfs2_xattr_bucket *bucket = loc->xl_storage;
+ struct super_block *sb = bucket->bu_inode->i_sb;
+
+ /*
+ * Bucket storage does not reclaim name+value pairs it cannot
+ * reuse. They live as holes until the bucket fills, and then
+ * the bucket is defragmented. However, the bucket can reclaim
+ * the ocfs2_xattr_entry.
+ */
+ if (loc->xl_entry) {
+ /* Don't need space if we're reusing! */
+ if (ocfs2_xa_can_reuse_entry(loc, xi))
+ needed_space = 0;
+ else
+ needed_space -= sizeof(struct ocfs2_xattr_entry);
+ }
+ BUG_ON(needed_space < 0);
+
+ free_start = ocfs2_bucket_align_free_start(sb, free_start, size);
+ return ocfs2_xa_has_space_helper(needed_space, free_start, count);
+}
+
static void ocfs2_xa_bucket_wipe_namevalue(struct ocfs2_xa_loc *loc)
{
le16_add_cpu(&loc->xl_header->xh_name_value_len,
-namevalue_size_xe(loc->xl_entry));
}
+static void ocfs2_xa_bucket_add_entry(struct ocfs2_xa_loc *loc, u32 name_hash)
+{
+ struct ocfs2_xattr_header *xh = loc->xl_header;
+ int count = le16_to_cpu(xh->xh_count);
+ int low = 0, high = count - 1, tmp;
+ struct ocfs2_xattr_entry *tmp_xe;
+
+ /*
+ * We keep buckets sorted by name_hash, so we need to find
+ * our insert place.
+ */
+ while (low <= high && count) {
+ tmp = (low + high) / 2;
+ tmp_xe = &xh->xh_entries[tmp];
+
+ if (name_hash > le32_to_cpu(tmp_xe->xe_name_hash))
+ low = tmp + 1;
+ else if (name_hash < le32_to_cpu(tmp_xe->xe_name_hash))
+ high = tmp - 1;
+ else {
+ low = tmp;
+ break;
+ }
+ }
+
+ if (low != count)
+ memmove(&xh->xh_entries[low + 1],
+ &xh->xh_entries[low],
+ ((count - low) * sizeof(struct ocfs2_xattr_entry)));
+
+ le16_add_cpu(&xh->xh_count, 1);
+ loc->xl_entry = &xh->xh_entries[low];
+ memset(loc->xl_entry, 0, sizeof(struct ocfs2_xattr_entry));
+}
+
+static void ocfs2_xa_bucket_add_namevalue(struct ocfs2_xa_loc *loc, int size)
+{
+ int free_start = ocfs2_xa_get_free_start(loc);
+ struct ocfs2_xattr_header *xh = loc->xl_header;
+ struct ocfs2_xattr_bucket *bucket = loc->xl_storage;
+ struct super_block *sb = bucket->bu_inode->i_sb;
+ int nameval_offset;
+
+ free_start = ocfs2_bucket_align_free_start(sb, free_start, size);
+ nameval_offset = free_start - size;
+ loc->xl_entry->xe_name_offset = cpu_to_le16(nameval_offset);
+ xh->xh_free_start = cpu_to_le16(nameval_offset);
+ le16_add_cpu(&xh->xh_name_value_len, size);
+
+}
+
/* Operations for xattrs stored in buckets. */
static const struct ocfs2_xa_loc_operations ocfs2_xa_bucket_loc_ops = {
.xlo_offset_pointer = ocfs2_xa_bucket_offset_pointer,
+ .xlo_has_space = ocfs2_xa_bucket_has_space,
+ .xlo_can_reuse = ocfs2_xa_bucket_can_reuse,
+ .xlo_get_free_start = ocfs2_xa_bucket_get_free_start,
.xlo_wipe_namevalue = ocfs2_xa_bucket_wipe_namevalue,
+ .xlo_add_entry = ocfs2_xa_bucket_add_entry,
+ .xlo_add_namevalue = ocfs2_xa_bucket_add_namevalue,
};
static void ocfs2_xa_remove_entry(struct ocfs2_xa_loc *loc)
@@ -1621,6 +1894,74 @@ static void ocfs2_xa_remove_entry(struct ocfs2_xa_loc *loc)
}
}
+/*
+ * Prepares loc->xl_entry to receive the new xattr. This includes
+ * properly setting up the name+value pair region. If loc->xl_entry
+ * already exists, it will take care of modifying it appropriately.
+ * This also includes deleting entries, but don't call this to remove
+ * a non-existant entry. That's just a bug.
+ *
+ * Note that this modifies the data. You did journal_access already,
+ * right?
+ */
+static int ocfs2_xa_prepare_entry(struct ocfs2_xa_loc *loc,
+ struct ocfs2_xattr_info *xi,
+ u32 name_hash)
+{
+ int rc = 0;
+ int name_size = OCFS2_XATTR_SIZE(xi->xi_name_len);
+ char *nameval_buf;
+
+ if (!xi->xi_value) {
+ ocfs2_xa_remove_entry(loc);
+ goto out;
+ }
+
+ rc = ocfs2_xa_has_space(loc, xi);
+ if (rc)
+ goto out;
+
+ if (loc->xl_entry) {
+ if (ocfs2_xa_can_reuse_entry(loc, xi)) {
+ nameval_buf = ocfs2_xa_offset_pointer(loc,
+ le16_to_cpu(loc->xl_entry->xe_name_offset));
+ memset(nameval_buf + name_size, 0,
+ namevalue_size_xe(loc->xl_entry) - name_size);
+ loc->xl_entry->xe_value_size =
+ cpu_to_le64(xi->xi_value_len);
+ goto out;
+ }
+
+ ocfs2_xa_wipe_namevalue(loc);
+ } else
+ ocfs2_xa_add_entry(loc, name_hash);
+
+ /*
+ * If we get here, we have a blank entry. Fill it. We grow our
+ * name+value pair back from the end.
+ */
+ ocfs2_xa_add_namevalue(loc, xi);
+
+out:
+ return rc;
+}
+
+/*
+ * Store the value portion of the name+value pair. This is either an
+ * inline value or the tree root of an external value.
+ */
+static void ocfs2_xa_store_inline_value(struct ocfs2_xa_loc *loc,
+ struct ocfs2_xattr_info *xi)
+{
+ int nameval_offset = le16_to_cpu(loc->xl_entry->xe_name_offset);
+ int name_size = OCFS2_XATTR_SIZE(xi->xi_name_len);
+ int size = namevalue_size_xi(xi);
+ char *nameval_buf;
+
+ nameval_buf = ocfs2_xa_offset_pointer(loc, nameval_offset);
+ memcpy(nameval_buf + name_size, xi->xi_value, size - name_size);
+}
+
static void ocfs2_init_dinode_xa_loc(struct ocfs2_xa_loc *loc,
struct inode *inode,
struct buffer_head *bh,
@@ -1671,81 +2012,6 @@ static void ocfs2_init_xattr_bucket_xa_loc(struct ocfs2_xa_loc *loc,
loc->xl_size = OCFS2_XATTR_BUCKET_SIZE;
}
-/*
- * ocfs2_xattr_set_entry_local()
- *
- * Set, replace or remove extended attribute in local.
- */
-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_xa_loc loc;
-
- if (xs->xattr_bh == xs->inode_bh)
- ocfs2_init_dinode_xa_loc(&loc, inode, xs->inode_bh,
- xs->not_found ? NULL : xs->here);
- else
- ocfs2_init_xattr_block_xa_loc(&loc, xs->xattr_bh,
- xs->not_found ? NULL : xs->here);
- if (xi->xi_value && xs->not_found) {
- /* Insert the new xattr entry. */
- le16_add_cpu(&xs->header->xh_count, 1);
- ocfs2_xattr_set_type(last, xi->xi_name_index);
- ocfs2_xattr_set_local(last, 1);
- last->xe_name_len = xi->xi_name_len;
- } else {
- void *first_val;
- void *val;
- size_t offs, size;
-
- first_val = xs->base + min_offs;
- offs = le16_to_cpu(xs->here->xe_name_offset);
- val = xs->base + offs;
-
- size = namevalue_size_xe(xs->here);
- if (xi->xi_value && (size == namevalue_size_xi(xi))) {
- /* The old and the new value have the
- same size. Just replace the value. */
- ocfs2_xattr_set_local(xs->here, 1);
- xs->here->xe_value_size = cpu_to_le64(xi->xi_value_len);
- /* Clear value bytes. */
- memset(val + OCFS2_XATTR_SIZE(xi->xi_name_len),
- 0,
- OCFS2_XATTR_SIZE(xi->xi_value_len));
- memcpy(val + OCFS2_XATTR_SIZE(xi->xi_name_len),
- xi->xi_value,
- xi->xi_value_len);
- return;
- }
-
- if (!xi->xi_value)
- ocfs2_xa_remove_entry(&loc);
- else
- ocfs2_xa_wipe_namevalue(&loc);
-
- min_offs += size;
- }
- if (xi->xi_value) {
- /* Insert the new name+value. */
- size_t size = namevalue_size_xi(xi);
- void *val = xs->base + min_offs - size;
-
- xs->here->xe_name_offset = cpu_to_le16(min_offs - size);
- memset(val, 0, size);
- memcpy(val, xi->xi_name, xi->xi_name_len);
- memcpy(val + OCFS2_XATTR_SIZE(xi->xi_name_len),
- xi->xi_value,
- xi->xi_value_len);
- xs->here->xe_value_size = cpu_to_le64(xi->xi_value_len);
- ocfs2_xattr_set_local(xs->here, 1);
- ocfs2_xattr_hash_entry(inode, xs->header, xs->here);
- }
-
- return;
-}
/*
* ocfs2_xattr_set_entry()
@@ -1753,7 +2019,7 @@ static void ocfs2_xattr_set_entry_local(struct inode *inode,
* Set extended attribute entry into inode or block.
*
* If extended attribute value size > OCFS2_XATTR_INLINE_SIZE,
- * We first insert tree root(ocfs2_xattr_value_root) with set_entry_local(),
+ * We first insert tree root(ocfs2_xattr_value_root) like a normal value,
* then set value in B tree with set_value_outside().
*/
static int ocfs2_xattr_set_entry(struct inode *inode,
@@ -1769,6 +2035,9 @@ static int ocfs2_xattr_set_entry(struct inode *inode,
size_t size_l = 0;
handle_t *handle = ctxt->handle;
int free, i, ret;
+ u32 name_hash = ocfs2_xattr_name_hash(inode, xi->xi_name,
+ xi->xi_name_len);
+ struct ocfs2_xa_loc loc;
struct ocfs2_xattr_info xi_l = {
.xi_name_index = xi->xi_name_index,
.xi_name = xi->xi_name,
@@ -1900,11 +2169,28 @@ static int ocfs2_xattr_set_entry(struct inode *inode,
}
}
+ if (xs->xattr_bh == xs->inode_bh)
+ ocfs2_init_dinode_xa_loc(&loc, inode, xs->inode_bh,
+ xs->not_found ? NULL : xs->here);
+ else
+ ocfs2_init_xattr_block_xa_loc(&loc, xs->xattr_bh,
+ xs->not_found ? NULL : xs->here);
+
/*
- * Set value in local, include set tree root in local.
- * This is the first step for value size >INLINE_SIZE.
+ * Prepare our entry and insert the inline value. This will
+ * be a value tree root for values that are larger than
+ * OCFS2_XATTR_INLINE_SIZE.
*/
- ocfs2_xattr_set_entry_local(inode, &xi_l, xs, last, min_offs);
+ ret = ocfs2_xa_prepare_entry(&loc, xi, name_hash);
+ if (ret) {
+ if (ret != -ENOSPC)
+ mlog_errno(ret);
+ goto out;
+ }
+ /* XXX For now, until we make ocfs2_xa_prepare_entry() primary */
+ BUG_ON(ret == -ENOSPC);
+ ocfs2_xa_store_inline_value(&loc, xi);
+ xs->here = loc.xl_entry;
if (!(flag & OCFS2_INLINE_XATTR_FL)) {
ret = ocfs2_journal_dirty(handle, xs->xattr_bh);
@@ -4945,139 +5231,6 @@ static inline char *ocfs2_xattr_bucket_get_val(struct inode *inode,
}
/*
- * Handle the normal xattr set, including replace, delete and new.
- *
- * Note: "local" indicates the real data's locality. So we can't
- * just its bucket locality by its length.
- */
-static void ocfs2_xattr_set_entry_normal(struct inode *inode,
- struct ocfs2_xattr_info *xi,
- struct ocfs2_xattr_search *xs,
- u32 name_hash,
- int local)
-{
- struct ocfs2_xattr_entry *last, *xe;
- struct ocfs2_xattr_header *xh = xs->header;
- u16 count = le16_to_cpu(xh->xh_count), start;
- size_t blocksize = inode->i_sb->s_blocksize;
- char *val;
- size_t offs, size, new_size;
- struct ocfs2_xa_loc loc;
-
- ocfs2_init_xattr_bucket_xa_loc(&loc, xs->bucket,
- xs->not_found ? NULL : xs->here);
- last = &xh->xh_entries[count];
- if (!xs->not_found) {
- xe = xs->here;
- offs = le16_to_cpu(xe->xe_name_offset);
- size = namevalue_size_xe(xe);
-
- /*
- * If the new value will be stored outside, xi->xi_value has
- * been initalized as an empty ocfs2_xattr_value_root, and
- * the same goes with xi->xi_value_len, so we can set
- * new_size safely here.
- * See ocfs2_xattr_set_in_bucket.
- */
- new_size = namevalue_size_xi(xi);
-
- if (xi->xi_value) {
- ocfs2_xa_wipe_namevalue(&loc);
- if (new_size > size)
- goto set_new_name_value;
-
- /* Now replace the old value with new one. */
- if (local)
- xe->xe_value_size =
- cpu_to_le64(xi->xi_value_len);
- else
- xe->xe_value_size = 0;
-
- val = ocfs2_xattr_bucket_get_val(inode,
- xs->bucket, offs);
- memset(val + OCFS2_XATTR_SIZE(xi->xi_name_len), 0,
- size - OCFS2_XATTR_SIZE(xi->xi_name_len));
- if (OCFS2_XATTR_SIZE(xi->xi_value_len) > 0)
- memcpy(val + OCFS2_XATTR_SIZE(xi->xi_name_len),
- xi->xi_value, xi->xi_value_len);
-
- le16_add_cpu(&xh->xh_name_value_len, new_size);
- ocfs2_xattr_set_local(xe, local);
- return;
- } else {
- ocfs2_xa_remove_entry(&loc);
- if (!xh->xh_count)
- xh->xh_free_start =
- cpu_to_le16(OCFS2_XATTR_BUCKET_SIZE);
-
- return;
- }
- } else {
- /* find a new entry for insert. */
- int low = 0, high = count - 1, tmp;
- struct ocfs2_xattr_entry *tmp_xe;
-
- while (low <= high && count) {
- tmp = (low + high) / 2;
- tmp_xe = &xh->xh_entries[tmp];
-
- if (name_hash > le32_to_cpu(tmp_xe->xe_name_hash))
- low = tmp + 1;
- else if (name_hash <
- le32_to_cpu(tmp_xe->xe_name_hash))
- high = tmp - 1;
- else {
- low = tmp;
- break;
- }
- }
-
- xe = &xh->xh_entries[low];
- if (low != count)
- memmove(xe + 1, xe, (void *)last - (void *)xe);
-
- le16_add_cpu(&xh->xh_count, 1);
- memset(xe, 0, sizeof(struct ocfs2_xattr_entry));
- xe->xe_name_hash = cpu_to_le32(name_hash);
- xe->xe_name_len = xi->xi_name_len;
- ocfs2_xattr_set_type(xe, xi->xi_name_index);
- }
-
-set_new_name_value:
- /* Insert the new name+value. */
- size = namevalue_size_xi(xi);
-
- /*
- * We must make sure that the name/value pair
- * exists in the same block.
- */
- offs = le16_to_cpu(xh->xh_free_start);
- start = offs - size;
-
- if (start >> inode->i_sb->s_blocksize_bits !=
- (offs - 1) >> inode->i_sb->s_blocksize_bits) {
- offs = offs - offs % blocksize;
- xh->xh_free_start = cpu_to_le16(offs);
- }
-
- val = ocfs2_xattr_bucket_get_val(inode, xs->bucket, offs - size);
- xe->xe_name_offset = cpu_to_le16(offs - size);
-
- memset(val, 0, size);
- memcpy(val, xi->xi_name, xi->xi_name_len);
- memcpy(val + OCFS2_XATTR_SIZE(xi->xi_name_len), xi->xi_value,
- xi->xi_value_len);
-
- xe->xe_value_size = cpu_to_le64(xi->xi_value_len);
- ocfs2_xattr_set_local(xe, local);
- xs->here = xe;
- le16_add_cpu(&xh->xh_free_start, -size);
- le16_add_cpu(&xh->xh_name_value_len, size);
-
- return;
-}
-
-/*
* Set the xattr entry in the specified bucket.
* The bucket is indicated by xs->bucket and it should have the enough
* space for the xattr insertion.
@@ -5091,6 +5244,7 @@ static int ocfs2_xattr_set_entry_in_bucket(struct inode *inode,
{
int ret;
u64 blkno;
+ struct ocfs2_xa_loc loc;
mlog(0, "Set xattr entry len = %lu index = %d in bucket %llu\n",
(unsigned long)xi->xi_value_len, xi->xi_name_index,
@@ -5113,7 +5267,19 @@ static int ocfs2_xattr_set_entry_in_bucket(struct inode *inode,
goto out;
}
- ocfs2_xattr_set_entry_normal(inode, xi, xs, name_hash, local);
+ ocfs2_init_xattr_bucket_xa_loc(&loc, xs->bucket,
+ xs->not_found ? NULL : xs->here);
+ ret = ocfs2_xa_prepare_entry(&loc, xi, name_hash);
+ if (ret) {
+ if (ret != -ENOSPC)
+ mlog_errno(ret);
+ goto out;
+ }
+ /* XXX For now, until we make ocfs2_xa_prepare_entry() primary */
+ BUG_ON(ret == -ENOSPC);
+ ocfs2_xa_store_inline_value(&loc, xi);
+ xs->here = loc.xl_entry;
+
ocfs2_xattr_bucket_journal_dirty(handle, xs->bucket);
out:
--
1.6.3.3
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 07/14] ocfs2: Handle value tree roots in ocfs2_xa_set_inline_value()
2009-08-28 8:35 [Ocfs2-devel] [PATCH 0/14] ocfs2: Unify the setting of extended attributes Joel Becker
` (5 preceding siblings ...)
2009-08-28 8:35 ` [Ocfs2-devel] [PATCH 06/14] ocfs2: Set the xattr name+value pair in one place Joel Becker
@ 2009-08-28 8:35 ` Joel Becker
2009-08-28 8:35 ` [Ocfs2-devel] [PATCH 08/14] ocfs2: Provide ocfs2_xa_fill_value_buf() for external value processing Joel Becker
` (6 subsequent siblings)
13 siblings, 0 replies; 45+ messages in thread
From: Joel Becker @ 2009-08-28 8:35 UTC (permalink / raw)
To: ocfs2-devel
Previously the xattr code would send in a fake value, containing a tree
root, to the function that installed name+value pairs. Instead, we pass
the real value to ocfs2_xa_set_inline_value(), and it notices that the
value cannot fit. Thus, it installs a tree root.
Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
fs/ocfs2/xattr.c | 54 ++++++++++++++++--------------------------------------
1 files changed, 16 insertions(+), 38 deletions(-)
diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 3d14c1c..4e64b4c 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -1955,11 +1955,16 @@ static void ocfs2_xa_store_inline_value(struct ocfs2_xa_loc *loc,
{
int nameval_offset = le16_to_cpu(loc->xl_entry->xe_name_offset);
int name_size = OCFS2_XATTR_SIZE(xi->xi_name_len);
- int size = namevalue_size_xi(xi);
+ int inline_value_size = namevalue_size_xi(xi) - name_size;
+ const void *value = xi->xi_value;
char *nameval_buf;
+ if (xi->xi_value_len > OCFS2_XATTR_INLINE_SIZE) {
+ value = &def_xv;
+ inline_value_size = OCFS2_XATTR_ROOT_SIZE;
+ }
nameval_buf = ocfs2_xa_offset_pointer(loc, nameval_offset);
- memcpy(nameval_buf + name_size, xi->xi_value, size - name_size);
+ memcpy(nameval_buf + name_size, value, inline_value_size);
}
static void ocfs2_init_dinode_xa_loc(struct ocfs2_xa_loc *loc,
@@ -2038,13 +2043,6 @@ static int ocfs2_xattr_set_entry(struct inode *inode,
u32 name_hash = ocfs2_xattr_name_hash(inode, xi->xi_name,
xi->xi_name_len);
struct ocfs2_xa_loc loc;
- struct ocfs2_xattr_info xi_l = {
- .xi_name_index = xi->xi_name_index,
- .xi_name = xi->xi_name,
- .xi_name_len = xi->xi_name_len,
- .xi_value = xi->xi_value,
- .xi_value_len = xi->xi_value_len,
- };
struct ocfs2_xattr_value_buf vb = {
.vb_bh = xs->xattr_bh,
.vb_access = ocfs2_journal_access_di,
@@ -2074,16 +2072,9 @@ static int ocfs2_xattr_set_entry(struct inode *inode,
free += ocfs2_xe_entry_usage(xs->here);
/* Check free space in inode or block */
- if (xi->xi_value) {
- if (free < ocfs2_xi_entry_usage(xi)) {
- ret = -ENOSPC;
- goto out;
- }
- if (xi->xi_value_len > OCFS2_XATTR_INLINE_SIZE) {
- size_l = namevalue_size_xi(xi);
- xi_l.xi_value = (void *)&def_xv;
- xi_l.xi_value_len = OCFS2_XATTR_ROOT_SIZE;
- }
+ if (xi->xi_value && (free < ocfs2_xi_entry_usage(xi))) {
+ ret = -ENOSPC;
+ goto out;
}
if (!xs->not_found) {
@@ -5239,8 +5230,7 @@ static int ocfs2_xattr_set_entry_in_bucket(struct inode *inode,
handle_t *handle,
struct ocfs2_xattr_info *xi,
struct ocfs2_xattr_search *xs,
- u32 name_hash,
- int local)
+ u32 name_hash)
{
int ret;
u64 blkno;
@@ -5555,13 +5545,14 @@ static int ocfs2_xattr_set_in_bucket(struct inode *inode,
struct ocfs2_xattr_search *xs,
struct ocfs2_xattr_set_ctxt *ctxt)
{
- int ret, local = 1;
+ int ret;
size_t value_len;
char *val = (char *)xi->xi_value;
struct ocfs2_xattr_entry *xe = xs->here;
u32 name_hash = ocfs2_xattr_name_hash(inode, xi->xi_name,
xi->xi_name_len);
+ value_len = xi->xi_value_len;
if (!xs->not_found && !ocfs2_xattr_is_local(xe)) {
/*
* We need to truncate the xattr storage first.
@@ -5575,9 +5566,7 @@ static int ocfs2_xattr_set_in_bucket(struct inode *inode,
* the modification to the xattr block will be done
* by following steps.
*/
- if (xi->xi_value_len > OCFS2_XATTR_INLINE_SIZE)
- value_len = xi->xi_value_len;
- else
+ if (xi->xi_value_len <= OCFS2_XATTR_INLINE_SIZE)
value_len = 0;
ret = ocfs2_xattr_bucket_value_truncate_xs(inode, xs,
@@ -5590,26 +5579,15 @@ static int ocfs2_xattr_set_in_bucket(struct inode *inode,
goto set_value_outside;
}
- value_len = xi->xi_value_len;
/* So we have to handle the inside block change now. */
- if (value_len > OCFS2_XATTR_INLINE_SIZE) {
- /*
- * If the new value will be stored outside of block,
- * initalize a new empty value root and insert it first.
- */
- local = 0;
- xi->xi_value = &def_xv;
- xi->xi_value_len = OCFS2_XATTR_ROOT_SIZE;
- }
-
ret = ocfs2_xattr_set_entry_in_bucket(inode, ctxt->handle, xi, xs,
- name_hash, local);
+ name_hash);
if (ret) {
mlog_errno(ret);
goto out;
}
- if (value_len <= OCFS2_XATTR_INLINE_SIZE)
+ if (xi->xi_value_len <= OCFS2_XATTR_INLINE_SIZE)
goto out;
/* allocate the space now for the outside block storage. */
--
1.6.3.3
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 08/14] ocfs2: Provide ocfs2_xa_fill_value_buf() for external value processing
2009-08-28 8:35 [Ocfs2-devel] [PATCH 0/14] ocfs2: Unify the setting of extended attributes Joel Becker
` (6 preceding siblings ...)
2009-08-28 8:35 ` [Ocfs2-devel] [PATCH 07/14] ocfs2: Handle value tree roots in ocfs2_xa_set_inline_value() Joel Becker
@ 2009-08-28 8:35 ` Joel Becker
2009-08-28 8:35 ` [Ocfs2-devel] [PATCH 09/14] ocfs2: Teach ocfs2_xa_loc how to do its own journal work Joel Becker
` (5 subsequent siblings)
13 siblings, 0 replies; 45+ messages in thread
From: Joel Becker @ 2009-08-28 8:35 UTC (permalink / raw)
To: ocfs2-devel
We use the ocfs2_xattr_value_buf structure to manage external values.
It lets the value tree code do its work regardless of the containing
storage. ocfs2_xa_fill_value_buf() initializes a value buf from an
ocfs2_xa_loc entry.
Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
fs/ocfs2/xattr.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 59 insertions(+), 0 deletions(-)
diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 4e64b4c..cb5357e 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -176,6 +176,13 @@ struct ocfs2_xa_loc_operations {
/* Add name+value storage to an entry */
void (*xlo_add_namevalue)(struct ocfs2_xa_loc *loc, int size);
+
+ /*
+ * Initialize the value buf's access and bh fields for this entry.
+ * ocfs2_xa_fill_value_buf() will handle the xv pointer.
+ */
+ void (*xlo_fill_value_buf)(struct ocfs2_xa_loc *loc,
+ struct ocfs2_xattr_value_buf *vb);
};
/*
@@ -1606,6 +1613,23 @@ static void ocfs2_xa_add_namevalue(struct ocfs2_xa_loc *loc,
memcpy(nameval_buf, xi->xi_name, xi->xi_name_len);
}
+static void ocfs2_xa_fill_value_buf(struct ocfs2_xa_loc *loc,
+ struct ocfs2_xattr_value_buf *vb)
+{
+ int nameval_offset = le16_to_cpu(loc->xl_entry->xe_name_offset);
+ int name_size = OCFS2_XATTR_SIZE(loc->xl_entry->xe_name_len);
+
+ /* Value bufs are for value trees */
+ BUG_ON(namevalue_size_xe(loc->xl_entry) !=
+ (name_size + OCFS2_XATTR_ROOT_SIZE));
+
+ loc->xl_ops->xlo_fill_value_buf(loc, vb);
+ vb->vb_xv =
+ (struct ocfs2_xattr_value_root *)ocfs2_xa_offset_pointer(loc,
+ nameval_offset +
+ name_size);
+}
+
static void *ocfs2_xa_block_offset_pointer(struct ocfs2_xa_loc *loc,
int offset)
{
@@ -1716,6 +1740,20 @@ static void ocfs2_xa_block_add_namevalue(struct ocfs2_xa_loc *loc, int size)
loc->xl_entry->xe_name_offset = cpu_to_le16(free_start - size);
}
+static void ocfs2_xa_block_fill_value_buf(struct ocfs2_xa_loc *loc,
+ struct ocfs2_xattr_value_buf *vb)
+{
+ struct buffer_head *bh = loc->xl_storage;
+
+ if (loc->xl_size == (bh->b_size -
+ offsetof(struct ocfs2_xattr_block,
+ xb_attrs.xb_header)))
+ vb->vb_access = ocfs2_journal_access_xb;
+ else
+ vb->vb_access = ocfs2_journal_access_di;
+ vb->vb_bh = bh;
+}
+
/*
* Operations for xattrs stored in blocks. This includes inline inode
* storage and unindexed ocfs2_xattr_blocks.
@@ -1728,6 +1766,7 @@ static const struct ocfs2_xa_loc_operations ocfs2_xa_block_loc_ops = {
.xlo_wipe_namevalue = ocfs2_xa_block_wipe_namevalue,
.xlo_add_entry = ocfs2_xa_block_add_entry,
.xlo_add_namevalue = ocfs2_xa_block_add_namevalue,
+ .xlo_fill_value_buf = ocfs2_xa_block_fill_value_buf,
};
static void *ocfs2_xa_bucket_offset_pointer(struct ocfs2_xa_loc *loc,
@@ -1857,6 +1896,25 @@ static void ocfs2_xa_bucket_add_namevalue(struct ocfs2_xa_loc *loc, int size)
}
+static void ocfs2_xa_bucket_fill_value_buf(struct ocfs2_xa_loc *loc,
+ struct ocfs2_xattr_value_buf *vb)
+{
+ struct ocfs2_xattr_bucket *bucket = loc->xl_storage;
+ struct super_block *sb = bucket->bu_inode->i_sb;
+ int nameval_offset = le16_to_cpu(loc->xl_entry->xe_name_offset);
+ int size = namevalue_size_xe(loc->xl_entry);
+ int block_offset = nameval_offset >> sb->s_blocksize_bits;
+
+ /* Values are not allowed to straddle block boundaries */
+ BUG_ON(block_offset !=
+ ((nameval_offset + size - 1) >> sb->s_blocksize_bits));
+ /* We expect the bucket to be filled in */
+ BUG_ON(!bucket->bu_bhs[block_offset]);
+
+ vb->vb_access = ocfs2_journal_access;
+ vb->vb_bh = bucket->bu_bhs[block_offset];
+}
+
/* Operations for xattrs stored in buckets. */
static const struct ocfs2_xa_loc_operations ocfs2_xa_bucket_loc_ops = {
.xlo_offset_pointer = ocfs2_xa_bucket_offset_pointer,
@@ -1866,6 +1924,7 @@ static const struct ocfs2_xa_loc_operations ocfs2_xa_bucket_loc_ops = {
.xlo_wipe_namevalue = ocfs2_xa_bucket_wipe_namevalue,
.xlo_add_entry = ocfs2_xa_bucket_add_entry,
.xlo_add_namevalue = ocfs2_xa_bucket_add_namevalue,
+ .xlo_fill_value_buf = ocfs2_xa_bucket_fill_value_buf,
};
static void ocfs2_xa_remove_entry(struct ocfs2_xa_loc *loc)
--
1.6.3.3
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 09/14] ocfs2: Teach ocfs2_xa_loc how to do its own journal work
2009-08-28 8:35 [Ocfs2-devel] [PATCH 0/14] ocfs2: Unify the setting of extended attributes Joel Becker
` (7 preceding siblings ...)
2009-08-28 8:35 ` [Ocfs2-devel] [PATCH 08/14] ocfs2: Provide ocfs2_xa_fill_value_buf() for external value processing Joel Becker
@ 2009-08-28 8:35 ` Joel Becker
2009-08-28 8:35 ` [Ocfs2-devel] [PATCH 10/14] ocfs2: Allocation in ocfs2_xa_prepare_entry() values in ocfs2_xa_store_value() Joel Becker
` (4 subsequent siblings)
13 siblings, 0 replies; 45+ messages in thread
From: Joel Becker @ 2009-08-28 8:35 UTC (permalink / raw)
To: ocfs2-devel
We're going to want to make sure our buffers get accessed and dirtied
correctly. So have the xa_loc do the work. This includes storing the
inode on ocfs2_xa_loc.
Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
fs/ocfs2/xattr.c | 115 ++++++++++++++++++++++++++++++++++++++++--------------
1 files changed, 86 insertions(+), 29 deletions(-)
diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index cb5357e..1bc12fa 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -146,6 +146,13 @@ struct ocfs2_xattr_search {
struct ocfs2_xa_loc;
struct ocfs2_xa_loc_operations {
/*
+ * Journal functions
+ */
+ int (*xlo_journal_access)(handle_t *handle, struct ocfs2_xa_loc *loc,
+ int type);
+ void (*xlo_journal_dirty)(handle_t *handle, struct ocfs2_xa_loc *loc);
+
+ /*
* Return a pointer to the appropriate buffer in loc->xl_storage
* at the given offset from loc->xl_header.
*/
@@ -190,6 +197,9 @@ struct ocfs2_xa_loc_operations {
* tracking the on-disk structure.
*/
struct ocfs2_xa_loc {
+ /* This xattr belongs to this inode */
+ struct inode *xl_inode;
+
/* The ocfs2_xattr_header inside the on-disk storage. Not NULL. */
struct ocfs2_xattr_header *xl_header;
@@ -1543,6 +1553,17 @@ static int ocfs2_xa_has_space_helper(int needed_space, int free_start,
return 0;
}
+static int ocfs2_xa_journal_access(handle_t *handle, struct ocfs2_xa_loc *loc,
+ int type)
+{
+ return loc->xl_ops->xlo_journal_access(handle, loc, type);
+}
+
+static void ocfs2_xa_journal_dirty(handle_t *handle, struct ocfs2_xa_loc *loc)
+{
+ loc->xl_ops->xlo_journal_dirty(handle, loc);
+}
+
/* Give a pointer into the storage for the given offset */
static void *ocfs2_xa_offset_pointer(struct ocfs2_xa_loc *loc, int offset)
{
@@ -1630,6 +1651,29 @@ static void ocfs2_xa_fill_value_buf(struct ocfs2_xa_loc *loc,
name_size);
}
+static int ocfs2_xa_block_journal_access(handle_t *handle,
+ struct ocfs2_xa_loc *loc, int type)
+{
+ struct buffer_head *bh = loc->xl_storage;
+ ocfs2_journal_access_func access;
+
+ if (loc->xl_size == (bh->b_size -
+ offsetof(struct ocfs2_xattr_block,
+ xb_attrs.xb_header)))
+ access = ocfs2_journal_access_xb;
+ else
+ access = ocfs2_journal_access_di;
+ return access(handle, INODE_CACHE(loc->xl_inode), bh, type);
+}
+
+static void ocfs2_xa_block_journal_dirty(handle_t *handle,
+ struct ocfs2_xa_loc *loc)
+{
+ struct buffer_head *bh = loc->xl_storage;
+
+ ocfs2_journal_dirty(handle, bh);
+}
+
static void *ocfs2_xa_block_offset_pointer(struct ocfs2_xa_loc *loc,
int offset)
{
@@ -1759,6 +1803,8 @@ static void ocfs2_xa_block_fill_value_buf(struct ocfs2_xa_loc *loc,
* storage and unindexed ocfs2_xattr_blocks.
*/
static const struct ocfs2_xa_loc_operations ocfs2_xa_block_loc_ops = {
+ .xlo_journal_access = ocfs2_xa_block_journal_access,
+ .xlo_journal_dirty = ocfs2_xa_block_journal_dirty,
.xlo_offset_pointer = ocfs2_xa_block_offset_pointer,
.xlo_has_space = ocfs2_xa_block_has_space,
.xlo_can_reuse = ocfs2_xa_block_can_reuse,
@@ -1769,6 +1815,22 @@ static const struct ocfs2_xa_loc_operations ocfs2_xa_block_loc_ops = {
.xlo_fill_value_buf = ocfs2_xa_block_fill_value_buf,
};
+static int ocfs2_xa_bucket_journal_access(handle_t *handle,
+ struct ocfs2_xa_loc *loc, int type)
+{
+ struct ocfs2_xattr_bucket *bucket = loc->xl_storage;
+
+ return ocfs2_xattr_bucket_journal_access(handle, bucket, type);
+}
+
+static void ocfs2_xa_bucket_journal_dirty(handle_t *handle,
+ struct ocfs2_xa_loc *loc)
+{
+ struct ocfs2_xattr_bucket *bucket = loc->xl_storage;
+
+ ocfs2_xattr_bucket_journal_dirty(handle, bucket);
+}
+
static void *ocfs2_xa_bucket_offset_pointer(struct ocfs2_xa_loc *loc,
int offset)
{
@@ -1777,8 +1839,8 @@ static void *ocfs2_xa_bucket_offset_pointer(struct ocfs2_xa_loc *loc,
BUG_ON(offset >= OCFS2_XATTR_BUCKET_SIZE);
- block = offset >> bucket->bu_inode->i_sb->s_blocksize_bits;
- block_offset = offset % bucket->bu_inode->i_sb->s_blocksize;
+ block = offset >> loc->xl_inode->i_sb->s_blocksize_bits;
+ block_offset = offset % loc->xl_inode->i_sb->s_blocksize;
return bucket_block(bucket, block) + block_offset;
}
@@ -1817,8 +1879,7 @@ static int ocfs2_xa_bucket_has_space(struct ocfs2_xa_loc *loc,
int free_start = ocfs2_xa_get_free_start(loc);
int needed_space = ocfs2_xi_entry_usage(xi);
int size = namevalue_size_xi(xi);
- struct ocfs2_xattr_bucket *bucket = loc->xl_storage;
- struct super_block *sb = bucket->bu_inode->i_sb;
+ struct super_block *sb = loc->xl_inode->i_sb;
/*
* Bucket storage does not reclaim name+value pairs it cannot
@@ -1884,8 +1945,7 @@ static void ocfs2_xa_bucket_add_namevalue(struct ocfs2_xa_loc *loc, int size)
{
int free_start = ocfs2_xa_get_free_start(loc);
struct ocfs2_xattr_header *xh = loc->xl_header;
- struct ocfs2_xattr_bucket *bucket = loc->xl_storage;
- struct super_block *sb = bucket->bu_inode->i_sb;
+ struct super_block *sb = loc->xl_inode->i_sb;
int nameval_offset;
free_start = ocfs2_bucket_align_free_start(sb, free_start, size);
@@ -1900,7 +1960,7 @@ static void ocfs2_xa_bucket_fill_value_buf(struct ocfs2_xa_loc *loc,
struct ocfs2_xattr_value_buf *vb)
{
struct ocfs2_xattr_bucket *bucket = loc->xl_storage;
- struct super_block *sb = bucket->bu_inode->i_sb;
+ struct super_block *sb = loc->xl_inode->i_sb;
int nameval_offset = le16_to_cpu(loc->xl_entry->xe_name_offset);
int size = namevalue_size_xe(loc->xl_entry);
int block_offset = nameval_offset >> sb->s_blocksize_bits;
@@ -1917,6 +1977,8 @@ static void ocfs2_xa_bucket_fill_value_buf(struct ocfs2_xa_loc *loc,
/* Operations for xattrs stored in buckets. */
static const struct ocfs2_xa_loc_operations ocfs2_xa_bucket_loc_ops = {
+ .xlo_journal_access = ocfs2_xa_bucket_journal_access,
+ .xlo_journal_dirty = ocfs2_xa_bucket_journal_dirty,
.xlo_offset_pointer = ocfs2_xa_bucket_offset_pointer,
.xlo_has_space = ocfs2_xa_bucket_has_space,
.xlo_can_reuse = ocfs2_xa_bucket_can_reuse,
@@ -2033,6 +2095,7 @@ static void ocfs2_init_dinode_xa_loc(struct ocfs2_xa_loc *loc,
{
struct ocfs2_dinode *di = (struct ocfs2_dinode *)bh->b_data;
+ loc->xl_inode = inode;
loc->xl_ops = &ocfs2_xa_block_loc_ops;
loc->xl_storage = bh;
loc->xl_entry = entry;
@@ -2049,6 +2112,7 @@ static void ocfs2_init_dinode_xa_loc(struct ocfs2_xa_loc *loc,
}
static void ocfs2_init_xattr_block_xa_loc(struct ocfs2_xa_loc *loc,
+ struct inode *inode,
struct buffer_head *bh,
struct ocfs2_xattr_entry *entry)
{
@@ -2057,6 +2121,7 @@ static void ocfs2_init_xattr_block_xa_loc(struct ocfs2_xa_loc *loc,
BUG_ON(le16_to_cpu(xb->xb_flags) & OCFS2_XATTR_INDEXED);
+ loc->xl_inode = inode;
loc->xl_ops = &ocfs2_xa_block_loc_ops;
loc->xl_storage = bh;
loc->xl_header = &(xb->xb_attrs.xb_header);
@@ -2069,6 +2134,7 @@ static void ocfs2_init_xattr_bucket_xa_loc(struct ocfs2_xa_loc *loc,
struct ocfs2_xattr_bucket *bucket,
struct ocfs2_xattr_entry *entry)
{
+ loc->xl_inode = bucket->bu_inode;
loc->xl_ops = &ocfs2_xa_bucket_loc_ops;
loc->xl_storage = bucket;
loc->xl_header = bucket_xh(bucket);
@@ -2210,21 +2276,18 @@ static int ocfs2_xattr_set_entry(struct inode *inode,
goto out;
}
- if (!(flag & OCFS2_INLINE_XATTR_FL)) {
- ret = vb.vb_access(handle, INODE_CACHE(inode), vb.vb_bh,
- OCFS2_JOURNAL_ACCESS_WRITE);
- if (ret) {
- mlog_errno(ret);
- goto out;
- }
- }
-
if (xs->xattr_bh == xs->inode_bh)
ocfs2_init_dinode_xa_loc(&loc, inode, xs->inode_bh,
xs->not_found ? NULL : xs->here);
else
- ocfs2_init_xattr_block_xa_loc(&loc, xs->xattr_bh,
+ ocfs2_init_xattr_block_xa_loc(&loc, inode, xs->xattr_bh,
xs->not_found ? NULL : xs->here);
+ ret = ocfs2_xa_journal_access(handle, &loc,
+ OCFS2_JOURNAL_ACCESS_WRITE);
+ if (ret) {
+ mlog_errno(ret);
+ goto out;
+ }
/*
* Prepare our entry and insert the inline value. This will
@@ -2242,13 +2305,7 @@ static int ocfs2_xattr_set_entry(struct inode *inode,
ocfs2_xa_store_inline_value(&loc, xi);
xs->here = loc.xl_entry;
- if (!(flag & OCFS2_INLINE_XATTR_FL)) {
- ret = ocfs2_journal_dirty(handle, xs->xattr_bh);
- if (ret < 0) {
- mlog_errno(ret);
- goto out;
- }
- }
+ ocfs2_xa_journal_dirty(handle, &loc);
if (!(oi->ip_dyn_features & OCFS2_INLINE_XATTR_FL) &&
(flag & OCFS2_INLINE_XATTR_FL)) {
@@ -5309,15 +5366,15 @@ static int ocfs2_xattr_set_entry_in_bucket(struct inode *inode,
}
}
- ret = ocfs2_xattr_bucket_journal_access(handle, xs->bucket,
- OCFS2_JOURNAL_ACCESS_WRITE);
+ ocfs2_init_xattr_bucket_xa_loc(&loc, xs->bucket,
+ xs->not_found ? NULL : xs->here);
+ ret = ocfs2_xa_journal_access(handle, &loc,
+ OCFS2_JOURNAL_ACCESS_WRITE);
if (ret < 0) {
mlog_errno(ret);
goto out;
}
- ocfs2_init_xattr_bucket_xa_loc(&loc, xs->bucket,
- xs->not_found ? NULL : xs->here);
ret = ocfs2_xa_prepare_entry(&loc, xi, name_hash);
if (ret) {
if (ret != -ENOSPC)
@@ -5329,7 +5386,7 @@ static int ocfs2_xattr_set_entry_in_bucket(struct inode *inode,
ocfs2_xa_store_inline_value(&loc, xi);
xs->here = loc.xl_entry;
- ocfs2_xattr_bucket_journal_dirty(handle, xs->bucket);
+ ocfs2_xa_journal_dirty(handle, &loc);
out:
return ret;
--
1.6.3.3
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 10/14] ocfs2: Allocation in ocfs2_xa_prepare_entry() values in ocfs2_xa_store_value()
2009-08-28 8:35 [Ocfs2-devel] [PATCH 0/14] ocfs2: Unify the setting of extended attributes Joel Becker
` (8 preceding siblings ...)
2009-08-28 8:35 ` [Ocfs2-devel] [PATCH 09/14] ocfs2: Teach ocfs2_xa_loc how to do its own journal work Joel Becker
@ 2009-08-28 8:35 ` Joel Becker
2009-09-01 8:55 ` Tao Ma
2009-08-28 8:36 ` [Ocfs2-devel] [PATCH 11/14] ocfs2: Gell into ocfs2_xa_set() Joel Becker
` (3 subsequent siblings)
13 siblings, 1 reply; 45+ messages in thread
From: Joel Becker @ 2009-08-28 8:35 UTC (permalink / raw)
To: ocfs2-devel
ocfs2_xa_prepare_entry() gets all the logic to add, remove, or modify
external value trees. Now, when it exits, the entry is ready to receive
a value of any size.
ocfs2_xa_store_inline_value() becomes ocfs2_xa_store_value(). It can
store any value.
ocfs2_xattr_set_entry() loses all the allocation logic and just uses
these functions. ocfs2_xattr_set_value_outside() disappears.
ocfs2_xattr_set_in_bucket() uses these functions and makes
ocfs2_xattr_set_entry_in_bucket() obsolete. That goes away, as does
ocfs2_xattr_bucket_set_value_outside() and
ocfs2_xattr_bucket_value_truncate().
Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
fs/ocfs2/xattr.c | 611 ++++++++++++------------------------------------------
1 files changed, 138 insertions(+), 473 deletions(-)
diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 1bc12fa..d922ad9 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -579,24 +579,6 @@ static u32 ocfs2_xattr_name_hash(struct inode *inode,
return hash;
}
-/*
- * ocfs2_xattr_hash_entry()
- *
- * Compute the hash of an extended attribute.
- */
-static void ocfs2_xattr_hash_entry(struct inode *inode,
- struct ocfs2_xattr_header *header,
- struct ocfs2_xattr_entry *entry)
-{
- u32 hash = 0;
- char *name = (char *)header + le16_to_cpu(entry->xe_name_offset);
-
- hash = ocfs2_xattr_name_hash(inode, name, entry->xe_name_len);
- entry->xe_name_hash = cpu_to_le32(hash);
-
- return;
-}
-
static int ocfs2_xattr_entry_real_size(int name_len, size_t value_len)
{
return namevalue_size(name_len, value_len) +
@@ -1429,113 +1411,6 @@ out:
return ret;
}
-static int ocfs2_xattr_cleanup(struct inode *inode,
- handle_t *handle,
- struct ocfs2_xattr_info *xi,
- struct ocfs2_xattr_search *xs,
- struct ocfs2_xattr_value_buf *vb,
- size_t offs)
-{
- int ret = 0;
- void *val = xs->base + offs;
- size_t size = namevalue_size_xi(xi);
-
- ret = vb->vb_access(handle, INODE_CACHE(inode), vb->vb_bh,
- OCFS2_JOURNAL_ACCESS_WRITE);
- if (ret) {
- mlog_errno(ret);
- goto out;
- }
- /* Decrease xattr count */
- le16_add_cpu(&xs->header->xh_count, -1);
- /* Remove the xattr entry and tree root which has already be set*/
- memset((void *)xs->here, 0, sizeof(struct ocfs2_xattr_entry));
- memset(val, 0, size);
-
- ret = ocfs2_journal_dirty(handle, vb->vb_bh);
- if (ret < 0)
- mlog_errno(ret);
-out:
- return ret;
-}
-
-static int ocfs2_xattr_update_entry(struct inode *inode,
- handle_t *handle,
- struct ocfs2_xattr_info *xi,
- struct ocfs2_xattr_search *xs,
- struct ocfs2_xattr_value_buf *vb,
- size_t offs)
-{
- int ret;
-
- ret = vb->vb_access(handle, INODE_CACHE(inode), vb->vb_bh,
- OCFS2_JOURNAL_ACCESS_WRITE);
- if (ret) {
- mlog_errno(ret);
- goto out;
- }
-
- xs->here->xe_name_offset = cpu_to_le16(offs);
- xs->here->xe_value_size = cpu_to_le64(xi->xi_value_len);
- if (xi->xi_value_len <= OCFS2_XATTR_INLINE_SIZE)
- ocfs2_xattr_set_local(xs->here, 1);
- else
- ocfs2_xattr_set_local(xs->here, 0);
- ocfs2_xattr_hash_entry(inode, xs->header, xs->here);
-
- ret = ocfs2_journal_dirty(handle, vb->vb_bh);
- if (ret < 0)
- mlog_errno(ret);
-out:
- return ret;
-}
-
-/*
- * ocfs2_xattr_set_value_outside()
- *
- * Set large size value in B tree.
- */
-static int ocfs2_xattr_set_value_outside(struct inode *inode,
- struct ocfs2_xattr_info *xi,
- struct ocfs2_xattr_search *xs,
- struct ocfs2_xattr_set_ctxt *ctxt,
- struct ocfs2_xattr_value_buf *vb,
- size_t offs)
-{
- void *val = xs->base + offs;
- struct ocfs2_xattr_value_root *xv = NULL;
- size_t size = namevalue_size_xi(xi);
- int ret = 0;
-
- memset(val, 0, size);
- memcpy(val, xi->xi_name, xi->xi_name_len);
- xv = (struct ocfs2_xattr_value_root *)
- (val + OCFS2_XATTR_SIZE(xi->xi_name_len));
- xv->xr_clusters = 0;
- xv->xr_last_eb_blk = 0;
- xv->xr_list.l_tree_depth = 0;
- xv->xr_list.l_count = cpu_to_le16(1);
- xv->xr_list.l_next_free_rec = 0;
- vb->vb_xv = xv;
-
- ret = ocfs2_xattr_value_truncate(inode, vb, xi->xi_value_len, ctxt);
- if (ret < 0) {
- mlog_errno(ret);
- return ret;
- }
- ret = ocfs2_xattr_update_entry(inode, ctxt->handle, xi, xs, vb, offs);
- if (ret < 0) {
- mlog_errno(ret);
- return ret;
- }
- ret = __ocfs2_xattr_set_value_outside(inode, ctxt->handle, vb,
- xi->xi_value, xi->xi_value_len);
- if (ret < 0)
- mlog_errno(ret);
-
- return ret;
-}
-
static int ocfs2_xa_has_space_helper(int needed_space, int free_start,
int num_entries)
{
@@ -2015,6 +1890,78 @@ static void ocfs2_xa_remove_entry(struct ocfs2_xa_loc *loc)
}
}
+static int ocfs2_xa_value_truncate(struct ocfs2_xa_loc *loc, u64 bytes,
+ struct ocfs2_xattr_set_ctxt *ctxt)
+{
+ struct ocfs2_xattr_value_buf vb;
+
+ ocfs2_xa_fill_value_buf(loc, &vb);
+ return ocfs2_xattr_value_truncate(loc->xl_inode, &vb, bytes, ctxt);
+}
+
+static void ocfs2_xa_install_value_root(struct ocfs2_xa_loc *loc)
+{
+ int name_size = OCFS2_XATTR_SIZE(loc->xl_entry->xe_name_len);
+ char *nameval_buf;
+
+ nameval_buf = ocfs2_xa_offset_pointer(loc,
+ le16_to_cpu(loc->xl_entry->xe_name_offset));
+ memcpy(nameval_buf + name_size, &def_xv, OCFS2_XATTR_ROOT_SIZE);
+}
+
+/*
+ * Take an existing entry and make it ready for the new value. This
+ * won't allocate space, but it may free space. It should be ready for
+ * ocfs2_xa_prepare_entry() to finish the work.
+ */
+static int ocfs2_xa_reuse_entry(struct ocfs2_xa_loc *loc,
+ struct ocfs2_xattr_info *xi,
+ struct ocfs2_xattr_set_ctxt *ctxt)
+{
+ int rc = 0;
+ int name_size = OCFS2_XATTR_SIZE(xi->xi_name_len);
+ char *nameval_buf;
+ int xe_local = ocfs2_xattr_is_local(loc->xl_entry);
+ int xi_local = xi->xi_value_len <= OCFS2_XATTR_INLINE_SIZE;
+
+ BUG_ON(OCFS2_XATTR_SIZE(loc->xl_entry->xe_name_len) !=
+ name_size);
+
+ nameval_buf = ocfs2_xa_offset_pointer(loc,
+ le16_to_cpu(loc->xl_entry->xe_name_offset));
+ if (xe_local) {
+ memset(nameval_buf + name_size, 0,
+ namevalue_size_xe(loc->xl_entry) - name_size);
+ if (!xi_local)
+ ocfs2_xa_install_value_root(loc);
+ } else {
+ if (xi_local) {
+ rc = ocfs2_xa_value_truncate(loc, 0, ctxt);
+ if (rc < 0) {
+ mlog_errno(rc);
+ goto out;
+ }
+ memset(nameval_buf + name_size, 0,
+ namevalue_size_xe(loc->xl_entry) -
+ name_size);
+ } else if (le64_to_cpu(loc->xl_entry->xe_value_size) >
+ xi->xi_value_len) {
+ rc = ocfs2_xa_value_truncate(loc, xi->xi_value_len,
+ ctxt);
+ if (rc < 0) {
+ mlog_errno(rc);
+ goto out;
+ }
+ }
+ }
+
+ loc->xl_entry->xe_value_size = cpu_to_le64(xi->xi_value_len);
+ ocfs2_xattr_set_local(loc->xl_entry, xi_local);
+
+out:
+ return rc;
+}
+
/*
* Prepares loc->xl_entry to receive the new xattr. This includes
* properly setting up the name+value pair region. If loc->xl_entry
@@ -2027,11 +1974,10 @@ static void ocfs2_xa_remove_entry(struct ocfs2_xa_loc *loc)
*/
static int ocfs2_xa_prepare_entry(struct ocfs2_xa_loc *loc,
struct ocfs2_xattr_info *xi,
- u32 name_hash)
+ u32 name_hash,
+ struct ocfs2_xattr_set_ctxt *ctxt)
{
int rc = 0;
- int name_size = OCFS2_XATTR_SIZE(xi->xi_name_len);
- char *nameval_buf;
if (!xi->xi_value) {
ocfs2_xa_remove_entry(loc);
@@ -2044,13 +1990,10 @@ static int ocfs2_xa_prepare_entry(struct ocfs2_xa_loc *loc,
if (loc->xl_entry) {
if (ocfs2_xa_can_reuse_entry(loc, xi)) {
- nameval_buf = ocfs2_xa_offset_pointer(loc,
- le16_to_cpu(loc->xl_entry->xe_name_offset));
- memset(nameval_buf + name_size, 0,
- namevalue_size_xe(loc->xl_entry) - name_size);
- loc->xl_entry->xe_value_size =
- cpu_to_le64(xi->xi_value_len);
- goto out;
+ rc = ocfs2_xa_reuse_entry(loc, xi, ctxt);
+ if (rc)
+ goto out;
+ goto alloc_value;
}
ocfs2_xa_wipe_namevalue(loc);
@@ -2062,30 +2005,46 @@ static int ocfs2_xa_prepare_entry(struct ocfs2_xa_loc *loc,
* name+value pair back from the end.
*/
ocfs2_xa_add_namevalue(loc, xi);
+ if (xi->xi_value_len > OCFS2_XATTR_INLINE_SIZE)
+ ocfs2_xa_install_value_root(loc);
+
+alloc_value:
+ if (xi->xi_value_len > OCFS2_XATTR_INLINE_SIZE) {
+ rc = ocfs2_xa_value_truncate(loc, xi->xi_value_len, ctxt);
+ if (rc < 0)
+ mlog_errno(rc);
+ }
out:
return rc;
}
/*
- * Store the value portion of the name+value pair. This is either an
- * inline value or the tree root of an external value.
+ * Store the value portion of the name+value pair. This will skip
+ * values that are stored externally. Their tree roots were set up
+ * by ocfs2_xa_prepare_entry().
*/
-static void ocfs2_xa_store_inline_value(struct ocfs2_xa_loc *loc,
- struct ocfs2_xattr_info *xi)
+static int ocfs2_xa_store_value(struct ocfs2_xa_loc *loc,
+ struct ocfs2_xattr_info *xi,
+ struct ocfs2_xattr_set_ctxt *ctxt)
{
+ int rc = 0;
int nameval_offset = le16_to_cpu(loc->xl_entry->xe_name_offset);
int name_size = OCFS2_XATTR_SIZE(xi->xi_name_len);
- int inline_value_size = namevalue_size_xi(xi) - name_size;
- const void *value = xi->xi_value;
char *nameval_buf;
+ struct ocfs2_xattr_value_buf vb;
- if (xi->xi_value_len > OCFS2_XATTR_INLINE_SIZE) {
- value = &def_xv;
- inline_value_size = OCFS2_XATTR_ROOT_SIZE;
- }
+ ocfs2_xa_fill_value_buf(loc, &vb);
nameval_buf = ocfs2_xa_offset_pointer(loc, nameval_offset);
- memcpy(nameval_buf + name_size, value, inline_value_size);
+ if (xi->xi_value_len > OCFS2_XATTR_INLINE_SIZE) {
+ rc = __ocfs2_xattr_set_value_outside(loc->xl_inode,
+ ctxt->handle, &vb,
+ xi->xi_value,
+ xi->xi_value_len);
+ } else
+ memcpy(nameval_buf + name_size, xi->xi_value, xi->xi_value_len);
+
+ return rc;
}
static void ocfs2_init_dinode_xa_loc(struct ocfs2_xa_loc *loc,
@@ -2158,117 +2117,19 @@ static int ocfs2_xattr_set_entry(struct inode *inode,
struct ocfs2_xattr_set_ctxt *ctxt,
int flag)
{
- 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;
- size_t size_l = 0;
handle_t *handle = ctxt->handle;
- int free, i, ret;
+ int ret;
u32 name_hash = ocfs2_xattr_name_hash(inode, xi->xi_name,
xi->xi_name_len);
struct ocfs2_xa_loc loc;
- struct ocfs2_xattr_value_buf vb = {
- .vb_bh = xs->xattr_bh,
- .vb_access = ocfs2_journal_access_di,
- };
- if (!(flag & OCFS2_INLINE_XATTR_FL)) {
+ if (!(flag & OCFS2_INLINE_XATTR_FL))
BUG_ON(xs->xattr_bh == xs->inode_bh);
- vb.vb_access = ocfs2_journal_access_xb;
- } else
+ 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) - OCFS2_XATTR_HEADER_GAP;
- if (free < 0)
- return -EIO;
-
- if (!xs->not_found)
- free += ocfs2_xe_entry_usage(xs->here);
-
- /* Check free space in inode or block */
- if (xi->xi_value && (free < ocfs2_xi_entry_usage(xi))) {
- ret = -ENOSPC;
- goto out;
- }
-
- if (!xs->not_found) {
- /* For existing extended attribute */
- size_t size = namevalue_size_xe(xs->here);
- size_t offs = le16_to_cpu(xs->here->xe_name_offset);
- void *val = xs->base + offs;
-
- if (ocfs2_xattr_is_local(xs->here) && size == size_l) {
- /* Replace existing local xattr with tree root */
- ret = ocfs2_xattr_set_value_outside(inode, xi, xs,
- ctxt, &vb, offs);
- if (ret < 0)
- mlog_errno(ret);
- goto out;
- } else if (!ocfs2_xattr_is_local(xs->here)) {
- /* For existing xattr which has value outside */
- vb.vb_xv = (struct ocfs2_xattr_value_root *)
- (val + OCFS2_XATTR_SIZE(xi->xi_name_len));
-
- if (xi->xi_value_len > OCFS2_XATTR_INLINE_SIZE) {
- /*
- * If new value need set outside also,
- * first truncate old value to new value,
- * then set new value with set_value_outside().
- */
- ret = ocfs2_xattr_value_truncate(inode,
- &vb,
- xi->xi_value_len,
- ctxt);
- if (ret < 0) {
- mlog_errno(ret);
- goto out;
- }
-
- ret = ocfs2_xattr_update_entry(inode,
- handle,
- xi,
- xs,
- &vb,
- offs);
- if (ret < 0) {
- mlog_errno(ret);
- goto out;
- }
-
- ret = __ocfs2_xattr_set_value_outside(inode,
- handle,
- &vb,
- xi->xi_value,
- xi->xi_value_len);
- if (ret < 0)
- mlog_errno(ret);
- goto out;
- } else {
- /*
- * If new value need set in local,
- * just trucate old value to zero.
- */
- ret = ocfs2_xattr_value_truncate(inode,
- &vb,
- 0,
- ctxt);
- if (ret < 0)
- mlog_errno(ret);
- }
- }
- }
-
ret = ocfs2_journal_access_di(handle, INODE_CACHE(inode), xs->inode_bh,
OCFS2_JOURNAL_ACCESS_WRITE);
if (ret) {
@@ -2289,22 +2150,20 @@ static int ocfs2_xattr_set_entry(struct inode *inode,
goto out;
}
- /*
- * Prepare our entry and insert the inline value. This will
- * be a value tree root for values that are larger than
- * OCFS2_XATTR_INLINE_SIZE.
- */
- ret = ocfs2_xa_prepare_entry(&loc, xi, name_hash);
+ ret = ocfs2_xa_prepare_entry(&loc, xi, name_hash, ctxt);
if (ret) {
if (ret != -ENOSPC)
mlog_errno(ret);
goto out;
}
- /* XXX For now, until we make ocfs2_xa_prepare_entry() primary */
- BUG_ON(ret == -ENOSPC);
- ocfs2_xa_store_inline_value(&loc, xi);
xs->here = loc.xl_entry;
+ ret = ocfs2_xa_store_value(&loc, xi, ctxt);
+ if (ret) {
+ mlog_errno(ret);
+ goto out;
+ }
+
ocfs2_xa_journal_dirty(handle, &loc);
if (!(oi->ip_dyn_features & OCFS2_INLINE_XATTR_FL) &&
@@ -2336,28 +2195,6 @@ static int ocfs2_xattr_set_entry(struct inode *inode,
if (ret < 0)
mlog_errno(ret);
- if (!ret && xi->xi_value_len > OCFS2_XATTR_INLINE_SIZE) {
- /*
- * Set value outside in B tree.
- * This is the second step for value size > INLINE_SIZE.
- */
- size_t offs = le16_to_cpu(xs->here->xe_name_offset);
- ret = ocfs2_xattr_set_value_outside(inode, xi, xs, ctxt,
- &vb, offs);
- if (ret < 0) {
- int ret2;
-
- mlog_errno(ret);
- /*
- * If set value outside failed, we have to clean
- * the junk tree root we have already set in local.
- */
- ret2 = ocfs2_xattr_cleanup(inode, ctxt->handle,
- xi, xs, &vb, offs);
- if (ret2 < 0)
- mlog_errno(ret2);
- }
- }
out:
return ret;
}
@@ -5338,61 +5175,6 @@ static inline char *ocfs2_xattr_bucket_get_val(struct inode *inode,
}
/*
- * Set the xattr entry in the specified bucket.
- * The bucket is indicated by xs->bucket and it should have the enough
- * space for the xattr insertion.
- */
-static int ocfs2_xattr_set_entry_in_bucket(struct inode *inode,
- handle_t *handle,
- struct ocfs2_xattr_info *xi,
- struct ocfs2_xattr_search *xs,
- u32 name_hash)
-{
- int ret;
- u64 blkno;
- struct ocfs2_xa_loc loc;
-
- mlog(0, "Set xattr entry len = %lu index = %d in bucket %llu\n",
- (unsigned long)xi->xi_value_len, xi->xi_name_index,
- (unsigned long long)bucket_blkno(xs->bucket));
-
- if (!xs->bucket->bu_bhs[1]) {
- blkno = bucket_blkno(xs->bucket);
- ocfs2_xattr_bucket_relse(xs->bucket);
- ret = ocfs2_read_xattr_bucket(xs->bucket, blkno);
- if (ret) {
- mlog_errno(ret);
- goto out;
- }
- }
-
- ocfs2_init_xattr_bucket_xa_loc(&loc, xs->bucket,
- xs->not_found ? NULL : xs->here);
- ret = ocfs2_xa_journal_access(handle, &loc,
- OCFS2_JOURNAL_ACCESS_WRITE);
- if (ret < 0) {
- mlog_errno(ret);
- goto out;
- }
-
- ret = ocfs2_xa_prepare_entry(&loc, xi, name_hash);
- if (ret) {
- if (ret != -ENOSPC)
- mlog_errno(ret);
- goto out;
- }
- /* XXX For now, until we make ocfs2_xa_prepare_entry() primary */
- BUG_ON(ret == -ENOSPC);
- ocfs2_xa_store_inline_value(&loc, xi);
- xs->here = loc.xl_entry;
-
- ocfs2_xa_journal_dirty(handle, &loc);
-
-out:
- return ret;
-}
-
-/*
* Truncate the specified xe_off entry in xattr bucket.
* bucket is indicated by header_bh and len is the new length.
* Both the ocfs2_xattr_value_root and the entry will be updated here.
@@ -5462,66 +5244,6 @@ out:
return ret;
}
-static int ocfs2_xattr_bucket_value_truncate_xs(struct inode *inode,
- struct ocfs2_xattr_search *xs,
- int len,
- struct ocfs2_xattr_set_ctxt *ctxt)
-{
- int ret, offset;
- struct ocfs2_xattr_entry *xe = xs->here;
- struct ocfs2_xattr_header *xh = (struct ocfs2_xattr_header *)xs->base;
-
- 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,
- offset, len, ctxt);
- if (ret)
- mlog_errno(ret);
-
- return ret;
-}
-
-static int ocfs2_xattr_bucket_set_value_outside(struct inode *inode,
- handle_t *handle,
- struct ocfs2_xattr_search *xs,
- char *val,
- int value_len)
-{
- int ret, offset, block_off;
- struct ocfs2_xattr_value_root *xv;
- struct ocfs2_xattr_entry *xe = xs->here;
- struct ocfs2_xattr_header *xh = bucket_xh(xs->bucket);
- void *base;
- struct ocfs2_xattr_value_buf vb = {
- .vb_access = ocfs2_journal_access,
- };
-
- BUG_ON(!xs->base || !xe || ocfs2_xattr_is_local(xe));
-
- ret = ocfs2_xattr_bucket_get_name_value(inode->i_sb, xh,
- xe - xh->xh_entries,
- &block_off,
- &offset);
- if (ret) {
- mlog_errno(ret);
- goto out;
- }
-
- base = bucket_block(xs->bucket, block_off);
- xv = (struct ocfs2_xattr_value_root *)(base + offset +
- OCFS2_XATTR_SIZE(xe->xe_name_len));
-
- vb.vb_xv = xv;
- vb.vb_bh = xs->bucket->bu_bhs[block_off];
- ret = __ocfs2_xattr_set_value_outside(inode, handle,
- &vb, val, value_len);
- if (ret)
- mlog_errno(ret);
-out:
- return ret;
-}
-
static int ocfs2_rm_xattr_cluster(struct inode *inode,
struct buffer_head *root_bh,
u64 blkno,
@@ -5620,41 +5342,8 @@ out:
return ret;
}
-static void ocfs2_xattr_bucket_remove_xs(struct inode *inode,
- handle_t *handle,
- struct ocfs2_xattr_search *xs)
-{
- struct ocfs2_xattr_header *xh = bucket_xh(xs->bucket);
- struct ocfs2_xattr_entry *last = &xh->xh_entries[
- le16_to_cpu(xh->xh_count) - 1];
- int ret = 0;
-
- ret = ocfs2_xattr_bucket_journal_access(handle, xs->bucket,
- OCFS2_JOURNAL_ACCESS_WRITE);
- if (ret) {
- mlog_errno(ret);
- return;
- }
-
- /* Remove the old entry. */
- memmove(xs->here, xs->here + 1,
- (void *)last - (void *)xs->here);
- memset(last, 0, sizeof(struct ocfs2_xattr_entry));
- le16_add_cpu(&xh->xh_count, -1);
-
- ocfs2_xattr_bucket_journal_dirty(handle, xs->bucket);
-}
-
/*
* Set the xattr name/value in the bucket specified in xs.
- *
- * As the new value in xi may be stored in the bucket or in an outside cluster,
- * we divide the whole process into 3 steps:
- * 1. insert name/value in the bucket(ocfs2_xattr_set_entry_in_bucket)
- * 2. truncate of the outside cluster(ocfs2_xattr_bucket_value_truncate_xs)
- * 3. Set the value to the outside cluster(ocfs2_xattr_bucket_set_value_outside)
- * 4. If the clusters for the new outside value can't be allocated, we need
- * to free the xattr we allocated in set.
*/
static int ocfs2_xattr_set_in_bucket(struct inode *inode,
struct ocfs2_xattr_info *xi,
@@ -5662,70 +5351,46 @@ static int ocfs2_xattr_set_in_bucket(struct inode *inode,
struct ocfs2_xattr_set_ctxt *ctxt)
{
int ret;
- size_t value_len;
- char *val = (char *)xi->xi_value;
- struct ocfs2_xattr_entry *xe = xs->here;
+ u64 blkno;
+ struct ocfs2_xa_loc loc;
u32 name_hash = ocfs2_xattr_name_hash(inode, xi->xi_name,
xi->xi_name_len);
- value_len = xi->xi_value_len;
- if (!xs->not_found && !ocfs2_xattr_is_local(xe)) {
- /*
- * We need to truncate the xattr storage first.
- *
- * If both the old and new value are stored to
- * outside block, we only need to truncate
- * the storage and then set the value outside.
- *
- * If the new value should be stored within block,
- * we should free all the outside block first and
- * the modification to the xattr block will be done
- * by following steps.
- */
- if (xi->xi_value_len <= OCFS2_XATTR_INLINE_SIZE)
- value_len = 0;
-
- ret = ocfs2_xattr_bucket_value_truncate_xs(inode, xs,
- value_len,
- ctxt);
- if (ret)
+ if (!xs->bucket->bu_bhs[1]) {
+ blkno = bucket_blkno(xs->bucket);
+ ocfs2_xattr_bucket_relse(xs->bucket);
+ ret = ocfs2_read_xattr_bucket(xs->bucket, blkno);
+ if (ret) {
+ mlog_errno(ret);
goto out;
-
- if (value_len)
- goto set_value_outside;
+ }
}
- /* So we have to handle the inside block change now. */
- ret = ocfs2_xattr_set_entry_in_bucket(inode, ctxt->handle, xi, xs,
- name_hash);
- if (ret) {
+ ocfs2_init_xattr_bucket_xa_loc(&loc, xs->bucket,
+ xs->not_found ? NULL : xs->here);
+ ret = ocfs2_xa_journal_access(ctxt->handle, &loc,
+ OCFS2_JOURNAL_ACCESS_WRITE);
+ if (ret < 0) {
mlog_errno(ret);
goto out;
}
- if (xi->xi_value_len <= OCFS2_XATTR_INLINE_SIZE)
+ ret = ocfs2_xa_prepare_entry(&loc, xi, name_hash, ctxt);
+ if (ret) {
+ if (ret != -ENOSPC)
+ mlog_errno(ret);
goto out;
+ }
+ xs->here = loc.xl_entry;
- /* allocate the space now for the outside block storage. */
- ret = ocfs2_xattr_bucket_value_truncate_xs(inode, xs,
- value_len, ctxt);
+ ret = ocfs2_xa_store_value(&loc, xi, ctxt);
if (ret) {
mlog_errno(ret);
-
- if (xs->not_found) {
- /*
- * We can't allocate enough clusters for outside
- * storage and we have allocated xattr already,
- * so need to remove it.
- */
- ocfs2_xattr_bucket_remove_xs(inode, ctxt->handle, xs);
- }
goto out;
}
-set_value_outside:
- ret = ocfs2_xattr_bucket_set_value_outside(inode, ctxt->handle,
- xs, val, value_len);
+ ocfs2_xa_journal_dirty(ctxt->handle, &loc);
+
out:
return ret;
}
--
1.6.3.3
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 11/14] ocfs2: Gell into ocfs2_xa_set()
2009-08-28 8:35 [Ocfs2-devel] [PATCH 0/14] ocfs2: Unify the setting of extended attributes Joel Becker
` (9 preceding siblings ...)
2009-08-28 8:35 ` [Ocfs2-devel] [PATCH 10/14] ocfs2: Allocation in ocfs2_xa_prepare_entry() values in ocfs2_xa_store_value() Joel Becker
@ 2009-08-28 8:36 ` Joel Becker
2009-08-28 8:36 ` [Ocfs2-devel] [PATCH 12/14] ocfs2: Let ocfs2_xa_prepare_entry() do space checks Joel Becker
` (2 subsequent siblings)
13 siblings, 0 replies; 45+ messages in thread
From: Joel Becker @ 2009-08-28 8:36 UTC (permalink / raw)
To: ocfs2-devel
ocfs2_xa_set() wraps the ocfs2_xa_prepare_entry()/ocfs2_xa_store_value()
logic. Both callers can now use the same routine.
Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
fs/ocfs2/xattr.c | 71 +++++++++++++++++++++++++++--------------------------
1 files changed, 36 insertions(+), 35 deletions(-)
diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index d922ad9..b4cda7e 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -2047,6 +2047,40 @@ static int ocfs2_xa_store_value(struct ocfs2_xa_loc *loc,
return rc;
}
+static int ocfs2_xa_set(struct ocfs2_xa_loc *loc,
+ struct ocfs2_xattr_info *xi,
+ struct ocfs2_xattr_set_ctxt *ctxt)
+{
+ int ret;
+ u32 name_hash = ocfs2_xattr_name_hash(loc->xl_inode, xi->xi_name,
+ xi->xi_name_len);
+
+ ret = ocfs2_xa_journal_access(ctxt->handle, loc,
+ OCFS2_JOURNAL_ACCESS_WRITE);
+ if (ret) {
+ mlog_errno(ret);
+ goto out;
+ }
+
+ ret = ocfs2_xa_prepare_entry(loc, xi, name_hash, ctxt);
+ if (ret) {
+ if (ret != -ENOSPC)
+ mlog_errno(ret);
+ goto out;
+ }
+
+ ret = ocfs2_xa_store_value(loc, xi, ctxt);
+ if (ret) {
+ mlog_errno(ret);
+ goto out;
+ }
+
+ ocfs2_xa_journal_dirty(ctxt->handle, loc);
+
+out:
+ return ret;
+}
+
static void ocfs2_init_dinode_xa_loc(struct ocfs2_xa_loc *loc,
struct inode *inode,
struct buffer_head *bh,
@@ -2121,8 +2155,6 @@ static int ocfs2_xattr_set_entry(struct inode *inode,
struct ocfs2_dinode *di = (struct ocfs2_dinode *)xs->inode_bh->b_data;
handle_t *handle = ctxt->handle;
int ret;
- u32 name_hash = ocfs2_xattr_name_hash(inode, xi->xi_name,
- xi->xi_name_len);
struct ocfs2_xa_loc loc;
if (!(flag & OCFS2_INLINE_XATTR_FL))
@@ -2143,14 +2175,8 @@ static int ocfs2_xattr_set_entry(struct inode *inode,
else
ocfs2_init_xattr_block_xa_loc(&loc, inode, xs->xattr_bh,
xs->not_found ? NULL : xs->here);
- ret = ocfs2_xa_journal_access(handle, &loc,
- OCFS2_JOURNAL_ACCESS_WRITE);
- if (ret) {
- mlog_errno(ret);
- goto out;
- }
- ret = ocfs2_xa_prepare_entry(&loc, xi, name_hash, ctxt);
+ ret = ocfs2_xa_set(&loc, xi, ctxt);
if (ret) {
if (ret != -ENOSPC)
mlog_errno(ret);
@@ -2158,14 +2184,6 @@ static int ocfs2_xattr_set_entry(struct inode *inode,
}
xs->here = loc.xl_entry;
- ret = ocfs2_xa_store_value(&loc, xi, ctxt);
- if (ret) {
- mlog_errno(ret);
- goto out;
- }
-
- ocfs2_xa_journal_dirty(handle, &loc);
-
if (!(oi->ip_dyn_features & OCFS2_INLINE_XATTR_FL) &&
(flag & OCFS2_INLINE_XATTR_FL)) {
struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
@@ -5353,8 +5371,6 @@ static int ocfs2_xattr_set_in_bucket(struct inode *inode,
int ret;
u64 blkno;
struct ocfs2_xa_loc loc;
- u32 name_hash = ocfs2_xattr_name_hash(inode, xi->xi_name,
- xi->xi_name_len);
if (!xs->bucket->bu_bhs[1]) {
blkno = bucket_blkno(xs->bucket);
@@ -5368,14 +5384,7 @@ static int ocfs2_xattr_set_in_bucket(struct inode *inode,
ocfs2_init_xattr_bucket_xa_loc(&loc, xs->bucket,
xs->not_found ? NULL : xs->here);
- ret = ocfs2_xa_journal_access(ctxt->handle, &loc,
- OCFS2_JOURNAL_ACCESS_WRITE);
- if (ret < 0) {
- mlog_errno(ret);
- goto out;
- }
-
- ret = ocfs2_xa_prepare_entry(&loc, xi, name_hash, ctxt);
+ ret = ocfs2_xa_set(&loc, xi, ctxt);
if (ret) {
if (ret != -ENOSPC)
mlog_errno(ret);
@@ -5383,14 +5392,6 @@ static int ocfs2_xattr_set_in_bucket(struct inode *inode,
}
xs->here = loc.xl_entry;
- ret = ocfs2_xa_store_value(&loc, xi, ctxt);
- if (ret) {
- mlog_errno(ret);
- goto out;
- }
-
- ocfs2_xa_journal_dirty(ctxt->handle, &loc);
-
out:
return ret;
}
--
1.6.3.3
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 12/14] ocfs2: Let ocfs2_xa_prepare_entry() do space checks.
2009-08-28 8:35 [Ocfs2-devel] [PATCH 0/14] ocfs2: Unify the setting of extended attributes Joel Becker
` (10 preceding siblings ...)
2009-08-28 8:36 ` [Ocfs2-devel] [PATCH 11/14] ocfs2: Gell into ocfs2_xa_set() Joel Becker
@ 2009-08-28 8:36 ` Joel Becker
2009-09-02 1:54 ` Tao Ma
2009-08-28 8:36 ` [Ocfs2-devel] [PATCH 13/14] ocfs2: Set xattr block entries with ocfs2_xa_set() Joel Becker
2009-08-28 8:36 ` [Ocfs2-devel] [PATCH 14/14] ocfs2: Set inline xattr " Joel Becker
13 siblings, 1 reply; 45+ messages in thread
From: Joel Becker @ 2009-08-28 8:36 UTC (permalink / raw)
To: ocfs2-devel
ocfs2_xattr_set_in_bucket() doesn't need to do its own hacky space
checking. Let's let ocfs2_xa_prepare_entry() (via ocfs2_xa_set()) do
the more accurate work. Whenever it doesn't have space,
ocfs2_xattr_set_in_bucket() can try to get more space.
Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
fs/ocfs2/xattr.c | 239 ++++++++++++++++-------------------------------------
1 files changed, 72 insertions(+), 167 deletions(-)
diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index b4cda7e..e400d64 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -5361,42 +5361,6 @@ out:
}
/*
- * Set the xattr name/value in the bucket specified in xs.
- */
-static int ocfs2_xattr_set_in_bucket(struct inode *inode,
- struct ocfs2_xattr_info *xi,
- struct ocfs2_xattr_search *xs,
- struct ocfs2_xattr_set_ctxt *ctxt)
-{
- int ret;
- u64 blkno;
- struct ocfs2_xa_loc loc;
-
- if (!xs->bucket->bu_bhs[1]) {
- blkno = bucket_blkno(xs->bucket);
- ocfs2_xattr_bucket_relse(xs->bucket);
- ret = ocfs2_read_xattr_bucket(xs->bucket, blkno);
- if (ret) {
- mlog_errno(ret);
- goto out;
- }
- }
-
- ocfs2_init_xattr_bucket_xa_loc(&loc, xs->bucket,
- xs->not_found ? NULL : xs->here);
- ret = ocfs2_xa_set(&loc, xi, ctxt);
- if (ret) {
- if (ret != -ENOSPC)
- mlog_errno(ret);
- goto out;
- }
- xs->here = loc.xl_entry;
-
-out:
- return ret;
-}
-
-/*
* check whether the xattr bucket is filled up with the same hash value.
* If we want to insert the xattr with the same hash, return -ENOSPC.
* If we want to insert a xattr with different hash value, go ahead
@@ -5429,151 +5393,92 @@ static int ocfs2_xattr_set_entry_index_block(struct inode *inode,
struct ocfs2_xattr_search *xs,
struct ocfs2_xattr_set_ctxt *ctxt)
{
- struct ocfs2_xattr_header *xh;
- struct ocfs2_xattr_entry *xe;
- u16 count, header_size, xh_free_start;
- int free, max_free, need, old;
- size_t value_size = 0;
- size_t blocksize = inode->i_sb->s_blocksize;
- int ret, allocation = 0;
+ int ret;
+ struct ocfs2_xa_loc loc;
mlog_entry("Set xattr %s in xattr index block\n", xi->xi_name);
-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);
- max_free = OCFS2_XATTR_BUCKET_SIZE - header_size -
- le16_to_cpu(xh->xh_name_value_len) - OCFS2_XATTR_HEADER_GAP;
-
- mlog_bug_on_msg(header_size > blocksize, "bucket %llu has header size "
- "of %u which exceed block size\n",
- (unsigned long long)bucket_blkno(xs->bucket),
- header_size);
-
- if (xi->xi_value && xi->xi_value_len > OCFS2_XATTR_INLINE_SIZE)
- value_size = OCFS2_XATTR_ROOT_SIZE;
- else if (xi->xi_value)
- value_size = OCFS2_XATTR_SIZE(xi->xi_value_len);
+ BUG_ON(!xs->bucket->bu_bhs[1]);
- if (xs->not_found)
- need = sizeof(struct ocfs2_xattr_entry) +
- OCFS2_XATTR_SIZE(xi->xi_name_len) + value_size;
- else {
- need = value_size + OCFS2_XATTR_SIZE(xi->xi_name_len);
+ ocfs2_init_xattr_bucket_xa_loc(&loc, xs->bucket,
+ xs->not_found ? NULL : xs->here);
+ ret = ocfs2_xa_set(&loc, xi, ctxt);
+ if (!ret) {
+ xs->here = loc.xl_entry;
+ goto out;
+ }
+ if (ret != -ENOSPC) {
+ mlog_errno(ret);
+ goto out;
+ }
- /*
- * We only replace the old value if the new length is smaller
- * than the old one. Otherwise we will allocate new space in the
- * bucket to store it.
- */
- xe = xs->here;
- if (ocfs2_xattr_is_local(xe))
- old = OCFS2_XATTR_SIZE(le64_to_cpu(xe->xe_value_size));
- else
- old = OCFS2_XATTR_SIZE(OCFS2_XATTR_ROOT_SIZE);
+ /* Ok, we need space. Let's try defragmenting the bucket. */
+ ret = ocfs2_defrag_xattr_bucket(inode, ctxt->handle,
+ xs->bucket);
+ if (ret) {
+ mlog_errno(ret);
+ goto out;
+ }
- if (old >= value_size)
- need = 0;
+ ret = ocfs2_xa_set(&loc, xi, ctxt);
+ if (!ret) {
+ xs->here = loc.xl_entry;
+ goto out;
+ }
+ if (ret != -ENOSPC) {
+ mlog_errno(ret);
+ goto out;
}
- free = xh_free_start - header_size - OCFS2_XATTR_HEADER_GAP;
+ /* Ack, need more space. Let's try to get another bucket! */
+
/*
- * We need to make sure the new name/value pair
- * can exist in the same block.
+ * We do not allow for overlapping ranges between buckets. And
+ * the maximum number of collisions we will allow for then is
+ * one bucket's worth, so check it here whether we need to
+ * add a new bucket for the insert.
*/
- if (xh_free_start % blocksize < need)
- free -= xh_free_start % blocksize;
-
- mlog(0, "xs->not_found = %d, in xattr bucket %llu: free = %d, "
- "need = %d, max_free = %d, xh_free_start = %u, xh_name_value_len ="
- " %u\n", xs->not_found,
- (unsigned long long)bucket_blkno(xs->bucket),
- free, need, max_free, le16_to_cpu(xh->xh_free_start),
- le16_to_cpu(xh->xh_name_value_len));
-
- if (free < need ||
- (xs->not_found &&
- count == ocfs2_xattr_max_xe_in_bucket(inode->i_sb))) {
- if (need <= max_free &&
- count < ocfs2_xattr_max_xe_in_bucket(inode->i_sb)) {
- /*
- * We can create the space by defragment. Since only the
- * name/value will be moved, the xe shouldn't be changed
- * in xs.
- */
- ret = ocfs2_defrag_xattr_bucket(inode, ctxt->handle,
- xs->bucket);
- if (ret) {
- mlog_errno(ret);
- goto out;
- }
-
- xh_free_start = le16_to_cpu(xh->xh_free_start);
- free = xh_free_start - header_size
- - OCFS2_XATTR_HEADER_GAP;
- if (xh_free_start % blocksize < need)
- free -= xh_free_start % blocksize;
-
- if (free >= need)
- goto xattr_set;
-
- mlog(0, "Can't get enough space for xattr insert by "
- "defragment. Need %u bytes, but we have %d, so "
- "allocate new bucket for it.\n", need, free);
- }
-
- /*
- * We have to add new buckets or clusters and one
- * allocation should leave us enough space for insert.
- */
- BUG_ON(allocation);
-
- /*
- * We do not allow for overlapping ranges between buckets. And
- * the maximum number of collisions we will allow for then is
- * one bucket's worth, so check it here whether we need to
- * add a new bucket for the insert.
- */
- ret = ocfs2_check_xattr_bucket_collision(inode,
- xs->bucket,
- xi->xi_name);
- if (ret) {
- mlog_errno(ret);
- goto out;
- }
-
- ret = ocfs2_add_new_xattr_bucket(inode,
- xs->xattr_bh,
+ ret = ocfs2_check_xattr_bucket_collision(inode,
xs->bucket,
- ctxt);
- if (ret) {
- mlog_errno(ret);
- goto out;
- }
+ xi->xi_name);
+ 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->xi_name_index,
- xi->xi_name, xs);
- if (ret && ret != -ENODATA)
- goto out;
- xs->not_found = ret;
- allocation = 1;
- goto try_again;
+ ret = ocfs2_add_new_xattr_bucket(inode,
+ xs->xattr_bh,
+ xs->bucket,
+ ctxt);
+ if (ret) {
+ mlog_errno(ret);
+ goto out;
}
-xattr_set:
- ret = ocfs2_xattr_set_in_bucket(inode, xi, xs, ctxt);
+ /*
+ * 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->xi_name_index,
+ xi->xi_name, xs);
+ if (ret && ret != -ENODATA)
+ goto out;
+ xs->not_found = ret;
+
+ /* We also need to re-init our ocfs2_xa_loc */
+ ocfs2_init_xattr_bucket_xa_loc(&loc, xs->bucket,
+ xs->not_found ? NULL : xs->here);
+ ret = ocfs2_xa_set(&loc, xi, ctxt);
+ if (!ret)
+ xs->here = loc.xl_entry;
+ else if (ret != -ENOSPC)
+ mlog_errno(ret);
+
out:
mlog_exit(ret);
return ret;
--
1.6.3.3
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 13/14] ocfs2: Set xattr block entries with ocfs2_xa_set()
2009-08-28 8:35 [Ocfs2-devel] [PATCH 0/14] ocfs2: Unify the setting of extended attributes Joel Becker
` (11 preceding siblings ...)
2009-08-28 8:36 ` [Ocfs2-devel] [PATCH 12/14] ocfs2: Let ocfs2_xa_prepare_entry() do space checks Joel Becker
@ 2009-08-28 8:36 ` Joel Becker
2009-09-02 2:50 ` Tao Ma
2009-08-28 8:36 ` [Ocfs2-devel] [PATCH 14/14] ocfs2: Set inline xattr " Joel Becker
13 siblings, 1 reply; 45+ messages in thread
From: Joel Becker @ 2009-08-28 8:36 UTC (permalink / raw)
To: ocfs2-devel
ocfs2_xattr_block_set() calls into ocfs2_xattr_set_entry() with just the
HAS_XATTR flag. Most of the machinery of ocfs2_xattr_set_entry() is
skipped. All that really happens other than the call to ocfs2_xa_set()
is making sure the HAS_XATTR flag is set on the inode.
But HAS_XATTR should be set when we also set di->i_xattr_loc. And
that's done in ocfs2_xattr_block_set(). So let's move it there, and
then ocfs2_xattr_block_set() can just call ocfs2_xa_set().
While we're there, ocfs2_create_xattr_block() can take the set_ctxt for
a smaller argument list. It also learns to set HAS_XATTR_FL, because it
knows for sure. ocfs2_create_empty_xatttr_block() in the reflink path
fakes a set_ctxt to call ocfs2_create_xattr_block().
Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
fs/ocfs2/xattr.c | 97 ++++++++++++++++++++++++++---------------------------
1 files changed, 48 insertions(+), 49 deletions(-)
diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index e400d64..862c56d 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -2157,10 +2157,8 @@ static int ocfs2_xattr_set_entry(struct inode *inode,
int ret;
struct ocfs2_xa_loc loc;
- if (!(flag & OCFS2_INLINE_XATTR_FL))
- BUG_ON(xs->xattr_bh == xs->inode_bh);
- else
- BUG_ON(xs->xattr_bh != xs->inode_bh);
+ BUG_ON(!(flag & OCFS2_INLINE_XATTR_FL));
+ BUG_ON(xs->xattr_bh != xs->inode_bh);
ret = ocfs2_journal_access_di(handle, INODE_CACHE(inode), xs->inode_bh,
OCFS2_JOURNAL_ACCESS_WRITE);
@@ -2169,13 +2167,8 @@ static int ocfs2_xattr_set_entry(struct inode *inode,
goto out;
}
- if (xs->xattr_bh == xs->inode_bh)
- ocfs2_init_dinode_xa_loc(&loc, inode, xs->inode_bh,
- xs->not_found ? NULL : xs->here);
- else
- ocfs2_init_xattr_block_xa_loc(&loc, inode, xs->xattr_bh,
- xs->not_found ? NULL : xs->here);
-
+ ocfs2_init_dinode_xa_loc(&loc, inode, xs->inode_bh,
+ xs->not_found ? NULL : xs->here);
ret = ocfs2_xa_set(&loc, xi, ctxt);
if (ret) {
if (ret != -ENOSPC)
@@ -2184,8 +2177,7 @@ static int ocfs2_xattr_set_entry(struct inode *inode,
}
xs->here = loc.xl_entry;
- if (!(oi->ip_dyn_features & OCFS2_INLINE_XATTR_FL) &&
- (flag & OCFS2_INLINE_XATTR_FL)) {
+ if (!(oi->ip_dyn_features & OCFS2_INLINE_XATTR_FL)) {
struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
unsigned int xattrsize = osb->s_xattr_inline_size;
@@ -2205,7 +2197,7 @@ static int ocfs2_xattr_set_entry(struct inode *inode,
}
/* Update xattr flag */
spin_lock(&oi->ip_lock);
- oi->ip_dyn_features |= flag;
+ oi->ip_dyn_features |= OCFS2_INLINE_XATTR_FL;
di->i_dyn_features = cpu_to_le16(oi->ip_dyn_features);
spin_unlock(&oi->ip_lock);
@@ -2699,12 +2691,11 @@ cleanup:
return ret;
}
-static int ocfs2_create_xattr_block(handle_t *handle,
- struct inode *inode,
+static int ocfs2_create_xattr_block(struct inode *inode,
struct buffer_head *inode_bh,
- struct ocfs2_alloc_context *meta_ac,
- struct buffer_head **ret_bh,
- int indexed)
+ struct ocfs2_xattr_set_ctxt *ctxt,
+ int indexed,
+ struct buffer_head **ret_bh)
{
int ret;
u16 suballoc_bit_start;
@@ -2715,14 +2706,14 @@ static int ocfs2_create_xattr_block(handle_t *handle,
struct buffer_head *new_bh = NULL;
struct ocfs2_xattr_block *xblk;
- ret = ocfs2_journal_access_di(handle, INODE_CACHE(inode), inode_bh,
- OCFS2_JOURNAL_ACCESS_CREATE);
+ ret = ocfs2_journal_access_di(ctxt->handle, INODE_CACHE(inode),
+ inode_bh, OCFS2_JOURNAL_ACCESS_CREATE);
if (ret < 0) {
mlog_errno(ret);
goto end;
}
- ret = ocfs2_claim_metadata(osb, handle, meta_ac, 1,
+ ret = ocfs2_claim_metadata(osb, ctxt->handle, ctxt->meta_ac, 1,
&suballoc_bit_start, &num_got,
&first_blkno);
if (ret < 0) {
@@ -2733,7 +2724,7 @@ static int ocfs2_create_xattr_block(handle_t *handle,
new_bh = sb_getblk(inode->i_sb, first_blkno);
ocfs2_set_new_buffer_uptodate(INODE_CACHE(inode), new_bh);
- ret = ocfs2_journal_access_xb(handle, INODE_CACHE(inode),
+ ret = ocfs2_journal_access_xb(ctxt->handle, INODE_CACHE(inode),
new_bh,
OCFS2_JOURNAL_ACCESS_CREATE);
if (ret < 0) {
@@ -2749,7 +2740,6 @@ static int ocfs2_create_xattr_block(handle_t *handle,
xblk->xb_suballoc_bit = cpu_to_le16(suballoc_bit_start);
xblk->xb_fs_generation = cpu_to_le32(osb->fs_generation);
xblk->xb_blkno = cpu_to_le64(first_blkno);
-
if (indexed) {
struct ocfs2_xattr_tree_root *xr = &xblk->xb_attrs.xb_root;
xr->xt_clusters = cpu_to_le32(1);
@@ -2760,14 +2750,17 @@ static int ocfs2_create_xattr_block(handle_t *handle,
xr->xt_list.l_next_free_rec = cpu_to_le16(1);
xblk->xb_flags = cpu_to_le16(OCFS2_XATTR_INDEXED);
}
+ ocfs2_journal_dirty(ctxt->handle, new_bh);
- ret = ocfs2_journal_dirty(handle, new_bh);
- if (ret < 0) {
- mlog_errno(ret);
- goto end;
- }
+ /* Add it to the inode */
di->i_xattr_loc = cpu_to_le64(first_blkno);
- ocfs2_journal_dirty(handle, inode_bh);
+
+ spin_lock(&OCFS2_I(inode)->ip_lock);
+ OCFS2_I(inode)->ip_dyn_features |= OCFS2_HAS_XATTR_FL;
+ di->i_dyn_features = cpu_to_le16(OCFS2_I(inode)->ip_dyn_features);
+ spin_unlock(&OCFS2_I(inode)->ip_lock);
+
+ ocfs2_journal_dirty(ctxt->handle, inode_bh);
*ret_bh = new_bh;
new_bh = NULL;
@@ -2789,13 +2782,13 @@ static int ocfs2_xattr_block_set(struct inode *inode,
struct ocfs2_xattr_set_ctxt *ctxt)
{
struct buffer_head *new_bh = NULL;
- handle_t *handle = ctxt->handle;
struct ocfs2_xattr_block *xblk = NULL;
int ret;
+ struct ocfs2_xa_loc loc;
if (!xs->xattr_bh) {
- ret = ocfs2_create_xattr_block(handle, inode, xs->inode_bh,
- ctxt->meta_ac, &new_bh, 0);
+ ret = ocfs2_create_xattr_block(inode, xs->inode_bh, ctxt,
+ 0, &new_bh);
if (ret) {
mlog_errno(ret);
goto end;
@@ -2811,21 +2804,25 @@ static int ocfs2_xattr_block_set(struct inode *inode,
xblk = (struct ocfs2_xattr_block *)xs->xattr_bh->b_data;
if (!(le16_to_cpu(xblk->xb_flags) & OCFS2_XATTR_INDEXED)) {
- /* Set extended attribute into external block */
- ret = ocfs2_xattr_set_entry(inode, xi, xs, ctxt,
- OCFS2_HAS_XATTR_FL);
- if (!ret || ret != -ENOSPC)
- goto end;
+ ocfs2_init_xattr_block_xa_loc(&loc, inode, xs->xattr_bh,
+ xs->not_found ? NULL : xs->here);
- ret = ocfs2_xattr_create_index_block(inode, xs, ctxt);
- if (ret)
+ ret = ocfs2_xa_set(&loc, xi, ctxt);
+ if (!ret)
+ xs->here = loc.xl_entry;
+ else if (ret != -ENOSPC)
goto end;
+ else {
+ ret = ocfs2_xattr_create_index_block(inode, xs, ctxt);
+ if (ret)
+ goto end;
+ }
}
- ret = ocfs2_xattr_set_entry_index_block(inode, xi, xs, ctxt);
+ if (le16_to_cpu(xblk->xb_flags) & OCFS2_XATTR_INDEXED)
+ ret = ocfs2_xattr_set_entry_index_block(inode, xi, xs, ctxt);
end:
-
return ret;
}
@@ -6366,9 +6363,11 @@ static int ocfs2_create_empty_xattr_block(struct inode *inode,
int indexed)
{
int ret;
- handle_t *handle;
struct ocfs2_alloc_context *meta_ac;
struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
+ struct ocfs2_xattr_set_ctxt ctxt = {
+ .meta_ac = meta_ac,
+ };
ret = ocfs2_reserve_new_metadata_blocks(osb, 1, &meta_ac);
if (ret < 0) {
@@ -6376,21 +6375,21 @@ static int ocfs2_create_empty_xattr_block(struct inode *inode,
return ret;
}
- handle = ocfs2_start_trans(osb, OCFS2_XATTR_BLOCK_CREATE_CREDITS);
- if (IS_ERR(handle)) {
- ret = PTR_ERR(handle);
+ ctxt.handle = ocfs2_start_trans(osb, OCFS2_XATTR_BLOCK_CREATE_CREDITS);
+ if (IS_ERR(ctxt.handle)) {
+ ret = PTR_ERR(ctxt.handle);
mlog_errno(ret);
goto out;
}
mlog(0, "create new xattr block for inode %llu, index = %d\n",
(unsigned long long)fe_bh->b_blocknr, indexed);
- ret = ocfs2_create_xattr_block(handle, inode, fe_bh,
- meta_ac, ret_bh, indexed);
+ ret = ocfs2_create_xattr_block(inode, fe_bh, &ctxt, indexed,
+ ret_bh);
if (ret)
mlog_errno(ret);
- ocfs2_commit_trans(osb, handle);
+ ocfs2_commit_trans(osb, ctxt.handle);
out:
ocfs2_free_alloc_context(meta_ac);
return ret;
--
1.6.3.3
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 14/14] ocfs2: Set inline xattr entries with ocfs2_xa_set()
2009-08-28 8:35 [Ocfs2-devel] [PATCH 0/14] ocfs2: Unify the setting of extended attributes Joel Becker
` (12 preceding siblings ...)
2009-08-28 8:36 ` [Ocfs2-devel] [PATCH 13/14] ocfs2: Set xattr block entries with ocfs2_xa_set() Joel Becker
@ 2009-08-28 8:36 ` Joel Becker
2009-09-02 2:58 ` Tao Ma
13 siblings, 1 reply; 45+ messages in thread
From: Joel Becker @ 2009-08-28 8:36 UTC (permalink / raw)
To: ocfs2-devel
ocfs2_xattr_ibody_set() is the only remaining user of
ocfs2_xattr_set_entry(). ocfs2_xattr_set_entry() actually does two
things: it calls ocfs2_xa_set(), and it initializes the inline xattrs.
Initializing the inline space really belongs in its own call.
We lift the initialization to ocfs2_xattr_ibody_init(), called from
ocfs2_xattr_ibody_set() only when necessary. Now
ocfs2_xattr_ibody_set() can call ocfs2_xa_set() directly.
ocfs2_xattr_set_entry() goes away.
Another nice fact is that ocfs2_init_dinode_xa_loc() can trust
i_xattr_inline_size.
Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
fs/ocfs2/xattr.c | 156 +++++++++++++++++++++++++-----------------------------
1 files changed, 72 insertions(+), 84 deletions(-)
diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 862c56d..d485932 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -2088,17 +2088,13 @@ static void ocfs2_init_dinode_xa_loc(struct ocfs2_xa_loc *loc,
{
struct ocfs2_dinode *di = (struct ocfs2_dinode *)bh->b_data;
+ BUG_ON(!(OCFS2_I(inode)->ip_dyn_features & OCFS2_INLINE_XATTR_FL));
+
loc->xl_inode = inode;
loc->xl_ops = &ocfs2_xa_block_loc_ops;
loc->xl_storage = bh;
loc->xl_entry = entry;
-
- if (OCFS2_I(inode)->ip_dyn_features & OCFS2_INLINE_XATTR_FL)
- loc->xl_size = le16_to_cpu(di->i_xattr_inline_size);
- else {
- BUG_ON(entry);
- loc->xl_size = OCFS2_SB(inode->i_sb)->s_xattr_inline_size;
- }
+ loc->xl_size = le16_to_cpu(di->i_xattr_inline_size);
loc->xl_header =
(struct ocfs2_xattr_header *)(bh->b_data + bh->b_size -
loc->xl_size);
@@ -2135,80 +2131,6 @@ static void ocfs2_init_xattr_bucket_xa_loc(struct ocfs2_xa_loc *loc,
loc->xl_size = OCFS2_XATTR_BUCKET_SIZE;
}
-
-/*
- * ocfs2_xattr_set_entry()
- *
- * Set extended attribute entry into inode or block.
- *
- * If extended attribute value size > OCFS2_XATTR_INLINE_SIZE,
- * We first insert tree root(ocfs2_xattr_value_root) like a normal value,
- * then set value in B tree with set_value_outside().
- */
-static int ocfs2_xattr_set_entry(struct inode *inode,
- struct ocfs2_xattr_info *xi,
- struct ocfs2_xattr_search *xs,
- struct ocfs2_xattr_set_ctxt *ctxt,
- int flag)
-{
- struct ocfs2_inode_info *oi = OCFS2_I(inode);
- struct ocfs2_dinode *di = (struct ocfs2_dinode *)xs->inode_bh->b_data;
- handle_t *handle = ctxt->handle;
- int ret;
- struct ocfs2_xa_loc loc;
-
- BUG_ON(!(flag & OCFS2_INLINE_XATTR_FL));
- BUG_ON(xs->xattr_bh != xs->inode_bh);
-
- ret = ocfs2_journal_access_di(handle, INODE_CACHE(inode), xs->inode_bh,
- OCFS2_JOURNAL_ACCESS_WRITE);
- if (ret) {
- mlog_errno(ret);
- goto out;
- }
-
- ocfs2_init_dinode_xa_loc(&loc, inode, xs->inode_bh,
- xs->not_found ? NULL : xs->here);
- ret = ocfs2_xa_set(&loc, xi, ctxt);
- if (ret) {
- if (ret != -ENOSPC)
- mlog_errno(ret);
- goto out;
- }
- xs->here = loc.xl_entry;
-
- if (!(oi->ip_dyn_features & OCFS2_INLINE_XATTR_FL)) {
- struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
- unsigned int xattrsize = osb->s_xattr_inline_size;
-
- /*
- * Adjust extent record count or inline data size
- * to reserve space for extended attribute.
- */
- if (oi->ip_dyn_features & OCFS2_INLINE_DATA_FL) {
- struct ocfs2_inline_data *idata = &di->id2.i_data;
- le16_add_cpu(&idata->id_count, -xattrsize);
- } else if (!(ocfs2_inode_is_fast_symlink(inode))) {
- struct ocfs2_extent_list *el = &di->id2.i_list;
- le16_add_cpu(&el->l_count, -(xattrsize /
- sizeof(struct ocfs2_extent_rec)));
- }
- di->i_xattr_inline_size = cpu_to_le16(xattrsize);
- }
- /* Update xattr flag */
- spin_lock(&oi->ip_lock);
- oi->ip_dyn_features |= OCFS2_INLINE_XATTR_FL;
- di->i_dyn_features = cpu_to_le16(oi->ip_dyn_features);
- spin_unlock(&oi->ip_lock);
-
- ret = ocfs2_journal_dirty(handle, xs->inode_bh);
- if (ret < 0)
- mlog_errno(ret);
-
-out:
- return ret;
-}
-
/*
* In xattr remove, if it is stored outside and refcounted, we may have
* the chance to split the refcount tree. So need the allocators.
@@ -2604,6 +2526,55 @@ static int ocfs2_xattr_ibody_find(struct inode *inode,
return 0;
}
+static int ocfs2_xattr_ibody_init(struct inode *inode,
+ struct buffer_head *di_bh,
+ struct ocfs2_xattr_set_ctxt *ctxt)
+{
+ int ret;
+ struct ocfs2_inode_info *oi = OCFS2_I(inode);
+ struct ocfs2_dinode *di = (struct ocfs2_dinode *)di_bh->b_data;
+ struct ocfs2_super *osb = OCFS2_SB(inode->i_sb);
+ unsigned int xattrsize = osb->s_xattr_inline_size;
+
+ if (!ocfs2_xattr_has_space_inline(inode, di)) {
+ ret = -ENOSPC;
+ goto out;
+ }
+
+ ret = ocfs2_journal_access_di(ctxt->handle, INODE_CACHE(inode), di_bh,
+ OCFS2_JOURNAL_ACCESS_WRITE);
+ if (ret) {
+ mlog_errno(ret);
+ goto out;
+ }
+
+ /*
+ * Adjust extent record count or inline data size
+ * to reserve space for extended attribute.
+ */
+ if (oi->ip_dyn_features & OCFS2_INLINE_DATA_FL) {
+ struct ocfs2_inline_data *idata = &di->id2.i_data;
+ le16_add_cpu(&idata->id_count, -xattrsize);
+ } else if (!(ocfs2_inode_is_fast_symlink(inode))) {
+ struct ocfs2_extent_list *el = &di->id2.i_list;
+ le16_add_cpu(&el->l_count, -(xattrsize /
+ sizeof(struct ocfs2_extent_rec)));
+ }
+ di->i_xattr_inline_size = cpu_to_le16(xattrsize);
+
+ spin_lock(&oi->ip_lock);
+ oi->ip_dyn_features |= OCFS2_INLINE_XATTR_FL|OCFS2_HAS_XATTR_FL;
+ di->i_dyn_features = cpu_to_le16(oi->ip_dyn_features);
+ spin_unlock(&oi->ip_lock);
+
+ ret = ocfs2_journal_dirty(ctxt->handle, di_bh);
+ if (ret < 0)
+ mlog_errno(ret);
+
+out:
+ return ret;
+}
+
/*
* ocfs2_xattr_ibody_set()
*
@@ -2615,9 +2586,10 @@ static int ocfs2_xattr_ibody_set(struct inode *inode,
struct ocfs2_xattr_search *xs,
struct ocfs2_xattr_set_ctxt *ctxt)
{
+ int ret;
struct ocfs2_inode_info *oi = OCFS2_I(inode);
struct ocfs2_dinode *di = (struct ocfs2_dinode *)xs->inode_bh->b_data;
- int ret;
+ struct ocfs2_xa_loc loc;
if (inode->i_sb->s_blocksize == OCFS2_MIN_BLOCKSIZE)
return -ENOSPC;
@@ -2630,8 +2602,24 @@ static int ocfs2_xattr_ibody_set(struct inode *inode,
}
}
- ret = ocfs2_xattr_set_entry(inode, xi, xs, ctxt,
- (OCFS2_INLINE_XATTR_FL | OCFS2_HAS_XATTR_FL));
+ if (!(oi->ip_dyn_features & OCFS2_INLINE_XATTR_FL)) {
+ ret = ocfs2_xattr_ibody_init(inode, xs->inode_bh, ctxt);
+ if (ret) {
+ mlog_errno(ret);
+ goto out;
+ }
+ }
+
+ ocfs2_init_dinode_xa_loc(&loc, inode, xs->inode_bh,
+ xs->not_found ? NULL : xs->here);
+ ret = ocfs2_xa_set(&loc, xi, ctxt);
+ if (ret) {
+ if (ret != -ENOSPC)
+ mlog_errno(ret);
+ goto out;
+ }
+ xs->here = loc.xl_entry;
+
out:
up_write(&oi->ip_alloc_sem);
--
1.6.3.3
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 01/14] ocfs2: Introduce ocfs2_xa_loc
2009-08-28 8:35 ` [Ocfs2-devel] [PATCH 01/14] ocfs2: Introduce ocfs2_xa_loc Joel Becker
@ 2009-09-01 5:32 ` Tao Ma
2009-09-01 8:26 ` Joel Becker
0 siblings, 1 reply; 45+ messages in thread
From: Tao Ma @ 2009-09-01 5:32 UTC (permalink / raw)
To: ocfs2-devel
Hi Joel,
Joel Becker wrote:
> The ocfs2 extended attribute (xattr) code is very flexible. It can
> store xattrs in the inode itself, in an external block, or in a tree of
> data structures. This allows the number of xattrs to be bounded by the
> filesystem size.
>
> However, the code that manages each possible storage location is
> different. Maintaining the ocfs2 xattr code requires changing each hunk
> separately.
>
> This patch is the start of a series introducing the ocfs2_xa_loc
> structure. This structure wraps the on-disk details of an xattr
> entry. The goal is that the generic xattr routines can use
> ocfs2_xa_loc without knowing the underlying storage location.
>
> This first pass merely implements the basic structure, initializing it,
> and wiping the name+value pair of the entry.
>
> Signed-off-by: Joel Becker <joel.becker@oracle.com>
> ---
> fs/ocfs2/xattr.c | 242 ++++++++++++++++++++++++++++++++++++++++++++++++++----
> 1 files changed, 227 insertions(+), 15 deletions(-)
>
> diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
> index a3eed49..848f24e 100644
> --- a/fs/ocfs2/xattr.c
> +++ b/fs/ocfs2/xattr.c
> @@ -141,6 +141,51 @@ struct ocfs2_xattr_search {
> int not_found;
> };
>
> +/* Operations on struct ocfs2_xa_entry */
> +struct ocfs2_xa_loc;
> +struct ocfs2_xa_loc_operations {
> + /*
> + * Return a pointer to the appropriate buffer in loc->xl_storage
> + * at the given offset from loc->xl_header.
> + */
> + void *(*xlo_offset_pointer)(struct ocfs2_xa_loc *loc, int offset);
> +
> + /*
> + * Remove the name+value at this location. Do whatever is
> + * appropriate with the remaining name+value pairs.
> + */
> + void (*xlo_wipe_namevalue)(struct ocfs2_xa_loc *loc);
> +};
<snip>
> +
> +static void *ocfs2_xa_block_offset_pointer(struct ocfs2_xa_loc *loc,
> + int offset)
> +{
> + struct buffer_head *bh = loc->xl_storage;
> +
> + BUG_ON(offset >= bh->b_size);
> + return bh->b_data + offset;
> +}
It looks that the "offset" is from the start of the buffer, while in
your description above "offset" is from "loc->xl_header". In both inline
and block case, header != buffer_start. xl_size seems to be against
xl_header, so I would guess here you need to change somehow.
Regards,
Tao
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 02/14] ocfs2: Remove xattrs via ocfs2_xa_loc
2009-08-28 8:35 ` [Ocfs2-devel] [PATCH 02/14] ocfs2: Remove xattrs via ocfs2_xa_loc Joel Becker
@ 2009-09-01 6:03 ` Tao Ma
2009-09-01 8:28 ` Joel Becker
0 siblings, 1 reply; 45+ messages in thread
From: Tao Ma @ 2009-09-01 6:03 UTC (permalink / raw)
To: ocfs2-devel
Joel Becker wrote:
> Add ocfs2_xa_remove_entry(), which will remove an xattr entry from its
> storage via the ocfs2_xa_loc descriptor.
>
> Signed-off-by: Joel Becker <joel.becker@oracle.com>
> ---
> fs/ocfs2/xattr.c | 61 ++++++++++++++++++++++++-----------------------------
> 1 files changed, 28 insertions(+), 33 deletions(-)
>
> diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
> index 848f24e..333d384 100644
> --- a/fs/ocfs2/xattr.c
> +++ b/fs/ocfs2/xattr.c
> @@ -1580,7 +1580,28 @@ static const struct ocfs2_xa_loc_operations ocfs2_xa_bucket_loc_ops = {
>
> static void ocfs2_xa_remove_entry(struct ocfs2_xa_loc *loc)
> {
> + int index, count;
> + struct ocfs2_xattr_header *xh = loc->xl_header;
> + struct ocfs2_xattr_entry *entry = loc->xl_entry;
> +
> ocfs2_xa_wipe_namevalue(loc);
> +
> + le16_add_cpu(&xh->xh_count, -1);
> + count = le16_to_cpu(xh->xh_count);
> +
> + /*
> + * Only zero out the entry if there are more remaining. This is
> + * important for an empty bucket, as it keeps track of the
> + * bucket's hash value. It doesn't hurt empty block storage.
> + */
> + if (count) {
> + index = ((char *)entry - (char *)&xh->xh_entries) /
> + sizeof(struct ocfs2_xattr_entry);
index = entry - xh->xh_entries should work?
> + memmove(&xh->xh_entries[index], &xh->xh_entries[index + 1],
> + (count - index) * sizeof(struct ocfs2_xattr_entry));
> + memset(&xh->xh_entries[count], 0,
> + sizeof(struct ocfs2_xattr_entry));
> + }
> }
Regards,
Tao
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 06/14] ocfs2: Set the xattr name+value pair in one place
2009-08-28 8:35 ` [Ocfs2-devel] [PATCH 06/14] ocfs2: Set the xattr name+value pair in one place Joel Becker
@ 2009-09-01 7:33 ` Tao Ma
2009-09-01 8:30 ` Joel Becker
0 siblings, 1 reply; 45+ messages in thread
From: Tao Ma @ 2009-09-01 7:33 UTC (permalink / raw)
To: ocfs2-devel
Hi Joel,
Joel Becker wrote:
> We create two new functions on ocfs2_xa_loc, ocfs2_xa_prepare_entry()
> and ocfs2_xa_store_inline_value().
>
> ocfs2_xa_prepare_entry() makes sure that the xl_entry field of
> ocfs2_xa_loc is ready to receive an xattr. The entry will point to an
> appropriately sized name+value region in storage. If an existing entry
> can be reused, it will be. If no entry already exists, it will be
> allocated. If there isn't space to allocate it, -ENOSPC will be
> returned.
>
> ocfs2_xa_store_inline_value() stores the data that goes into the 'value'
> part of the name+value pair. For values that don't fit directly, this
> stores the value tree root.
>
> A number of operations are added to ocfs2_xa_loc_operations to support
> these functions. This reflects the disparate behaviors of xattr blocks
> and buckets.
>
> With these functions, the overlapping ocfs2_xattr_set_entry_local() and
> ocfs2_xattr_set_entry_normal() can be replaced with a single call
> scheme.
>
> Signed-off-by: Joel Becker <joel.becker@oracle.com>
> ---
> fs/ocfs2/xattr.c | 606 ++++++++++++++++++++++++++++++++++--------------------
> 1 files changed, 386 insertions(+), 220 deletions(-)
>
> diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
> index 4c5c566..3d14c1c 100644
> --- a/fs/ocfs2/xattr.c
> +++ b/fs/ocfs2/xattr.c
<snip>
> +/*
> + * Prepares loc->xl_entry to receive the new xattr. This includes
> + * properly setting up the name+value pair region. If loc->xl_entry
> + * already exists, it will take care of modifying it appropriately.
> + * This also includes deleting entries, but don't call this to remove
> + * a non-existant entry. That's just a bug.
> + *
> + * Note that this modifies the data. You did journal_access already,
> + * right?
> + */
> +static int ocfs2_xa_prepare_entry(struct ocfs2_xa_loc *loc,
> + struct ocfs2_xattr_info *xi,
> + u32 name_hash)
> +{
> + int rc = 0;
> + int name_size = OCFS2_XATTR_SIZE(xi->xi_name_len);
> + char *nameval_buf;
> +
> + if (!xi->xi_value) {
> + ocfs2_xa_remove_entry(loc);
> + goto out;
> + }
> +
> + rc = ocfs2_xa_has_space(loc, xi);
> + if (rc)
> + goto out;
could you please add some comments here or change the function name.
when I read ocfs2_xa_has_space, I always think that "if we have space,
goto out". But actually we get 0 here if we have space.
> +
> + if (loc->xl_entry) {
> + if (ocfs2_xa_can_reuse_entry(loc, xi)) {
> + nameval_buf = ocfs2_xa_offset_pointer(loc,
> + le16_to_cpu(loc->xl_entry->xe_name_offset));
oh, I see you use ocfs2_xa_offset_pointer and offset = xe_name_offset
here. So in patch 01/14, that is really a bug in
ocfs2_xa_block_offset_pointer.
Regards,
Tao
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 01/14] ocfs2: Introduce ocfs2_xa_loc
2009-09-01 5:32 ` Tao Ma
@ 2009-09-01 8:26 ` Joel Becker
2009-09-01 8:45 ` Joel Becker
0 siblings, 1 reply; 45+ messages in thread
From: Joel Becker @ 2009-09-01 8:26 UTC (permalink / raw)
To: ocfs2-devel
On Tue, Sep 01, 2009 at 01:32:37PM +0800, Tao Ma wrote:
> Joel Becker wrote:
> >+static void *ocfs2_xa_block_offset_pointer(struct ocfs2_xa_loc *loc,
> >+ int offset)
> >+{
> >+ struct buffer_head *bh = loc->xl_storage;
> >+
> >+ BUG_ON(offset >= bh->b_size);
> >+ return bh->b_data + offset;
> >+}
> It looks that the "offset" is from the start of the buffer, while in
> your description above "offset" is from "loc->xl_header". In both
> inline and block case, header != buffer_start. xl_size seems to be
> against xl_header, so I would guess here you need to change somehow.
Doh! That's a bug. The description matches reality -
xe_name_offset is from the start of the header, not the block, right?
So this function is just wrong!
Joel
--
"We will have to repent in this generation not merely for the
vitriolic words and actions of the bad people, but for the
appalling silence of the good people."
- Rev. Dr. Martin Luther King, Jr.
Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 02/14] ocfs2: Remove xattrs via ocfs2_xa_loc
2009-09-01 6:03 ` Tao Ma
@ 2009-09-01 8:28 ` Joel Becker
0 siblings, 0 replies; 45+ messages in thread
From: Joel Becker @ 2009-09-01 8:28 UTC (permalink / raw)
To: ocfs2-devel
On Tue, Sep 01, 2009 at 02:03:55PM +0800, Tao Ma wrote:
> >+ if (count) {
> >+ index = ((char *)entry - (char *)&xh->xh_entries) /
> >+ sizeof(struct ocfs2_xattr_entry);
> index = entry - xh->xh_entries should work?
Yes, but I never use that C math, because it confuses people.
Converting to char* means it's byte math.
Joel
--
Life's Little Instruction Book #30
"Never buy a house without a fireplace."
Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 06/14] ocfs2: Set the xattr name+value pair in one place
2009-09-01 7:33 ` Tao Ma
@ 2009-09-01 8:30 ` Joel Becker
2009-09-01 8:47 ` Tao Ma
0 siblings, 1 reply; 45+ messages in thread
From: Joel Becker @ 2009-09-01 8:30 UTC (permalink / raw)
To: ocfs2-devel
On Tue, Sep 01, 2009 at 03:33:07PM +0800, Tao Ma wrote:
> Joel Becker wrote:
> >+ rc = ocfs2_xa_has_space(loc, xi);
> >+ if (rc)
> >+ goto out;
> could you please add some comments here or change the function name.
> when I read ocfs2_xa_has_space, I always think that "if we have
> space, goto out". But actually we get 0 here if we have space.
A very good point. It really should be ocfs2_xa_space_needed().
Does that work?
> >+ if (loc->xl_entry) {
> >+ if (ocfs2_xa_can_reuse_entry(loc, xi)) {
> >+ nameval_buf = ocfs2_xa_offset_pointer(loc,
> >+ le16_to_cpu(loc->xl_entry->xe_name_offset));
> oh, I see you use ocfs2_xa_offset_pointer and offset =
> xe_name_offset here. So in patch 01/14, that is really a bug in
> ocfs2_xa_block_offset_pointer.
Yeah, total bug.
Joel
--
"In a crisis, don't hide behind anything or anybody. They're going
to find you anyway."
- Paul "Bear" Bryant
Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 01/14] ocfs2: Introduce ocfs2_xa_loc
2009-09-01 8:26 ` Joel Becker
@ 2009-09-01 8:45 ` Joel Becker
2009-09-01 8:49 ` Tao Ma
0 siblings, 1 reply; 45+ messages in thread
From: Joel Becker @ 2009-09-01 8:45 UTC (permalink / raw)
To: ocfs2-devel
On Tue, Sep 01, 2009 at 01:26:54AM -0700, Joel Becker wrote:
> On Tue, Sep 01, 2009 at 01:32:37PM +0800, Tao Ma wrote:
> > Joel Becker wrote:
> > >+static void *ocfs2_xa_block_offset_pointer(struct ocfs2_xa_loc *loc,
> > >+ int offset)
> > >+{
> > >+ struct buffer_head *bh = loc->xl_storage;
> > >+
> > >+ BUG_ON(offset >= bh->b_size);
> > >+ return bh->b_data + offset;
> > >+}
> > It looks that the "offset" is from the start of the buffer, while in
> > your description above "offset" is from "loc->xl_header". In both
> > inline and block case, header != buffer_start. xl_size seems to be
> > against xl_header, so I would guess here you need to change somehow.
>
> Doh! That's a bug. The description matches reality -
> xe_name_offset is from the start of the header, not the block, right?
> So this function is just wrong!
What about this delta?
Joel
diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index d485932..abc8043 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -1442,6 +1442,7 @@ static void ocfs2_xa_journal_dirty(handle_t *handle, struct ocfs2_xa_loc *loc)
/* Give a pointer into the storage for the given offset */
static void *ocfs2_xa_offset_pointer(struct ocfs2_xa_loc *loc, int offset)
{
+ BUG_ON(offset >= loc->xl_size);
return loc->xl_ops->xlo_offset_pointer(loc, offset);
}
@@ -1552,10 +1553,7 @@ static void ocfs2_xa_block_journal_dirty(handle_t *handle,
static void *ocfs2_xa_block_offset_pointer(struct ocfs2_xa_loc *loc,
int offset)
{
- struct buffer_head *bh = loc->xl_storage;
-
- BUG_ON(offset >= bh->b_size);
- return bh->b_data + offset;
+ return (char *)loc->xl_header + offset;
}
static int ocfs2_xa_block_can_reuse(struct ocfs2_xa_loc *loc,
@@ -1712,8 +1710,7 @@ static void *ocfs2_xa_bucket_offset_pointer(struct ocfs2_xa_loc *loc,
struct ocfs2_xattr_bucket *bucket = loc->xl_storage;
int block, block_offset;
- BUG_ON(offset >= OCFS2_XATTR_BUCKET_SIZE);
-
+ /* The header is at the front of the bucket */
block = offset >> loc->xl_inode->i_sb->s_blocksize_bits;
block_offset = offset % loc->xl_inode->i_sb->s_blocksize;
--
"Three o'clock is always too late or too early for anything you
want to do."
- Jean-Paul Sartre
Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 06/14] ocfs2: Set the xattr name+value pair in one place
2009-09-01 8:30 ` Joel Becker
@ 2009-09-01 8:47 ` Tao Ma
2009-09-01 9:30 ` Joel Becker
0 siblings, 1 reply; 45+ messages in thread
From: Tao Ma @ 2009-09-01 8:47 UTC (permalink / raw)
To: ocfs2-devel
Joel Becker wrote:
> On Tue, Sep 01, 2009 at 03:33:07PM +0800, Tao Ma wrote:
>> Joel Becker wrote:
>>> + rc = ocfs2_xa_has_space(loc, xi);
>>> + if (rc)
>>> + goto out;
>> could you please add some comments here or change the function name.
>> when I read ocfs2_xa_has_space, I always think that "if we have
>> space, goto out". But actually we get 0 here if we have space.
>
> A very good point. It really should be ocfs2_xa_space_needed().
> Does that work?
actually your function just return either -ENOSPC, -EIO or 0.
And we go ahead when we get 0. so maybe ocfs2_xa_check_space? So <0
means we meet with some errors and 0 means OK.
Regards,
Tao
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 01/14] ocfs2: Introduce ocfs2_xa_loc
2009-09-01 8:45 ` Joel Becker
@ 2009-09-01 8:49 ` Tao Ma
0 siblings, 0 replies; 45+ messages in thread
From: Tao Ma @ 2009-09-01 8:49 UTC (permalink / raw)
To: ocfs2-devel
Joel Becker wrote:
> On Tue, Sep 01, 2009 at 01:26:54AM -0700, Joel Becker wrote:
>> On Tue, Sep 01, 2009 at 01:32:37PM +0800, Tao Ma wrote:
>>> Joel Becker wrote:
>>>> +static void *ocfs2_xa_block_offset_pointer(struct ocfs2_xa_loc *loc,
>>>> + int offset)
>>>> +{
>>>> + struct buffer_head *bh = loc->xl_storage;
>>>> +
>>>> + BUG_ON(offset >= bh->b_size);
>>>> + return bh->b_data + offset;
>>>> +}
>>> It looks that the "offset" is from the start of the buffer, while in
>>> your description above "offset" is from "loc->xl_header". In both
>>> inline and block case, header != buffer_start. xl_size seems to be
>>> against xl_header, so I would guess here you need to change somehow.
>> Doh! That's a bug. The description matches reality -
>> xe_name_offset is from the start of the header, not the block, right?
>> So this function is just wrong!
>
> What about this delta?
looks good to me.
Regards,
Tao
>
> Joel
>
>
> diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
> index d485932..abc8043 100644
> --- a/fs/ocfs2/xattr.c
> +++ b/fs/ocfs2/xattr.c
> @@ -1442,6 +1442,7 @@ static void ocfs2_xa_journal_dirty(handle_t *handle, struct ocfs2_xa_loc *loc)
> /* Give a pointer into the storage for the given offset */
> static void *ocfs2_xa_offset_pointer(struct ocfs2_xa_loc *loc, int offset)
> {
> + BUG_ON(offset >= loc->xl_size);
> return loc->xl_ops->xlo_offset_pointer(loc, offset);
> }
>
> @@ -1552,10 +1553,7 @@ static void ocfs2_xa_block_journal_dirty(handle_t *handle,
> static void *ocfs2_xa_block_offset_pointer(struct ocfs2_xa_loc *loc,
> int offset)
> {
> - struct buffer_head *bh = loc->xl_storage;
> -
> - BUG_ON(offset >= bh->b_size);
> - return bh->b_data + offset;
> + return (char *)loc->xl_header + offset;
> }
>
> static int ocfs2_xa_block_can_reuse(struct ocfs2_xa_loc *loc,
> @@ -1712,8 +1710,7 @@ static void *ocfs2_xa_bucket_offset_pointer(struct ocfs2_xa_loc *loc,
> struct ocfs2_xattr_bucket *bucket = loc->xl_storage;
> int block, block_offset;
>
> - BUG_ON(offset >= OCFS2_XATTR_BUCKET_SIZE);
> -
> + /* The header is at the front of the bucket */
> block = offset >> loc->xl_inode->i_sb->s_blocksize_bits;
> block_offset = offset % loc->xl_inode->i_sb->s_blocksize;
>
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 10/14] ocfs2: Allocation in ocfs2_xa_prepare_entry() values in ocfs2_xa_store_value()
2009-08-28 8:35 ` [Ocfs2-devel] [PATCH 10/14] ocfs2: Allocation in ocfs2_xa_prepare_entry() values in ocfs2_xa_store_value() Joel Becker
@ 2009-09-01 8:55 ` Tao Ma
2009-09-01 20:21 ` Joel Becker
0 siblings, 1 reply; 45+ messages in thread
From: Tao Ma @ 2009-09-01 8:55 UTC (permalink / raw)
To: ocfs2-devel
Joel Becker wrote:
> ocfs2_xa_prepare_entry() gets all the logic to add, remove, or modify
> external value trees. Now, when it exits, the entry is ready to receive
> a value of any size.
>
> ocfs2_xa_store_inline_value() becomes ocfs2_xa_store_value(). It can
> store any value.
>
> ocfs2_xattr_set_entry() loses all the allocation logic and just uses
> these functions. ocfs2_xattr_set_value_outside() disappears.
>
> ocfs2_xattr_set_in_bucket() uses these functions and makes
> ocfs2_xattr_set_entry_in_bucket() obsolete. That goes away, as does
> ocfs2_xattr_bucket_set_value_outside() and
> ocfs2_xattr_bucket_value_truncate().
>
> Signed-off-by: Joel Becker <joel.becker@oracle.com>
> ---
> fs/ocfs2/xattr.c | 611 ++++++++++++------------------------------------------
> 1 files changed, 138 insertions(+), 473 deletions(-)
>
> diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
> index 1bc12fa..d922ad9 100644
> --- a/fs/ocfs2/xattr.c
> +++ b/fs/ocfs2/xattr.c
<snip>
> +/*
> + * Take an existing entry and make it ready for the new value. This
> + * won't allocate space, but it may free space. It should be ready for
> + * ocfs2_xa_prepare_entry() to finish the work.
> + */
> +static int ocfs2_xa_reuse_entry(struct ocfs2_xa_loc *loc,
> + struct ocfs2_xattr_info *xi,
> + struct ocfs2_xattr_set_ctxt *ctxt)
> +{
> + int rc = 0;
> + int name_size = OCFS2_XATTR_SIZE(xi->xi_name_len);
> + char *nameval_buf;
> + int xe_local = ocfs2_xattr_is_local(loc->xl_entry);
> + int xi_local = xi->xi_value_len <= OCFS2_XATTR_INLINE_SIZE;
> +
> + BUG_ON(OCFS2_XATTR_SIZE(loc->xl_entry->xe_name_len) !=
> + name_size);
> +
> + nameval_buf = ocfs2_xa_offset_pointer(loc,
> + le16_to_cpu(loc->xl_entry->xe_name_offset));
> + if (xe_local) {
> + memset(nameval_buf + name_size, 0,
> + namevalue_size_xe(loc->xl_entry) - name_size);
> + if (!xi_local)
> + ocfs2_xa_install_value_root(loc);
> + } else {
> + if (xi_local) {
> + rc = ocfs2_xa_value_truncate(loc, 0, ctxt);
> + if (rc < 0) {
> + mlog_errno(rc);
> + goto out;
> + }
> + memset(nameval_buf + name_size, 0,
> + namevalue_size_xe(loc->xl_entry) -
> + name_size);
> + } else if (le64_to_cpu(loc->xl_entry->xe_value_size) >
> + xi->xi_value_len) {
> + rc = ocfs2_xa_value_truncate(loc, xi->xi_value_len,
> + ctxt);
From your comments above, we don't allocate space. I am just curious
why we can't extend it here if value_size < xi->xi_value_len?
> + if (rc < 0) {
> + mlog_errno(rc);
> + goto out;
> + }
> + }
> + }
> +
> + loc->xl_entry->xe_value_size = cpu_to_le64(xi->xi_value_len);
> + ocfs2_xattr_set_local(loc->xl_entry, xi_local);
> +
> +out:
> + return rc;
> +}
> +
> /*
> * Prepares loc->xl_entry to receive the new xattr. This includes
> * properly setting up the name+value pair region. If loc->xl_entry
> @@ -2027,11 +1974,10 @@ static void ocfs2_xa_remove_entry(struct ocfs2_xa_loc *loc)
> */
> static int ocfs2_xa_prepare_entry(struct ocfs2_xa_loc *loc,
> struct ocfs2_xattr_info *xi,
> - u32 name_hash)
> + u32 name_hash,
> + struct ocfs2_xattr_set_ctxt *ctxt)
> {
> int rc = 0;
> - int name_size = OCFS2_XATTR_SIZE(xi->xi_name_len);
> - char *nameval_buf;
>
> if (!xi->xi_value) {
> ocfs2_xa_remove_entry(loc);
> @@ -2044,13 +1990,10 @@ static int ocfs2_xa_prepare_entry(struct ocfs2_xa_loc *loc,
>
> if (loc->xl_entry) {
> if (ocfs2_xa_can_reuse_entry(loc, xi)) {
> - nameval_buf = ocfs2_xa_offset_pointer(loc,
> - le16_to_cpu(loc->xl_entry->xe_name_offset));
> - memset(nameval_buf + name_size, 0,
> - namevalue_size_xe(loc->xl_entry) - name_size);
> - loc->xl_entry->xe_value_size =
> - cpu_to_le64(xi->xi_value_len);
> - goto out;
> + rc = ocfs2_xa_reuse_entry(loc, xi, ctxt);
> + if (rc)
> + goto out;
> + goto alloc_value;
As I said above in xa_reuse_entry, if we have already "alloc value" in
ocfs2_xa_reuse_entry, we don't need to goto it again?
> }
>
> ocfs2_xa_wipe_namevalue(loc);
> static void ocfs2_init_dinode_xa_loc(struct ocfs2_xa_loc *loc,
<snip>
> @@ -2336,28 +2195,6 @@ static int ocfs2_xattr_set_entry(struct inode *inode,
> if (ret < 0)
> mlog_errno(ret);
>
> - if (!ret && xi->xi_value_len > OCFS2_XATTR_INLINE_SIZE) {
> - /*
> - * Set value outside in B tree.
> - * This is the second step for value size > INLINE_SIZE.
> - */
> - size_t offs = le16_to_cpu(xs->here->xe_name_offset);
> - ret = ocfs2_xattr_set_value_outside(inode, xi, xs, ctxt,
> - &vb, offs);
> - if (ret < 0) {
> - int ret2;
> -
> - mlog_errno(ret);
> - /*
> - * If set value outside failed, we have to clean
> - * the junk tree root we have already set in local.
> - */
> - ret2 = ocfs2_xattr_cleanup(inode, ctxt->handle,
> - xi, xs, &vb, offs);
you just removed this part which will remove the entry if we fail in
cluster extension. I haven't seen some new implementation like that. Am
I missing something here?
> - if (ret2 < 0)
> - mlog_errno(ret2);
> - }
> - }
> out:
> return ret;
> }
Regards,
Tao
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 06/14] ocfs2: Set the xattr name+value pair in one place
2009-09-01 8:47 ` Tao Ma
@ 2009-09-01 9:30 ` Joel Becker
2009-09-01 12:12 ` Tao Ma
0 siblings, 1 reply; 45+ messages in thread
From: Joel Becker @ 2009-09-01 9:30 UTC (permalink / raw)
To: ocfs2-devel
On Tue, Sep 01, 2009 at 04:47:41PM +0800, Tao Ma wrote:
> Joel Becker wrote:
> >On Tue, Sep 01, 2009 at 03:33:07PM +0800, Tao Ma wrote:
> >>Joel Becker wrote:
> >>>+ rc = ocfs2_xa_has_space(loc, xi);
> >>>+ if (rc)
> >>>+ goto out;
> >>could you please add some comments here or change the function name.
> >>when I read ocfs2_xa_has_space, I always think that "if we have
> >>space, goto out". But actually we get 0 here if we have space.
> >
> > A very good point. It really should be ocfs2_xa_space_needed().
> >Does that work?
> actually your function just return either -ENOSPC, -EIO or 0.
> And we go ahead when we get 0. so maybe ocfs2_xa_check_space? So <0
> means we meet with some errors and 0 means OK.
How's this?
Joel
diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index 2e8f5c6..b1d05ef 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -156,8 +156,8 @@ struct ocfs2_xa_loc_operations {
struct ocfs2_xattr_info *xi);
/* How much space is needed for the new value? */
- int (*xlo_has_space)(struct ocfs2_xa_loc *loc,
- struct ocfs2_xattr_info *xi);
+ int (*xlo_check_space)(struct ocfs2_xa_loc *loc,
+ struct ocfs2_xattr_info *xi);
/*
* Return the offset of the first name+value pair. This is
@@ -1519,8 +1519,8 @@ static int ocfs2_xattr_set_value_outside(struct inode *inode,
return ret;
}
-static int ocfs2_xa_has_space_helper(int needed_space, int free_start,
- int num_entries)
+static int ocfs2_xa_check_space_helper(int needed_space, int free_start,
+ int num_entries)
{
int free_space;
@@ -1569,10 +1570,10 @@ static int ocfs2_xa_can_reuse_entry(struct ocfs2_xa_loc *loc,
}
/* How much free space is needed to set the new value */
-static int ocfs2_xa_has_space(struct ocfs2_xa_loc *loc,
- struct ocfs2_xattr_info *xi)
+static int ocfs2_xa_check_space(struct ocfs2_xa_loc *loc,
+ struct ocfs2_xattr_info *xi)
{
- return loc->xl_ops->xlo_has_space(loc, xi);
+ return loc->xl_ops->xlo_check_space(loc, xi);
}
static void ocfs2_xa_add_entry(struct ocfs2_xa_loc *loc, u32 name_hash)
@@ -1639,8 +1639,8 @@ static int ocfs2_xa_block_get_free_start(struct ocfs2_xa_loc *loc)
return free_start;
}
-static int ocfs2_xa_block_has_space(struct ocfs2_xa_loc *loc,
- struct ocfs2_xattr_info *xi)
+static int ocfs2_xa_block_check_space(struct ocfs2_xa_loc *loc,
+ struct ocfs2_xattr_info *xi)
{
int count = le16_to_cpu(loc->xl_header->xh_count);
int free_start = ocfs2_xa_get_free_start(loc);
@@ -1660,7 +1660,7 @@ static int ocfs2_xa_block_has_space(struct ocfs2_xa_loc *loc,
}
if (needed_space < 0)
needed_space = 0;
- return ocfs2_xa_has_space_helper(needed_space, free_start, count);
+ return ocfs2_xa_check_space_helper(needed_space, free_start, count);
}
/*
@@ -1720,7 +1720,7 @@ static void ocfs2_xa_block_add_namevalue(struct ocfs2_xa_loc *loc, int size)
*/
static const struct ocfs2_xa_loc_operations ocfs2_xa_block_loc_ops = {
.xlo_offset_pointer = ocfs2_xa_block_offset_pointer,
- .xlo_has_space = ocfs2_xa_block_has_space,
+ .xlo_check_space = ocfs2_xa_block_check_space,
.xlo_can_reuse = ocfs2_xa_block_can_reuse,
.xlo_get_free_start = ocfs2_xa_block_get_free_start,
.xlo_wipe_namevalue = ocfs2_xa_block_wipe_namevalue,
@@ -1770,8 +1768,8 @@ static int ocfs2_bucket_align_free_start(struct super_block *sb,
return free_start;
}
-static int ocfs2_xa_bucket_has_space(struct ocfs2_xa_loc *loc,
- struct ocfs2_xattr_info *xi)
+static int ocfs2_xa_bucket_check_space(struct ocfs2_xa_loc *loc,
+ struct ocfs2_xattr_info *xi)
{
int count = le16_to_cpu(loc->xl_header->xh_count);
int free_start = ocfs2_xa_get_free_start(loc);
@@ -1796,7 +1794,7 @@ static int ocfs2_xa_bucket_has_space(struct ocfs2_xa_loc *loc,
BUG_ON(needed_space < 0);
free_start = ocfs2_bucket_align_free_start(sb, free_start, size);
- return ocfs2_xa_has_space_helper(needed_space, free_start, count);
+ return ocfs2_xa_check_space_helper(needed_space, free_start, count);
}
static void ocfs2_xa_bucket_wipe_namevalue(struct ocfs2_xa_loc *loc)
@@ -1859,7 +1857,7 @@ static void ocfs2_xa_bucket_add_namevalue(struct ocfs2_xa_loc *loc, int size)
/* Operations for xattrs stored in buckets. */
static const struct ocfs2_xa_loc_operations ocfs2_xa_bucket_loc_ops = {
.xlo_offset_pointer = ocfs2_xa_bucket_offset_pointer,
- .xlo_has_space = ocfs2_xa_bucket_has_space,
+ .xlo_check_space = ocfs2_xa_bucket_check_space,
.xlo_can_reuse = ocfs2_xa_bucket_can_reuse,
.xlo_get_free_start = ocfs2_xa_bucket_get_free_start,
.xlo_wipe_namevalue = ocfs2_xa_bucket_wipe_namevalue,
@@ -1916,7 +1914,7 @@ static int ocfs2_xa_prepare_entry(struct ocfs2_xa_loc *loc,
goto out;
}
- rc = ocfs2_xa_has_space(loc, xi);
+ rc = ocfs2_xa_check_space(loc, xi);
if (rc)
goto out;
--
"Drake! We're LEAVING!"
Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker@oracle.com
Phone: (650) 506-8127
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 06/14] ocfs2: Set the xattr name+value pair in one place
2009-09-01 9:30 ` Joel Becker
@ 2009-09-01 12:12 ` Tao Ma
0 siblings, 0 replies; 45+ messages in thread
From: Tao Ma @ 2009-09-01 12:12 UTC (permalink / raw)
To: ocfs2-devel
Joel Becker wrote:
> On Tue, Sep 01, 2009 at 04:47:41PM +0800, Tao Ma wrote:
>
>> Joel Becker wrote:
>>
>>> On Tue, Sep 01, 2009 at 03:33:07PM +0800, Tao Ma wrote:
>>>
>>>> Joel Becker wrote:
>>>>
>>>>> + rc = ocfs2_xa_has_space(loc, xi);
>>>>> + if (rc)
>>>>> + goto out;
>>>>>
>>>> could you please add some comments here or change the function name.
>>>> when I read ocfs2_xa_has_space, I always think that "if we have
>>>> space, goto out". But actually we get 0 here if we have space.
>>>>
>>> A very good point. It really should be ocfs2_xa_space_needed().
>>> Does that work?
>>>
>> actually your function just return either -ENOSPC, -EIO or 0.
>> And we go ahead when we get 0. so maybe ocfs2_xa_check_space? So <0
>> means we meet with some errors and 0 means OK.
>>
>
> How's this?
>
looks good to me.
Regards,
Tao
> Joel
>
>
> diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
> index 2e8f5c6..b1d05ef 100644
> --- a/fs/ocfs2/xattr.c
> +++ b/fs/ocfs2/xattr.c
> @@ -156,8 +156,8 @@ struct ocfs2_xa_loc_operations {
> struct ocfs2_xattr_info *xi);
>
> /* How much space is needed for the new value? */
> - int (*xlo_has_space)(struct ocfs2_xa_loc *loc,
> - struct ocfs2_xattr_info *xi);
> + int (*xlo_check_space)(struct ocfs2_xa_loc *loc,
> + struct ocfs2_xattr_info *xi);
>
> /*
> * Return the offset of the first name+value pair. This is
> @@ -1519,8 +1519,8 @@ static int ocfs2_xattr_set_value_outside(struct inode *inode,
> return ret;
> }
>
> -static int ocfs2_xa_has_space_helper(int needed_space, int free_start,
> - int num_entries)
> +static int ocfs2_xa_check_space_helper(int needed_space, int free_start,
> + int num_entries)
> {
> int free_space;
>
> @@ -1569,10 +1570,10 @@ static int ocfs2_xa_can_reuse_entry(struct ocfs2_xa_loc *loc,
> }
>
> /* How much free space is needed to set the new value */
> -static int ocfs2_xa_has_space(struct ocfs2_xa_loc *loc,
> - struct ocfs2_xattr_info *xi)
> +static int ocfs2_xa_check_space(struct ocfs2_xa_loc *loc,
> + struct ocfs2_xattr_info *xi)
> {
> - return loc->xl_ops->xlo_has_space(loc, xi);
> + return loc->xl_ops->xlo_check_space(loc, xi);
> }
>
> static void ocfs2_xa_add_entry(struct ocfs2_xa_loc *loc, u32 name_hash)
> @@ -1639,8 +1639,8 @@ static int ocfs2_xa_block_get_free_start(struct ocfs2_xa_loc *loc)
> return free_start;
> }
>
> -static int ocfs2_xa_block_has_space(struct ocfs2_xa_loc *loc,
> - struct ocfs2_xattr_info *xi)
> +static int ocfs2_xa_block_check_space(struct ocfs2_xa_loc *loc,
> + struct ocfs2_xattr_info *xi)
> {
> int count = le16_to_cpu(loc->xl_header->xh_count);
> int free_start = ocfs2_xa_get_free_start(loc);
> @@ -1660,7 +1660,7 @@ static int ocfs2_xa_block_has_space(struct ocfs2_xa_loc *loc,
> }
> if (needed_space < 0)
> needed_space = 0;
> - return ocfs2_xa_has_space_helper(needed_space, free_start, count);
> + return ocfs2_xa_check_space_helper(needed_space, free_start, count);
> }
>
> /*
> @@ -1720,7 +1720,7 @@ static void ocfs2_xa_block_add_namevalue(struct ocfs2_xa_loc *loc, int size)
> */
> static const struct ocfs2_xa_loc_operations ocfs2_xa_block_loc_ops = {
> .xlo_offset_pointer = ocfs2_xa_block_offset_pointer,
> - .xlo_has_space = ocfs2_xa_block_has_space,
> + .xlo_check_space = ocfs2_xa_block_check_space,
> .xlo_can_reuse = ocfs2_xa_block_can_reuse,
> .xlo_get_free_start = ocfs2_xa_block_get_free_start,
> .xlo_wipe_namevalue = ocfs2_xa_block_wipe_namevalue,
> @@ -1770,8 +1768,8 @@ static int ocfs2_bucket_align_free_start(struct super_block *sb,
> return free_start;
> }
>
> -static int ocfs2_xa_bucket_has_space(struct ocfs2_xa_loc *loc,
> - struct ocfs2_xattr_info *xi)
> +static int ocfs2_xa_bucket_check_space(struct ocfs2_xa_loc *loc,
> + struct ocfs2_xattr_info *xi)
> {
> int count = le16_to_cpu(loc->xl_header->xh_count);
> int free_start = ocfs2_xa_get_free_start(loc);
> @@ -1796,7 +1794,7 @@ static int ocfs2_xa_bucket_has_space(struct ocfs2_xa_loc *loc,
> BUG_ON(needed_space < 0);
>
> free_start = ocfs2_bucket_align_free_start(sb, free_start, size);
> - return ocfs2_xa_has_space_helper(needed_space, free_start, count);
> + return ocfs2_xa_check_space_helper(needed_space, free_start, count);
> }
>
> static void ocfs2_xa_bucket_wipe_namevalue(struct ocfs2_xa_loc *loc)
> @@ -1859,7 +1857,7 @@ static void ocfs2_xa_bucket_add_namevalue(struct ocfs2_xa_loc *loc, int size)
> /* Operations for xattrs stored in buckets. */
> static const struct ocfs2_xa_loc_operations ocfs2_xa_bucket_loc_ops = {
> .xlo_offset_pointer = ocfs2_xa_bucket_offset_pointer,
> - .xlo_has_space = ocfs2_xa_bucket_has_space,
> + .xlo_check_space = ocfs2_xa_bucket_check_space,
> .xlo_can_reuse = ocfs2_xa_bucket_can_reuse,
> .xlo_get_free_start = ocfs2_xa_bucket_get_free_start,
> .xlo_wipe_namevalue = ocfs2_xa_bucket_wipe_namevalue,
> @@ -1916,7 +1914,7 @@ static int ocfs2_xa_prepare_entry(struct ocfs2_xa_loc *loc,
> goto out;
> }
>
> - rc = ocfs2_xa_has_space(loc, xi);
> + rc = ocfs2_xa_check_space(loc, xi);
> if (rc)
> goto out;
>
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 10/14] ocfs2: Allocation in ocfs2_xa_prepare_entry() values in ocfs2_xa_store_value()
2009-09-01 8:55 ` Tao Ma
@ 2009-09-01 20:21 ` Joel Becker
2009-09-02 1:51 ` Joel Becker
0 siblings, 1 reply; 45+ messages in thread
From: Joel Becker @ 2009-09-01 20:21 UTC (permalink / raw)
To: ocfs2-devel
On Tue, Sep 01, 2009 at 04:55:04PM +0800, Tao Ma wrote:
> >+ } else if (le64_to_cpu(loc->xl_entry->xe_value_size) >
> >+ xi->xi_value_len) {
> >+ rc = ocfs2_xa_value_truncate(loc, xi->xi_value_len,
> >+ ctxt);
> From your comments above, we don't allocate space. I am just curious
> why we can't extend it here if value_size < xi->xi_value_len?
Because we extend in ocfs2_xa_prepare_entry(). No point in
doing it two places. The flow is cleaner here, and if we need to change
the extend code, we only change it once.
> > static int ocfs2_xa_prepare_entry(struct ocfs2_xa_loc *loc,
> > struct ocfs2_xattr_info *xi,
> >- u32 name_hash)
> >+ u32 name_hash,
> >+ struct ocfs2_xattr_set_ctxt *ctxt)
> > {
> > int rc = 0;
> >- int name_size = OCFS2_XATTR_SIZE(xi->xi_name_len);
> >- char *nameval_buf;
> > if (!xi->xi_value) {
> > ocfs2_xa_remove_entry(loc);
> >@@ -2044,13 +1990,10 @@ static int ocfs2_xa_prepare_entry(struct ocfs2_xa_loc *loc,
> > if (loc->xl_entry) {
> > if (ocfs2_xa_can_reuse_entry(loc, xi)) {
> >- nameval_buf = ocfs2_xa_offset_pointer(loc,
> >- le16_to_cpu(loc->xl_entry->xe_name_offset));
> >- memset(nameval_buf + name_size, 0,
> >- namevalue_size_xe(loc->xl_entry) - name_size);
> >- loc->xl_entry->xe_value_size =
> >- cpu_to_le64(xi->xi_value_len);
> >- goto out;
> >+ rc = ocfs2_xa_reuse_entry(loc, xi, ctxt);
> >+ if (rc)
> >+ goto out;
> >+ goto alloc_value;
> As I said above in xa_reuse_entry, if we have already "alloc value"
> in ocfs2_xa_reuse_entry, we don't need to goto it again?
Because this way if we have to handle allocation errors, we only
handle them in one place. Also flow of ocfs2_prepare_entry() is
straightforward. First we find an entry, then we allocate any new
storage for it. One of the spaghetti problems of the previous code was
the seven different ways to end up allocating the storage ;-)
> >@@ -2336,28 +2195,6 @@ static int ocfs2_xattr_set_entry(struct inode *inode,
> > if (ret < 0)
> > mlog_errno(ret);
> >- if (!ret && xi->xi_value_len > OCFS2_XATTR_INLINE_SIZE) {
> >- /*
> >- * Set value outside in B tree.
> >- * This is the second step for value size > INLINE_SIZE.
> >- */
> >- size_t offs = le16_to_cpu(xs->here->xe_name_offset);
> >- ret = ocfs2_xattr_set_value_outside(inode, xi, xs, ctxt,
> >- &vb, offs);
> >- if (ret < 0) {
> >- int ret2;
> >-
> >- mlog_errno(ret);
> >- /*
> >- * If set value outside failed, we have to clean
> >- * the junk tree root we have already set in local.
> >- */
> >- ret2 = ocfs2_xattr_cleanup(inode, ctxt->handle,
> >- xi, xs, &vb, offs);
> you just removed this part which will remove the entry if we fail in
> cluster extension. I haven't seen some new implementation like that.
> Am I missing something here?
No, you're not. ocfs2_xattr_cleanup() was broken anyway - it
never freed the storage, just zeroed out the value root. Plus, it only
worked for entries at the end of the entry list. Now, the old code was
safe in that it would only call ocfs2_xattr_cleanup() for an entry at
the end of the entry list. But the old code never cleaned up a reused
entry. So if it was reusing an entry and had a problem truncating to a
smaller size or growing to a larger size, it would leave the entry in
place.
So my current code has the latter behavior. You get
half-initialized xattrs in the case of error. But it's consistent - you
get that for new entries and reused ones!
This leads to the real question: what do we do when we have a
problem? There are multiple problems to have.
1) We have trouble allocating space for a new xattr. This leaves us
with an empty xattr.
2) We overwrote an existing local xattr with a value root, and now we
have an error allocating the storage. This leaves us an empty xattr.
where there used to be a value. The value is lost.
3) We have trouble truncating a reused value. This leaves us with the
original entry pointing to the truncated original value. The value
is lost.
4) We have trouble extending the storage on a reused value. This leaves
us with the original value safely in place, but with more storage
allocated when needed.
For the sake of argument, I'm going to ignore problems storing
local xattrs, as that can only really be journal aborts, and those are
horrible anyway :-)
Condition (1) is where the original ocfs2_xattr_cleanup() wiped
the entry and leaked the storage. We can certainly try to wipe the
entry and cleanup the storage. Case (4) can either be ignored or we can
re-truncate back to the original size. Cases (2) and (3) are the hard
ones. The values are already lost. Do we just leave the partial values
in place? That sucks. Do we remove the entries? Well that means a
failed set is just like a delete. Ugh!
Anyone have any ideas?
Joel
--
"Sometimes one pays most for the things one gets for nothing."
- Albert Einstein
Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 10/14] ocfs2: Allocation in ocfs2_xa_prepare_entry() values in ocfs2_xa_store_value()
2009-09-01 20:21 ` Joel Becker
@ 2009-09-02 1:51 ` Joel Becker
2009-09-02 4:59 ` Tao Ma
0 siblings, 1 reply; 45+ messages in thread
From: Joel Becker @ 2009-09-02 1:51 UTC (permalink / raw)
To: ocfs2-devel
On Tue, Sep 01, 2009 at 01:21:12PM -0700, Joel Becker wrote:
> This leads to the real question: what do we do when we have a
> problem? There are multiple problems to have.
Ok, here's a full error handling patch. It's 15th in the
series. It sits atop a couple of changes I've ma to support it
(separating ocfs2_xa_remove_entry() from the truncate and having
ocfs2_xa_remove() that does truncate+wipe). The latest xa_loc in
ocfs2.git has all this.
From b6be52624330a9fa735e31a9ea867988d24c103e Mon Sep 17 00:00:00 2001
From: Joel Becker <joel.becker@oracle.com>
Date: Tue, 1 Sep 2009 18:38:27 -0700
Subject: [PATCH] ocfs2: Handle errors while setting external xattr values.
ocfs2 can store extended attribute values as large as a single file. It
does this using a standard ocfs2 btree for the large value. However,
the previous code did not handle all error cases cleanly.
There are multiple problems to have.
1) We have trouble allocating space for a new xattr. This leaves us
with an empty xattr.
2) We overwrote an existing local xattr with a value root, and now we
have an error allocating the storage. This leaves us an empty xattr.
where there used to be a value. The value is lost.
3) We have trouble truncating a reused value. This leaves us with the
original entry pointing to the truncated original value. The value
is lost.
4) We have trouble extending the storage on a reused value. This leaves
us with the original value safely in place, but with more storage
allocated when needed.
This doesn't consider storing local xattrs (values that don't require a
btree). Those only fail when the journal fails.
Case (1) is easy. We just remove the xattr we added. We leak the
storage because we can't safely remove it, but otherwise everything is
happy. We'll print a warning about the leak.
Case (4) is easy. We still have the original value in place. We can
just leave the extra storage attached to this xattr. We return the
error, but the old value is untouched. We print a warning about the
storage.
Case (2) and (3) are hard because we've lost the original values. In
the old code, we ended up with values that could be partially read.
That's not good. Instead, we just wipe the xattr entry and leak the
storage. It stinks that the original value is lost, but now there isn't
a partial value to be read. We'll print a big fat warning.
Signed-off-by: Joel Becker <joel.becker@oracle.com>
---
fs/ocfs2/xattr.c | 141 +++++++++++++++++++++++++++++++++++++++++++++++------
1 files changed, 125 insertions(+), 16 deletions(-)
diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
index f62985a..20323bb 100644
--- a/fs/ocfs2/xattr.c
+++ b/fs/ocfs2/xattr.c
@@ -1861,6 +1861,17 @@ static const struct ocfs2_xa_loc_operations ocfs2_xa_bucket_loc_ops = {
.xlo_fill_value_buf = ocfs2_xa_bucket_fill_value_buf,
};
+static unsigned int ocfs2_xa_value_clusters(struct ocfs2_xa_loc *loc)
+{
+ struct ocfs2_xattr_value_buf vb;
+
+ if (ocfs2_xattr_is_local(loc->xl_entry))
+ return 0;
+
+ ocfs2_xa_fill_value_buf(loc, &vb);
+ return le32_to_cpu(vb.vb_xv->xr_clusters);
+}
+
static int ocfs2_xa_value_truncate(struct ocfs2_xa_loc *loc, u64 bytes,
struct ocfs2_xattr_set_ctxt *ctxt)
{
@@ -1897,17 +1908,87 @@ static void ocfs2_xa_remove_entry(struct ocfs2_xa_loc *loc)
}
}
+/*
+ * If we have a problem adjusting the size of an external value during
+ * ocfs2_xa_prepare_entry() or ocfs2_xa_remove(), we may have an xattr
+ * in an intermediate state. For example, the value may be partially
+ * truncated.
+ *
+ * If the value tree hasn't changed, the extend/truncate went nowhere.
+ * We have nothing to do. The caller can treat it as a straight error.
+ *
+ * If the value tree got partially truncated, we now have a corrupted
+ * extended attribute. We're going to wipe its entry and leak the
+ * clusters. Better to leak some storage than leave a corrupt entry.
+ *
+ * If the value tree grew, it obviously didn't grow enough for the
+ * new entry. We're not going to try and reclaim those clusters either.
+ * If there was already an external value there (orig_clusters != 0),
+ * the new clusters are attached safely and we can just leave the old
+ * value in place. If there was no external value there, we remove
+ * then entry.
+ *
+ * This way, the xattr block we store in the journal will be consistent.
+ * If the size change broke because of the journal, no changes will hit
+ * disk anyway.
+ */
+static void ocfs2_xa_cleanup_value_truncate(struct ocfs2_xa_loc *loc,
+ const char *what,
+ unsigned int orig_clusters)
+{
+ unsigned int new_clusters = ocfs2_xa_value_clusters(loc);
+ char *nameval_buf = ocfs2_xa_offset_pointer(loc,
+ le16_to_cpu(loc->xl_entry->xe_name_offset));
+
+ if (new_clusters < orig_clusters) {
+ mlog(ML_ERROR,
+ "Partial truncate while %s xattr %.*s. Leaking "
+ "%u clusters and removing the entry\n",
+ what, loc->xl_entry->xe_name_len, nameval_buf,
+ orig_clusters - new_clusters);
+ ocfs2_xa_remove_entry(loc);
+ } else if (!orig_clusters) {
+ mlog(ML_ERROR,
+ "Unable to allocate an external value for xattr "
+ "%.*s safely. Leaking %u clusters and removing the "
+ "entry\n",
+ loc->xl_entry->xe_name_len, nameval_buf,
+ new_clusters - orig_clusters);
+ ocfs2_xa_remove_entry(loc);
+ } else if (new_clusters > orig_clusters)
+ mlog(ML_ERROR,
+ "Unable to grow xattr %.*s safely. %u new clusters "
+ "have been added, but the value will not be "
+ "modified\n",
+ loc->xl_entry->xe_name_len, nameval_buf,
+ new_clusters - orig_clusters);
+}
+
static int ocfs2_xa_remove(struct ocfs2_xa_loc *loc,
struct ocfs2_xattr_set_ctxt *ctxt)
{
int rc;
+ unsigned int orig_clusters;
if (!ocfs2_xattr_is_local(loc->xl_entry)) {
+ orig_clusters = ocfs2_xa_value_clusters(loc);
rc = ocfs2_xa_value_truncate(loc, 0, ctxt);
if (rc) {
mlog_errno(rc);
- goto out;
+ /*
+ * Since this is remove, we can return 0 if
+ * ocfs2_xa_cleanup_value_truncate() is going to
+ * wipe the entry anyway. So we check the
+ * cluster count as well.
+ */
+ if (orig_clusters != ocfs2_xa_value_clusters(loc))
+ rc = 0;
+ ocfs2_xa_cleanup_value_truncate(loc, "removing",
+ orig_clusters);
}
+
+ if (rc)
+ goto out;
}
ocfs2_xa_remove_entry(loc);
@@ -1937,6 +2018,7 @@ static int ocfs2_xa_reuse_entry(struct ocfs2_xa_loc *loc,
{
int rc = 0;
int name_size = OCFS2_XATTR_SIZE(xi->xi_name_len);
+ unsigned int orig_clusters;
char *nameval_buf;
int xe_local = ocfs2_xattr_is_local(loc->xl_entry);
int xi_local = xi->xi_value_len <= OCFS2_XATTR_INLINE_SIZE;
@@ -1952,23 +2034,27 @@ static int ocfs2_xa_reuse_entry(struct ocfs2_xa_loc *loc,
if (!xi_local)
ocfs2_xa_install_value_root(loc);
} else {
+ orig_clusters = ocfs2_xa_value_clusters(loc);
if (xi_local) {
rc = ocfs2_xa_value_truncate(loc, 0, ctxt);
- if (rc < 0) {
+ if (rc < 0)
mlog_errno(rc);
- goto out;
- }
- memset(nameval_buf + name_size, 0,
- namevalue_size_xe(loc->xl_entry) -
- name_size);
+ else
+ memset(nameval_buf + name_size, 0,
+ namevalue_size_xe(loc->xl_entry) -
+ name_size);
} else if (le64_to_cpu(loc->xl_entry->xe_value_size) >
xi->xi_value_len) {
rc = ocfs2_xa_value_truncate(loc, xi->xi_value_len,
ctxt);
- if (rc < 0) {
+ if (rc < 0)
mlog_errno(rc);
- goto out;
- }
+ }
+
+ if (rc) {
+ ocfs2_xa_cleanup_value_truncate(loc, "reusing",
+ orig_clusters);
+ goto out;
}
}
@@ -1993,6 +2079,8 @@ static int ocfs2_xa_prepare_entry(struct ocfs2_xa_loc *loc,
struct ocfs2_xattr_set_ctxt *ctxt)
{
int rc = 0;
+ unsigned int orig_clusters;
+ __le64 orig_value_size = 0;
rc = ocfs2_xa_check_space(loc, xi);
if (rc)
@@ -2000,6 +2088,7 @@ static int ocfs2_xa_prepare_entry(struct ocfs2_xa_loc *loc,
if (loc->xl_entry) {
if (ocfs2_xa_can_reuse_entry(loc, xi)) {
+ orig_value_size = loc->xl_entry->xe_value_size;
rc = ocfs2_xa_reuse_entry(loc, xi, ctxt);
if (rc)
goto out;
@@ -2007,9 +2096,13 @@ static int ocfs2_xa_prepare_entry(struct ocfs2_xa_loc *loc,
}
if (!ocfs2_xattr_is_local(loc->xl_entry)) {
+ orig_clusters = ocfs2_xa_value_clusters(loc);
rc = ocfs2_xa_value_truncate(loc, 0, ctxt);
if (rc) {
mlog_errno(rc);
+ ocfs2_xa_cleanup_value_truncate(loc,
+ "overwriting",
+ orig_clusters);
goto out;
}
}
@@ -2027,9 +2120,20 @@ static int ocfs2_xa_prepare_entry(struct ocfs2_xa_loc *loc,
alloc_value:
if (xi->xi_value_len > OCFS2_XATTR_INLINE_SIZE) {
+ orig_clusters = ocfs2_xa_value_clusters(loc);
rc = ocfs2_xa_value_truncate(loc, xi->xi_value_len, ctxt);
- if (rc < 0)
+ if (rc < 0) {
+ /*
+ * If we tried to grow an existing external value,
+ * ocfs2_xa_cleanuP-value_truncate() is going to
+ * let it stand. We have to restore its original
+ * value size.
+ */
+ loc->xl_entry->xe_value_size = orig_value_size;
+ ocfs2_xa_cleanup_value_truncate(loc, "growing",
+ orig_clusters);
mlog_errno(rc);
+ }
}
out:
@@ -2079,25 +2183,30 @@ static int ocfs2_xa_set(struct ocfs2_xa_loc *loc,
goto out;
}
+ /*
+ * From here on out, everything is going to modify the buffer a
+ * little. Errors are going to leave the xattr header in a
+ * sane state. Thus, even with errors we dirty the sucker.
+ */
+
/* Don't worry, we are never called with !xi_value and !xl_entry */
if (!xi->xi_value) {
ret = ocfs2_xa_remove(loc, ctxt);
- goto out;
+ goto out_dirty;
}
ret = ocfs2_xa_prepare_entry(loc, xi, name_hash, ctxt);
if (ret) {
if (ret != -ENOSPC)
mlog_errno(ret);
- goto out;
+ goto out_dirty;
}
ret = ocfs2_xa_store_value(loc, xi, ctxt);
- if (ret) {
+ if (ret)
mlog_errno(ret);
- goto out;
- }
+out_dirty:
ocfs2_xa_journal_dirty(ctxt->handle, loc);
out:
--
1.6.3.3
--
"Sometimes I think the surest sign intelligent
life exists elsewhere in the universe is that
none of it has tried to contact us."
-Calvin & Hobbes
Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127
^ permalink raw reply related [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 12/14] ocfs2: Let ocfs2_xa_prepare_entry() do space checks.
2009-08-28 8:36 ` [Ocfs2-devel] [PATCH 12/14] ocfs2: Let ocfs2_xa_prepare_entry() do space checks Joel Becker
@ 2009-09-02 1:54 ` Tao Ma
2009-09-02 2:11 ` Joel Becker
0 siblings, 1 reply; 45+ messages in thread
From: Tao Ma @ 2009-09-02 1:54 UTC (permalink / raw)
To: ocfs2-devel
Hi Joel,
Joel Becker wrote:
> ocfs2_xattr_set_in_bucket() doesn't need to do its own hacky space
> checking. Let's let ocfs2_xa_prepare_entry() (via ocfs2_xa_set()) do
> the more accurate work. Whenever it doesn't have space,
> ocfs2_xattr_set_in_bucket() can try to get more space.
>
> Signed-off-by: Joel Becker <joel.becker@oracle.com>
> ---
> fs/ocfs2/xattr.c | 239 ++++++++++++++++-------------------------------------
> 1 files changed, 72 insertions(+), 167 deletions(-)
>
> diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
> index b4cda7e..e400d64 100644
> --- a/fs/ocfs2/xattr.c
> +++ b/fs/ocfs2/xattr.c
<snip>
> - free = xh_free_start - header_size - OCFS2_XATTR_HEADER_GAP;
> + /* Ack, need more space. Let's try to get another bucket! */
> +
> /*
> - * We need to make sure the new name/value pair
> - * can exist in the same block.
> + * We do not allow for overlapping ranges between buckets. And
> + * the maximum number of collisions we will allow for then is
> + * one bucket's worth, so check it here whether we need to
> + * add a new bucket for the insert.
> */
> - if (xh_free_start % blocksize < need)
> - free -= xh_free_start % blocksize;
> -
> - mlog(0, "xs->not_found = %d, in xattr bucket %llu: free = %d, "
> - "need = %d, max_free = %d, xh_free_start = %u, xh_name_value_len ="
> - " %u\n", xs->not_found,
> - (unsigned long long)bucket_blkno(xs->bucket),
> - free, need, max_free, le16_to_cpu(xh->xh_free_start),
> - le16_to_cpu(xh->xh_name_value_len));
> -
> - if (free < need ||
> - (xs->not_found &&
> - count == ocfs2_xattr_max_xe_in_bucket(inode->i_sb))) {
> - if (need <= max_free &&
> - count < ocfs2_xattr_max_xe_in_bucket(inode->i_sb)) {
I just have one concern. Here we used to check whether count ==
ocfs2_xattr_max_xe_in_bucket(inode->i_sb) which make sure
ocfs2_xattr_entry to be stored only in the first block of a bucket(this
is a legacy issue before you added ocfs2_xattr_bucket abstraction), so
now you remove this restriction. Maybe it is ok since now we already
read the whole bucket and it looks that your current xa_* abstraction
can handle it. Just want to remind you of it.
Regards,
Tao
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 12/14] ocfs2: Let ocfs2_xa_prepare_entry() do space checks.
2009-09-02 1:54 ` Tao Ma
@ 2009-09-02 2:11 ` Joel Becker
2009-09-02 2:21 ` Tao Ma
0 siblings, 1 reply; 45+ messages in thread
From: Joel Becker @ 2009-09-02 2:11 UTC (permalink / raw)
To: ocfs2-devel
On Wed, Sep 02, 2009 at 09:54:31AM +0800, Tao Ma wrote:
> I just have one concern. Here we used to check whether count ==
> ocfs2_xattr_max_xe_in_bucket(inode->i_sb) which make sure
> ocfs2_xattr_entry to be stored only in the first block of a
> bucket(this is a legacy issue before you added ocfs2_xattr_bucket
> abstraction), so now you remove this restriction. Maybe it is ok
> since now we already read the whole bucket and it looks that your
> current xa_* abstraction can handle it. Just want to remind you of
> it.
Oh, right. I knew about that, but clearly forgot while I was
doing this particular patch. Intellectually I'd like to remove the
restriction, as that makes our code more general and flexible. But all
code prior to this, even if it read the whole bucket, assumed that the
entries were in a linear buffer. Before this xa_loc abstraction, that's
false. We'd have older kernels (2.6.30, 2.6.31) crashing on anything
with this problem. Of course, how many people have that many xattrs?
Maybe we can just figure that by they time they have that many, they
have 2.6.32? :-)
Joel
--
"If you took all of the grains of sand in the world, and lined
them up end to end in a row, you'd be working for the government!"
- Mr. Interesting
Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 12/14] ocfs2: Let ocfs2_xa_prepare_entry() do space checks.
2009-09-02 2:11 ` Joel Becker
@ 2009-09-02 2:21 ` Tao Ma
0 siblings, 0 replies; 45+ messages in thread
From: Tao Ma @ 2009-09-02 2:21 UTC (permalink / raw)
To: ocfs2-devel
Joel Becker wrote:
> On Wed, Sep 02, 2009 at 09:54:31AM +0800, Tao Ma wrote:
>> I just have one concern. Here we used to check whether count ==
>> ocfs2_xattr_max_xe_in_bucket(inode->i_sb) which make sure
>> ocfs2_xattr_entry to be stored only in the first block of a
>> bucket(this is a legacy issue before you added ocfs2_xattr_bucket
>> abstraction), so now you remove this restriction. Maybe it is ok
>> since now we already read the whole bucket and it looks that your
>> current xa_* abstraction can handle it. Just want to remind you of
>> it.
>
> Oh, right. I knew about that, but clearly forgot while I was
> doing this particular patch. Intellectually I'd like to remove the
> restriction, as that makes our code more general and flexible. But all
> code prior to this, even if it read the whole bucket, assumed that the
> entries were in a linear buffer. Before this xa_loc abstraction, that's
> false. We'd have older kernels (2.6.30, 2.6.31) crashing on anything
> with this problem. Of course, how many people have that many xattrs?
> Maybe we can just figure that by they time they have that many, they
> have 2.6.32? :-)
yeah, I'd like to remove it also after I got from you that reading a 4K
bucket is as fast as or even faster than we read the 1st and 3rd block
individually. So let us do it.
btw, please remove ocfs2_xattr_max_xe_in_bucket since now there are no
caller for it.
Regards,
Tao
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 13/14] ocfs2: Set xattr block entries with ocfs2_xa_set()
2009-08-28 8:36 ` [Ocfs2-devel] [PATCH 13/14] ocfs2: Set xattr block entries with ocfs2_xa_set() Joel Becker
@ 2009-09-02 2:50 ` Tao Ma
0 siblings, 0 replies; 45+ messages in thread
From: Tao Ma @ 2009-09-02 2:50 UTC (permalink / raw)
To: ocfs2-devel
Joel Becker wrote:
> ocfs2_xattr_block_set() calls into ocfs2_xattr_set_entry() with just the
> HAS_XATTR flag. Most of the machinery of ocfs2_xattr_set_entry() is
> skipped. All that really happens other than the call to ocfs2_xa_set()
> is making sure the HAS_XATTR flag is set on the inode.
>
> But HAS_XATTR should be set when we also set di->i_xattr_loc. And
> that's done in ocfs2_xattr_block_set(). So let's move it there, and
it is done in ocfs2_create_xattr_block actually. ;)
Otherwise it looks good.
Regards,
Tao
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 14/14] ocfs2: Set inline xattr entries with ocfs2_xa_set()
2009-08-28 8:36 ` [Ocfs2-devel] [PATCH 14/14] ocfs2: Set inline xattr " Joel Becker
@ 2009-09-02 2:58 ` Tao Ma
0 siblings, 0 replies; 45+ messages in thread
From: Tao Ma @ 2009-09-02 2:58 UTC (permalink / raw)
To: ocfs2-devel
Joel Becker wrote:
> ocfs2_xattr_ibody_set() is the only remaining user of
> ocfs2_xattr_set_entry(). ocfs2_xattr_set_entry() actually does two
> things: it calls ocfs2_xa_set(), and it initializes the inline xattrs.
> Initializing the inline space really belongs in its own call.
>
> We lift the initialization to ocfs2_xattr_ibody_init(), called from
> ocfs2_xattr_ibody_set() only when necessary. Now
> ocfs2_xattr_ibody_set() can call ocfs2_xa_set() directly.
> ocfs2_xattr_set_entry() goes away.
>
> Another nice fact is that ocfs2_init_dinode_xa_loc() can trust
> i_xattr_inline_size.
>
> Signed-off-by: Joel Becker <joel.becker@oracle.com>
> ---
> fs/ocfs2/xattr.c | 156 +++++++++++++++++++++++++-----------------------------
> 1 files changed, 72 insertions(+), 84 deletions(-)
>
> diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
> index 862c56d..d485932 100644
> --- a/fs/ocfs2/xattr.c
> +++ b/fs/ocfs2/xattr.c
> @@ -2088,17 +2088,13 @@ static void ocfs2_init_dinode_xa_loc(struct ocfs2_xa_loc *loc,
> {
> struct ocfs2_dinode *di = (struct ocfs2_dinode *)bh->b_data;
> @@ -2615,9 +2586,10 @@ static int ocfs2_xattr_ibody_set(struct inode *inode,
> struct ocfs2_xattr_search *xs,
> struct ocfs2_xattr_set_ctxt *ctxt)
> {
> + int ret;
> struct ocfs2_inode_info *oi = OCFS2_I(inode);
> struct ocfs2_dinode *di = (struct ocfs2_dinode *)xs->inode_bh->b_data;
> - int ret;
> + struct ocfs2_xa_loc loc;
>
> if (inode->i_sb->s_blocksize == OCFS2_MIN_BLOCKSIZE)
> return -ENOSPC;
> @@ -2630,8 +2602,24 @@ static int ocfs2_xattr_ibody_set(struct inode *inode,
> }
> }
>
> - ret = ocfs2_xattr_set_entry(inode, xi, xs, ctxt,
> - (OCFS2_INLINE_XATTR_FL | OCFS2_HAS_XATTR_FL));
> + if (!(oi->ip_dyn_features & OCFS2_INLINE_XATTR_FL)) {
> + ret = ocfs2_xattr_ibody_init(inode, xs->inode_bh, ctxt);
> + if (ret) {
> + mlog_errno(ret);
if ret == -ENOSPC, we don't need to print error message.
> + goto out;
> + }
> + }
Regards,
Tao
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 10/14] ocfs2: Allocation in ocfs2_xa_prepare_entry() values in ocfs2_xa_store_value()
2009-09-02 1:51 ` Joel Becker
@ 2009-09-02 4:59 ` Tao Ma
2009-09-02 8:05 ` Joel Becker
2009-09-02 8:37 ` Joel Becker
0 siblings, 2 replies; 45+ messages in thread
From: Tao Ma @ 2009-09-02 4:59 UTC (permalink / raw)
To: ocfs2-devel
Joel Becker wrote:
> On Tue, Sep 01, 2009 at 01:21:12PM -0700, Joel Becker wrote:
> There are multiple problems to have.
>
> 1) We have trouble allocating space for a new xattr. This leaves us
> with an empty xattr.
> 2) We overwrote an existing local xattr with a value root, and now we
> have an error allocating the storage. This leaves us an empty xattr.
> where there used to be a value. The value is lost.
> 3) We have trouble truncating a reused value. This leaves us with the
> original entry pointing to the truncated original value. The value
> is lost.
> 4) We have trouble extending the storage on a reused value. This leaves
> us with the original value safely in place, but with more storage
> allocated when needed.
>
> This doesn't consider storing local xattrs (values that don't require a
> btree). Those only fail when the journal fails.
>
> Case (1) is easy. We just remove the xattr we added. We leak the
> storage because we can't safely remove it, but otherwise everything is
> happy. We'll print a warning about the leak.
>
> Case (4) is easy. We still have the original value in place. We can
> just leave the extra storage attached to this xattr. We return the
> error, but the old value is untouched. We print a warning about the
> storage.
>
> Case (2) and (3) are hard because we've lost the original values. In
> the old code, we ended up with values that could be partially read.
> That's not good. Instead, we just wipe the xattr entry and leak the
> storage. It stinks that the original value is lost, but now there isn't
> a partial value to be read. We'll print a big fat warning.
actually case (2) is rarely to happen since we should have already
reserved enough clusters before we start the transaction. As for (3),
the only chance is that the b-tree is corrupted. And in this case, I
think remove the corrupted b-tree root is OK for me.
small comments for the patch.
>
> Signed-off-by: Joel Becker <joel.becker@oracle.com>
> ---
> fs/ocfs2/xattr.c | 141 +++++++++++++++++++++++++++++++++++++++++++++++------
> 1 files changed, 125 insertions(+), 16 deletions(-)
>
> diff --git a/fs/ocfs2/xattr.c b/fs/ocfs2/xattr.c
> index f62985a..20323bb 100644
> --- a/fs/ocfs2/xattr.c
> +++ b/fs/ocfs2/xattr.c
> @@ -1897,17 +1908,87 @@ static void ocfs2_xa_remove_entry(struct ocfs2_xa_loc *loc)
> }
> }
>
> + * If the value tree grew, it obviously didn't grow enough for the
> + * new entry. We're not going to try and reclaim those clusters either.
> + * If there was already an external value there (orig_clusters != 0),
> + * the new clusters are attached safely and we can just leave the old
> + * value in place. If there was no external value there, we remove
> + * then entry.
the entry.
> + *
> + * This way, the xattr block we store in the journal will be consistent.
> + * If the size change broke because of the journal, no changes will hit
> + * disk anyway.
> + */
> +static void ocfs2_xa_cleanup_value_truncate(struct ocfs2_xa_loc *loc,
> + const char *what,
> + unsigned int orig_clusters)
> +{
> + unsigned int new_clusters = ocfs2_xa_value_clusters(loc);
> + char *nameval_buf = ocfs2_xa_offset_pointer(loc,
> + le16_to_cpu(loc->xl_entry->xe_name_offset));
> +
> + if (new_clusters < orig_clusters) {
> + mlog(ML_ERROR,
> + "Partial truncate while %s xattr %.*s. Leaking "
> + "%u clusters and removing the entry\n",
> + what, loc->xl_entry->xe_name_len, nameval_buf,
> + orig_clusters - new_clusters);
> + ocfs2_xa_remove_entry(loc);
> + } else if (!orig_clusters) {
> + mlog(ML_ERROR,
> + "Unable to allocate an external value for xattr "
> + "%.*s safely. Leaking %u clusters and removing the "
> + "entry\n",
> + loc->xl_entry->xe_name_len, nameval_buf,
> + new_clusters - orig_clusters);
> + ocfs2_xa_remove_entry(loc);
> + } else if (new_clusters > orig_clusters)
> + mlog(ML_ERROR,
> + "Unable to grow xattr %.*s safely. %u new clusters "
> + "have been added, but the value will not be "
> + "modified\n",
> + loc->xl_entry->xe_name_len, nameval_buf,
> + new_clusters - orig_clusters);
> +}
> +
> static int ocfs2_xa_remove(struct ocfs2_xa_loc *loc,
> struct ocfs2_xattr_set_ctxt *ctxt)
> {
> int rc;
> + unsigned int orig_clusters;
>
> if (!ocfs2_xattr_is_local(loc->xl_entry)) {
> + orig_clusters = ocfs2_xa_value_clusters(loc);
> rc = ocfs2_xa_value_truncate(loc, 0, ctxt);
> if (rc) {
> mlog_errno(rc);
> - goto out;
> + /*
> + * Since this is remove, we can return 0 if
> + * ocfs2_xa_cleanup_value_truncate() is going to
> + * wipe the entry anyway. So we check the
> + * cluster count as well.
> + */
> + if (orig_clusters != ocfs2_xa_value_clusters(loc))
> + rc = 0;
> + ocfs2_xa_cleanup_value_truncate(loc, "removing",
> + orig_clusters);
> }
> +
> + if (rc)
> + goto out;
move this after ocfs2_xa_cleanup_value_truncate. No need to check it
twice. And even if you set rc = 0 above, in
ocfs2_xa_cleanup_value_truncate(new_clusters < orig_clusters) we will
remove the entry I think. So we don't need to call ocfs2_xa_remove_entry
below.
OK, I think I have finished reviewing the whole patch set. Thanks for
the work. You can add my ACK to this set now. And once it get merged
into merge-window, I can ask tristan to run it with his xattr test case.
Regards,
Tao
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 10/14] ocfs2: Allocation in ocfs2_xa_prepare_entry() values in ocfs2_xa_store_value()
2009-09-02 4:59 ` Tao Ma
@ 2009-09-02 8:05 ` Joel Becker
2009-09-02 8:37 ` Joel Becker
1 sibling, 0 replies; 45+ messages in thread
From: Joel Becker @ 2009-09-02 8:05 UTC (permalink / raw)
To: ocfs2-devel
On Wed, Sep 02, 2009 at 12:59:43PM +0800, Tao Ma wrote:
> >Case (2) and (3) are hard because we've lost the original values. In
> >the old code, we ended up with values that could be partially read.
> >That's not good. Instead, we just wipe the xattr entry and leak the
> >storage. It stinks that the original value is lost, but now there isn't
> >a partial value to be read. We'll print a big fat warning.
> actually case (2) is rarely to happen since we should have already
> reserved enough clusters before we start the transaction. As for
> (3), the only chance is that the b-tree is corrupted. And in this
> case, I think remove the corrupted b-tree root is OK for me.
Both case (2) and (3) are unlikely, because we've already tried
to reserve everything we need. Errors there should be EIO, btree
corruption, or journal abort.
> > static int ocfs2_xa_remove(struct ocfs2_xa_loc *loc,
> > struct ocfs2_xattr_set_ctxt *ctxt)
> > {
> > int rc;
> >+ unsigned int orig_clusters;
> > if (!ocfs2_xattr_is_local(loc->xl_entry)) {
> >+ orig_clusters = ocfs2_xa_value_clusters(loc);
> > rc = ocfs2_xa_value_truncate(loc, 0, ctxt);
> > if (rc) {
> > mlog_errno(rc);
> >- goto out;
> >+ /*
> >+ * Since this is remove, we can return 0 if
> >+ * ocfs2_xa_cleanup_value_truncate() is going to
> >+ * wipe the entry anyway. So we check the
> >+ * cluster count as well.
> >+ */
> >+ if (orig_clusters != ocfs2_xa_value_clusters(loc))
> >+ rc = 0;
> >+ ocfs2_xa_cleanup_value_truncate(loc, "removing",
> >+ orig_clusters);
> > }
> >+
> >+ if (rc)
> >+ goto out;
> move this after ocfs2_xa_cleanup_value_truncate. No need to check it
> twice. And even if you set rc = 0 above, in
> ocfs2_xa_cleanup_value_truncate(new_clusters < orig_clusters) we
> will remove the entry I think. So we don't need to call
> ocfs2_xa_remove_entry below.
Yeah, you're right, it can go inside the if block.
Joel
--
"This is the end, beautiful friend.
This is the end, my only friend the end
Of our elaborate plans, the end
Of everything that stands, the end
No safety or surprise, the end
I'll never look into your eyes again."
Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 10/14] ocfs2: Allocation in ocfs2_xa_prepare_entry() values in ocfs2_xa_store_value()
2009-09-02 4:59 ` Tao Ma
2009-09-02 8:05 ` Joel Becker
@ 2009-09-02 8:37 ` Joel Becker
2009-09-02 9:01 ` tristan.ye
2009-09-03 10:48 ` tristan.ye
1 sibling, 2 replies; 45+ messages in thread
From: Joel Becker @ 2009-09-02 8:37 UTC (permalink / raw)
To: ocfs2-devel
On Wed, Sep 02, 2009 at 12:59:43PM +0800, Tao Ma wrote:
> OK, I think I have finished reviewing the whole patch set. Thanks
> for the work. You can add my ACK to this set now. And once it get
> merged into merge-window, I can ask tristan to run it with his xattr
> test case.
All of the changes are in xa_loc. Can you have tristan against
that first? I don't want to push this into merge_window without at
least a quick pass to make sure there isn't a disasterous idea in there
:-)
Joel
--
"Gone to plant a weeping willow
On the bank's green edge it will roll, roll, roll.
Sing a lulaby beside the waters.
Lovers come and go, the river roll, roll, rolls."
Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 10/14] ocfs2: Allocation in ocfs2_xa_prepare_entry() values in ocfs2_xa_store_value()
2009-09-02 8:37 ` Joel Becker
@ 2009-09-02 9:01 ` tristan.ye
2009-09-02 10:24 ` Joel Becker
2009-09-03 10:48 ` tristan.ye
1 sibling, 1 reply; 45+ messages in thread
From: tristan.ye @ 2009-09-02 9:01 UTC (permalink / raw)
To: ocfs2-devel
On Wed, 2009-09-02 at 01:37 -0700, Joel Becker wrote:
> On Wed, Sep 02, 2009 at 12:59:43PM +0800, Tao Ma wrote:
> > OK, I think I have finished reviewing the whole patch set. Thanks
> > for the work. You can add my ACK to this set now. And once it get
> > merged into merge-window, I can ask tristan to run it with his xattr
> > test case.
>
> All of the changes are in xa_loc. Can you have tristan against
> that first? I don't want to push this into merge_window without at
> least a quick pass to make sure there isn't a disasterous idea in there
> :-)
Joel,
Ok, let me know the kernel branch where all your updated patches
applied, then I can run the tests for you guys.
Tao,
test4/5 nodes will be dedicated to do this.
Tristan.
>
> Joel
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 10/14] ocfs2: Allocation in ocfs2_xa_prepare_entry() values in ocfs2_xa_store_value()
2009-09-02 9:01 ` tristan.ye
@ 2009-09-02 10:24 ` Joel Becker
2009-09-02 10:31 ` Joel Becker
0 siblings, 1 reply; 45+ messages in thread
From: Joel Becker @ 2009-09-02 10:24 UTC (permalink / raw)
To: ocfs2-devel
On Wed, Sep 02, 2009 at 05:01:10PM +0800, tristan.ye wrote:
> On Wed, 2009-09-02 at 01:37 -0700, Joel Becker wrote:
> > On Wed, Sep 02, 2009 at 12:59:43PM +0800, Tao Ma wrote:
> > > OK, I think I have finished reviewing the whole patch set. Thanks
> > > for the work. You can add my ACK to this set now. And once it get
> > > merged into merge-window, I can ask tristan to run it with his xattr
> > > test case.
> >
> > All of the changes are in xa_loc. Can you have tristan against
> > that first? I don't want to push this into merge_window without at
> > least a quick pass to make sure there isn't a disasterous idea in there
> > :-)
>
> Joel,
>
> Ok, let me know the kernel branch where all your updated patches
> applied, then I can run the tests for you guys.
git://git.kernel.org/pub/scm/linux/kernel/git/jlbec/ocfs2.git xa_loc
Thanks!
Joel
--
"Copy from one, it's plagiarism; copy from two, it's research."
- Wilson Mizner
Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 10/14] ocfs2: Allocation in ocfs2_xa_prepare_entry() values in ocfs2_xa_store_value()
2009-09-02 10:24 ` Joel Becker
@ 2009-09-02 10:31 ` Joel Becker
0 siblings, 0 replies; 45+ messages in thread
From: Joel Becker @ 2009-09-02 10:31 UTC (permalink / raw)
To: ocfs2-devel
On Wed, Sep 02, 2009 at 03:24:13AM -0700, Joel Becker wrote:
> On Wed, Sep 02, 2009 at 05:01:10PM +0800, tristan.ye wrote:
> > On Wed, 2009-09-02 at 01:37 -0700, Joel Becker wrote:
> > > On Wed, Sep 02, 2009 at 12:59:43PM +0800, Tao Ma wrote:
> > > > OK, I think I have finished reviewing the whole patch set. Thanks
> > > > for the work. You can add my ACK to this set now. And once it get
> > > > merged into merge-window, I can ask tristan to run it with his xattr
> > > > test case.
> > >
> > > All of the changes are in xa_loc. Can you have tristan against
> > > that first? I don't want to push this into merge_window without at
> > > least a quick pass to make sure there isn't a disasterous idea in there
> > > :-)
> >
> > Joel,
> >
> > Ok, let me know the kernel branch where all your updated patches
> > applied, then I can run the tests for you guys.
>
> git://git.kernel.org/pub/scm/linux/kernel/git/jlbec/ocfs2.git xa_loc
Make sure the xa_loc you get is commit id
c221d964ba14e3e32c904b8769b6d5aeb23543bf. I just fixed a bug Tiger
found.
Joel
--
"But all my words come back to me
In shades of mediocrity.
Like emptiness in harmony
I need someone to comfort me."
Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 10/14] ocfs2: Allocation in ocfs2_xa_prepare_entry() values in ocfs2_xa_store_value()
2009-09-02 8:37 ` Joel Becker
2009-09-02 9:01 ` tristan.ye
@ 2009-09-03 10:48 ` tristan.ye
2009-09-05 1:35 ` Joel Becker
1 sibling, 1 reply; 45+ messages in thread
From: tristan.ye @ 2009-09-03 10:48 UTC (permalink / raw)
To: ocfs2-devel
On Wed, 2009-09-02 at 01:37 -0700, Joel Becker wrote:
> On Wed, Sep 02, 2009 at 12:59:43PM +0800, Tao Ma wrote:
> > OK, I think I have finished reviewing the whole patch set. Thanks
> > for the work. You can add my ACK to this set now. And once it get
> > merged into merge-window, I can ask tristan to run it with his xattr
> > test case.
>
> All of the changes are in xa_loc. Can you have tristan against
> that first? I don't want to push this into merge_window without at
> least a quick pass to make sure there isn't a disasterous idea in there
> :-)
Joel,
Your patches set against xattr on xa_loc branch made the life of
xattr-testing be suffering:-(
The tests failed at the very beginning with a lite workload... I also
tried to do some tests manually aiming to narrow down the problem,
following bugs can provide you detailed info when I hit error/panic. I'm
not sure whether they were in same kind or not, just filed them
separately with different symptoms.
http://oss.oracle.com/bugzilla/show_bug.cgi?id=1168
http://oss.oracle.com/bugzilla/show_bug.cgi?id=1169
http://oss.oracle.com/bugzilla/show_bug.cgi?id=1170
http://oss.oracle.com/bugzilla/show_bug.cgi?id=1171
Regards,
Tristan.
>
> Joel
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 10/14] ocfs2: Allocation in ocfs2_xa_prepare_entry() values in ocfs2_xa_store_value()
2009-09-03 10:48 ` tristan.ye
@ 2009-09-05 1:35 ` Joel Becker
2009-09-08 1:25 ` tristan.ye
0 siblings, 1 reply; 45+ messages in thread
From: Joel Becker @ 2009-09-05 1:35 UTC (permalink / raw)
To: ocfs2-devel
On Thu, Sep 03, 2009 at 06:48:41PM +0800, tristan.ye wrote:
> Your patches set against xattr on xa_loc branch made the life of
> xattr-testing be suffering:-(
>
> The tests failed at the very beginning with a lite workload... I also
> tried to do some tests manually aiming to narrow down the problem,
> following bugs can provide you detailed info when I hit error/panic. I'm
> not sure whether they were in same kind or not, just filed them
> separately with different symptoms.
It was all dumb mistakes by me. Thanks for the excellent tests,
they helped me narrow down each problem.
> http://oss.oracle.com/bugzilla/show_bug.cgi?id=1168
> http://oss.oracle.com/bugzilla/show_bug.cgi?id=1169
> http://oss.oracle.com/bugzilla/show_bug.cgi?id=1170
> http://oss.oracle.com/bugzilla/show_bug.cgi?id=1171
These all have patches now. The fixes are pushed to the xa_loc
branch, so if you regrab that, they should be fixed. Let me know what
else I broke :-)
Joel
--
"Vote early and vote often."
- Al Capone
Joel Becker
Principal Software Developer
Oracle
E-mail: joel.becker at oracle.com
Phone: (650) 506-8127
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 10/14] ocfs2: Allocation in ocfs2_xa_prepare_entry() values in ocfs2_xa_store_value()
2009-09-05 1:35 ` Joel Becker
@ 2009-09-08 1:25 ` tristan.ye
0 siblings, 0 replies; 45+ messages in thread
From: tristan.ye @ 2009-09-08 1:25 UTC (permalink / raw)
To: ocfs2-devel
On Fri, 2009-09-04 at 18:35 -0700, Joel Becker wrote:
> On Thu, Sep 03, 2009 at 06:48:41PM +0800, tristan.ye wrote:
> > Your patches set against xattr on xa_loc branch made the life of
> > xattr-testing be suffering:-(
> >
> > The tests failed at the very beginning with a lite workload... I also
> > tried to do some tests manually aiming to narrow down the problem,
> > following bugs can provide you detailed info when I hit error/panic. I'm
> > not sure whether they were in same kind or not, just filed them
> > separately with different symptoms.
>
> It was all dumb mistakes by me. Thanks for the excellent tests,
> they helped me narrow down each problem.
>
> > http://oss.oracle.com/bugzilla/show_bug.cgi?id=1168
> > http://oss.oracle.com/bugzilla/show_bug.cgi?id=1169
> > http://oss.oracle.com/bugzilla/show_bug.cgi?id=1170
> > http://oss.oracle.com/bugzilla/show_bug.cgi?id=1171
>
> These all have patches now. The fixes are pushed to the xa_loc
> branch, so if you regrab that, they should be fixed. Let me know what
> else I broke :-)
The above bugs got fixed applying your latest patches, but test still
gets blocked by following bug:
http://oss.oracle.com/bugzilla/show_bug.cgi?id=1172
Regards,
Tristan
>
> Joel
>
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 0/14] ocfs2: Unify the setting of extended attributes
2009-08-19 19:54 [Ocfs2-devel] [PATCH 0/14] ocfs2: Unify the setting of extended attributes Joel Becker
@ 2009-08-20 2:03 ` Tao Ma
0 siblings, 0 replies; 45+ messages in thread
From: Tao Ma @ 2009-08-20 2:03 UTC (permalink / raw)
To: ocfs2-devel
Hi Joel,
Joel Becker wrote:
> ocfs2 can store extended attributes in many ways. They can live inside
> the inode's inline data area, they can be stored in a single external
> block, or they can be in a "bucket" hanging off of a lookup tree. There
> are differences in how each storage type manages the attributes, and the
> current ocfs2 code reflects this.
>
> There are two entirely separate code paths for setting extended
> attributes. The first code path handles "block" storage - the inline
> inode or the external block. The second handles "bucket" storage. They
> do very similar things, but they go about them in very different ways.
> This makes reading and understanding the ocfs2 xattr code harder than it
> needs to be. Worse, updating the xattr code requires understanding and
> modifying both paths.
>
> This patch series unifies the xattr set code such that inodes, block,
> and buckets make the same call. It removes most of the redundant code,
> like the repeated implementations of truncating externally stored values.
Due to some historical reasons, the setting of xattrs has been divided
into 2 parts. I am so sorry for that and to be frank, I want to change
it for a very long time. So thanks for doing this.
>
> The core of the implementation is the ocfs2_xa_loc structure. This is
> an in-memory representation of an ocfs2_xattr_entry. It has operations
> that allow blocks and buckets to behave differently during the xattr
> modification process. Blocks and buckets are different enough that
> ocfs2_xa_loc_operations has ten different ops! But with these
> differences encapsulated, we can have a single code path to modify an
> entry.
>
> The changes don't reduce the size of the source code perceptibly (~10
> lines of actual code without comments or blanks), but to my mind they
> greatly improve the readability and maintainability.
>
> The current series is based on the 'fixes' branch; I'll need to rebase
> it atop cachme before it can go upstream. I wanted to send it out as it
> stands now so that the concept can be reviewed while I work on
> merge-window.
>
> Tao and Tiger, I'd really appreciate you going over the changes with a
> find-toothed comb. Make sure I have my ideas about block and bucket
> stuff correct, etc. Let me know if you think this is a stupid change,
> too :-)
the idea is cool and it is really helpful and I will review it carefully.
Regards,
Tao
^ permalink raw reply [flat|nested] 45+ messages in thread
* [Ocfs2-devel] [PATCH 0/14] ocfs2: Unify the setting of extended attributes
@ 2009-08-19 19:54 Joel Becker
2009-08-20 2:03 ` Tao Ma
0 siblings, 1 reply; 45+ messages in thread
From: Joel Becker @ 2009-08-19 19:54 UTC (permalink / raw)
To: ocfs2-devel
ocfs2 can store extended attributes in many ways. They can live inside
the inode's inline data area, they can be stored in a single external
block, or they can be in a "bucket" hanging off of a lookup tree. There
are differences in how each storage type manages the attributes, and the
current ocfs2 code reflects this.
There are two entirely separate code paths for setting extended
attributes. The first code path handles "block" storage - the inline
inode or the external block. The second handles "bucket" storage. They
do very similar things, but they go about them in very different ways.
This makes reading and understanding the ocfs2 xattr code harder than it
needs to be. Worse, updating the xattr code requires understanding and
modifying both paths.
This patch series unifies the xattr set code such that inodes, block,
and buckets make the same call. It removes most of the redundant code,
like the repeated implementations of truncating externally stored values.
The core of the implementation is the ocfs2_xa_loc structure. This is
an in-memory representation of an ocfs2_xattr_entry. It has operations
that allow blocks and buckets to behave differently during the xattr
modification process. Blocks and buckets are different enough that
ocfs2_xa_loc_operations has ten different ops! But with these
differences encapsulated, we can have a single code path to modify an
entry.
The changes don't reduce the size of the source code perceptibly (~10
lines of actual code without comments or blanks), but to my mind they
greatly improve the readability and maintainability.
The current series is based on the 'fixes' branch; I'll need to rebase
it atop cachme before it can go upstream. I wanted to send it out as it
stands now so that the concept can be reviewed while I work on
merge-window.
Tao and Tiger, I'd really appreciate you going over the changes with a
find-toothed comb. Make sure I have my ideas about block and bucket
stuff correct, etc. Let me know if you think this is a stupid change,
too :-)
Joel
[Pull]
git://git.kernel.org/pub/scm/linux/kernel/git/jlbec/ocfs2.git xa_loc
[View]
http://git.kernel.org/?p=linux/kernel/git/jlbec/ocfs2.git;a=shortlog;h=xa_loc
^ permalink raw reply [flat|nested] 45+ messages in thread
end of thread, other threads:[~2009-09-08 1:25 UTC | newest]
Thread overview: 45+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2009-08-28 8:35 [Ocfs2-devel] [PATCH 0/14] ocfs2: Unify the setting of extended attributes Joel Becker
2009-08-28 8:35 ` [Ocfs2-devel] [PATCH 01/14] ocfs2: Introduce ocfs2_xa_loc Joel Becker
2009-09-01 5:32 ` Tao Ma
2009-09-01 8:26 ` Joel Becker
2009-09-01 8:45 ` Joel Becker
2009-09-01 8:49 ` Tao Ma
2009-08-28 8:35 ` [Ocfs2-devel] [PATCH 02/14] ocfs2: Remove xattrs via ocfs2_xa_loc Joel Becker
2009-09-01 6:03 ` Tao Ma
2009-09-01 8:28 ` Joel Becker
2009-08-28 8:35 ` [Ocfs2-devel] [PATCH 03/14] ocfs2: Prefix the member fields of struct ocfs2_xattr_info Joel Becker
2009-08-28 8:35 ` [Ocfs2-devel] [PATCH 04/14] ocfs2: Add a name_len field to ocfs2_xattr_info Joel Becker
2009-08-28 8:35 ` [Ocfs2-devel] [PATCH 05/14] ocfs2: Wrap calculation of name+value pair size Joel Becker
2009-08-28 8:35 ` [Ocfs2-devel] [PATCH 06/14] ocfs2: Set the xattr name+value pair in one place Joel Becker
2009-09-01 7:33 ` Tao Ma
2009-09-01 8:30 ` Joel Becker
2009-09-01 8:47 ` Tao Ma
2009-09-01 9:30 ` Joel Becker
2009-09-01 12:12 ` Tao Ma
2009-08-28 8:35 ` [Ocfs2-devel] [PATCH 07/14] ocfs2: Handle value tree roots in ocfs2_xa_set_inline_value() Joel Becker
2009-08-28 8:35 ` [Ocfs2-devel] [PATCH 08/14] ocfs2: Provide ocfs2_xa_fill_value_buf() for external value processing Joel Becker
2009-08-28 8:35 ` [Ocfs2-devel] [PATCH 09/14] ocfs2: Teach ocfs2_xa_loc how to do its own journal work Joel Becker
2009-08-28 8:35 ` [Ocfs2-devel] [PATCH 10/14] ocfs2: Allocation in ocfs2_xa_prepare_entry() values in ocfs2_xa_store_value() Joel Becker
2009-09-01 8:55 ` Tao Ma
2009-09-01 20:21 ` Joel Becker
2009-09-02 1:51 ` Joel Becker
2009-09-02 4:59 ` Tao Ma
2009-09-02 8:05 ` Joel Becker
2009-09-02 8:37 ` Joel Becker
2009-09-02 9:01 ` tristan.ye
2009-09-02 10:24 ` Joel Becker
2009-09-02 10:31 ` Joel Becker
2009-09-03 10:48 ` tristan.ye
2009-09-05 1:35 ` Joel Becker
2009-09-08 1:25 ` tristan.ye
2009-08-28 8:36 ` [Ocfs2-devel] [PATCH 11/14] ocfs2: Gell into ocfs2_xa_set() Joel Becker
2009-08-28 8:36 ` [Ocfs2-devel] [PATCH 12/14] ocfs2: Let ocfs2_xa_prepare_entry() do space checks Joel Becker
2009-09-02 1:54 ` Tao Ma
2009-09-02 2:11 ` Joel Becker
2009-09-02 2:21 ` Tao Ma
2009-08-28 8:36 ` [Ocfs2-devel] [PATCH 13/14] ocfs2: Set xattr block entries with ocfs2_xa_set() Joel Becker
2009-09-02 2:50 ` Tao Ma
2009-08-28 8:36 ` [Ocfs2-devel] [PATCH 14/14] ocfs2: Set inline xattr " Joel Becker
2009-09-02 2:58 ` Tao Ma
-- strict thread matches above, loose matches on Subject: below --
2009-08-19 19:54 [Ocfs2-devel] [PATCH 0/14] ocfs2: Unify the setting of extended attributes Joel Becker
2009-08-20 2:03 ` Tao Ma
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.