Re: [PATCH v3] quota: propagate error from __dquot_initialize

[Jan Kara]

On Tue 28-11-17 23:01:44, Chao Yu wrote:
> From: Chao Yu <yuchao0@xxxxxxxxxx>
> 
> In commit 6184fc0b8dd7 ("quota: Propagate error from ->acquire_dquot()"),
> we have propagated error from __dquot_initialize to caller, but we forgot
> to handle such error in add_dquot_ref(), so, currently, during quota
> accounting information initialization flow, if we failed for some of
> inodes, we just ignore such error, and do account for others, which is
> not a good implementation.
> 
> In this patch, we choose to let user be aware of such error, so after
> turning on quota successfully, we can make sure all inodes disk usage
> can be accounted, which will be more reasonable.
> 
> Suggested-by: Jan Kara <jack@xxxxxxx>
> Signed-off-by: Chao Yu <yuchao0@xxxxxxxxxx>

Thanks. Added to my tree.

								Honza

> ---
> v3:
> - call dquot_disable with correct args in error path of vfs_load_quota_inode
>  fs/quota/dquot.c | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/fs/quota/dquot.c b/fs/quota/dquot.c
> index 8381db9db6d9..e44d9e12d885 100644
> --- a/fs/quota/dquot.c
> +++ b/fs/quota/dquot.c
> @@ -933,12 +933,13 @@ static int dqinit_needed(struct inode *inode, int type)
>  }
>  
>  /* This routine is guarded by s_umount semaphore */
> -static void add_dquot_ref(struct super_block *sb, int type)
> +static int add_dquot_ref(struct super_block *sb, int type)
>  {
>  	struct inode *inode, *old_inode = NULL;
>  #ifdef CONFIG_QUOTA_DEBUG
>  	int reserved = 0;
>  #endif
> +	int err = 0;
>  
>  	spin_lock(&sb->s_inode_list_lock);
>  	list_for_each_entry(inode, &sb->s_inodes, i_sb_list) {
> @@ -958,7 +959,11 @@ static void add_dquot_ref(struct super_block *sb, int type)
>  			reserved = 1;
>  #endif
>  		iput(old_inode);
> -		__dquot_initialize(inode, type);
> +		err = __dquot_initialize(inode, type);
> +		if (err) {
> +			iput(inode);
> +			goto out;
> +		}
>  
>  		/*
>  		 * We hold a reference to 'inode' so it couldn't have been
> @@ -973,7 +978,7 @@ static void add_dquot_ref(struct super_block *sb, int type)
>  	}
>  	spin_unlock(&sb->s_inode_list_lock);
>  	iput(old_inode);
> -
> +out:
>  #ifdef CONFIG_QUOTA_DEBUG
>  	if (reserved) {
>  		quota_error(sb, "Writes happened before quota was turned on "
> @@ -981,6 +986,7 @@ static void add_dquot_ref(struct super_block *sb, int type)
>  			"Please run quotacheck(8)");
>  	}
>  #endif
> +	return err;
>  }
>  
>  /*
> @@ -2364,10 +2370,11 @@ static int vfs_load_quota_inode(struct inode *inode, int type, int format_id,
>  	dqopt->flags |= dquot_state_flag(flags, type);
>  	spin_unlock(&dq_state_lock);
>  
> -	add_dquot_ref(sb, type);
> -
> -	return 0;
> +	error = add_dquot_ref(sb, type);
> +	if (error)
> +		dquot_disable(sb, type, flags);
>  
> +	return error;
>  out_file_init:
>  	dqopt->files[type] = NULL;
>  	iput(inode);
> -- 
> 2.14.1.145.gb3622a4ee
> 
> 
-- 
Jan Kara <jack@xxxxxxxx>
SUSE Labs, CR