From mboxrd@z Thu Jan 1 00:00:00 1970 From: Abhijith Das Date: Tue, 17 Feb 2015 10:00:42 -0500 (EST) Subject: [Cluster-devel] [GFS2 PATCH 1/3] gfs2: perform quota checks against allocation parameters In-Reply-To: <54E30BFF.3010905@redhat.com> References: <1424109583-42670-1-git-send-email-adas@redhat.com> <1424109583-42670-2-git-send-email-adas@redhat.com> <54E30BFF.3010905@redhat.com> Message-ID: <1423440144.14354789.1424185242954.JavaMail.zimbra@redhat.com> List-Id: To: cluster-devel.redhat.com MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit ----- Original Message ----- > From: "Steven Whitehouse" > To: "Abhi Das" , 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 > > --- > > 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