[PATCH] ext4: fix quota accounting in case of fallocate

[PATCH] ext4: fix quota accounting in case of fallocate

[Dmitry Monakhov]

allocated_meta_data is already included in 'used' variable.

Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
---
 fs/ext4/inode.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index bec222c..bf989fb 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -1110,7 +1110,8 @@ void ext4_da_update_reserve_space(struct inode *inode,
 		 */
 		if (allocated_meta_blocks)
 			dquot_claim_block(inode, allocated_meta_blocks);
-		dquot_release_reservation_block(inode, mdb_free + used);
+		dquot_release_reservation_block(inode, mdb_free + used -
+			allocated_meta_blocks);
 	}
 
 	/*
-- 
1.6.6.1

Re: [PATCH] ext4: fix quota accounting in case of fallocate

[Dmitry Monakhov]

Dmitry Monakhov <dmonakhov@openvz.org> writes:

> allocated_meta_data is already included in 'used' variable.
Since 2.6.33 is also affected the patch have to be pushed to
stable IMHO.
>
> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
> ---
>  fs/ext4/inode.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
>
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index bec222c..bf989fb 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -1110,7 +1110,8 @@ void ext4_da_update_reserve_space(struct inode *inode,
>  		 */
>  		if (allocated_meta_blocks)
>  			dquot_claim_block(inode, allocated_meta_blocks);
> -		dquot_release_reservation_block(inode, mdb_free + used);
> +		dquot_release_reservation_block(inode, mdb_free + used -
> +			allocated_meta_blocks);
>  	}
>  
>  	/*

Re: [PATCH] ext4: fix quota accounting in case of fallocate

[Aneesh Kumar K. V]

On Tue, 30 Mar 2010 18:24:35 +0400, Dmitry Monakhov <dmonakhov@openvz.org> wrote:
> allocated_meta_data is already included in 'used' variable.
> 
> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
> ---
>  fs/ext4/inode.c |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
> index bec222c..bf989fb 100644
> --- a/fs/ext4/inode.c
> +++ b/fs/ext4/inode.c
> @@ -1110,7 +1110,8 @@ void ext4_da_update_reserve_space(struct inode *inode,
>  		 */
>  		if (allocated_meta_blocks)
>  			dquot_claim_block(inode, allocated_meta_blocks);
> -		dquot_release_reservation_block(inode, mdb_free + used);
> +		dquot_release_reservation_block(inode, mdb_free + used -
> +			allocated_meta_blocks);
>  	}
> 
>  	/*

Do we really need to do this ? IIUC reservation count and actual
allocated count are two different. One block allocation we need to
remove all the blocks reserved from the reservation count and add
actually allocated blocks to the allocated count.

-aneesh

Re: [PATCH] ext4: fix quota accounting in case of fallocate

[dmonakhov]

"Aneesh Kumar K. V" <aneesh.kumar@linux.vnet.ibm.com> writes:

> On Tue, 30 Mar 2010 18:24:35 +0400, Dmitry Monakhov <dmonakhov@openvz.org> wrote:
>> allocated_meta_data is already included in 'used' variable.
>> 
>> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>
>> ---
>>  fs/ext4/inode.c |    3 ++-
>>  1 files changed, 2 insertions(+), 1 deletions(-)
>> 
>> diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
>> index bec222c..bf989fb 100644
>> --- a/fs/ext4/inode.c
>> +++ b/fs/ext4/inode.c
>> @@ -1110,7 +1110,8 @@ void ext4_da_update_reserve_space(struct inode *inode,
>>  		 */
>>  		if (allocated_meta_blocks)
>>  			dquot_claim_block(inode, allocated_meta_blocks);
>> -		dquot_release_reservation_block(inode, mdb_free + used);
>> +		dquot_release_reservation_block(inode, mdb_free + used -
>> +			allocated_meta_blocks);
>>  	}
>> 
>>  	/*
>
> Do we really need to do this ? IIUC reservation count and actual
> allocated count are two different. One block allocation we need to
> remove all the blocks reserved from the reservation count and add
Yes. remove all, but minus already allocated_metadata, which was
accounted in to metadata reservation.
> actually allocated blocks to the allocated count.
Just try an example:
reserve_space (inode, lblock := 1024 ) {
   md_needed = ext4_calc_metadata_amount(inode, lblock) (let it be '2')
   dquot_reserve_block(inode, md_needed + 1) /* '3' i.e blocks  reserved*/
   /* If this is first reservation for this inode then
    dq_rsv = inode->i_reserved_data_blocks + inode->i_reserved_meta_block
   */
}
Later called from fallocate
update_rerved_space(inode, used:=1, claim :=0) {
  /* Let i_allocatd_meta_data is '1' (as it so in most cases)  */
  ei->i_reserved_data_blocks -= used;  /* 1 - 1 => 0 */
  used += ei->i_allocated_meta_blocks; /* 1 + 1 => 2 */
  ei->i_reserved_meta_blocks -= ei->i_allocated_meta_blocks /* 2 - 1 => 1 */
  allocated_meta_blocks = ei->i_allocated_meta_blocks; /*  1 */
  ei->i_allocated_meta_blocks = 0;

  if (ei->i_reserved_data_blocks == 0) /* True in our case */
     mdb_free = ei->i_reserved_meta_blocks; /* mbd_free == 1*/

  if (allocated_meta_blocks)
         dquot_claim_block(inode, allocated_meta_blocks); /* claim '1' block*/
  dquot_release_reservation_block(inode, mdb_free + used); /* free (1 + 2) */

  /* So we reserved:
     dq_rsv = i_reserved_data_blocks + i_reserved_meta_block ( 3 blocks)
     But during update we claim + free:
          i_allocated_meta_data+(i_reserved_data_block+i_reserved_meta_data)
          (4 blocks).
          Which result in incorrect dquota reservation accounting(it
          goes negative)
  */

Initially i've found the issue by executing fsstress with falloc support.
It takes enouth process to catch writepage/fallocate overlapping.
xfstests-dev/ltp/xfsfsstress -p100 -n99999999

Re: [PATCH] ext4: fix quota accounting in case of fallocate

[tytso]

On Tue, Mar 30, 2010 at 06:24:35PM +0400, Dmitry Monakhov wrote:
> allocated_meta_data is already included in 'used' variable.
> 
> Signed-off-by: Dmitry Monakhov <dmonakhov@openvz.org>

Thanks, added to the ext4 patch queue.

-- Ted