This allocation should use the passed in GFP_ flags instead of
GFP_KERNEL. All the callers that I reviewed passed GFP_KERNEL as the
allocation flags so this might not affect runtime, but it's still worth
cleaning up.
Fixes: 5c83746a0cf2 ("pnfs/blocklayout: in-kernel GETDEVICEINFO XDR parsing")
Signed-off-by: Dan Carpenter <[email protected]>
---
fs/nfs/blocklayout/dev.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/fs/nfs/blocklayout/dev.c b/fs/nfs/blocklayout/dev.c
index 70f5563a8e81..65cbb5607a5f 100644
--- a/fs/nfs/blocklayout/dev.c
+++ b/fs/nfs/blocklayout/dev.c
@@ -404,7 +404,7 @@ bl_parse_concat(struct nfs_server *server, struct pnfs_block_dev *d,
int ret, i;
d->children = kcalloc(v->concat.volumes_count,
- sizeof(struct pnfs_block_dev), GFP_KERNEL);
+ sizeof(struct pnfs_block_dev), gfp_mask);
if (!d->children)
return -ENOMEM;
@@ -433,7 +433,7 @@ bl_parse_stripe(struct nfs_server *server, struct pnfs_block_dev *d,
int ret, i;
d->children = kcalloc(v->stripe.volumes_count,
- sizeof(struct pnfs_block_dev), GFP_KERNEL);
+ sizeof(struct pnfs_block_dev), gfp_mask);
if (!d->children)
return -ENOMEM;
--
2.39.2
Hi Dan,
On Fri, Jul 21, 2023 at 10:58 AM Dan Carpenter <[email protected]> wrote:
>
> This allocation should use the passed in GFP_ flags instead of
> GFP_KERNEL. All the callers that I reviewed passed GFP_KERNEL as the
> allocation flags so this might not affect runtime, but it's still worth
> cleaning up.
If all the callers are passing GFP_KERNEL anyway, then can we instead
remove the gfp_mask argument from these functions?
Anna
>
> Fixes: 5c83746a0cf2 ("pnfs/blocklayout: in-kernel GETDEVICEINFO XDR parsing")
> Signed-off-by: Dan Carpenter <[email protected]>
> ---
> fs/nfs/blocklayout/dev.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/fs/nfs/blocklayout/dev.c b/fs/nfs/blocklayout/dev.c
> index 70f5563a8e81..65cbb5607a5f 100644
> --- a/fs/nfs/blocklayout/dev.c
> +++ b/fs/nfs/blocklayout/dev.c
> @@ -404,7 +404,7 @@ bl_parse_concat(struct nfs_server *server, struct pnfs_block_dev *d,
> int ret, i;
>
> d->children = kcalloc(v->concat.volumes_count,
> - sizeof(struct pnfs_block_dev), GFP_KERNEL);
> + sizeof(struct pnfs_block_dev), gfp_mask);
> if (!d->children)
> return -ENOMEM;
>
> @@ -433,7 +433,7 @@ bl_parse_stripe(struct nfs_server *server, struct pnfs_block_dev *d,
> int ret, i;
>
> d->children = kcalloc(v->stripe.volumes_count,
> - sizeof(struct pnfs_block_dev), GFP_KERNEL);
> + sizeof(struct pnfs_block_dev), gfp_mask);
> if (!d->children)
> return -ENOMEM;
>
> --
> 2.39.2
>
Le 21/07/2023 à 19:28, Anna Schumaker a écrit :
> Hi Dan,
>
> On Fri, Jul 21, 2023 at 10:58 AM Dan Carpenter <[email protected]> wrote:
>>
>> This allocation should use the passed in GFP_ flags instead of
>> GFP_KERNEL. All the callers that I reviewed passed GFP_KERNEL as the
>> allocation flags so this might not affect runtime, but it's still worth
>> cleaning up.
>
> If all the callers are passing GFP_KERNEL anyway, then can we instead
> remove the gfp_mask argument from these functions?
Hi,
I won't be able to remind the (verrrrrryyyyy long) call chain, but I
managed to arrive up to [1].
So, *if I'm right*, 'gfp_mask' is NOT always GFP_KERNEL.
[1]:
https://elixir.bootlin.com/linux/v6.5-rc1/source/fs/nfs/filelayout/filelayout.c#L904
Just my 2c,
CJ
>
> Anna
>
>>
>> Fixes: 5c83746a0cf2 ("pnfs/blocklayout: in-kernel GETDEVICEINFO XDR parsing")
>> Signed-off-by: Dan Carpenter <[email protected]>
>> ---
>> fs/nfs/blocklayout/dev.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/fs/nfs/blocklayout/dev.c b/fs/nfs/blocklayout/dev.c
>> index 70f5563a8e81..65cbb5607a5f 100644
>> --- a/fs/nfs/blocklayout/dev.c
>> +++ b/fs/nfs/blocklayout/dev.c
>> @@ -404,7 +404,7 @@ bl_parse_concat(struct nfs_server *server, struct pnfs_block_dev *d,
>> int ret, i;
>>
>> d->children = kcalloc(v->concat.volumes_count,
>> - sizeof(struct pnfs_block_dev), GFP_KERNEL);
>> + sizeof(struct pnfs_block_dev), gfp_mask);
>> if (!d->children)
>> return -ENOMEM;
>>
>> @@ -433,7 +433,7 @@ bl_parse_stripe(struct nfs_server *server, struct pnfs_block_dev *d,
>> int ret, i;
>>
>> d->children = kcalloc(v->stripe.volumes_count,
>> - sizeof(struct pnfs_block_dev), GFP_KERNEL);
>> + sizeof(struct pnfs_block_dev), gfp_mask);
>> if (!d->children)
>> return -ENOMEM;
>>
>> --
>> 2.39.2
>>
>
Le 21/07/2023 à 19:59, Christophe JAILLET a écrit :
> Le 21/07/2023 à 19:28, Anna Schumaker a écrit :
>> Hi Dan,
>>
>> On Fri, Jul 21, 2023 at 10:58 AM Dan Carpenter
>> <[email protected]> wrote:
>>>
>>> This allocation should use the passed in GFP_ flags instead of
>>> GFP_KERNEL. All the callers that I reviewed passed GFP_KERNEL as the
>>> allocation flags so this might not affect runtime, but it's still worth
>>> cleaning up.
>>
>> If all the callers are passing GFP_KERNEL anyway, then can we instead
>> remove the gfp_mask argument from these functions?
>
> Hi,
>
> I won't be able to remind the (verrrrrryyyyy long) call chain, but I
> managed to arrive up to [1].
> So, *if I'm right*, 'gfp_mask' is NOT always GFP_KERNEL.
I can't remind the call chain myself, but my browser history can.
gfp_mask is propagated in all the below functions:
filelayout_pg_init_write()
--> fl_pnfs_update_layout(..., GFP_NOFS)
--> filelayout_check_deviceid()
--> nfs4_find_get_deviceid()
--> nfs4_get_device_info()
--> alloc_deviceid_node() function pointer in struct pnfs_layoutdriver_type
--> bl_alloc_deviceid_node()
--> bl_parse_deviceid()
--> bl_parse_slice() / bl_parse_concat()
CJ
>
> [1]:
> https://elixir.bootlin.com/linux/v6.5-rc1/source/fs/nfs/filelayout/filelayout.c#L904
>
> Just my 2c,
>
> CJ
>
>>
>> Anna
>>
>>>
>>> Fixes: 5c83746a0cf2 ("pnfs/blocklayout: in-kernel GETDEVICEINFO XDR
>>> parsing")
>>> Signed-off-by: Dan Carpenter
>>> <[email protected]>
>>> ---
>>> fs/nfs/blocklayout/dev.c | 4 ++--
>>> 1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/fs/nfs/blocklayout/dev.c b/fs/nfs/blocklayout/dev.c
>>> index 70f5563a8e81..65cbb5607a5f 100644
>>> --- a/fs/nfs/blocklayout/dev.c
>>> +++ b/fs/nfs/blocklayout/dev.c
>>> @@ -404,7 +404,7 @@ bl_parse_concat(struct nfs_server *server, struct
>>> pnfs_block_dev *d,
>>> int ret, i;
>>>
>>> d->children = kcalloc(v->concat.volumes_count,
>>> - sizeof(struct pnfs_block_dev), GFP_KERNEL);
>>> + sizeof(struct pnfs_block_dev), gfp_mask);
>>> if (!d->children)
>>> return -ENOMEM;
>>>
>>> @@ -433,7 +433,7 @@ bl_parse_stripe(struct nfs_server *server, struct
>>> pnfs_block_dev *d,
>>> int ret, i;
>>>
>>> d->children = kcalloc(v->stripe.volumes_count,
>>> - sizeof(struct pnfs_block_dev), GFP_KERNEL);
>>> + sizeof(struct pnfs_block_dev), gfp_mask);
>>> if (!d->children)
>>> return -ENOMEM;
>>>
>>> --
>>> 2.39.2
>>>
>>
>
>
On Fri, Jul 21, 2023 at 08:14:03PM +0200, Marion & Christophe JAILLET wrote:
> I can't remind the call chain myself, but my browser history can.
>
> gfp_mask is propagated in all the below functions:
>
> filelayout_pg_init_write()
> --> fl_pnfs_update_layout(..., GFP_NOFS)
> --> filelayout_check_deviceid()
> --> nfs4_find_get_deviceid()
> --> nfs4_get_device_info()
> --> alloc_deviceid_node() function pointer in struct pnfs_layoutdriver_type
> --> bl_alloc_deviceid_node()
> --> bl_parse_deviceid()
> --> bl_parse_slice() / bl_parse_concat()
>
> CJ
Thanks Christophe,
Let me re-send with a corrected commit message.
regards,
dan carpenter