[PATCH] btrfs: Move loop termination condition in while()

[PATCH] btrfs: Move loop termination condition in while()

[Nikolay Borisov]

Fallocating a file in btrfs goes through several stages. The one before actually
inserting the fallocated extents is to create a qgroup reservation, covering
the desired range. To this end there is a loop in btrfs_fallocate which checks
to see if there are holes in the fallocated range or !PREALLOC extents past EOF
and if so create qgroup reservations for them. Unfortunately, the main condition
of the loop is burried right at the end of its body rather than in the actual
while statement which makes it non-obvious. Fix this by moving the condition
in the while statement where it belongs. No functional changes.

Signed-off-by: Nikolay Borisov <nborisov@suse.com>
---
 fs/btrfs/file.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
index e0d15c0d1641..ecbe186cb5da 100644
--- a/fs/btrfs/file.c
+++ b/fs/btrfs/file.c
@@ -3168,7 +3168,7 @@ static long btrfs_fallocate(struct file *file, int mode,
 
 	/* First, check if we exceed the qgroup limit */
 	INIT_LIST_HEAD(&reserve_list);
-	while (1) {
+	while (cur_offset < alloc_end) {
 		em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, cur_offset,
 				      alloc_end - cur_offset, 0);
 		if (IS_ERR(em)) {
@@ -3204,8 +3204,6 @@ static long btrfs_fallocate(struct file *file, int mode,
 		}
 		free_extent_map(em);
 		cur_offset = last_byte;
-		if (cur_offset >= alloc_end)
-			break;
 	}
 
 	/*
-- 
2.7.4

Re: [PATCH] btrfs: Move loop termination condition in while()

[Roman Mamedov]

On Wed,  1 Nov 2017 11:32:18 +0200
Nikolay Borisov <nborisov@suse.com> wrote:

> Fallocating a file in btrfs goes through several stages. The one before actually
> inserting the fallocated extents is to create a qgroup reservation, covering
> the desired range. To this end there is a loop in btrfs_fallocate which checks
> to see if there are holes in the fallocated range or !PREALLOC extents past EOF
> and if so create qgroup reservations for them. Unfortunately, the main condition
> of the loop is burried right at the end of its body rather than in the actual
> while statement which makes it non-obvious. Fix this by moving the condition
> in the while statement where it belongs. No functional changes.

If it turns out that "cur_offset >= alloc_end" from the get go, previously the
loop body would be entered and executed once. With this change, it will not
anymore.

I did not examine the context to see if such case is possible, likely,
beneficial or harmful. But if you wanted 100% no functional changes no matter
what, maybe better use a "do ... while" loop?

> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
> ---
>  fs/btrfs/file.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
> index e0d15c0d1641..ecbe186cb5da 100644
> --- a/fs/btrfs/file.c
> +++ b/fs/btrfs/file.c
> @@ -3168,7 +3168,7 @@ static long btrfs_fallocate(struct file *file, int mode,
>  
>  	/* First, check if we exceed the qgroup limit */
>  	INIT_LIST_HEAD(&reserve_list);
> -	while (1) {
> +	while (cur_offset < alloc_end) {
>  		em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, cur_offset,
>  				      alloc_end - cur_offset, 0);
>  		if (IS_ERR(em)) {
> @@ -3204,8 +3204,6 @@ static long btrfs_fallocate(struct file *file, int mode,
>  		}
>  		free_extent_map(em);
>  		cur_offset = last_byte;
> -		if (cur_offset >= alloc_end)
> -			break;
>  	}
>  
>  	/*



-- 
With respect,
Roman

Re: [PATCH] btrfs: Move loop termination condition in while()

[Nikolay Borisov]

On  1.11.2017 11:46, Roman Mamedov wrote:
> On Wed,  1 Nov 2017 11:32:18 +0200
> Nikolay Borisov <nborisov@suse.com> wrote:
> 
>> Fallocating a file in btrfs goes through several stages. The one before actually
>> inserting the fallocated extents is to create a qgroup reservation, covering
>> the desired range. To this end there is a loop in btrfs_fallocate which checks
>> to see if there are holes in the fallocated range or !PREALLOC extents past EOF
>> and if so create qgroup reservations for them. Unfortunately, the main condition
>> of the loop is burried right at the end of its body rather than in the actual
>> while statement which makes it non-obvious. Fix this by moving the condition
>> in the while statement where it belongs. No functional changes.
> 
> If it turns out that "cur_offset >= alloc_end" from the get go, previously the
> loop body would be entered and executed once. With this change, it will not
> anymore.

Good point, however this cannot happen, because for this to happen then
the following 2 expression need to be equal:

alloc_start = round_down(offset, blocksize);
alloc_end = round_up(offset + len, blocksize);

However, len is guaranteed to be > 0  due to a check in vfs_fallocate so
those can't really be equal.


> 
> I did not examine the context to see if such case is possible, likely,
> beneficial or harmful. But if you wanted 100% no functional changes no matter> what, maybe better use a "do ... while" loop?



> 
>> Signed-off-by: Nikolay Borisov <nborisov@suse.com>
>> ---
>>  fs/btrfs/file.c | 4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>>
>> diff --git a/fs/btrfs/file.c b/fs/btrfs/file.c
>> index e0d15c0d1641..ecbe186cb5da 100644
>> --- a/fs/btrfs/file.c
>> +++ b/fs/btrfs/file.c
>> @@ -3168,7 +3168,7 @@ static long btrfs_fallocate(struct file *file, int mode,
>>  
>>  	/* First, check if we exceed the qgroup limit */
>>  	INIT_LIST_HEAD(&reserve_list);
>> -	while (1) {
>> +	while (cur_offset < alloc_end) {
>>  		em = btrfs_get_extent(BTRFS_I(inode), NULL, 0, cur_offset,
>>  				      alloc_end - cur_offset, 0);
>>  		if (IS_ERR(em)) {
>> @@ -3204,8 +3204,6 @@ static long btrfs_fallocate(struct file *file, int mode,
>>  		}
>>  		free_extent_map(em);
>>  		cur_offset = last_byte;
>> -		if (cur_offset >= alloc_end)
>> -			break;
>>  	}
>>  
>>  	/*
> 
> 
> 

Re: [PATCH] btrfs: Move loop termination condition in while()

[David Sterba]

On Wed, Nov 01, 2017 at 12:42:18PM +0200, Nikolay Borisov wrote:
> 
> 
> On  1.11.2017 11:46, Roman Mamedov wrote:
> > On Wed,  1 Nov 2017 11:32:18 +0200
> > Nikolay Borisov <nborisov@suse.com> wrote:
> > 
> >> Fallocating a file in btrfs goes through several stages. The one before actually
> >> inserting the fallocated extents is to create a qgroup reservation, covering
> >> the desired range. To this end there is a loop in btrfs_fallocate which checks
> >> to see if there are holes in the fallocated range or !PREALLOC extents past EOF
> >> and if so create qgroup reservations for them. Unfortunately, the main condition
> >> of the loop is burried right at the end of its body rather than in the actual
> >> while statement which makes it non-obvious. Fix this by moving the condition
> >> in the while statement where it belongs. No functional changes.
> > 
> > If it turns out that "cur_offset >= alloc_end" from the get go, previously the
> > loop body would be entered and executed once. With this change, it will not
> > anymore.
> 
> Good point, however this cannot happen, because for this to happen then
> the following 2 expression need to be equal:
> 
> alloc_start = round_down(offset, blocksize);
> alloc_end = round_up(offset + len, blocksize);
> 
> However, len is guaranteed to be > 0  due to a check in vfs_fallocate so
> those can't really be equal.

Agreed, and

Reviewed-by: David Sterba <dsterba@suse.com>