| Message ID | 20180530194807.31657-5-longli@linuxonhyperv.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 5/30/2018 3:47 PM, Long Li wrote: > From: Long Li <longli@microsoft.com> > > Add a function to allocate wdata without allocating pages for data > transfer. This gives the caller an option to pass a number of pages that > point to the data buffer to write to. > > wdata is reponsible for free those pages after it's done. Same comment as for the earlier patch. "Caller" is responsible for "freeing" those pages? Confusing, as worded. > > Signed-off-by: Long Li <longli@microsoft.com> > --- > fs/cifs/cifsglob.h | 2 +- > fs/cifs/cifsproto.h | 2 ++ > fs/cifs/cifssmb.c | 17 ++++++++++++++--- > 3 files changed, 17 insertions(+), 4 deletions(-) > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h > index 56864a87..7f62c98 100644 > --- a/fs/cifs/cifsglob.h > +++ b/fs/cifs/cifsglob.h > @@ -1205,7 +1205,7 @@ struct cifs_writedata { > unsigned int tailsz; > unsigned int credits; > unsigned int nr_pages; > - struct page *pages[]; > + struct page **pages; Also same comment as for earlier patch, these are syntactically equivalent and maybe not needed to change. > }; > > /* > diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h > index 1f27d8e..7933c5f 100644 > --- a/fs/cifs/cifsproto.h > +++ b/fs/cifs/cifsproto.h > @@ -533,6 +533,8 @@ int cifs_async_writev(struct cifs_writedata *wdata, > void cifs_writev_complete(struct work_struct *work); > struct cifs_writedata *cifs_writedata_alloc(unsigned int nr_pages, > work_func_t complete); > +struct cifs_writedata *cifs_writedata_direct_alloc(struct page **pages, > + work_func_t complete); > void cifs_writedata_release(struct kref *refcount); > int cifs_query_mf_symlink(unsigned int xid, struct cifs_tcon *tcon, > struct cifs_sb_info *cifs_sb, > diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c > index c8e4278..5aca336 100644 > --- a/fs/cifs/cifssmb.c > +++ b/fs/cifs/cifssmb.c > @@ -1952,6 +1952,7 @@ cifs_writedata_release(struct kref *refcount) > if (wdata->cfile) > cifsFileInfo_put(wdata->cfile); > > + kvfree(wdata->pages); > kfree(wdata); > } > > @@ -2075,12 +2076,22 @@ cifs_writev_complete(struct work_struct *work) > struct cifs_writedata * > cifs_writedata_alloc(unsigned int nr_pages, work_func_t complete) > { > + struct page **pages = > + kzalloc(sizeof(struct page *) * nr_pages, GFP_NOFS); Why do you do a GFP_NOFS here but GFP_KERNEL in the earlier patch? > + if (pages) > + return cifs_writedata_direct_alloc(pages, complete); > + > + return NULL; > +} > + > +struct cifs_writedata * > +cifs_writedata_direct_alloc(struct page **pages, work_func_t complete) > +{ > struct cifs_writedata *wdata; > > - /* writedata + number of page pointers */ > - wdata = kzalloc(sizeof(*wdata) + > - sizeof(struct page *) * nr_pages, GFP_NOFS); > + wdata = kzalloc(sizeof(*wdata), GFP_NOFS); > if (wdata != NULL) { > + wdata->pages = pages; > kref_init(&wdata->refcount); > INIT_LIST_HEAD(&wdata->list); > init_completion(&wdata->done); > -- To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Subject: Re: [Patch v2 04/15] CIFS: Add support for direct pages in wdata > > On 5/30/2018 3:47 PM, Long Li wrote: > > From: Long Li <longli@microsoft.com> > > > > Add a function to allocate wdata without allocating pages for data > > transfer. This gives the caller an option to pass a number of pages > > that point to the data buffer to write to. > > > > wdata is reponsible for free those pages after it's done. > > Same comment as for the earlier patch. "Caller" is responsible for "freeing" > those pages? Confusing, as worded. > > > > > Signed-off-by: Long Li <longli@microsoft.com> > > --- > > fs/cifs/cifsglob.h | 2 +- > > fs/cifs/cifsproto.h | 2 ++ > > fs/cifs/cifssmb.c | 17 ++++++++++++++--- > > 3 files changed, 17 insertions(+), 4 deletions(-) > > > > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h index > > 56864a87..7f62c98 100644 > > --- a/fs/cifs/cifsglob.h > > +++ b/fs/cifs/cifsglob.h > > @@ -1205,7 +1205,7 @@ struct cifs_writedata { > > unsigned int tailsz; > > unsigned int credits; > > unsigned int nr_pages; > > - struct page *pages[]; > > + struct page **pages; > > Also same comment as for earlier patch, these are syntactically equivalent > and maybe not needed to change. > > > }; > > > > /* > > diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h index > > 1f27d8e..7933c5f 100644 > > --- a/fs/cifs/cifsproto.h > > +++ b/fs/cifs/cifsproto.h > > @@ -533,6 +533,8 @@ int cifs_async_writev(struct cifs_writedata *wdata, > > void cifs_writev_complete(struct work_struct *work); > > struct cifs_writedata *cifs_writedata_alloc(unsigned int nr_pages, > > work_func_t complete); > > +struct cifs_writedata *cifs_writedata_direct_alloc(struct page **pages, > > + work_func_t complete); > > void cifs_writedata_release(struct kref *refcount); > > int cifs_query_mf_symlink(unsigned int xid, struct cifs_tcon *tcon, > > struct cifs_sb_info *cifs_sb, > > diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c index > > c8e4278..5aca336 100644 > > --- a/fs/cifs/cifssmb.c > > +++ b/fs/cifs/cifssmb.c > > @@ -1952,6 +1952,7 @@ cifs_writedata_release(struct kref *refcount) > > if (wdata->cfile) > > cifsFileInfo_put(wdata->cfile); > > > > + kvfree(wdata->pages); > > kfree(wdata); > > } > > > > @@ -2075,12 +2076,22 @@ cifs_writev_complete(struct work_struct > *work) > > struct cifs_writedata * > > cifs_writedata_alloc(unsigned int nr_pages, work_func_t complete) > > { > > + struct page **pages = > > + kzalloc(sizeof(struct page *) * nr_pages, GFP_NOFS); > > Why do you do a GFP_NOFS here but GFP_KERNEL in the earlier patch? This is for the I/O write path. The earlier patch is for I/O read path. This code preserves the behavior from the buffer I/O code. The write path is possibly used when memory is under pressure, so it's better not to call FS (and CIFS) that may result in a loop. > > > + if (pages) > > + return cifs_writedata_direct_alloc(pages, complete); > > + > > + return NULL; > > +} > > + > > +struct cifs_writedata * > > +cifs_writedata_direct_alloc(struct page **pages, work_func_t > > +complete) { > > struct cifs_writedata *wdata; > > > > - /* writedata + number of page pointers */ > > - wdata = kzalloc(sizeof(*wdata) + > > - sizeof(struct page *) * nr_pages, GFP_NOFS); > > + wdata = kzalloc(sizeof(*wdata), GFP_NOFS); > > if (wdata != NULL) { > > + wdata->pages = pages; > > kref_init(&wdata->refcount); > > INIT_LIST_HEAD(&wdata->list); > > init_completion(&wdata->done); > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the > body of a message to majordomo@vger.kernel.org More majordomo info at > https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvger.ke > rnel.org%2Fmajordomo- > info.html&data=02%7C01%7Clongli%40microsoft.com%7C20e0bcbdcce3 > 402133ee08d5d9767977%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0% > 7C636654025229749549&sdata=p4P9AqS8cnOHErCFc1XALeaZxHuB7%2B > VOF10SjaaycAI%3D&reserved=0
diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h index 56864a87..7f62c98 100644 --- a/fs/cifs/cifsglob.h +++ b/fs/cifs/cifsglob.h @@ -1205,7 +1205,7 @@ struct cifs_writedata { unsigned int tailsz; unsigned int credits; unsigned int nr_pages; - struct page *pages[]; + struct page **pages; }; /* diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h index 1f27d8e..7933c5f 100644 --- a/fs/cifs/cifsproto.h +++ b/fs/cifs/cifsproto.h @@ -533,6 +533,8 @@ int cifs_async_writev(struct cifs_writedata *wdata, void cifs_writev_complete(struct work_struct *work); struct cifs_writedata *cifs_writedata_alloc(unsigned int nr_pages, work_func_t complete); +struct cifs_writedata *cifs_writedata_direct_alloc(struct page **pages, + work_func_t complete); void cifs_writedata_release(struct kref *refcount); int cifs_query_mf_symlink(unsigned int xid, struct cifs_tcon *tcon, struct cifs_sb_info *cifs_sb, diff --git a/fs/cifs/cifssmb.c b/fs/cifs/cifssmb.c index c8e4278..5aca336 100644 --- a/fs/cifs/cifssmb.c +++ b/fs/cifs/cifssmb.c @@ -1952,6 +1952,7 @@ cifs_writedata_release(struct kref *refcount) if (wdata->cfile) cifsFileInfo_put(wdata->cfile); + kvfree(wdata->pages); kfree(wdata); } @@ -2075,12 +2076,22 @@ cifs_writev_complete(struct work_struct *work) struct cifs_writedata * cifs_writedata_alloc(unsigned int nr_pages, work_func_t complete) { + struct page **pages = + kzalloc(sizeof(struct page *) * nr_pages, GFP_NOFS); + if (pages) + return cifs_writedata_direct_alloc(pages, complete); + + return NULL; +} + +struct cifs_writedata * +cifs_writedata_direct_alloc(struct page **pages, work_func_t complete) +{ struct cifs_writedata *wdata; - /* writedata + number of page pointers */ - wdata = kzalloc(sizeof(*wdata) + - sizeof(struct page *) * nr_pages, GFP_NOFS); + wdata = kzalloc(sizeof(*wdata), GFP_NOFS); if (wdata != NULL) { + wdata->pages = pages; kref_init(&wdata->refcount); INIT_LIST_HEAD(&wdata->list); init_completion(&wdata->done);