[PATCH] fs: ramfs: make chunk counting in truncate() better readable

[PATCH] fs: ramfs: make chunk counting in truncate() better readable

[Sascha Hauer]

In ramfs_truncate() "newchunks" denotes the number of chunks we
want to have after the call. We decrease that number while iterating
over the existing chunks and decrease it further with every newly
allocated chunk until "newchunks" is zero.
This is a bit hard to read. Instead we drop the decreasing while
iterating over existing chunks and increase "oldchunks" while allocating
until it reaches "newchunks".

This is mainly done to make the next patch easier.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 fs/ramfs.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/fs/ramfs.c b/fs/ramfs.c
index 09dafe02ae..8ba8d77de9 100644
--- a/fs/ramfs.c
+++ b/fs/ramfs.c
@@ -384,19 +384,18 @@ static int ramfs_truncate(struct device_d *dev, FILE *f, ulong size)
 			if (!node->data)
 				return -ENOMEM;
 			data = node->data;
+			newchunks = 1;
 		}
 
-		newchunks--;
-		while (data->next) {
-			newchunks--;
+		while (data->next)
 			data = data->next;
-		}
 
-		while (newchunks--) {
+		while (newchunks > oldchunks) {
 			data->next = ramfs_get_chunk();
 			if (!data->next)
 				return -ENOMEM;
 			data = data->next;
+			oldchunks++;
 		}
 	}
 	node->size = size;
-- 
2.19.0


_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] fs: ramfs: make chunk counting in truncate() better readable

[Marcin Niestrój]

Sascha Hauer <s.hauer@pengutronix.de> writes:

> In ramfs_truncate() "newchunks" denotes the number of chunks we
> want to have after the call. We decrease that number while iterating
> over the existing chunks and decrease it further with every newly
> allocated chunk until "newchunks" is zero.
> This is a bit hard to read. Instead we drop the decreasing while
> iterating over existing chunks and increase "oldchunks" while allocating
> until it reaches "newchunks".
>
> This is mainly done to make the next patch easier.
>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  fs/ramfs.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
>
> diff --git a/fs/ramfs.c b/fs/ramfs.c
> index 09dafe02ae..8ba8d77de9 100644
> --- a/fs/ramfs.c
> +++ b/fs/ramfs.c
> @@ -384,19 +384,18 @@ static int ramfs_truncate(struct device_d *dev, FILE *f, ulong size)
>  			if (!node->data)
>  				return -ENOMEM;
>  			data = node->data;
> +			newchunks = 1;
>  		}
>  
> -		newchunks--;
> -		while (data->next) {
> -			newchunks--;
> +		while (data->next)
>  			data = data->next;
> -		}
>  
> -		while (newchunks--) {
> +		while (newchunks > oldchunks) {
>  			data->next = ramfs_get_chunk();
>  			if (!data->next)
>  				return -ENOMEM;
>  			data = data->next;
> +			oldchunks++;
>  		}
>  	}
>  	node->size = size;

Reviewed-by: Marcin Niestroj <m.niestroj@grinn-global.com>

-- 
Marcin Niestrój

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] fs: ramfs: make chunk counting in truncate() better readable

[Marcin Niestrój]

I think I was a little bit too early with review :) Below I have some
comments.

Marcin Niestrój <m.niestroj@grinn-global.com> writes:

> Sascha Hauer <s.hauer@pengutronix.de> writes:
>
>> In ramfs_truncate() "newchunks" denotes the number of chunks we
>> want to have after the call. We decrease that number while iterating
>> over the existing chunks and decrease it further with every newly
>> allocated chunk until "newchunks" is zero.
>> This is a bit hard to read. Instead we drop the decreasing while
>> iterating over existing chunks and increase "oldchunks" while allocating
>> until it reaches "newchunks".
>>
>> This is mainly done to make the next patch easier.
>>
>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
>> ---
>>  fs/ramfs.c | 9 ++++-----
>>  1 file changed, 4 insertions(+), 5 deletions(-)
>>
>> diff --git a/fs/ramfs.c b/fs/ramfs.c
>> index 09dafe02ae..8ba8d77de9 100644
>> --- a/fs/ramfs.c
>> +++ b/fs/ramfs.c
>> @@ -384,19 +384,18 @@ static int ramfs_truncate(struct device_d *dev, FILE *f, ulong size)
>>  			if (!node->data)
>>  				return -ENOMEM;
>>  			data = node->data;
>> +			newchunks = 1;

What is the reason of this instruction? What if 'size' == 16384 and we
do it on freshly opened file (with truncate(fd, 16384)? 'newchunk'
should be 2 in that case, or not?

Regards,
Marcin

>>  		}
>>  
>> -		newchunks--;
>> -		while (data->next) {
>> -			newchunks--;
>> +		while (data->next)
>>  			data = data->next;
>> -		}
>>  
>> -		while (newchunks--) {
>> +		while (newchunks > oldchunks) {
>>  			data->next = ramfs_get_chunk();
>>  			if (!data->next)
>>  				return -ENOMEM;
>>  			data = data->next;
>> +			oldchunks++;
>>  		}
>>  	}
>>  	node->size = size;
>
> Reviewed-by: Marcin Niestroj <m.niestroj@grinn-global.com>

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox

Re: [PATCH] fs: ramfs: make chunk counting in truncate() better readable

[Sascha Hauer]

On Wed, Sep 26, 2018 at 01:22:28PM +0200, Marcin Niestrój wrote:
> 
> I think I was a little bit too early with review :) Below I have some
> comments.
> 
> Marcin Niestrój <m.niestroj@grinn-global.com> writes:
> 
> > Sascha Hauer <s.hauer@pengutronix.de> writes:
> >
> >> In ramfs_truncate() "newchunks" denotes the number of chunks we
> >> want to have after the call. We decrease that number while iterating
> >> over the existing chunks and decrease it further with every newly
> >> allocated chunk until "newchunks" is zero.
> >> This is a bit hard to read. Instead we drop the decreasing while
> >> iterating over existing chunks and increase "oldchunks" while allocating
> >> until it reaches "newchunks".
> >>
> >> This is mainly done to make the next patch easier.
> >>
> >> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> >> ---
> >>  fs/ramfs.c | 9 ++++-----
> >>  1 file changed, 4 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/fs/ramfs.c b/fs/ramfs.c
> >> index 09dafe02ae..8ba8d77de9 100644
> >> --- a/fs/ramfs.c
> >> +++ b/fs/ramfs.c
> >> @@ -384,19 +384,18 @@ static int ramfs_truncate(struct device_d *dev, FILE *f, ulong size)
> >>  			if (!node->data)
> >>  				return -ENOMEM;
> >>  			data = node->data;
> >> +			newchunks = 1;
> 
> What is the reason of this instruction? What if 'size' == 16384 and we
> do it on freshly opened file (with truncate(fd, 16384)? 'newchunk'
> should be 2 in that case, or not?

Yes, you're right. It should be "oldchunks = 1" instead. Then we have:

>		if (!data) {
>			node->data = ramfs_get_chunk();
>			if (!node->data)
>				return -ENOMEM;
>			data = node->data;
>			oldchunks = 1;
>		}

!data we have no chunks allocated. We allocate one and set oldchunks to one. When we
do:

>		while (newchunks > oldchunks) {
>			data->next = ramfs_get_chunk();
>			if (!data->next)
>				return -ENOMEM;
>			data = data->next;
>			oldchunks++;
>		}

In your example above we execute this loop once, allocate the second chunk, oldchunks
will become two which is the same as newchunks and then we go out.

I hope this is correct now ;)

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

_______________________________________________
barebox mailing list
barebox@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/barebox