On Jun 26, 2017, at 7:43 AM, Tahsin Erdogan <tahsin@google.com> wrote:
> 
> With ea_inode feature, i_blocks include the disk space used by
> referenced xattr inodes. Make e2fsck aware of that.

This makes it all the more important that the "fast symlink" checking be
divorced from i_blocks, and instead depend on i_size.  Otherwise, we will
have yet another bunch of problems as large xattrs on a fast symlink will
incorrectly result in problems with the symlink.

This was most recently discussed in the thread:
[PATCH] libext2fs: correctly subtract xattr blocks on bigalloc filesystems
https://www.spinics.net/lists/linux-ext4/msg56601.html

All indications are that using i_size to distinguish fast symlinks is safe,
while using i_blocks has repeatedly caused breakage as new blocks are
allocated to the inode.

Cheers, Andreas

> Signed-off-by: Tahsin Erdogan <tahsin@google.com>
> ---
> e2fsck/e2fsck.c |   4 ++
> e2fsck/e2fsck.h |   5 +++
> e2fsck/pass1.c  | 124 +++++++++++++++++++++++++++++++++++++++++---------------
> 3 files changed, 100 insertions(+), 33 deletions(-)
> 
> diff --git a/e2fsck/e2fsck.c b/e2fsck/e2fsck.c
> index 0f9da46a5a12..e0f449ee2047 100644
> --- a/e2fsck/e2fsck.c
> +++ b/e2fsck/e2fsck.c
> @@ -98,6 +98,10 @@ 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->block_dup_map) {
> 		ext2fs_free_block_bitmap(ctx->block_dup_map);
> 		ctx->block_dup_map = 0;
> diff --git a/e2fsck/e2fsck.h b/e2fsck/e2fsck.h
> index f9285d01978c..79eeda9fbafb 100644
> --- a/e2fsck/e2fsck.h
> +++ b/e2fsck/e2fsck.h
> @@ -269,6 +269,11 @@ struct e2fsck_struct {
> 	ext2_refcount_t	refcount;
> 	ext2_refcount_t refcount_extra;
> 
> +	/*
> +	 * Quota blocks to be charged for each ea block.
> +	 */
> +	ext2_refcount_t ea_block_quota;
> +
> 	/*
> 	 * Array of flags indicating whether an inode bitmap, block
> 	 * bitmap, or inode table is invalid
> diff --git a/e2fsck/pass1.c b/e2fsck/pass1.c
> index 9ee2c5e89a61..cc88a7d05cef 100644
> --- a/e2fsck/pass1.c
> +++ b/e2fsck/pass1.c
> @@ -66,7 +66,7 @@ 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);
> +			 char *block_buf, blk64_t ea_ibody_quota_blocks);
> 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,6 +103,7 @@ struct process_block_struct {
> 
> struct process_inode_block {
> 	ext2_ino_t ino;
> +	blk64_t ea_ibody_quota_blocks;
> 	struct ext2_inode_large inode;
> };
> 
> @@ -351,13 +352,25 @@ static void mark_inode_ea_map(e2fsck_t ctx, struct problem_context *pctx,
> 	ext2fs_mark_inode_bitmap2(ctx->inode_ea_map, ino);
> }
> 
> +/*
> + * For a given size, calculate how many blocks would be charged towards quota.
> + */
> +static blk64_t size_to_quota_blocks(ext2_filsys fs, size_t size)
> +{
> +	blk64_t clusters;
> +
> +	clusters = DIV_ROUND_UP(size, fs->blocksize << fs->cluster_ratio_bits);
> +	return EXT2FS_C2B(fs, clusters);
> +}
> +
> /*
>  * Check validity of EA inode. Return 0 if EA inode is valid, otherwise return
>  * the problem code.
>  */
> static problem_t check_large_ea_inode(e2fsck_t ctx,
> 				      struct ext2_ext_attr_entry *entry,
> -				      struct problem_context *pctx)
> +				      struct problem_context *pctx,
> +				      blk64_t *quota_blocks)
> {
> 	struct ext2_inode inode;
> 	__u32 hash;
> @@ -380,11 +393,15 @@ static problem_t check_large_ea_inode(e2fsck_t ctx,
> 		fatal_error(ctx, 0);
> 	}
> 
> -	if (hash != entry->e_hash) {
> +	if (hash == entry->e_hash) {
> +		*quota_blocks = size_to_quota_blocks(ctx->fs,
> +						     entry->e_value_size);
> +	} else {
> 		/* This might be an old Lustre-style ea_inode reference. */
> -		if (inode.i_mtime != pctx->ino ||
> -		    inode.i_generation != pctx->inode->i_generation) {
> -
> +		if (inode.i_mtime == pctx->ino &&
> +		    inode.i_generation == pctx->inode->i_generation) {
> +			*quota_blocks = 0;
> +		} else {
> 			/* If target inode is also missing EA_INODE flag,
> 			 * this is likely to be a bad reference.
> 			 */
> @@ -411,7 +428,8 @@ static problem_t check_large_ea_inode(e2fsck_t ctx,
> 	return 0;
> }
> 
> -static void check_ea_in_inode(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 ext2_super_block *sb = ctx->fs->super;
> 	struct ext2_inode_large *inode;
> @@ -420,6 +438,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;
> 
> 	inode = (struct ext2_inode_large *) pctx->inode;
> 	storage_size = EXT2_INODE_SIZE(ctx->fs->super) - EXT2_GOOD_OLD_INODE_SIZE -
> @@ -496,11 +517,15 @@ static void check_ea_in_inode(e2fsck_t ctx, struct problem_context *pctx)
> 				goto fix;
> 			}
> 		} else {
> -			problem = check_large_ea_inode(ctx, entry, pctx);
> +			blk64_t entry_quota_blocks;
> +
> +			problem = check_large_ea_inode(ctx, entry, pctx,
> +						       &entry_quota_blocks);
> 			if (problem != 0)
> 				goto fix;
> 
> 			mark_inode_ea_map(ctx, pctx, entry->e_value_inum);
> +			quota_blocks += entry_quota_blocks;
> 		}
> 
> 		/* If EA value is stored in external inode then it does not
> @@ -523,8 +548,10 @@ fix:
> 	 * it seems like a corruption. it's very unlikely we could repair
> 	 * EA(s) in automatic fashion -bzzz
> 	 */
> -	if (problem == 0 || !fix_problem(ctx, problem, pctx))
> +	if (problem == 0 || !fix_problem(ctx, problem, pctx)) {
> +		*ea_ibody_quota_blocks = quota_blocks;
> 		return;
> +	}
> 
> 	/* simply remove all possible EA(s) */
> 	*((__u32 *)header) = 0UL;
> @@ -547,13 +574,16 @@ 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)
> +static void check_inode_extra_space(e2fsck_t ctx, struct problem_context *pctx,
> +				    blk64_t *ea_ibody_quota_blocks)
> {
> 	struct ext2_super_block *sb = ctx->fs->super;
> 	struct ext2_inode_large *inode;
> 	__u32 *eamagic;
> 	int min, max;
> 
> +	*ea_ibody_quota_blocks = 0;
> +
> 	inode = (struct ext2_inode_large *) pctx->inode;
> 	if (EXT2_INODE_SIZE(sb) == EXT2_GOOD_OLD_INODE_SIZE) {
> 		/* this isn't large inode. so, nothing to check */
> @@ -592,7 +622,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);
> +		check_ea_in_inode(ctx, pctx, ea_ibody_quota_blocks);
> 	}
> 
> 	/*
> @@ -1150,6 +1180,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;
> 
> 	init_resource_track(&rtrack, ctx->fs->io);
> 	clear_problem_context(&pctx);
> @@ -1695,7 +1726,7 @@ void e2fsck_pass1(e2fsck_t ctx)
> 							   "pass1");
> 					failed_csum = 0;
> 				}
> -				check_blocks(ctx, &pctx, block_buf);
> +				check_blocks(ctx, &pctx, block_buf, 0);
> 				FINISH_INODE_LOOP(ctx, ino, &pctx, failed_csum);
> 				continue;
> 			}
> @@ -1722,7 +1753,7 @@ void e2fsck_pass1(e2fsck_t ctx)
> 							"pass1");
> 					failed_csum = 0;
> 				}
> -				check_blocks(ctx, &pctx, block_buf);
> +				check_blocks(ctx, &pctx, block_buf, 0);
> 				FINISH_INODE_LOOP(ctx, ino, &pctx, failed_csum);
> 				continue;
> 			}
> @@ -1760,7 +1791,7 @@ void e2fsck_pass1(e2fsck_t ctx)
> 					failed_csum = 0;
> 				}
> 			}
> -			check_blocks(ctx, &pctx, block_buf);
> +			check_blocks(ctx, &pctx, block_buf, 0);
> 			FINISH_INODE_LOOP(ctx, ino, &pctx, failed_csum);
> 			continue;
> 		}
> @@ -1825,7 +1856,7 @@ void e2fsck_pass1(e2fsck_t ctx)
> 			}
> 		}
> 
> -		check_inode_extra_space(ctx, &pctx);
> +		check_inode_extra_space(ctx, &pctx, &ea_ibody_quota_blocks);
> 		check_is_really_dir(ctx, &pctx, block_buf);
> 
> 		/*
> @@ -1872,7 +1903,8 @@ void e2fsck_pass1(e2fsck_t ctx)
> 				continue;
> 			} else if (ext2fs_inode_data_blocks(fs, inode) == 0) {
> 				ctx->fs_fast_symlinks_count++;
> -				check_blocks(ctx, &pctx, block_buf);
> +				check_blocks(ctx, &pctx, block_buf,
> +					     ea_ibody_quota_blocks);
> 				FINISH_INODE_LOOP(ctx, ino, &pctx, failed_csum);
> 				continue;
> 			}
> @@ -1906,17 +1938,19 @@ void e2fsck_pass1(e2fsck_t ctx)
> 		     inode->i_block[EXT2_DIND_BLOCK] ||
> 		     inode->i_block[EXT2_TIND_BLOCK] ||
> 		     ext2fs_file_acl_block(fs, inode))) {
> -			struct ext2_inode_large *ip;
> +			struct process_inode_block *itp;
> 
> -			inodes_to_process[process_inode_count].ino = ino;
> -			ip = &inodes_to_process[process_inode_count].inode;
> +			itp = &inodes_to_process[process_inode_count];
> +			itp->ino = ino;
> +			itp->ea_ibody_quota_blocks = ea_ibody_quota_blocks;
> 			if (inode_size < sizeof(struct ext2_inode_large))
> -				memcpy(ip, inode, inode_size);
> +				memcpy(&itp->inode, inode, inode_size);
> 			else
> -				memcpy(ip, inode, sizeof(*ip));
> +				memcpy(&itp->inode, inode, sizeof(itp->inode));
> 			process_inode_count++;
> 		} else
> -			check_blocks(ctx, &pctx, block_buf);
> +			check_blocks(ctx, &pctx, block_buf,
> +				     ea_ibody_quota_blocks);
> 
> 		FINISH_INODE_LOOP(ctx, ino, &pctx, failed_csum);
> 
> @@ -2086,7 +2120,8 @@ static void process_inodes(e2fsck_t ctx, char *block_buf)
> 		sprintf(buf, _("reading indirect blocks of inode %u"),
> 			pctx.ino);
> 		ehandler_operation(buf);
> -		check_blocks(ctx, &pctx, block_buf);
> +		check_blocks(ctx, &pctx, block_buf,
> +			     inodes_to_process[i].ea_ibody_quota_blocks);
> 		if (ctx->flags & E2F_FLAG_SIGNAL_MASK)
> 			break;
> 	}
> @@ -2306,7 +2341,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)
> +			   char *block_buf, blk64_t *ea_block_quota_blocks)
> {
> 	ext2_filsys fs = ctx->fs;
> 	ext2_ino_t	ino = pctx->ino;
> @@ -2315,7 +2350,7 @@ static int check_ext_attr(e2fsck_t ctx, struct problem_context *pctx,
> 	char *		end;
> 	struct ext2_ext_attr_header *header;
> 	struct ext2_ext_attr_entry *entry;
> -	int		count;
> +	blk64_t		quota_blocks = EXT2FS_C2B(fs, 1);
> 	region_t	region = 0;
> 	int		failed_csum = 0;
> 
> @@ -2369,6 +2404,12 @@ 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;
> +
> 		if (ea_refcount_decrement(ctx->refcount, blk, 0) == 0)
> 			return 1;
> 		/* Ooops, this EA was referenced more than it stated */
> @@ -2477,13 +2518,17 @@ static int check_ext_attr(e2fsck_t ctx, struct problem_context *pctx,
> 			}
> 		} else {
> 			problem_t problem;
> +			blk64_t entry_quota_blocks;
> 
> -			problem = check_large_ea_inode(ctx, entry, pctx);
> +			problem = check_large_ea_inode(ctx, entry, pctx,
> +						       &entry_quota_blocks);
> 			if (problem == 0)
> 				mark_inode_ea_map(ctx, pctx,
> 						  entry->e_value_inum);
> 			else if (fix_problem(ctx, problem, pctx))
> 				goto clear_extattr;
> +
> +			quota_blocks += entry_quota_blocks;
> 		}
> 
> 		entry = EXT2_EXT_ATTR_NEXT(entry);
> @@ -2506,9 +2551,21 @@ static int check_ext_attr(e2fsck_t ctx, struct problem_context *pctx,
> 			return 0;
> 	}
> 
> -	count = header->h_refcount - 1;
> -	if (count)
> -		ea_refcount_store(ctx->refcount, blk, count);
> +	*ea_block_quota_blocks = quota_blocks;
> +	if (quota_blocks) {
> +		if (!ctx->ea_block_quota) {
> +			pctx->errcode = ea_refcount_create(0,
> +							&ctx->ea_block_quota);
> +			if (pctx->errcode) {
> +				pctx->num = 3;
> +				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->refcount, blk, header->h_refcount - 1);
> 	mark_block_used(ctx, blk);
> 	ext2fs_fast_mark_block_bitmap2(ctx->block_ea_map, blk);
> 	return 1;
> @@ -3162,7 +3219,7 @@ err:
>  * blocks used by that inode.
>  */
> static void check_blocks(e2fsck_t ctx, struct problem_context *pctx,
> -			 char *block_buf)
> +			 char *block_buf, blk64_t ea_ibody_quota_blocks)
> {
> 	ext2_filsys fs = ctx->fs;
> 	struct process_block_struct pb;
> @@ -3173,9 +3230,10 @@ 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;
> 
> 	pb.ino = ino;
> -	pb.num_blocks = 0;
> +	pb.num_blocks = EXT2FS_B2C(ctx->fs, ea_ibody_quota_blocks);
> 	pb.last_block = ~0;
> 	pb.last_init_lblock = -1;
> 	pb.last_db_block = -1;
> @@ -3198,10 +3256,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)) {
> +	if (check_ext_attr(ctx, pctx, block_buf, &ea_block_quota_blocks)) {
> 		if (ctx->flags & E2F_FLAG_SIGNAL_MASK)
> 			goto out;
> -		pb.num_blocks++;
> +		pb.num_blocks += EXT2FS_B2C(ctx->fs, ea_block_quota_blocks);
> 	}
> 
> 	if (inlinedata_fs && (inode->i_flags & EXT4_INLINE_DATA_FL))
> --
> 2.13.1.611.g7e3b11ae1-goog
> 


Cheers, Andreas