-
Notifications
You must be signed in to change notification settings - Fork 49
sn/object: Optimize RANGE handling #3967
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -25,11 +25,12 @@ type Storage interface { | |
| // binary format. Returns [apistatus.ObjectNotFound] if object is missing. | ||
| GetBytes(oid.Address) ([]byte, error) | ||
| Get(oid.Address) (*object.Object, error) | ||
| GetRangeStream(addr oid.Address, off uint64, ln uint64) (io.ReadCloser, error) | ||
| GetRangeStream(addr oid.Address, off uint64, ln uint64) (uint64, io.ReadCloser, error) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Technically read-closer should be sufficient. Also, we know payload length from the metabase normally. But I can live with this for now. |
||
| GetStream(oid.Address) (*object.Object, io.ReadCloser, error) | ||
| Head(oid.Address) (*object.Object, error) | ||
| ReadHeader(oid.Address, []byte) (int, error) | ||
| ReadObject(oid.Address, []byte) (int, io.ReadCloser, error) | ||
| ReadPayloadRange(oid.Address, uint64, uint64, []byte) (io.ReadCloser, error) | ||
| Exists(oid.Address) (bool, error) | ||
| Put(oid.Address, []byte) error | ||
| PutBatch(map[oid.Address][]byte) error | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -14,6 +14,8 @@ import ( | |||||||||||
| "strings" | ||||||||||||
| "time" | ||||||||||||
|
|
||||||||||||
| objectwire "github.com/nspcc-dev/neofs-node/internal/object" | ||||||||||||
| iprotobuf "github.com/nspcc-dev/neofs-node/internal/protobuf" | ||||||||||||
| "github.com/nspcc-dev/neofs-node/pkg/local_object_storage/blobstor/common" | ||||||||||||
| "github.com/nspcc-dev/neofs-node/pkg/local_object_storage/blobstor/compression" | ||||||||||||
| "github.com/nspcc-dev/neofs-node/pkg/local_object_storage/util/logicerr" | ||||||||||||
|
|
@@ -530,46 +532,208 @@ func (t *FSTree) GetStream(addr oid.Address) (*object.Object, io.ReadCloser, err | |||||||||||
| // | ||||||||||||
| // If the range is out of payload bounds, GetRangeStream returns | ||||||||||||
| // [apistatus.ErrObjectOutOfRange]. | ||||||||||||
| func (t *FSTree) GetRangeStream(addr oid.Address, off uint64, ln uint64) (io.ReadCloser, error) { | ||||||||||||
| if ln == 0 && off != 0 { | ||||||||||||
| return nil, fmt.Errorf("invalid range off=%d,ln=0", off) | ||||||||||||
| func (t *FSTree) GetRangeStream(addr oid.Address, off uint64, ln uint64) (uint64, io.ReadCloser, error) { | ||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How about allocating a buffer and calling |
||||||||||||
| if err := verifyRequestedRange(off, ln); err != nil { | ||||||||||||
| return 0, nil, err | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| // TODO: we need only one header field. Consider decoding only it + jumping to payload | ||||||||||||
| hdr, stream, err := t.getObjectStream(addr) | ||||||||||||
| if err != nil { | ||||||||||||
| return nil, err | ||||||||||||
| return 0, nil, err | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| pldLen := hdr.PayloadSize() | ||||||||||||
|
|
||||||||||||
| if ln == 0 && off == 0 { | ||||||||||||
| return stream, nil | ||||||||||||
| return pldLen, stream, nil | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| if off >= pldLen || pldLen-off < ln { | ||||||||||||
| if !checkPayloadBounds(pldLen, off, ln) { | ||||||||||||
| stream.Close() | ||||||||||||
| return nil, apistatus.ErrObjectOutOfRange | ||||||||||||
| return 0, nil, apistatus.ErrObjectOutOfRange | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| if off > math.MaxInt64 || ln > math.MaxInt64 { // 8 exabytes, amply | ||||||||||||
| if err := checkTooBigRange(off, ln); err != nil { | ||||||||||||
| stream.Close() | ||||||||||||
| return nil, fmt.Errorf("range overflowing int64 is not supported by this server: off=%d,len=%d", off, ln) | ||||||||||||
| return 0, nil, err | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| if off > 0 { | ||||||||||||
| if _, err := stream.Seek(int64(off), io.SeekStart); err != nil { | ||||||||||||
| stream.Close() | ||||||||||||
| return nil, fmt.Errorf("seek offset in payload stream: %w", err) | ||||||||||||
| return 0, nil, fmt.Errorf("seek offset in payload stream: %w", err) | ||||||||||||
| } | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| return readerCloser{ | ||||||||||||
| return pldLen, readerCloser{ | ||||||||||||
| Reader: io.LimitReader(stream, int64(ln)), | ||||||||||||
| Closer: stream, | ||||||||||||
| }, nil | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| // ReadPayloadRange is [FSTree.ReadObject] analogue for payload range reading. | ||||||||||||
| // Zero range means full payload. Returns full payload range length. | ||||||||||||
| // | ||||||||||||
| // If given range is out of payload bounds, ReadPayloadRange returns | ||||||||||||
| // [apistatus.ErrObjectOutOfRange]. | ||||||||||||
| func (t *FSTree) ReadPayloadRange(addr oid.Address, off, ln uint64, hdrBuf []byte) (io.ReadCloser, error) { | ||||||||||||
|
Comment on lines
+575
to
+580
|
||||||||||||
| if err := verifyRequestedRange(off, ln); err != nil { | ||||||||||||
| return nil, err | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| initial, stream, err := t._readObject(addr, hdrBuf) | ||||||||||||
| if err != nil { | ||||||||||||
| return nil, err | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| // stream can be nil | ||||||||||||
| pldLen, pldFldOff, err := objectwire.GetPayloadLengthAndFieldOffset(initial) | ||||||||||||
| if err != nil { | ||||||||||||
| if stream != nil { | ||||||||||||
| stream.Close() | ||||||||||||
| } | ||||||||||||
| return nil, fmt.Errorf("get payload length and field in read header: %w", err) | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| if ln != 0 && !checkPayloadBounds(pldLen, off, ln) { | ||||||||||||
| if stream != nil { | ||||||||||||
| stream.Close() | ||||||||||||
| } | ||||||||||||
| return nil, apistatus.ErrObjectOutOfRange | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| if pldFldOff < 0 { | ||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Check it before |
||||||||||||
| if pldLen != 0 { | ||||||||||||
| return nil, fmt.Errorf("missing payload field tag in %d bytes header, payload len in header = %d", len(initial), pldLen) | ||||||||||||
| } | ||||||||||||
| return nopReadCloser{}, nil | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| _, n, err := iprotobuf.ParseVarint(initial[pldFldOff:]) | ||||||||||||
| if err != nil { | ||||||||||||
| if !errors.Is(err, io.ErrUnexpectedEOF) && !errors.Is(err, io.EOF) { | ||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can this realistically happen for size-limited header? Combined reading? I'd try to avoid retries if possible. |
||||||||||||
| if stream != nil { | ||||||||||||
| stream.Close() | ||||||||||||
| } | ||||||||||||
| return nil, fmt.Errorf("parse payload field len: %w", err) | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| if pldFldOff+binary.MaxVarintLen64 > len(initial) { | ||||||||||||
| if pldFldOff >= binary.MaxVarintLen64 { | ||||||||||||
| n = copy(initial[pldFldOff-binary.MaxVarintLen64:], initial[pldFldOff:]) | ||||||||||||
| pldFldOff -= binary.MaxVarintLen64 | ||||||||||||
| } else { | ||||||||||||
| initial = make([]byte, binary.MaxVarintLen64) | ||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why not just |
||||||||||||
| n = copy(initial, initial[pldFldOff:]) | ||||||||||||
|
Comment on lines
+627
to
+628
|
||||||||||||
| initial = make([]byte, binary.MaxVarintLen64) | |
| n = copy(initial, initial[pldFldOff:]) | |
| tmp := initial | |
| initial = make([]byte, binary.MaxVarintLen64) | |
| n = copy(initial, tmp[pldFldOff:]) |
Copilot
AI
Apr 30, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ReadPayloadRange can call io.ReadFull(stream, ...) even when stream is nil (the function explicitly allows stream to be nil). If ParseVarint returns EOF/UnexpectedEOF and stream is nil, this will panic. Add an explicit stream==nil check in this recovery path and return an error indicating truncated/corrupted object data instead of reading from a nil reader.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GetUint64Field doesn't handle the "field is missing" case: SeekFieldByNumber can return off < 0, which will make buf[off+tagLn:] panic. This contradicts the docstring and can crash callers (e.g., GetPayloadLengthAndFieldOffset when payload length isn't present). Handle off < 0 by returning (0, nil) similar to GetLENFieldBounds, and validate typ before parsing.