respect the msize value for read/write #27

simonferquel · 2016-09-30T13:11:50Z

There was a typo in the msize negociation code, and for large read / writes, the msize value was not taken into account (if the size of the data to write + size of the header message > msize, we must chunk the data)

Fix read/write to take msize into account (and chunking data if necessary) Signed-off-by: Simon Ferquel <[email protected]>

codecov-io · 2016-09-30T13:13:17Z

Current coverage is 15.52% (diff: 13.33%)

Merging #27 into master will decrease coverage by 0.43%

@@             master        #27   diff @@
==========================================
  Files            13         15     +2   
  Lines          1128       1179    +51   
  Methods           0          0          
  Messages          0          0          
  Branches          0          0          
==========================================
+ Hits            180        183     +3   
- Misses          900        948    +48   
  Partials         48         48

Powered by Codecov. Last update 5a23906...9fd7daa

dgageot · 2016-09-30T19:24:05Z

ping @stevvooe :-)

stevvooe

Msize should be handled in the channel, not the client.

stevvooe · 2016-10-03T14:25:21Z

version.go

@@ -38,7 +38,8 @@ func clientnegotiate(ctx context.Context, ch Channel, version string) (string, e
 			return "", fmt.Errorf("unsupported server version: %v", version)
 		}

-		if int(v.MSize) > ch.MSize() {
+		if int(v.MSize) < ch.MSize() {


Why reverse this condition?

The condition was wrong : client and server negociate to know wich has the smallest msize, and agree to stick with that

The client already sent their size. This scales down the local msize if the server responds with a smaller value. The protocol works like this:

Client sends msize.

Server responds with msize.
v.Msize > msize, use server's msize.

At least, this was how I understand from study of 9p. Do you have documentation countering this? What does plan9 do?

Also, please move this to a separate PR.

stevvooe · 2016-10-03T14:26:36Z

client.go

-	if !ok {
-		return 0, ErrUnexpectedMsg
+	// size[4] Rread tag[2] count[4] data[count]
+	const rreadMessageEnvelopeSize int = 4 + 1 + 2 + 4


This calculation should be a part of the message type. Please modify this correctly in codec.

stevvooe · 2016-10-03T14:35:08Z

client.go

+	maxChunkSize := c.msize - rreadMessageEnvelopeSize
+	totalRead := 0
+
+	for {


This breaks the abstraction: there should only be a single message sent per method call.

You can create a helper, similar to io.ReadFull, to handle chunking.

Signed-off-by: Simon Ferquel <[email protected]>

stevvooe · 2016-10-03T16:45:07Z

channel.go

+			fcall.Message = MessageTread{Offset: m.Offset, Fid: m.Fid, Count: m.Count - uint32(overflow)}
+		}
+	}
+
 	p, err := ch.codec.Marshal(fcall)


Should this sizing be represented by the codec? Just test len(p) against the returned buffer. We can add a Size method to codec to avoid alloc.

stevvooe · 2016-10-03T16:45:51Z

session.go

+}
+
+// ReadFull reads the whole file and returns it in a slice
+func ReadFull(s Session, ctx context.Context, fid Fid, offset int64) (p []byte, err error) {


Go back and look at io.ReadFull. It takes a buffer. This is going to cause unnecessary allocs.

After some thought here, it would be much better to implement an io.Reader or io.ReadSeeker that tracks offset and fid. This way, the io utility functions can be used composably. The singature would be something like this:

NewReadSeeker(ctx context.Context, s Session, fid Fid) (io.ReadSeeker, error)

This should open up correct buffering.

stevvooe · 2016-10-03T16:46:20Z

session.go

+
+// ReadFull reads the whole file and returns it in a slice
+func ReadFull(s Session, ctx context.Context, fid Fid, offset int64) (p []byte, err error) {
+	var buf [4096]byte


Right here is the problem: this fixes the buffer size and removes control from the caller.

stevvooe · 2016-10-03T16:46:38Z

session.go

@@ -31,3 +31,30 @@ type Session interface {
 	// session implementation.
 	Version() (msize int, version string)
 }
+
+// WriteFull writes the whole p slice to the specified fid
+func WriteFull(s Session, ctx context.Context, fid Fid, p []byte, offset int64) error {


Please order the context argument correctly.

stevvooe · 2016-10-03T16:47:51Z

session.go

@@ -31,3 +31,30 @@ type Session interface {
 	// session implementation.
 	Version() (msize int, version string)
 }
+
+// WriteFull writes the whole p slice to the specified fid
+func WriteFull(s Session, ctx context.Context, fid Fid, p []byte, offset int64) error {


Place these in a separate file. They are just methods to use with session. See readdir.go for an example.

stevvooe · 2016-10-03T16:54:46Z

version.go

@@ -38,7 +38,8 @@ func clientnegotiate(ctx context.Context, ch Channel, version string) (string, e
 			return "", fmt.Errorf("unsupported server version: %v", version)
 		}

-		if int(v.MSize) > ch.MSize() {
+		if int(v.MSize) < ch.MSize() {


The client already sent their size. This scales down the local msize if the server responds with a smaller value. The protocol works like this:

Client sends msize.

Server responds with msize.
v.Msize > msize, use server's msize.

At least, this was how I understand from study of 9p. Do you have documentation countering this? What does plan9 do?

stevvooe · 2016-10-03T17:41:17Z

session.go

+}
+
+// ReadFull reads the whole file and returns it in a slice
+func ReadFull(s Session, ctx context.Context, fid Fid, offset int64) (p []byte, err error) {


After some thought here, it would be much better to implement an io.Reader or io.ReadSeeker that tracks offset and fid. This way, the io utility functions can be used composably. The singature would be something like this:

NewReadSeeker(ctx context.Context, s Session, fid Fid) (io.ReadSeeker, error)

This should open up correct buffering.

Signed-off-by: Simon Ferquel <[email protected]>

…ayer + expose / read write operations as standard io.Reader|Writer interfaces Signed-off-by: Simon Ferquel <[email protected]>

Signed-off-by: Simon Ferquel <[email protected]>

simonferquel · 2016-10-04T13:40:24Z

Should be ready to merge. I have successfully tested it in the context of Docker for Windows / Mac.
Some structural changes have been made :

Codec has now knowledge of the max size it can use to marshall messages
This has required some plumbing for tests and 9pr to build and run
High level clients should prefer to rely on NewFileReader / NewFileWriter that implement io.Reader / io.Writer behaviors to access data streams

This also have a change in the package signature (newly exposed NewFileReader / Writer functions and newly exposed Codec from the session - so that 9pr can be reliable)

simonferquel · 2016-10-06T13:55:32Z

This way of dealing with the problem is too much invasive prefer #29

Fix client side msize negociation

2693498

Fix read/write to take msize into account (and chunking data if necessary) Signed-off-by: Simon Ferquel <[email protected]>

GordonTheTurtle added the status/0-triage label Sep 30, 2016

stevvooe suggested changes Oct 3, 2016

View reviewed changes

stevvooe reviewed Oct 3, 2016

View reviewed changes

simonferquel changed the title ~~Fix msize negociation and respect the msize value for read/write~~ [DO NOT MERGE / WIP] Fix msize negociation and respect the msize value for read/write Oct 3, 2016

Code refactoring according to review

7fc7fd7

Signed-off-by: Simon Ferquel <[email protected]>

stevvooe reviewed Oct 3, 2016

View reviewed changes

stevvooe suggested changes Oct 3, 2016

View reviewed changes

simonferquel added 2 commits October 4, 2016 10:24

Reverted change for Splitting PR

d653a22

Signed-off-by: Simon Ferquel <[email protected]>

Move the message truncation / limit to partial read to the encoding l…

5804889

…ayer + expose / read write operations as standard io.Reader|Writer interfaces Signed-off-by: Simon Ferquel <[email protected]>

simonferquel changed the title ~~[DO NOT MERGE / WIP] Fix msize negociation and respect the msize value for read/write~~ respect the msize value for read/write Oct 4, 2016

simonferquel added 3 commits October 4, 2016 15:11

Fix auto imports

cb59176

Signed-off-by: Simon Ferquel <[email protected]>

Fix tests

b6be954

Signed-off-by: Simon Ferquel <[email protected]>

Fix test/build

9fd7daa

Signed-off-by: Simon Ferquel <[email protected]>

simonferquel mentioned this pull request Oct 4, 2016

Go-datakit handle large values when reading / writing in 9db moby/datakit#292

Merged

simonferquel closed this Oct 6, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

respect the msize value for read/write #27

respect the msize value for read/write #27

simonferquel commented Sep 30, 2016

codecov-io commented Sep 30, 2016 •

edited

Loading

dgageot commented Sep 30, 2016

stevvooe left a comment

stevvooe Oct 3, 2016

simonferquel Oct 3, 2016

stevvooe Oct 3, 2016

stevvooe Oct 3, 2016

stevvooe Oct 3, 2016

stevvooe Oct 3, 2016

stevvooe Oct 3, 2016

stevvooe Oct 3, 2016

stevvooe Oct 3, 2016

stevvooe Oct 3, 2016

stevvooe Oct 3, 2016

stevvooe Oct 3, 2016

stevvooe Oct 3, 2016

stevvooe Oct 3, 2016

simonferquel commented Oct 4, 2016

simonferquel commented Oct 6, 2016

respect the msize value for read/write #27

respect the msize value for read/write #27

Conversation

simonferquel commented Sep 30, 2016

codecov-io commented Sep 30, 2016 • edited Loading

Current coverage is 15.52% (diff: 13.33%)

dgageot commented Sep 30, 2016

stevvooe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

simonferquel commented Oct 4, 2016

simonferquel commented Oct 6, 2016

codecov-io commented Sep 30, 2016 •

edited

Loading