| Message ID | 20181220154236.GC6063@fieldses.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sunrpc: convert unnecessary GFP_ATOMIC to GFP_NOFS | expand |
> On Dec 20, 2018, at 10:42 AM, J. Bruce Fields <bfields@fieldses.org> wrote: > > From: "J. Bruce Fields" <bfields@redhat.com> > > It's OK to sleep here, we just don't want to recurse into the filesystem > as this writeout could be waiting on this. "as a writeout" > Future work: the documentation for GFP_NOFS says "Please try to avoid > using this flag directly and instead use memalloc_nofs_{save,restore} to > mark the whole scope which cannot/shouldn't recurse into the FS layer > with a short explanation why. All allocation requests will inherit > GFP_NOFS implicitly." > > But I'm not sure where to do this. Should the workqueue be arranging > that for us in the case of workqueues created with WQ_MEM_RECLAIM? There seem to be plenty of uses of GFP_NOFS in NFS and sunrpc. That sounds like a big project. > Reported-by: Trond Myklebust <trondmy@hammer.space> > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > --- > net/sunrpc/rpcb_clnt.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > Also, I've still got this one. (And still haven't looked into whether > it should be using a memalloc_nofs_{save,restore} elsewhere instead.) > > diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c > index 08b5fa4a2852..41a971ac1c63 100644 > --- a/net/sunrpc/rpcb_clnt.c > +++ b/net/sunrpc/rpcb_clnt.c > @@ -752,7 +752,7 @@ void rpcb_getport_async(struct rpc_task *task) > goto bailout_nofree; > } > > - map = kzalloc(sizeof(struct rpcbind_args), GFP_ATOMIC); > + map = kzalloc(sizeof(struct rpcbind_args), GFP_NOFS); > if (!map) { > status = -ENOMEM; > dprintk("RPC: %5u %s: no memory available\n", > @@ -770,7 +770,7 @@ void rpcb_getport_async(struct rpc_task *task) > case RPCBVERS_4: > case RPCBVERS_3: > map->r_netid = xprt->address_strings[RPC_DISPLAY_NETID]; > - map->r_addr = rpc_sockaddr2uaddr(sap, GFP_ATOMIC); > + map->r_addr = rpc_sockaddr2uaddr(sap, GFP_NOFS); > if (!map->r_addr) { > status = -ENOMEM; > dprintk("RPC: %5u %s: no memory available\n", > -- > 2.19.2 > -- Chuck Lever
On Thu, Dec 20, 2018 at 10:47:25AM -0500, Chuck Lever wrote: > > > > On Dec 20, 2018, at 10:42 AM, J. Bruce Fields <bfields@fieldses.org> wrote: > > > > From: "J. Bruce Fields" <bfields@redhat.com> > > > > It's OK to sleep here, we just don't want to recurse into the filesystem > > as this writeout could be waiting on this. > > "as a writeout" Oops, thanks. > > Future work: the documentation for GFP_NOFS says "Please try to avoid > > using this flag directly and instead use memalloc_nofs_{save,restore} to > > mark the whole scope which cannot/shouldn't recurse into the FS layer > > with a short explanation why. All allocation requests will inherit > > GFP_NOFS implicitly." > > > > But I'm not sure where to do this. Should the workqueue be arranging > > that for us in the case of workqueues created with WQ_MEM_RECLAIM? > > There seem to be plenty of uses of GFP_NOFS in NFS and sunrpc. > That sounds like a big project. Yeah, just noting it for future reference. --b. > > Reported-by: Trond Myklebust <trondmy@hammer.space> > > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > > --- > > net/sunrpc/rpcb_clnt.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > Also, I've still got this one. (And still haven't looked into whether > > it should be using a memalloc_nofs_{save,restore} elsewhere instead.) > > > > diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c > > index 08b5fa4a2852..41a971ac1c63 100644 > > --- a/net/sunrpc/rpcb_clnt.c > > +++ b/net/sunrpc/rpcb_clnt.c > > @@ -752,7 +752,7 @@ void rpcb_getport_async(struct rpc_task *task) > > goto bailout_nofree; > > } > > > > - map = kzalloc(sizeof(struct rpcbind_args), GFP_ATOMIC); > > + map = kzalloc(sizeof(struct rpcbind_args), GFP_NOFS); > > if (!map) { > > status = -ENOMEM; > > dprintk("RPC: %5u %s: no memory available\n", > > @@ -770,7 +770,7 @@ void rpcb_getport_async(struct rpc_task *task) > > case RPCBVERS_4: > > case RPCBVERS_3: > > map->r_netid = xprt->address_strings[RPC_DISPLAY_NETID]; > > - map->r_addr = rpc_sockaddr2uaddr(sap, GFP_ATOMIC); > > + map->r_addr = rpc_sockaddr2uaddr(sap, GFP_NOFS); > > if (!map->r_addr) { > > status = -ENOMEM; > > dprintk("RPC: %5u %s: no memory available\n", > > -- > > 2.19.2 > > > > -- > Chuck Lever > >
On Thu, Dec 20, 2018 at 10:52:19AM -0500, Bruce Fields wrote: > On Thu, Dec 20, 2018 at 10:47:25AM -0500, Chuck Lever wrote: > > > > > > > On Dec 20, 2018, at 10:42 AM, J. Bruce Fields <bfields@fieldses.org> wrote: > > > > > > From: "J. Bruce Fields" <bfields@redhat.com> > > > > > > It's OK to sleep here, we just don't want to recurse into the filesystem > > > as this writeout could be waiting on this. > > > > "as a writeout" > > Oops, thanks. (Trond or Anna, I'm assuming you can fix that up, but let me know if you'd rather I resent.) --b. > > > > Future work: the documentation for GFP_NOFS says "Please try to avoid > > > using this flag directly and instead use memalloc_nofs_{save,restore} to > > > mark the whole scope which cannot/shouldn't recurse into the FS layer > > > with a short explanation why. All allocation requests will inherit > > > GFP_NOFS implicitly." > > > > > > But I'm not sure where to do this. Should the workqueue be arranging > > > that for us in the case of workqueues created with WQ_MEM_RECLAIM? > > > > There seem to be plenty of uses of GFP_NOFS in NFS and sunrpc. > > That sounds like a big project. > > Yeah, just noting it for future reference. > > --b. > > > > Reported-by: Trond Myklebust <trondmy@hammer.space> > > > Signed-off-by: J. Bruce Fields <bfields@redhat.com> > > > --- > > > net/sunrpc/rpcb_clnt.c | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > Also, I've still got this one. (And still haven't looked into whether > > > it should be using a memalloc_nofs_{save,restore} elsewhere instead.) > > > > > > diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c > > > index 08b5fa4a2852..41a971ac1c63 100644 > > > --- a/net/sunrpc/rpcb_clnt.c > > > +++ b/net/sunrpc/rpcb_clnt.c > > > @@ -752,7 +752,7 @@ void rpcb_getport_async(struct rpc_task *task) > > > goto bailout_nofree; > > > } > > > > > > - map = kzalloc(sizeof(struct rpcbind_args), GFP_ATOMIC); > > > + map = kzalloc(sizeof(struct rpcbind_args), GFP_NOFS); > > > if (!map) { > > > status = -ENOMEM; > > > dprintk("RPC: %5u %s: no memory available\n", > > > @@ -770,7 +770,7 @@ void rpcb_getport_async(struct rpc_task *task) > > > case RPCBVERS_4: > > > case RPCBVERS_3: > > > map->r_netid = xprt->address_strings[RPC_DISPLAY_NETID]; > > > - map->r_addr = rpc_sockaddr2uaddr(sap, GFP_ATOMIC); > > > + map->r_addr = rpc_sockaddr2uaddr(sap, GFP_NOFS); > > > if (!map->r_addr) { > > > status = -ENOMEM; > > > dprintk("RPC: %5u %s: no memory available\n", > > > -- > > > 2.19.2 > > > > > > > -- > > Chuck Lever > > > >
On Thu, 2018-12-20 at 10:52 -0500, Bruce Fields wrote: > On Thu, Dec 20, 2018 at 10:47:25AM -0500, Chuck Lever wrote: > > > > > On Dec 20, 2018, at 10:42 AM, J. Bruce Fields < > > > bfields@fieldses.org> wrote: > > > > > > From: "J. Bruce Fields" <bfields@redhat.com> > > > > > > It's OK to sleep here, we just don't want to recurse into the > > > filesystem > > > as this writeout could be waiting on this. > > > > "as a writeout" > > Oops, thanks. > > > > Future work: the documentation for GFP_NOFS says "Please try to > > > avoid > > > using this flag directly and instead use > > > memalloc_nofs_{save,restore} to > > > mark the whole scope which cannot/shouldn't recurse into the FS > > > layer > > > with a short explanation why. All allocation requests will > > > inherit > > > GFP_NOFS implicitly." > > > > > > But I'm not sure where to do this. Should the workqueue be > > > arranging > > > that for us in the case of workqueues created with > > > WQ_MEM_RECLAIM? > > > > There seem to be plenty of uses of GFP_NOFS in NFS and sunrpc. > > That sounds like a big project. > > Yeah, just noting it for future reference. > I'd suggest that we can probably just call memalloc_nofs_save() in rpc_execute(), and otherwise in those workqueue callback functions that are executed directly by rpciod and xprtiod. That doesn't make for too many callsites.
diff --git a/net/sunrpc/rpcb_clnt.c b/net/sunrpc/rpcb_clnt.c index 08b5fa4a2852..41a971ac1c63 100644 --- a/net/sunrpc/rpcb_clnt.c +++ b/net/sunrpc/rpcb_clnt.c @@ -752,7 +752,7 @@ void rpcb_getport_async(struct rpc_task *task) goto bailout_nofree; } - map = kzalloc(sizeof(struct rpcbind_args), GFP_ATOMIC); + map = kzalloc(sizeof(struct rpcbind_args), GFP_NOFS); if (!map) { status = -ENOMEM; dprintk("RPC: %5u %s: no memory available\n", @@ -770,7 +770,7 @@ void rpcb_getport_async(struct rpc_task *task) case RPCBVERS_4: case RPCBVERS_3: map->r_netid = xprt->address_strings[RPC_DISPLAY_NETID]; - map->r_addr = rpc_sockaddr2uaddr(sap, GFP_ATOMIC); + map->r_addr = rpc_sockaddr2uaddr(sap, GFP_NOFS); if (!map->r_addr) { status = -ENOMEM; dprintk("RPC: %5u %s: no memory available\n",