RE: [Patch v2 02/15] CIFS: Add support for direct pages in rdata
> Subject: Re: [Patch v2 02/15] CIFS: Add support for direct pages in rdata
>
> On 6/25/2018 5:01 PM, Jason Gunthorpe wrote:
> > On Sat, Jun 23, 2018 at 09:50:20PM -0400, Tom Talpey wrote:
> >> On 5/30/2018 3:47 PM, Long Li wrote:
> >>> From: Long Li
> >>>
> >>> 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?
> >>
> >>>
> >>> Signed-off-by: Long Li
> >>> 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
> >>> +++ b/fs/cifs/cifsglob.h
> >>> @@ -1179,7 +1179,7 @@ struct cifs_readdata {
> >>> unsigned inttailsz;
> >>> unsigned intcredits;
> >>> unsigned intnr_pages;
> >>> - struct page *pages[];
> >>> + struct page **pages;
> >>
> >> Technically speaking, these are syntactically equivalent. It may not
> >> be worth changing this historic definition.
> >
> > [] is a C99 'flex array', it has a different allocation behavior than
> > ** and is not interchangeable..
>
> In that case, it's an even better reason to not change the declaration.
No, it needs to be declared separately.
With Direct I/O, **pages are allocated and returned from
iov_iter_get_pages_alloc() when locking those user pages. They can't be
allocated as part of struct cifs_readdata.
RE: [Patch v2 02/15] CIFS: Add support for direct pages in rdata
> Subject: Re: [Patch v2 02/15] CIFS: Add support for direct pages in rdata
>
> On 6/25/2018 5:01 PM, Jason Gunthorpe wrote:
> > On Sat, Jun 23, 2018 at 09:50:20PM -0400, Tom Talpey wrote:
> >> On 5/30/2018 3:47 PM, Long Li wrote:
> >>> From: Long Li
> >>>
> >>> 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?
> >>
> >>>
> >>> Signed-off-by: Long Li
> >>> 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
> >>> +++ b/fs/cifs/cifsglob.h
> >>> @@ -1179,7 +1179,7 @@ struct cifs_readdata {
> >>> unsigned inttailsz;
> >>> unsigned intcredits;
> >>> unsigned intnr_pages;
> >>> - struct page *pages[];
> >>> + struct page **pages;
> >>
> >> Technically speaking, these are syntactically equivalent. It may not
> >> be worth changing this historic definition.
> >
> > [] is a C99 'flex array', it has a different allocation behavior than
> > ** and is not interchangeable..
>
> In that case, it's an even better reason to not change the declaration.
No, it needs to be declared separately.
With Direct I/O, **pages are allocated and returned from
iov_iter_get_pages_alloc() when locking those user pages. They can't be
allocated as part of struct cifs_readdata.
Re: [Patch v2 02/15] CIFS: Add support for direct pages in rdata
On 6/25/2018 5:01 PM, Jason Gunthorpe wrote:
On Sat, Jun 23, 2018 at 09:50:20PM -0400, Tom Talpey wrote:
On 5/30/2018 3:47 PM, Long Li wrote:
From: Long Li
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?
Signed-off-by: Long Li
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
+++ b/fs/cifs/cifsglob.h
@@ -1179,7 +1179,7 @@ struct cifs_readdata {
unsigned inttailsz;
unsigned intcredits;
unsigned intnr_pages;
- struct page *pages[];
+ struct page **pages;
Technically speaking, these are syntactically equivalent. It may not
be worth changing this historic definition.
[] is a C99 'flex array', it has a different allocation behavior than
** and is not interchangeable..
In that case, it's an even better reason to not change the declaration.
Tom.
Re: [Patch v2 02/15] CIFS: Add support for direct pages in rdata
On 6/25/2018 5:01 PM, Jason Gunthorpe wrote:
On Sat, Jun 23, 2018 at 09:50:20PM -0400, Tom Talpey wrote:
On 5/30/2018 3:47 PM, Long Li wrote:
From: Long Li
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?
Signed-off-by: Long Li
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
+++ b/fs/cifs/cifsglob.h
@@ -1179,7 +1179,7 @@ struct cifs_readdata {
unsigned inttailsz;
unsigned intcredits;
unsigned intnr_pages;
- struct page *pages[];
+ struct page **pages;
Technically speaking, these are syntactically equivalent. It may not
be worth changing this historic definition.
[] is a C99 'flex array', it has a different allocation behavior than
** and is not interchangeable..
In that case, it's an even better reason to not change the declaration.
Tom.
Re: [Patch v2 02/15] CIFS: Add support for direct pages in rdata
On Sat, Jun 23, 2018 at 09:50:20PM -0400, Tom Talpey wrote:
> On 5/30/2018 3:47 PM, Long Li wrote:
> >From: Long Li
> >
> >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?
>
> >
> >Signed-off-by: Long Li
> > 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
> >+++ b/fs/cifs/cifsglob.h
> >@@ -1179,7 +1179,7 @@ struct cifs_readdata {
> > unsigned inttailsz;
> > unsigned intcredits;
> > unsigned intnr_pages;
> >-struct page *pages[];
> >+struct page **pages;
>
> Technically speaking, these are syntactically equivalent. It may not
> be worth changing this historic definition.
[] is a C99 'flex array', it has a different allocation behavior than
** and is not interchangeable..
Jason
Re: [Patch v2 02/15] CIFS: Add support for direct pages in rdata
On Sat, Jun 23, 2018 at 09:50:20PM -0400, Tom Talpey wrote:
> On 5/30/2018 3:47 PM, Long Li wrote:
> >From: Long Li
> >
> >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?
>
> >
> >Signed-off-by: Long Li
> > 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
> >+++ b/fs/cifs/cifsglob.h
> >@@ -1179,7 +1179,7 @@ struct cifs_readdata {
> > unsigned inttailsz;
> > unsigned intcredits;
> > unsigned intnr_pages;
> >-struct page *pages[];
> >+struct page **pages;
>
> Technically speaking, these are syntactically equivalent. It may not
> be worth changing this historic definition.
[] is a C99 'flex array', it has a different allocation behavior than
** and is not interchangeable..
Jason
RE: [Patch v2 02/15] CIFS: Add support for direct pages in rdata
> From: Tom Talpey
> Sent: Saturday, June 23, 2018 6:50 PM
> To: Long Li ; Steve French ;
> 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
> >
> > 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
> > ---
> > 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 inttailsz;
> > unsigned intcredits;
> > unsigned intnr_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(>refcount);
> > INIT_LIST_HEAD(>list);
> > init_completion(>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);
> > }
> >
> >
RE: [Patch v2 02/15] CIFS: Add support for direct pages in rdata
> From: Tom Talpey
> Sent: Saturday, June 23, 2018 6:50 PM
> To: Long Li ; Steve French ;
> 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
> >
> > 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
> > ---
> > 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 inttailsz;
> > unsigned intcredits;
> > unsigned intnr_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(>refcount);
> > INIT_LIST_HEAD(>list);
> > init_completion(>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);
> > }
> >
> >
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
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?
Signed-off-by: Long Li
---
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 inttailsz;
unsigned intcredits;
unsigned intnr_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(>refcount);
INIT_LIST_HEAD(>list);
init_completion(>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);
}
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
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?
Signed-off-by: Long Li
---
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 inttailsz;
unsigned intcredits;
unsigned intnr_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(>refcount);
INIT_LIST_HEAD(>list);
init_completion(>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);
}
RE: [Patch v2 02/15] CIFS: Add support for direct pages in rdata
> Subject: RE: [Patch v2 02/15] CIFS: Add support for direct pages in rdata
>
> >-Original Message-
> >From: linux-rdma-ow...@vger.kernel.org [mailto:linux-rdma-
> >ow...@vger.kernel.org] On Behalf Of Long Li
> >Sent: Wednesday, May 30, 2018 3:48 PM
> >To: Steve French ; linux-c...@vger.kernel.org;
> >samba- techni...@lists.samba.org; linux-kernel@vger.kernel.org; linux-
> >r...@vger.kernel.org
> >Cc: Long Li
> >Subject: [Patch v2 02/15] CIFS: Add support for direct pages in rdata
> >
> >From: Long Li
> >
> >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.
> >
> >Signed-off-by: Long Li
> >---
> > 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 inttailsz;
> > unsigned intcredits;
> > unsigned intnr_pages;
> >-struct page *pages[];
> >+struct page **pages;
> > };
> >
> > 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(>refcount);
> > INIT_LIST_HEAD(>list);
> > init_completion(>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);
>
> Is the kvfree() correct?
>
> You use kzalloc() and kfree in cifs_readdata_alloc().
This function is shared by both non-direct I/O and direct I/O code paths.
Direct I/O uses kvmalloc to allocate pages, so kvfree is used here to handle
both cases.
>
> Mike
>
> > kfree(rdata);
> > }
> >
> >--
> >2.7.4
> >
> >--
> >To unsubscribe from this list: send the line "unsubscribe linux-rdma"
> >in the body of a message to majord...@vger.kernel.org More majordomo
> >info at
> >https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvger.k
> e
> >rnel.org%2Fmajordomo-
> info.html=02%7C01%7Clongli%40microsoft.com%7C
> >e810388a534643737ab108d5c66bd6df%7C72f988bf86f141af91ab2d7cd011db
> 47%7C1
> >%7C0%7C636633088833938755=iHKiji2rUhLHpbH5x13SJBWCvHExSr4a
> rz9Xiv3
> >1rMQ%3D=0
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the
> body of a message to majord...@vger.kernel.org More majordomo info at
> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvger.ke
> rnel.org%2Fmajordomo-
> info.html=02%7C01%7Clongli%40microsoft.com%7Ce810388a53464373
> 7ab108d5c66bd6df%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63
> 6633088833938755=iHKiji2rUhLHpbH5x13SJBWCvHExSr4arz9Xiv31rMQ
> %3D=0
RE: [Patch v2 02/15] CIFS: Add support for direct pages in rdata
> Subject: RE: [Patch v2 02/15] CIFS: Add support for direct pages in rdata
>
> >-Original Message-
> >From: linux-rdma-ow...@vger.kernel.org [mailto:linux-rdma-
> >ow...@vger.kernel.org] On Behalf Of Long Li
> >Sent: Wednesday, May 30, 2018 3:48 PM
> >To: Steve French ; linux-c...@vger.kernel.org;
> >samba- techni...@lists.samba.org; linux-kernel@vger.kernel.org; linux-
> >r...@vger.kernel.org
> >Cc: Long Li
> >Subject: [Patch v2 02/15] CIFS: Add support for direct pages in rdata
> >
> >From: Long Li
> >
> >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.
> >
> >Signed-off-by: Long Li
> >---
> > 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 inttailsz;
> > unsigned intcredits;
> > unsigned intnr_pages;
> >-struct page *pages[];
> >+struct page **pages;
> > };
> >
> > 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(>refcount);
> > INIT_LIST_HEAD(>list);
> > init_completion(>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);
>
> Is the kvfree() correct?
>
> You use kzalloc() and kfree in cifs_readdata_alloc().
This function is shared by both non-direct I/O and direct I/O code paths.
Direct I/O uses kvmalloc to allocate pages, so kvfree is used here to handle
both cases.
>
> Mike
>
> > kfree(rdata);
> > }
> >
> >--
> >2.7.4
> >
> >--
> >To unsubscribe from this list: send the line "unsubscribe linux-rdma"
> >in the body of a message to majord...@vger.kernel.org More majordomo
> >info at
> >https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvger.k
> e
> >rnel.org%2Fmajordomo-
> info.html=02%7C01%7Clongli%40microsoft.com%7C
> >e810388a534643737ab108d5c66bd6df%7C72f988bf86f141af91ab2d7cd011db
> 47%7C1
> >%7C0%7C636633088833938755=iHKiji2rUhLHpbH5x13SJBWCvHExSr4a
> rz9Xiv3
> >1rMQ%3D=0
> --
> To unsubscribe from this list: send the line "unsubscribe linux-cifs" in the
> body of a message to majord...@vger.kernel.org More majordomo info at
> https://na01.safelinks.protection.outlook.com/?url=http%3A%2F%2Fvger.ke
> rnel.org%2Fmajordomo-
> info.html=02%7C01%7Clongli%40microsoft.com%7Ce810388a53464373
> 7ab108d5c66bd6df%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63
> 6633088833938755=iHKiji2rUhLHpbH5x13SJBWCvHExSr4arz9Xiv31rMQ
> %3D=0
RE: [Patch v2 02/15] CIFS: Add support for direct pages in rdata
>-Original Message-
>From: linux-rdma-ow...@vger.kernel.org [mailto:linux-rdma-
>ow...@vger.kernel.org] On Behalf Of Long Li
>Sent: Wednesday, May 30, 2018 3:48 PM
>To: Steve French ; linux-c...@vger.kernel.org; samba-
>techni...@lists.samba.org; linux-kernel@vger.kernel.org; linux-
>r...@vger.kernel.org
>Cc: Long Li
>Subject: [Patch v2 02/15] CIFS: Add support for direct pages in rdata
>
>From: Long Li
>
>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.
>
>Signed-off-by: Long Li
>---
> 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 inttailsz;
> unsigned intcredits;
> unsigned intnr_pages;
>- struct page *pages[];
>+ struct page **pages;
> };
>
> 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(>refcount);
> INIT_LIST_HEAD(>list);
> init_completion(>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);
Is the kvfree() correct?
You use kzalloc() and kfree in cifs_readdata_alloc().
Mike
> kfree(rdata);
> }
>
>--
>2.7.4
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
>the body of a message to majord...@vger.kernel.org
>More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [Patch v2 02/15] CIFS: Add support for direct pages in rdata
>-Original Message-
>From: linux-rdma-ow...@vger.kernel.org [mailto:linux-rdma-
>ow...@vger.kernel.org] On Behalf Of Long Li
>Sent: Wednesday, May 30, 2018 3:48 PM
>To: Steve French ; linux-c...@vger.kernel.org; samba-
>techni...@lists.samba.org; linux-kernel@vger.kernel.org; linux-
>r...@vger.kernel.org
>Cc: Long Li
>Subject: [Patch v2 02/15] CIFS: Add support for direct pages in rdata
>
>From: Long Li
>
>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.
>
>Signed-off-by: Long Li
>---
> 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 inttailsz;
> unsigned intcredits;
> unsigned intnr_pages;
>- struct page *pages[];
>+ struct page **pages;
> };
>
> 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(>refcount);
> INIT_LIST_HEAD(>list);
> init_completion(>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);
Is the kvfree() correct?
You use kzalloc() and kfree in cifs_readdata_alloc().
Mike
> kfree(rdata);
> }
>
>--
>2.7.4
>
>--
>To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
>the body of a message to majord...@vger.kernel.org
>More majordomo info at http://vger.kernel.org/majordomo-info.html
