Re: [PATCH 01/12] quota: Add proper error handling on quota initialization.

[Jan Kara <jack@suse.cz>]

On Wed 19-05-10 10:01:57, Dmitry Monakhov wrote:
> Currently dqget_initialize() is a black-box so IO errors are simply
> ignored. In order to pass to the caller real error codes quota
> interface must being redesigned in following way.
> 
> - return real error code from dqget()
> - return real error code from dquot_initialize()
> 
> Now filesystem it is filesystem's responsibility to dquot_initialize()
> return value.
> 
> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
...
> diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
> index 7c624e1..95aa445 100644
> --- a/fs/quota/dquot.c
> +++ b/fs/quota/dquot.c
> @@ -720,7 +720,7 @@ void dqput(struct dquot *dquot)
>  {
>  	int ret;
>  
> -	if (!dquot)
> +	if (!dquot || IS_ERR(dquot))
  It is unusual for "put" functions to silently ignore IS_ERR pointer
(while it is common for them to ignore NULL). So I'd prefer to keep the
old test so that the behavior is consistent with the rest of kernel and
make sure no callers of dqput() pass IS_ERR pointer to it...

> @@ -1332,18 +1340,19 @@ static int info_bdq_free(struct dquot *dquot, qsize_t space)
>   * It is better to call this function outside of any transaction as it
>   * might need a lot of space in journal for dquot structure allocation.
>   */
> -static void __dquot_initialize(struct inode *inode, int type)
> +static int __dquot_initialize(struct inode *inode, int type)
>  {
>  	unsigned int id = 0;
> -	int cnt;
> +	int cnt, err = 0;
>  	struct dquot *got[MAXQUOTAS];
>  	struct super_block *sb = inode->i_sb;
>  	qsize_t rsv;
>  
> +repeat:
>  	/* First test before acquiring mutex - solves deadlocks when we
>           * re-enter the quota code and are already holding the mutex */
>  	if (!sb_any_quota_active(inode->i_sb) || IS_NOQUOTA(inode))
> -		return;
> +		return 0;
>  
>  	/* First get references to structures we might need. */
>  	for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
> @@ -1362,35 +1371,50 @@ static void __dquot_initialize(struct inode *inode, int type)
>  	}
>  
>  	down_write(&sb_dqopt(sb)->dqptr_sem);
> -	if (IS_NOQUOTA(inode))
> +	if (IS_NOQUOTA(inode)) {
> +		err = 0;
>  		goto out_err;
> +	}
>  	for (cnt = 0; cnt < MAXQUOTAS; cnt++) {
>  		if (type != -1 && cnt != type)
>  			continue;
>  		/* Avoid races with quotaoff() */
>  		if (!sb_has_quota_active(sb, cnt))
>  			continue;
> -		if (!inode->i_dquot[cnt]) {
> -			inode->i_dquot[cnt] = got[cnt];
> -			got[cnt] = NULL;
> -			/*
> -			 * Make quota reservation system happy if someone
> -			 * did a write before quota was turned on
> -			 */
> -			rsv = inode_get_rsv_space(inode);
> -			if (unlikely(rsv))
> -				dquot_resv_space(inode->i_dquot[cnt], rsv);
> +		if (inode->i_dquot[cnt])
> +			continue;
> +		if (unlikely(IS_ERR(got[cnt]))) {
> +			err = PTR_ERR(got[cnt]);
> +			/* If dqget() was raced with quotaon() then we have to
> +			 * repeat lookup. */
  If we race with quotaon(), then add_dquot_ref() is responsible for adding
proper dquot reference. Therefore there's no need to retry lookup.
  Also I would do all the error checking just after dqget() because there's
not much point in doing it in this loop.

> +			if (err == -ESRCH) {
> +				err = 0;
> +				up_write(&sb_dqopt(sb)->dqptr_sem);
> +				dqput_all(got);
> +				goto repeat;
> +			}
> +			goto out_err;
>  		}
> +		inode->i_dquot[cnt] = got[cnt];
> +		got[cnt] = NULL;
> +		/*
> +		 * Make quota reservation system happy if someone
> +		 * did a write before quota was turned on
> +		 */
> +		rsv = inode_get_rsv_space(inode);
> +		if (unlikely(rsv))
> +			dquot_resv_space(inode->i_dquot[cnt], rsv);
>  	}
>  out_err:
>  	up_write(&sb_dqopt(sb)->dqptr_sem);
>  	/* Drop unused references */
>  	dqput_all(got);
> +	return err;
>  }
>  
> -void dquot_initialize(struct inode *inode)
> +int dquot_initialize(struct inode *inode)
>  {
> -	__dquot_initialize(inode, -1);
> +	return __dquot_initialize(inode, -1);
>  }
>  EXPORT_SYMBOL(dquot_initialize);
>  
...
> @@ -2092,24 +2127,30 @@ static int vfs_load_quota_inode(struct inode *inode, int type, int format_id,
>  	}
>  	mutex_unlock(&dqopt->dqio_mutex);
>  	spin_lock(&dq_state_lock);
> +	dqflags = dqopt->flags;
>  	dqopt->flags |= dquot_state_flag(flags, type);
>  	spin_unlock(&dq_state_lock);
>  
> -	add_dquot_ref(sb, type);
> +	error = add_dquot_ref(sb, type);
> +	if (error)
> +		goto out_dquot_init;
  This is actually wrong as quota state flags have already been set and
quota for some inodes is already initialized and even may have been already
changed. So I think you have no other option than release dqonoff_mutex and
call dquot_disable (that's the name after Christoph's cleanups) to cleanup
everything...

>  	mutex_unlock(&dqopt->dqonoff_mutex);
>  
>  	return 0;
> -
> +out_dquot_init:
> +	spin_lock(&dq_state_lock);
> +	dqopt->flags = dqflags;
> +	spin_unlock(&dq_state_lock);
>  out_file_init:
>  	dqopt->files[type] = NULL;
>  	iput(inode);
>  out_lock:
> -	if (oldflags != -1) {
> +	if (iflags != -1) {
>  		mutex_lock_nested(&inode->i_mutex, I_MUTEX_QUOTA);
>  		/* Set the flags back (in the case of accidental quotaon()
>  		 * on a wrong file we don't want to mess up the flags) */
>  		inode->i_flags &= ~(S_NOATIME | S_NOQUOTA | S_IMMUTABLE);
> -		inode->i_flags |= oldflags;
> +		inode->i_flags |= iflags;
>  		mutex_unlock(&inode->i_mutex);
>  	}
>  	mutex_unlock(&dqopt->dqonoff_mutex);


								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR