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

[Bob Peterson <rpeterso@redhat.com>]

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