* Re: [PATCH 1/3] befs: remove off argument of befs_read_datastream
2016-07-27 23:37 ` [PATCH 1/3] befs: remove off argument of befs_read_datastream Salah Triki
@ 2016-07-28 15:53 ` Luis de Bethencourt
0 siblings, 0 replies; 2+ messages in thread
From: Luis de Bethencourt @ 2016-07-28 15:53 UTC (permalink / raw)
To: Salah Triki; +Cc: linux-kernel, akpm, viro, linux-fsdevel
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
^ permalink raw reply [flat|nested] 2+ messages in thread