RE: [Patch v2 02/15] CIFS: Add support for direct pages in rdata

[Long Li <longli@microsoft.com>]

> From: Tom Talpey <tom@talpey.com>
> Sent: Saturday, June 23, 2018 6:50 PM
> To: Long Li <longli@microsoft.com>; Steve French <sfrench@samba.org>;
> linux-cifs@vger.kernel.org; samba-technical@lists.samba.org; linux-
> kernel@vger.kernel.org; linux-rdma@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 <longli@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 <longli@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);
> >   }
> >
> >