diff mbox

[1/3] pnfs: defer release of pages in layoutget

Message ID 1343037907-26457-2-git-send-email-idank@tonian.com (mailing list archive)
State New, archived
Headers show

Commit Message

Idan Kedar July 23, 2012, 10:05 a.m. UTC 
we have encountered a bug whereby reading a lot of files (copying
fedora's /bin) from a pNFS mount and hitting Ctrl+C in the middle caused
a general protection fault in xdr_shrink_bufhead. this function is
called when decoding the response from LAYOUTGET. the decoding is done
by a worker thread, and the caller of LAYOUTGET waits for the worker
thread to complete.

hitting Ctrl+C caused the synchronous wait to end and the next thing the
caller does is to free the pages, so when the worker thread calls
xdr_shrink_bufhead, the pages are gone. therefore, the cleanup of these
pages has been moved to nfs4_layoutget_release.

Signed-off-by: Idan Kedar <idank@tonian.com>
Signed-off-by: Benny Halevy <bhalevy@tonian.com>
---
 fs/nfs/nfs4proc.c |   57 ++++++++++++++++++++++++++++++++++++++++++++++++++++-
 fs/nfs/pnfs.c     |   39 +-----------------------------------
 fs/nfs/pnfs.h     |    2 +-
 3 files changed, 58 insertions(+), 40 deletions(-)

Comments

Trond Myklebust July 31, 2012, 6:54 p.m. UTC | #1 
On Mon, 2012-07-23 at 13:05 +0300, Idan Kedar wrote:
> we have encountered a bug whereby reading a lot of files (copying

> fedora's /bin) from a pNFS mount and hitting Ctrl+C in the middle caused

> a general protection fault in xdr_shrink_bufhead. this function is

> called when decoding the response from LAYOUTGET. the decoding is done

> by a worker thread, and the caller of LAYOUTGET waits for the worker

> thread to complete.

> 

> hitting Ctrl+C caused the synchronous wait to end and the next thing the

> caller does is to free the pages, so when the worker thread calls

> xdr_shrink_bufhead, the pages are gone. therefore, the cleanup of these

> pages has been moved to nfs4_layoutget_release.

> 

> Signed-off-by: Idan Kedar <idank@tonian.com>

> Signed-off-by: Benny Halevy <bhalevy@tonian.com>

> ---

>  fs/nfs/nfs4proc.c |   57 ++++++++++++++++++++++++++++++++++++++++++++++++++++-

>  fs/nfs/pnfs.c     |   39 +-----------------------------------

>  fs/nfs/pnfs.h     |    2 +-

>  3 files changed, 58 insertions(+), 40 deletions(-)

> 

> diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c

> index 15fc7e4..31a7858 100644

> --- a/fs/nfs/nfs4proc.c

> +++ b/fs/nfs/nfs4proc.c

> @@ -6164,11 +6164,58 @@ static void nfs4_layoutget_done(struct rpc_task *task, void *calldata)

>  	dprintk("<-- %s\n", __func__);

>  }

>  

> +static u32 max_response_pages(struct nfs_server *server)

> +{

> +	u32 max_resp_sz = server->nfs_client->cl_session->fc_attrs.max_resp_sz;

> +	return nfs_page_array_len(0, max_resp_sz);

> +}

> +

> +static void free_pagevec(struct page **pages, u32 size)


We already have a 'struct pagevec' (see include/linux/pagevec.h), so the
above name is confusing. In addition, you really should use a 'nfs4_'
prefix in order to be consistent with the naming scheme for functions in
fs/nfs/nfs4proc.c

> +{

> +	int i;

> +

> +	if (!pages)

> +		return;

> +

> +	for (i = 0; i < size; i++) {

> +		if (!pages[i])

> +			break;

> +		__free_page(pages[i]);

> +	}

> +	kfree(pages);

> +}

> +

> +static struct page **alloc_pagevec(u32 size, gfp_t gfp_flags)


Ditto. See above...

> +{

> +	struct page **pages;

> +	int i;

> +

> +	pages = kzalloc(size * sizeof(struct page *), gfp_flags);


Please use kcalloc when allocating a zero-initialised array.

> +	if (!pages) {

> +		dprintk("%s: can't alloc array of %zu pages\n", __func__, size);

> +		return NULL;

> +	}

> +

> +	for (i = 0; i < size; i++) {

> +		pages[i] = alloc_page(gfp_flags);

> +		if (!pages[i]) {

> +			dprintk("%s: failed to allocate page\n", __func__);

> +			free_pagevec(pages, size);

> +			return NULL;

> +		}

> +	}

> +

> +	return pages;

> +}

> +

>  static void nfs4_layoutget_release(void *calldata)

>  {

>  	struct nfs4_layoutget *lgp = calldata;

> +	struct nfs_server *server = NFS_SERVER(lgp->args.inode);

> +	u32 max_pages = max_response_pages(server);

>  

>  	dprintk("--> %s\n", __func__);

> +	free_pagevec(lgp->args.layout.pages, max_pages);

>  	put_nfs_open_context(lgp->args.ctx);

>  	kfree(calldata);

>  	dprintk("<-- %s\n", __func__);

> @@ -6180,9 +6227,10 @@ static const struct rpc_call_ops nfs4_layoutget_call_ops = {

>  	.rpc_release = nfs4_layoutget_release,

>  };

>  

> -int nfs4_proc_layoutget(struct nfs4_layoutget *lgp)

> +int nfs4_proc_layoutget(struct nfs4_layoutget *lgp, gfp_t gfp_flags)

>  {

>  	struct nfs_server *server = NFS_SERVER(lgp->args.inode);

> +	u32 max_pages = max_response_pages(server);

>  	struct rpc_task *task;

>  	struct rpc_message msg = {

>  		.rpc_proc = &nfs4_procedures[NFSPROC4_CLNT_LAYOUTGET],

> @@ -6200,6 +6248,13 @@ int nfs4_proc_layoutget(struct nfs4_layoutget *lgp)

>  

>  	dprintk("--> %s\n", __func__);

>  

> +	lgp->args.layout.pages = alloc_pagevec(max_pages, gfp_flags);

> +	if (!lgp->args.layout.pages) {

> +		nfs4_layoutget_release(lgp);

> +		return -ENOMEM;

> +	}

> +	lgp->args.layout.pglen = max_pages * PAGE_SIZE;

> +

>  	lgp->res.layoutp = &lgp->args.layout;

>  	lgp->res.seq_res.sr_slot = NULL;

>  	nfs41_init_sequence(&lgp->args.seq_args, &lgp->res.seq_res, 0);

> diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c

> index bbc49ca..8229a0e 100644

> --- a/fs/nfs/pnfs.c

> +++ b/fs/nfs/pnfs.c

> @@ -583,9 +583,6 @@ send_layoutget(struct pnfs_layout_hdr *lo,

>  	struct nfs_server *server = NFS_SERVER(ino);

>  	struct nfs4_layoutget *lgp;

>  	struct pnfs_layout_segment *lseg = NULL;

> -	struct page **pages = NULL;

> -	int i;

> -	u32 max_resp_sz, max_pages;

>  

>  	dprintk("--> %s\n", __func__);

>  

> @@ -594,20 +591,6 @@ send_layoutget(struct pnfs_layout_hdr *lo,

>  	if (lgp == NULL)

>  		return NULL;

>  

> -	/* allocate pages for xdr post processing */

> -	max_resp_sz = server->nfs_client->cl_session->fc_attrs.max_resp_sz;

> -	max_pages = nfs_page_array_len(0, max_resp_sz);

> -

> -	pages = kcalloc(max_pages, sizeof(struct page *), gfp_flags);

> -	if (!pages)

> -		goto out_err_free;

> -

> -	for (i = 0; i < max_pages; i++) {

> -		pages[i] = alloc_page(gfp_flags);

> -		if (!pages[i])

> -			goto out_err_free;

> -	}

> -

>  	lgp->args.minlength = PAGE_CACHE_SIZE;

>  	if (lgp->args.minlength > range->length)

>  		lgp->args.minlength = range->length;

> @@ -616,39 +599,19 @@ send_layoutget(struct pnfs_layout_hdr *lo,

>  	lgp->args.type = server->pnfs_curr_ld->id;

>  	lgp->args.inode = ino;

>  	lgp->args.ctx = get_nfs_open_context(ctx);

> -	lgp->args.layout.pages = pages;

> -	lgp->args.layout.pglen = max_pages * PAGE_SIZE;

>  	lgp->lsegpp = &lseg;

>  	lgp->gfp_flags = gfp_flags;

>  

>  	/* Synchronously retrieve layout information from server and

>  	 * store in lseg.

>  	 */

> -	nfs4_proc_layoutget(lgp);

> +	nfs4_proc_layoutget(lgp, gfp_flags);

>  	if (!lseg) {

>  		/* remember that LAYOUTGET failed and suspend trying */

>  		set_bit(lo_fail_bit(range->iomode), &lo->plh_flags);

>  	}

>  

> -	/* free xdr pages */

> -	for (i = 0; i < max_pages; i++)

> -		__free_page(pages[i]);

> -	kfree(pages);

> -

>  	return lseg;

> -

> -out_err_free:

> -	/* free any allocated xdr pages, lgp as it's not used */

> -	if (pages) {

> -		for (i = 0; i < max_pages; i++) {

> -			if (!pages[i])

> -				break;

> -			__free_page(pages[i]);

> -		}

> -		kfree(pages);

> -	}

> -	kfree(lgp);

> -	return NULL;

>  }

>  

>  /* Initiates a LAYOUTRETURN(FILE) */

> diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h

> index 64f90d8..9a31ff3 100644

> --- a/fs/nfs/pnfs.h

> +++ b/fs/nfs/pnfs.h

> @@ -171,7 +171,7 @@ extern int nfs4_proc_getdevicelist(struct nfs_server *server,

>  				   struct pnfs_devicelist *devlist);

>  extern int nfs4_proc_getdeviceinfo(struct nfs_server *server,

>  				   struct pnfs_device *dev);

> -extern int nfs4_proc_layoutget(struct nfs4_layoutget *lgp);

> +extern int nfs4_proc_layoutget(struct nfs4_layoutget *lgp, gfp_t gfp_flags);

>  extern int nfs4_proc_layoutreturn(struct nfs4_layoutreturn *lrp);

>  

>  /* pnfs.c */
diff mbox

Patch

diff --git a/fs/nfs/nfs4proc.c b/fs/nfs/nfs4proc.c
index 15fc7e4..31a7858 100644
--- a/fs/nfs/nfs4proc.c
+++ b/fs/nfs/nfs4proc.c
@@ -6164,11 +6164,58 @@  static void nfs4_layoutget_done(struct rpc_task *task, void *calldata)
 	dprintk("<-- %s\n", __func__);
 }
 
+static u32 max_response_pages(struct nfs_server *server)
+{
+	u32 max_resp_sz = server->nfs_client->cl_session->fc_attrs.max_resp_sz;
+	return nfs_page_array_len(0, max_resp_sz);
+}
+
+static void free_pagevec(struct page **pages, u32 size)
+{
+	int i;
+
+	if (!pages)
+		return;
+
+	for (i = 0; i < size; i++) {
+		if (!pages[i])
+			break;
+		__free_page(pages[i]);
+	}
+	kfree(pages);
+}
+
+static struct page **alloc_pagevec(u32 size, gfp_t gfp_flags)
+{
+	struct page **pages;
+	int i;
+
+	pages = kzalloc(size * sizeof(struct page *), gfp_flags);
+	if (!pages) {
+		dprintk("%s: can't alloc array of %zu pages\n", __func__, size);
+		return NULL;
+	}
+
+	for (i = 0; i < size; i++) {
+		pages[i] = alloc_page(gfp_flags);
+		if (!pages[i]) {
+			dprintk("%s: failed to allocate page\n", __func__);
+			free_pagevec(pages, size);
+			return NULL;
+		}
+	}
+
+	return pages;
+}
+
 static void nfs4_layoutget_release(void *calldata)
 {
 	struct nfs4_layoutget *lgp = calldata;
+	struct nfs_server *server = NFS_SERVER(lgp->args.inode);
+	u32 max_pages = max_response_pages(server);
 
 	dprintk("--> %s\n", __func__);
+	free_pagevec(lgp->args.layout.pages, max_pages);
 	put_nfs_open_context(lgp->args.ctx);
 	kfree(calldata);
 	dprintk("<-- %s\n", __func__);
@@ -6180,9 +6227,10 @@  static const struct rpc_call_ops nfs4_layoutget_call_ops = {
 	.rpc_release = nfs4_layoutget_release,
 };
 
-int nfs4_proc_layoutget(struct nfs4_layoutget *lgp)
+int nfs4_proc_layoutget(struct nfs4_layoutget *lgp, gfp_t gfp_flags)
 {
 	struct nfs_server *server = NFS_SERVER(lgp->args.inode);
+	u32 max_pages = max_response_pages(server);
 	struct rpc_task *task;
 	struct rpc_message msg = {
 		.rpc_proc = &nfs4_procedures[NFSPROC4_CLNT_LAYOUTGET],
@@ -6200,6 +6248,13 @@  int nfs4_proc_layoutget(struct nfs4_layoutget *lgp)
 
 	dprintk("--> %s\n", __func__);
 
+	lgp->args.layout.pages = alloc_pagevec(max_pages, gfp_flags);
+	if (!lgp->args.layout.pages) {
+		nfs4_layoutget_release(lgp);
+		return -ENOMEM;
+	}
+	lgp->args.layout.pglen = max_pages * PAGE_SIZE;
+
 	lgp->res.layoutp = &lgp->args.layout;
 	lgp->res.seq_res.sr_slot = NULL;
 	nfs41_init_sequence(&lgp->args.seq_args, &lgp->res.seq_res, 0);
diff --git a/fs/nfs/pnfs.c b/fs/nfs/pnfs.c
index bbc49ca..8229a0e 100644
--- a/fs/nfs/pnfs.c
+++ b/fs/nfs/pnfs.c
@@ -583,9 +583,6 @@  send_layoutget(struct pnfs_layout_hdr *lo,
 	struct nfs_server *server = NFS_SERVER(ino);
 	struct nfs4_layoutget *lgp;
 	struct pnfs_layout_segment *lseg = NULL;
-	struct page **pages = NULL;
-	int i;
-	u32 max_resp_sz, max_pages;
 
 	dprintk("--> %s\n", __func__);
 
@@ -594,20 +591,6 @@  send_layoutget(struct pnfs_layout_hdr *lo,
 	if (lgp == NULL)
 		return NULL;
 
-	/* allocate pages for xdr post processing */
-	max_resp_sz = server->nfs_client->cl_session->fc_attrs.max_resp_sz;
-	max_pages = nfs_page_array_len(0, max_resp_sz);
-
-	pages = kcalloc(max_pages, sizeof(struct page *), gfp_flags);
-	if (!pages)
-		goto out_err_free;
-
-	for (i = 0; i < max_pages; i++) {
-		pages[i] = alloc_page(gfp_flags);
-		if (!pages[i])
-			goto out_err_free;
-	}
-
 	lgp->args.minlength = PAGE_CACHE_SIZE;
 	if (lgp->args.minlength > range->length)
 		lgp->args.minlength = range->length;
@@ -616,39 +599,19 @@  send_layoutget(struct pnfs_layout_hdr *lo,
 	lgp->args.type = server->pnfs_curr_ld->id;
 	lgp->args.inode = ino;
 	lgp->args.ctx = get_nfs_open_context(ctx);
-	lgp->args.layout.pages = pages;
-	lgp->args.layout.pglen = max_pages * PAGE_SIZE;
 	lgp->lsegpp = &lseg;
 	lgp->gfp_flags = gfp_flags;
 
 	/* Synchronously retrieve layout information from server and
 	 * store in lseg.
 	 */
-	nfs4_proc_layoutget(lgp);
+	nfs4_proc_layoutget(lgp, gfp_flags);
 	if (!lseg) {
 		/* remember that LAYOUTGET failed and suspend trying */
 		set_bit(lo_fail_bit(range->iomode), &lo->plh_flags);
 	}
 
-	/* free xdr pages */
-	for (i = 0; i < max_pages; i++)
-		__free_page(pages[i]);
-	kfree(pages);
-
 	return lseg;
-
-out_err_free:
-	/* free any allocated xdr pages, lgp as it's not used */
-	if (pages) {
-		for (i = 0; i < max_pages; i++) {
-			if (!pages[i])
-				break;
-			__free_page(pages[i]);
-		}
-		kfree(pages);
-	}
-	kfree(lgp);
-	return NULL;
 }
 
 /* Initiates a LAYOUTRETURN(FILE) */
diff --git a/fs/nfs/pnfs.h b/fs/nfs/pnfs.h
index 64f90d8..9a31ff3 100644
--- a/fs/nfs/pnfs.h
+++ b/fs/nfs/pnfs.h
@@ -171,7 +171,7 @@  extern int nfs4_proc_getdevicelist(struct nfs_server *server,
 				   struct pnfs_devicelist *devlist);
 extern int nfs4_proc_getdeviceinfo(struct nfs_server *server,
 				   struct pnfs_device *dev);
-extern int nfs4_proc_layoutget(struct nfs4_layoutget *lgp);
+extern int nfs4_proc_layoutget(struct nfs4_layoutget *lgp, gfp_t gfp_flags);
 extern int nfs4_proc_layoutreturn(struct nfs4_layoutreturn *lrp);
 
 /* pnfs.c */