From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758934AbcG1Px2 (ORCPT ); Thu, 28 Jul 2016 11:53:28 -0400 Received: from ec2-52-27-115-49.us-west-2.compute.amazonaws.com ([52.27.115.49]:55935 "EHLO s-opensource.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752180AbcG1PxT (ORCPT ); Thu, 28 Jul 2016 11:53:19 -0400 Message-ID: <579A2A69.6050005@osg.samsung.com> Date: Thu, 28 Jul 2016 16:53:13 +0100 From: Luis de Bethencourt User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.8.0 MIME-Version: 1.0 To: Salah Triki CC: linux-kernel@vger.kernel.org, akpm@linux-foundation.org, viro@zeniv.linux.org.uk, linux-fsdevel@vger.kernel.org Subject: Re: [PATCH 1/3] befs: remove off argument of befs_read_datastream References: <1468278170-3550-1-git-send-email-luisbg@osg.samsung.com> <20160727233708.GA3942@pc> In-Reply-To: <20160727233708.GA3942@pc> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >> --- >> 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