fix: return the read sectors length when it reads the expansion part

[ChanYiLin]

ref: longhorn/longhorn#6899

I think we should still return len(read sectors) when reading out of the boundary (expansion part of the specific image)
The reason is that, from the caller's perspective, it is reading from the "Volume"

func (r *Replica) ReadAt(buf []byte, offset int64) (int, error) {
	r.RLock()
	c, err := r.volume.ReadAt(buf, offset) // which will call diffDisk.fullReadAt()
	r.RUnlock()
	return c, err
}

So it has no knowledge about the snapshot chain underneath.

If it reads outside of the boundary of the "Volume", we do return io.ErrUnexpectedEOF error here
But if it only reads outside of the boundary of "specific snapshot image", and we allow it to do so, then we should consider it as reading a bunch of "0", and that is also true. The caller does get a bunch of "0".
The caller should not take care of the situation.
It might get confused that it doesn't get EOF error but how come the data length is not correct.

[ChanYiLin]

Test is now running

-m coretest
https://ci.longhorn.io/job/private/job/longhorn-tests-regression/6007/console

[shuo-wu]

The read count of the below d.files[target].ReadAt(newBuf, offset) can be smaller than bufLength.

For example:

buf         [xxxxxxxx]
newBuf      [xxxx]
disk        [xxxx]

diffDisk.read will return len(newBuf) = 4 rather than len(buf) = 8. Then the upper caller final result won't be the length the application wants to read

[PhanLe1010]

The read count of the below d.files[target].ReadAt(newBuf, offset) can be smaller than bufLength.

For example:

buf         [xxxxxxxx]
newBuf      [xxxx]
disk        [xxxx]

diffDisk.read will return len(newBuf) = 4 rather than len(buf) = 8. Then the upper caller final result won't be the length the application wants to read

Good catch. So maybe we should always return bufLength like?

func (d *diffDisk) read(target byte, buf []byte, startOffset int64, startSector int64, sectors int64) (int, error) {
	bufStart := startSector * d.sectorSize
	bufLength := sectors * d.sectorSize
	offset := startOffset + bufStart
	size, err := d.files[target].Size()
	if err != nil {
		return 0, err
	}

	// Reading the out-of-bound part is not allowed
	if bufLength > d.size-offset {
		logrus.Warn("Trying to read the out-of-bound part")
		return 0, io.ErrUnexpectedEOF
	}

	// May read the expanded part
	if offset >= size {
		return int(bufLength), nil
	}
	var newBuf []byte
	if bufLength > size-offset {
		newBuf = buf[bufStart : bufStart+size-offset]
	} else {
		newBuf = buf[bufStart : bufStart+bufLength]
	}
	count, err := d.files[target].ReadAt(newBuf, offset)
        if err != nil {
               return count, err
        }
        return int(bufLength), nil
}

[ChanYiLin]

I have summarize all the case for read() and rewrite the function

I think the point here is that when reading from the "specific target disk"

• We only return EOF when it reads out or "Volume" bound
• It is allowed to read out of disk bound as long as it is still in the valid range of the Volume, and we should fill the space with "0"
• We should return the count no matter it is "0" we fill in or the data actually read from the disk

Steps:

1. We read the data inside disk boundary => get count and check error
2. If diskReadEndOffset is inside the disk boundary => Return the count with nil error

But if the diskReadEndOffset is outside the disk boundary, then it breaks into 2 cases
- if diskReadEndOffset is inside the volume boundary, then we fill in 0 to to remaining valid space and the count of "0" should be [diskReadEndOffset - max(dSize, diskReadStartOffset)]. Return count with nil error
- If diskReadEndOffset is also out of the volume boundary, then we fill in 0 to remaining valid space and the count of "0" should be [volumeSize - max(dSize, diskReadStartOffset)]. Return count and EOF

Note: the "fill in 0" here, we don't actually do anything, the buf[] is already init to 0
Note: disk is definitely smaller and equal (<=) than volume

Index   [0, 1, 2, 3, 4, 5, 6]
Disk    [_, x, x]
Volume  [_, _, _, _, _]

Read       [x, x]               => count = 2
Read          [x, 0]            => count = 2    `diskReadEndOffset` is inside the volume boundary
Read             [0, 0]         => count = 2    `diskReadEndOffset` is inside the volume boundary
Read                [0, _]      => count = 1, EOF    `diskReadEndOffset` is out of the volume boundary,
Read                   [_, _]   => count = 0, EOF    `diskReadEndOffset` is out of the volume boundary,
Read          [x, 0, 0, _]      => count = 3, EOF    `diskReadEndOffset` is out of the volume boundary,
Read             [0, 0, _]      => count = 2, EOF    `diskReadEndOffset` is out of the volume boundary,

In this case the uppercaller diffDisk.fullReadAt won't need to do any change, the count return from the read() has already consider the boundary issue, and the count should be correct no matter

• disk is smaller than volume
• read is out of disk boundary
• read is out of volume boundary

and even Volume EOF, the count would be correct and the buf[] will still have valid data for valid range.

[ChanYiLin]

Manual Test - PASSED

test with the original harvester test case

• cksum are the same
• filesystem is not corrupted

[PhanLe1010]

We need some unit test for this modification

[shuo-wu]

LGTM

[ChanYiLin]

Hi @PhanLe1010 @derekbit
I have added unit tests, thanks

[derekbit]

LGTM and just one remaining question.

[innobead]

@mergify backport v1.6.x v1.5.x v1.4.x

[mergify]

backport v1.6.x v1.5.x v1.4.x

✅ Backports have been created

• #991 fix: return the read sectors length when it reads the expansion part (backport #986) has been created for branch v1.6.x
• #992 fix: return the read sectors length when it reads the expansion part (backport #986) has been created for branch v1.5.x
• #993 fix: return the read sectors length when it reads the expansion part (backport #986) has been created for branch v1.4.x