| Message ID | 20230323023259.6924-1-jiapeng.chong@linux.alibaba.com (mailing list archive) |
|---|---|
| State | Mainlined, archived |
| Headers | show |
| Series | [v2] fs/buffer: Remove redundant assignment to err | expand |
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
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
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.
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
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.
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
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.
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);
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(-)