Re: [PATCH 02/12] quota: Check what quota is properly initialized for inode before charge

[Jan Kara <jack@suse.cz>]

On Wed 19-05-10 10:01:58, Dmitry Monakhov wrote:
> Due to previous IO error or other bugs an inode may not has quota
> pointer. This definite sign of quota inconsistency/corruption.
> In fact this condition must being checked in all charge/uncharge
> functions. And if error was detected it is reasonable to fail whole
> operation. But uncharge(free_space/claim_space/free_inode) functions
> has no fail nature. This is because caller can't handle errors from
> such functions. How can you handle error from following call-trace?
> write_page()->quota_claim_space()
> 
> So alloc_space() and alloc_inode() are the only functions which we may
> reliably abort in case of any errors.
> 
> This patch add corresponding checks only for charge functions.
  Actually, I've realized that this patch has a problem if
dquot_alloc_space is called while add_dquot_ref() is running. Then it would
return EIO although it should just silently succeed (and yes, quotas will
be inconsistent but that's "life" when quota isn't filesystem metadata...).
So I don't think this is a move in the right direction at least for now...

								Honza
> 
> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
> ---
>  fs/quota/dquot.c |   41 +++++++++++++++++++++++++++++++++++------
>  1 files changed, 35 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
> index 95aa445..f0e68b3 100644
> --- a/fs/quota/dquot.c
> +++ b/fs/quota/dquot.c
> @@ -1541,7 +1541,7 @@ static void inode_decr_space(struct inode *inode, qsize_t number, int reserve)
>  int __dquot_alloc_space(struct inode *inode, qsize_t number,
>  		int warn, int reserve)
>  {
> -	int cnt, ret = 0;
> +	int cnt, active, ret = 0;
>  	char warntype[MAXQUOTAS];
>  
>  	/*
> @@ -1554,13 +1554,28 @@ int __dquot_alloc_space(struct inode *inode, qsize_t number,
>  	}
>  
>  	down_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
> +	/* Now recheck reliably when holding dqptr_sem */
> +	active = sb_any_quota_active(inode->i_sb);
> +	if (!active || IS_NOQUOTA(inode)) {
> +		inode_incr_space(inode, number, reserve);
> +		goto out;
> +	}
> +
>  	for (cnt = 0; cnt < MAXQUOTAS; cnt++)
>  		warntype[cnt] = QUOTA_NL_NOWARN;
>  
>  	spin_lock(&dq_data_lock);
>  	for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
> -		if (!inode->i_dquot[cnt])
> +		if (!(active & (1 << cnt)))
>  			continue;
> +		/*
> +		 * Given quota type is active, so quota must being
> +		 * initialized for this inode
> +		 */
> +		if (!inode->i_dquot[cnt]) {
> +			ret = -EIO;
> +			goto out_flush_warn;
> +		}
>  		ret = check_bdq(inode->i_dquot[cnt], number, !warn,
>  				warntype+cnt);
>  		if (ret) {
> @@ -1569,7 +1584,7 @@ int __dquot_alloc_space(struct inode *inode, qsize_t number,
>  		}
>  	}
>  	for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
> -		if (!inode->i_dquot[cnt])
> +		if (!(active & (1 << cnt)))
>  			continue;
>  		if (reserve)
>  			dquot_resv_space(inode->i_dquot[cnt], number);
> @@ -1595,7 +1610,7 @@ EXPORT_SYMBOL(__dquot_alloc_space);
>   */
>  int dquot_alloc_inode(const struct inode *inode)
>  {
> -	int cnt, ret = 0;
> +	int cnt, active, ret = 0;
>  	char warntype[MAXQUOTAS];
>  
>  	/* First test before acquiring mutex - solves deadlocks when we
> @@ -1605,17 +1620,31 @@ int dquot_alloc_inode(const struct inode *inode)
>  	for (cnt = 0; cnt < MAXQUOTAS; cnt++)
>  		warntype[cnt] = QUOTA_NL_NOWARN;
>  	down_read(&sb_dqopt(inode->i_sb)->dqptr_sem);
> +	/* Now recheck reliably when holding dqptr_sem */
> +	active = sb_any_quota_active(inode->i_sb);
> +	if (!active || IS_NOQUOTA(inode)) {
> +		return 0;
> +	}
> +
>  	spin_lock(&dq_data_lock);
>  	for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
> -		if (!inode->i_dquot[cnt])
> +		if (!(active & (1 < cnt)))
>  			continue;
> +		/*
> +		 * Given quota type is active, so quota must being
> +		 * initialized for this inode
> +		 */
> +		if (!inode->i_dquot[cnt]) {
> +			ret = -EIO;
> +			goto warn_put_all;
> +		}
>  		ret = check_idq(inode->i_dquot[cnt], 1, warntype + cnt);
>  		if (ret)
>  			goto warn_put_all;
>  	}
>  
>  	for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
> -		if (!inode->i_dquot[cnt])
> +		if ((!active & (1 < cnt)))
>  			continue;
>  		dquot_incr_inodes(inode->i_dquot[cnt], 1);
>  	}
> -- 
> 1.6.6.1
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR