Re: [PATCH] delalloc: add quota handling

[Mingming <cmm@us.ibm.com>]

On Sat, 2008-06-21 at 16:18 +0530, Aneesh Kumar K.V wrote:
> On Fri, Jun 20, 2008 at 06:14:38PM -0700, Mingming wrote:
> > Correct quota handling in delayed allocation. With this patch,
> > the quota for blocks are counted at block reservation time when
> > the fs free blocks counter are updated, instead of at later 
> > block allocation time.
> > 
> > Signed-off-by: Mingming Cao <cmm@us.ibm.com>
> > ---
> >  fs/ext4/balloc.c  |    8 +++++---
> >  fs/ext4/inode.c   |    5 +++++
> >  fs/ext4/mballoc.c |   22 ++++++++++++----------
> >  3 files changed, 22 insertions(+), 13 deletions(-)
> > 
> > Index: linux-2.6.26-rc6/fs/ext4/balloc.c
> > ===================================================================
> > --- linux-2.6.26-rc6.orig/fs/ext4/balloc.c	2008-06-20 18:06:02.000000000 -0700
> > +++ linux-2.6.26-rc6/fs/ext4/balloc.c	2008-06-20 18:06:05.000000000 -0700
> > @@ -1716,7 +1716,8 @@ ext4_fsblk_t ext4_old_new_blocks(handle_
> >  	/*
> >  	 * Check quota for allocation of this block.
> >  	 */
> > -	if (DQUOT_ALLOC_BLOCK(inode, num)) {
> > +	if (!EXT4_I(inode)->i_delalloc_reserved_flag &&
> > +	     DQUOT_ALLOC_BLOCK(inode, num)) {
> >  		*errp = -EDQUOT;
> >  		return 0;
> >  	}
> > @@ -1928,7 +1929,8 @@ allocated:
> > 
> >  	*errp = 0;
> >  	brelse(bitmap_bh);
> > -	DQUOT_FREE_BLOCK(inode, *count-num);
> > +	if (!EXT4_I(inode)->i_delalloc_reserved_flag)
> > +		DQUOT_FREE_BLOCK(inode, *count-num);
> >  	*count = num;
> >  	return ret_block;
> > 
> > @@ -1942,7 +1944,7 @@ out:
> >  	/*
> >  	 * Undo the block allocation
> >  	 */
> > -	if (!performed_allocation)
> > +	if (!EXT4_I(inode)->i_delalloc_reserved_flag && !performed_allocation)
> >  		DQUOT_FREE_BLOCK(inode, *count);
> >  	brelse(bitmap_bh);
> >  	return 0;
> > Index: linux-2.6.26-rc6/fs/ext4/inode.c
> > ===================================================================
> > --- linux-2.6.26-rc6.orig/fs/ext4/inode.c	2008-06-20 18:06:02.000000000 -0700
> > +++ linux-2.6.26-rc6/fs/ext4/inode.c	2008-06-20 18:06:05.000000000 -0700
> > @@ -1450,6 +1450,9 @@ static int ext4_da_reserve_space(struct 
> >  		return -ENOSPC;
> >  	}
> > 
> > +	if (DQUOT_ALLOC_BLOCK(inode, total))
> > +		return -EDQUOT;
> > +
> 
> We should not be counting meta-data blocks for quota. I guess this
> should be 
> 
> 	if (DQUOT_ALLOC_BLOCK(inode, nrblocks))
> 		return -EDQUOT;
> 

Metadata blocks are also accounted for quota today with mballoc and non
mballoc allocation.

> Also i think we should be doing quota check first. 

Okay, I will move the quota check/update before checking fs free blocks

> In mballoc we request
> for less number of blocks if quota is limited. In case of
> ext4_da_reserve_space even though we get called only with one block
> reservation request to make the code correct i guess we should do the
> mballoc equivalent.
> 
> while (ar->len && DQUOT_ALLOC_BLOCK(ar->inode, ar->len)) {
> 	ar->flags |= EXT4_MB_HINT_NOPREALLOC;
> 	ar->len--;
> }
> 
Hmm.. not sure if the requested allocation length can be reduced since
this is all needed for allocating one block. Reduce the length may
result in later ENOSPC.  BTW ar is a temprory variable that only alive
in the path doing block allocation, we need some addition field in inode
to pass the hint flag from write_begin() time down to the writepages().

> 
> Also in mballoc we should do some test for free quota blocks and
> should set the EXT4_MB_HINT_NOPREALLOC even for delalloc.
> 

With this patch, since the quota has already accounted at write_begin()
time  with delayed allocation., Should we check the free quota again
here (at the block allocation time)?

I think doing the quota reservation at write_begin() time for delalloc
is probably the right way, as you suggested on IRC.

Mingming
> >  	/* reduce fs free blocks counter */
> >  	percpu_counter_sub(&sbi->s_freeblocks_counter, total);
> > 
> > @@ -1479,6 +1482,8 @@ void ext4_da_release_space(struct inode 
> > 
> >  	release = to_free + mdb_free;
> > 
> > +	DQUOT_FREE_BLOCK(inode, release);
> 
> 
> This should be
> 	DQUOT_FREE_BLOCK(inode, to_free);
> 
> > +
> >  	/* update fs free blocks counter for truncate case */
> >  	percpu_counter_add(&sbi->s_freeblocks_counter, release);
> > 
> > Index: linux-2.6.26-rc6/fs/ext4/mballoc.c
> > ===================================================================
> > --- linux-2.6.26-rc6.orig/fs/ext4/mballoc.c	2008-06-20 18:06:01.000000000 -0700
> > +++ linux-2.6.26-rc6/fs/ext4/mballoc.c	2008-06-20 18:06:05.000000000 -0700
> > @@ -4037,7 +4037,7 @@ ext4_fsblk_t ext4_mb_new_blocks(handle_t
> >  	struct super_block *sb;
> >  	ext4_fsblk_t block = 0;
> >  	int freed;
> > -	int inquota;
> > +	int inquota = 0;
> > 
> >  	sb = ar->inode->i_sb;
> >  	sbi = EXT4_SB(sb);
> > @@ -4059,15 +4059,17 @@ ext4_fsblk_t ext4_mb_new_blocks(handle_t
> >  		return 0;
> >  	}
> > 
> > -	while (ar->len && DQUOT_ALLOC_BLOCK(ar->inode, ar->len)) {
> > -		ar->flags |= EXT4_MB_HINT_NOPREALLOC;
> > -		ar->len--;
> > -	}
> > -	if (ar->len == 0) {
> > -		*errp = -EDQUOT;
> > -		return 0;
> > +	if (!EXT4_I(ar->inode)->i_delalloc_reserved_flag) {
> > +		while (ar->len && DQUOT_ALLOC_BLOCK(ar->inode, ar->len)) {
> > +			ar->flags |= EXT4_MB_HINT_NOPREALLOC;
> > +			ar->len--;
> > +		}
> 
> 
> We need to set ar->flags even for delalloc. Otherwise we will try to
> normalize the request in mballoc.
> 
> 
> > +		if (ar->len == 0) {
> > +			*errp = -EDQUOT;
> > +			return 0;
> > +		}
> > +		inquota = ar->len;
> >  	}
> > -	inquota = ar->len;
> > 
> >  	if (EXT4_I(ar->inode)->i_delalloc_reserved_flag)
> >  		ar->flags |= EXT4_MB_DELALLOC_RESERVED;
> > @@ -4134,7 +4136,7 @@ repeat:
> >  out2:
> >  	kmem_cache_free(ext4_ac_cachep, ac);
> >  out1:
> > -	if (ar->len < inquota)
> > +	if (!EXT4_I(ar->inode)->i_delalloc_reserved_flag && ar->len < inquota)
> >  		DQUOT_FREE_BLOCK(ar->inode, inquota - ar->len);
> > 
> >  	return block;
> > 
> > 
> 
> -aneesh