Rename grow_dev_page() to grow_dev_folio() and make it return a bool.
Document what that bool means; it's more subtle than it first appears.
Also rename the 'failed' label to 'unlock' beacuse it's not exactly
'failed'. It just hasn't succeeded.
Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
---
fs/buffer.c | 39 ++++++++++++++++++++++-----------------
1 file changed, 22 insertions(+), 17 deletions(-)
diff --git a/fs/buffer.c b/fs/buffer.c
index b33cc74e9649..dec41d84044b 100644
--- a/fs/buffer.c
+++ b/fs/buffer.c
@@ -1024,24 +1024,26 @@ static sector_t folio_init_buffers(struct folio *folio,
}
/*
- * Create the page-cache page that contains the requested block.
+ * Create the page-cache folio that contains the requested block.
*
* This is used purely for blockdev mappings.
+ *
+ * Returns false if we have a 'permanent' failure. Returns true if
+ * we succeeded, or the caller should retry.
*/
-static int
-grow_dev_page(struct block_device *bdev, sector_t block,
+static bool grow_dev_folio(struct block_device *bdev, sector_t block,
pgoff_t index, int size, int sizebits, gfp_t gfp)
{
struct inode *inode = bdev->bd_inode;
struct folio *folio;
struct buffer_head *bh;
sector_t end_block;
- int ret = 0;
+ bool ret;
folio = __filemap_get_folio(inode->i_mapping, index,
FGP_LOCK | FGP_ACCESSED | FGP_CREAT, gfp);
if (IS_ERR(folio))
- return PTR_ERR(folio);
+ return false;
bh = folio_buffers(folio);
if (bh) {
@@ -1050,14 +1052,17 @@ grow_dev_page(struct block_device *bdev, sector_t block,
(sector_t)index << sizebits, size);
goto done;
}
+
+ /* Caller should retry if this call fails */
+ ret = true;
if (!try_to_free_buffers(folio))
- goto failed;
+ goto unlock;
}
- ret = -ENOMEM;
+ ret = false;
bh = folio_alloc_buffers(folio, size, gfp | __GFP_ACCOUNT);
if (!bh)
- goto failed;
+ goto unlock;
/*
* Link the folio to the buffers and initialise them. Take the
@@ -1070,19 +1075,19 @@ grow_dev_page(struct block_device *bdev, sector_t block,
(sector_t)index << sizebits, size);
spin_unlock(&inode->i_mapping->private_lock);
done:
- ret = (block < end_block) ? 1 : -ENXIO;
-failed:
+ ret = block < end_block;
+unlock:
folio_unlock(folio);
folio_put(folio);
return ret;
}
/*
- * Create buffers for the specified block device block's page. If
- * that page was dirty, the buffers are set dirty also.
+ * Create buffers for the specified block device block's folio. If
+ * that folio was dirty, the buffers are set dirty also.
*/
-static int
-grow_buffers(struct block_device *bdev, sector_t block, int size, gfp_t gfp)
+static bool grow_buffers(struct block_device *bdev, sector_t block,
+ int size, gfp_t gfp)
{
pgoff_t index;
int sizebits;
@@ -1099,11 +1104,11 @@ grow_buffers(struct block_device *bdev, sector_t block, int size, gfp_t gfp)
"device %pg\n",
__func__, (unsigned long long)block,
bdev);
- return -EIO;
+ return false;
}
- /* Create a page with the proper size buffers.. */
- return grow_dev_page(bdev, block, index, size, sizebits, gfp);
+ /* Create a folio with the proper size buffers.. */
+ return grow_dev_folio(bdev, block, index, size, sizebits, gfp);
}
static struct buffer_head *
--
2.40.1
On Tue, Oct 17, 2023 at 5:11 AM Matthew Wilcox (Oracle) wrote:
>
> Rename grow_dev_page() to grow_dev_folio() and make it return a bool.
> Document what that bool means; it's more subtle than it first appears.
> Also rename the 'failed' label to 'unlock' beacuse it's not exactly
> 'failed'. It just hasn't succeeded.
>
> Signed-off-by: Matthew Wilcox (Oracle) <[email protected]>
> ---
> fs/buffer.c | 39 ++++++++++++++++++++++-----------------
> 1 file changed, 22 insertions(+), 17 deletions(-)
>
> diff --git a/fs/buffer.c b/fs/buffer.c
> index b33cc74e9649..dec41d84044b 100644
> --- a/fs/buffer.c
> +++ b/fs/buffer.c
> @@ -1024,24 +1024,26 @@ static sector_t folio_init_buffers(struct folio *folio,
> }
>
> /*
> - * Create the page-cache page that contains the requested block.
> + * Create the page-cache folio that contains the requested block.
> *
> * This is used purely for blockdev mappings.
> + *
> + * Returns false if we have a 'permanent' failure. Returns true if
> + * we succeeded, or the caller should retry.
> */
> -static int
> -grow_dev_page(struct block_device *bdev, sector_t block,
> +static bool grow_dev_folio(struct block_device *bdev, sector_t block,
> pgoff_t index, int size, int sizebits, gfp_t gfp)
> {
> struct inode *inode = bdev->bd_inode;
> struct folio *folio;
> struct buffer_head *bh;
> sector_t end_block;
> - int ret = 0;
> + bool ret;
>
> folio = __filemap_get_folio(inode->i_mapping, index,
> FGP_LOCK | FGP_ACCESSED | FGP_CREAT, gfp);
> if (IS_ERR(folio))
> - return PTR_ERR(folio);
> + return false;
>
> bh = folio_buffers(folio);
> if (bh) {
> @@ -1050,14 +1052,17 @@ grow_dev_page(struct block_device *bdev, sector_t block,
> (sector_t)index << sizebits, size);
> goto done;
> }
> +
> + /* Caller should retry if this call fails */
> + ret = true;
> if (!try_to_free_buffers(folio))
> - goto failed;
> + goto unlock;
> }
>
> - ret = -ENOMEM;
> + ret = false;
> bh = folio_alloc_buffers(folio, size, gfp | __GFP_ACCOUNT);
> if (!bh)
> - goto failed;
> + goto unlock;
>
> /*
> * Link the folio to the buffers and initialise them. Take the
> @@ -1070,19 +1075,19 @@ grow_dev_page(struct block_device *bdev, sector_t block,
> (sector_t)index << sizebits, size);
> spin_unlock(&inode->i_mapping->private_lock);
> done:
> - ret = (block < end_block) ? 1 : -ENXIO;
> -failed:
> + ret = block < end_block;
> +unlock:
> folio_unlock(folio);
> folio_put(folio);
> return ret;
> }
>
> /*
> - * Create buffers for the specified block device block's page. If
> - * that page was dirty, the buffers are set dirty also.
> + * Create buffers for the specified block device block's folio. If
> + * that folio was dirty, the buffers are set dirty also.
> */
> -static int
> -grow_buffers(struct block_device *bdev, sector_t block, int size, gfp_t gfp)
> +static bool grow_buffers(struct block_device *bdev, sector_t block,
> + int size, gfp_t gfp)
> {
> pgoff_t index;
> int sizebits;
> @@ -1099,11 +1104,11 @@ grow_buffers(struct block_device *bdev, sector_t block, int size, gfp_t gfp)
> "device %pg\n",
> __func__, (unsigned long long)block,
> bdev);
> - return -EIO;
> + return false;
> }
>
> - /* Create a page with the proper size buffers.. */
> - return grow_dev_page(bdev, block, index, size, sizebits, gfp);
> + /* Create a folio with the proper size buffers.. */
> + return grow_dev_folio(bdev, block, index, size, sizebits, gfp);
> }
>
> static struct buffer_head *
> --
> 2.40.1
This changes the return type of grow_buffers() from "int" to "bool".
But, it seems that the caller, __getblk_slow(), has not changed the
type of the variable "ret" that receives its return value:
for (;;) {
struct buffer_head *bh;
int ret;
bh = __find_get_block(bdev, block, size);
if (bh)
return bh;
ret = grow_buffers(bdev, block, size, gfp);
if (ret < 0)
return NULL;
}
So, it looks like the error check immediately after calling
grow_buffers() will not branch like before.
Is this okay ? Or, am I missing some other changes?
Also, there is a typo in the changelog: "beacuse" -> "because".
Regards,
Ryusuke Konishi
On Wed, Oct 18, 2023 at 04:41:44AM +0900, Ryusuke Konishi wrote:
> On Tue, Oct 17, 2023 at 5:11 AM Matthew Wilcox (Oracle) wrote:
> >
> > Rename grow_dev_page() to grow_dev_folio() and make it return a bool.
> > Document what that bool means; it's more subtle than it first appears.
> > Also rename the 'failed' label to 'unlock' beacuse it's not exactly
> > 'failed'. It just hasn't succeeded.
>
> This changes the return type of grow_buffers() from "int" to "bool".
> But, it seems that the caller, __getblk_slow(), has not changed the
> type of the variable "ret" that receives its return value:
[...]
>
> So, it looks like the error check immediately after calling
> grow_buffers() will not branch like before.
> Is this okay ? Or, am I missing some other changes?
>
> Also, there is a typo in the changelog: "beacuse" -> "because".
Argh, yes. Andrew, please drop this patch for now. I'll submit
something better next cycle.