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

[Luis de Bethencourt <luisbg@osg.samsung.com>]

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