* [bug report] buffer: hoist GFP flags from grow_dev_page() to __getblk_gfp()
@ 2023-09-27 7:40 Dan Carpenter
2023-09-27 14:52 ` Matthew Wilcox
2023-09-27 15:02 ` Matthew Wilcox
0 siblings, 2 replies; 4+ messages in thread
From: Dan Carpenter @ 2023-09-27 7:40 UTC (permalink / raw)
To: willy; +Cc: linux-fsdevel
Hello Matthew Wilcox (Oracle),
The patch a3c38500d469: "buffer: hoist GFP flags from grow_dev_page()
to __getblk_gfp()" from Sep 14, 2023 (linux-next), leads to the
following Smatch static checker warning:
fs/buffer.c:1065 grow_dev_page()
warn: NEW missing error code 'ret'
fs/buffer.c
1037 static int
1038 grow_dev_page(struct block_device *bdev, sector_t block,
1039 pgoff_t index, int size, int sizebits, gfp_t gfp)
1040 {
1041 struct inode *inode = bdev->bd_inode;
1042 struct folio *folio;
1043 struct buffer_head *bh;
1044 sector_t end_block;
1045 int ret = 0;
1046
1047 folio = __filemap_get_folio(inode->i_mapping, index,
1048 FGP_LOCK | FGP_ACCESSED | FGP_CREAT, gfp);
1049 if (IS_ERR(folio))
1050 return PTR_ERR(folio);
1051
1052 bh = folio_buffers(folio);
1053 if (bh) {
1054 if (bh->b_size == size) {
1055 end_block = folio_init_buffers(folio, bdev,
1056 (sector_t)index << sizebits, size);
1057 goto done;
1058 }
1059 if (!try_to_free_buffers(folio))
1060 goto failed;
1061 }
1062
1063 bh = folio_alloc_buffers(folio, size, gfp | __GFP_ACCOUNT);
1064 if (!bh)
--> 1065 goto failed;
Should this be an error code? It's kind of complicated because I think
the other goto failed path deliberately returns zero?
1066
1067 /*
1068 * Link the folio to the buffers and initialise them. Take the
1069 * lock to be atomic wrt __find_get_block(), which does not
1070 * run under the folio lock.
1071 */
1072 spin_lock(&inode->i_mapping->private_lock);
1073 link_dev_buffers(folio, bh);
1074 end_block = folio_init_buffers(folio, bdev,
1075 (sector_t)index << sizebits, size);
1076 spin_unlock(&inode->i_mapping->private_lock);
1077 done:
1078 ret = (block < end_block) ? 1 : -ENXIO;
1079 failed:
1080 folio_unlock(folio);
1081 folio_put(folio);
1082 return ret;
1083 }
regards,
dan carpenter
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] buffer: hoist GFP flags from grow_dev_page() to __getblk_gfp()
2023-09-27 7:40 [bug report] buffer: hoist GFP flags from grow_dev_page() to __getblk_gfp() Dan Carpenter
@ 2023-09-27 14:52 ` Matthew Wilcox
2023-09-27 15:02 ` Matthew Wilcox
1 sibling, 0 replies; 4+ messages in thread
From: Matthew Wilcox @ 2023-09-27 14:52 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-fsdevel
On Wed, Sep 27, 2023 at 10:40:21AM +0300, Dan Carpenter wrote:
> The patch a3c38500d469: "buffer: hoist GFP flags from grow_dev_page()
> to __getblk_gfp()" from Sep 14, 2023 (linux-next), leads to the
> following Smatch static checker warning:
>
> fs/buffer.c:1065 grow_dev_page()
> warn: NEW missing error code 'ret'
Smatch is right.
> fs/buffer.c
> 1037 static int
> 1038 grow_dev_page(struct block_device *bdev, sector_t block,
> 1039 pgoff_t index, int size, int sizebits, gfp_t gfp)
> 1040 {
> 1041 struct inode *inode = bdev->bd_inode;
> 1042 struct folio *folio;
> 1043 struct buffer_head *bh;
> 1044 sector_t end_block;
> 1045 int ret = 0;
> 1046
> 1047 folio = __filemap_get_folio(inode->i_mapping, index,
> 1048 FGP_LOCK | FGP_ACCESSED | FGP_CREAT, gfp);
> 1049 if (IS_ERR(folio))
> 1050 return PTR_ERR(folio);
> 1051
> 1052 bh = folio_buffers(folio);
> 1053 if (bh) {
> 1054 if (bh->b_size == size) {
> 1055 end_block = folio_init_buffers(folio, bdev,
> 1056 (sector_t)index << sizebits, size);
> 1057 goto done;
> 1058 }
> 1059 if (!try_to_free_buffers(folio))
> 1060 goto failed;
> 1061 }
> 1062
> 1063 bh = folio_alloc_buffers(folio, size, gfp | __GFP_ACCOUNT);
> 1064 if (!bh)
> --> 1065 goto failed;
>
> Should this be an error code? It's kind of complicated because I think
> the other goto failed path deliberately returns zero?
Yes, it's very confusing. Which is a common refrain in this part of the
VFS. Thanks for the report, I'll sort it out.
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [bug report] buffer: hoist GFP flags from grow_dev_page() to __getblk_gfp()
2023-09-27 7:40 [bug report] buffer: hoist GFP flags from grow_dev_page() to __getblk_gfp() Dan Carpenter
2023-09-27 14:52 ` Matthew Wilcox
@ 2023-09-27 15:02 ` Matthew Wilcox
2023-09-27 15:03 ` Matthew Wilcox
1 sibling, 1 reply; 4+ messages in thread
From: Matthew Wilcox @ 2023-09-27 15:02 UTC (permalink / raw)
To: Dan Carpenter; +Cc: linux-fsdevel
On Wed, Sep 27, 2023 at 10:40:21AM +0300, Dan Carpenter wrote:
> Hello Matthew Wilcox (Oracle),
>
> The patch a3c38500d469: "buffer: hoist GFP flags from grow_dev_page()
> to __getblk_gfp()" from Sep 14, 2023 (linux-next), leads to the
> following Smatch static checker warning:
>
> fs/buffer.c:1065 grow_dev_page()
> warn: NEW missing error code 'ret'
Andrew, please add this -fix patch to "buffer: hoist GFP flags from
grow_dev_page() to __getblk_gfp()". I'll send a further cleanup patch
to make the return values a bit more meaningful.
diff --git a/fs/buffer.c b/fs/buffer.c
index 3fe293c9f3ca..b1610202eb5c 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -1060,6 +1060,7 @@ grow_dev_page(struct block_device *bdev, sector_t block,
goto failed;
}
+ ret = -ENOMEM;
bh = folio_alloc_buffers(folio, size, gfp | __GFP_ACCOUNT);
if (!bh)
goto failed;
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [bug report] buffer: hoist GFP flags from grow_dev_page() to __getblk_gfp()
2023-09-27 15:02 ` Matthew Wilcox
@ 2023-09-27 15:03 ` Matthew Wilcox
0 siblings, 0 replies; 4+ messages in thread
From: Matthew Wilcox @ 2023-09-27 15:03 UTC (permalink / raw)
To: Andrew Morton; +Cc: linux-fsdevel, Dan Carpenter
... you know, it helps if one actually ccs akpm when one want him to do
something ...
On Wed, Sep 27, 2023 at 04:02:40PM +0100, Matthew Wilcox wrote:
> On Wed, Sep 27, 2023 at 10:40:21AM +0300, Dan Carpenter wrote:
> > Hello Matthew Wilcox (Oracle),
> >
> > The patch a3c38500d469: "buffer: hoist GFP flags from grow_dev_page()
> > to __getblk_gfp()" from Sep 14, 2023 (linux-next), leads to the
> > following Smatch static checker warning:
> >
> > fs/buffer.c:1065 grow_dev_page()
> > warn: NEW missing error code 'ret'
>
> Andrew, please add this -fix patch to "buffer: hoist GFP flags from
> grow_dev_page() to __getblk_gfp()". I'll send a further cleanup patch
> to make the return values a bit more meaningful.
>
> diff --git a/fs/buffer.c b/fs/buffer.c
> index 3fe293c9f3ca..b1610202eb5c 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -1060,6 +1060,7 @@ grow_dev_page(struct block_device *bdev, sector_t block,
> goto failed;
> }
>
> + ret = -ENOMEM;
> bh = folio_alloc_buffers(folio, size, gfp | __GFP_ACCOUNT);
> if (!bh)
> goto failed;
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2023-09-27 15:03 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2023-09-27 7:40 [bug report] buffer: hoist GFP flags from grow_dev_page() to __getblk_gfp() Dan Carpenter
2023-09-27 14:52 ` Matthew Wilcox
2023-09-27 15:02 ` Matthew Wilcox
2023-09-27 15:03 ` Matthew Wilcox
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).