Closed Bug 791301 Opened 7 years ago Closed 7 years ago

Generic fallocate code is broken for files smaller than system block size

Categories

(Core :: General, defect)

x86_64
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18

People

(Reporter: gcp, Assigned: gcp)

References

Details

Attachments

(1 file)

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: nobody → gpascutto
Blocks: 741808
Attachment #661283 - Flags: review?(taras.mozilla)
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+
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.
https://hg.mozilla.org/mozilla-central/rev/00be82b96291
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
You need to log in before you can comment on or make changes to this bug.