[v2,05/15] CIFS: Calculate the correct request length based on page offset and tail size

Message ID 20180530194807.31657-6-longli@linuxonhyperv.com
State New
Headers show
Series
  • CIFS: Add direct I/O support
Related show

Commit Message

Long Li May 30, 2018, 7:47 p.m.
From: Long Li <longli@microsoft.com>

It's possible that the page offset is non-zero in the pages in a request,
change the function to calculate the correct data buffer length.

Signed-off-by: Long Li <longli@microsoft.com>
---
 fs/cifs/transport.c | 20 +++++++++++++++++---
 1 file changed, 17 insertions(+), 3 deletions(-)

Comments

Tom Talpey June 24, 2018, 2:07 a.m. | #1
On 5/30/2018 3:47 PM, Long Li wrote:
> From: Long Li <longli@microsoft.com>
> 
> It's possible that the page offset is non-zero in the pages in a request,
> change the function to calculate the correct data buffer length.
> 
> Signed-off-by: Long Li <longli@microsoft.com>
> ---
>   fs/cifs/transport.c | 20 +++++++++++++++++---
>   1 file changed, 17 insertions(+), 3 deletions(-)
> 
> diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
> index 927226a..d6b5523 100644
> --- a/fs/cifs/transport.c
> +++ b/fs/cifs/transport.c
> @@ -212,10 +212,24 @@ rqst_len(struct smb_rqst *rqst)
>   	for (i = 0; i < rqst->rq_nvec; i++)
>   		buflen += iov[i].iov_len;
>   
> -	/* add in the page array if there is one */
> +	/*
> +	 * Add in the page array if there is one. The caller needs to make
> +	 * sure rq_offset and rq_tailsz are set correctly. If a buffer of
> +	 * multiple pages ends at page boundary, rq_tailsz needs to be set to
> +	 * PAGE_SIZE.
> +	 */
>   	if (rqst->rq_npages) {
> -		buflen += rqst->rq_pagesz * (rqst->rq_npages - 1);
> -		buflen += rqst->rq_tailsz;
> +		if (rqst->rq_npages == 1)
> +			buflen += rqst->rq_tailsz;
> +		else {
> +			/*
> +			 * If there is more than one page, calculate the
> +			 * buffer length based on rq_offset and rq_tailsz
> +			 */
> +			buflen += rqst->rq_pagesz * (rqst->rq_npages - 1) -
> +					rqst->rq_offset;
> +			buflen += rqst->rq_tailsz;
> +		}

Wouldn't it be simpler to keep the original code, but then just subtract
the rq_offset?

	buflen += rqst->rq_pagesz * (rqst->rq_npages - 1);
	buflen += rqst->rq_tailsz;
	buflen -= rqst->rq_offset;

It's kind of confusing as written. Also, what if it's just one page, but
has a non-zero offset? Is that somehow not possible? My suggested code
would take that into account, yours doesn't.

Tom.

>   	}
>   
>   	return buflen;
> 
--
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, 9:07 p.m. | #2
PiBTdWJqZWN0OiBSZTogW1BhdGNoIHYyIDA1LzE1XSBDSUZTOiBDYWxjdWxhdGUgdGhlIGNvcnJl
Y3QgcmVxdWVzdCBsZW5ndGggYmFzZWQNCj4gb24gcGFnZSBvZmZzZXQgYW5kIHRhaWwgc2l6ZQ0K
PiANCj4gT24gNS8zMC8yMDE4IDM6NDcgUE0sIExvbmcgTGkgd3JvdGU6DQo+ID4gRnJvbTogTG9u
ZyBMaSA8bG9uZ2xpQG1pY3Jvc29mdC5jb20+DQo+ID4NCj4gPiBJdCdzIHBvc3NpYmxlIHRoYXQg
dGhlIHBhZ2Ugb2Zmc2V0IGlzIG5vbi16ZXJvIGluIHRoZSBwYWdlcyBpbiBhDQo+ID4gcmVxdWVz
dCwgY2hhbmdlIHRoZSBmdW5jdGlvbiB0byBjYWxjdWxhdGUgdGhlIGNvcnJlY3QgZGF0YSBidWZm
ZXIgbGVuZ3RoLg0KPiA+DQo+ID4gU2lnbmVkLW9mZi1ieTogTG9uZyBMaSA8bG9uZ2xpQG1pY3Jv
c29mdC5jb20+DQo+ID4gLS0tDQo+ID4gICBmcy9jaWZzL3RyYW5zcG9ydC5jIHwgMjAgKysrKysr
KysrKysrKysrKystLS0NCj4gPiAgIDEgZmlsZSBjaGFuZ2VkLCAxNyBpbnNlcnRpb25zKCspLCAz
IGRlbGV0aW9ucygtKQ0KPiA+DQo+ID4gZGlmZiAtLWdpdCBhL2ZzL2NpZnMvdHJhbnNwb3J0LmMg
Yi9mcy9jaWZzL3RyYW5zcG9ydC5jIGluZGV4DQo+ID4gOTI3MjI2YS4uZDZiNTUyMyAxMDA2NDQN
Cj4gPiAtLS0gYS9mcy9jaWZzL3RyYW5zcG9ydC5jDQo+ID4gKysrIGIvZnMvY2lmcy90cmFuc3Bv
cnQuYw0KPiA+IEBAIC0yMTIsMTAgKzIxMiwyNCBAQCBycXN0X2xlbihzdHJ1Y3Qgc21iX3Jxc3Qg
KnJxc3QpDQo+ID4gICAJZm9yIChpID0gMDsgaSA8IHJxc3QtPnJxX252ZWM7IGkrKykNCj4gPiAg
IAkJYnVmbGVuICs9IGlvdltpXS5pb3ZfbGVuOw0KPiA+DQo+ID4gLQkvKiBhZGQgaW4gdGhlIHBh
Z2UgYXJyYXkgaWYgdGhlcmUgaXMgb25lICovDQo+ID4gKwkvKg0KPiA+ICsJICogQWRkIGluIHRo
ZSBwYWdlIGFycmF5IGlmIHRoZXJlIGlzIG9uZS4gVGhlIGNhbGxlciBuZWVkcyB0byBtYWtlDQo+
ID4gKwkgKiBzdXJlIHJxX29mZnNldCBhbmQgcnFfdGFpbHN6IGFyZSBzZXQgY29ycmVjdGx5LiBJ
ZiBhIGJ1ZmZlciBvZg0KPiA+ICsJICogbXVsdGlwbGUgcGFnZXMgZW5kcyBhdCBwYWdlIGJvdW5k
YXJ5LCBycV90YWlsc3ogbmVlZHMgdG8gYmUgc2V0IHRvDQo+ID4gKwkgKiBQQUdFX1NJWkUuDQo+
ID4gKwkgKi8NCj4gPiAgIAlpZiAocnFzdC0+cnFfbnBhZ2VzKSB7DQo+ID4gLQkJYnVmbGVuICs9
IHJxc3QtPnJxX3BhZ2VzeiAqIChycXN0LT5ycV9ucGFnZXMgLSAxKTsNCj4gPiAtCQlidWZsZW4g
Kz0gcnFzdC0+cnFfdGFpbHN6Ow0KPiA+ICsJCWlmIChycXN0LT5ycV9ucGFnZXMgPT0gMSkNCj4g
PiArCQkJYnVmbGVuICs9IHJxc3QtPnJxX3RhaWxzejsNCj4gPiArCQllbHNlIHsNCj4gPiArCQkJ
LyoNCj4gPiArCQkJICogSWYgdGhlcmUgaXMgbW9yZSB0aGFuIG9uZSBwYWdlLCBjYWxjdWxhdGUg
dGhlDQo+ID4gKwkJCSAqIGJ1ZmZlciBsZW5ndGggYmFzZWQgb24gcnFfb2Zmc2V0IGFuZCBycV90
YWlsc3oNCj4gPiArCQkJICovDQo+ID4gKwkJCWJ1ZmxlbiArPSBycXN0LT5ycV9wYWdlc3ogKiAo
cnFzdC0+cnFfbnBhZ2VzIC0gMSkgLQ0KPiA+ICsJCQkJCXJxc3QtPnJxX29mZnNldDsNCj4gPiAr
CQkJYnVmbGVuICs9IHJxc3QtPnJxX3RhaWxzejsNCj4gPiArCQl9DQo+IA0KPiBXb3VsZG4ndCBp
dCBiZSBzaW1wbGVyIHRvIGtlZXAgdGhlIG9yaWdpbmFsIGNvZGUsIGJ1dCB0aGVuIGp1c3Qgc3Vi
dHJhY3QgdGhlDQo+IHJxX29mZnNldD8NCj4gDQo+IAlidWZsZW4gKz0gcnFzdC0+cnFfcGFnZXN6
ICogKHJxc3QtPnJxX25wYWdlcyAtIDEpOw0KPiAJYnVmbGVuICs9IHJxc3QtPnJxX3RhaWxzejsN
Cj4gCWJ1ZmxlbiAtPSBycXN0LT5ycV9vZmZzZXQ7DQo+IA0KPiBJdCdzIGtpbmQgb2YgY29uZnVz
aW5nIGFzIHdyaXR0ZW4uIEFsc28sIHdoYXQgaWYgaXQncyBqdXN0IG9uZSBwYWdlLCBidXQgaGFz
IGENCj4gbm9uLXplcm8gb2Zmc2V0PyBJcyB0aGF0IHNvbWVob3cgbm90IHBvc3NpYmxlPyBNeSBz
dWdnZXN0ZWQgY29kZSB3b3VsZA0KPiB0YWtlIHRoYXQgaW50byBhY2NvdW50LCB5b3VycyBkb2Vz
bid0Lg0KDQpJIHRoaW5rIEl0IGNhbiBiZSBjaGFuZ2VkIHRvIHRoaXM6DQoNCglidWZsZW4gKz0g
cnFzdC0+cnFfcGFnZXN6ICogKHJxc3QtPnJxX25wYWdlcyAtIDEpOw0KIAlidWZsZW4gKz0gcnFz
dC0+cnFfdGFpbHN6Ow0KCWlmIChycXN0LT5ycV9ucGFnZXMgPiAxKQ0KIAkJYnVmbGVuIC09IHJx
c3QtPnJxX29mZnNldDsNCg0KVGhpcyBsb29rcyBjbGVhbmVyIHRoYW4gYmVmb3JlLg0KDQpXaGVu
IHRoZXJlIGlzIG9ubHkgb25lIHBhZ2Ugd2l0aCBhIG5vbi16ZXJvIG9mZnNldCwgcnFfdGFpbHN6
IGlzIHRoZSBhY3R1YWwgZGF0YSBsZW5ndGguDQoNCj4gDQo+IFRvbS4NCj4gDQo+ID4gICAJfQ0K
PiA+DQo+ID4gICAJcmV0dXJuIGJ1ZmxlbjsNCj4gPg0K
--
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

Patch

diff --git a/fs/cifs/transport.c b/fs/cifs/transport.c
index 927226a..d6b5523 100644
--- a/fs/cifs/transport.c
+++ b/fs/cifs/transport.c
@@ -212,10 +212,24 @@  rqst_len(struct smb_rqst *rqst)
 	for (i = 0; i < rqst->rq_nvec; i++)
 		buflen += iov[i].iov_len;
 
-	/* add in the page array if there is one */
+	/*
+	 * Add in the page array if there is one. The caller needs to make
+	 * sure rq_offset and rq_tailsz are set correctly. If a buffer of
+	 * multiple pages ends at page boundary, rq_tailsz needs to be set to
+	 * PAGE_SIZE.
+	 */
 	if (rqst->rq_npages) {
-		buflen += rqst->rq_pagesz * (rqst->rq_npages - 1);
-		buflen += rqst->rq_tailsz;
+		if (rqst->rq_npages == 1)
+			buflen += rqst->rq_tailsz;
+		else {
+			/*
+			 * If there is more than one page, calculate the
+			 * buffer length based on rq_offset and rq_tailsz
+			 */
+			buflen += rqst->rq_pagesz * (rqst->rq_npages - 1) -
+					rqst->rq_offset;
+			buflen += rqst->rq_tailsz;
+		}
 	}
 
 	return buflen;