diff mbox

[v2,06/15] CIFS: Introduce helper function to get page offset and length in smb_rqst

Message ID 20180530194807.31657-7-longli@linuxonhyperv.com (mailing list archive)
State New, archived
Headers show

Commit Message

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

Introduce a function rqst_page_get_length to return the page offset and
length for a given page in smb_rqst. This function is to be used by
following patches.

Signed-off-by: Long Li <longli@microsoft.com>
---
 fs/cifs/cifsproto.h |  3 +++
 fs/cifs/misc.c      | 17 +++++++++++++++++
 2 files changed, 20 insertions(+)

Comments

Tom Talpey June 24, 2018, 2:09 a.m. UTC | #1 
On 5/30/2018 3:47 PM, Long Li wrote:
> From: Long Li <longli@microsoft.com>
> 
> Introduce a function rqst_page_get_length to return the page offset and
> length for a given page in smb_rqst. This function is to be used by
> following patches.
> 
> Signed-off-by: Long Li <longli@microsoft.com>
> ---
>   fs/cifs/cifsproto.h |  3 +++
>   fs/cifs/misc.c      | 17 +++++++++++++++++
>   2 files changed, 20 insertions(+)
> 
> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
> index 7933c5f..89dda14 100644
> --- a/fs/cifs/cifsproto.h
> +++ b/fs/cifs/cifsproto.h
> @@ -557,4 +557,7 @@ int cifs_alloc_hash(const char *name, struct crypto_shash **shash,
>   		    struct sdesc **sdesc);
>   void cifs_free_hash(struct crypto_shash **shash, struct sdesc **sdesc);
>   
> +extern void rqst_page_get_length(struct smb_rqst *rqst, unsigned int page,
> +				unsigned int *len, unsigned int *offset);
> +
>   #endif			/* _CIFSPROTO_H */
> diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
> index 96849b5..e951417 100644
> --- a/fs/cifs/misc.c
> +++ b/fs/cifs/misc.c
> @@ -905,3 +905,20 @@ cifs_free_hash(struct crypto_shash **shash, struct sdesc **sdesc)
>   		crypto_free_shash(*shash);
>   	*shash = NULL;
>   }
> +
> +/**
> + * rqst_page_get_length - obtain the length and offset for a page in smb_rqst
> + * Input: rqst - a smb_rqst, page - a page index for rqst
> + * Output: *len - the length for this page, *offset - the offset for this page
> + */
> +void rqst_page_get_length(struct smb_rqst *rqst, unsigned int page,
> +				unsigned int *len, unsigned int *offset)
> +{
> +	*len = rqst->rq_pagesz;
> +	*offset = (page == 0) ? rqst->rq_offset : 0;

Really? Page 0 always has a zero offset??

> +
> +	if (rqst->rq_npages == 1 || page == rqst->rq_npages-1)
> +		*len = rqst->rq_tailsz;
> +	else if (page == 0)
> +		*len = rqst->rq_pagesz - rqst->rq_offset;
> +}
> 

This subroutine does what patch 5 does inline. Why not push this patch
up in the sequence and use the helper?

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 25, 2018, 9:14 p.m. UTC | #2 
PiBTdWJqZWN0OiBSZTogW1BhdGNoIHYyIDA2LzE1XSBDSUZTOiBJbnRyb2R1Y2UgaGVscGVyIGZ1
bmN0aW9uIHRvIGdldCBwYWdlDQo+IG9mZnNldCBhbmQgbGVuZ3RoIGluIHNtYl9ycXN0DQo+IA0K
PiBPbiA1LzMwLzIwMTggMzo0NyBQTSwgTG9uZyBMaSB3cm90ZToNCj4gPiBGcm9tOiBMb25nIExp
IDxsb25nbGlAbWljcm9zb2Z0LmNvbT4NCj4gPg0KPiA+IEludHJvZHVjZSBhIGZ1bmN0aW9uIHJx
c3RfcGFnZV9nZXRfbGVuZ3RoIHRvIHJldHVybiB0aGUgcGFnZSBvZmZzZXQNCj4gPiBhbmQgbGVu
Z3RoIGZvciBhIGdpdmVuIHBhZ2UgaW4gc21iX3Jxc3QuIFRoaXMgZnVuY3Rpb24gaXMgdG8gYmUg
dXNlZA0KPiA+IGJ5IGZvbGxvd2luZyBwYXRjaGVzLg0KPiA+DQo+ID4gU2lnbmVkLW9mZi1ieTog
TG9uZyBMaSA8bG9uZ2xpQG1pY3Jvc29mdC5jb20+DQo+ID4gLS0tDQo+ID4gICBmcy9jaWZzL2Np
ZnNwcm90by5oIHwgIDMgKysrDQo+ID4gICBmcy9jaWZzL21pc2MuYyAgICAgIHwgMTcgKysrKysr
KysrKysrKysrKysNCj4gPiAgIDIgZmlsZXMgY2hhbmdlZCwgMjAgaW5zZXJ0aW9ucygrKQ0KPiA+
DQo+ID4gZGlmZiAtLWdpdCBhL2ZzL2NpZnMvY2lmc3Byb3RvLmggYi9mcy9jaWZzL2NpZnNwcm90
by5oIGluZGV4DQo+ID4gNzkzM2M1Zi4uODlkZGExNCAxMDA2NDQNCj4gPiAtLS0gYS9mcy9jaWZz
L2NpZnNwcm90by5oDQo+ID4gKysrIGIvZnMvY2lmcy9jaWZzcHJvdG8uaA0KPiA+IEBAIC01NTcs
NCArNTU3LDcgQEAgaW50IGNpZnNfYWxsb2NfaGFzaChjb25zdCBjaGFyICpuYW1lLCBzdHJ1Y3QN
Cj4gY3J5cHRvX3NoYXNoICoqc2hhc2gsDQo+ID4gICAJCSAgICBzdHJ1Y3Qgc2Rlc2MgKipzZGVz
Yyk7DQo+ID4gICB2b2lkIGNpZnNfZnJlZV9oYXNoKHN0cnVjdCBjcnlwdG9fc2hhc2ggKipzaGFz
aCwgc3RydWN0IHNkZXNjDQo+ID4gKipzZGVzYyk7DQo+ID4NCj4gPiArZXh0ZXJuIHZvaWQgcnFz
dF9wYWdlX2dldF9sZW5ndGgoc3RydWN0IHNtYl9ycXN0ICpycXN0LCB1bnNpZ25lZCBpbnQNCj4g
cGFnZSwNCj4gPiArCQkJCXVuc2lnbmVkIGludCAqbGVuLCB1bnNpZ25lZCBpbnQgKm9mZnNldCk7
DQo+ID4gKw0KPiA+ICAgI2VuZGlmCQkJLyogX0NJRlNQUk9UT19IICovDQo+ID4gZGlmZiAtLWdp
dCBhL2ZzL2NpZnMvbWlzYy5jIGIvZnMvY2lmcy9taXNjLmMgaW5kZXggOTY4NDliNS4uZTk1MTQx
Nw0KPiA+IDEwMDY0NA0KPiA+IC0tLSBhL2ZzL2NpZnMvbWlzYy5jDQo+ID4gKysrIGIvZnMvY2lm
cy9taXNjLmMNCj4gPiBAQCAtOTA1LDMgKzkwNSwyMCBAQCBjaWZzX2ZyZWVfaGFzaChzdHJ1Y3Qg
Y3J5cHRvX3NoYXNoICoqc2hhc2gsDQo+IHN0cnVjdCBzZGVzYyAqKnNkZXNjKQ0KPiA+ICAgCQlj
cnlwdG9fZnJlZV9zaGFzaCgqc2hhc2gpOw0KPiA+ICAgCSpzaGFzaCA9IE5VTEw7DQo+ID4gICB9
DQo+ID4gKw0KPiA+ICsvKioNCj4gPiArICogcnFzdF9wYWdlX2dldF9sZW5ndGggLSBvYnRhaW4g
dGhlIGxlbmd0aCBhbmQgb2Zmc2V0IGZvciBhIHBhZ2UgaW4NCj4gPiArc21iX3Jxc3QNCj4gPiAr
ICogSW5wdXQ6IHJxc3QgLSBhIHNtYl9ycXN0LCBwYWdlIC0gYSBwYWdlIGluZGV4IGZvciBycXN0
DQo+ID4gKyAqIE91dHB1dDogKmxlbiAtIHRoZSBsZW5ndGggZm9yIHRoaXMgcGFnZSwgKm9mZnNl
dCAtIHRoZSBvZmZzZXQgZm9yDQo+ID4gK3RoaXMgcGFnZSAgKi8gdm9pZCBycXN0X3BhZ2VfZ2V0
X2xlbmd0aChzdHJ1Y3Qgc21iX3Jxc3QgKnJxc3QsDQo+ID4gK3Vuc2lnbmVkIGludCBwYWdlLA0K
PiA+ICsJCQkJdW5zaWduZWQgaW50ICpsZW4sIHVuc2lnbmVkIGludCAqb2Zmc2V0KSB7DQo+ID4g
KwkqbGVuID0gcnFzdC0+cnFfcGFnZXN6Ow0KPiA+ICsJKm9mZnNldCA9IChwYWdlID09IDApID8g
cnFzdC0+cnFfb2Zmc2V0IDogMDsNCj4gDQo+IFJlYWxseT8gUGFnZSAwIGFsd2F5cyBoYXMgYSB6
ZXJvIG9mZnNldD8/DQoNCkkgdGhpbmsgeW91IGFyZSBtaXNyZWFkaW5nIHRoaXMgbGluZS4gVGhl
IG9mZnNldCBmb3IgcGFnZSAwIGlzIHJxX29mZnNldCwgdGhlIG9mZnNldCBmb3IgYWxsIHN1YnNl
cXVlbnQgcGFnZXMgYXJlIDAuDQoNCj4gDQo+ID4gKw0KPiA+ICsJaWYgKHJxc3QtPnJxX25wYWdl
cyA9PSAxIHx8IHBhZ2UgPT0gcnFzdC0+cnFfbnBhZ2VzLTEpDQo+ID4gKwkJKmxlbiA9IHJxc3Qt
PnJxX3RhaWxzejsNCj4gPiArCWVsc2UgaWYgKHBhZ2UgPT0gMCkNCj4gPiArCQkqbGVuID0gcnFz
dC0+cnFfcGFnZXN6IC0gcnFzdC0+cnFfb2Zmc2V0OyB9DQo+ID4NCj4gDQo+IFRoaXMgc3Vicm91
dGluZSBkb2VzIHdoYXQgcGF0Y2ggNSBkb2VzIGlubGluZS4gV2h5IG5vdCBwdXNoIHRoaXMgcGF0
Y2ggdXAgaW4NCj4gdGhlIHNlcXVlbmNlIGFuZCB1c2UgdGhlIGhlbHBlcj8NCg0KVGhpcyBpcyBh
Y3R1YWxseSBhIGxpdHRsZSBkaWZmZXJlbnQuIFRoaXMgZnVuY3Rpb24gcmV0dXJucyB0aGUgbGVu
Z3RoIGFuZCBvZmZzZXQgZm9yIGEgZ2l2ZW4gcGFnZSBpbiB0aGUgcmVxdWVzdC4gVGhlcmUgbWln
aHQgYmUgbXVsdGlwbGUgcGFnZXMgaW4gYSByZXF1ZXN0LiANCg0KVGhlIG90aGVyIGZ1bmN0aW9u
IGNhbGN1bGF0ZXMgdGhlIHRvdGFsIGxlbmd0aCBvZiBhbGwgdGhlIHBhZ2VzIGluIGEgcmVxdWVz
dC4NCg0KPiANCj4gVG9tLg0K
--
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, 1:16 p.m. UTC | #3 
On 6/25/2018 5:14 PM, Long Li wrote:
>> Subject: Re: [Patch v2 06/15] CIFS: Introduce helper function to get page
>> offset and length in smb_rqst
>>
>> On 5/30/2018 3:47 PM, Long Li wrote:
>>> From: Long Li <longli@microsoft.com>
>>>
>>> Introduce a function rqst_page_get_length to return the page offset
>>> and length for a given page in smb_rqst. This function is to be used
>>> by following patches.
>>>
>>> Signed-off-by: Long Li <longli@microsoft.com>
>>> ---
>>>    fs/cifs/cifsproto.h |  3 +++
>>>    fs/cifs/misc.c      | 17 +++++++++++++++++
>>>    2 files changed, 20 insertions(+)
>>>
>>> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h index
>>> 7933c5f..89dda14 100644
>>> --- a/fs/cifs/cifsproto.h
>>> +++ b/fs/cifs/cifsproto.h
>>> @@ -557,4 +557,7 @@ int cifs_alloc_hash(const char *name, struct
>> crypto_shash **shash,
>>>    		    struct sdesc **sdesc);
>>>    void cifs_free_hash(struct crypto_shash **shash, struct sdesc
>>> **sdesc);
>>>
>>> +extern void rqst_page_get_length(struct smb_rqst *rqst, unsigned int
>> page,
>>> +				unsigned int *len, unsigned int *offset);
>>> +
>>>    #endif			/* _CIFSPROTO_H */
>>> diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c index 96849b5..e951417
>>> 100644
>>> --- a/fs/cifs/misc.c
>>> +++ b/fs/cifs/misc.c
>>> @@ -905,3 +905,20 @@ cifs_free_hash(struct crypto_shash **shash,
>> struct sdesc **sdesc)
>>>    		crypto_free_shash(*shash);
>>>    	*shash = NULL;
>>>    }
>>> +
>>> +/**
>>> + * rqst_page_get_length - obtain the length and offset for a page in
>>> +smb_rqst
>>> + * Input: rqst - a smb_rqst, page - a page index for rqst
>>> + * Output: *len - the length for this page, *offset - the offset for
>>> +this page  */ void rqst_page_get_length(struct smb_rqst *rqst,
>>> +unsigned int page,
>>> +				unsigned int *len, unsigned int *offset) {
>>> +	*len = rqst->rq_pagesz;
>>> +	*offset = (page == 0) ? rqst->rq_offset : 0;
>>
>> Really? Page 0 always has a zero offset??
> 
> I think you are misreading this line. The offset for page 0 is rq_offset, the offset for all subsequent pages are 0.

Ah, yes, sorry I did read it incorrectly.
>>> +
>>> +	if (rqst->rq_npages == 1 || page == rqst->rq_npages-1)
>>> +		*len = rqst->rq_tailsz;
>>> +	else if (page == 0)
>>> +		*len = rqst->rq_pagesz - rqst->rq_offset; }
>>>
>>
>> This subroutine does what patch 5 does inline. Why not push this patch up in
>> the sequence and use the helper?
> 
> This is actually a little different. This function returns the length and offset for a given page in the request. There might be multiple pages in a request.

Ok, but I still think there is quite a bit of inline computation of
this stuff that would more clearly and more robustly be done in a set
of common functions. If someone ever touches the code to support a
new upper layer, or even integrate with more complex compounding,
things will get ugly fast.

> The other function calculates the total length of all the pages in a request.

Again, a great candidate for a common set of subroutines, IMO.

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:24 a.m. UTC | #4 
> Subject: Re: [Patch v2 06/15] CIFS: Introduce helper function to get page

> offset and length in smb_rqst

> 

> On 6/25/2018 5:14 PM, Long Li wrote:

> >> Subject: Re: [Patch v2 06/15] CIFS: Introduce helper function to get

> >> page offset and length in smb_rqst

> >>

> >> On 5/30/2018 3:47 PM, Long Li wrote:

> >>> From: Long Li <longli@microsoft.com>

> >>>

> >>> Introduce a function rqst_page_get_length to return the page offset

> >>> and length for a given page in smb_rqst. This function is to be used

> >>> by following patches.

> >>>

> >>> Signed-off-by: Long Li <longli@microsoft.com>

> >>> ---

> >>>    fs/cifs/cifsproto.h |  3 +++

> >>>    fs/cifs/misc.c      | 17 +++++++++++++++++

> >>>    2 files changed, 20 insertions(+)

> >>>

> >>> diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h index

> >>> 7933c5f..89dda14 100644

> >>> --- a/fs/cifs/cifsproto.h

> >>> +++ b/fs/cifs/cifsproto.h

> >>> @@ -557,4 +557,7 @@ int cifs_alloc_hash(const char *name, struct

> >> crypto_shash **shash,

> >>>    		    struct sdesc **sdesc);

> >>>    void cifs_free_hash(struct crypto_shash **shash, struct sdesc

> >>> **sdesc);

> >>>

> >>> +extern void rqst_page_get_length(struct smb_rqst *rqst, unsigned

> >>> +int

> >> page,

> >>> +				unsigned int *len, unsigned int *offset);

> >>> +

> >>>    #endif			/* _CIFSPROTO_H */

> >>> diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c index 96849b5..e951417

> >>> 100644

> >>> --- a/fs/cifs/misc.c

> >>> +++ b/fs/cifs/misc.c

> >>> @@ -905,3 +905,20 @@ cifs_free_hash(struct crypto_shash **shash,

> >> struct sdesc **sdesc)

> >>>    		crypto_free_shash(*shash);

> >>>    	*shash = NULL;

> >>>    }

> >>> +

> >>> +/**

> >>> + * rqst_page_get_length - obtain the length and offset for a page

> >>> +in smb_rqst

> >>> + * Input: rqst - a smb_rqst, page - a page index for rqst

> >>> + * Output: *len - the length for this page, *offset - the offset

> >>> +for this page  */ void rqst_page_get_length(struct smb_rqst *rqst,

> >>> +unsigned int page,

> >>> +				unsigned int *len, unsigned int *offset) {

> >>> +	*len = rqst->rq_pagesz;

> >>> +	*offset = (page == 0) ? rqst->rq_offset : 0;

> >>

> >> Really? Page 0 always has a zero offset??

> >

> > I think you are misreading this line. The offset for page 0 is rq_offset, the

> offset for all subsequent pages are 0.

> 

> Ah, yes, sorry I did read it incorrectly.

> >>> +

> >>> +	if (rqst->rq_npages == 1 || page == rqst->rq_npages-1)

> >>> +		*len = rqst->rq_tailsz;

> >>> +	else if (page == 0)

> >>> +		*len = rqst->rq_pagesz - rqst->rq_offset; }

> >>>

> >>

> >> This subroutine does what patch 5 does inline. Why not push this

> >> patch up in the sequence and use the helper?

> >

> > This is actually a little different. This function returns the length and offset

> for a given page in the request. There might be multiple pages in a request.

> 

> Ok, but I still think there is quite a bit of inline computation of this stuff that

> would more clearly and more robustly be done in a set of common functions.

> If someone ever touches the code to support a new upper layer, or even

> integrate with more complex compounding, things will get ugly fast.

> 

> > The other function calculates the total length of all the pages in a request.

> 

> Again, a great candidate for a common set of subroutines, IMO.


I will look into this. The reason I didn't make a common function is that this is the only patch that will use it in this way.
diff mbox

Patch

diff --git a/fs/cifs/cifsproto.h b/fs/cifs/cifsproto.h
index 7933c5f..89dda14 100644
--- a/fs/cifs/cifsproto.h
+++ b/fs/cifs/cifsproto.h
@@ -557,4 +557,7 @@  int cifs_alloc_hash(const char *name, struct crypto_shash **shash,
 		    struct sdesc **sdesc);
 void cifs_free_hash(struct crypto_shash **shash, struct sdesc **sdesc);
 
+extern void rqst_page_get_length(struct smb_rqst *rqst, unsigned int page,
+				unsigned int *len, unsigned int *offset);
+
 #endif			/* _CIFSPROTO_H */
diff --git a/fs/cifs/misc.c b/fs/cifs/misc.c
index 96849b5..e951417 100644
--- a/fs/cifs/misc.c
+++ b/fs/cifs/misc.c
@@ -905,3 +905,20 @@  cifs_free_hash(struct crypto_shash **shash, struct sdesc **sdesc)
 		crypto_free_shash(*shash);
 	*shash = NULL;
 }
+
+/**
+ * rqst_page_get_length - obtain the length and offset for a page in smb_rqst
+ * Input: rqst - a smb_rqst, page - a page index for rqst
+ * Output: *len - the length for this page, *offset - the offset for this page
+ */
+void rqst_page_get_length(struct smb_rqst *rqst, unsigned int page,
+				unsigned int *len, unsigned int *offset)
+{
+	*len = rqst->rq_pagesz;
+	*offset = (page == 0) ? rqst->rq_offset : 0;
+
+	if (rqst->rq_npages == 1 || page == rqst->rq_npages-1)
+		*len = rqst->rq_tailsz;
+	else if (page == 0)
+		*len = rqst->rq_pagesz - rqst->rq_offset;
+}