[PATCH 1/3] befs: remove off argument of befs_read_datastream

[PATCH 1/3] befs: remove off argument of befs_read_datastream

[Luis de Bethencourt]

befs_read_datastream() is used to read the inode from the disk, off is
meant to provide the offset of the data in the buffer head. But the only
function using this argument already knows the starting offset of the node,
so this argument isn't needed.

Signed-off-by: Luis de Bethencourt <luisbg@osg.samsung.com>
---
Hi,

I know we are in release candidate 7 and maintainers are busy with important
bugs and regressions. Just sending this now so it is in the queue when the
merge window opens in two weeks.

befs_bt_read_node() is the only case where befs_read_datastream() was called
with an off pointer, the rest had NULL.

befs_read_datastream() effectively did:
block = pos >> BEFS_SB(sb)->block_shift;
*off = pos - (block << BEFS_SB(sb)->block_shift);

Since we only use it for inodes, pos above is either 0 or 1204, the node size
in BeFS by design. That shifted makes block equal 0. So off always ends up
being the same as pos. We can use this directly in befs_bt_read_node().

Thank for the reviews,
Luis

 fs/befs/btree.c      |  8 +++-----
 fs/befs/datastream.c | 10 +++-------
 fs/befs/datastream.h |  2 +-
 3 files changed, 7 insertions(+), 13 deletions(-)

diff --git a/fs/befs/btree.c b/fs/befs/btree.c
index 307645f9..3995d58 100644
--- a/fs/befs/btree.c
+++ b/fs/befs/btree.c
@@ -142,7 +142,7 @@ befs_bt_read_super(struct super_block *sb, const befs_data_stream *ds,
 
 	befs_debug(sb, "---> %s", __func__);
 
-	bh = befs_read_datastream(sb, ds, 0, NULL);
+	bh = befs_read_datastream(sb, ds, 0);
 
 	if (!bh) {
 		befs_error(sb, "Couldn't read index header.");
@@ -196,14 +196,12 @@ static int
 befs_bt_read_node(struct super_block *sb, const befs_data_stream *ds,
 		  struct befs_btree_node *node, befs_off_t node_off)
 {
-	uint off = 0;
-
 	befs_debug(sb, "---> %s", __func__);
 
 	if (node->bh)
 		brelse(node->bh);
 
-	node->bh = befs_read_datastream(sb, ds, node_off, &off);
+	node->bh = befs_read_datastream(sb, ds, node_off);
 	if (!node->bh) {
 		befs_error(sb, "%s failed to read "
 			   "node at %llu", __func__, node_off);
@@ -212,7 +210,7 @@ befs_bt_read_node(struct super_block *sb, const befs_data_stream *ds,
 		return BEFS_ERR;
 	}
 	node->od_node =
-	    (befs_btree_nodehead *) ((void *) node->bh->b_data + off);
+	    (befs_btree_nodehead *) ((void *) node->bh->b_data + node_off);
 
 	befs_dump_index_node(sb, node->od_node);
 
diff --git a/fs/befs/datastream.c b/fs/befs/datastream.c
index 26cc417..3c14c84 100644
--- a/fs/befs/datastream.c
+++ b/fs/befs/datastream.c
@@ -39,14 +39,12 @@ static int befs_find_brun_dblindirect(struct super_block *sb,
  * @sb: Filesystem superblock
  * @ds: datastrem to find data with
  * @pos: start of data
- * @off: offset of data in buffer_head->b_data
  *
- * Returns pointer to buffer_head containing data starting with offset @off,
- * if you don't need to know offset just set @off = NULL.
+ * Returns pointer to buffer_head containing data starting from pos.
  */
 struct buffer_head *
 befs_read_datastream(struct super_block *sb, const befs_data_stream *ds,
-		     befs_off_t pos, uint * off)
+		     befs_off_t pos)
 {
 	struct buffer_head *bh;
 	befs_block_run run;
@@ -54,8 +52,6 @@ befs_read_datastream(struct super_block *sb, const befs_data_stream *ds,
 
 	befs_debug(sb, "---> %s %llu", __func__, pos);
 	block = pos >> BEFS_SB(sb)->block_shift;
-	if (off)
-		*off = pos - (block << BEFS_SB(sb)->block_shift);
 
 	if (befs_fblock2brun(sb, ds, block, &run) != BEFS_OK) {
 		befs_error(sb, "BeFS: Error finding disk addr of block %lu",
@@ -131,7 +127,7 @@ befs_read_lsymlink(struct super_block *sb, const befs_data_stream *ds,
 	befs_debug(sb, "---> %s length: %llu", __func__, len);
 
 	while (bytes_read < len) {
-		bh = befs_read_datastream(sb, ds, bytes_read, NULL);
+		bh = befs_read_datastream(sb, ds, bytes_read);
 		if (!bh) {
 			befs_error(sb, "BeFS: Error reading datastream block "
 				   "starting from %llu", bytes_read);
diff --git a/fs/befs/datastream.h b/fs/befs/datastream.h
index 91ba820..76e1ab5 100644
--- a/fs/befs/datastream.h
+++ b/fs/befs/datastream.h
@@ -5,7 +5,7 @@
 
 struct buffer_head *befs_read_datastream(struct super_block *sb,
 					 const befs_data_stream *ds,
-					 befs_off_t pos, uint * off);
+					 befs_off_t pos);
 
 int befs_fblock2brun(struct super_block *sb, const befs_data_stream *data,
 		     befs_blocknr_t fblock, befs_block_run * run);
-- 
2.5.3

[PATCH 2/3] befs: in memory free_node_ptr and max_size never read

[Luis de Bethencourt]

The only place the values of free_node_ptr and max_size are read is in
befs_dump_index_entry(), which both times it is called, it is passed the on
disk superblock. Removing assignment of unused values.

Signed-off-by: Luis de Bethencourt <luisbg@osg.samsung.com>
---
Hi,

Noticed this when reading befs' btree code.

Thanks,
Luis

 fs/befs/btree.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/fs/befs/btree.c b/fs/befs/btree.c
index 3995d58..2b5b635 100644
--- a/fs/befs/btree.c
+++ b/fs/befs/btree.c
@@ -156,8 +156,6 @@ befs_bt_read_super(struct super_block *sb, const befs_data_stream *ds,
 	sup->max_depth = fs32_to_cpu(sb, od_sup->max_depth);
 	sup->data_type = fs32_to_cpu(sb, od_sup->data_type);
 	sup->root_node_ptr = fs64_to_cpu(sb, od_sup->root_node_ptr);
-	sup->free_node_ptr = fs64_to_cpu(sb, od_sup->free_node_ptr);
-	sup->max_size = fs64_to_cpu(sb, od_sup->max_size);
 
 	brelse(bh);
 	if (sup->magic != BEFS_BTREE_MAGIC) {
-- 
2.5.3

[PATCH 3/3] befs: fix typo in befs_bt_read_node documentation

[Luis de Bethencourt]

Fixing a grammatical error in the documentation.

Signed-off-by: Luis de Bethencourt <luisbg@osg.samsung.com>
---
Hi,

Since befs_bt_read_node() calls befs_read_datastream(), and was touched
in the first commit of the series I decided to send a fix for the
grammatical error I had noticed there a while ago.

Thanks for all the reviews,
Luis

 fs/befs/btree.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/fs/befs/btree.c b/fs/befs/btree.c
index 2b5b635..bf12b2c 100644
--- a/fs/befs/btree.c
+++ b/fs/befs/btree.c
@@ -181,8 +181,8 @@ befs_bt_read_super(struct super_block *sb, const befs_data_stream *ds,
  * Calls befs_read_datastream to read in the indicated btree node and
  * makes sure its header fields are in cpu byteorder, byteswapping if
  * necessary.
- * Note: node->bh must be NULL when this function called first
- * time. Don't forget brelse(node->bh) after last call.
+ * Note: node->bh must be NULL when this function is called the first time.
+ * Don't forget brelse(node->bh) after last call.
  *
  * On success, returns BEFS_OK and *@node contains the btree node that
  * starts at @node_off, with the node->head fields in cpu byte order.
-- 
2.5.3

Re: [PATCH 1/3] befs: remove off argument of befs_read_datastream

[Salah Triki]

On Tue, Jul 12, 2016 at 12:02:48AM +0100, Luis de Bethencourt wrote:
> befs_read_datastream() is used to read the inode from the disk, off is
> meant to provide the offset of the data in the buffer head. But the only
> function using this argument already knows the starting offset of the node,
> so this argument isn't needed.
> 
> Signed-off-by: Luis de Bethencourt <luisbg@osg.samsung.com>
> ---
> Hi,
> 
> I know we are in release candidate 7 and maintainers are busy with important
> bugs and regressions. Just sending this now so it is in the queue when the
> merge window opens in two weeks.
> 
> befs_bt_read_node() is the only case where befs_read_datastream() was called
> with an off pointer, the rest had NULL.
> 
> befs_read_datastream() effectively did:
> block = pos >> BEFS_SB(sb)->block_shift;
> *off = pos - (block << BEFS_SB(sb)->block_shift);
> 
> Since we only use it for inodes, pos above is either 0 or 1204, the node size
> in BeFS by design. That shifted makes block equal 0. So off always ends up
> being the same as pos. We can use this directly in befs_bt_read_node().
> 
> Thank for the reviews,
> Luis
> 
>  fs/befs/btree.c      |  8 +++-----
>  fs/befs/datastream.c | 10 +++-------
>  fs/befs/datastream.h |  2 +-
>  3 files changed, 7 insertions(+), 13 deletions(-)
> 
> diff --git a/fs/befs/btree.c b/fs/befs/btree.c
> index 307645f9..3995d58 100644
> --- a/fs/befs/btree.c
> +++ b/fs/befs/btree.c
> @@ -142,7 +142,7 @@ befs_bt_read_super(struct super_block *sb, const befs_data_stream *ds,
>  
>  	befs_debug(sb, "---> %s", __func__);
>  
> -	bh = befs_read_datastream(sb, ds, 0, NULL);
> +	bh = befs_read_datastream(sb, ds, 0);
>  
>  	if (!bh) {
>  		befs_error(sb, "Couldn't read index header.");
> @@ -196,14 +196,12 @@ static int
>  befs_bt_read_node(struct super_block *sb, const befs_data_stream *ds,
>  		  struct befs_btree_node *node, befs_off_t node_off)
>  {
> -	uint off = 0;
> -
>  	befs_debug(sb, "---> %s", __func__);
>  
>  	if (node->bh)
>  		brelse(node->bh);
>  
> -	node->bh = befs_read_datastream(sb, ds, node_off, &off);
> +	node->bh = befs_read_datastream(sb, ds, node_off);
>  	if (!node->bh) {
>  		befs_error(sb, "%s failed to read "
>  			   "node at %llu", __func__, node_off);
> @@ -212,7 +210,7 @@ befs_bt_read_node(struct super_block *sb, const befs_data_stream *ds,
>  		return BEFS_ERR;
>  	}
>  	node->od_node =
> -	    (befs_btree_nodehead *) ((void *) node->bh->b_data + off);
> +	    (befs_btree_nodehead *) ((void *) node->bh->b_data + node_off);
>  
>  	befs_dump_index_node(sb, node->od_node);
>  
> diff --git a/fs/befs/datastream.c b/fs/befs/datastream.c
> index 26cc417..3c14c84 100644
> --- a/fs/befs/datastream.c
> +++ b/fs/befs/datastream.c
> @@ -39,14 +39,12 @@ static int befs_find_brun_dblindirect(struct super_block *sb,
>   * @sb: Filesystem superblock
>   * @ds: datastrem to find data with
>   * @pos: start of data
> - * @off: offset of data in buffer_head->b_data
>   *
> - * Returns pointer to buffer_head containing data starting with offset @off,
> - * if you don't need to know offset just set @off = NULL.
> + * Returns pointer to buffer_head containing data starting from pos.
>   */
>  struct buffer_head *
>  befs_read_datastream(struct super_block *sb, const befs_data_stream *ds,
> -		     befs_off_t pos, uint * off)
> +		     befs_off_t pos)
>  {
>  	struct buffer_head *bh;
>  	befs_block_run run;
> @@ -54,8 +52,6 @@ befs_read_datastream(struct super_block *sb, const befs_data_stream *ds,
>  
>  	befs_debug(sb, "---> %s %llu", __func__, pos);
>  	block = pos >> BEFS_SB(sb)->block_shift;
> -	if (off)
> -		*off = pos - (block << BEFS_SB(sb)->block_shift);
>  
>  	if (befs_fblock2brun(sb, ds, block, &run) != BEFS_OK) {
>  		befs_error(sb, "BeFS: Error finding disk addr of block %lu",
> @@ -131,7 +127,7 @@ befs_read_lsymlink(struct super_block *sb, const befs_data_stream *ds,
>  	befs_debug(sb, "---> %s length: %llu", __func__, len);
>  
>  	while (bytes_read < len) {
> -		bh = befs_read_datastream(sb, ds, bytes_read, NULL);
> +		bh = befs_read_datastream(sb, ds, bytes_read);
>  		if (!bh) {
>  			befs_error(sb, "BeFS: Error reading datastream block "
>  				   "starting from %llu", bytes_read);
> diff --git a/fs/befs/datastream.h b/fs/befs/datastream.h
> index 91ba820..76e1ab5 100644
> --- a/fs/befs/datastream.h
> +++ b/fs/befs/datastream.h
> @@ -5,7 +5,7 @@
>  
>  struct buffer_head *befs_read_datastream(struct super_block *sb,
>  					 const befs_data_stream *ds,
> -					 befs_off_t pos, uint * off);
> +					 befs_off_t pos);
>  
>  int befs_fblock2brun(struct super_block *sb, const befs_data_stream *data,
>  		     befs_blocknr_t fblock, befs_block_run * run);
> -- 
> 2.5.3
> 

[...]
> Since we only use it for inodes, pos above is either 0 or 1204, the node size
> in BeFS by design. 

pos is the byte address that corresponds to a FS block number

> block = pos >> BEFS_SB(sb)->block_shift;
> *off = pos - (block << BEFS_SB(sb)->block_shift);

the result of the second shift is equal to pos, so *off is always set to
zero.

Am I wrong ?

best regards
--
salah

Re: [PATCH 1/3] befs: remove off argument of befs_read_datastream

[Luis de Bethencourt]

On 28/07/16 00:37, Salah Triki wrote:
> On Tue, Jul 12, 2016 at 12:02:48AM +0100, Luis de Bethencourt wrote:
>> befs_read_datastream() is used to read the inode from the disk, off is
>> meant to provide the offset of the data in the buffer head. But the only
>> function using this argument already knows the starting offset of the node,
>> so this argument isn't needed.
>>
>> Signed-off-by: Luis de Bethencourt <luisbg@osg.samsung.com>
>> ---
>> Hi,
>>
>> I know we are in release candidate 7 and maintainers are busy with important
>> bugs and regressions. Just sending this now so it is in the queue when the
>> merge window opens in two weeks.
>>
>> befs_bt_read_node() is the only case where befs_read_datastream() was called
>> with an off pointer, the rest had NULL.
>>
>> befs_read_datastream() effectively did:
>> block = pos >> BEFS_SB(sb)->block_shift;
>> *off = pos - (block << BEFS_SB(sb)->block_shift);
>>
>> Since we only use it for inodes, pos above is either 0 or 1204, the node size
>> in BeFS by design. That shifted makes block equal 0. So off always ends up
>> being the same as pos. We can use this directly in befs_bt_read_node().
>>
>> Thank for the reviews,
>> Luis
>>
>>  fs/befs/btree.c      |  8 +++-----
>>  fs/befs/datastream.c | 10 +++-------
>>  fs/befs/datastream.h |  2 +-
>>  3 files changed, 7 insertions(+), 13 deletions(-)
>>
>> diff --git a/fs/befs/btree.c b/fs/befs/btree.c
>> index 307645f9..3995d58 100644
>> --- a/fs/befs/btree.c
>> +++ b/fs/befs/btree.c
>> @@ -142,7 +142,7 @@ befs_bt_read_super(struct super_block *sb, const befs_data_stream *ds,
>>  
>>  	befs_debug(sb, "---> %s", __func__);
>>  
>> -	bh = befs_read_datastream(sb, ds, 0, NULL);
>> +	bh = befs_read_datastream(sb, ds, 0);
>>  
>>  	if (!bh) {
>>  		befs_error(sb, "Couldn't read index header.");
>> @@ -196,14 +196,12 @@ static int
>>  befs_bt_read_node(struct super_block *sb, const befs_data_stream *ds,
>>  		  struct befs_btree_node *node, befs_off_t node_off)
>>  {
>> -	uint off = 0;
>> -
>>  	befs_debug(sb, "---> %s", __func__);
>>  
>>  	if (node->bh)
>>  		brelse(node->bh);
>>  
>> -	node->bh = befs_read_datastream(sb, ds, node_off, &off);
>> +	node->bh = befs_read_datastream(sb, ds, node_off);
>>  	if (!node->bh) {
>>  		befs_error(sb, "%s failed to read "
>>  			   "node at %llu", __func__, node_off);
>> @@ -212,7 +210,7 @@ befs_bt_read_node(struct super_block *sb, const befs_data_stream *ds,
>>  		return BEFS_ERR;
>>  	}
>>  	node->od_node =
>> -	    (befs_btree_nodehead *) ((void *) node->bh->b_data + off);
>> +	    (befs_btree_nodehead *) ((void *) node->bh->b_data + node_off);
>>  
>>  	befs_dump_index_node(sb, node->od_node);
>>  
>> diff --git a/fs/befs/datastream.c b/fs/befs/datastream.c
>> index 26cc417..3c14c84 100644
>> --- a/fs/befs/datastream.c
>> +++ b/fs/befs/datastream.c
>> @@ -39,14 +39,12 @@ static int befs_find_brun_dblindirect(struct super_block *sb,
>>   * @sb: Filesystem superblock
>>   * @ds: datastrem to find data with
>>   * @pos: start of data
>> - * @off: offset of data in buffer_head->b_data
>>   *
>> - * Returns pointer to buffer_head containing data starting with offset @off,
>> - * if you don't need to know offset just set @off = NULL.
>> + * Returns pointer to buffer_head containing data starting from pos.
>>   */
>>  struct buffer_head *
>>  befs_read_datastream(struct super_block *sb, const befs_data_stream *ds,
>> -		     befs_off_t pos, uint * off)
>> +		     befs_off_t pos)
>>  {
>>  	struct buffer_head *bh;
>>  	befs_block_run run;
>> @@ -54,8 +52,6 @@ befs_read_datastream(struct super_block *sb, const befs_data_stream *ds,
>>  
>>  	befs_debug(sb, "---> %s %llu", __func__, pos);
>>  	block = pos >> BEFS_SB(sb)->block_shift;
>> -	if (off)
>> -		*off = pos - (block << BEFS_SB(sb)->block_shift);
>>  
>>  	if (befs_fblock2brun(sb, ds, block, &run) != BEFS_OK) {
>>  		befs_error(sb, "BeFS: Error finding disk addr of block %lu",
>> @@ -131,7 +127,7 @@ befs_read_lsymlink(struct super_block *sb, const befs_data_stream *ds,
>>  	befs_debug(sb, "---> %s length: %llu", __func__, len);
>>  
>>  	while (bytes_read < len) {
>> -		bh = befs_read_datastream(sb, ds, bytes_read, NULL);
>> +		bh = befs_read_datastream(sb, ds, bytes_read);
>>  		if (!bh) {
>>  			befs_error(sb, "BeFS: Error reading datastream block "
>>  				   "starting from %llu", bytes_read);
>> diff --git a/fs/befs/datastream.h b/fs/befs/datastream.h
>> index 91ba820..76e1ab5 100644
>> --- a/fs/befs/datastream.h
>> +++ b/fs/befs/datastream.h
>> @@ -5,7 +5,7 @@
>>  
>>  struct buffer_head *befs_read_datastream(struct super_block *sb,
>>  					 const befs_data_stream *ds,
>> -					 befs_off_t pos, uint * off);
>> +					 befs_off_t pos);
>>  
>>  int befs_fblock2brun(struct super_block *sb, const befs_data_stream *data,
>>  		     befs_blocknr_t fblock, befs_block_run * run);
>> -- 
>> 2.5.3
>>
> 
> [...]
>> Since we only use it for inodes, pos above is either 0 or 1204, the node size
>> in BeFS by design. 
> 
> pos is the byte address that corresponds to a FS block number
> 
>> block = pos >> BEFS_SB(sb)->block_shift;
>> *off = pos - (block << BEFS_SB(sb)->block_shift);
> 
> the result of the second shift is equal to pos, so *off is always set to
> zero.
> 
> Am I wrong ?
> 
> best regards
> --
> salah
> 

Hi Salah,

If you do the following in befs_read_datastream()

-       if (off)
+       if (off) {
                *off = pos - (block << BEFS_SB(sb)->block_shift);
+               befs_debug(sb, "read_datastream: off %u\n", *off);
+       }

You will see that off is a multiple of 1024 (node size). 0, 1024, 2048, etc

But now that I look into this again, I realize my patch logic didn't cover all
use cases of befs_bt_read_node(). So I am going to remove it from the commit
list in:
https://github.com/luisbg/linux-befs/commits/for-next

Nacked.

Thanks!
Luis