2017-07-07 12:12:52

by Tahsin Erdogan

[permalink] [raw]
Subject: [PATCH 1/4] e2fsck: update quota inode accounting for ea_inode feature

Extended attribute inodes are charged to all referencing inodes.
Update e2fsck so that it can correctly track inode quota charges.

Signed-off-by: Tahsin Erdogan <[email protected]>
---
e2fsck/e2fsck.c | 10 +++--
e2fsck/e2fsck.h | 5 ++-
e2fsck/pass1.c | 134 ++++++++++++++++++++++++++++++++++++++------------------
3 files changed, 101 insertions(+), 48 deletions(-)

diff --git a/e2fsck/e2fsck.c b/e2fsck/e2fsck.c
index 63a986b92af9..88088a2578bb 100644
--- a/e2fsck/e2fsck.c
+++ b/e2fsck/e2fsck.c
@@ -98,9 +98,13 @@ errcode_t e2fsck_reset_context(e2fsck_t ctx)
ea_refcount_free(ctx->refcount_extra);
ctx->refcount_extra = 0;
}
- if (ctx->ea_block_quota) {
- ea_refcount_free(ctx->ea_block_quota);
- ctx->ea_block_quota = 0;
+ if (ctx->ea_block_quota_blocks) {
+ ea_refcount_free(ctx->ea_block_quota_blocks);
+ ctx->ea_block_quota_blocks = 0;
+ }
+ if (ctx->ea_block_quota_inodes) {
+ ea_refcount_free(ctx->ea_block_quota_inodes);
+ ctx->ea_block_quota_inodes = 0;
}
if (ctx->ea_inode_refs) {
ea_refcount_free(ctx->ea_inode_refs);
diff --git a/e2fsck/e2fsck.h b/e2fsck/e2fsck.h
index b25c5eb9179b..ca414ccb662a 100644
--- a/e2fsck/e2fsck.h
+++ b/e2fsck/e2fsck.h
@@ -269,9 +269,10 @@ struct e2fsck_struct {
ext2_refcount_t refcount_extra;

/*
- * Quota blocks to be charged for each ea block.
+ * Quota blocks and inodes to be charged for each ea block.
*/
- ext2_refcount_t ea_block_quota;
+ ext2_refcount_t ea_block_quota_blocks;
+ ext2_refcount_t ea_block_quota_inodes;

/*
* ea_inode references from attr entries.
diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
index 6deaab20e6f3..690d1dc79e98 100644
--- a/e2fsck/pass1.c
+++ b/e2fsck/pass1.c
@@ -59,6 +59,11 @@

#undef DEBUG

+struct ea_quota {
+ blk64_t blocks;
+ __u64 inodes;
+};
+
static int process_block(ext2_filsys fs, blk64_t *blocknr,
e2_blkcnt_t blockcnt, blk64_t ref_blk,
int ref_offset, void *priv_data);
@@ -66,7 +71,8 @@ static int process_bad_block(ext2_filsys fs, blk64_t *block_nr,
e2_blkcnt_t blockcnt, blk64_t ref_blk,
int ref_offset, void *priv_data);
static void check_blocks(e2fsck_t ctx, struct problem_context *pctx,
- char *block_buf, blk64_t ea_ibody_quota_blocks);
+ char *block_buf,
+ const struct ea_quota *ea_ibody_quota);
static void mark_table_blocks(e2fsck_t ctx);
static void alloc_bb_map(e2fsck_t ctx);
static void alloc_imagic_map(e2fsck_t ctx);
@@ -103,7 +109,7 @@ struct process_block_struct {

struct process_inode_block {
ext2_ino_t ino;
- blk64_t ea_ibody_quota_blocks;
+ struct ea_quota ea_ibody_quota;
struct ext2_inode_large inode;
};

@@ -411,7 +417,7 @@ static void inc_ea_inode_refs(e2fsck_t ctx, struct problem_context *pctx,
}

static void check_ea_in_inode(e2fsck_t ctx, struct problem_context *pctx,
- blk64_t *ea_ibody_quota_blocks)
+ struct ea_quota *ea_ibody_quota)
{
struct ext2_super_block *sb = ctx->fs->super;
struct ext2_inode_large *inode;
@@ -420,9 +426,9 @@ static void check_ea_in_inode(e2fsck_t ctx, struct problem_context *pctx,
unsigned int storage_size, remain;
problem_t problem = 0;
region_t region = 0;
- blk64_t quota_blocks = 0;

- *ea_ibody_quota_blocks = 0;
+ ea_ibody_quota->blocks = 0;
+ ea_ibody_quota->inodes = 0;

inode = (struct ext2_inode_large *) pctx->inode;
storage_size = EXT2_INODE_SIZE(ctx->fs->super) - EXT2_GOOD_OLD_INODE_SIZE -
@@ -500,14 +506,15 @@ static void check_ea_in_inode(e2fsck_t ctx, struct problem_context *pctx,
goto fix;
}
} else {
- blk64_t entry_quota_blocks;
+ blk64_t quota_blocks;

problem = check_large_ea_inode(ctx, entry, pctx,
- &entry_quota_blocks);
+ &quota_blocks);
if (problem != 0)
goto fix;

- quota_blocks += entry_quota_blocks;
+ ea_ibody_quota->blocks += quota_blocks;
+ ea_ibody_quota->inodes++;
}

/* If EA value is stored in external inode then it does not
@@ -533,7 +540,6 @@ fix:
if (problem == 0 || !fix_problem(ctx, problem, pctx)) {
inc_ea_inode_refs(ctx, pctx,
(struct ext2_ext_attr_entry *)start, end);
- *ea_ibody_quota_blocks = quota_blocks;
return;
}

@@ -541,6 +547,8 @@ fix:
*((__u32 *)header) = 0UL;
e2fsck_write_inode_full(ctx, pctx->ino, pctx->inode,
EXT2_INODE_SIZE(sb), "pass1");
+ ea_ibody_quota->blocks = 0;
+ ea_ibody_quota->inodes = 0;
}

static int check_inode_extra_negative_epoch(__u32 xtime, __u32 extra) {
@@ -559,14 +567,15 @@ static int check_inode_extra_negative_epoch(__u32 xtime, __u32 extra) {
#define EXT4_EXTRA_NEGATIVE_DATE_CUTOFF 2 * (1LL << 32)

static void check_inode_extra_space(e2fsck_t ctx, struct problem_context *pctx,
- blk64_t *ea_ibody_quota_blocks)
+ struct ea_quota *ea_ibody_quota)
{
struct ext2_super_block *sb = ctx->fs->super;
struct ext2_inode_large *inode;
__u32 *eamagic;
int min, max;

- *ea_ibody_quota_blocks = 0;
+ ea_ibody_quota->blocks = 0;
+ ea_ibody_quota->inodes = 0;

inode = (struct ext2_inode_large *) pctx->inode;
if (EXT2_INODE_SIZE(sb) == EXT2_GOOD_OLD_INODE_SIZE) {
@@ -606,7 +615,7 @@ static void check_inode_extra_space(e2fsck_t ctx, struct problem_context *pctx,
inode->i_extra_isize);
if (*eamagic == EXT2_EXT_ATTR_MAGIC) {
/* it seems inode has an extended attribute(s) in body */
- check_ea_in_inode(ctx, pctx, ea_ibody_quota_blocks);
+ check_ea_in_inode(ctx, pctx, ea_ibody_quota);
}

/*
@@ -1164,7 +1173,7 @@ void e2fsck_pass1(e2fsck_t ctx)
int failed_csum = 0;
ext2_ino_t ino_threshold = 0;
dgrp_t ra_group = 0;
- blk64_t ea_ibody_quota_blocks;
+ struct ea_quota ea_ibody_quota;

init_resource_track(&rtrack, ctx->fs->io);
clear_problem_context(&pctx);
@@ -1710,7 +1719,7 @@ void e2fsck_pass1(e2fsck_t ctx)
"pass1");
failed_csum = 0;
}
- check_blocks(ctx, &pctx, block_buf, 0);
+ check_blocks(ctx, &pctx, block_buf, NULL);
FINISH_INODE_LOOP(ctx, ino, &pctx, failed_csum);
continue;
}
@@ -1737,7 +1746,7 @@ void e2fsck_pass1(e2fsck_t ctx)
"pass1");
failed_csum = 0;
}
- check_blocks(ctx, &pctx, block_buf, 0);
+ check_blocks(ctx, &pctx, block_buf, NULL);
FINISH_INODE_LOOP(ctx, ino, &pctx, failed_csum);
continue;
}
@@ -1775,7 +1784,7 @@ void e2fsck_pass1(e2fsck_t ctx)
failed_csum = 0;
}
}
- check_blocks(ctx, &pctx, block_buf, 0);
+ check_blocks(ctx, &pctx, block_buf, NULL);
FINISH_INODE_LOOP(ctx, ino, &pctx, failed_csum);
continue;
}
@@ -1840,7 +1849,7 @@ void e2fsck_pass1(e2fsck_t ctx)
}
}

- check_inode_extra_space(ctx, &pctx, &ea_ibody_quota_blocks);
+ check_inode_extra_space(ctx, &pctx, &ea_ibody_quota);
check_is_really_dir(ctx, &pctx, block_buf);

/*
@@ -1888,7 +1897,7 @@ void e2fsck_pass1(e2fsck_t ctx)
} else if (ext2fs_is_fast_symlink(inode)) {
ctx->fs_fast_symlinks_count++;
check_blocks(ctx, &pctx, block_buf,
- ea_ibody_quota_blocks);
+ &ea_ibody_quota);
FINISH_INODE_LOOP(ctx, ino, &pctx, failed_csum);
continue;
}
@@ -1926,15 +1935,14 @@ void e2fsck_pass1(e2fsck_t ctx)

itp = &inodes_to_process[process_inode_count];
itp->ino = ino;
- itp->ea_ibody_quota_blocks = ea_ibody_quota_blocks;
+ itp->ea_ibody_quota = ea_ibody_quota;
if (inode_size < sizeof(struct ext2_inode_large))
memcpy(&itp->inode, inode, inode_size);
else
memcpy(&itp->inode, inode, sizeof(itp->inode));
process_inode_count++;
} else
- check_blocks(ctx, &pctx, block_buf,
- ea_ibody_quota_blocks);
+ check_blocks(ctx, &pctx, block_buf, &ea_ibody_quota);

FINISH_INODE_LOOP(ctx, ino, &pctx, failed_csum);

@@ -1972,9 +1980,14 @@ void e2fsck_pass1(e2fsck_t ctx)
ctx->refcount_extra = 0;
}

- if (ctx->ea_block_quota) {
- ea_refcount_free(ctx->ea_block_quota);
- ctx->ea_block_quota = 0;
+ if (ctx->ea_block_quota_blocks) {
+ ea_refcount_free(ctx->ea_block_quota_blocks);
+ ctx->ea_block_quota_blocks = 0;
+ }
+
+ if (ctx->ea_block_quota_inodes) {
+ ea_refcount_free(ctx->ea_block_quota_inodes);
+ ctx->ea_block_quota_inodes = 0;
}

if (ctx->invalid_bitmaps)
@@ -2110,7 +2123,7 @@ static void process_inodes(e2fsck_t ctx, char *block_buf)
pctx.ino);
ehandler_operation(buf);
check_blocks(ctx, &pctx, block_buf,
- inodes_to_process[i].ea_ibody_quota_blocks);
+ &inodes_to_process[i].ea_ibody_quota);
if (ctx->flags & E2F_FLAG_SIGNAL_MASK)
break;
}
@@ -2330,7 +2343,7 @@ static void adjust_extattr_refcount(e2fsck_t ctx, ext2_refcount_t refcount,
* Handle processing the extended attribute blocks
*/
static int check_ext_attr(e2fsck_t ctx, struct problem_context *pctx,
- char *block_buf, blk64_t *ea_block_quota_blocks)
+ char *block_buf, struct ea_quota *ea_block_quota)
{
ext2_filsys fs = ctx->fs;
ext2_ino_t ino = pctx->ino;
@@ -2340,9 +2353,13 @@ static int check_ext_attr(e2fsck_t ctx, struct problem_context *pctx,
struct ext2_ext_attr_header *header;
struct ext2_ext_attr_entry *first, *entry;
blk64_t quota_blocks = EXT2FS_C2B(fs, 1);
+ __u64 quota_inodes = 0;
region_t region = 0;
int failed_csum = 0;

+ ea_block_quota->blocks = 0;
+ ea_block_quota->inodes = 0;
+
blk = ext2fs_file_acl_block(fs, inode);
if (blk == 0)
return 0;
@@ -2393,11 +2410,19 @@ static int check_ext_attr(e2fsck_t ctx, struct problem_context *pctx,

/* Have we seen this EA block before? */
if (ext2fs_fast_test_block_bitmap2(ctx->block_ea_map, blk)) {
- if (ctx->ea_block_quota)
- ea_refcount_fetch(ctx->ea_block_quota, blk,
- ea_block_quota_blocks);
- else
- *ea_block_quota_blocks = 0;
+ ea_block_quota->blocks = EXT2FS_C2B(fs, 1);
+ ea_block_quota->inodes = 0;
+
+ if (ctx->ea_block_quota_blocks) {
+ ea_refcount_fetch(ctx->ea_block_quota_blocks, blk,
+ &quota_blocks);
+ if (quota_blocks)
+ ea_block_quota->blocks = quota_blocks;
+ }
+
+ if (ctx->ea_block_quota_inodes)
+ ea_refcount_fetch(ctx->ea_block_quota_inodes, blk,
+ &ea_block_quota->inodes);

if (ea_refcount_decrement(ctx->refcount, blk, 0) == 0)
return 1;
@@ -2516,6 +2541,7 @@ static int check_ext_attr(e2fsck_t ctx, struct problem_context *pctx,
goto clear_extattr;

quota_blocks += entry_quota_blocks;
+ quota_inodes++;
}

entry = EXT2_EXT_ATTR_NEXT(entry);
@@ -2538,20 +2564,38 @@ static int check_ext_attr(e2fsck_t ctx, struct problem_context *pctx,
return 0;
}

- *ea_block_quota_blocks = quota_blocks;
- if (quota_blocks) {
- if (!ctx->ea_block_quota) {
+ if (quota_blocks != EXT2FS_C2B(fs, 1)) {
+ if (!ctx->ea_block_quota_blocks) {
pctx->errcode = ea_refcount_create(0,
- &ctx->ea_block_quota);
+ &ctx->ea_block_quota_blocks);
if (pctx->errcode) {
pctx->num = 3;
+ goto refcount_fail;
+ }
+ }
+ ea_refcount_store(ctx->ea_block_quota_blocks, blk,
+ quota_blocks);
+ }
+
+ if (quota_inodes) {
+ if (!ctx->ea_block_quota_inodes) {
+ pctx->errcode = ea_refcount_create(0,
+ &ctx->ea_block_quota_inodes);
+ if (pctx->errcode) {
+ pctx->num = 4;
+refcount_fail:
fix_problem(ctx, PR_1_ALLOCATE_REFCOUNT, pctx);
ctx->flags |= E2F_FLAG_ABORT;
return 0;
}
}
- ea_refcount_store(ctx->ea_block_quota, blk, quota_blocks);
+
+ ea_refcount_store(ctx->ea_block_quota_inodes, blk,
+ quota_inodes);
}
+ ea_block_quota->blocks = quota_blocks;
+ ea_block_quota->inodes = quota_inodes;
+
inc_ea_inode_refs(ctx, pctx, first, end);
ea_refcount_store(ctx->refcount, blk, header->h_refcount - 1);
mark_block_used(ctx, blk);
@@ -3207,7 +3251,7 @@ err:
* blocks used by that inode.
*/
static void check_blocks(e2fsck_t ctx, struct problem_context *pctx,
- char *block_buf, blk64_t ea_ibody_quota_blocks)
+ char *block_buf, const struct ea_quota *ea_ibody_quota)
{
ext2_filsys fs = ctx->fs;
struct process_block_struct pb;
@@ -3218,10 +3262,11 @@ static void check_blocks(e2fsck_t ctx, struct problem_context *pctx,
int extent_fs;
int inlinedata_fs;
__u64 size;
- blk64_t ea_block_quota_blocks = 0;
+ struct ea_quota ea_block_quota;

pb.ino = ino;
- pb.num_blocks = EXT2FS_B2C(ctx->fs, ea_ibody_quota_blocks);
+ pb.num_blocks = EXT2FS_B2C(ctx->fs,
+ ea_ibody_quota ? ea_ibody_quota->blocks : 0);
pb.last_block = ~0;
pb.last_init_lblock = -1;
pb.last_db_block = -1;
@@ -3244,10 +3289,10 @@ static void check_blocks(e2fsck_t ctx, struct problem_context *pctx,
extent_fs = ext2fs_has_feature_extents(ctx->fs->super);
inlinedata_fs = ext2fs_has_feature_inline_data(ctx->fs->super);

- if (check_ext_attr(ctx, pctx, block_buf, &ea_block_quota_blocks)) {
+ if (check_ext_attr(ctx, pctx, block_buf, &ea_block_quota)) {
if (ctx->flags & E2F_FLAG_SIGNAL_MASK)
goto out;
- pb.num_blocks += EXT2FS_B2C(ctx->fs, ea_block_quota_blocks);
+ pb.num_blocks += EXT2FS_B2C(ctx->fs, ea_block_quota.blocks);
}

if (inlinedata_fs && (inode->i_flags & EXT4_INLINE_DATA_FL))
@@ -3337,12 +3382,15 @@ static void check_blocks(e2fsck_t ctx, struct problem_context *pctx,
}

if (ino != quota_type2inum(PRJQUOTA, fs->super) &&
- (ino == EXT2_ROOT_INO || ino >= EXT2_FIRST_INODE(ctx->fs->super))) {
+ (ino == EXT2_ROOT_INO || ino >= EXT2_FIRST_INODE(ctx->fs->super)) &&
+ !(inode->i_flags & EXT4_EA_INODE_FL)) {
quota_data_add(ctx->qctx, (struct ext2_inode_large *) inode,
ino,
pb.num_blocks * EXT2_CLUSTER_SIZE(fs->super));
quota_data_inodes(ctx->qctx, (struct ext2_inode_large *) inode,
- ino, +1);
+ ino, (ea_ibody_quota ?
+ ea_ibody_quota->inodes : 0) +
+ ea_block_quota.inodes + 1);
}

if (!ext2fs_has_feature_huge_file(fs->super) ||
--
2.13.2.725.g09c95d1e9-goog


2017-07-07 12:12:53

by Tahsin Erdogan

[permalink] [raw]
Subject: [PATCH 2/4] libext2fs: rename ext2_xattr_handle->length to capacity

ext2_xattr_handle has two fields 'count' and 'length' which
represent number of filled elements vs total element count.
They have close meanings so are easy to confuse, thus make code less
readable. Rename length to capacity.

Signed-off-by: Tahsin Erdogan <[email protected]>
---
lib/ext2fs/ext_attr.c | 34 +++++++++++++++++-----------------
1 file changed, 17 insertions(+), 17 deletions(-)

diff --git a/lib/ext2fs/ext_attr.c b/lib/ext2fs/ext_attr.c
index 4357bdd0caae..2e9fc96d114d 100644
--- a/lib/ext2fs/ext_attr.c
+++ b/lib/ext2fs/ext_attr.c
@@ -291,7 +291,7 @@ struct ext2_xattr_handle {
errcode_t magic;
ext2_filsys fs;
struct ext2_xattr *attrs;
- size_t length, count;
+ size_t capacity, count;
ext2_ino_t ino;
unsigned int flags;
int dirty;
@@ -303,14 +303,14 @@ static errcode_t ext2fs_xattrs_expand(struct ext2_xattr_handle *h,
struct ext2_xattr *new_attrs;
errcode_t err;

- err = ext2fs_get_arrayzero(h->length + expandby,
+ err = ext2fs_get_arrayzero(h->capacity + expandby,
sizeof(struct ext2_xattr), &new_attrs);
if (err)
return err;

- memcpy(new_attrs, h->attrs, h->length * sizeof(struct ext2_xattr));
+ memcpy(new_attrs, h->attrs, h->capacity * sizeof(struct ext2_xattr));
ext2fs_free_mem(&h->attrs);
- h->length += expandby;
+ h->capacity += expandby;
h->attrs = new_attrs;

return 0;
@@ -675,7 +675,7 @@ static errcode_t write_xattrs_to_buffer(struct ext2_xattr_handle *handle,

memset(entries_start, 0, storage_size);
/* For all remaining x... */
- for (; x < handle->attrs + handle->length; x++) {
+ for (; x < handle->attrs + handle->capacity; x++) {
if (!x->name)
continue;

@@ -771,7 +771,7 @@ errcode_t ext2fs_xattrs_write(struct ext2_xattr_handle *handle)
* to the end.
*/
x = handle->attrs;
- qsort(x, handle->length, sizeof(struct ext2_xattr), attr_compare);
+ qsort(x, handle->capacity, sizeof(struct ext2_xattr), attr_compare);

/* Does the inode have space for EA? */
if (inode->i_extra_isize < sizeof(inode->i_extra_isize) ||
@@ -813,7 +813,7 @@ write_ea_block:
if (err)
goto out2;

- if (x < handle->attrs + handle->length) {
+ if (x < handle->attrs + handle->capacity) {
err = EXT2_ET_EA_NO_SPACE;
goto out2;
}
@@ -904,11 +904,11 @@ static errcode_t read_xattrs_from_buffer(struct ext2_xattr_handle *handle,
!EXT2_EXT_IS_LAST_ENTRY(entry)) {

/* Allocate space for more attrs? */
- if (x == handle->attrs + handle->length) {
+ if (x == handle->attrs + handle->capacity) {
err = ext2fs_xattrs_expand(handle, 4);
if (err)
return err;
- x = handle->attrs + handle->length - 4;
+ x = handle->attrs + handle->capacity - 4;
}

/* header eats this space */
@@ -1026,7 +1026,7 @@ static void xattrs_free_keys(struct ext2_xattr_handle *h)
struct ext2_xattr *a = h->attrs;
size_t i;

- for (i = 0; i < h->length; i++) {
+ for (i = 0; i < h->capacity; i++) {
if (a[i].name)
ext2fs_free_mem(&a[i].name);
if (a[i].value)
@@ -1149,7 +1149,7 @@ errcode_t ext2fs_xattrs_iterate(struct ext2_xattr_handle *h,
int ret;

EXT2_CHECK_MAGIC(h, EXT2_ET_MAGIC_EA_HANDLE);
- for (x = h->attrs; x < h->attrs + h->length; x++) {
+ for (x = h->attrs; x < h->attrs + h->capacity; x++) {
if (!x->name)
continue;

@@ -1171,7 +1171,7 @@ errcode_t ext2fs_xattr_get(struct ext2_xattr_handle *h, const char *key,
errcode_t err;

EXT2_CHECK_MAGIC(h, EXT2_ET_MAGIC_EA_HANDLE);
- for (x = h->attrs; x < h->attrs + h->length; x++) {
+ for (x = h->attrs; x < h->attrs + h->capacity; x++) {
if (!x->name || strcmp(x->name, key))
continue;

@@ -1280,7 +1280,7 @@ errcode_t ext2fs_xattr_set(struct ext2_xattr_handle *handle,
} else
memcpy(new_value, value, value_len);

- for (x = handle->attrs; x < handle->attrs + handle->length; x++) {
+ for (x = handle->attrs; x < handle->attrs + handle->capacity; x++) {
if (!x->name) {
last_empty = x;
continue;
@@ -1314,7 +1314,7 @@ errcode_t ext2fs_xattr_set(struct ext2_xattr_handle *handle,
if (err)
goto errout;

- x = handle->attrs + handle->length - 4;
+ x = handle->attrs + handle->capacity - 4;
err = ext2fs_get_mem(strlen(key) + 1, &x->name);
if (err)
goto errout;
@@ -1339,7 +1339,7 @@ errcode_t ext2fs_xattr_remove(struct ext2_xattr_handle *handle,
struct ext2_xattr *x;

EXT2_CHECK_MAGIC(handle, EXT2_ET_MAGIC_EA_HANDLE);
- for (x = handle->attrs; x < handle->attrs + handle->length; x++) {
+ for (x = handle->attrs; x < handle->attrs + handle->capacity; x++) {
if (!x->name)
continue;

@@ -1372,8 +1372,8 @@ errcode_t ext2fs_xattrs_open(ext2_filsys fs, ext2_ino_t ino,
return err;

h->magic = EXT2_ET_MAGIC_EA_HANDLE;
- h->length = 4;
- err = ext2fs_get_arrayzero(h->length, sizeof(struct ext2_xattr),
+ h->capacity = 4;
+ err = ext2fs_get_arrayzero(h->capacity, sizeof(struct ext2_xattr),
&h->attrs);
if (err) {
ext2fs_free_mem(&h);
--
2.13.2.725.g09c95d1e9-goog

2017-07-07 12:12:57

by Tahsin Erdogan

[permalink] [raw]
Subject: [PATCH 4/4] libext2fs: add ea_inode support to set xattr

This patch is a major update to how we decide where to put extended
attributes. The main motivation is to enable creating values in extended
attribute inodes. While doing this, we want to implement a behavior
that is as close to kernel as possible.

Existing set ea code deviates from kernel behavior which makes it harder
to implement ea_inode feature:

- kernel only sorts ea entries in xattr block, e2fsprogs implementation
sorts all entries on every update.

- e2fsprogs implementation shuffled things on every update so the order
of updates does not matter. Kernel does not reshuffle things.

- e2fsprogs could evacuate entries from inode body to xattr block and
vice versa. This behavior does not exist in kernel.

Such differences could lead to inconsistent behavior between fuse2fs and
a kernel mount.

With ea_inode feature, we also need to decide whether to put a value in
an inode or keep it 'inline'. In kernel implementation this depends on
current placement of entries.

To close the behavioral gap, ext2fs_xattr_set() now takes over the
decision about where to place ea values. This also allows it to raise
errors early instead of delaying them to a separate
ext2fs_xattrs_write() call later.

Signed-off-by: Tahsin Erdogan <[email protected]>
---
contrib/android/perms.c | 6 -
debugfs/xattrs.c | 11 -
e2fsck/pass4.c | 20 +-
lib/ext2fs/ext2fs.h | 2 +
lib/ext2fs/ext_attr.c | 659 +++++++++++++++++++++++++++++++++++------------
lib/ext2fs/inline_data.c | 10 -
misc/fuse2fs.c | 17 --
7 files changed, 499 insertions(+), 226 deletions(-)

diff --git a/contrib/android/perms.c b/contrib/android/perms.c
index 9a7a93f5564b..08fb861486bf 100644
--- a/contrib/android/perms.c
+++ b/contrib/android/perms.c
@@ -48,12 +48,6 @@ static errcode_t ino_add_xattr(ext2_filsys fs, ext2_ino_t ino, const char *name,
_("while setting xattrs of inode %u"), ino);
goto xattrs_close;
}
- retval = ext2fs_xattrs_write(xhandle);
- if (retval) {
- com_err(__func__, retval,
- _("while writting xattrs of inode %u"), ino);
- goto xattrs_close;
- }
xattrs_close:
close_retval = ext2fs_xattrs_close(&xhandle);
if (close_retval) {
diff --git a/debugfs/xattrs.c b/debugfs/xattrs.c
index 9b87d14551dc..fdb76e41a230 100644
--- a/debugfs/xattrs.c
+++ b/debugfs/xattrs.c
@@ -306,13 +306,6 @@ void do_set_xattr(int argc, char **argv)
}

err = ext2fs_xattr_set(h, argv[optind + 1], buf, buflen);
- if (err)
- goto out;
-
- err = ext2fs_xattrs_write(h);
- if (err)
- goto out;
-
out:
ext2fs_xattrs_close(&h);
if (err)
@@ -360,10 +353,6 @@ void do_rm_xattr(int argc, char **argv)
if (err)
goto out;
}
-
- err = ext2fs_xattrs_write(h);
- if (err)
- goto out;
out:
ext2fs_xattrs_close(&h);
if (err)
diff --git a/e2fsck/pass4.c b/e2fsck/pass4.c
index 663f87ab59c0..4125c53b1b97 100644
--- a/e2fsck/pass4.c
+++ b/e2fsck/pass4.c
@@ -87,22 +87,6 @@ static int disconnect_inode(e2fsck_t ctx, ext2_ino_t i,
return 0;
}

-/*
- * Get/set ref functions below could later be moved to somewhere in lib/ext2fs/.
- * Currently the only user is e2fsck so we rather not expose it in common
- * library until there are more users.
- */
-static __u64 ea_inode_get_ref(struct ext2_inode_large *inode)
-{
- return ((__u64)inode->i_ctime << 32) | inode->osd1.linux1.l_i_version;
-}
-
-static void ea_inode_set_ref(struct ext2_inode_large *inode, __u64 ref_count)
-{
- inode->i_ctime = (__u32)(ref_count >> 32);
- inode->osd1.linux1.l_i_version = (__u32)ref_count;
-}
-
static void check_ea_inode(e2fsck_t ctx, ext2_ino_t i,
struct ext2_inode_large *inode, __u16 *link_counted)
{
@@ -140,7 +124,7 @@ static void check_ea_inode(e2fsck_t ctx, ext2_ino_t i,
*/
*link_counted = 1;

- ref_count = ea_inode_get_ref(inode);
+ ref_count = ext2fs_get_ea_inode_ref(EXT2_INODE(inode));

/* Old Lustre-style xattr inodes do not have a stored refcount.
* However, their i_ctime and i_atime should be the same.
@@ -153,7 +137,7 @@ static void check_ea_inode(e2fsck_t ctx, ext2_ino_t i,
pctx.num = ref_count;
pctx.num2 = actual_refs;
if (fix_problem(ctx, PR_4_EA_INODE_REF_COUNT, &pctx)) {
- ea_inode_set_ref(inode, actual_refs);
+ ext2fs_set_ea_inode_ref(EXT2_INODE(inode), actual_refs);
e2fsck_write_inode(ctx, i, EXT2_INODE(inode), "pass4");
}
}
diff --git a/lib/ext2fs/ext2fs.h b/lib/ext2fs/ext2fs.h
index b734f1aa2d5b..a2c8edaa4ac2 100644
--- a/lib/ext2fs/ext2fs.h
+++ b/lib/ext2fs/ext2fs.h
@@ -1251,6 +1251,8 @@ extern void ext2fs_ext_attr_block_rehash(struct ext2_ext_attr_header *header,
struct ext2_ext_attr_entry *end);
extern __u32 ext2fs_get_ea_inode_hash(struct ext2_inode *inode);
extern void ext2fs_set_ea_inode_hash(struct ext2_inode *inode, __u32 hash);
+extern __u64 ext2fs_get_ea_inode_ref(struct ext2_inode *inode);
+extern void ext2fs_set_ea_inode_ref(struct ext2_inode *inode, __u64 ref_count);

/* extent.c */
extern errcode_t ext2fs_extent_header_verify(void *ptr, int size);
diff --git a/lib/ext2fs/ext_attr.c b/lib/ext2fs/ext_attr.c
index 00ff79ae3890..7cecb16885aa 100644
--- a/lib/ext2fs/ext_attr.c
+++ b/lib/ext2fs/ext_attr.c
@@ -139,6 +139,17 @@ void ext2fs_set_ea_inode_hash(struct ext2_inode *inode, __u32 hash)
inode->i_atime = hash;
}

+__u64 ext2fs_get_ea_inode_ref(struct ext2_inode *inode)
+{
+ return ((__u64)inode->i_ctime << 32) | inode->osd1.linux1.l_i_version;
+}
+
+void ext2fs_set_ea_inode_ref(struct ext2_inode *inode, __u64 ref_count)
+{
+ inode->i_ctime = (__u32)(ref_count >> 32);
+ inode->osd1.linux1.l_i_version = (__u32)ref_count;
+}
+
static errcode_t check_ext_attr_header(struct ext2_ext_attr_header *header)
{
if ((header->h_magic != EXT2_EXT_ATTR_MAGIC_v1 &&
@@ -284,17 +295,19 @@ errcode_t ext2fs_adjust_ea_refcount(ext2_filsys fs, blk_t blk,
struct ext2_xattr {
char *name;
void *value;
- size_t value_len;
+ unsigned int value_len;
+ ext2_ino_t ea_ino;
};

struct ext2_xattr_handle {
errcode_t magic;
ext2_filsys fs;
struct ext2_xattr *attrs;
- size_t capacity, count;
+ int capacity;
+ int count;
+ int ibody_count;
ext2_ino_t ino;
unsigned int flags;
- int dirty;
};

static errcode_t ext2fs_xattrs_expand(struct ext2_xattr_handle *h,
@@ -333,40 +346,6 @@ static struct ea_name_index ea_names[] = {
{0, NULL},
};

-static int find_ea_index(char *fullname, char **name, int *index);
-
-/* Pull inlinedata to the front. */
-static int attr_compare(const void *a, const void *b)
-{
- const struct ext2_xattr *xa = a, *xb = b;
- char *xa_suffix, *xb_suffix;
- int xa_idx, xb_idx;
- int cmp;
-
- if (!strcmp(xa->name, "system.data"))
- return -1;
- else if (!strcmp(xb->name, "system.data"))
- return +1;
-
- /*
- * Duplicate the kernel's sorting algorithm because xattr blocks
- * require sorted keys.
- */
- xa_suffix = xa->name;
- xb_suffix = xb->name;
- xa_idx = xb_idx = 0;
- find_ea_index(xa->name, &xa_suffix, &xa_idx);
- find_ea_index(xb->name, &xb_suffix, &xb_idx);
- cmp = xa_idx - xb_idx;
- if (cmp)
- return cmp;
- cmp = strlen(xa_suffix) - strlen(xb_suffix);
- if (cmp)
- return cmp;
- cmp = strcmp(xa_suffix, xb_suffix);
- return cmp;
-}
-
static const char *find_ea_prefix(int index)
{
struct ea_name_index *e;
@@ -378,7 +357,7 @@ static const char *find_ea_prefix(int index)
return NULL;
}

-static int find_ea_index(char *fullname, char **name, int *index)
+static int find_ea_index(const char *fullname, char **name, int *index)
{
struct ea_name_index *e;

@@ -654,24 +633,21 @@ static errcode_t convert_disk_buffer_to_posix_acl(const void *value, size_t size
return 0;
}

-
-static errcode_t write_xattrs_to_buffer(struct ext2_xattr_handle *handle,
- struct ext2_xattr **pos,
- void *entries_start,
- unsigned int storage_size,
- unsigned int value_offset_correction,
- int write_hash)
+static errcode_t
+write_xattrs_to_buffer(ext2_filsys fs, struct ext2_xattr *attrs, int count,
+ void *entries_start, unsigned int storage_size,
+ unsigned int value_offset_correction, int write_hash)
{
- struct ext2_xattr *x = *pos;
+ struct ext2_xattr *x;
struct ext2_ext_attr_entry *e = entries_start;
- char *end = (char *) entries_start + storage_size;
+ void *end = entries_start + storage_size;
char *shortname;
unsigned int entry_size, value_size;
int idx, ret;
+ errcode_t err;

memset(entries_start, 0, storage_size);
- /* For all remaining x... */
- for (; x < handle->attrs + handle->count; x++) {
+ for (x = attrs; x < attrs + count; x++) {
/* Calculate index and shortname position */
shortname = x->name;
ret = find_ea_index(x->name, &shortname, &idx);
@@ -683,43 +659,43 @@ static errcode_t write_xattrs_to_buffer(struct ext2_xattr_handle *handle,
value_size = ((x->value_len + EXT2_EXT_ATTR_PAD - 1) /
EXT2_EXT_ATTR_PAD) * EXT2_EXT_ATTR_PAD;

- /*
- * Would entry collide with value?
- * Note that we must leave sufficient room for a (u32)0 to
- * mark the end of the entries.
- */
- if ((char *)e + entry_size + sizeof(__u32) > end - value_size)
- break;
-
/* Fill out e appropriately */
e->e_name_len = strlen(shortname);
e->e_name_index = (ret ? idx : 0);
- e->e_value_offs = end - value_size - (char *)entries_start +
- value_offset_correction;
- e->e_value_inum = 0;
+
e->e_value_size = x->value_len;
+ e->e_value_inum = x->ea_ino;

- /* Store name and value */
- end -= value_size;
+ /* Store name */
memcpy((char *)e + sizeof(*e), shortname, e->e_name_len);
- memcpy(end, x->value, e->e_value_size);
+ if (x->ea_ino) {
+ e->e_value_offs = 0;
+ } else {
+ end -= value_size;
+ e->e_value_offs = end - entries_start +
+ value_offset_correction;
+ memcpy(end, x->value, e->e_value_size);
+ }

- if (write_hash)
- e->e_hash = ext2fs_ext_attr_hash_entry(e, end);
- else
+ if (write_hash || x->ea_ino) {
+ err = ext2fs_ext_attr_hash_entry2(fs, e,
+ x->ea_ino ? 0 : end,
+ &e->e_hash);
+ if (err)
+ return err;
+ } else
e->e_hash = 0;

e = EXT2_EXT_ATTR_NEXT(e);
*(__u32 *)e = 0;
}
- *pos = x;
-
return 0;
}

errcode_t ext2fs_xattrs_write(struct ext2_xattr_handle *handle)
{
- struct ext2_xattr *x;
+ ext2_filsys fs = handle->fs;
+ const int inode_size = EXT2_INODE_SIZE(fs->super);
struct ext2_inode_large *inode;
char *start, *block_buf = NULL;
struct ext2_ext_attr_header *header;
@@ -730,24 +706,23 @@ errcode_t ext2fs_xattrs_write(struct ext2_xattr_handle *handle)
errcode_t err;

EXT2_CHECK_MAGIC(handle, EXT2_ET_MAGIC_EA_HANDLE);
- i = EXT2_INODE_SIZE(handle->fs->super);
+ i = inode_size;
if (i < sizeof(*inode))
i = sizeof(*inode);
err = ext2fs_get_memzero(i, &inode);
if (err)
return err;

- err = ext2fs_read_inode_full(handle->fs, handle->ino,
- (struct ext2_inode *)inode,
- EXT2_INODE_SIZE(handle->fs->super));
+ err = ext2fs_read_inode_full(fs, handle->ino, EXT2_INODE(inode),
+ inode_size);
if (err)
goto out;

/* If extra_isize isn't set, we need to set it now */
if (inode->i_extra_isize == 0 &&
- EXT2_INODE_SIZE(handle->fs->super) > EXT2_GOOD_OLD_INODE_SIZE) {
+ inode_size > EXT2_GOOD_OLD_INODE_SIZE) {
char *p = (char *)inode;
- size_t extra = handle->fs->super->s_want_extra_isize;
+ size_t extra = fs->super->s_want_extra_isize;

if (extra == 0)
extra = sizeof(__u32);
@@ -759,55 +734,45 @@ errcode_t ext2fs_xattrs_write(struct ext2_xattr_handle *handle)
goto out;
}

- /* Force the inlinedata attr to the front. */
- x = handle->attrs;
- qsort(x, handle->count, sizeof(struct ext2_xattr), attr_compare);
-
/* Does the inode have space for EA? */
if (inode->i_extra_isize < sizeof(inode->i_extra_isize) ||
- EXT2_INODE_SIZE(handle->fs->super) <= EXT2_GOOD_OLD_INODE_SIZE +
- inode->i_extra_isize +
- sizeof(__u32))
+ inode_size <= EXT2_GOOD_OLD_INODE_SIZE + inode->i_extra_isize +
+ sizeof(__u32))
goto write_ea_block;

/* Write the inode EA */
ea_inode_magic = EXT2_EXT_ATTR_MAGIC;
memcpy(((char *) inode) + EXT2_GOOD_OLD_INODE_SIZE +
inode->i_extra_isize, &ea_inode_magic, sizeof(__u32));
- storage_size = EXT2_INODE_SIZE(handle->fs->super) -
- EXT2_GOOD_OLD_INODE_SIZE - inode->i_extra_isize -
- sizeof(__u32);
+ storage_size = inode_size - EXT2_GOOD_OLD_INODE_SIZE -
+ inode->i_extra_isize - sizeof(__u32);
start = ((char *) inode) + EXT2_GOOD_OLD_INODE_SIZE +
- inode->i_extra_isize + sizeof(__u32);
+ inode->i_extra_isize + sizeof(__u32);

- err = write_xattrs_to_buffer(handle, &x, start, storage_size, 0, 0);
+ err = write_xattrs_to_buffer(fs, handle->attrs, handle->ibody_count,
+ start, storage_size, 0, 0);
if (err)
goto out;
-
write_ea_block:
/* Are we done? */
- if (x >= handle->attrs + handle->count)
+ if (handle->ibody_count == handle->count &&
+ !ext2fs_file_acl_block(fs, EXT2_INODE(inode)))
goto skip_ea_block;

/* Write the EA block */
- err = ext2fs_get_memzero(handle->fs->blocksize, &block_buf);
+ err = ext2fs_get_memzero(fs->blocksize, &block_buf);
if (err)
goto out;

- storage_size = handle->fs->blocksize -
- sizeof(struct ext2_ext_attr_header);
+ storage_size = fs->blocksize - sizeof(struct ext2_ext_attr_header);
start = block_buf + sizeof(struct ext2_ext_attr_header);

- err = write_xattrs_to_buffer(handle, &x, start, storage_size,
- start - block_buf, 1);
+ err = write_xattrs_to_buffer(fs, handle->attrs + handle->ibody_count,
+ handle->count - handle->ibody_count, start,
+ storage_size, start - block_buf, 1);
if (err)
goto out2;

- if (x < handle->attrs + handle->count) {
- err = EXT2_ET_EA_NO_SPACE;
- goto out2;
- }
-
/* Write a header on the EA block */
header = (struct ext2_ext_attr_header *) block_buf;
header->h_magic = EXT2_EXT_ATTR_MAGIC;
@@ -815,31 +780,28 @@ write_ea_block:
header->h_blocks = 1;

/* Get a new block for writing */
- err = prep_ea_block_for_write(handle->fs, handle->ino, inode);
+ err = prep_ea_block_for_write(fs, handle->ino, inode);
if (err)
goto out2;

/* Finally, write the new EA block */
- blk = ext2fs_file_acl_block(handle->fs,
- (struct ext2_inode *)inode);
- err = ext2fs_write_ext_attr3(handle->fs, blk, block_buf,
- handle->ino);
+ blk = ext2fs_file_acl_block(fs, EXT2_INODE(inode));
+ err = ext2fs_write_ext_attr3(fs, blk, block_buf, handle->ino);
if (err)
goto out2;

skip_ea_block:
- blk = ext2fs_file_acl_block(handle->fs, (struct ext2_inode *)inode);
+ blk = ext2fs_file_acl_block(fs, (struct ext2_inode *)inode);
if (!block_buf && blk) {
/* xattrs shrunk, free the block */
- err = ext2fs_free_ext_attr(handle->fs, handle->ino, inode);
+ err = ext2fs_free_ext_attr(fs, handle->ino, inode);
if (err)
goto out;
}

/* Write the inode */
- err = ext2fs_write_inode_full(handle->fs, handle->ino,
- (struct ext2_inode *)inode,
- EXT2_INODE_SIZE(handle->fs->super));
+ err = ext2fs_write_inode_full(fs, handle->ino, EXT2_INODE(inode),
+ inode_size);
if (err)
goto out2;

@@ -847,7 +809,6 @@ out2:
ext2fs_free_mem(&block_buf);
out:
ext2fs_free_mem(&inode);
- handle->dirty = 0;
return err;
}

@@ -970,6 +931,7 @@ static errcode_t read_xattrs_from_buffer(struct ext2_xattr_handle *handle,
return err;
}

+ x->ea_ino = entry->e_value_inum;
x->value_len = entry->e_value_size;

/* e_hash may be 0 in older inode's ea */
@@ -1018,6 +980,7 @@ static void xattrs_free_keys(struct ext2_xattr_handle *h)
ext2fs_free_mem(&a[i].value);
}
h->count = 0;
+ h->ibody_count = 0;
}

errcode_t ext2fs_xattrs_read(struct ext2_xattr_handle *handle)
@@ -1073,6 +1036,8 @@ errcode_t ext2fs_xattrs_read(struct ext2_xattr_handle *handle)
storage_size, start);
if (err)
goto out;
+
+ handle->ibody_count = handle->count;
}

read_ea_block:
@@ -1131,17 +1096,20 @@ errcode_t ext2fs_xattrs_iterate(struct ext2_xattr_handle *h,
void *data)
{
struct ext2_xattr *x;
+ int dirty = 0;
int ret;

EXT2_CHECK_MAGIC(h, EXT2_ET_MAGIC_EA_HANDLE);
for (x = h->attrs; x < h->attrs + h->count; x++) {
ret = func(x->name, x->value, x->value_len, data);
if (ret & XATTR_CHANGED)
- h->dirty = 1;
+ dirty = 1;
if (ret & XATTR_ABORT)
- return 0;
+ break;
}

+ if (dirty)
+ return ext2fs_xattrs_write(h);
return 0;
}

@@ -1237,66 +1205,433 @@ out:
return err;
}

-errcode_t ext2fs_xattr_set(struct ext2_xattr_handle *handle,
- const char *key,
+static errcode_t xattr_create_ea_inode(ext2_filsys fs, const void *value,
+ size_t value_len, ext2_ino_t *ea_ino)
+{
+ struct ext2_inode inode;
+ ext2_ino_t ino;
+ ext2_file_t file;
+ __u32 hash;
+ errcode_t ret;
+
+ ret = ext2fs_new_inode(fs, 0, 0, 0, &ino);
+ if (ret)
+ return ret;
+
+ memset(&inode, 0, sizeof(inode));
+ inode.i_flags |= EXT4_EA_INODE_FL;
+ if (ext2fs_has_feature_extents(fs->super))
+ inode.i_flags |= EXT4_EXTENTS_FL;
+ inode.i_size = 0;
+ inode.i_mode = LINUX_S_IFREG | 0600;
+ inode.i_links_count = 1;
+ ret = ext2fs_write_new_inode(fs, ino, &inode);
+ if (ret)
+ return ret;
+ /*
+ * ref_count and hash utilize inode's i_*time fields.
+ * ext2fs_write_new_inode() call above initializes these fields with
+ * current time. That's why ref count and hash updates are done
+ * separately below.
+ */
+ ext2fs_set_ea_inode_ref(&inode, 1);
+ hash = ext2fs_crc32c_le(fs->csum_seed, value, value_len);
+ ext2fs_set_ea_inode_hash(&inode, hash);
+
+ ret = ext2fs_write_inode(fs, ino, &inode);
+ if (ret)
+ return ret;
+
+ ret = ext2fs_file_open(fs, ino, EXT2_FILE_WRITE, &file);
+ if (ret)
+ return ret;
+ ret = ext2fs_file_write(file, value, value_len, NULL);
+ ext2fs_file_close(file);
+ if (ret)
+ return ret;
+
+ ext2fs_inode_alloc_stats2(fs, ino, 1 /* inuse */, 0 /* isdir */);
+
+ *ea_ino = ino;
+ return 0;
+}
+
+static errcode_t xattr_inode_dec_ref(ext2_filsys fs, ext2_ino_t ino)
+{
+ struct ext2_inode_large inode;
+ __u64 ref_count;
+ errcode_t ret;
+
+ ret = ext2fs_read_inode_full(fs, ino, (struct ext2_inode *)&inode,
+ sizeof(inode));
+ if (ret)
+ goto out;
+
+ ref_count = ext2fs_get_ea_inode_ref(EXT2_INODE(&inode));
+ ref_count--;
+ ext2fs_set_ea_inode_ref(EXT2_INODE(&inode), ref_count);
+
+ if (ref_count)
+ goto write_out;
+
+ inode.i_links_count = 0;
+ inode.i_dtime = fs->now ? fs->now : time(0);
+
+ ret = ext2fs_free_ext_attr(fs, ino, &inode);
+ if (ret)
+ goto write_out;
+
+ if (ext2fs_inode_has_valid_blocks2(fs, (struct ext2_inode *)&inode)) {
+ ret = ext2fs_punch(fs, ino, (struct ext2_inode *)&inode, NULL,
+ 0, ~0ULL);
+ if (ret)
+ goto out;
+ }
+
+ ext2fs_inode_alloc_stats2(fs, ino, -1 /* inuse */, 0 /* is_dir */);
+
+write_out:
+ ret = ext2fs_write_inode_full(fs, ino, (struct ext2_inode *)&inode,
+ sizeof(inode));
+out:
+ return ret;
+}
+
+static errcode_t xattr_update_entry(ext2_filsys fs, struct ext2_xattr *x,
+ const char *name, const void *value,
+ size_t value_len, int in_inode)
+{
+ ext2_ino_t ea_ino = 0;
+ void *new_value = NULL;
+ char *new_name = NULL;
+ int name_len;
+ errcode_t ret;
+
+ if (!x->name) {
+ name_len = strlen(name);
+ ret = ext2fs_get_mem(name_len + 1, &new_name);
+ if (ret)
+ goto fail;
+ memcpy(new_name, name, name_len + 1);
+ }
+
+ ret = ext2fs_get_mem(value_len, &new_value);
+ if (ret)
+ goto fail;
+ memcpy(new_value, value, value_len);
+
+ if (in_inode) {
+ ret = xattr_create_ea_inode(fs, value, value_len, &ea_ino);
+ if (ret)
+ goto fail;
+ }
+
+ if (x->ea_ino) {
+ ret = xattr_inode_dec_ref(fs, x->ea_ino);
+ if (ret)
+ goto fail;
+ }
+
+ if (!x->name)
+ x->name = new_name;
+
+ if (x->value)
+ ext2fs_free_mem(&x->value);
+ x->value = new_value;
+ x->value_len = value_len;
+ x->ea_ino = ea_ino;
+ return 0;
+fail:
+ if (new_name)
+ ext2fs_free_mem(&new_name);
+ if (new_value)
+ ext2fs_free_mem(&new_value);
+ if (ea_ino)
+ xattr_inode_dec_ref(fs, ea_ino);
+ return ret;
+}
+
+static int xattr_find_position(struct ext2_xattr *attrs, int count, const char *name)
+{
+ struct ext2_xattr *x;
+ int i;
+ char *shortname, *x_shortname;
+ int name_idx, x_name_idx;
+ int shortname_len, x_shortname_len;
+
+ find_ea_index(name, &shortname, &name_idx);
+ shortname_len = strlen(shortname);
+
+ for (i = 0, x = attrs; i < count; i++, x++) {
+ find_ea_index(x->name, &x_shortname, &x_name_idx);
+ if (name_idx < x_name_idx)
+ break;
+ if (name_idx > x_name_idx)
+ continue;
+
+ x_shortname_len = strlen(x_shortname);
+ if (shortname_len < x_shortname_len)
+ break;
+ if (shortname_len > x_shortname_len)
+ continue;
+
+ if (memcmp(shortname, x_shortname, shortname_len) <= 0)
+ break;
+ }
+ return i;
+}
+
+errcode_t xattr_array_update(struct ext2_xattr_handle *h, const char *name,
+ const void *value, size_t value_len, int ibody_free,
+ int block_free, int old_idx, int in_inode)
+{
+ struct ext2_xattr tmp;
+ int add_to_ibody;
+ int needed;
+ int name_len, name_idx;
+ char *shortname;
+ int new_idx;
+ int ret;
+
+ find_ea_index(name, &shortname, &name_idx);
+ name_len = strlen(shortname);
+
+ needed = EXT2_EXT_ATTR_LEN(name_len);
+ if (!in_inode)
+ needed += EXT2_EXT_ATTR_SIZE(value_len);
+
+ if (0 <= old_idx && old_idx < h->ibody_count) {
+ ibody_free += EXT2_EXT_ATTR_LEN(name_len);
+ if (!h->attrs[old_idx].ea_ino)
+ ibody_free += EXT2_EXT_ATTR_SIZE(
+ h->attrs[old_idx].value_len);
+ }
+
+ if (needed <= ibody_free) {
+ if (0 <= old_idx) {
+ /* Update the existing entry. */
+ ret = xattr_update_entry(h->fs, &h->attrs[old_idx],
+ name, value, value_len,
+ in_inode);
+ if (ret)
+ return ret;
+ if (h->ibody_count <= old_idx) {
+ /* Move entry from block to the end of ibody. */
+ tmp = h->attrs[old_idx];
+ memmove(h->attrs + h->ibody_count + 1,
+ h->attrs + h->ibody_count,
+ (old_idx - h->ibody_count)
+ * sizeof(*h->attrs));
+ h->attrs[h->ibody_count] = tmp;
+ h->ibody_count++;
+ }
+ return 0;
+ } else {
+ new_idx = h->ibody_count;
+ add_to_ibody = 1;
+ goto add_new;
+ }
+ }
+
+ if (h->ibody_count <= old_idx) {
+ block_free += EXT2_EXT_ATTR_LEN(name_len);
+ if (!h->attrs[old_idx].ea_ino)
+ block_free += EXT2_EXT_ATTR_SIZE(
+ h->attrs[old_idx].value_len);
+ }
+
+ if (needed <= block_free) {
+ if (0 <= old_idx) {
+ /* Update the existing entry. */
+ ret = xattr_update_entry(h->fs, &h->attrs[old_idx],
+ name, value, value_len,
+ in_inode);
+ if (ret)
+ return ret;
+ if (old_idx < h->ibody_count) {
+ /*
+ * Move entry from ibody to the block. Note that
+ * entries in the block are sorted.
+ */
+ new_idx = xattr_find_position(
+ h->attrs + h->ibody_count,
+ h->count - h->ibody_count,
+ name);
+ new_idx += h->ibody_count - 1;
+ tmp = h->attrs[old_idx];
+ memmove(h->attrs + old_idx,
+ h->attrs + old_idx + 1,
+ (new_idx - old_idx)*sizeof(*h->attrs));
+ h->attrs[new_idx] = tmp;
+ h->ibody_count--;
+ }
+ return 0;
+ } else {
+ new_idx = xattr_find_position(h->attrs + h->ibody_count,
+ h->count - h->ibody_count,
+ name);
+ new_idx += h->ibody_count;
+ add_to_ibody = 0;
+ goto add_new;
+ }
+ }
+
+ return EXT2_ET_EA_NO_SPACE;
+
+add_new:
+ if (h->count == h->capacity) {
+ ret = ext2fs_xattrs_expand(h, 4);
+ if (ret)
+ return ret;
+ }
+
+ ret = xattr_update_entry(h->fs, &h->attrs[h->count], name, value,
+ value_len, in_inode);
+ if (ret)
+ return ret;
+
+ tmp = h->attrs[h->count];
+ memmove(h->attrs + new_idx + 1, h->attrs + new_idx,
+ (h->count - new_idx)*sizeof(*h->attrs));
+ h->attrs[new_idx] = tmp;
+ if (add_to_ibody)
+ h->ibody_count++;
+ h->count++;
+ return 0;
+}
+
+int space_used(struct ext2_xattr *attrs, int count)
+{
+ int total = 0;
+ struct ext2_xattr *x;
+ char *shortname;
+ int i, len, name_idx;
+
+ for (i = 0, x = attrs; i < count; i++, x++) {
+ find_ea_index(x->name, &shortname, &name_idx);
+ len = strlen(shortname);
+ total += EXT2_EXT_ATTR_LEN(len);
+ if (!x->ea_ino)
+ total += EXT2_EXT_ATTR_SIZE(x->value_len);
+ }
+ return total;
+}
+
+/*
+ * The minimum size of EA value when you start storing it in an external inode
+ * size of block - size of header - size of 1 entry - 4 null bytes
+*/
+#define EXT4_XATTR_MIN_LARGE_EA_SIZE(b) \
+ ((b) - EXT2_EXT_ATTR_LEN(3) - sizeof(struct ext2_ext_attr_header) - 4)
+
+errcode_t ext2fs_xattr_set(struct ext2_xattr_handle *h,
+ const char *name,
const void *value,
size_t value_len)
{
+ ext2_filsys fs = h->fs;
+ const int inode_size = EXT2_INODE_SIZE(fs->super);
+ struct ext2_inode_large *inode = NULL;
struct ext2_xattr *x;
char *new_value;
- errcode_t err;
+ int ibody_free, block_free;
+ int in_inode = 0;
+ int old_idx = -1;
+ int extra_isize;
+ errcode_t ret;

- EXT2_CHECK_MAGIC(handle, EXT2_ET_MAGIC_EA_HANDLE);
+ EXT2_CHECK_MAGIC(h, EXT2_ET_MAGIC_EA_HANDLE);

- err = ext2fs_get_mem(value_len, &new_value);
- if (err)
- return err;
- if (!(handle->flags & XATTR_HANDLE_FLAG_RAW) &&
- ((strcmp(key, "system.posix_acl_default") == 0) ||
- (strcmp(key, "system.posix_acl_access") == 0))) {
- err = convert_posix_acl_to_disk_buffer(value, value_len,
+ ret = ext2fs_get_mem(value_len, &new_value);
+ if (ret)
+ return ret;
+ if (!(h->flags & XATTR_HANDLE_FLAG_RAW) &&
+ ((strcmp(name, "system.posix_acl_default") == 0) ||
+ (strcmp(name, "system.posix_acl_access") == 0))) {
+ ret = convert_posix_acl_to_disk_buffer(value, value_len,
new_value, &value_len);
- if (err)
- goto errout;
+ if (ret)
+ goto out;
} else
memcpy(new_value, value, value_len);

- for (x = handle->attrs; x < handle->attrs + handle->count; x++) {
- /* Replace xattr */
- if (strcmp(x->name, key) == 0) {
- ext2fs_free_mem(&x->value);
- x->value = new_value;
- x->value_len = value_len;
- handle->dirty = 1;
- return 0;
+ /* Imitate kernel behavior by skipping update if value is the same. */
+ for (x = h->attrs; x < h->attrs + h->count; x++) {
+ if (!strcmp(x->name, name)) {
+ if (!x->ea_ino && x->value_len == value_len &&
+ !memcmp(x->value, new_value, value_len)) {
+ ret = 0;
+ goto out;
+ }
+ old_idx = x - h->attrs;
+ break;
}
}

- if (handle->count == handle->capacity) {
- /* Expand array, append slot */
- err = ext2fs_xattrs_expand(handle, 4);
- if (err)
- goto errout;
+ ret = ext2fs_get_memzero(inode_size, &inode);
+ if (ret)
+ goto out;
+ ret = ext2fs_read_inode_full(fs, h->ino,
+ (struct ext2_inode *)inode,
+ inode_size);
+ if (ret)
+ goto out;
+ if (inode_size > EXT2_GOOD_OLD_INODE_SIZE) {
+ extra_isize = inode->i_extra_isize;
+ if (extra_isize == 0) {
+ extra_isize = fs->super->s_want_extra_isize;
+ if (extra_isize == 0)
+ extra_isize = sizeof(__u32);
+ }
+ ibody_free = inode_size - EXT2_GOOD_OLD_INODE_SIZE;
+ ibody_free -= extra_isize;
+ /* Extended attribute magic and final null entry. */
+ ibody_free -= sizeof(__u32) * 2;
+ ibody_free -= space_used(h->attrs, h->ibody_count);
+ } else
+ ibody_free = 0;

- x = handle->attrs + handle->capacity - 4;
+ /* Inline data can only go to ibody. */
+ if (strcmp(name, "system.data") == 0) {
+ if (h->ibody_count <= old_idx) {
+ ret = EXT2_ET_FILESYSTEM_CORRUPTED;
+ goto out;
+ }
+ ret = xattr_array_update(h, name, value, value_len, ibody_free,
+ 0 /* block_free */, old_idx,
+ 0 /* in_inode */);
+ if (ret)
+ goto out;
+ goto write_out;
}

- err = ext2fs_get_mem(strlen(key) + 1, &x->name);
- if (err)
- goto errout;
- strcpy(x->name, key);
+ block_free = fs->blocksize;
+ block_free -= sizeof(struct ext2_ext_attr_header);
+ /* Final null entry. */
+ block_free -= sizeof(__u32);
+ block_free -= space_used(h->attrs + h->ibody_count,
+ h->count - h->ibody_count);
+
+ if (ext2fs_has_feature_ea_inode(fs->super) &&
+ value_len > EXT4_XATTR_MIN_LARGE_EA_SIZE(fs->blocksize))
+ in_inode = 1;
+
+ ret = xattr_array_update(h, name, value, value_len, ibody_free,
+ block_free, old_idx, in_inode);
+ if (ret == EXT2_ET_EA_NO_SPACE && !in_inode &&
+ ext2fs_has_feature_ea_inode(fs->super))
+ ret = xattr_array_update(h, name, value, value_len, ibody_free,
+ block_free, old_idx, 1 /* in_inode */);
+ if (ret)
+ goto out;

- err = ext2fs_get_mem(value_len, &x->value);
- if (err)
- goto errout;
- memcpy(x->value, value, value_len);
- x->value_len = value_len;
- handle->dirty = 1;
- handle->count++;
- return 0;
-errout:
+write_out:
+ ret = ext2fs_xattrs_write(h);
+out:
+ if (inode)
+ ext2fs_free_mem(&inode);
ext2fs_free_mem(&new_value);
- return err;
+ return ret;
}

errcode_t ext2fs_xattr_remove(struct ext2_xattr_handle *handle,
@@ -1310,11 +1645,14 @@ errcode_t ext2fs_xattr_remove(struct ext2_xattr_handle *handle,
if (strcmp(x->name, key) == 0) {
ext2fs_free_mem(&x->name);
ext2fs_free_mem(&x->value);
- memmove(x, x + 1, (char *)end - (char *)(x + 1));
- memset(end, 0, sizeof(*end));
- handle->dirty = 1;
+ if (x->ea_ino)
+ xattr_inode_dec_ref(handle->fs, x->ea_ino);
+ memmove(x, x + 1, (end - x - 1)*sizeof(*x));
+ memset(end - 1, 0, sizeof(*end));
+ if (x < handle->attrs + handle->ibody_count)
+ handle->ibody_count--;
handle->count--;
- return 0;
+ return ext2fs_xattrs_write(handle);
}
}

@@ -1354,15 +1692,8 @@ errcode_t ext2fs_xattrs_open(ext2_filsys fs, ext2_ino_t ino,
errcode_t ext2fs_xattrs_close(struct ext2_xattr_handle **handle)
{
struct ext2_xattr_handle *h = *handle;
- errcode_t err;

EXT2_CHECK_MAGIC(h, EXT2_ET_MAGIC_EA_HANDLE);
- if (h->dirty) {
- err = ext2fs_xattrs_write(h);
- if (err)
- return err;
- }
-
xattrs_free_keys(h);
ext2fs_free_mem(&h->attrs);
ext2fs_free_mem(handle);
diff --git a/lib/ext2fs/inline_data.c b/lib/ext2fs/inline_data.c
index c0fc4ed1d6c7..0c1eef6c73d2 100644
--- a/lib/ext2fs/inline_data.c
+++ b/lib/ext2fs/inline_data.c
@@ -42,11 +42,6 @@ static errcode_t ext2fs_inline_data_ea_set(struct ext2_inline_data *data)

retval = ext2fs_xattr_set(handle, "system.data",
data->ea_data, data->ea_size);
- if (retval)
- goto err;
-
- retval = ext2fs_xattrs_write(handle);
-
err:
(void) ext2fs_xattrs_close(&handle);
return retval;
@@ -270,11 +265,6 @@ errcode_t ext2fs_inline_data_ea_remove(ext2_filsys fs, ext2_ino_t ino)
goto err;

retval = ext2fs_xattr_remove(handle, "system.data");
- if (retval)
- goto err;
-
- retval = ext2fs_xattrs_write(handle);
-
err:
(void) ext2fs_xattrs_close(&handle);
return retval;
diff --git a/misc/fuse2fs.c b/misc/fuse2fs.c
index 27c94a62a18d..9435e428c823 100644
--- a/misc/fuse2fs.c
+++ b/misc/fuse2fs.c
@@ -2653,12 +2653,6 @@ static int op_setxattr(const char *path EXT2FS_ATTR((unused)),
goto out3;
}

- err = ext2fs_xattrs_write(h);
- if (err) {
- ret = translate_error(fs, ino, err);
- goto out3;
- }
-
ret = update_ctime(fs, ino, NULL);
out3:
if (cvalue != value)
@@ -2725,12 +2719,6 @@ static int op_removexattr(const char *path, const char *key)
goto out2;
}

- err = ext2fs_xattrs_write(h);
- if (err) {
- ret = translate_error(fs, ino, err);
- goto out2;
- }

2017-07-07 12:12:55

by Tahsin Erdogan

[permalink] [raw]
Subject: [PATCH 3/4] libext2fs: eliminate empty element holes in ext2_xattr_handle->attrs

When an extended attribute is removed, its array element is emptied.
This creates holes in the array so various places that want to walk
filled elements have to do an empty element check.

Have remove operation shift remaining filled elements to the left.
This allows a simple iteration up to ext2_xattr_handle->count to walk
all filled entries, and so empty element checks become unnecessary.

Signed-off-by: Tahsin Erdogan <[email protected]>
---
lib/ext2fs/ext_attr.c | 93 ++++++++++++++++-----------------------------------
1 file changed, 29 insertions(+), 64 deletions(-)

diff --git a/lib/ext2fs/ext_attr.c b/lib/ext2fs/ext_attr.c
index 2e9fc96d114d..00ff79ae3890 100644
--- a/lib/ext2fs/ext_attr.c
+++ b/lib/ext2fs/ext_attr.c
@@ -335,7 +335,7 @@ static struct ea_name_index ea_names[] = {

static int find_ea_index(char *fullname, char **name, int *index);

-/* Push empty attributes to the end and inlinedata to the front. */
+/* Pull inlinedata to the front. */
static int attr_compare(const void *a, const void *b)
{
const struct ext2_xattr *xa = a, *xb = b;
@@ -343,11 +343,7 @@ static int attr_compare(const void *a, const void *b)
int xa_idx, xb_idx;
int cmp;

- if (xa->name == NULL)
- return +1;
- else if (xb->name == NULL)
- return -1;
- else if (!strcmp(xa->name, "system.data"))
+ if (!strcmp(xa->name, "system.data"))
return -1;
else if (!strcmp(xb->name, "system.data"))
return +1;
@@ -675,10 +671,7 @@ static errcode_t write_xattrs_to_buffer(struct ext2_xattr_handle *handle,

memset(entries_start, 0, storage_size);
/* For all remaining x... */
- for (; x < handle->attrs + handle->capacity; x++) {
- if (!x->name)
- continue;
-
+ for (; x < handle->attrs + handle->count; x++) {
/* Calculate index and shortname position */
shortname = x->name;
ret = find_ea_index(x->name, &shortname, &idx);
@@ -766,12 +759,9 @@ errcode_t ext2fs_xattrs_write(struct ext2_xattr_handle *handle)
goto out;
}

- /*
- * Force the inlinedata attr to the front and the empty entries
- * to the end.
- */
+ /* Force the inlinedata attr to the front. */
x = handle->attrs;
- qsort(x, handle->capacity, sizeof(struct ext2_xattr), attr_compare);
+ qsort(x, handle->count, sizeof(struct ext2_xattr), attr_compare);

/* Does the inode have space for EA? */
if (inode->i_extra_isize < sizeof(inode->i_extra_isize) ||
@@ -813,7 +803,7 @@ write_ea_block:
if (err)
goto out2;

- if (x < handle->attrs + handle->capacity) {
+ if (x < handle->attrs + handle->count) {
err = EXT2_ET_EA_NO_SPACE;
goto out2;
}
@@ -865,8 +855,7 @@ static errcode_t read_xattrs_from_buffer(struct ext2_xattr_handle *handle,
struct ext2_inode_large *inode,
struct ext2_ext_attr_entry *entries,
unsigned int storage_size,
- char *value_start,
- size_t *nr_read)
+ char *value_start)
{
struct ext2_xattr *x;
struct ext2_ext_attr_entry *entry, *end;
@@ -876,10 +865,6 @@ static errcode_t read_xattrs_from_buffer(struct ext2_xattr_handle *handle,
unsigned int values_size = storage_size +
((char *)entries - value_start);

- x = handle->attrs;
- while (x->name)
- x++;
-
/* find the end */
end = entries;
remain = storage_size;
@@ -904,13 +889,14 @@ static errcode_t read_xattrs_from_buffer(struct ext2_xattr_handle *handle,
!EXT2_EXT_IS_LAST_ENTRY(entry)) {

/* Allocate space for more attrs? */
- if (x == handle->attrs + handle->capacity) {
+ if (handle->count == handle->capacity) {
err = ext2fs_xattrs_expand(handle, 4);
if (err)
return err;
- x = handle->attrs + handle->capacity - 4;
}

+ x = handle->attrs + handle->count;
+
/* header eats this space */
remain -= sizeof(struct ext2_ext_attr_entry);

@@ -1013,8 +999,7 @@ static errcode_t read_xattrs_from_buffer(struct ext2_xattr_handle *handle,
}
}

- x++;
- (*nr_read)++;
+ handle->count++;
entry = EXT2_EXT_ATTR_NEXT(entry);
}

@@ -1084,8 +1069,8 @@ errcode_t ext2fs_xattrs_read(struct ext2_xattr_handle *handle)
inode->i_extra_isize + sizeof(__u32);

err = read_xattrs_from_buffer(handle, inode,
- (struct ext2_ext_attr_entry *) start, storage_size,
- start, &handle->count);
+ (struct ext2_ext_attr_entry *) start,
+ storage_size, start);
if (err)
goto out;
}
@@ -1121,8 +1106,8 @@ read_ea_block:
sizeof(struct ext2_ext_attr_header);
start = block_buf + sizeof(struct ext2_ext_attr_header);
err = read_xattrs_from_buffer(handle, inode,
- (struct ext2_ext_attr_entry *) start, storage_size,
- block_buf, &handle->count);
+ (struct ext2_ext_attr_entry *) start,
+ storage_size, block_buf);
if (err)
goto out3;

@@ -1149,10 +1134,7 @@ errcode_t ext2fs_xattrs_iterate(struct ext2_xattr_handle *h,
int ret;

EXT2_CHECK_MAGIC(h, EXT2_ET_MAGIC_EA_HANDLE);
- for (x = h->attrs; x < h->attrs + h->capacity; x++) {
- if (!x->name)
- continue;
-
+ for (x = h->attrs; x < h->attrs + h->count; x++) {
ret = func(x->name, x->value, x->value_len, data);
if (ret & XATTR_CHANGED)
h->dirty = 1;
@@ -1171,8 +1153,8 @@ errcode_t ext2fs_xattr_get(struct ext2_xattr_handle *h, const char *key,
errcode_t err;

EXT2_CHECK_MAGIC(h, EXT2_ET_MAGIC_EA_HANDLE);
- for (x = h->attrs; x < h->attrs + h->capacity; x++) {
- if (!x->name || strcmp(x->name, key))
+ for (x = h->attrs; x < h->attrs + h->count; x++) {
+ if (strcmp(x->name, key))
continue;

if (!(h->flags & XATTR_HANDLE_FLAG_RAW) &&
@@ -1260,12 +1242,11 @@ errcode_t ext2fs_xattr_set(struct ext2_xattr_handle *handle,
const void *value,
size_t value_len)
{
- struct ext2_xattr *x, *last_empty;
+ struct ext2_xattr *x;
char *new_value;
errcode_t err;

EXT2_CHECK_MAGIC(handle, EXT2_ET_MAGIC_EA_HANDLE);
- last_empty = NULL;

err = ext2fs_get_mem(value_len, &new_value);
if (err)
@@ -1280,12 +1261,7 @@ errcode_t ext2fs_xattr_set(struct ext2_xattr_handle *handle,
} else
memcpy(new_value, value, value_len);

- for (x = handle->attrs; x < handle->attrs + handle->capacity; x++) {
- if (!x->name) {
- last_empty = x;
- continue;
- }
-
+ for (x = handle->attrs; x < handle->attrs + handle->count; x++) {
/* Replace xattr */
if (strcmp(x->name, key) == 0) {
ext2fs_free_mem(&x->value);
@@ -1296,25 +1272,15 @@ errcode_t ext2fs_xattr_set(struct ext2_xattr_handle *handle,
}
}

- /* Add attr to empty slot */
- if (last_empty) {
- err = ext2fs_get_mem(strlen(key) + 1, &last_empty->name);
+ if (handle->count == handle->capacity) {
+ /* Expand array, append slot */
+ err = ext2fs_xattrs_expand(handle, 4);
if (err)
goto errout;
- strcpy(last_empty->name, key);
- last_empty->value = new_value;
- last_empty->value_len = value_len;
- handle->dirty = 1;
- handle->count++;
- return 0;
- }

- /* Expand array, append slot */
- err = ext2fs_xattrs_expand(handle, 4);
- if (err)
- goto errout;
+ x = handle->attrs + handle->capacity - 4;
+ }

- x = handle->attrs + handle->capacity - 4;
err = ext2fs_get_mem(strlen(key) + 1, &x->name);
if (err)
goto errout;
@@ -1337,16 +1303,15 @@ errcode_t ext2fs_xattr_remove(struct ext2_xattr_handle *handle,
const char *key)
{
struct ext2_xattr *x;
+ struct ext2_xattr *end = handle->attrs + handle->count;

EXT2_CHECK_MAGIC(handle, EXT2_ET_MAGIC_EA_HANDLE);
- for (x = handle->attrs; x < handle->attrs + handle->capacity; x++) {
- if (!x->name)
- continue;

2017-07-24 01:25:52

by Theodore Ts'o

[permalink] [raw]
Subject: Re: [PATCH 1/4] e2fsck: update quota inode accounting for ea_inode feature

On Fri, Jul 07, 2017 at 05:12:43AM -0700, Tahsin Erdogan wrote:
> Extended attribute inodes are charged to all referencing inodes.
> Update e2fsck so that it can correctly track inode quota charges.
>
> Signed-off-by: Tahsin Erdogan <[email protected]>

Thanks, applied.

- Ted

2017-07-24 02:46:38

by Theodore Ts'o

[permalink] [raw]
Subject: Re: [PATCH 2/4] libext2fs: rename ext2_xattr_handle->length to capacity

On Fri, Jul 07, 2017 at 05:12:44AM -0700, Tahsin Erdogan wrote:
> ext2_xattr_handle has two fields 'count' and 'length' which
> represent number of filled elements vs total element count.
> They have close meanings so are easy to confuse, thus make code less
> readable. Rename length to capacity.
>
> Signed-off-by: Tahsin Erdogan <[email protected]>

Applied, thanks.

- Ted

2017-07-24 02:53:53

by Theodore Ts'o

[permalink] [raw]
Subject: Re: [PATCH 3/4] libext2fs: eliminate empty element holes in ext2_xattr_handle->attrs

On Fri, Jul 07, 2017 at 05:12:45AM -0700, Tahsin Erdogan wrote:
> When an extended attribute is removed, its array element is emptied.
> This creates holes in the array so various places that want to walk
> filled elements have to do an empty element check.
>
> Have remove operation shift remaining filled elements to the left.
> This allows a simple iteration up to ext2_xattr_handle->count to walk
> all filled entries, and so empty element checks become unnecessary.
>
> Signed-off-by: Tahsin Erdogan <[email protected]>

Thanks, applied.

- Ted

2017-07-24 03:30:52

by Theodore Ts'o

[permalink] [raw]
Subject: Re: [PATCH 4/4] libext2fs: add ea_inode support to set xattr

Applied, with some style cleanups. See below for the interdiff.

The cleanups are mostly checkpatch style warnings, plus simplifying
complexity by reducing indentation levels. Yes, it makes the code
somewhat less parallel, but on the whole I think it makes the code
more readable.

- Ted

lib/ext2fs/ext_attr.c | 112 +++++++++++++++++++++------------------------
1 file changed, 52 insertions(+), 60 deletions(-)

diff --git a/lib/ext2fs/ext_attr.c b/lib/ext2fs/ext_attr.c
index 7cecb1688..f4cc2d056 100644
--- a/lib/ext2fs/ext_attr.c
+++ b/lib/ext2fs/ext_attr.c
@@ -1334,7 +1334,7 @@ static errcode_t xattr_update_entry(ext2_filsys fs, struct ext2_xattr *x,

if (!x->name)
x->name = new_name;
-
+
if (x->value)
ext2fs_free_mem(&x->value);
x->value = new_value;
@@ -1351,7 +1351,8 @@ fail:
return ret;
}

-static int xattr_find_position(struct ext2_xattr *attrs, int count, const char *name)
+static int xattr_find_position(struct ext2_xattr *attrs, int count,
+ const char *name)
{
struct ext2_xattr *x;
int i;
@@ -1400,7 +1401,7 @@ errcode_t xattr_array_update(struct ext2_xattr_handle *h, const char *name,
if (!in_inode)
needed += EXT2_EXT_ATTR_SIZE(value_len);

- if (0 <= old_idx && old_idx < h->ibody_count) {
+ if (old_idx >= 0 && old_idx < h->ibody_count) {
ibody_free += EXT2_EXT_ATTR_LEN(name_len);
if (!h->attrs[old_idx].ea_ino)
ibody_free += EXT2_EXT_ATTR_SIZE(
@@ -1408,75 +1409,66 @@ errcode_t xattr_array_update(struct ext2_xattr_handle *h, const char *name,
}

if (needed <= ibody_free) {
- if (0 <= old_idx) {
- /* Update the existing entry. */
- ret = xattr_update_entry(h->fs, &h->attrs[old_idx],
- name, value, value_len,
- in_inode);
- if (ret)
- return ret;
- if (h->ibody_count <= old_idx) {
- /* Move entry from block to the end of ibody. */
- tmp = h->attrs[old_idx];
- memmove(h->attrs + h->ibody_count + 1,
- h->attrs + h->ibody_count,
- (old_idx - h->ibody_count)
- * sizeof(*h->attrs));
- h->attrs[h->ibody_count] = tmp;
- h->ibody_count++;
- }
- return 0;
- } else {
+ if (old_idx < 0) {
new_idx = h->ibody_count;
add_to_ibody = 1;
goto add_new;
}
+
+ /* Update the existing entry. */
+ ret = xattr_update_entry(h->fs, &h->attrs[old_idx], name,
+ value, value_len, in_inode);
+ if (ret)
+ return ret;
+ if (h->ibody_count <= old_idx) {
+ /* Move entry from block to the end of ibody. */
+ tmp = h->attrs[old_idx];
+ memmove(h->attrs + h->ibody_count + 1,
+ h->attrs + h->ibody_count,
+ (old_idx - h->ibody_count) * sizeof(*h->attrs));
+ h->attrs[h->ibody_count] = tmp;
+ h->ibody_count++;
+ }
+ return 0;
}

if (h->ibody_count <= old_idx) {
block_free += EXT2_EXT_ATTR_LEN(name_len);
if (!h->attrs[old_idx].ea_ino)
- block_free += EXT2_EXT_ATTR_SIZE(
- h->attrs[old_idx].value_len);
+ block_free +=
+ EXT2_EXT_ATTR_SIZE(h->attrs[old_idx].value_len);
}

- if (needed <= block_free) {
- if (0 <= old_idx) {
- /* Update the existing entry. */
- ret = xattr_update_entry(h->fs, &h->attrs[old_idx],
- name, value, value_len,
- in_inode);
- if (ret)
- return ret;
- if (old_idx < h->ibody_count) {
- /*
- * Move entry from ibody to the block. Note that
- * entries in the block are sorted.
- */
- new_idx = xattr_find_position(
- h->attrs + h->ibody_count,
- h->count - h->ibody_count,
- name);
- new_idx += h->ibody_count - 1;
- tmp = h->attrs[old_idx];
- memmove(h->attrs + old_idx,
- h->attrs + old_idx + 1,
- (new_idx - old_idx)*sizeof(*h->attrs));
- h->attrs[new_idx] = tmp;
- h->ibody_count--;
- }
- return 0;
- } else {
+ if (needed > block_free)
+ return EXT2_ET_EA_NO_SPACE;
+
+ if (old_idx >= 0) {
+ /* Update the existing entry. */
+ ret = xattr_update_entry(h->fs, &h->attrs[old_idx], name,
+ value, value_len, in_inode);
+ if (ret)
+ return ret;
+ if (old_idx < h->ibody_count) {
+ /*
+ * Move entry from ibody to the block. Note that
+ * entries in the block are sorted.
+ */
new_idx = xattr_find_position(h->attrs + h->ibody_count,
- h->count - h->ibody_count,
- name);
- new_idx += h->ibody_count;
- add_to_ibody = 0;
- goto add_new;
+ h->count - h->ibody_count, name);
+ new_idx += h->ibody_count - 1;
+ tmp = h->attrs[old_idx];
+ memmove(h->attrs + old_idx, h->attrs + old_idx + 1,
+ (new_idx - old_idx) * sizeof(*h->attrs));
+ h->attrs[new_idx] = tmp;
+ h->ibody_count--;
}
+ return 0;
}

- return EXT2_ET_EA_NO_SPACE;
+ new_idx = xattr_find_position(h->attrs + h->ibody_count,
+ h->count - h->ibody_count, name);
+ new_idx += h->ibody_count;
+ add_to_ibody = 0;

add_new:
if (h->count == h->capacity) {
@@ -1520,7 +1512,7 @@ int space_used(struct ext2_xattr *attrs, int count)
/*
* The minimum size of EA value when you start storing it in an external inode
* size of block - size of header - size of 1 entry - 4 null bytes
-*/
+ */
#define EXT4_XATTR_MIN_LARGE_EA_SIZE(b) \
((b) - EXT2_EXT_ATTR_LEN(3) - sizeof(struct ext2_ext_attr_header) - 4)

@@ -1579,8 +1571,8 @@ errcode_t ext2fs_xattr_set(struct ext2_xattr_handle *h,
if (inode_size > EXT2_GOOD_OLD_INODE_SIZE) {
extra_isize = inode->i_extra_isize;
if (extra_isize == 0) {
- extra_isize = fs->super->s_want_extra_isize;
- if (extra_isize == 0)
+ extra_isize = fs->super->s_want_extra_isize;
+ if (extra_isize == 0)
extra_isize = sizeof(__u32);
}
ibody_free = inode_size - EXT2_GOOD_OLD_INODE_SIZE;