[Cluster-devel] [GFS2 PATCH 1/3] gfs2: perform quota checks against allocation parameters

[Steven Whitehouse <swhiteho@redhat.com>]

Hi,

On 16/02/15 17:59, Abhi Das wrote:
> Use struct gfs2_alloc_parms as an argument to gfs2_quota_check()
> and gfs2_quota_lock_check() to check for quota violations while
> accounting for the new blocks requested by the current operation
> in ap->target.
>
> Previously, the number of new blocks requested during an operation
> were not accounted for during quota_check and would allow these
> operations to exceed quota. This was not very apparent since most
> operations allocated only 1 block at a time and quotas would get
> violated in the next operation. i.e. quota excess would only be by
> 1 block or so. With fallocate, (where we allocate a bunch of blocks
> at once) the quota excess is non-trivial and is addressed by this
> patch.
>
> Resolves: rhbz#1174295
> Signed-off-by: Abhi Das <adas@redhat.com>
> ---
>   fs/gfs2/aops.c   |  6 +++---
>   fs/gfs2/bmap.c   |  2 +-
>   fs/gfs2/file.c   |  9 +++++----
>   fs/gfs2/incore.h |  2 +-
>   fs/gfs2/inode.c  | 18 ++++++++++--------
>   fs/gfs2/quota.c  |  6 +++---
>   fs/gfs2/quota.h  |  8 +++++---
>   fs/gfs2/xattr.c  |  2 +-
>   8 files changed, 29 insertions(+), 24 deletions(-)
>
> diff --git a/fs/gfs2/aops.c b/fs/gfs2/aops.c
> index 805b37f..0261126 100644
> --- a/fs/gfs2/aops.c
> +++ b/fs/gfs2/aops.c
> @@ -671,12 +671,12 @@ static int gfs2_write_begin(struct file *file, struct address_space *mapping,
>   
>   	if (alloc_required) {
>   		struct gfs2_alloc_parms ap = { .aflags = 0, };
> -		error = gfs2_quota_lock_check(ip);
> +		requested = data_blocks + ind_blocks;
> +		ap.target = requested;
> +		error = gfs2_quota_lock_check(ip, &ap);
>   		if (error)
>   			goto out_unlock;
>   
> -		requested = data_blocks + ind_blocks;
> -		ap.target = requested;
>   		error = gfs2_inplace_reserve(ip, &ap);
>   		if (error)
>   			goto out_qunlock;
> diff --git a/fs/gfs2/bmap.c b/fs/gfs2/bmap.c
> index f0b945a..61296ec 100644
> --- a/fs/gfs2/bmap.c
> +++ b/fs/gfs2/bmap.c
> @@ -1224,7 +1224,7 @@ static int do_grow(struct inode *inode, u64 size)
>   
>   	if (gfs2_is_stuffed(ip) &&
>   	    (size > (sdp->sd_sb.sb_bsize - sizeof(struct gfs2_dinode)))) {
> -		error = gfs2_quota_lock_check(ip);
> +		error = gfs2_quota_lock_check(ip, &ap);
>   		if (error)
>   			return error;
>   
> diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
> index 6e600ab..2ea420a 100644
> --- a/fs/gfs2/file.c
> +++ b/fs/gfs2/file.c
> @@ -429,11 +429,11 @@ static int gfs2_page_mkwrite(struct vm_area_struct *vma, struct vm_fault *vmf)
>   	if (ret)
>   		goto out_unlock;
>   
> -	ret = gfs2_quota_lock_check(ip);
> -	if (ret)
> -		goto out_unlock;
>   	gfs2_write_calc_reserv(ip, PAGE_CACHE_SIZE, &data_blocks, &ind_blocks);
>   	ap.target = data_blocks + ind_blocks;
> +	ret = gfs2_quota_lock_check(ip, &ap);
> +	if (ret)
> +		goto out_unlock;
>   	ret = gfs2_inplace_reserve(ip, &ap);
>   	if (ret)
>   		goto out_quota_unlock;
> @@ -828,7 +828,8 @@ static long __gfs2_fallocate(struct file *file, int mode, loff_t offset, loff_t
>   			offset += bytes;
>   			continue;
>   		}
> -		error = gfs2_quota_lock_check(ip);
> +		ap.target = bytes >> sdp->sd_sb.sb_bsize_shift;
> +		error = gfs2_quota_lock_check(ip, &ap);
>   		if (error)
>   			return error;
>   retry:
> diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h
> index 7a2dbbc..3a4ea50 100644
> --- a/fs/gfs2/incore.h
> +++ b/fs/gfs2/incore.h
> @@ -301,7 +301,7 @@ struct gfs2_blkreserv {
>    * to the allocation code.
>    */
>   struct gfs2_alloc_parms {
> -	u32 target;
> +	u64 target;
Why does this need to be 64 bits in size? The max size of an rgrp is 
2^32, so there should be no need to represent an extent larger than that,

Steve.

>   	u32 aflags;
>   };
>   
> diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> index 73c72253..08bc84d 100644
> --- a/fs/gfs2/inode.c
> +++ b/fs/gfs2/inode.c
> @@ -382,7 +382,7 @@ static int alloc_dinode(struct gfs2_inode *ip, u32 flags, unsigned *dblocks)
>   	struct gfs2_alloc_parms ap = { .target = *dblocks, .aflags = flags, };
>   	int error;
>   
> -	error = gfs2_quota_lock_check(ip);
> +	error = gfs2_quota_lock_check(ip, &ap);
>   	if (error)
>   		goto out;
>   
> @@ -525,7 +525,7 @@ static int link_dinode(struct gfs2_inode *dip, const struct qstr *name,
>   	int error;
>   
>   	if (da->nr_blocks) {
> -		error = gfs2_quota_lock_check(dip);
> +		error = gfs2_quota_lock_check(dip, &ap);
>   		if (error)
>   			goto fail_quota_locks;
>   
> @@ -953,7 +953,7 @@ static int gfs2_link(struct dentry *old_dentry, struct inode *dir,
>   
>   	if (da.nr_blocks) {
>   		struct gfs2_alloc_parms ap = { .target = da.nr_blocks, };
> -		error = gfs2_quota_lock_check(dip);
> +		error = gfs2_quota_lock_check(dip, &ap);
>   		if (error)
>   			goto out_gunlock;
>   
> @@ -1470,7 +1470,7 @@ static int gfs2_rename(struct inode *odir, struct dentry *odentry,
>   
>   	if (da.nr_blocks) {
>   		struct gfs2_alloc_parms ap = { .target = da.nr_blocks, };
> -		error = gfs2_quota_lock_check(ndip);
> +		error = gfs2_quota_lock_check(ndip, &ap);
>   		if (error)
>   			goto out_gunlock;
>   
> @@ -1669,6 +1669,7 @@ static int setattr_chown(struct inode *inode, struct iattr *attr)
>   	kuid_t ouid, nuid;
>   	kgid_t ogid, ngid;
>   	int error;
> +	struct gfs2_alloc_parms ap;
>   
>   	ouid = inode->i_uid;
>   	ogid = inode->i_gid;
> @@ -1696,9 +1697,11 @@ static int setattr_chown(struct inode *inode, struct iattr *attr)
>   	if (error)
>   		goto out;
>   
> +	ap.target = gfs2_get_inode_blocks(&ip->i_inode);
> +
>   	if (!uid_eq(ouid, NO_UID_QUOTA_CHANGE) ||
>   	    !gid_eq(ogid, NO_GID_QUOTA_CHANGE)) {
> -		error = gfs2_quota_check(ip, nuid, ngid);
> +		error = gfs2_quota_check(ip, nuid, ngid, &ap);
>   		if (error)
>   			goto out_gunlock_q;
>   	}
> @@ -1713,9 +1716,8 @@ static int setattr_chown(struct inode *inode, struct iattr *attr)
>   
>   	if (!uid_eq(ouid, NO_UID_QUOTA_CHANGE) ||
>   	    !gid_eq(ogid, NO_GID_QUOTA_CHANGE)) {
> -		u64 blocks = gfs2_get_inode_blocks(&ip->i_inode);
> -		gfs2_quota_change(ip, -blocks, ouid, ogid);
> -		gfs2_quota_change(ip, blocks, nuid, ngid);
> +		gfs2_quota_change(ip, -ap.target, ouid, ogid);
> +		gfs2_quota_change(ip, ap.target, nuid, ngid);
>   	}
>   
>   out_end_trans:
> diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
> index c8b148b..3a0b780 100644
> --- a/fs/gfs2/quota.c
> +++ b/fs/gfs2/quota.c
> @@ -1093,7 +1093,8 @@ static int print_message(struct gfs2_quota_data *qd, char *type)
>   	return 0;
>   }
>   
> -int gfs2_quota_check(struct gfs2_inode *ip, kuid_t uid, kgid_t gid)
> +int gfs2_quota_check(struct gfs2_inode *ip, kuid_t uid, kgid_t gid,
> +		     struct gfs2_alloc_parms *ap)
>   {
>   	struct gfs2_sbd *sdp = GFS2_SB(&ip->i_inode);
>   	struct gfs2_quota_data *qd;
> @@ -1116,14 +1117,13 @@ int gfs2_quota_check(struct gfs2_inode *ip, kuid_t uid, kgid_t gid)
>   
>   		value = (s64)be64_to_cpu(qd->qd_qb.qb_value);
>   		spin_lock(&qd_lock);
> -		value += qd->qd_change;
> +		value += qd->qd_change + ap->target;
>   		spin_unlock(&qd_lock);
>   
>   		if (be64_to_cpu(qd->qd_qb.qb_limit) && (s64)be64_to_cpu(qd->qd_qb.qb_limit) < value) {
>   			print_message(qd, "exceeded");
>   			quota_send_warning(qd->qd_id,
>   					   sdp->sd_vfs->s_dev, QUOTA_NL_BHARDWARN);
> -
>   			error = -EDQUOT;
>   			break;
>   		} else if (be64_to_cpu(qd->qd_qb.qb_warn) &&
> diff --git a/fs/gfs2/quota.h b/fs/gfs2/quota.h
> index 55d506e..ad04b3a 100644
> --- a/fs/gfs2/quota.h
> +++ b/fs/gfs2/quota.h
> @@ -24,7 +24,8 @@ extern void gfs2_quota_unhold(struct gfs2_inode *ip);
>   extern int gfs2_quota_lock(struct gfs2_inode *ip, kuid_t uid, kgid_t gid);
>   extern void gfs2_quota_unlock(struct gfs2_inode *ip);
>   
> -extern int gfs2_quota_check(struct gfs2_inode *ip, kuid_t uid, kgid_t gid);
> +extern int gfs2_quota_check(struct gfs2_inode *ip, kuid_t uid, kgid_t gid,
> +			    struct gfs2_alloc_parms *ap);
>   extern void gfs2_quota_change(struct gfs2_inode *ip, s64 change,
>   			      kuid_t uid, kgid_t gid);
>   
> @@ -37,7 +38,8 @@ extern int gfs2_quotad(void *data);
>   
>   extern void gfs2_wake_up_statfs(struct gfs2_sbd *sdp);
>   
> -static inline int gfs2_quota_lock_check(struct gfs2_inode *ip)
> +static inline int gfs2_quota_lock_check(struct gfs2_inode *ip,
> +					struct gfs2_alloc_parms *ap)
>   {
>   	struct gfs2_sbd *sdp = GFS2_SB(&ip->i_inode);
>   	int ret;
> @@ -48,7 +50,7 @@ static inline int gfs2_quota_lock_check(struct gfs2_inode *ip)
>   		return ret;
>   	if (sdp->sd_args.ar_quota != GFS2_QUOTA_ON)
>   		return 0;
> -	ret = gfs2_quota_check(ip, ip->i_inode.i_uid, ip->i_inode.i_gid);
> +	ret = gfs2_quota_check(ip, ip->i_inode.i_uid, ip->i_inode.i_gid, ap);
>   	if (ret)
>   		gfs2_quota_unlock(ip);
>   	return ret;
> diff --git a/fs/gfs2/xattr.c b/fs/gfs2/xattr.c
> index 0b81f78..fd260ce 100644
> --- a/fs/gfs2/xattr.c
> +++ b/fs/gfs2/xattr.c
> @@ -732,7 +732,7 @@ static int ea_alloc_skeleton(struct gfs2_inode *ip, struct gfs2_ea_request *er,
>   	if (error)
>   		return error;
>   
> -	error = gfs2_quota_lock_check(ip);
> +	error = gfs2_quota_lock_check(ip, &ap);
>   	if (error)
>   		return error;
>