Re: ENOSPC returned during writepages

[Mingming Cao <cmm@us.ibm.com>]

在 2008-08-21四的 20:42 +0530，Aneesh Kumar K.V写道：

> This is what i am testing now. It is much better. I am getting few
> errors now. But I guess that may be due to our meta-data block
> reservation going wrong. Still debugging.
> 
> -aneesh
> 
> commit be0a76f17f45b1009c40b2adb7d95f93dfdbb95a
> Author: Aneesh Kumar K.V <aneesh.kumar@linux.vnet.ibm.com>
> Date:   Thu Aug 21 15:56:19 2008 +0530
> 
>    ENOSPC handling 
> 
> diff --git a/fs/ext4/balloc.c b/fs/ext4/balloc.c
> index dfe2d4f..5d0a676 100644
> --- a/fs/ext4/balloc.c
> +++ b/fs/ext4/balloc.c
> @@ -1602,6 +1602,64 @@ ext4_try_to_allocate_with_rsv(struct super_block *sb, handle_t *handle,
>  	return ret;
>  }
> 
> +int ext4_claim_free_blocks(struct ext4_sb_info *sbi,
> +						ext4_fsblk_t nblocks)
> +{
> +	int cpu;
> +	s64 free_blocks;
> +	ext4_fsblk_t root_blocks = 0;
> +	struct percpu_counter *fbc = &sbi->s_freeblocks_counter;
> +
> +	free_blocks = percpu_counter_read(fbc);
> +
> +	if (!capable(CAP_SYS_RESOURCE) &&
> +		sbi->s_resuid != current->fsuid &&
> +		(sbi->s_resgid == 0 || !in_group_p(sbi->s_resgid)))
> +		root_blocks = ext4_r_blocks_count(sbi->s_es);
> +#ifdef CONFIG_SMP
> +	/* Each CPU can accumulate FBC_BATCH blocks in their local
> +	 * counters. So we need to make sure we have free blocks more
> +	 * than FBC_BATCH  * nr_cpu_ids. Also add a window of 4 times.
> +	 */
> +	if (free_blocks - (nblocks + root_blocks) <
> +			(4 * (FBC_BATCH * nr_cpu_ids))) {
> +		/*
> +		 * We need to sum and claim under lock
> +		 * This is the slow patch which will be
> +		 * taken when we are very low on free blocks
> +		 */
> +		spin_lock(&fbc->lock);
> +		free_blocks = fbc->count;
> +		for_each_online_cpu(cpu) {
> +			s32 *pcount = per_cpu_ptr(fbc->counters, cpu);
> +			free_blocks += *pcount;
> +			*pcount = 0;
> +		}
> +		fbc->count = free_blocks;
> +		if (free_blocks <= root_blocks) {
> +			/* we don't have free space */
> +			spin_unlock(&fbc->lock);
> +			return -ENOSPC;
> +		}
> +		if (free_blocks - root_blocks < nblocks) {
> +			spin_unlock(&fbc->lock);
> +			return -ENOSPC;
> +		}
> +		fbc->count -= nblocks;
> +		spin_unlock(&fbc->lock);
> +		return 0;
> +	}
> +#endif
> +	if (free_blocks <= root_blocks)
> +		/* we don't have free space */
> +		return -ENOSPC;
> +	if (free_blocks - root_blocks < nblocks)
> +		return -ENOSPC;
> +	/* reduce fs free blocks counter */
> +	percpu_counter_sub(fbc, nblocks);
> +	return 0;
> +}
> +

Like I proposed earlier, I would prefer create a new percpu counter API
to do the percpu_counter_sum_and_sub(), hide the CONFIG_SMP there,
rather than expose the percpu counter implementation details in ext4 fs
code here.

It also be nice and sane to define

#define  EXT4_FREEBLOCKS_LOW_WATERMARK		4* nr_cpu_ids * nr_cpu_ids


Other than this, patch looks good.


Mingming
>  /**
>   * ext4_has_free_blocks()
>   * @sbi:	in-core super block structure.
> @@ -1624,9 +1682,15 @@ ext4_fsblk_t ext4_has_free_blocks(struct ext4_sb_info *sbi,
>  		(sbi->s_resgid == 0 || !in_group_p(sbi->s_resgid)))
>  		root_blocks = ext4_r_blocks_count(sbi->s_es);
>  #ifdef CONFIG_SMP
> -	if (free_blocks - root_blocks < FBC_BATCH)
> +	/* Each CPU can accumulate FBC_BATCH blocks in their local
> +	 * counters. So we need to make sure we have free blocks more
> +	 * than FBC_BATCH  * nr_cpu_ids. Also add a window of 4 times.
> +	 */
> +	if (free_blocks - (nblocks + root_blocks) <
> +					(4 * (FBC_BATCH * nr_cpu_ids))) {
>  		free_blocks =
>  			percpu_counter_sum_and_set(&sbi->s_freeblocks_counter);
> +	}
>  #endif
>  	if (free_blocks <= root_blocks)
>  		/* we don't have free space */
> @@ -1634,7 +1698,7 @@ ext4_fsblk_t ext4_has_free_blocks(struct ext4_sb_info *sbi,
>  	if (free_blocks - root_blocks < nblocks)
>  		return free_blocks - root_blocks;
>  	return nblocks;
> - }
> +}
> 
> 
>  /**
> @@ -1713,14 +1777,11 @@ ext4_fsblk_t ext4_old_new_blocks(handle_t *handle, struct inode *inode,
>  		/*
>  		 * With delalloc we already reserved the blocks
>  		 */
> -		*count = ext4_has_free_blocks(sbi, *count);
> -	}
> -	if (*count == 0) {
> -		*errp = -ENOSPC;
> -		return 0;	/*return with ENOSPC error */
> +		if (ext4_claim_free_blocks(sbi, *count)) {
> +			*errp = -ENOSPC;
> +			return 0;	/*return with ENOSPC error */
> +		}
>  	}
> -	num = *count;
> -
>  	/*
>  	 * Check quota for allocation of this block.
>  	 */
> @@ -1915,9 +1976,13 @@ ext4_fsblk_t ext4_old_new_blocks(handle_t *handle, struct inode *inode,
>  	le16_add_cpu(&gdp->bg_free_blocks_count, -num);
>  	gdp->bg_checksum = ext4_group_desc_csum(sbi, group_no, gdp);
>  	spin_unlock(sb_bgl_lock(sbi, group_no));
> -	if (!EXT4_I(inode)->i_delalloc_reserved_flag)
> -		percpu_counter_sub(&sbi->s_freeblocks_counter, num);
> -
> +	if (!EXT4_I(inode)->i_delalloc_reserved_flag && (*count != num)) {
> +		/*
> +		 * we allocated less blocks than we
> +		 * claimed. Add the difference back.
> +		 */
> +		percpu_counter_add(&sbi->s_freeblocks_counter, *count - num);
> +	}
>  	if (sbi->s_log_groups_per_flex) {
>  		ext4_group_t flex_group = ext4_flex_group(sbi, group_no);
>  		spin_lock(sb_bgl_lock(sbi, flex_group));
> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 7f11b25..3738039 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -1047,6 +1047,8 @@ extern ext4_fsblk_t ext4_new_blocks(handle_t *handle, struct inode *inode,
>  					unsigned long *count, int *errp);
>  extern ext4_fsblk_t ext4_old_new_blocks(handle_t *handle, struct inode *inode,
>  			ext4_fsblk_t goal, unsigned long *count, int *errp);
> +extern int ext4_claim_free_blocks(struct ext4_sb_info *sbi,
> +						ext4_fsblk_t nblocks);
>  extern ext4_fsblk_t ext4_has_free_blocks(struct ext4_sb_info *sbi,
>  						ext4_fsblk_t nblocks);
>  extern void ext4_free_blocks (handle_t *handle, struct inode *inode,
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index 1c289c1..d965a05 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -1537,13 +1537,10 @@ static int ext4_da_reserve_space(struct inode *inode, int nrblocks)
>  	md_needed = mdblocks - EXT4_I(inode)->i_reserved_meta_blocks;
>  	total = md_needed + nrblocks;
> 
> -	if (ext4_has_free_blocks(sbi, total) < total) {
> +	if (ext4_claim_free_blocks(sbi, total)) {
>  		spin_unlock(&EXT4_I(inode)->i_block_reservation_lock);
>  		return -ENOSPC;
>  	}
> -	/* reduce fs free blocks counter */
> -	percpu_counter_sub(&sbi->s_freeblocks_counter, total);
> -
>  	EXT4_I(inode)->i_reserved_data_blocks += nrblocks;
>  	EXT4_I(inode)->i_reserved_meta_blocks = mdblocks;
> 
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index 5267efc..7d94119 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -2977,9 +2977,15 @@ ext4_mb_mark_diskspace_used(struct ext4_allocation_context *ac,
>  	 * at write_begin() time for delayed allocation
>  	 * do not double accounting
>  	 */
> -	if (!(ac->ac_flags & EXT4_MB_DELALLOC_RESERVED))
> -		percpu_counter_sub(&sbi->s_freeblocks_counter,
> -					ac->ac_b_ex.fe_len);
> +	if (!(ac->ac_flags & EXT4_MB_DELALLOC_RESERVED) &&
> +			ac->ac_o_ex.fe_len != ac->ac_b_ex.fe_len) {
> +		/*
> +		 * we allocated less blocks than we calimed
> +		 * Add the difference back
> +		 */
> +		percpu_counter_add(&sbi->s_freeblocks_counter,
> +				ac->ac_o_ex.fe_len -ac->ac_b_ex.fe_len);
> +	}
> 
>  	if (sbi->s_log_groups_per_flex) {
>  		ext4_group_t flex_group = ext4_flex_group(sbi,
> @@ -4392,14 +4398,11 @@ ext4_fsblk_t ext4_mb_new_blocks(handle_t *handle,
>  		/*
>  		 * With delalloc we already reserved the blocks
>  		 */
> -		ar->len = ext4_has_free_blocks(sbi, ar->len);
> -	}
> -
> -	if (ar->len == 0) {
> -		*errp = -ENOSPC;
> -		return 0;
> +		if (ext4_claim_free_blocks(sbi, ar->len)) {
> +			*errp = -ENOSPC;
> +			return 0;
> +		}
>  	}
> -
>  	while (ar->len && DQUOT_ALLOC_BLOCK(ar->inode, ar->len)) {
>  		ar->flags |= EXT4_MB_HINT_NOPREALLOC;
>  		ar->len--;
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html