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

[Nikolay Borisov <nborisov@suse.com>]

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;
>>  	}
>>  
>>  	/*
> 
> 
>