| Message ID | 1410395791-2385-4-git-send-email-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 09/10/2014 08:36 PM, Christoph Hellwig wrote: > Instead of overflowing the XDR send buffer with our extent list allocate > pages and pre-encode the layoutupdate payload into them. We optimistically > allocate a single page use alloc_page and only switch to vmalloc when we > have more extents outstanding. Currently there is only a single testcase > (xfstests generic/113) which can reproduce large enough extent lists for > this to occur. > > Signed-off-by: Christoph Hellwig <hch@lst.de> > --- > fs/nfs/blocklayout/blocklayout.c | 15 ++---- > fs/nfs/blocklayout/blocklayout.h | 8 ++-- > fs/nfs/blocklayout/extent_tree.c | 100 +++++++++++++++++++++++++++++++-------- > 3 files changed, 90 insertions(+), 33 deletions(-) > > diff --git a/fs/nfs/blocklayout/blocklayout.c b/fs/nfs/blocklayout/blocklayout.c > index 25ba9e0..900eb09 100644 > --- a/fs/nfs/blocklayout/blocklayout.c > +++ b/fs/nfs/blocklayout/blocklayout.c > @@ -500,21 +500,16 @@ bl_return_range(struct pnfs_layout_hdr *lo, > err = ext_tree_remove(bl, range->iomode & IOMODE_RW, offset, end); > } > > -static void > -bl_encode_layoutcommit(struct pnfs_layout_hdr *lo, struct xdr_stream *xdr, > - const struct nfs4_layoutcommit_args *arg) > +static int > +bl_prepare_layoutcommit(struct nfs4_layoutcommit_args *arg) > { > - dprintk("%s enter\n", __func__); > - ext_tree_encode_commit(BLK_LO2EXT(lo), xdr); > + return ext_tree_prepare_commit(arg); > } > > static void > bl_cleanup_layoutcommit(struct nfs4_layoutcommit_data *lcdata) > { > - struct pnfs_layout_hdr *lo = NFS_I(lcdata->args.inode)->layout; > - > - dprintk("%s enter\n", __func__); > - ext_tree_mark_committed(BLK_LO2EXT(lo), lcdata->res.status); > + ext_tree_mark_committed(&lcdata->args, lcdata->res.status); > } > > static int > @@ -670,7 +665,7 @@ static struct pnfs_layoutdriver_type blocklayout_type = { > .alloc_lseg = bl_alloc_lseg, > .free_lseg = bl_free_lseg, > .return_range = bl_return_range, > - .encode_layoutcommit = bl_encode_layoutcommit, > + .prepare_layoutcommit = bl_prepare_layoutcommit, > .cleanup_layoutcommit = bl_cleanup_layoutcommit, > .set_layoutdriver = bl_set_layoutdriver, > .alloc_deviceid_node = bl_alloc_deviceid_node, > diff --git a/fs/nfs/blocklayout/blocklayout.h b/fs/nfs/blocklayout/blocklayout.h > index 19fae5e..9757f3e 100644 > --- a/fs/nfs/blocklayout/blocklayout.h > +++ b/fs/nfs/blocklayout/blocklayout.h > @@ -72,6 +72,9 @@ struct pnfs_block_extent { > unsigned int be_tag; > }; > > +/* on the wire size of the extent */ > +#define BL_EXTENT_SIZE (7 * sizeof(__be32) + NFS4_DEVICEID4_SIZE) > + > struct pnfs_block_layout { > struct pnfs_layout_hdr bl_layout; > struct rb_root bl_ext_rw; > @@ -129,8 +132,7 @@ int ext_tree_mark_written(struct pnfs_block_layout *bl, sector_t start, > sector_t len); > bool ext_tree_lookup(struct pnfs_block_layout *bl, sector_t isect, > struct pnfs_block_extent *ret, bool rw); > -int ext_tree_encode_commit(struct pnfs_block_layout *bl, > - struct xdr_stream *xdr); > -void ext_tree_mark_committed(struct pnfs_block_layout *bl, int status); > +int ext_tree_prepare_commit(struct nfs4_layoutcommit_args *arg); > +void ext_tree_mark_committed(struct nfs4_layoutcommit_args *arg, int status); > > #endif /* FS_NFS_NFS4BLOCKLAYOUT_H */ > diff --git a/fs/nfs/blocklayout/extent_tree.c b/fs/nfs/blocklayout/extent_tree.c > index 43e891b..2a2c261 100644 > --- a/fs/nfs/blocklayout/extent_tree.c > +++ b/fs/nfs/blocklayout/extent_tree.c > @@ -462,19 +462,25 @@ out: > return err; > } > > -int > -ext_tree_encode_commit(struct pnfs_block_layout *bl, struct xdr_stream *xdr) > +static void ext_tree_free_commitdata(struct nfs4_layoutcommit_args *arg, > + size_t buffer_size) > { > - struct pnfs_block_extent *be; > - unsigned int count = 0; > - __be32 *p, *xdr_start; > - int ret = 0; > + if (arg->layoutupdate_pages != &arg->layoutupdate_page) { > + int nr_pages = DIV_ROUND_UP(buffer_size, PAGE_SIZE), i; > > - dprintk("%s enter\n", __func__); > + for (i = 0; i < nr_pages; i++) > + put_page(arg->layoutupdate_pages[i]); > + kfree(arg->layoutupdate_pages); > + } else { > + put_page(arg->layoutupdate_page); > + } > +} > > - xdr_start = xdr_reserve_space(xdr, 8); > - if (!xdr_start) > - return -ENOSPC; > +static int ext_tree_encode_commit(struct pnfs_block_layout *bl, __be32 *p, > + size_t buffer_size, size_t *count) > +{ > + struct pnfs_block_extent *be; > + int ret = 0; > > spin_lock(&bl->bl_ext_lock); > for (be = ext_tree_first(&bl->bl_ext_rw); be; be = ext_tree_next(be)) { > @@ -482,12 +488,11 @@ ext_tree_encode_commit(struct pnfs_block_layout *bl, struct xdr_stream *xdr) > be->be_tag != EXTENT_WRITTEN) > continue; > > - p = xdr_reserve_space(xdr, 7 * sizeof(__be32) + > - NFS4_DEVICEID4_SIZE); > - if (!p) { > - printk("%s: out of space for extent list\n", __func__); > + (*count)++; > + if (*count * BL_EXTENT_SIZE > buffer_size) { > + /* keep counting.. */ > ret = -ENOSPC; > - break; > + continue; > } > > p = xdr_encode_opaque_fixed(p, be->be_device->deviceid.data, > @@ -498,25 +503,80 @@ ext_tree_encode_commit(struct pnfs_block_layout *bl, struct xdr_stream *xdr) > *p++ = cpu_to_be32(PNFS_BLOCK_READWRITE_DATA); > > be->be_tag = EXTENT_COMMITTING; > - count++; > } > spin_unlock(&bl->bl_ext_lock); > > - xdr_start[0] = cpu_to_be32((xdr->p - xdr_start - 1) * 4); > - xdr_start[1] = cpu_to_be32(count); > + return ret; > +} > + > +int > +ext_tree_prepare_commit(struct nfs4_layoutcommit_args *arg) > +{ > + struct pnfs_block_layout *bl = BLK_LO2EXT(NFS_I(arg->inode)->layout); > + size_t count = 0, buffer_size = PAGE_SIZE; > + __be32 *start_p; > + int ret; > + > + dprintk("%s enter\n", __func__); > + > + arg->layoutupdate_page = alloc_page(GFP_NOFS); > + if (!arg->layoutupdate_page) > + return -ENOMEM; > + start_p = page_address(arg->layoutupdate_page); > + arg->layoutupdate_pages = &arg->layoutupdate_page; > + > +retry: > + ret = ext_tree_encode_commit(bl, start_p + 1, buffer_size, &count); > + if (unlikely(ret)) { > + ext_tree_free_commitdata(arg, buffer_size); > + > + buffer_size = sizeof(__be32) + BL_EXTENT_SIZE * count; > + count = 0; > + > + arg->layoutupdate_pages = > + kcalloc(DIV_ROUND_UP(buffer_size, PAGE_SIZE), > + sizeof(struct page *), GFP_NOFS); I don't think I've seen kcalloc() before. What does it do? > + if (!arg->layoutupdate_pages) > + return -ENOMEM; > + > + start_p = __vmalloc(buffer_size, GFP_NOFS, PAGE_KERNEL); > + if (!start_p) { > + kfree(arg->layoutupdate_pages); > + return -ENOMEM; > + } > + > + goto retry; > + } > + > + *start_p = cpu_to_be32(count); > + arg->layoutupdate_len = sizeof(__be32) + BL_EXTENT_SIZE * count; > + > + if (unlikely(arg->layoutupdate_pages != &arg->layoutupdate_page)) { > + __be32 *p = start_p; > + int i = 0; > + > + for (p = start_p; > + p < start_p + arg->layoutupdate_len; > + p += PAGE_SIZE) { > + arg->layoutupdate_pages[i++] = vmalloc_to_page(p); > + } > + } > > dprintk("%s found %i ranges\n", __func__, count); I'm getting this: fs/nfs/blocklayout/extent_tree.c: In function ‘ext_tree_prepare_commit’: fs/nfs/blocklayout/extent_tree.c:565:2: warning: format ‘%i’ expects argument of type ‘int’, but argument 3 has type ‘size_t’ [-Wformat=] dprintk("%s found %i ranges\n", __func__, count); Anna > - return ret; > + return 0; > } > > void > -ext_tree_mark_committed(struct pnfs_block_layout *bl, int status) > +ext_tree_mark_committed(struct nfs4_layoutcommit_args *arg, int status) > { > + struct pnfs_block_layout *bl = BLK_LO2EXT(NFS_I(arg->inode)->layout); > struct rb_root *root = &bl->bl_ext_rw; > struct pnfs_block_extent *be; > > dprintk("%s status %d\n", __func__, status); > > + ext_tree_free_commitdata(arg, arg->layoutupdate_len); > + > spin_lock(&bl->bl_ext_lock); > for (be = ext_tree_first(root); be; be = ext_tree_next(be)) { > if (be->be_state != PNFS_BLOCK_INVALID_DATA || -- 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 Thu, Sep 11, 2014 at 10:16:56AM -0400, Anna Schumaker wrote: > > + arg->layoutupdate_pages = > > + kcalloc(DIV_ROUND_UP(buffer_size, PAGE_SIZE), > > + sizeof(struct page *), GFP_NOFS); > > I don't think I've seen kcalloc() before. What does it do? The same as calloc(3) in userspace - allocate an array of nent entries of size, and zero the result that is returned. > > dprintk("%s found %i ranges\n", __func__, count); > > I'm getting this: > > fs/nfs/blocklayout/extent_tree.c: In function ‘ext_tree_prepare_commit’: > fs/nfs/blocklayout/extent_tree.c:565:2: warning: format ‘%i’ expects argument of type ‘int’, but argument 3 has type ‘size_t’ [-Wformat=] > dprintk("%s found %i ranges\n", __func__, count); For some reason I haven't managed to trigger this warning even after turning on CONFIG_SUNRPC_DEBUG, but it should be easy to fix up.. -- 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 09/11/2014 11:27 AM, Christoph Hellwig wrote: > On Thu, Sep 11, 2014 at 10:16:56AM -0400, Anna Schumaker wrote: >>> + arg->layoutupdate_pages = >>> + kcalloc(DIV_ROUND_UP(buffer_size, PAGE_SIZE), >>> + sizeof(struct page *), GFP_NOFS); >> I don't think I've seen kcalloc() before. What does it do? > The same as calloc(3) in userspace - allocate an array of nent entries > of size, and zero the result that is returned. I haven't run across it in userspace either ... thanks for answering! :) > >>> dprintk("%s found %i ranges\n", __func__, count); >> I'm getting this: >> >> fs/nfs/blocklayout/extent_tree.c: In function ‘ext_tree_prepare_commit’: >> fs/nfs/blocklayout/extent_tree.c:565:2: warning: format ‘%i’ expects argument of type ‘int’, but argument 3 has type ‘size_t’ [-Wformat=] >> dprintk("%s found %i ranges\n", __func__, count); > For some reason I haven't managed to trigger this warning even after > turning on CONFIG_SUNRPC_DEBUG, but it should be easy to fix up.. Thanks. I'm not sure what I have set differently, but it's consistently showing up in my compile logs. Anna > > -- > 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 -- 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 09/11/2014 11:27 AM, Christoph Hellwig wrote: > On Thu, Sep 11, 2014 at 10:16:56AM -0400, Anna Schumaker wrote: >>> + arg->layoutupdate_pages = >>> + kcalloc(DIV_ROUND_UP(buffer_size, PAGE_SIZE), >>> + sizeof(struct page *), GFP_NOFS); >> I don't think I've seen kcalloc() before. What does it do? > The same as calloc(3) in userspace - allocate an array of nent entries > of size, and zero the result that is returned. I haven't run across it in userspace either ... thanks for answering! :) > >>> dprintk("%s found %i ranges\n", __func__, count); >> I'm getting this: >> >> fs/nfs/blocklayout/extent_tree.c: In function ‘ext_tree_prepare_commit’: >> fs/nfs/blocklayout/extent_tree.c:565:2: warning: format ‘%i’ expects argument of type ‘int’, but argument 3 has type ‘size_t’ [-Wformat=] >> dprintk("%s found %i ranges\n", __func__, count); > For some reason I haven't managed to trigger this warning even after > turning on CONFIG_SUNRPC_DEBUG, but it should be easy to fix up.. Thanks. I'm not sure what I have set differently, but it's consistently showing up in my compile logs. Anna > > -- > 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 -- 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/fs/nfs/blocklayout/blocklayout.c b/fs/nfs/blocklayout/blocklayout.c index 25ba9e0..900eb09 100644 --- a/fs/nfs/blocklayout/blocklayout.c +++ b/fs/nfs/blocklayout/blocklayout.c @@ -500,21 +500,16 @@ bl_return_range(struct pnfs_layout_hdr *lo, err = ext_tree_remove(bl, range->iomode & IOMODE_RW, offset, end); } -static void -bl_encode_layoutcommit(struct pnfs_layout_hdr *lo, struct xdr_stream *xdr, - const struct nfs4_layoutcommit_args *arg) +static int +bl_prepare_layoutcommit(struct nfs4_layoutcommit_args *arg) { - dprintk("%s enter\n", __func__); - ext_tree_encode_commit(BLK_LO2EXT(lo), xdr); + return ext_tree_prepare_commit(arg); } static void bl_cleanup_layoutcommit(struct nfs4_layoutcommit_data *lcdata) { - struct pnfs_layout_hdr *lo = NFS_I(lcdata->args.inode)->layout; - - dprintk("%s enter\n", __func__); - ext_tree_mark_committed(BLK_LO2EXT(lo), lcdata->res.status); + ext_tree_mark_committed(&lcdata->args, lcdata->res.status); } static int @@ -670,7 +665,7 @@ static struct pnfs_layoutdriver_type blocklayout_type = { .alloc_lseg = bl_alloc_lseg, .free_lseg = bl_free_lseg, .return_range = bl_return_range, - .encode_layoutcommit = bl_encode_layoutcommit, + .prepare_layoutcommit = bl_prepare_layoutcommit, .cleanup_layoutcommit = bl_cleanup_layoutcommit, .set_layoutdriver = bl_set_layoutdriver, .alloc_deviceid_node = bl_alloc_deviceid_node, diff --git a/fs/nfs/blocklayout/blocklayout.h b/fs/nfs/blocklayout/blocklayout.h index 19fae5e..9757f3e 100644 --- a/fs/nfs/blocklayout/blocklayout.h +++ b/fs/nfs/blocklayout/blocklayout.h @@ -72,6 +72,9 @@ struct pnfs_block_extent { unsigned int be_tag; }; +/* on the wire size of the extent */ +#define BL_EXTENT_SIZE (7 * sizeof(__be32) + NFS4_DEVICEID4_SIZE) + struct pnfs_block_layout { struct pnfs_layout_hdr bl_layout; struct rb_root bl_ext_rw; @@ -129,8 +132,7 @@ int ext_tree_mark_written(struct pnfs_block_layout *bl, sector_t start, sector_t len); bool ext_tree_lookup(struct pnfs_block_layout *bl, sector_t isect, struct pnfs_block_extent *ret, bool rw); -int ext_tree_encode_commit(struct pnfs_block_layout *bl, - struct xdr_stream *xdr); -void ext_tree_mark_committed(struct pnfs_block_layout *bl, int status); +int ext_tree_prepare_commit(struct nfs4_layoutcommit_args *arg); +void ext_tree_mark_committed(struct nfs4_layoutcommit_args *arg, int status); #endif /* FS_NFS_NFS4BLOCKLAYOUT_H */ diff --git a/fs/nfs/blocklayout/extent_tree.c b/fs/nfs/blocklayout/extent_tree.c index 43e891b..2a2c261 100644 --- a/fs/nfs/blocklayout/extent_tree.c +++ b/fs/nfs/blocklayout/extent_tree.c @@ -462,19 +462,25 @@ out: return err; } -int -ext_tree_encode_commit(struct pnfs_block_layout *bl, struct xdr_stream *xdr) +static void ext_tree_free_commitdata(struct nfs4_layoutcommit_args *arg, + size_t buffer_size) { - struct pnfs_block_extent *be; - unsigned int count = 0; - __be32 *p, *xdr_start; - int ret = 0; + if (arg->layoutupdate_pages != &arg->layoutupdate_page) { + int nr_pages = DIV_ROUND_UP(buffer_size, PAGE_SIZE), i; - dprintk("%s enter\n", __func__); + for (i = 0; i < nr_pages; i++) + put_page(arg->layoutupdate_pages[i]); + kfree(arg->layoutupdate_pages); + } else { + put_page(arg->layoutupdate_page); + } +} - xdr_start = xdr_reserve_space(xdr, 8); - if (!xdr_start) - return -ENOSPC; +static int ext_tree_encode_commit(struct pnfs_block_layout *bl, __be32 *p, + size_t buffer_size, size_t *count) +{ + struct pnfs_block_extent *be; + int ret = 0; spin_lock(&bl->bl_ext_lock); for (be = ext_tree_first(&bl->bl_ext_rw); be; be = ext_tree_next(be)) { @@ -482,12 +488,11 @@ ext_tree_encode_commit(struct pnfs_block_layout *bl, struct xdr_stream *xdr) be->be_tag != EXTENT_WRITTEN) continue; - p = xdr_reserve_space(xdr, 7 * sizeof(__be32) + - NFS4_DEVICEID4_SIZE); - if (!p) { - printk("%s: out of space for extent list\n", __func__); + (*count)++; + if (*count * BL_EXTENT_SIZE > buffer_size) { + /* keep counting.. */ ret = -ENOSPC; - break; + continue; } p = xdr_encode_opaque_fixed(p, be->be_device->deviceid.data, @@ -498,25 +503,80 @@ ext_tree_encode_commit(struct pnfs_block_layout *bl, struct xdr_stream *xdr) *p++ = cpu_to_be32(PNFS_BLOCK_READWRITE_DATA); be->be_tag = EXTENT_COMMITTING; - count++; } spin_unlock(&bl->bl_ext_lock); - xdr_start[0] = cpu_to_be32((xdr->p - xdr_start - 1) * 4); - xdr_start[1] = cpu_to_be32(count); + return ret; +} + +int +ext_tree_prepare_commit(struct nfs4_layoutcommit_args *arg) +{ + struct pnfs_block_layout *bl = BLK_LO2EXT(NFS_I(arg->inode)->layout); + size_t count = 0, buffer_size = PAGE_SIZE; + __be32 *start_p; + int ret; + + dprintk("%s enter\n", __func__); + + arg->layoutupdate_page = alloc_page(GFP_NOFS); + if (!arg->layoutupdate_page) + return -ENOMEM; + start_p = page_address(arg->layoutupdate_page); + arg->layoutupdate_pages = &arg->layoutupdate_page; + +retry: + ret = ext_tree_encode_commit(bl, start_p + 1, buffer_size, &count); + if (unlikely(ret)) { + ext_tree_free_commitdata(arg, buffer_size); + + buffer_size = sizeof(__be32) + BL_EXTENT_SIZE * count; + count = 0; + + arg->layoutupdate_pages = + kcalloc(DIV_ROUND_UP(buffer_size, PAGE_SIZE), + sizeof(struct page *), GFP_NOFS); + if (!arg->layoutupdate_pages) + return -ENOMEM; + + start_p = __vmalloc(buffer_size, GFP_NOFS, PAGE_KERNEL); + if (!start_p) { + kfree(arg->layoutupdate_pages); + return -ENOMEM; + } + + goto retry; + } + + *start_p = cpu_to_be32(count); + arg->layoutupdate_len = sizeof(__be32) + BL_EXTENT_SIZE * count; + + if (unlikely(arg->layoutupdate_pages != &arg->layoutupdate_page)) { + __be32 *p = start_p; + int i = 0; + + for (p = start_p; + p < start_p + arg->layoutupdate_len; + p += PAGE_SIZE) { + arg->layoutupdate_pages[i++] = vmalloc_to_page(p); + } + } dprintk("%s found %i ranges\n", __func__, count); - return ret; + return 0; } void -ext_tree_mark_committed(struct pnfs_block_layout *bl, int status) +ext_tree_mark_committed(struct nfs4_layoutcommit_args *arg, int status) { + struct pnfs_block_layout *bl = BLK_LO2EXT(NFS_I(arg->inode)->layout); struct rb_root *root = &bl->bl_ext_rw; struct pnfs_block_extent *be; dprintk("%s status %d\n", __func__, status); + ext_tree_free_commitdata(arg, arg->layoutupdate_len); + spin_lock(&bl->bl_ext_lock); for (be = ext_tree_first(root); be; be = ext_tree_next(be)) { if (be->be_state != PNFS_BLOCK_INVALID_DATA ||
Instead of overflowing the XDR send buffer with our extent list allocate pages and pre-encode the layoutupdate payload into them. We optimistically allocate a single page use alloc_page and only switch to vmalloc when we have more extents outstanding. Currently there is only a single testcase (xfstests generic/113) which can reproduce large enough extent lists for this to occur. Signed-off-by: Christoph Hellwig <hch@lst.de> --- fs/nfs/blocklayout/blocklayout.c | 15 ++---- fs/nfs/blocklayout/blocklayout.h | 8 ++-- fs/nfs/blocklayout/extent_tree.c | 100 +++++++++++++++++++++++++++++++-------- 3 files changed, 90 insertions(+), 33 deletions(-)