Re: [PATCH] fs: befs: Replace befs_bread by sb_bread

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

On 24/06/16 21:30, Salah Triki wrote:
> Since befs_bread merely calls sb_bread, replace it by sb_bread.
> 
> Signed-off-by: Salah Triki <salah.triki@gmail.com>
> ---
>  fs/befs/datastream.c |  6 +++---
>  fs/befs/io.c         | 24 ------------------------
>  fs/befs/io.h         |  2 --
>  fs/befs/linuxvfs.c   |  2 +-
>  4 files changed, 4 insertions(+), 30 deletions(-)
> 
> diff --git a/fs/befs/datastream.c b/fs/befs/datastream.c
> index af1bc19..26cc417 100644
> --- a/fs/befs/datastream.c
> +++ b/fs/befs/datastream.c
> @@ -326,7 +326,7 @@ befs_find_brun_indirect(struct super_block *sb,
>  
>  	/* Examine blocks of the indirect run one at a time */
>  	for (i = 0; i < indirect.len; i++) {
> -		indirblock = befs_bread(sb, indirblockno + i);
> +		indirblock = sb_bread(sb, indirblockno + i);
>  		if (indirblock == NULL) {
>  			befs_debug(sb, "---> %s failed to read "
>  				   "disk block %lu from the indirect brun",
> @@ -471,7 +471,7 @@ befs_find_brun_dblindirect(struct super_block *sb,
>  	}
>  
>  	dbl_indir_block =
> -	    befs_bread(sb, iaddr2blockno(sb, &data->double_indirect) +
> +	    sb_bread(sb, iaddr2blockno(sb, &data->double_indirect) +
>  					dbl_which_block);
>  	if (dbl_indir_block == NULL) {
>  		befs_error(sb, "%s couldn't read the "
> @@ -499,7 +499,7 @@ befs_find_brun_dblindirect(struct super_block *sb,
>  	}
>  
>  	indir_block =
> -	    befs_bread(sb, iaddr2blockno(sb, &indir_run) + which_block);
> +	    sb_bread(sb, iaddr2blockno(sb, &indir_run) + which_block);
>  	if (indir_block == NULL) {
>  		befs_error(sb, "%s couldn't read the indirect block "
>  			   "at blockno %lu", __func__, (unsigned long)
> diff --git a/fs/befs/io.c b/fs/befs/io.c
> index 523c8af..4223b77 100644
> --- a/fs/befs/io.c
> +++ b/fs/befs/io.c
> @@ -59,27 +59,3 @@ befs_bread_iaddr(struct super_block *sb, befs_inode_addr iaddr)
>  	befs_debug(sb, "<--- %s ERROR", __func__);
>  	return NULL;
>  }
> -
> -struct buffer_head *
> -befs_bread(struct super_block *sb, befs_blocknr_t block)
> -{
> -	struct buffer_head *bh;
> -
> -	befs_debug(sb, "---> Enter %s %lu", __func__, (unsigned long)block);
> -
> -	bh = sb_bread(sb, block);
> -
> -	if (bh == NULL) {
> -		befs_error(sb, "Failed to read block %lu",
> -			   (unsigned long)block);
> -		goto error;
> -	}
> -
> -	befs_debug(sb, "<--- %s", __func__);
> -
> -	return bh;
> -
> -      error:
> -	befs_debug(sb, "<--- %s ERROR", __func__);
> -	return NULL;
> -}
> diff --git a/fs/befs/io.h b/fs/befs/io.h
> index 9b78266..78d7bc6 100644
> --- a/fs/befs/io.h
> +++ b/fs/befs/io.h
> @@ -5,5 +5,3 @@
>  struct buffer_head *befs_bread_iaddr(struct super_block *sb,
>  				     befs_inode_addr iaddr);
>  
> -struct buffer_head *befs_bread(struct super_block *sb, befs_blocknr_t block);
> -
> diff --git a/fs/befs/linuxvfs.c b/fs/befs/linuxvfs.c
> index 2fea87b..6bc5b40 100644
> --- a/fs/befs/linuxvfs.c
> +++ b/fs/befs/linuxvfs.c
> @@ -319,7 +319,7 @@ static struct inode *befs_iget(struct super_block *sb, unsigned long ino)
>  		   befs_ino->i_inode_num.allocation_group,
>  		   befs_ino->i_inode_num.start, befs_ino->i_inode_num.len);
>  
> -	bh = befs_bread(sb, inode->i_ino);
> +	bh = sb_bread(sb, inode->i_ino);
>  	if (!bh) {
>  		befs_error(sb, "unable to read inode block - "
>  			   "inode = %lu", inode->i_ino);
> 

This simplifies the code.

The only thing you lose is this line:
befs_debug(sb, "---> Enter %s %lu", __func__, (unsigned long)block);

Which doesn't have much use when the function doesn't exist.

All calls of befs_bread() later check if the return is NULL and write a
befs_error() if its. So you keep all error checking, which is important.

I think it is a good idea. I don't foresee new calls to befs_bread() added
that would justify its existence.

Acked-by: Luis de Bethencourt <luisbg@osg.samsung.com>

Thanks Salah,
Luis