[PATCH v2] fs/buffer: Remove redundant assignment to err

[PATCH v2] fs/buffer: Remove redundant assignment to err

[Jiapeng Chong]

Variable 'err' set but not used.

fs/buffer.c:2613:2: warning: Value stored to 'err' is never read.

Reported-by: Abaci Robot <abaci@linux.alibaba.com>
Link: https://bugzilla.openanolis.cn/show_bug.cgi?id=4589
Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>
---
Changes in v2:
  -Remove unused out label.

 fs/buffer.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/fs/buffer.c b/fs/buffer.c
index d759b105c1e7..b3eb905f87d6 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -2580,7 +2580,7 @@ int block_truncate_page(struct address_space *mapping,
 	struct inode *inode = mapping->host;
 	struct page *page;
 	struct buffer_head *bh;
-	int err;
+	int err = 0;
 
 	blocksize = i_blocksize(inode);
 	length = offset & (blocksize - 1);
@@ -2593,9 +2593,8 @@ int block_truncate_page(struct address_space *mapping,
 	iblock = (sector_t)index << (PAGE_SHIFT - inode->i_blkbits);
 	
 	page = grab_cache_page(mapping, index);
-	err = -ENOMEM;
 	if (!page)
-		goto out;
+		return -ENOMEM;
 
 	if (!page_has_buffers(page))
 		create_empty_buffers(page, blocksize, 0);
@@ -2609,7 +2608,6 @@ int block_truncate_page(struct address_space *mapping,
 		pos += blocksize;
 	}
 
-	err = 0;
 	if (!buffer_mapped(bh)) {
 		WARN_ON(bh->b_size != blocksize);
 		err = get_block(inode, iblock, bh, 0);
@@ -2633,12 +2631,11 @@ int block_truncate_page(struct address_space *mapping,
 
 	zero_user(page, offset, length);
 	mark_buffer_dirty(bh);
-	err = 0;
 
 unlock:
 	unlock_page(page);
 	put_page(page);
-out:
+
 	return err;
 }
 EXPORT_SYMBOL(block_truncate_page);
-- 
2.20.1.7.g153144c

Re: [PATCH v2] fs/buffer: Remove redundant assignment to err

[Christian Brauner]

On Thu, 23 Mar 2023 10:32:59 +0800, Jiapeng Chong wrote:
> Variable 'err' set but not used.
> 
> fs/buffer.c:2613:2: warning: Value stored to 'err' is never read.
> 
> 

Applied to

tree: git://git.kernel.org/pub/scm/linux/kernel/git/vfs/idmapping.git
branch: fs.misc
[1/1] fs/buffer: Remove redundant assignment to err
      commit: dc7cb2d29805fe4fa4000fc0b09740fc24c93408

Thanks!
Christian

Re: [PATCH v2] fs/buffer: Remove redundant assignment to err

[Matthew Wilcox]

On Mon, Mar 27, 2023 at 10:10:10AM +0200, Christian Brauner wrote:
> 
> On Thu, 23 Mar 2023 10:32:59 +0800, Jiapeng Chong wrote:
> > Variable 'err' set but not used.
> > 
> > fs/buffer.c:2613:2: warning: Value stored to 'err' is never read.
> > 
> > 
> 
> Applied to

I think you should exercise extreme care with patches from "Abaci Robot".
It's wrong more often than it's right, and the people interpreting the
output from it do not appear to be experienced programmers.

Re: [PATCH v2] fs/buffer: Remove redundant assignment to err

[Christian Brauner]

On Mon, Apr 03, 2023 at 05:13:19PM +0100, Matthew Wilcox wrote:
> On Mon, Mar 27, 2023 at 10:10:10AM +0200, Christian Brauner wrote:
> > 
> > On Thu, 23 Mar 2023 10:32:59 +0800, Jiapeng Chong wrote:
> > > Variable 'err' set but not used.
> > > 
> > > fs/buffer.c:2613:2: warning: Value stored to 'err' is never read.
> > > 
> > > 
> > 
> > Applied to
> 
> I think you should exercise extreme care with patches from "Abaci Robot".
> It's wrong more often than it's right, and the people interpreting the
> output from it do not appear to be experienced programmers.

Thank you! I've tried to be extra careful with these patches and will
continue to do so.

Re: [PATCH v2] fs/buffer: Remove redundant assignment to err

[Jan Kara]

On Thu 23-03-23 10:32:59, Jiapeng Chong wrote:
> Variable 'err' set but not used.
> 
> fs/buffer.c:2613:2: warning: Value stored to 'err' is never read.
> 
> Reported-by: Abaci Robot <abaci@linux.alibaba.com>
> Link: https://bugzilla.openanolis.cn/show_bug.cgi?id=4589
> Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>

I don't think the patch is quite correct (Christian, please drop it if I'm
correct). See below:

> diff --git a/fs/buffer.c b/fs/buffer.c
> index d759b105c1e7..b3eb905f87d6 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -2580,7 +2580,7 @@ int block_truncate_page(struct address_space *mapping,
>  	struct inode *inode = mapping->host;
>  	struct page *page;
>  	struct buffer_head *bh;
> -	int err;
> +	int err = 0;
>  
>  	blocksize = i_blocksize(inode);
>  	length = offset & (blocksize - 1);
> @@ -2593,9 +2593,8 @@ int block_truncate_page(struct address_space *mapping,
>  	iblock = (sector_t)index << (PAGE_SHIFT - inode->i_blkbits);
>  	
>  	page = grab_cache_page(mapping, index);
> -	err = -ENOMEM;
>  	if (!page)
> -		goto out;
> +		return -ENOMEM;
>  
>  	if (!page_has_buffers(page))
>  		create_empty_buffers(page, blocksize, 0);
> @@ -2609,7 +2608,6 @@ int block_truncate_page(struct address_space *mapping,
>  		pos += blocksize;
>  	}
>  
> -	err = 0;
>  	if (!buffer_mapped(bh)) {
>  		WARN_ON(bh->b_size != blocksize);
>  		err = get_block(inode, iblock, bh, 0);
> @@ -2633,12 +2631,11 @@ int block_truncate_page(struct address_space *mapping,
>  
>  	zero_user(page, offset, length);
>  	mark_buffer_dirty(bh);
> -	err = 0;

There is:

        if (!buffer_uptodate(bh) && !buffer_delay(bh) && !buffer_unwritten(bh))
                err = -EIO;

above this assignment. So now we'll be returning -EIO if
block_truncate_page() needs to read the block AFAICT. Did this pass fstests
with some filesystem exercising this code (ext2 driver comes to mind)?

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2] fs/buffer: Remove redundant assignment to err

[Jan Kara]

On Mon 03-04-23 18:10:43, Jan Kara wrote:
> On Thu 23-03-23 10:32:59, Jiapeng Chong wrote:
> > Variable 'err' set but not used.
> > 
> > fs/buffer.c:2613:2: warning: Value stored to 'err' is never read.
> > 
> > Reported-by: Abaci Robot <abaci@linux.alibaba.com>
> > Link: https://bugzilla.openanolis.cn/show_bug.cgi?id=4589
> > Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>
> 
> I don't think the patch is quite correct (Christian, please drop it if I'm
> correct). See below:

Ah, sorry. I had too old kernel accidentally checked out. The patch is fine
with current upstream. Sorry for the noise!

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH v2] fs/buffer: Remove redundant assignment to err

[Christian Brauner]

On Mon, Apr 03, 2023 at 06:38:02PM +0200, Jan Kara wrote:
> On Mon 03-04-23 18:10:43, Jan Kara wrote:
> > On Thu 23-03-23 10:32:59, Jiapeng Chong wrote:
> > > Variable 'err' set but not used.
> > > 
> > > fs/buffer.c:2613:2: warning: Value stored to 'err' is never read.
> > > 
> > > Reported-by: Abaci Robot <abaci@linux.alibaba.com>
> > > Link: https://bugzilla.openanolis.cn/show_bug.cgi?id=4589
> > > Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>
> > 
> > I don't think the patch is quite correct (Christian, please drop it if I'm
> > correct). See below:
> 
> Ah, sorry. I had too old kernel accidentally checked out. The patch is fine
> with current upstream. Sorry for the noise!

No problem at all. I'm very happy that you took the time to review this.
This is very much needed!

Christian

Re: [PATCH v2] fs/buffer: Remove redundant assignment to err

[Christian Brauner]

On Mon, Apr 03, 2023 at 06:10:43PM +0200, Jan Kara wrote:
> On Thu 23-03-23 10:32:59, Jiapeng Chong wrote:
> > Variable 'err' set but not used.
> > 
> > fs/buffer.c:2613:2: warning: Value stored to 'err' is never read.
> > 
> > Reported-by: Abaci Robot <abaci@linux.alibaba.com>
> > Link: https://bugzilla.openanolis.cn/show_bug.cgi?id=4589
> > Signed-off-by: Jiapeng Chong <jiapeng.chong@linux.alibaba.com>
> 
> I don't think the patch is quite correct (Christian, please drop it if I'm
> correct). See below:

Thank you for taking a look at this!

> 
> > diff --git a/fs/buffer.c b/fs/buffer.c
> > index d759b105c1e7..b3eb905f87d6 100644
> > --- a/fs/buffer.c
> > +++ b/fs/buffer.c
> > @@ -2580,7 +2580,7 @@ int block_truncate_page(struct address_space *mapping,
> >  	struct inode *inode = mapping->host;
> >  	struct page *page;
> >  	struct buffer_head *bh;
> > -	int err;
> > +	int err = 0;
> >  
> >  	blocksize = i_blocksize(inode);
> >  	length = offset & (blocksize - 1);
> > @@ -2593,9 +2593,8 @@ int block_truncate_page(struct address_space *mapping,
> >  	iblock = (sector_t)index << (PAGE_SHIFT - inode->i_blkbits);
> >  	
> >  	page = grab_cache_page(mapping, index);
> > -	err = -ENOMEM;
> >  	if (!page)
> > -		goto out;
> > +		return -ENOMEM;
> >  
> >  	if (!page_has_buffers(page))
> >  		create_empty_buffers(page, blocksize, 0);
> > @@ -2609,7 +2608,6 @@ int block_truncate_page(struct address_space *mapping,
> >  		pos += blocksize;
> >  	}
> >  
> > -	err = 0;
> >  	if (!buffer_mapped(bh)) {
> >  		WARN_ON(bh->b_size != blocksize);
> >  		err = get_block(inode, iblock, bh, 0);
> > @@ -2633,12 +2631,11 @@ int block_truncate_page(struct address_space *mapping,
> >  
> >  	zero_user(page, offset, length);
> >  	mark_buffer_dirty(bh);
> > -	err = 0;
> 
> There is:
> 
>         if (!buffer_uptodate(bh) && !buffer_delay(bh) && !buffer_unwritten(bh))
>                 err = -EIO;
> 
> above this assignment. So now we'll be returning -EIO if
> block_truncate_page() needs to read the block AFAICT. Did this pass fstests
> with some filesystem exercising this code (ext2 driver comes to mind)?

Hm, the code in current mainline is:

        if (!buffer_uptodate(bh) && !buffer_delay(bh) && !buffer_unwritten(bh)) {
                err = bh_read(bh, 0);
                /* Uhhuh. Read error. Complain and punt. */
                if (err < 0)
                        goto unlock;
        }

Before e7ea1129afab ("fs/buffer: replace ll_rw_block()") that code really was

        if (!buffer_uptodate(bh) && !buffer_delay(bh) && !buffer_unwritten(bh)) {
                err = -EIO;
                ll_rw_block(REQ_OP_READ, 1, &bh);
                wait_on_buffer(bh);
                /* Uhhuh. Read error. Complain and punt. */
                if (!buffer_uptodate(bh))
                        goto unlock;
        }

which would've indeed caused this change to return -EIO.
Is this still an issue now? Sorry if I'm being dense here.