> From: Tom Talpey <t...@talpey.com>
> Sent: Saturday, June 23, 2018 6:50 PM
> To: Long Li <lon...@microsoft.com>; Steve French <sfre...@samba.org>;
> linux-c...@vger.kernel.org; samba-techni...@lists.samba.org; linux-
> ker...@vger.kernel.org; linux-r...@vger.kernel.org
> Subject: Re: [Patch v2 02/15] CIFS: Add support for direct pages in rdata
>
> On 5/30/2018 3:47 PM, Long Li wrote:
> > From: Long Li <lon...@microsoft.com>
> >
> > Add a function to allocate rdata without allocating pages for data
> > transfer. This gives the caller an option to pass a number of pages
> > that point to the data buffer.
> >
> > rdata is still reponsible for free those pages after it's done.
>
> "Caller" is still responsible? Or is the rdata somehow freeing itself via
> another
> mechanism?
I meant to say free the array "struct page **pages", not the pages themselves.
Before the patch, the page array is allocated at the end of the struct
cifs_readdata:
rdata = kzalloc(sizeof(*rdata) + (sizeof(struct page *) * nr_pages),
GFP_KERNEL);
If this is the case, they are automatically freed when cifs_readdata is freed.
With the direct I/O patch, the page array is handled separately. So they need
to be freed after being used.
>
> >
> > Signed-off-by: Long Li <lon...@microsoft.com>
> > ---
> > fs/cifs/cifsglob.h | 2 +-
> > fs/cifs/file.c | 23 ++++++++++++++++++++---
> > 2 files changed, 21 insertions(+), 4 deletions(-)
> >
> > diff --git a/fs/cifs/cifsglob.h b/fs/cifs/cifsglob.h index
> > 8d16c3e..56864a87 100644
> > --- a/fs/cifs/cifsglob.h
> > +++ b/fs/cifs/cifsglob.h
> > @@ -1179,7 +1179,7 @@ struct cifs_readdata {
> > unsigned int tailsz;
> > unsigned int credits;
> > unsigned int nr_pages;
> > - struct page *pages[];
> > + struct page **pages;
>
> Technically speaking, these are syntactically equivalent. It may not be worth
> changing this historic definition.
>
> Tom.
>
>
> > };
> >
> > struct cifs_writedata;
> > diff --git a/fs/cifs/file.c b/fs/cifs/file.c index 23fd430..1c98293
> > 100644
> > --- a/fs/cifs/file.c
> > +++ b/fs/cifs/file.c
> > @@ -2880,13 +2880,13 @@ cifs_strict_writev(struct kiocb *iocb, struct
> iov_iter *from)
> > }
> >
> > static struct cifs_readdata *
> > -cifs_readdata_alloc(unsigned int nr_pages, work_func_t complete)
> > +cifs_readdata_direct_alloc(struct page **pages, work_func_t complete)
> > {
> > struct cifs_readdata *rdata;
> >
> > - rdata = kzalloc(sizeof(*rdata) + (sizeof(struct page *) * nr_pages),
> > - GFP_KERNEL);
> > + rdata = kzalloc(sizeof(*rdata), GFP_KERNEL);
> > if (rdata != NULL) {
> > + rdata->pages = pages;
> > kref_init(&rdata->refcount);
> > INIT_LIST_HEAD(&rdata->list);
> > init_completion(&rdata->done);
> > @@ -2896,6 +2896,22 @@ cifs_readdata_alloc(unsigned int nr_pages,
> work_func_t complete)
> > return rdata;
> > }
> >
> > +static struct cifs_readdata *
> > +cifs_readdata_alloc(unsigned int nr_pages, work_func_t complete) {
> > + struct page **pages =
> > + kzalloc(sizeof(struct page *) * nr_pages, GFP_KERNEL);
> > + struct cifs_readdata *ret = NULL;
> > +
> > + if (pages) {
> > + ret = cifs_readdata_direct_alloc(pages, complete);
> > + if (!ret)
> > + kfree(pages);
> > + }
> > +
> > + return ret;
> > +}
> > +
> > void
> > cifs_readdata_release(struct kref *refcount)
> > {
> > @@ -2910,6 +2926,7 @@ cifs_readdata_release(struct kref *refcount)
> > if (rdata->cfile)
> > cifsFileInfo_put(rdata->cfile);
> >
> > + kvfree(rdata->pages);
> > kfree(rdata);
> > }
> >
> >
