----- Original Message -----
> From: "Steven Whitehouse" <swhit...@redhat.com>
> To: "Abhi Das" <a...@redhat.com>, cluster-devel@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 <a...@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
