Closed
Bug 791301
Opened 12 years ago
Closed 12 years ago
Generic fallocate code is broken for files smaller than system block size
Categories
(Core :: General, defect)
Tracking
()
RESOLVED
FIXED
mozilla18
People
(Reporter: gcp, Assigned: gcp)
References
Details
Attachments
(1 file)
(deleted),
patch
|
taras.mozilla
:
review+
|
Details | Diff | Splinter Review |
As observed in bug 741808, fallocate doesn't work correctly on Android for small files.
The problem is the following logic in mozilla::fallocate:
int64_t iWrite = ((buf.st_size + 2 * nBlk - 1) / nBlk) * nBlk - 1; // Next offset to write to
do {
nWrite = 0;
if (PR_Seek64(aFD, iWrite, PR_SEEK_SET) == iWrite)
nWrite = PR_Write(aFD, "", 1);
iWrite += nBlk;
} while (nWrite == 1 && iWrite < aLength);
Because of the do {} while loop we will always write at least one extra block to the file. This means it would also be broken for large files that are extended by less than a filesystem block.
The code purports to be copied from sqlite, but the sqlite source in our tree does do it correctly:
iWrite = ((buf.st_size + 2*nBlk - 1)/nBlk)*nBlk-1;
while( iWrite<nSize ){
int nWrite = seekAndWrite(pFile, iWrite, "", 1);
if( nWrite!=1 ) return SQLITE_IOERR_WRITE;
iWrite += nBlk;
}
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → gpascutto
Assignee | ||
Comment 1•12 years ago
|
||
Attachment #661283 -
Flags: review?(taras.mozilla)
Comment 2•12 years ago
|
||
Comment on attachment 661283 [details] [diff] [review]
Patch 1. Fix broken fallocate logic
Good catch. Oops. Shouldn't have copied this blindly.
While we should fix this, the proper fix here is to implement the fallocate syscall on android.
I'm starting to question the value of this fallback path. A crappy filesystem will still fragment under the emulated fallocate, but this fallback costs us non-constant io(whereas fallocate has a constant io cost). We should change fallocate to try_to_fallocate to make it more of an advisory call that does not execute fallback paths(other than a single sparse write to get a consistent file size).
Attachment #661283 -
Flags: review?(taras.mozilla) → review+
Assignee | ||
Comment 3•12 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=8b8880b38702
>While we should fix this, the proper fix here is to implement the fallocate
>syscall on android.
Bug 693485.
>I'm starting to question the value of this fallback path.
It probably works reasonably well on Android, which has a crappy libc but in most cases should have a reasonably filesystem. For our other core platforms we have working fallocate code.
Assignee | ||
Comment 4•12 years ago
|
||
Comment 5•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in
before you can comment on or make changes to this bug.
Description
•