[Cluster-devel] [GFS2 PATCH 0/3] fallocate quota fixes

[Cluster-devel] [GFS2 PATCH 0/3] fallocate quota fixes

[Abhi Das]

This is a revised set of patches for bz1174295 to address
fallocate/quota issues. These patches are only compile
tested so far.

patch 1 - This is the patch that actually addresses the quota
	  exceed issue. Quota checks were not being performed
	  against the blocks about to be allocated.

patch 2 - Modify gfs2_inplace_reserve() and gfs2_quota_check()
	  to return available number of blocks in case of
	  failure.

patch 3 - Allows fallocate to take advantage of patch 2 to
	  efficiently max out quotas or fill up the fs instead
	  of returning -EDQUOT/-ENOSPC and leaving some
	  available blocks unallocated.

Abhi Das (3):
  gfs2: perform quota checks against allocation parameters
  gfs2: allow quota_check and inplace_reserve to return available blocks
  gfs2: allow fallocate to max out quotas/fs efficiently

 fs/gfs2/aops.c   |  6 +++---
 fs/gfs2/bmap.c   |  2 +-
 fs/gfs2/file.c   | 27 ++++++++++++++++-----------
 fs/gfs2/incore.h |  3 ++-
 fs/gfs2/inode.c  | 18 ++++++++++--------
 fs/gfs2/quota.c  | 21 ++++++++++++++++++---
 fs/gfs2/quota.h  |  8 +++++---
 fs/gfs2/rgrp.c   |  8 +++++++-
 fs/gfs2/rgrp.h   |  2 +-
 fs/gfs2/xattr.c  |  2 +-
 10 files changed, 64 insertions(+), 33 deletions(-)

-- 
1.8.1.4

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

[Abhi Das]

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;
 	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;
 
-- 
1.8.1.4

[Cluster-devel] [GFS2 PATCH 2/3] gfs2: allow quota_check and inplace_reserve to return available blocks

[Abhi Das]

struct gfs2_alloc_parms is passed to gfs2_quota_check() and
gfs2_inplace_reserve() with ap->target containing the number of
blocks being requested for allocation in the current operation.

We add a new field to struct gfs2_alloc_parms called 'allowed'.
gfs2_quota_check() and gfs2_inplace_reserve() return -EDQUOT and
-ENOSPC respectively when they can't allow ap->target blocks to
be allocated due to quota violations or lack of space on the fs.
In such cases, we make these functions return the number of blocks
available in ap->allowed. A subsequent call with this value set
as ap->target is less likely to fail.

Signed-off-by: Abhi Das <adas@redhat.com>
---
 fs/gfs2/incore.h |  1 +
 fs/gfs2/quota.c  | 15 +++++++++++++++
 fs/gfs2/rgrp.c   |  8 +++++++-
 fs/gfs2/rgrp.h   |  2 +-
 4 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h
index 3a4ea50..bff2d7f 100644
--- a/fs/gfs2/incore.h
+++ b/fs/gfs2/incore.h
@@ -303,6 +303,7 @@ struct gfs2_blkreserv {
 struct gfs2_alloc_parms {
 	u64 target;
 	u32 aflags;
+	u64 allowed;
 };
 
 enum {
diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
index 3a0b780..c0d36e9 100644
--- a/fs/gfs2/quota.c
+++ b/fs/gfs2/quota.c
@@ -1093,6 +1093,19 @@ static int print_message(struct gfs2_quota_data *qd, char *type)
 	return 0;
 }
 
+/**
+ * gfs2_quota_check - check if allocating new blocks will exceed quota
+ * @ip:  The inode for which this check is being performed
+ * @uid: The uid to check against
+ * @gid: The gid to check against
+ * @ap:  The allocation parameters. ap->target contains the requested
+ *       blocks.
+ *
+ * Returns: 0 on success
+ *          -EDQUOT on quota violation, ap->allowed is set to number of
+ *                  blocks available
+ *          error code on any other error.
+ */
 int gfs2_quota_check(struct gfs2_inode *ip, kuid_t uid, kgid_t gid,
 		     struct gfs2_alloc_parms *ap)
 {
@@ -1125,6 +1138,8 @@ int gfs2_quota_check(struct gfs2_inode *ip, kuid_t uid, kgid_t gid,
 			quota_send_warning(qd->qd_id,
 					   sdp->sd_vfs->s_dev, QUOTA_NL_BHARDWARN);
 			error = -EDQUOT;
+			ap->allowed = (s64)be64_to_cpu(qd->qd_qb.qb_limit) -
+				(value - ap->target);
 			break;
 		} else if (be64_to_cpu(qd->qd_qb.qb_warn) &&
 			   (s64)be64_to_cpu(qd->qd_qb.qb_warn) < value &&
diff --git a/fs/gfs2/rgrp.c b/fs/gfs2/rgrp.c
index 9150207..e763f31 100644
--- a/fs/gfs2/rgrp.c
+++ b/fs/gfs2/rgrp.c
@@ -1947,9 +1947,11 @@ static inline int fast_to_acquire(struct gfs2_rgrpd *rgd)
  * @ap: the allocation parameters
  *
  * Returns: errno
+ *          if error is -ENOSPC, ap->allowed is set to the maximum number
+ *          of blocks available for allocation.
  */
 
-int gfs2_inplace_reserve(struct gfs2_inode *ip, const struct gfs2_alloc_parms *ap)
+int gfs2_inplace_reserve(struct gfs2_inode *ip, struct gfs2_alloc_parms *ap)
 {
 	struct gfs2_sbd *sdp = GFS2_SB(&ip->i_inode);
 	struct gfs2_rgrpd *begin = NULL;
@@ -1958,6 +1960,7 @@ int gfs2_inplace_reserve(struct gfs2_inode *ip, const struct gfs2_alloc_parms *a
 	u64 last_unlinked = NO_BLOCK;
 	int loops = 0;
 	u32 skip = 0;
+	u32 avail = 0;
 
 	if (sdp->sd_args.ar_rgrplvb)
 		flags |= GL_SKIP;
@@ -2030,6 +2033,8 @@ int gfs2_inplace_reserve(struct gfs2_inode *ip, const struct gfs2_alloc_parms *a
 		if (rs->rs_rbm.rgd->rd_free_clone >= ap->target) {
 			ip->i_rgd = rs->rs_rbm.rgd;
 			return 0;
+		} else if (rs->rs_rbm.rgd->rd_free_clone > avail) {
+			avail = rs->rs_rbm.rgd->rd_free_clone;
 		}
 
 check_rgrp:
@@ -2068,6 +2073,7 @@ next_rgrp:
 			gfs2_log_flush(sdp, NULL, NORMAL_FLUSH);
 	}
 
+	ap->allowed = avail;
 	return -ENOSPC;
 }
 
diff --git a/fs/gfs2/rgrp.h b/fs/gfs2/rgrp.h
index b104f4a..d38e0b1 100644
--- a/fs/gfs2/rgrp.h
+++ b/fs/gfs2/rgrp.h
@@ -41,7 +41,7 @@ extern void gfs2_rgrp_go_unlock(struct gfs2_holder *gh);
 extern struct gfs2_alloc *gfs2_alloc_get(struct gfs2_inode *ip);
 
 #define GFS2_AF_ORLOV 1
-extern int gfs2_inplace_reserve(struct gfs2_inode *ip, const struct gfs2_alloc_parms *ap);
+extern int gfs2_inplace_reserve(struct gfs2_inode *ip, struct gfs2_alloc_parms *ap);
 extern void gfs2_inplace_release(struct gfs2_inode *ip);
 
 extern int gfs2_alloc_blocks(struct gfs2_inode *ip, u64 *bn, unsigned int *n,
-- 
1.8.1.4

[Cluster-devel] [GFS2 PATCH 3/3] gfs2: allow fallocate to max out quotas/fs efficiently

[Abhi Das]

We can quickly get an estimate of how many more blocks are
available for allocation restricted by quota and fs size
respectively using the ap->allowed field in the gfs2_alloc_parms
structure. gfs2_quota_check() and gfs2_inplace_reserve() provide
these values.

By re-trying to allocate what's available instead of guessing, we
can max out quotas or the filesystem efficiently.

Bear in mind that this applies only when the requested fallocate
operation would otherwise error out with -EDQUOT or -ENOSPC without
utilizing all the blocks that might still be available.

Signed-off-by: Abhi Das <adas@redhat.com>
---
 fs/gfs2/file.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
index 2ea420a..57129fa 100644
--- a/fs/gfs2/file.c
+++ b/fs/gfs2/file.c
@@ -829,20 +829,24 @@ static long __gfs2_fallocate(struct file *file, int mode, loff_t offset, loff_t
 			continue;
 		}
 		ap.target = bytes >> sdp->sd_sb.sb_bsize_shift;
+	quota_retry:
 		error = gfs2_quota_lock_check(ip, &ap);
-		if (error)
+		if (error) {
+			if (error == -EDQUOT && ap.allowed) {
+				bytes = ap.allowed << sdp->sd_sb.sb_bsize_shift;
+				ap.target = ap.allowed;
+				goto quota_retry;
+			}
 			return error;
-retry:
+		}
+	retry:
 		gfs2_write_calc_reserv(ip, bytes, &data_blocks, &ind_blocks);
 
 		ap.target = data_blocks + ind_blocks;
 		error = gfs2_inplace_reserve(ip, &ap);
 		if (error) {
-			if (error == -ENOSPC && bytes > sdp->sd_sb.sb_bsize) {
-				bytes >>= 1;
-				bytes &= bsize_mask;
-				if (bytes == 0)
-					bytes = sdp->sd_sb.sb_bsize;
+			if (error == -ENOSPC && ap.allowed) {
+				bytes = ap.allowed << sdp->sd_sb.sb_bsize_shift;
 				goto retry;
 			}
 			goto out_qunlock;
-- 
1.8.1.4

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

[Steven Whitehouse]

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;
>   

[Cluster-devel] [GFS2 PATCH 3/3] gfs2: allow fallocate to max out quotas/fs efficiently

[Steven Whitehouse]

Hi,

On 16/02/15 17:59, Abhi Das wrote:
> We can quickly get an estimate of how many more blocks are
> available for allocation restricted by quota and fs size
> respectively using the ap->allowed field in the gfs2_alloc_parms
> structure. gfs2_quota_check() and gfs2_inplace_reserve() provide
> these values.
>
> By re-trying to allocate what's available instead of guessing, we
> can max out quotas or the filesystem efficiently.
>
> Bear in mind that this applies only when the requested fallocate
> operation would otherwise error out with -EDQUOT or -ENOSPC without
> utilizing all the blocks that might still be available.
>
> Signed-off-by: Abhi Das <adas@redhat.com>
> ---
>   fs/gfs2/file.c | 18 +++++++++++-------
>   1 file changed, 11 insertions(+), 7 deletions(-)
>
> diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
> index 2ea420a..57129fa 100644
> --- a/fs/gfs2/file.c
> +++ b/fs/gfs2/file.c
> @@ -829,20 +829,24 @@ static long __gfs2_fallocate(struct file *file, int mode, loff_t offset, loff_t
>   			continue;
>   		}
>   		ap.target = bytes >> sdp->sd_sb.sb_bsize_shift;
> +	quota_retry:
>   		error = gfs2_quota_lock_check(ip, &ap);
> -		if (error)
> +		if (error) {
> +			if (error == -EDQUOT && ap.allowed) {
> +				bytes = ap.allowed << sdp->sd_sb.sb_bsize_shift;
> +				ap.target = ap.allowed;
> +				goto quota_retry;
> +			}
>   			return error;
Do you really need to loop here if -EDQUOT is returned and ap.allowed is 
set? Why not just continue after having updated the target value?

Otherwise I think these patches look good,

Steve.

> -retry:
> +		}
> +	retry:
>   		gfs2_write_calc_reserv(ip, bytes, &data_blocks, &ind_blocks);
>   
>   		ap.target = data_blocks + ind_blocks;
>   		error = gfs2_inplace_reserve(ip, &ap);
>   		if (error) {
> -			if (error == -ENOSPC && bytes > sdp->sd_sb.sb_bsize) {
> -				bytes >>= 1;
> -				bytes &= bsize_mask;
> -				if (bytes == 0)
> -					bytes = sdp->sd_sb.sb_bsize;
> +			if (error == -ENOSPC && ap.allowed) {
> +				bytes = ap.allowed << sdp->sd_sb.sb_bsize_shift;
>   				goto retry;
>   			}
>   			goto out_qunlock;

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

[Abhijith Das]

----- Original Message -----
> From: "Steven Whitehouse" <swhiteho@redhat.com>
> To: "Abhi Das" <adas@redhat.com>, cluster-devel at redhat.com
> Sent: Tuesday, February 17, 2015 3:38:07 AM
> Subject: Re: [Cluster-devel] [GFS2 PATCH 1/3] gfs2: perform quota checks against allocation parameters
> 
> 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.
> 

This has got to do with setattr_chown() which calls gfs2_get_inode_blocks() to
get the number of blocks used by the inode being chowned. This is a u64 value
and since we also use ap.target to check against quotas in gfs2_quota_check(),
I had to change its size.

Cheers!
--Abhi

[Cluster-devel] [GFS2 PATCH 3/3] gfs2: allow fallocate to max out quotas/fs efficiently

[Abhijith Das]

----- Original Message -----
> From: "Steven Whitehouse" <swhiteho@redhat.com>
> To: "Abhi Das" <adas@redhat.com>, cluster-devel at redhat.com
> Sent: Tuesday, February 17, 2015 3:41:15 AM
> Subject: Re: [Cluster-devel] [GFS2 PATCH 3/3] gfs2: allow fallocate to max out quotas/fs efficiently
> 
> Hi,
> 
> On 16/02/15 17:59, Abhi Das wrote:
> > We can quickly get an estimate of how many more blocks are
> > available for allocation restricted by quota and fs size
> > respectively using the ap->allowed field in the gfs2_alloc_parms
> > structure. gfs2_quota_check() and gfs2_inplace_reserve() provide
> > these values.
> >
> > By re-trying to allocate what's available instead of guessing, we
> > can max out quotas or the filesystem efficiently.
> >
> > Bear in mind that this applies only when the requested fallocate
> > operation would otherwise error out with -EDQUOT or -ENOSPC without
> > utilizing all the blocks that might still be available.
> >
> > Signed-off-by: Abhi Das <adas@redhat.com>
> > ---
> >   fs/gfs2/file.c | 18 +++++++++++-------
> >   1 file changed, 11 insertions(+), 7 deletions(-)
> >
> > diff --git a/fs/gfs2/file.c b/fs/gfs2/file.c
> > index 2ea420a..57129fa 100644
> > --- a/fs/gfs2/file.c
> > +++ b/fs/gfs2/file.c
> > @@ -829,20 +829,24 @@ static long __gfs2_fallocate(struct file *file, int
> > mode, loff_t offset, loff_t
> >   			continue;
> >   		}
> >   		ap.target = bytes >> sdp->sd_sb.sb_bsize_shift;
> > +	quota_retry:
> >   		error = gfs2_quota_lock_check(ip, &ap);
> > -		if (error)
> > +		if (error) {
> > +			if (error == -EDQUOT && ap.allowed) {
> > +				bytes = ap.allowed << sdp->sd_sb.sb_bsize_shift;
> > +				ap.target = ap.allowed;
> > +				goto quota_retry;
> > +			}
> >   			return error;
> Do you really need to loop here if -EDQUOT is returned and ap.allowed is
> set? Why not just continue after having updated the target value?
> 
> Otherwise I think these patches look good,
> 
> Steve.
> 

If gfs2_quota_lock_check() fails, the quota isn't locked, which we'd have to do
anyway. And I figured there's a race here where some other process could use up
all or part of ap.allowed blocks before we re-lock the quota thereby causing us
to exceed quotas. So, I decided to retry gfs2_quota_lock_check() itself.

Cheers!
--Abhi