| Message ID | 20180508161529.GD6151@fieldses.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> On May 8, 2018, at 12:15 PM, bfields@fieldses.org wrote: > > On Tue, Apr 17, 2018 at 09:54:36PM +0000, Trond Myklebust wrote: >> Yes, and we can probably convert it, and the other GFP_ATOMIC >> allocations in the rpcbind client to use GFP_NOFS in order to improve >> reliability. > > Chuck, I think the GFP_ATOMIC is unnecessary here as well? > > --b. > > diff --git a/net/sunrpc/xprtrdma/rpc_rdma.c b/net/sunrpc/xprtrdma/rpc_rdma.c > index e8adad33d0bb..de90c6c90cde 100644 > --- a/net/sunrpc/xprtrdma/rpc_rdma.c > +++ b/net/sunrpc/xprtrdma/rpc_rdma.c > @@ -228,7 +228,7 @@ rpcrdma_convert_iovs(struct rpcrdma_xprt *r_xprt, struct xdr_buf *xdrbuf, > /* XXX: Certain upper layer operations do > * not provide receive buffer pages. > */ > - *ppages = alloc_page(GFP_ATOMIC); > + *ppages = alloc_page(GFP_NOFS); > if (!*ppages) > return -EAGAIN; > } This code can't sleep, as I understand it. Caller is holding the transport write lock. This logic was copied from xdr_partial_copy_from_skb, which uses GFP_ATOMIC. Recall that this is here because of GETACL. As I've stated in the past, the correct solution is to ensure that these pages are provided in every case by the upper layer, making this alloc_page call site unnecessary. -- Chuck Lever chucklever@gmail.com -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, May 08, 2018 at 12:34:48PM -0400, Chuck Lever wrote: > > > > On May 8, 2018, at 12:15 PM, bfields@fieldses.org wrote: > > > > On Tue, Apr 17, 2018 at 09:54:36PM +0000, Trond Myklebust wrote: > >> Yes, and we can probably convert it, and the other GFP_ATOMIC > >> allocations in the rpcbind client to use GFP_NOFS in order to improve > >> reliability. > > > > Chuck, I think the GFP_ATOMIC is unnecessary here as well? > > > > --b. > > > > diff --git a/net/sunrpc/xprtrdma/rpc_rdma.c b/net/sunrpc/xprtrdma/rpc_rdma.c > > index e8adad33d0bb..de90c6c90cde 100644 > > --- a/net/sunrpc/xprtrdma/rpc_rdma.c > > +++ b/net/sunrpc/xprtrdma/rpc_rdma.c > > @@ -228,7 +228,7 @@ rpcrdma_convert_iovs(struct rpcrdma_xprt *r_xprt, struct xdr_buf *xdrbuf, > > /* XXX: Certain upper layer operations do > > * not provide receive buffer pages. > > */ > > - *ppages = alloc_page(GFP_ATOMIC); > > + *ppages = alloc_page(GFP_NOFS); > > if (!*ppages) > > return -EAGAIN; > > } > > This code can't sleep, as I understand it. Caller is holding > the transport write lock. This logic was copied from > xdr_partial_copy_from_skb, which uses GFP_ATOMIC. OK. > Recall that this is here because of GETACL. As I've stated in > the past, the correct solution is to ensure that these pages > are provided in every case by the upper layer, making this > alloc_page call site unnecessary. Got it. --b. -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/sunrpc/xprtrdma/rpc_rdma.c b/net/sunrpc/xprtrdma/rpc_rdma.c index e8adad33d0bb..de90c6c90cde 100644 --- a/net/sunrpc/xprtrdma/rpc_rdma.c +++ b/net/sunrpc/xprtrdma/rpc_rdma.c @@ -228,7 +228,7 @@ rpcrdma_convert_iovs(struct rpcrdma_xprt *r_xprt, struct xdr_buf *xdrbuf, /* XXX: Certain upper layer operations do * not provide receive buffer pages. */ - *ppages = alloc_page(GFP_ATOMIC); + *ppages = alloc_page(GFP_NOFS); if (!*ppages) return -EAGAIN; }