[Cluster-devel] [GFS2 PATCH 0/2] More quota fixes

[Cluster-devel] [GFS2 PATCH 0/2] More quota fixes

[Abhi Das]

This is a follow-up set of patches to fix the issues discovered during
testing of my previous set that reworked significant portions of the
quota code.

Abhi Das (2):
  gfs2: fix quota updates on block boundaries
  gfs2: limit quota log messages

 fs/gfs2/incore.h |   1 +
 fs/gfs2/inode.c  |   2 +-
 fs/gfs2/quota.c  | 210 +++++++++++++++++++++++++++++++++----------------------
 3 files changed, 129 insertions(+), 84 deletions(-)

-- 
1.8.1.4

[Cluster-devel] [GFS2 PATCH 1/2] gfs2: fix quota updates on block boundaries

[Abhi Das]

For smaller block sizes (512B, 1K, 2K), some quotas straddle block
boundaries such that the usage value is on one block and the rest
of the quota is on the previous block. In such cases, the value
does not get updated correctly. This patch fixes that by addressing
the boundary conditions correctly.

This patch also adds a (s64) cast that was missing in a call to
gfs2_quota_change() in inode.c

Resolves: rhbz#1174295
Signed-off-by: Abhi Das <adas@redhat.com>
---
 fs/gfs2/inode.c |   2 +-
 fs/gfs2/quota.c | 197 +++++++++++++++++++++++++++++++++-----------------------
 2 files changed, 119 insertions(+), 80 deletions(-)

diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
index 4d809eb..a088e54 100644
--- a/fs/gfs2/inode.c
+++ b/fs/gfs2/inode.c
@@ -1889,7 +1889,7 @@ 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)) {
-		gfs2_quota_change(ip, -ap.target, ouid, ogid);
+		gfs2_quota_change(ip, -(s64)ap.target, ouid, ogid);
 		gfs2_quota_change(ip, ap.target, nuid, ngid);
 	}
 
diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
index 5c27e48..01f4d40 100644
--- a/fs/gfs2/quota.c
+++ b/fs/gfs2/quota.c
@@ -652,6 +652,112 @@ static void do_qc(struct gfs2_quota_data *qd, s64 change)
 	mutex_unlock(&sdp->sd_quota_mutex);
 }
 
+static int gfs2_write_buf_to_page(struct gfs2_inode *ip, unsigned long index,
+				  unsigned off, void *buf, unsigned bytes)
+{
+	struct inode *inode = &ip->i_inode;
+	struct gfs2_sbd *sdp = GFS2_SB(inode);
+	struct address_space *mapping = inode->i_mapping;
+	struct page *page;
+	struct buffer_head *bh;
+	void *kaddr;
+	unsigned long fs_blk;
+	unsigned bsize = sdp->sd_sb.sb_bsize, bnum = 0, boff = 0;
+	unsigned to_write = bytes, pg_off = off;
+	int done = 0;
+
+	fs_blk = index << (PAGE_CACHE_SHIFT - sdp->sd_sb.sb_bsize_shift);
+	boff = off % bsize;
+
+	page = find_or_create_page(mapping, index, GFP_NOFS);
+	if (!page)
+		return -ENOMEM;
+	if (!page_has_buffers(page))
+		create_empty_buffers(page, bsize, 0);
+
+	bh = page_buffers(page);
+	while (!done) {
+		/* Find the beginning block within the page */
+		if (pg_off >= ((bnum * bsize) + bsize)) {
+			bh = bh->b_this_page;
+			bnum++;
+			fs_blk++;
+			continue;
+		}
+		if (!buffer_mapped(bh)) {
+			gfs2_block_map(inode, fs_blk, bh, 1);
+			if (!buffer_mapped(bh))
+				goto unlock_out;
+			/* If it's a newly allocated disk block, zero it */
+			if (buffer_new(bh))
+				zero_user(page, bnum * bsize, bh->b_size);
+		}
+		if (PageUptodate(page))
+			set_buffer_uptodate(bh);
+		if (!buffer_uptodate(bh)) {
+			ll_rw_block(READ | REQ_META, 1, &bh);
+			wait_on_buffer(bh);
+			if (!buffer_uptodate(bh))
+				goto unlock_out;
+		}
+		gfs2_trans_add_data(ip->i_gl, bh);
+
+		/* If we need to write to the next block as well */
+		if (to_write > (bsize - boff)) {
+			pg_off += (bsize - boff);
+			to_write -= (bsize - boff);
+			boff = pg_off % bsize;
+			continue;
+		}
+		done = 1;
+	}
+
+	/* Write to the page, now that we have setup the buffer(s) */
+	kaddr = kmap_atomic(page);
+	memcpy(kaddr + off, buf, bytes);
+	flush_dcache_page(page);
+	kunmap_atomic(kaddr);
+	unlock_page(page);
+	page_cache_release(page);
+
+	return 0;
+
+unlock_out:
+	unlock_page(page);
+	page_cache_release(page);
+	return -EIO;
+}
+
+static int gfs2_write_disk_quota(struct gfs2_inode *ip, struct gfs2_quota *qp,
+				 loff_t loc)
+{
+	unsigned long pg_beg;
+	unsigned pg_off, nbytes, overflow = 0;
+	int pg_oflow = 0, error;
+	void *ptr;
+
+	nbytes = sizeof(struct gfs2_quota);
+
+	pg_beg = loc >> PAGE_CACHE_SHIFT;
+	pg_off = loc % PAGE_CACHE_SIZE;
+
+	/* If the quota straddles a page boundary, split the write in two */
+	if ((pg_off + nbytes) > PAGE_CACHE_SIZE) {
+		pg_oflow = 1;
+		overflow = (pg_off + nbytes) - PAGE_CACHE_SIZE;
+	}
+
+	ptr = qp;
+	error = gfs2_write_buf_to_page(ip, pg_beg, pg_off, ptr,
+				       nbytes - overflow);
+	/* If there's an overflow, write the remaining bytes to the next page */
+	if (!error && pg_oflow)
+		error = gfs2_write_buf_to_page(ip, pg_beg + 1, 0,
+					       ptr + nbytes - overflow,
+					       overflow);
+	return error;
+}
+
 /**
  * gfs2_adjust_quota - adjust record of current block usage
  * @ip: The quota inode
@@ -672,15 +778,8 @@ static int gfs2_adjust_quota(struct gfs2_inode *ip, loff_t loc,
 {
 	struct inode *inode = &ip->i_inode;
 	struct gfs2_sbd *sdp = GFS2_SB(inode);
-	struct address_space *mapping = inode->i_mapping;
-	unsigned long index = loc >> PAGE_CACHE_SHIFT;
-	unsigned offset = loc & (PAGE_CACHE_SIZE - 1);
-	unsigned blocksize, iblock, pos;
-	struct buffer_head *bh;
-	struct page *page;
-	void *kaddr, *ptr;
 	struct gfs2_quota q;
-	int err, nbytes;
+	int err;
 	u64 size;
 
 	if (gfs2_is_stuffed(ip)) {
@@ -694,8 +793,11 @@ static int gfs2_adjust_quota(struct gfs2_inode *ip, loff_t loc,
 	if (err < 0)
 		return err;
 
+	loc -= sizeof(q); /* gfs2_internal_read would've advanced the loc ptr */
 	err = -EIO;
 	be64_add_cpu(&q.qu_value, change);
+	if (be64_to_cpu(q.qu_value) < 0)
+		q.qu_value = 0; /* Never go negative on quota usage */
 	qd->qd_qb.qb_value = q.qu_value;
 	if (fdq) {
 		if (fdq->d_fieldmask & QC_SPC_SOFT) {
@@ -712,79 +814,16 @@ static int gfs2_adjust_quota(struct gfs2_inode *ip, loff_t loc,
 		}
 	}
 
-	/* Write the quota into the quota file on disk */
-	ptr = &q;
-	nbytes = sizeof(struct gfs2_quota);
-get_a_page:
-	page = find_or_create_page(mapping, index, GFP_NOFS);
-	if (!page)
-		return -ENOMEM;
-
-	blocksize = inode->i_sb->s_blocksize;
-	iblock = index << (PAGE_CACHE_SHIFT - inode->i_sb->s_blocksize_bits);
-
-	if (!page_has_buffers(page))
-		create_empty_buffers(page, blocksize, 0);
-
-	bh = page_buffers(page);
-	pos = blocksize;
-	while (offset >= pos) {
-		bh = bh->b_this_page;
-		iblock++;
-		pos += blocksize;
-	}
-
-	if (!buffer_mapped(bh)) {
-		gfs2_block_map(inode, iblock, bh, 1);
-		if (!buffer_mapped(bh))
-			goto unlock_out;
-		/* If it's a newly allocated disk block for quota, zero it */
-		if (buffer_new(bh))
-			zero_user(page, pos - blocksize, bh->b_size);
-	}
-
-	if (PageUptodate(page))
-		set_buffer_uptodate(bh);
-
-	if (!buffer_uptodate(bh)) {
-		ll_rw_block(READ | REQ_META, 1, &bh);
-		wait_on_buffer(bh);
-		if (!buffer_uptodate(bh))
-			goto unlock_out;
-	}
-
-	gfs2_trans_add_data(ip->i_gl, bh);
-
-	kaddr = kmap_atomic(page);
-	if (offset + sizeof(struct gfs2_quota) > PAGE_CACHE_SIZE)
-		nbytes = PAGE_CACHE_SIZE - offset;
-	memcpy(kaddr + offset, ptr, nbytes);
-	flush_dcache_page(page);
-	kunmap_atomic(kaddr);
-	unlock_page(page);
-	page_cache_release(page);
-
-	/* If quota straddles page boundary, we need to update the rest of the
-	 * quota at the beginning of the next page */
-	if ((offset + sizeof(struct gfs2_quota)) > PAGE_CACHE_SIZE) {
-		ptr = ptr + nbytes;
-		nbytes = sizeof(struct gfs2_quota) - nbytes;
-		offset = 0;
-		index++;
-		goto get_a_page;
+	err = gfs2_write_disk_quota(ip, &q, loc);
+	if (!err) {
+		size = loc + sizeof(struct gfs2_quota);
+		if (size > inode->i_size)
+			i_size_write(inode, size);
+		inode->i_mtime = inode->i_atime = CURRENT_TIME;
+		mark_inode_dirty(inode);
+		set_bit(QDF_REFRESH, &qd->qd_flags);
 	}
 
-	size = loc + sizeof(struct gfs2_quota);
-	if (size > inode->i_size)
-		i_size_write(inode, size);
-	inode->i_mtime = inode->i_atime = CURRENT_TIME;
-	mark_inode_dirty(inode);
-	set_bit(QDF_REFRESH, &qd->qd_flags);
-	return 0;
-
-unlock_out:
-	unlock_page(page);
-	page_cache_release(page);
 	return err;
 }
 
-- 
1.8.1.4

[Cluster-devel] [GFS2 PATCH 1/2] gfs2: fix quota updates on block boundaries

[Bob Peterson]

Hi,

Some comments embedded.

----- Original Message -----
> For smaller block sizes (512B, 1K, 2K), some quotas straddle block
> boundaries such that the usage value is on one block and the rest
> of the quota is on the previous block. In such cases, the value
> does not get updated correctly. This patch fixes that by addressing
> the boundary conditions correctly.
> 
> This patch also adds a (s64) cast that was missing in a call to
> gfs2_quota_change() in inode.c
> 
> Resolves: rhbz#1174295
> Signed-off-by: Abhi Das <adas@redhat.com>
> ---
>  fs/gfs2/inode.c |   2 +-
>  fs/gfs2/quota.c | 197
>  +++++++++++++++++++++++++++++++++-----------------------
>  2 files changed, 119 insertions(+), 80 deletions(-)
> 
> diff --git a/fs/gfs2/inode.c b/fs/gfs2/inode.c
> index 4d809eb..a088e54 100644
> --- a/fs/gfs2/inode.c
> +++ b/fs/gfs2/inode.c
> @@ -1889,7 +1889,7 @@ 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)) {
> -		gfs2_quota_change(ip, -ap.target, ouid, ogid);
> +		gfs2_quota_change(ip, -(s64)ap.target, ouid, ogid);
>  		gfs2_quota_change(ip, ap.target, nuid, ngid);
>  	}
>  
> diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
> index 5c27e48..01f4d40 100644
> --- a/fs/gfs2/quota.c
> +++ b/fs/gfs2/quota.c
> @@ -652,6 +652,112 @@ static void do_qc(struct gfs2_quota_data *qd, s64
> change)
>  	mutex_unlock(&sdp->sd_quota_mutex);
>  }
>  
> +static int gfs2_write_buf_to_page(struct gfs2_inode *ip, unsigned long
> index,
> +				  unsigned off, void *buf, unsigned bytes)
> +{
> +	struct inode *inode = &ip->i_inode;
> +	struct gfs2_sbd *sdp = GFS2_SB(inode);
> +	struct address_space *mapping = inode->i_mapping;
> +	struct page *page;
> +	struct buffer_head *bh;
> +	void *kaddr;
> +	unsigned long fs_blk;

If this is truly a file system block, it should probably be u64, not
unsigned long.

> +	unsigned bsize = sdp->sd_sb.sb_bsize, bnum = 0, boff = 0;
> +	unsigned to_write = bytes, pg_off = off;
> +	int done = 0;
> +
> +	fs_blk = index << (PAGE_CACHE_SHIFT - sdp->sd_sb.sb_bsize_shift);
> +	boff = off % bsize;
> +
> +	page = find_or_create_page(mapping, index, GFP_NOFS);
> +	if (!page)
> +		return -ENOMEM;
> +	if (!page_has_buffers(page))
> +		create_empty_buffers(page, bsize, 0);
> +
> +	bh = page_buffers(page);
> +	while (!done) {
> +		/* Find the beginning block within the page */
> +		if (pg_off >= ((bnum * bsize) + bsize)) {
> +			bh = bh->b_this_page;
> +			bnum++;
> +			fs_blk++;
> +			continue;
> +		}
> +		if (!buffer_mapped(bh)) {
> +			gfs2_block_map(inode, fs_blk, bh, 1);
> +			if (!buffer_mapped(bh))
> +				goto unlock_out;
> +			/* If it's a newly allocated disk block, zero it */
> +			if (buffer_new(bh))
> +				zero_user(page, bnum * bsize, bh->b_size);
> +		}
> +		if (PageUptodate(page))
> +			set_buffer_uptodate(bh);
> +		if (!buffer_uptodate(bh)) {
> +			ll_rw_block(READ | REQ_META, 1, &bh);

I know this was just copied from the section below, but I don't think this read
should have REQ_META set because it's not metadata, right? At any rate, it's
treated as data in the trans_add_data below.

> +			wait_on_buffer(bh);
> +			if (!buffer_uptodate(bh))
> +				goto unlock_out;
> +		}
> +		gfs2_trans_add_data(ip->i_gl, bh);
> +
> +		/* If we need to write to the next block as well */
> +		if (to_write > (bsize - boff)) {
> +			pg_off += (bsize - boff);
> +			to_write -= (bsize - boff);
> +			boff = pg_off % bsize;
> +			continue;
> +		}
> +		done = 1;
> +	}
> +
> +	/* Write to the page, now that we have setup the buffer(s) */
> +	kaddr = kmap_atomic(page);
> +	memcpy(kaddr + off, buf, bytes);
> +	flush_dcache_page(page);
> +	kunmap_atomic(kaddr);
> +	unlock_page(page);
> +	page_cache_release(page);
> +
> +	return 0;
> +
> +unlock_out:
> +	unlock_page(page);
> +	page_cache_release(page);
> +	return -EIO;
> +}
> +
> +static int gfs2_write_disk_quota(struct gfs2_inode *ip, struct gfs2_quota
> *qp,
> +				 loff_t loc)
> +{
> +	unsigned long pg_beg;
> +	unsigned pg_off, nbytes, overflow = 0;
> +	int pg_oflow = 0, error;
> +	void *ptr;
> +
> +	nbytes = sizeof(struct gfs2_quota);
> +
> +	pg_beg = loc >> PAGE_CACHE_SHIFT;
> +	pg_off = loc % PAGE_CACHE_SIZE;
> +
> +	/* If the quota straddles a page boundary, split the write in two */
> +	if ((pg_off + nbytes) > PAGE_CACHE_SIZE) {
> +		pg_oflow = 1;
> +		overflow = (pg_off + nbytes) - PAGE_CACHE_SIZE;
> +	}
> +
> +	ptr = qp;
> +	error = gfs2_write_buf_to_page(ip, pg_beg, pg_off, ptr,
> +				       nbytes - overflow);
> +	/* If there's an overflow, write the remaining bytes to the next page */
> +	if (!error && pg_oflow)
> +		error = gfs2_write_buf_to_page(ip, pg_beg + 1, 0,
> +					       ptr + nbytes - overflow,
> +					       overflow);
> +	return error;
> +}
> +
>  /**
>   * gfs2_adjust_quota - adjust record of current block usage
>   * @ip: The quota inode
> @@ -672,15 +778,8 @@ static int gfs2_adjust_quota(struct gfs2_inode *ip,
> loff_t loc,
>  {
>  	struct inode *inode = &ip->i_inode;
>  	struct gfs2_sbd *sdp = GFS2_SB(inode);
> -	struct address_space *mapping = inode->i_mapping;
> -	unsigned long index = loc >> PAGE_CACHE_SHIFT;
> -	unsigned offset = loc & (PAGE_CACHE_SIZE - 1);
> -	unsigned blocksize, iblock, pos;
> -	struct buffer_head *bh;
> -	struct page *page;
> -	void *kaddr, *ptr;
>  	struct gfs2_quota q;
> -	int err, nbytes;
> +	int err;
>  	u64 size;
>  
>  	if (gfs2_is_stuffed(ip)) {
> @@ -694,8 +793,11 @@ static int gfs2_adjust_quota(struct gfs2_inode *ip,
> loff_t loc,
>  	if (err < 0)
>  		return err;
>  
> +	loc -= sizeof(q); /* gfs2_internal_read would've advanced the loc ptr */
>  	err = -EIO;
>  	be64_add_cpu(&q.qu_value, change);
> +	if (be64_to_cpu(q.qu_value) < 0)
> +		q.qu_value = 0; /* Never go negative on quota usage */
>  	qd->qd_qb.qb_value = q.qu_value;
>  	if (fdq) {
>  		if (fdq->d_fieldmask & QC_SPC_SOFT) {
> @@ -712,79 +814,16 @@ static int gfs2_adjust_quota(struct gfs2_inode *ip,
> loff_t loc,
>  		}
>  	}
>  
> -	/* Write the quota into the quota file on disk */
> -	ptr = &q;
> -	nbytes = sizeof(struct gfs2_quota);
> -get_a_page:
> -	page = find_or_create_page(mapping, index, GFP_NOFS);
> -	if (!page)
> -		return -ENOMEM;
> -
> -	blocksize = inode->i_sb->s_blocksize;
> -	iblock = index << (PAGE_CACHE_SHIFT - inode->i_sb->s_blocksize_bits);
> -
> -	if (!page_has_buffers(page))
> -		create_empty_buffers(page, blocksize, 0);
> -
> -	bh = page_buffers(page);
> -	pos = blocksize;
> -	while (offset >= pos) {
> -		bh = bh->b_this_page;
> -		iblock++;
> -		pos += blocksize;
> -	}
> -
> -	if (!buffer_mapped(bh)) {
> -		gfs2_block_map(inode, iblock, bh, 1);
> -		if (!buffer_mapped(bh))
> -			goto unlock_out;
> -		/* If it's a newly allocated disk block for quota, zero it */
> -		if (buffer_new(bh))
> -			zero_user(page, pos - blocksize, bh->b_size);
> -	}
> -
> -	if (PageUptodate(page))
> -		set_buffer_uptodate(bh);
> -
> -	if (!buffer_uptodate(bh)) {
> -		ll_rw_block(READ | REQ_META, 1, &bh);
> -		wait_on_buffer(bh);
> -		if (!buffer_uptodate(bh))
> -			goto unlock_out;
> -	}
> -
> -	gfs2_trans_add_data(ip->i_gl, bh);
> -
> -	kaddr = kmap_atomic(page);
> -	if (offset + sizeof(struct gfs2_quota) > PAGE_CACHE_SIZE)
> -		nbytes = PAGE_CACHE_SIZE - offset;
> -	memcpy(kaddr + offset, ptr, nbytes);
> -	flush_dcache_page(page);
> -	kunmap_atomic(kaddr);
> -	unlock_page(page);
> -	page_cache_release(page);
> -
> -	/* If quota straddles page boundary, we need to update the rest of the
> -	 * quota at the beginning of the next page */
> -	if ((offset + sizeof(struct gfs2_quota)) > PAGE_CACHE_SIZE) {
> -		ptr = ptr + nbytes;
> -		nbytes = sizeof(struct gfs2_quota) - nbytes;
> -		offset = 0;
> -		index++;
> -		goto get_a_page;
> +	err = gfs2_write_disk_quota(ip, &q, loc);
> +	if (!err) {
> +		size = loc + sizeof(struct gfs2_quota);
> +		if (size > inode->i_size)
> +			i_size_write(inode, size);
> +		inode->i_mtime = inode->i_atime = CURRENT_TIME;
> +		mark_inode_dirty(inode);
> +		set_bit(QDF_REFRESH, &qd->qd_flags);
>  	}
>  
> -	size = loc + sizeof(struct gfs2_quota);
> -	if (size > inode->i_size)
> -		i_size_write(inode, size);
> -	inode->i_mtime = inode->i_atime = CURRENT_TIME;
> -	mark_inode_dirty(inode);
> -	set_bit(QDF_REFRESH, &qd->qd_flags);
> -	return 0;
> -
> -unlock_out:
> -	unlock_page(page);
> -	page_cache_release(page);
>  	return err;
>  }
>  
> --
> 1.8.1.4
> 
> 

[Cluster-devel] [GFS2 PATCH 1/2] gfs2: fix quota updates on block boundaries

[Abhijith Das]

----- Original Message -----
> From: "Bob Peterson" <rpeterso@redhat.com>
> To: "Abhi Das" <adas@redhat.com>
> Cc: cluster-devel at redhat.com
> Sent: Monday, June 1, 2015 10:25:45 AM
> Subject: Re: [Cluster-devel] [GFS2 PATCH 1/2] gfs2: fix quota updates on block	boundaries
> 
> Hi,
> 
> Some comments embedded.
> 
> ----- Original Message -----
> >  
> > +static int gfs2_write_buf_to_page(struct gfs2_inode *ip, unsigned long
> > index,
> > +				  unsigned off, void *buf, unsigned bytes)
> > +{
> > +	struct inode *inode = &ip->i_inode;
> > +	struct gfs2_sbd *sdp = GFS2_SB(inode);
> > +	struct address_space *mapping = inode->i_mapping;
> > +	struct page *page;
> > +	struct buffer_head *bh;
> > +	void *kaddr;
> > +	unsigned long fs_blk;
> 
> If this is truly a file system block, it should probably be u64, not
> unsigned long.
> 

Yes. I'll fix this in my next version. That field is named poorly. It is the fs block
offset of the page index supplied. For the quota file, unsigned long should be plenty,
but my aim was to split this function out so it can be of general use. I'll fix it to
u64.

> > +	unsigned bsize = sdp->sd_sb.sb_bsize, bnum = 0, boff = 0;
> > +	unsigned to_write = bytes, pg_off = off;
> > +	int done = 0;
> > +
> > +	fs_blk = index << (PAGE_CACHE_SHIFT - sdp->sd_sb.sb_bsize_shift);
> > +	boff = off % bsize;
> > +
> > +	page = find_or_create_page(mapping, index, GFP_NOFS);
> > +	if (!page)
> > +		return -ENOMEM;
> > +	if (!page_has_buffers(page))
> > +		create_empty_buffers(page, bsize, 0);
> > +
> > +	bh = page_buffers(page);
> > +	while (!done) {
> > +		/* Find the beginning block within the page */
> > +		if (pg_off >= ((bnum * bsize) + bsize)) {
> > +			bh = bh->b_this_page;
> > +			bnum++;
> > +			fs_blk++;
> > +			continue;
> > +		}
> > +		if (!buffer_mapped(bh)) {
> > +			gfs2_block_map(inode, fs_blk, bh, 1);
> > +			if (!buffer_mapped(bh))
> > +				goto unlock_out;
> > +			/* If it's a newly allocated disk block, zero it */
> > +			if (buffer_new(bh))
> > +				zero_user(page, bnum * bsize, bh->b_size);
> > +		}
> > +		if (PageUptodate(page))
> > +			set_buffer_uptodate(bh);
> > +		if (!buffer_uptodate(bh)) {
> > +			ll_rw_block(READ | REQ_META, 1, &bh);
> 
> I know this was just copied from the section below, but I don't think this
> read
> should have REQ_META set because it's not metadata, right? At any rate, it's
> treated as data in the trans_add_data below.
> 

Yeah, another issue due to copying. I wonder if this piece was written before REQ_PRIO
was introduced and was meant to issue a priority read or if we want to treat the system
quota file contents as metadata.

Cheers!
--Abhi

[Cluster-devel] [GFS2 PATCH 2/2] gfs2: limit quota log messages

[Abhi Das]

This patch makes the quota subsystem only report once that a
particular user/group has exceeded their allotted quota.

Previously, it was possible for a program to continuously try
exceeding quota (despite receiving EDQUOT) and in turn trigger
gfs2 to issue a kernel log message about quota exceed. In theory,
this could get out of hand and flood the log and the filesystem
hosting the log files.

Resolves: rhbz#1174295
Signed-off-by: Abhi Das <adas@redhat.com>
---
 fs/gfs2/incore.h |  1 +
 fs/gfs2/quota.c  | 13 +++++++++----
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h
index 58b75ab..304a223 100644
--- a/fs/gfs2/incore.h
+++ b/fs/gfs2/incore.h
@@ -432,6 +432,7 @@ enum {
 	QDF_CHANGE		= 1,
 	QDF_LOCKED		= 2,
 	QDF_REFRESH		= 3,
+	QDF_QMSG_QUIET          = 4,
 };
 
 struct gfs2_quota_data {
diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
index 01f4d40..3dc13b53 100644
--- a/fs/gfs2/quota.c
+++ b/fs/gfs2/quota.c
@@ -649,6 +649,8 @@ static void do_qc(struct gfs2_quota_data *qd, s64 change)
 		slot_hold(qd);
 	}
 
+	if (change < 0) /* Reset quiet flag if we freed some blocks */
+		clear_bit(QDF_QMSG_QUIET, &qd->qd_flags);
 	mutex_unlock(&sdp->sd_quota_mutex);
 }
 
@@ -1187,10 +1189,13 @@ int gfs2_quota_check(struct gfs2_inode *ip, kuid_t uid, kgid_t gid,
 			/* If no min_target specified or we don't meet
 			 * min_target, return -EDQUOT */
 			if (!ap->min_target || ap->min_target > ap->allowed) {
-				print_message(qd, "exceeded");
-				quota_send_warning(qd->qd_id,
-						   sdp->sd_vfs->s_dev,
-						   QUOTA_NL_BHARDWARN);
+				if (!test_bit(QDF_QMSG_QUIET, &qd->qd_flags)) {
+					print_message(qd, "exceeded");
+					quota_send_warning(qd->qd_id,
+							   sdp->sd_vfs->s_dev,
+							   QUOTA_NL_BHARDWARN);
+					set_bit(QDF_QMSG_QUIET, &qd->qd_flags);
+				}
 				error = -EDQUOT;
 				break;
 			}
-- 
1.8.1.4

[Cluster-devel] [GFS2 PATCH 2/2] gfs2: limit quota log messages

[Bob Peterson]

Hi,

Comment embedded below:

----- Original Message -----
> This patch makes the quota subsystem only report once that a
> particular user/group has exceeded their allotted quota.
> 
> Previously, it was possible for a program to continuously try
> exceeding quota (despite receiving EDQUOT) and in turn trigger
> gfs2 to issue a kernel log message about quota exceed. In theory,
> this could get out of hand and flood the log and the filesystem
> hosting the log files.
> 
> Resolves: rhbz#1174295
> Signed-off-by: Abhi Das <adas@redhat.com>
> ---
>  fs/gfs2/incore.h |  1 +
>  fs/gfs2/quota.c  | 13 +++++++++----
>  2 files changed, 10 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h
> index 58b75ab..304a223 100644
> --- a/fs/gfs2/incore.h
> +++ b/fs/gfs2/incore.h
> @@ -432,6 +432,7 @@ enum {
>  	QDF_CHANGE		= 1,
>  	QDF_LOCKED		= 2,
>  	QDF_REFRESH		= 3,
> +	QDF_QMSG_QUIET          = 4,
>  };
>  
>  struct gfs2_quota_data {
> diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
> index 01f4d40..3dc13b53 100644
> --- a/fs/gfs2/quota.c
> +++ b/fs/gfs2/quota.c
> @@ -649,6 +649,8 @@ static void do_qc(struct gfs2_quota_data *qd, s64 change)
>  		slot_hold(qd);
>  	}
>  
> +	if (change < 0) /* Reset quiet flag if we freed some blocks */
> +		clear_bit(QDF_QMSG_QUIET, &qd->qd_flags);
>  	mutex_unlock(&sdp->sd_quota_mutex);
>  }
>  
> @@ -1187,10 +1189,13 @@ int gfs2_quota_check(struct gfs2_inode *ip, kuid_t
> uid, kgid_t gid,
>  			/* If no min_target specified or we don't meet
>  			 * min_target, return -EDQUOT */
>  			if (!ap->min_target || ap->min_target > ap->allowed) {
> -				print_message(qd, "exceeded");
> -				quota_send_warning(qd->qd_id,
> -						   sdp->sd_vfs->s_dev,
> -						   QUOTA_NL_BHARDWARN);
> +				if (!test_bit(QDF_QMSG_QUIET, &qd->qd_flags)) {
> +					print_message(qd, "exceeded");
> +					quota_send_warning(qd->qd_id,
> +							   sdp->sd_vfs->s_dev,
> +							   QUOTA_NL_BHARDWARN);
> +					set_bit(QDF_QMSG_QUIET, &qd->qd_flags);
> +				}

Looks good, except that this is a perfect place to use test_and_set_bit().

I don't know the gfs2 quota code like you do, but does the bit also need to
be cleared if the user's quotas are increased?

Regards,

Bob Peterson
Red Hat File Systems

[Cluster-devel] [GFS2 PATCH 2/2] gfs2: limit quota log messages

[Abhijith Das]

----- Original Message -----
> From: "Bob Peterson" <rpeterso@redhat.com>
> To: "Abhi Das" <adas@redhat.com>
> Cc: cluster-devel at redhat.com
> Sent: Monday, June 1, 2015 11:27:59 AM
> Subject: Re: [Cluster-devel] [GFS2 PATCH 2/2] gfs2: limit quota log messages
> 
> Hi,
> 
> Comment embedded below:
> 
> ----- Original Message -----
> > This patch makes the quota subsystem only report once that a
> > particular user/group has exceeded their allotted quota.
> > 
> > Previously, it was possible for a program to continuously try
> > exceeding quota (despite receiving EDQUOT) and in turn trigger
> > gfs2 to issue a kernel log message about quota exceed. In theory,
> > this could get out of hand and flood the log and the filesystem
> > hosting the log files.
> > 
> > Resolves: rhbz#1174295
> > Signed-off-by: Abhi Das <adas@redhat.com>
> > ---
> >  fs/gfs2/incore.h |  1 +
> >  fs/gfs2/quota.c  | 13 +++++++++----
> >  2 files changed, 10 insertions(+), 4 deletions(-)
> > 
> > diff --git a/fs/gfs2/incore.h b/fs/gfs2/incore.h
> > index 58b75ab..304a223 100644
> > --- a/fs/gfs2/incore.h
> > +++ b/fs/gfs2/incore.h
> > @@ -432,6 +432,7 @@ enum {
> >  	QDF_CHANGE		= 1,
> >  	QDF_LOCKED		= 2,
> >  	QDF_REFRESH		= 3,
> > +	QDF_QMSG_QUIET          = 4,
> >  };
> >  
> >  struct gfs2_quota_data {
> > diff --git a/fs/gfs2/quota.c b/fs/gfs2/quota.c
> > index 01f4d40..3dc13b53 100644
> > --- a/fs/gfs2/quota.c
> > +++ b/fs/gfs2/quota.c
> > @@ -649,6 +649,8 @@ static void do_qc(struct gfs2_quota_data *qd, s64
> > change)
> >  		slot_hold(qd);
> >  	}
> >  
> > +	if (change < 0) /* Reset quiet flag if we freed some blocks */
> > +		clear_bit(QDF_QMSG_QUIET, &qd->qd_flags);
> >  	mutex_unlock(&sdp->sd_quota_mutex);
> >  }
> >  
> > @@ -1187,10 +1189,13 @@ int gfs2_quota_check(struct gfs2_inode *ip, kuid_t
> > uid, kgid_t gid,
> >  			/* If no min_target specified or we don't meet
> >  			 * min_target, return -EDQUOT */
> >  			if (!ap->min_target || ap->min_target > ap->allowed) {
> > -				print_message(qd, "exceeded");
> > -				quota_send_warning(qd->qd_id,
> > -						   sdp->sd_vfs->s_dev,
> > -						   QUOTA_NL_BHARDWARN);
> > +				if (!test_bit(QDF_QMSG_QUIET, &qd->qd_flags)) {
> > +					print_message(qd, "exceeded");
> > +					quota_send_warning(qd->qd_id,
> > +							   sdp->sd_vfs->s_dev,
> > +							   QUOTA_NL_BHARDWARN);
> > +					set_bit(QDF_QMSG_QUIET, &qd->qd_flags);
> > +				}
> 
> Looks good, except that this is a perfect place to use test_and_set_bit().
> 
> I don't know the gfs2 quota code like you do, but does the bit also need to
> be cleared if the user's quotas are increased?
> 

That's a good catch. I think even on decrease, it might make sense to un-silence
the warning for the next time the exceed event occurs.

Cheers!
--Abhi