| Message ID | e655db6f-471b-4184-8907-0551e909acbb@moroto.mountain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | nfs/blocklayout: Use the passed in gfp flags | expand |
Hi Dan, On Fri, Jul 21, 2023 at 10:58 AM Dan Carpenter <dan.carpenter@linaro.org> 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 <dan.carpenter@linaro.org> > --- > 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 <dan.carpenter@linaro.org> 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 <dan.carpenter-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> >> --- >> 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 >> <dan.carpenter@linaro.org> 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 >>> <dan.carpenter-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> >>> --- >>> 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
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;
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 <dan.carpenter@linaro.org> --- fs/nfs/blocklayout/dev.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)