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@xxxxxxxxxx>
> Signed-off-by: Benny Halevy <bhalevy@xxxxxxxxxx>
> ---
> 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 */
��.n��������+%������w��{.n�����{��w��� jg��� �����ݢj����G���� ���j:+v���w�m������w�������h�����٥
