i2p/haiku-beta6
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

IORequest: Refactor IOOperation transferred-bytes and status accounting.

Browse Source 
Until the introduction of the nvme_disk driver, these classes were
mostly only used directly by the IO scheduler, and then a few direct
usages of IOOperation itself in the individual disk drivers; so
API confusions were easily missed.

When writing the nvme_disk driver's IORequest support, however, it
became readily apparent that there were some pretty bad confusions
around transferred-bytes accounting in IOOperation. This commit
attempts to resolve all of those.

There are two basic changes here:

1. Move transferred-bytes accounting into IOOperation::SetStatus.

The "TransferredBytes" field of IOOperation is against the *original*
range, not the actual operation's range (which will be wider, due to
bouncing, etc.), and furthermore only applies to the actual content
of the request (and not e.g. to a read half of a bounced write.)

These two facts meant that determining what value to pass to
SetTransferredBytes was not trivial, and was easy to get wrong.
I recall messing that up when working on nvme_disk multiple times
before reading the API carefully.

2. Do not pass redundant values to IORequest::OperationFinished.

All of the values here can be derived (albeit indirectly) from the
IOOperation, and all consumers of this API basically did just that.
Rather than make them do it, make the IORequest take care of
computing all of those values itself.

Change-Id: Ic9ae29e1100319e5b7647647c4db7e5aad4d125e
This commit is contained in:
Augustin Cavalier
2023-04-28 14:59:21 -04:00
parent 8561b2c894
commit be8080575a
5 changed files with 46 additions and 37 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/add-ons/kernel/drivers/disk/nvme/nvme_disk.cpp
+3 -7
View File
@@ -570,7 +570,6 @@ nvme_disk_bounced_io(nvme_disk_handle* handle, io_request* request)
if (status != B_OK)
break;
size_t transferredBytes = 0;
do {
TRACE("%p: IOO offset: %" B_PRIdOFF ", length: %" B_PRIuGENADDR
", write: %s\n", request, operation.Offset(),
@@ -583,10 +582,9 @@ nvme_disk_bounced_io(nvme_disk_handle* handle, io_request* request)
nvme_request.iovec_count = operation.VecCount();
status = do_nvme_io_request(handle->info, &nvme_request);
if (status == B_OK && nvme_request.write == request->IsWrite())
transferredBytes += operation.OriginalLength();
operation.SetStatus(status);
operation.SetStatus(status,
status == B_OK ? operation.Length() : 0);
} while (status == B_OK && !operation.Finish());
if (status == B_OK && operation.Status() != B_OK) {
@@ -594,9 +592,7 @@ nvme_disk_bounced_io(nvme_disk_handle* handle, io_request* request)
status = operation.Status();
}
operation.SetTransferredBytes(transferredBytes);
request->OperationFinished(&operation, status, status != B_OK,
operation.OriginalOffset() + transferredBytes);
request->OperationFinished(&operation);
handle->info->dma_resource.RecycleBuffer(operation.Buffer());
src/system/kernel/device_manager/IOCache.cpp
+3 -2
View File
@@ -203,6 +203,8 @@ void
IOCache::OperationCompleted(IOOperation* operation, status_t status,
generic_size_t transferredBytes)
{
operation->SetStatus(status, transferredBytes);
if (status == B_OK) {
// always fail in case of partial transfers
((Operation*)operation)->finishedCondition.NotifyAll(
@@ -466,8 +468,7 @@ IOCache::_TransferRequestLineUncached(IORequest* request, off_t lineOffset,
error = _DoOperation(operation);
request->OperationFinished(&operation, error, false,
error == B_OK ? operation.OriginalLength() : 0);
request->OperationFinished(&operation);
request->SetUnfinished();
// Keep the request in unfinished state. ScheduleRequest() will set
// the final status and notify.
src/system/kernel/device_manager/IORequest.cpp
+33 -5
View File
@@ -291,10 +291,33 @@ IOBuffer::Dump() const
// #pragma mark -
void
IOOperation::SetStatus(status_t status, generic_size_t completedLength)
{
IORequestChunk::SetStatus(status);
if (IsWrite() == fParent->IsWrite()) {
// Determine how many bytes we actually read or wrote,
// relative to the original range, not the translated range.
const generic_size_t partialBegin = (fOriginalOffset - fOffset);
generic_size_t originalTransferredBytes = completedLength;
if (originalTransferredBytes < partialBegin)
originalTransferredBytes = 0;
else
originalTransferredBytes -= partialBegin;
if (originalTransferredBytes > fOriginalLength)
originalTransferredBytes = fOriginalLength;
fTransferredBytes += originalTransferredBytes;
}
}
bool
IOOperation::Finish()
{
TRACE("IOOperation::Finish()\n");
if (fStatus == B_OK) {
if (fParent->IsWrite()) {
TRACE(" is write\n");
@@ -318,7 +341,7 @@ IOOperation::Finish()
return false;
}
SetStatus(error);
IORequestChunk::SetStatus(error);
} else if (fPhase == PHASE_READ_END) {
TRACE(" phase read end\n");
// repair phase adjusted vec
@@ -338,7 +361,7 @@ IOOperation::Finish()
return false;
}
SetStatus(error);
IORequestChunk::SetStatus(error);
}
}
}
@@ -402,7 +425,7 @@ IOOperation::Finish()
}
if (error != B_OK)
SetStatus(error);
IORequestChunk::SetStatus(error);
}
return true;
@@ -1012,8 +1035,7 @@ IORequest::SetStatusAndNotify(status_t status)
void
IORequest::OperationFinished(IOOperation* operation, status_t status,
bool partialTransfer, generic_size_t transferEndOffset)
IORequest::OperationFinished(IOOperation* operation)
{
TRACE("IORequest::OperationFinished(%p, %#" B_PRIx32 "): request: %p\n",
operation, status, this);
@@ -1023,6 +1045,12 @@ IORequest::OperationFinished(IOOperation* operation, status_t status,
fChildren.Remove(operation);
operation->SetParent(NULL);
const status_t status = operation->Status();
const bool partialTransfer =
(operation->TransferredBytes() < operation->OriginalLength());
const generic_size_t transferEndOffset =
(operation->OriginalOffset() + operation->TransferredBytes());
if (status != B_OK || partialTransfer) {
if (fTransferSize > transferEndOffset)
fTransferSize = transferEndOffset;
src/system/kernel/device_manager/IORequest.h
+5 -8
View File
@@ -127,6 +127,9 @@ struct IOOperation : IORequestChunk, DoublyLinkedListLinkImpl<IOOperation> {
public:
bool Finish();
// returns true, if it can be recycled
// otherwise, there is more to be done
void SetStatus(status_t status, generic_size_t completedLength);
status_t Prepare(IORequest* request);
void SetOriginalRange(off_t offset,
@@ -134,11 +137,9 @@ public:
// also sets range
void SetRange(off_t offset, generic_size_t length);
void SetStatus(status_t status)
{ IORequestChunk::SetStatus(status); }
off_t Offset() const;
generic_size_t Length() const;
off_t OriginalOffset() const
{ return fOriginalOffset; }
generic_size_t OriginalLength() const
@@ -146,8 +147,6 @@ public:
generic_size_t TransferredBytes() const
{ return fTransferredBytes; }
void SetTransferredBytes(generic_size_t bytes)
{ fTransferredBytes = bytes; }
generic_io_vec* Vecs() const;
uint32 VecCount() const;
@@ -256,9 +255,7 @@ struct IORequest : IORequestChunk, DoublyLinkedListLinkImpl<IORequest> {
bool HasCallbacks() const;
void SetStatusAndNotify(status_t status);
void OperationFinished(IOOperation* operation,
status_t status, bool partialTransfer,
generic_size_t transferEndOffset);
void OperationFinished(IOOperation* operation);
void SubRequestFinished(IORequest* request,
status_t status, bool partialTransfer,
generic_size_t transferEndOffset);
src/system/kernel/device_manager/IOSchedulerSimple.cpp
+2 -15
View File
@@ -291,13 +291,7 @@ IOSchedulerSimple::OperationCompleted(IOOperation* operation, status_t status,
if (operation->Status() <= 0)
return;
operation->SetStatus(status);
// set the bytes transferred (of the net data)
generic_size_t partialBegin
= operation->OriginalOffset() - operation->Offset();
operation->SetTransferredBytes(
transferredBytes > partialBegin ? transferredBytes - partialBegin : 0);
operation->SetStatus(status, transferredBytes);
fCompletedOperations.Add(operation);
fFinishedOperationCondition.NotifyAll();
@@ -344,7 +338,6 @@ IOSchedulerSimple::_Finisher()
if (!operationFinished) {
TRACE(" operation: %p not finished yet\n", operation);
MutexLocker _(fLock);
operation->SetTransferredBytes(0);
operation->Parent()->Owner()->operations.Add(operation);
fPendingOperations--;
continue;
@@ -353,13 +346,7 @@ IOSchedulerSimple::_Finisher()
// notify request and remove operation
IORequest* request = operation->Parent();
generic_size_t operationOffset
= operation->OriginalOffset() - request->Offset();
request->OperationFinished(operation, operation->Status(),
operation->TransferredBytes() < operation->OriginalLength(),
operation->Status() == B_OK
? operationOffset + operation->OriginalLength()
: operationOffset);
request->OperationFinished(operation);
// recycle the operation
MutexLocker _(fLock);