diff mbox series

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

Message ID 20180530194807.31657-3-longli@linuxonhyperv.com
State New
Headers show
Series CIFS: Add direct I/O support | expand

Commit Message

Long Li May 30, 2018, 7:47 p.m. UTC 
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.

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(-)

Comments

Ruhl, Michael J May 30, 2018, 8:27 p.m. UTC | #1 
>-----Original Message-----
>From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma-
>owner@vger.kernel.org] On Behalf Of Long Li
>Sent: Wednesday, May 30, 2018 3:48 PM
>To: 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
>Cc: Long Li <longli@microsoft.com>
>Subject: [Patch v2 02/15] CIFS: Add support for direct pages in rdata
>
>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.
>
>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;
> };
>
> 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);

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 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-cifs" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Long Li May 30, 2018, 8:57 p.m. UTC | #2 
> Subject: RE: [Patch v2 02/15] CIFS: Add support for direct pages in rdata
> 
> >-----Original Message-----
> >From: linux-rdma-owner@vger.kernel.org [mailto:linux-rdma-
> >owner@vger.kernel.org] On Behalf Of Long Li
> >Sent: Wednesday, May 30, 2018 3:48 PM
> >To: 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
> >Cc: Long Li <longli@microsoft.com>
> >Subject: [Patch v2 02/15] CIFS: Add support for direct pages in rdata
> >
> >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.
> >
> >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;
> > };
> >
> > 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);
> 
> 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 majordomo@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&data=02%7C01%7Clongli%40microsoft.com%7C
> >e810388a534643737ab108d5c66bd6df%7C72f988bf86f141af91ab2d7cd011db
> 47%7C1
> >%7C0%7C636633088833938755&sdata=iHKiji2rUhLHpbH5x13SJBWCvHExSr4a
> rz9Xiv3
> >1rMQ%3D&reserved=0
> --
> 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%7Ce810388a53464373
> 7ab108d5c66bd6df%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C63
> 6633088833938755&sdata=iHKiji2rUhLHpbH5x13SJBWCvHExSr4arz9Xiv31rMQ
> %3D&reserved=0
--
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
Tom Talpey June 24, 2018, 1:50 a.m. UTC | #3 
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?

> 
> 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);
>   }
>   
> 
--
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
Long Li June 25, 2018, 8:25 p.m. UTC | #4 
PiBGcm9tOiBUb20gVGFscGV5IDx0b21AdGFscGV5LmNvbT4NCj4gU2VudDogU2F0dXJkYXksIEp1
bmUgMjMsIDIwMTggNjo1MCBQTQ0KPiBUbzogTG9uZyBMaSA8bG9uZ2xpQG1pY3Jvc29mdC5jb20+
OyBTdGV2ZSBGcmVuY2ggPHNmcmVuY2hAc2FtYmEub3JnPjsNCj4gbGludXgtY2lmc0B2Z2VyLmtl
cm5lbC5vcmc7IHNhbWJhLXRlY2huaWNhbEBsaXN0cy5zYW1iYS5vcmc7IGxpbnV4LQ0KPiBrZXJu
ZWxAdmdlci5rZXJuZWwub3JnOyBsaW51eC1yZG1hQHZnZXIua2VybmVsLm9yZw0KPiBTdWJqZWN0
OiBSZTogW1BhdGNoIHYyIDAyLzE1XSBDSUZTOiBBZGQgc3VwcG9ydCBmb3IgZGlyZWN0IHBhZ2Vz
IGluIHJkYXRhDQo+IA0KPiBPbiA1LzMwLzIwMTggMzo0NyBQTSwgTG9uZyBMaSB3cm90ZToNCj4g
PiBGcm9tOiBMb25nIExpIDxsb25nbGlAbWljcm9zb2Z0LmNvbT4NCj4gPg0KPiA+IEFkZCBhIGZ1
bmN0aW9uIHRvIGFsbG9jYXRlIHJkYXRhIHdpdGhvdXQgYWxsb2NhdGluZyBwYWdlcyBmb3IgZGF0
YQ0KPiA+IHRyYW5zZmVyLiBUaGlzIGdpdmVzIHRoZSBjYWxsZXIgYW4gb3B0aW9uIHRvIHBhc3Mg
YSBudW1iZXIgb2YgcGFnZXMNCj4gPiB0aGF0IHBvaW50IHRvIHRoZSBkYXRhIGJ1ZmZlci4NCj4g
Pg0KPiA+IHJkYXRhIGlzIHN0aWxsIHJlcG9uc2libGUgZm9yIGZyZWUgdGhvc2UgcGFnZXMgYWZ0
ZXIgaXQncyBkb25lLg0KPiANCj4gIkNhbGxlciIgaXMgc3RpbGwgcmVzcG9uc2libGU/IE9yIGlz
IHRoZSByZGF0YSBzb21laG93IGZyZWVpbmcgaXRzZWxmIHZpYSBhbm90aGVyDQo+IG1lY2hhbmlz
bT8NCg0KSSBtZWFudCB0byBzYXkgZnJlZSB0aGUgYXJyYXkgInN0cnVjdCBwYWdlICoqcGFnZXMi
LCBub3QgdGhlIHBhZ2VzIHRoZW1zZWx2ZXMuDQoNCkJlZm9yZSB0aGUgcGF0Y2gsIHRoZSBwYWdl
IGFycmF5IGlzIGFsbG9jYXRlZCBhdCB0aGUgZW5kIG9mIHRoZSBzdHJ1Y3QgY2lmc19yZWFkZGF0
YToNCg0KICAgICAgcmRhdGEgPSBremFsbG9jKHNpemVvZigqcmRhdGEpICsgKHNpemVvZihzdHJ1
Y3QgcGFnZSAqKSAqIG5yX3BhZ2VzKSwNCiAgICAgICAgICAgICAgICAgICAgICAgR0ZQX0tFUk5F
TCk7DQoNCklmIHRoaXMgaXMgdGhlIGNhc2UsIHRoZXkgYXJlIGF1dG9tYXRpY2FsbHkgZnJlZWQg
d2hlbiBjaWZzX3JlYWRkYXRhIGlzIGZyZWVkLg0KDQpXaXRoIHRoZSBkaXJlY3QgSS9PIHBhdGNo
LCB0aGUgcGFnZSBhcnJheSBpcyBoYW5kbGVkIHNlcGFyYXRlbHkuIFNvIHRoZXkgbmVlZCB0byBi
ZSBmcmVlZCBhZnRlciBiZWluZyB1c2VkLg0KDQo+IA0KPiA+DQo+ID4gU2lnbmVkLW9mZi1ieTog
TG9uZyBMaSA8bG9uZ2xpQG1pY3Jvc29mdC5jb20+DQo+ID4gLS0tDQo+ID4gICBmcy9jaWZzL2Np
ZnNnbG9iLmggfCAgMiArLQ0KPiA+ICAgZnMvY2lmcy9maWxlLmMgICAgIHwgMjMgKysrKysrKysr
KysrKysrKysrKystLS0NCj4gPiAgIDIgZmlsZXMgY2hhbmdlZCwgMjEgaW5zZXJ0aW9ucygrKSwg
NCBkZWxldGlvbnMoLSkNCj4gPg0KPiA+IGRpZmYgLS1naXQgYS9mcy9jaWZzL2NpZnNnbG9iLmgg
Yi9mcy9jaWZzL2NpZnNnbG9iLmggaW5kZXgNCj4gPiA4ZDE2YzNlLi41Njg2NGE4NyAxMDA2NDQN
Cj4gPiAtLS0gYS9mcy9jaWZzL2NpZnNnbG9iLmgNCj4gPiArKysgYi9mcy9jaWZzL2NpZnNnbG9i
LmgNCj4gPiBAQCAtMTE3OSw3ICsxMTc5LDcgQEAgc3RydWN0IGNpZnNfcmVhZGRhdGEgew0KPiA+
ICAgCXVuc2lnbmVkIGludAkJCXRhaWxzejsNCj4gPiAgIAl1bnNpZ25lZCBpbnQJCQljcmVkaXRz
Ow0KPiA+ICAgCXVuc2lnbmVkIGludAkJCW5yX3BhZ2VzOw0KPiA+IC0Jc3RydWN0IHBhZ2UJCQkq
cGFnZXNbXTsNCj4gPiArCXN0cnVjdCBwYWdlCQkJKipwYWdlczsNCj4gDQo+IFRlY2huaWNhbGx5
IHNwZWFraW5nLCB0aGVzZSBhcmUgc3ludGFjdGljYWxseSBlcXVpdmFsZW50LiBJdCBtYXkgbm90
IGJlIHdvcnRoDQo+IGNoYW5naW5nIHRoaXMgaGlzdG9yaWMgZGVmaW5pdGlvbi4NCj4gDQo+IFRv
bS4NCj4gDQo+IA0KPiA+ICAgfTsNCj4gPg0KPiA+ICAgc3RydWN0IGNpZnNfd3JpdGVkYXRhOw0K
PiA+IGRpZmYgLS1naXQgYS9mcy9jaWZzL2ZpbGUuYyBiL2ZzL2NpZnMvZmlsZS5jIGluZGV4IDIz
ZmQ0MzAuLjFjOTgyOTMNCj4gPiAxMDA2NDQNCj4gPiAtLS0gYS9mcy9jaWZzL2ZpbGUuYw0KPiA+
ICsrKyBiL2ZzL2NpZnMvZmlsZS5jDQo+ID4gQEAgLTI4ODAsMTMgKzI4ODAsMTMgQEAgY2lmc19z
dHJpY3Rfd3JpdGV2KHN0cnVjdCBraW9jYiAqaW9jYiwgc3RydWN0DQo+IGlvdl9pdGVyICpmcm9t
KQ0KPiA+ICAgfQ0KPiA+DQo+ID4gICBzdGF0aWMgc3RydWN0IGNpZnNfcmVhZGRhdGEgKg0KPiA+
IC1jaWZzX3JlYWRkYXRhX2FsbG9jKHVuc2lnbmVkIGludCBucl9wYWdlcywgd29ya19mdW5jX3Qg
Y29tcGxldGUpDQo+ID4gK2NpZnNfcmVhZGRhdGFfZGlyZWN0X2FsbG9jKHN0cnVjdCBwYWdlICoq
cGFnZXMsIHdvcmtfZnVuY190IGNvbXBsZXRlKQ0KPiA+ICAgew0KPiA+ICAgCXN0cnVjdCBjaWZz
X3JlYWRkYXRhICpyZGF0YTsNCj4gPg0KPiA+IC0JcmRhdGEgPSBremFsbG9jKHNpemVvZigqcmRh
dGEpICsgKHNpemVvZihzdHJ1Y3QgcGFnZSAqKSAqIG5yX3BhZ2VzKSwNCj4gPiAtCQkJR0ZQX0tF
Uk5FTCk7DQo+ID4gKwlyZGF0YSA9IGt6YWxsb2Moc2l6ZW9mKCpyZGF0YSksIEdGUF9LRVJORUwp
Ow0KPiA+ICAgCWlmIChyZGF0YSAhPSBOVUxMKSB7DQo+ID4gKwkJcmRhdGEtPnBhZ2VzID0gcGFn
ZXM7DQo+ID4gICAJCWtyZWZfaW5pdCgmcmRhdGEtPnJlZmNvdW50KTsNCj4gPiAgIAkJSU5JVF9M
SVNUX0hFQUQoJnJkYXRhLT5saXN0KTsNCj4gPiAgIAkJaW5pdF9jb21wbGV0aW9uKCZyZGF0YS0+
ZG9uZSk7DQo+ID4gQEAgLTI4OTYsNiArMjg5NiwyMiBAQCBjaWZzX3JlYWRkYXRhX2FsbG9jKHVu
c2lnbmVkIGludCBucl9wYWdlcywNCj4gd29ya19mdW5jX3QgY29tcGxldGUpDQo+ID4gICAJcmV0
dXJuIHJkYXRhOw0KPiA+ICAgfQ0KPiA+DQo+ID4gK3N0YXRpYyBzdHJ1Y3QgY2lmc19yZWFkZGF0
YSAqDQo+ID4gK2NpZnNfcmVhZGRhdGFfYWxsb2ModW5zaWduZWQgaW50IG5yX3BhZ2VzLCB3b3Jr
X2Z1bmNfdCBjb21wbGV0ZSkgew0KPiA+ICsJc3RydWN0IHBhZ2UgKipwYWdlcyA9DQo+ID4gKwkJ
a3phbGxvYyhzaXplb2Yoc3RydWN0IHBhZ2UgKikgKiBucl9wYWdlcywgR0ZQX0tFUk5FTCk7DQo+
ID4gKwlzdHJ1Y3QgY2lmc19yZWFkZGF0YSAqcmV0ID0gTlVMTDsNCj4gPiArDQo+ID4gKwlpZiAo
cGFnZXMpIHsNCj4gPiArCQlyZXQgPSBjaWZzX3JlYWRkYXRhX2RpcmVjdF9hbGxvYyhwYWdlcywg
Y29tcGxldGUpOw0KPiA+ICsJCWlmICghcmV0KQ0KPiA+ICsJCQlrZnJlZShwYWdlcyk7DQo+ID4g
Kwl9DQo+ID4gKw0KPiA+ICsJcmV0dXJuIHJldDsNCj4gPiArfQ0KPiA+ICsNCj4gPiAgIHZvaWQN
Cj4gPiAgIGNpZnNfcmVhZGRhdGFfcmVsZWFzZShzdHJ1Y3Qga3JlZiAqcmVmY291bnQpDQo+ID4g
ICB7DQo+ID4gQEAgLTI5MTAsNiArMjkyNiw3IEBAIGNpZnNfcmVhZGRhdGFfcmVsZWFzZShzdHJ1
Y3Qga3JlZiAqcmVmY291bnQpDQo+ID4gICAJaWYgKHJkYXRhLT5jZmlsZSkNCj4gPiAgIAkJY2lm
c0ZpbGVJbmZvX3B1dChyZGF0YS0+Y2ZpbGUpOw0KPiA+DQo+ID4gKwlrdmZyZWUocmRhdGEtPnBh
Z2VzKTsNCj4gPiAgIAlrZnJlZShyZGF0YSk7DQo+ID4gICB9DQo+ID4NCj4gPg0K
--
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
Jason Gunthorpe June 25, 2018, 9:01 p.m. UTC | #5 
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 <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?
> 
> >
> >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
> >+++ 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.

[] is a C99 'flex array', it has a different allocation behavior than
** and is not interchangeable..

Jason
--
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
Tom Talpey June 26, 2018, 3:13 p.m. UTC | #6 
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 <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?
>>
>>>
>>> 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
>>> +++ 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.
> 
> [] 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.

--
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
Long Li June 27, 2018, 3:21 a.m. UTC | #7 
PiBTdWJqZWN0OiBSZTogW1BhdGNoIHYyIDAyLzE1XSBDSUZTOiBBZGQgc3VwcG9ydCBmb3IgZGly
ZWN0IHBhZ2VzIGluIHJkYXRhDQo+IA0KPiBPbiA2LzI1LzIwMTggNTowMSBQTSwgSmFzb24gR3Vu
dGhvcnBlIHdyb3RlOg0KPiA+IE9uIFNhdCwgSnVuIDIzLCAyMDE4IGF0IDA5OjUwOjIwUE0gLTA0
MDAsIFRvbSBUYWxwZXkgd3JvdGU6DQo+ID4+IE9uIDUvMzAvMjAxOCAzOjQ3IFBNLCBMb25nIExp
IHdyb3RlOg0KPiA+Pj4gRnJvbTogTG9uZyBMaSA8bG9uZ2xpQG1pY3Jvc29mdC5jb20+DQo+ID4+
Pg0KPiA+Pj4gQWRkIGEgZnVuY3Rpb24gdG8gYWxsb2NhdGUgcmRhdGEgd2l0aG91dCBhbGxvY2F0
aW5nIHBhZ2VzIGZvciBkYXRhDQo+ID4+PiB0cmFuc2Zlci4gVGhpcyBnaXZlcyB0aGUgY2FsbGVy
IGFuIG9wdGlvbiB0byBwYXNzIGEgbnVtYmVyIG9mIHBhZ2VzDQo+ID4+PiB0aGF0IHBvaW50IHRv
IHRoZSBkYXRhIGJ1ZmZlci4NCj4gPj4+DQo+ID4+PiByZGF0YSBpcyBzdGlsbCByZXBvbnNpYmxl
IGZvciBmcmVlIHRob3NlIHBhZ2VzIGFmdGVyIGl0J3MgZG9uZS4NCj4gPj4NCj4gPj4gIkNhbGxl
ciIgaXMgc3RpbGwgcmVzcG9uc2libGU/IE9yIGlzIHRoZSByZGF0YSBzb21laG93IGZyZWVpbmcg
aXRzZWxmDQo+ID4+IHZpYSBhbm90aGVyIG1lY2hhbmlzbT8NCj4gPj4NCj4gPj4+DQo+ID4+PiBT
aWduZWQtb2ZmLWJ5OiBMb25nIExpIDxsb25nbGlAbWljcm9zb2Z0LmNvbT4NCj4gPj4+ICAgZnMv
Y2lmcy9jaWZzZ2xvYi5oIHwgIDIgKy0NCj4gPj4+ICAgZnMvY2lmcy9maWxlLmMgICAgIHwgMjMg
KysrKysrKysrKysrKysrKysrKystLS0NCj4gPj4+ICAgMiBmaWxlcyBjaGFuZ2VkLCAyMSBpbnNl
cnRpb25zKCspLCA0IGRlbGV0aW9ucygtKQ0KPiA+Pj4NCj4gPj4+IGRpZmYgLS1naXQgYS9mcy9j
aWZzL2NpZnNnbG9iLmggYi9mcy9jaWZzL2NpZnNnbG9iLmggaW5kZXgNCj4gPj4+IDhkMTZjM2Uu
LjU2ODY0YTg3IDEwMDY0NA0KPiA+Pj4gKysrIGIvZnMvY2lmcy9jaWZzZ2xvYi5oDQo+ID4+PiBA
QCAtMTE3OSw3ICsxMTc5LDcgQEAgc3RydWN0IGNpZnNfcmVhZGRhdGEgew0KPiA+Pj4gICAJdW5z
aWduZWQgaW50CQkJdGFpbHN6Ow0KPiA+Pj4gICAJdW5zaWduZWQgaW50CQkJY3JlZGl0czsNCj4g
Pj4+ICAgCXVuc2lnbmVkIGludAkJCW5yX3BhZ2VzOw0KPiA+Pj4gLQlzdHJ1Y3QgcGFnZQkJCSpw
YWdlc1tdOw0KPiA+Pj4gKwlzdHJ1Y3QgcGFnZQkJCSoqcGFnZXM7DQo+ID4+DQo+ID4+IFRlY2hu
aWNhbGx5IHNwZWFraW5nLCB0aGVzZSBhcmUgc3ludGFjdGljYWxseSBlcXVpdmFsZW50LiBJdCBt
YXkgbm90DQo+ID4+IGJlIHdvcnRoIGNoYW5naW5nIHRoaXMgaGlzdG9yaWMgZGVmaW5pdGlvbi4N
Cj4gPg0KPiA+IFtdIGlzIGEgQzk5ICdmbGV4IGFycmF5JywgaXQgaGFzIGEgZGlmZmVyZW50IGFs
bG9jYXRpb24gYmVoYXZpb3IgdGhhbg0KPiA+ICoqIGFuZCBpcyBub3QgaW50ZXJjaGFuZ2VhYmxl
Li4NCj4gDQo+IEluIHRoYXQgY2FzZSwgaXQncyBhbiBldmVuIGJldHRlciByZWFzb24gdG8gbm90
IGNoYW5nZSB0aGUgZGVjbGFyYXRpb24uDQoNCk5vLCBpdCBuZWVkcyB0byBiZSBkZWNsYXJlZCBz
ZXBhcmF0ZWx5Lg0KDQpXaXRoIERpcmVjdCBJL08sICoqcGFnZXMgYXJlIGFsbG9jYXRlZCBhbmQg
cmV0dXJuZWQgZnJvbSBpb3ZfaXRlcl9nZXRfcGFnZXNfYWxsb2MoKSB3aGVuIGxvY2tpbmcgdGhv
c2UgdXNlciBwYWdlcy4gVGhleSBjYW4ndCBiZSBhbGxvY2F0ZWQgYXMgcGFydCBvZiBzdHJ1Y3Qg
Y2lmc19yZWFkZGF0YS4NCg0K
--
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
diff mbox series

Patch

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;
 };
 
 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);
 }