fix: do graceful nbd shutdown #1539
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| } | ||
|
|
||
| // Validate request parameters | ||
| deviceSize, err := d.prov.Size() |
There was a problem hiding this comment.
The validation calls d.prov.Size() on every single NBD request (read, write, trim). This happens in the hot path for every I/O operation. Consider caching the device size in the Dispatch struct during initialization, as the device size is unlikely to change during the connection lifetime. This could significantly impact performance under high I/O loads.
| if request.Length > dispatchMaxWriteBufferSize { | ||
| return fmt.Errorf("nbd read request length %d exceeds maximum %d", request.Length, dispatchMaxWriteBufferSize) | ||
| } | ||
| err := d.cmdRead(context.WithoutCancel(ctx), request.Handle, request.From, request.Length) |
There was a problem hiding this comment.
Using context.WithoutCancel(ctx) for read/write operations means these operations will continue even after the parent context is cancelled. However, the performRead and performWrite functions still check ctx.Done() before sending responses. This creates inconsistent behavior: the I/O operation will complete but the response handling respects the (now detached) context cancellation. Consider whether this is the intended behavior or if the entire operation should respect shutdown signals.
| rp += dataCopied | ||
|
|
||
| // Validate that rp hasn't exceeded wp after copy | ||
| if rp > wp { |
There was a problem hiding this comment.
The buffer pointer validation if rp > wp after the copy operation at line 221 can never be true. Since rp += dataCopied and dataCopied = copy(data, buffer[rp:wp]) where rp < wp initially, and copy returns at most wp - rp, the new rp value will always be <= wp. This check is redundant and can be removed.
No description provided.