Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2860943005:
Ensure that memory-maped files are fully realized. (Closed)
DescriptionEnsure that memory-maped files are fully realized.
Memory-mapped files cause crashes if there are I/O problems. By ensuring
that the file is fully realized on disk, we eliminate a disk-full condition
from causing a crash down the road.
BUG=715523, 717331
Review-Url: https://codereview.chromium.org/2860943005
Cr-Commit-Position: refs/heads/master@{#471056}
Committed: https://chromium.googlesource.com/chromium/src/+/e41b5ce8a877875575ad0c370bd4bdc8593113b2
Patch Set 1 #Patch Set 2 : fix cross-platform build problems #
Total comments: 10
Patch Set 3 : do manual extension on older Android #Patch Set 4 : fix other build problems #
Total comments: 2
Patch Set 5 : addressed review comments by mark #
Total comments: 2
Patch Set 6 : use existing_byte for write #Messages
Total messages: 66 (50 generated)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
bcwhite@chromium.org changed reviewers: + mark@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
mark@chromium.org changed reviewers: + primiano@chromium.org
+primiano for the filesystem question https://codereview.chromium.org/2860943005/diff/40001/base/files/memory_mappe... File base/files/memory_mapped_file_posix.cc (right): https://codereview.chromium.org/2860943005/diff/40001/base/files/memory_mappe... base/files/memory_mapped_file_posix.cc:95: if (!file_.SetLength(std::max(file_len, region.offset + region.size))) { This isn’t necessary if you’re going to call [posix_]fallocate(). You can move it into the OS_MACOSX case, or (depending on how you handle fallocate() failure) you can use it as a fallback if fallocate() fails with EOPNOTSUPP. https://codereview.chromium.org/2860943005/diff/40001/base/files/memory_mappe... base/files/memory_mapped_file_posix.cc:100: // Realize the extent of the file so that it can't fail (and crash) later This is one of clang-format’s problems, these comments were probably intended to have been bumped over to line up with the code, not the #if. https://codereview.chromium.org/2860943005/diff/40001/base/files/memory_mappe... base/files/memory_mapped_file_posix.cc:107: // Android doesn't support the POSIX call so use the native Linux one. Curious about this. It looks like both fallocate() and posix_fallocate() were added to Bionic in https://android.googlesource.com/platform/bionic/+/f64b8ea09db3bdd84eed59f772.... Surprised that you found that one worked but the other didn’t. I’d expect either both to be present, or both not to be. Since they both showed up in API 21 (Android 5.0/“L”), I’d expect this to work for our 64-bit Android builds (for which we target API 21), but not to for our 32-bit builds (for which we target API 16). The trybots ought to know for sure. https://codereview.chromium.org/2860943005/diff/40001/base/files/memory_mappe... base/files/memory_mapped_file_posix.cc:108: if (fallocate(file_.GetPlatformFile(), 0, region.offset, region.size) != Anyway, whether it’s fallocate() or posix_fallocate(), Bionic doesn’t seem to fall back to writing to the file to force allocation if the underlying filesystem doesn’t support this call. In contrast, glibc does this for posix_fallocate(): https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/posi... where internal_fallocate() winds up being: https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/posix/posix_fallocat... ext4 will support fallocate(). ext3 won’t. I’m not positive that we’re assured of ext4, or that we’re assured of a filesystem that supports fallocate(). Primiano may know.
https://codereview.chromium.org/2860943005/diff/40001/base/files/memory_mappe... File base/files/memory_mapped_file_posix.cc (right): https://codereview.chromium.org/2860943005/diff/40001/base/files/memory_mappe... base/files/memory_mapped_file_posix.cc:107: // Android doesn't support the POSIX call so use the native Linux one. On 2017/05/04 21:00:46, Mark Mentovai wrote: > Curious about this. It looks like both fallocate() and posix_fallocate() were > added to Bionic in > https://android.googlesource.com/platform/bionic/+/f64b8ea09db3bdd84eed59f772.... > Surprised that you found that one worked but the other didn’t. I’d expect either > both to be present, or both not to be. > > Since they both showed up in API 21 (Android 5.0/“L”), I’d expect this to work > for our 64-bit Android builds (for which we target API 21), but not to for our > 32-bit builds (for which we target API 16). The trybots ought to know for sure. Yeah, the Android builds all fail with either call. <sigh> And unfortunately, it's the most important platform. I'm thinking that, for 32-bit builds, I should just do the write of bytes. I could try the low-level system call and just write the bytes if it returns not-implemented. What do you think?
I guess so. Can we use linux-syscall-support in here for that?
Filling the file seems the less controversial proposal. My only comment is that you should definitely expect both fallocate and ftruncate to fail on some versions of android out there and I think you should have a fallback based on unlink & write zeros. Consider that devices out there use a mixture of vfat, ext4 and f2fs so I don't know what to expect. There is at least a bug on ftruncate+Selinux I remember of (b/22567870). I remember we were using ftruncate in breakpad and at some point had to get rid of them because of that bug. https://codereview.chromium.org/2860943005/diff/40001/base/files/memory_mappe... File base/files/memory_mapped_file_posix.cc (right): https://codereview.chromium.org/2860943005/diff/40001/base/files/memory_mappe... base/files/memory_mapped_file_posix.cc:107: // Android doesn't support the POSIX call so use the native Linux one. On 2017/05/04 21:00:46, Mark Mentovai wrote: > Curious about this. It looks like both fallocate() and posix_fallocate() were > added to Bionic in > https://android.googlesource.com/platform/bionic/+/f64b8ea09db3bdd84eed59f772.... > Surprised that you found that one worked but the other didn’t. I’d expect either > both to be present, or both not to be. > > Since they both showed up in API 21 (Android 5.0/“L”), I’d expect this to work > for our 64-bit Android builds (for which we target API 21), but not to for our > 32-bit builds (for which we target API 16). The trybots ought to know for sure. FYI we currently ship 32 bit only chrome also on 64 bit devices. The only case where we ship a 64 bit binary is: - webview (which I think is not going to be affected by this) - canary (to make sure our 64 bit chrome doesn't rot)
On 2017/05/05 15:56:24, Mark Mentovai wrote: > I guess so. Can we use linux-syscall-support in here for that? Maybe it's sufficient to do byte-writes based on #if defined(OS_ANDROID) && !defined(ARCH_CPU_64_BITS) I'll try it.
#include <android/api-level.h> and use the __ANDROID_API__ macro. If it’s >= 21, fallocate() will be available. So will posix_fallocate(), but it won’t have its own built-in byte-writing fallback as glibc’s does, so you’ll still need to deal with EOPNOTSUPP.
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:60001) has been deleted
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Patchset #4 (id:80001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Patchset #4 (id:100001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) chromeos_daisy_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Patchset #5 (id:140001) has been deleted
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Patchset #4 (id:160001) has been deleted
https://codereview.chromium.org/2860943005/diff/40001/base/files/memory_mappe... File base/files/memory_mapped_file_posix.cc (right): https://codereview.chromium.org/2860943005/diff/40001/base/files/memory_mappe... base/files/memory_mapped_file_posix.cc:95: if (!file_.SetLength(std::max(file_len, region.offset + region.size))) { On 2017/05/04 21:00:47, Mark Mentovai wrote: > This isn’t necessary if you’re going to call [posix_]fallocate(). You can move > it into the OS_MACOSX case, or (depending on how you handle fallocate() failure) > you can use it as a fallback if fallocate() fails with EOPNOTSUPP. Acknowledged. https://codereview.chromium.org/2860943005/diff/40001/base/files/memory_mappe... base/files/memory_mapped_file_posix.cc:100: // Realize the extent of the file so that it can't fail (and crash) later On 2017/05/04 21:00:47, Mark Mentovai wrote: > This is one of clang-format’s problems, these comments were probably intended to > have been bumped over to line up with the code, not the #if. Done. https://codereview.chromium.org/2860943005/diff/40001/base/files/memory_mappe... base/files/memory_mapped_file_posix.cc:107: // Android doesn't support the POSIX call so use the native Linux one. On 2017/05/05 16:23:33, Primiano Tucci wrote: > On 2017/05/04 21:00:46, Mark Mentovai wrote: > > Curious about this. It looks like both fallocate() and posix_fallocate() were > > added to Bionic in > > > https://android.googlesource.com/platform/bionic/+/f64b8ea09db3bdd84eed59f772.... > > Surprised that you found that one worked but the other didn’t. I’d expect > either > > both to be present, or both not to be. > > > > Since they both showed up in API 21 (Android 5.0/“L”), I’d expect this to work > > for our 64-bit Android builds (for which we target API 21), but not to for our > > 32-bit builds (for which we target API 16). The trybots ought to know for > sure. > > FYI we currently ship 32 bit only chrome also on 64 bit devices. > The only case where we ship a 64 bit binary is: > - webview (which I think is not going to be affected by this) > - canary (to make sure our 64 bit chrome doesn't rot) Acknowledged. https://codereview.chromium.org/2860943005/diff/40001/base/files/memory_mappe... base/files/memory_mapped_file_posix.cc:108: if (fallocate(file_.GetPlatformFile(), 0, region.offset, region.size) != On 2017/05/04 21:00:46, Mark Mentovai wrote: > Anyway, whether it’s fallocate() or posix_fallocate(), Bionic doesn’t seem to > fall back to writing to the file to force allocation if the underlying > filesystem doesn’t support this call. In contrast, glibc does this for > posix_fallocate(): > > https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/unix/sysv/linux/posi... > > where internal_fallocate() winds up being: > > https://sourceware.org/git/?p=glibc.git;a=blob;f=sysdeps/posix/posix_fallocat... > > ext4 will support fallocate(). ext3 won’t. I’m not positive that we’re assured > of ext4, or that we’re assured of a filesystem that supports fallocate(). > Primiano may know. Done.
https://codereview.chromium.org/2860943005/diff/180001/base/files/memory_mapp... File base/files/memory_mapped_file_posix.cc (right): https://codereview.chromium.org/2860943005/diff/180001/base/files/memory_mapp... base/files/memory_mapped_file_posix.cc:135: if (i > original_file_len) { I don’t see anything here that requires file_ to have not been a sparse file to begin with. It’d be safer to get rid of this check and instead probe with pread, pwrite-ing over any byte that shows up as 0.
> I think you should have a fallback based on unlink & write zeros. Or just return failure here and fall back not using a mmapped file in the cases where we fallocate/ftruncate don't work.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
> Or just return failure here and fall back not using a > mmapped file in the cases where we fallocate/ftruncate > don't work. That's most likely on Android which is probably the platform that will benefit the most from persistence. https://codereview.chromium.org/2860943005/diff/180001/base/files/memory_mapp... File base/files/memory_mapped_file_posix.cc (right): https://codereview.chromium.org/2860943005/diff/180001/base/files/memory_mapp... base/files/memory_mapped_file_posix.cc:135: if (i > original_file_len) { On 2017/05/05 18:31:44, Mark Mentovai wrote: > I don’t see anything here that requires file_ to have not been a sparse file to > begin with. It’d be safer to get rid of this check and instead probe with pread, > pwrite-ing over any byte that shows up as 0. Correct. It's making the assumption that whatever previously wrote the file didn't leave any holes. I was trying to be efficient but I guess it's read-modify-write of the block any way we look at it so probably better to be safe.
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ping?
LGTM! https://codereview.chromium.org/2860943005/diff/200001/base/files/memory_mapp... File base/files/memory_mapped_file_posix.cc (right): https://codereview.chromium.org/2860943005/diff/200001/base/files/memory_mapp... base/files/memory_mapped_file_posix.cc:140: if (pwrite(file_.GetPlatformFile(), "", 1, i) != 1) You can recycle &existing_byte here, maybe slightly clearer than ""
The CQ bit was checked by bcwhite@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2860943005/diff/200001/base/files/memory_mapp... File base/files/memory_mapped_file_posix.cc (right): https://codereview.chromium.org/2860943005/diff/200001/base/files/memory_mapp... base/files/memory_mapped_file_posix.cc:140: if (pwrite(file_.GetPlatformFile(), "", 1, i) != 1) On 2017/05/10 19:10:14, Mark Mentovai wrote: > You can recycle &existing_byte here, maybe slightly clearer than "" Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by bcwhite@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mark@chromium.org Link to the patchset: https://codereview.chromium.org/2860943005/#ps220001 (title: "use existing_byte for write")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 220001, "attempt_start_ts": 1494533598584250,
"parent_rev": "1a94a262a90ca1f0dad5952b021f89628f1c81f7", "commit_rev":
"e41b5ce8a877875575ad0c370bd4bdc8593113b2"}
Message was sent while issue was closed.
Description was changed from ========== Ensure that memory-maped files are fully realized. Memory-mapped files cause crashes if there are I/O problems. By ensuring that the file is fully realized on disk, we eliminate a disk-full condition from causing a crash down the road. BUG=715523, 717331 ========== to ========== Ensure that memory-maped files are fully realized. Memory-mapped files cause crashes if there are I/O problems. By ensuring that the file is fully realized on disk, we eliminate a disk-full condition from causing a crash down the road. BUG=715523, 717331 Review-Url: https://codereview.chromium.org/2860943005 Cr-Commit-Position: refs/heads/master@{#471056} Committed: https://chromium.googlesource.com/chromium/src/+/e41b5ce8a877875575ad0c370bd4... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:220001) as https://chromium.googlesource.com/chromium/src/+/e41b5ce8a877875575ad0c370bd4... |
