2022-10-25 11:05:34

by Li zeming

Subject: [PATCH v2] btrfs: volumes: Increase bioc pointer check

This patch has the following changes:
1. Modify "is returned" in the comments to "should be returned".
2. Remove the __GFP_NOFAIL flag from the kzalloc function, which returns
NULL if kzalloc fails to allocate memory for bioc.

Reviewed-by: Qu Wenruo <[email protected]>
Signed-off-by: Li zeming <[email protected]>
---
v2: Add annotation vocabulary modify, remove __GFP_NOFAIL flag.

fs/btrfs/volumes.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
index 064ab2a79c80..b8d901f58995 100644
--- a/fs/btrfs/volumes.c
+++ b/fs/btrfs/volumes.c
@@ -5891,7 +5891,9 @@ static struct btrfs_io_context *alloc_btrfs_io_context(struct btrfs_fs_info *fs_
* and the stripes.
*/
sizeof(u64) * (total_stripes),
- GFP_NOFS|__GFP_NOFAIL);
+ GFP_NOFS);
+ if (!bioc)
+ return NULL;

atomic_set(&bioc->error, 0);
refcount_set(&bioc->refs, 1);
@@ -6071,7 +6073,7 @@ struct btrfs_discard_stripe *btrfs_map_discard(struct btrfs_fs_info *fs_info,
* array of stripes.
* For READ, it also needs to be supported using the same mirror number.
*
- * If the requested block is not left of the left cursor, EIO is returned. This
+ * If the requested block is not left of the left cursor, EIO should be returned. This
* can happen because btrfs_num_copies() returns one more in the dev-replace
* case.
*/
--
2.18.2

2022-10-25 11:46:20

by Qu Wenruo

Subject: Re: [PATCH v2] btrfs: volumes: Increase bioc pointer check

On 2022/10/25 18:52, Li zeming wrote:
> This patch has the following changes:
> 1. Modify "is returned" in the comments to "should be returned".
> 2. Remove the __GFP_NOFAIL flag from the kzalloc function, which returns
> NULL if kzalloc fails to allocate memory for bioc.

Firstly this part should be in change log, not commit message.

You can just do a search in the mail list and see how we handle patches
with newer versions.

Secondly, you didn't mention why we can remove the __GFP_NOFAIL flag at all.

The commit message should look like this instead:

```
Currently we allocate memory for btrfs_io_context using
(GFP_NOFS|__GFP_NOFAIL) in alloc_btrfs_io_context().

But there is nothing special for that function to require NOFAIL flag.

Furthermore the only caller of alloc_btrfs_io_context() is already
handling the ENOMEM error properly.

Thus we can safely remove the __GFP_NOFAIL flag, and handle allocation
failure properly.
```
>
> Reviewed-by: Qu Wenruo <[email protected]>

I'd say, please don't add my tag until everything is fine.
I did a wrong expectation.

Thanks,
Qu

> Signed-off-by: Li zeming <[email protected]>
> ---
> v2: Add annotation vocabulary modify, remove __GFP_NOFAIL flag.
>
> fs/btrfs/volumes.c | 6 ++++--
> 1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/fs/btrfs/volumes.c b/fs/btrfs/volumes.c
> index 064ab2a79c80..b8d901f58995 100644
> --- a/fs/btrfs/volumes.c
> +++ b/fs/btrfs/volumes.c
> @@ -5891,7 +5891,9 @@ static struct btrfs_io_context *alloc_btrfs_io_context(struct btrfs_fs_info *fs_
> * and the stripes.
> */
> sizeof(u64) * (total_stripes),
> - GFP_NOFS|__GFP_NOFAIL);
> + GFP_NOFS);
> + if (!bioc)
> + return NULL;
>
> atomic_set(&bioc->error, 0);
> refcount_set(&bioc->refs, 1);
> @@ -6071,7 +6073,7 @@ struct btrfs_discard_stripe *btrfs_map_discard(struct btrfs_fs_info *fs_info,
> * array of stripes.
> * For READ, it also needs to be supported using the same mirror number.
> *
> - * If the requested block is not left of the left cursor, EIO is returned. This
> + * If the requested block is not left of the left cursor, EIO should be returned. This
> * can happen because btrfs_num_copies() returns one more in the dev-replace
> * case.
> */