Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1023103002:
Let ImportantFileWriter Use fdatasync (Closed)
|
Created:
5 years, 9 months ago by hashimoto Modified:
5 years, 9 months ago Reviewers:
rvargas (doing something else) CC:
chromium-reviews, tfarina, erikwright+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionLet ImportantFileWriter Use fdatasync
Add a new method base::File::FlushData to use fdatasync.
BUG=469071
Committed: https://crrev.com/1d2b56283016e17cad1595b131c770590f342059
Cr-Commit-Position: refs/heads/master@{#321943}
Patch Set 1 : #
Total comments: 9
Patch Set 2 : Flush() uses fdatasync #Messages
Total messages: 13 (5 generated)
Patchset #2 (id:20001) has been deleted
Patchset #1 (id:1) has been deleted
Patchset #1 (id:40001) has been deleted
hashimoto@chromium.org changed reviewers: + rvargas@chromium.org
https://codereview.chromium.org/1023103002/diff/60001/base/files/file.h File base/files/file.h (right): https://codereview.chromium.org/1023103002/diff/60001/base/files/file.h#newco... base/files/file.h:262: // (Linux: fdatasync, other platforms: same as Flush()) I don't think we want to end up with two methods that do almost the same thing, especially if we are not keeping the same behavior across all platforms* (which is one of the points of having base). (*) as in both methods doing the same thing on Windows. We should probably move to fdatasync and be done with it (I doubt FlushFileBuffers guarantees that metadata is also flushed to disk). If someone actually requires all metadata to be written then something must be done at the directory level. https://codereview.chromium.org/1023103002/diff/60001/base/files/file_posix.cc File base/files/file_posix.cc (right): https://codereview.chromium.org/1023103002/diff/60001/base/files/file_posix.c... base/files/file_posix.cc:440: return !HANDLE_EINTR(fdatasync(file_.get())); is fdatasync a Linux-only thing? What's the behavior on BSD? https://codereview.chromium.org/1023103002/diff/60001/base/files/important_fi... File base/files/important_file_writer.cc (right): https://codereview.chromium.org/1023103002/diff/60001/base/files/important_fi... base/files/important_file_writer.cc:93: bool flush_success = tmp_file.FlushData(); Incidentally, crbug.com/468046 is about doing _more_ work here instead of less.
https://codereview.chromium.org/1023103002/diff/60001/base/files/file.h File base/files/file.h (right): https://codereview.chromium.org/1023103002/diff/60001/base/files/file.h#newco... base/files/file.h:262: // (Linux: fdatasync, other platforms: same as Flush()) On 2015/03/20 18:56:05, rvargas wrote: > I don't think we want to end up with two methods that do almost the same thing, > especially if we are not keeping the same behavior across all platforms* (which > is one of the points of having base). > > (*) as in both methods doing the same thing on Windows. > > We should probably move to fdatasync and be done with it (I doubt > FlushFileBuffers guarantees that metadata is also flushed to disk). If someone > actually requires all metadata to be written then something must be done at the > directory level. Good point, I thought someone might be relying on Flush() to flush metadata, but if we've not been guaranteeing that after all, it should be OK. Just switching to fdatasync sounds good. (maybe it might be better to make a PSA on chromium-dev about the change?) https://codereview.chromium.org/1023103002/diff/60001/base/files/file_posix.cc File base/files/file_posix.cc (right): https://codereview.chromium.org/1023103002/diff/60001/base/files/file_posix.c... base/files/file_posix.cc:440: return !HANDLE_EINTR(fdatasync(file_.get())); On 2015/03/20 18:56:05, rvargas wrote: > is fdatasync a Linux-only thing? What's the behavior on BSD? fdatasync is a part of POSIX, but it's not mandatory. According to what I get from casual searching, Mac OS does not implement fdatasync. https://codereview.chromium.org/1023103002/diff/60001/base/files/important_fi... File base/files/important_file_writer.cc (right): https://codereview.chromium.org/1023103002/diff/60001/base/files/important_fi... base/files/important_file_writer.cc:93: bool flush_success = tmp_file.FlushData(); On 2015/03/20 18:56:05, rvargas wrote: > Incidentally, crbug.com/468046 is about doing _more_ work here instead of less. Hmm, ImportantFileWriter's slowness is already a problem (crbug.com/469071). Is it possible to do something to make it faster? (e.g. always keep 2 files to avoid creating a temporary file every time, toggling between them when write happens.)
https://codereview.chromium.org/1023103002/diff/60001/base/files/important_fi... File base/files/important_file_writer.cc (right): https://codereview.chromium.org/1023103002/diff/60001/base/files/important_fi... base/files/important_file_writer.cc:93: bool flush_success = tmp_file.FlushData(); On 2015/03/20 19:38:57, hashimoto wrote: > On 2015/03/20 18:56:05, rvargas wrote: > > Incidentally, crbug.com/468046 is about doing _more_ work here instead of > less. > > Hmm, ImportantFileWriter's slowness is already a problem (crbug.com/469071). > Is it possible to do something to make it faster? (e.g. always keep 2 files to > avoid creating a temporary file every time, toggling between them when write > happens.) Oops, wrong bug number. crbug.com/418627 is the correct one.
lgtm https://codereview.chromium.org/1023103002/diff/60001/base/files/file.h File base/files/file.h (right): https://codereview.chromium.org/1023103002/diff/60001/base/files/file.h#newco... base/files/file.h:262: // (Linux: fdatasync, other platforms: same as Flush()) On 2015/03/20 19:38:56, hashimoto wrote: > On 2015/03/20 18:56:05, rvargas wrote: > > I don't think we want to end up with two methods that do almost the same > thing, > > especially if we are not keeping the same behavior across all platforms* > (which > > is one of the points of having base). > > > > (*) as in both methods doing the same thing on Windows. > > > > We should probably move to fdatasync and be done with it (I doubt > > FlushFileBuffers guarantees that metadata is also flushed to disk). If someone > > actually requires all metadata to be written then something must be done at > the > > directory level. > > Good point, I thought someone might be relying on Flush() to flush metadata, but > if we've not been guaranteeing that after all, it should be OK. > Just switching to fdatasync sounds good. (maybe it might be better to make a PSA > on chromium-dev about the change?) PSA sounds fine. https://codereview.chromium.org/1023103002/diff/60001/base/files/important_fi... File base/files/important_file_writer.cc (right): https://codereview.chromium.org/1023103002/diff/60001/base/files/important_fi... base/files/important_file_writer.cc:93: bool flush_success = tmp_file.FlushData(); On 2015/03/20 19:40:30, hashimoto wrote: > On 2015/03/20 19:38:57, hashimoto wrote: > > On 2015/03/20 18:56:05, rvargas wrote: > > > Incidentally, crbug.com/468046 is about doing _more_ work here instead of > > less. > > > > Hmm, ImportantFileWriter's slowness is already a problem (crbug.com/469071). > > Is it possible to do something to make it faster? (e.g. always keep 2 files to > > avoid creating a temporary file every time, toggling between them when write > > happens.) > > Oops, wrong bug number. > crbug.com/418627 is the correct one. Shouldn't we profile it from the field? The fact that it appears in a crash (caused by a slow shutdown) doesn't really imply that the operation is too slow (as in, having actual numbers will probably tell us more about what's going on). On the other hand, while I really see no issue in ignoring some metadata (like the times) when writing an "important" file, I see the issue in returning too soon saying all's done only to find out after reboot that there's no file (or an old one). Before deciding to do something else to make this faster we need some data.
The CQ bit was checked by hashimoto@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1023103002/80001
Message was sent while issue was closed.
Committed patchset #2 (id:80001)
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/1d2b56283016e17cad1595b131c770590f342059 Cr-Commit-Position: refs/heads/master@{#321943} |
