On Tue, 16 Sep 2014, Yan, Zheng wrote:
> Current code uses page array to present MDS request data. Pages in the
> array are allocated/freed by caller of ceph_mdsc_do_request(). If request
> is interrupted, the pages can be freed while they are still being used by
> the request message.
>
> The fix is use pagelist to present MDS request data. Pagelist is
> reference counted.
>
> Signed-off-by: Yan, Zheng <zyan@xxxxxxxxxx>
So much nicer!
Reviewed-by: Sage Weil <sage@xxxxxxxxxx>
> ---
> fs/ceph/mds_client.c | 14 +++++++++-----
> fs/ceph/mds_client.h | 4 +---
> fs/ceph/xattr.c | 46 ++++++++++++++++------------------------------
> 3 files changed, 26 insertions(+), 38 deletions(-)
>
> diff --git a/fs/ceph/mds_client.c b/fs/ceph/mds_client.c
> index 30d7338..80d9f07 100644
> --- a/fs/ceph/mds_client.c
> +++ b/fs/ceph/mds_client.c
> @@ -542,6 +542,8 @@ void ceph_mdsc_release_request(struct kref *kref)
> }
> kfree(req->r_path1);
> kfree(req->r_path2);
> + if (req->r_pagelist)
> + ceph_pagelist_release(req->r_pagelist);
> put_request_session(req);
> ceph_unreserve_caps(req->r_mdsc, &req->r_caps_reservation);
> kfree(req);
> @@ -1847,13 +1849,15 @@ static struct ceph_msg *create_request_message(struct ceph_mds_client *mdsc,
> msg->front.iov_len = p - msg->front.iov_base;
> msg->hdr.front_len = cpu_to_le32(msg->front.iov_len);
>
> - if (req->r_data_len) {
> - /* outbound data set only by ceph_sync_setxattr() */
> - BUG_ON(!req->r_pages);
> - ceph_msg_data_add_pages(msg, req->r_pages, req->r_data_len, 0);
> + if (req->r_pagelist) {
> + struct ceph_pagelist *pagelist = req->r_pagelist;
> + atomic_inc(&pagelist->refcnt);
> + ceph_msg_data_add_pagelist(msg, pagelist);
> + msg->hdr.data_len = cpu_to_le32(pagelist->length);
> + } else {
> + msg->hdr.data_len = 0;
> }
>
> - msg->hdr.data_len = cpu_to_le32(req->r_data_len);
> msg->hdr.data_off = cpu_to_le16(0);
>
> out_free2:
> diff --git a/fs/ceph/mds_client.h b/fs/ceph/mds_client.h
> index e00737c..23015f7 100644
> --- a/fs/ceph/mds_client.h
> +++ b/fs/ceph/mds_client.h
> @@ -202,9 +202,7 @@ struct ceph_mds_request {
> bool r_direct_is_hash; /* true if r_direct_hash is valid */
>
> /* data payload is used for xattr ops */
> - struct page **r_pages;
> - int r_num_pages;
> - int r_data_len;
> + struct ceph_pagelist *r_pagelist;
>
> /* what caps shall we drop? */
> int r_inode_drop, r_inode_unless;
> diff --git a/fs/ceph/xattr.c b/fs/ceph/xattr.c
> index eab3e2f..c7b18b2 100644
> --- a/fs/ceph/xattr.c
> +++ b/fs/ceph/xattr.c
> @@ -1,4 +1,5 @@
> #include <linux/ceph/ceph_debug.h>
> +#include <linux/ceph/pagelist.h>
>
> #include "super.h"
> #include "mds_client.h"
> @@ -852,28 +853,17 @@ static int ceph_sync_setxattr(struct dentry *dentry, const char *name,
> struct ceph_mds_request *req;
> struct ceph_mds_client *mdsc = fsc->mdsc;
> int err;
> - int i, nr_pages;
> - struct page **pages = NULL;
> - void *kaddr;
> -
> - /* copy value into some pages */
> - nr_pages = calc_pages_for(0, size);
> - if (nr_pages) {
> - pages = kmalloc(sizeof(pages[0])*nr_pages, GFP_NOFS);
> - if (!pages)
> - return -ENOMEM;
> - err = -ENOMEM;
> - for (i = 0; i < nr_pages; i++) {
> - pages[i] = __page_cache_alloc(GFP_NOFS);
> - if (!pages[i]) {
> - nr_pages = i;
> - goto out;
> - }
> - kaddr = kmap(pages[i]);
> - memcpy(kaddr, value + i*PAGE_CACHE_SIZE,
> - min(PAGE_CACHE_SIZE, size-i*PAGE_CACHE_SIZE));
> - }
> - }
> + struct ceph_pagelist *pagelist;
> +
> + /* copy value into pagelist */
> + pagelist = kmalloc(sizeof(*pagelist), GFP_NOFS);
> + if (!pagelist)
> + return -ENOMEM;
> +
> + ceph_pagelist_init(pagelist);
> + err = ceph_pagelist_append(pagelist, value, size);
> + if (err)
> + goto out;
>
> dout("setxattr value=%.*s\n", (int)size, value);
>
> @@ -894,9 +884,8 @@ static int ceph_sync_setxattr(struct dentry *dentry, const char *name,
> req->r_args.setxattr.flags = cpu_to_le32(flags);
> req->r_path2 = kstrdup(name, GFP_NOFS);
>
> - req->r_pages = pages;
> - req->r_num_pages = nr_pages;
> - req->r_data_len = size;
> + req->r_pagelist = pagelist;
> + pagelist = NULL;
>
> dout("xattr.ver (before): %lld\n", ci->i_xattrs.version);
> err = ceph_mdsc_do_request(mdsc, NULL, req);
> @@ -904,11 +893,8 @@ static int ceph_sync_setxattr(struct dentry *dentry, const char *name,
> dout("xattr.ver (after): %lld\n", ci->i_xattrs.version);
>
> out:
> - if (pages) {
> - for (i = 0; i < nr_pages; i++)
> - __free_page(pages[i]);
> - kfree(pages);
> - }
> + if (pagelist)
> + ceph_pagelist_release(pagelist);
> return err;
> }
>
> --
> 1.9.3
>
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
--
To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
