Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
7671039:
Count-up/down the dirty count in the usage cache at FileSystemOperation, not at QuotaFU. (Closed)
|
Created:
9 years, 4 months ago by Dai Mikurube (NOT FULLTIME) Modified:
9 years, 3 months ago CC:
chromium-reviews, kinuko+watch, darin-cc_chromium.org Visibility:
Public. |
DescriptionCount-up/down the dirty count in the usage cache at FileSystemOperation, not at QuotaFU.
BUG=none
TEST=none
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=98942
Patch Set 1 #
Total comments: 5
Patch Set 2 : Renamed ScopedOriginUpdateHelper. #
Total comments: 6
Patch Set 3 : Removed (Scoped)OriginUpdateHelper. #Patch Set 4 : Reflected the comments. #
Total comments: 2
Patch Set 5 : Reflected the comments. #
Total comments: 3
Messages
Total messages: 20 (0 generated)
It's the first cut.
http://codereview.chromium.org/7671039/diff/1/webkit/fileapi/file_system_oper... File webkit/fileapi/file_system_operation.cc (right): http://codereview.chromium.org/7671039/diff/1/webkit/fileapi/file_system_oper... webkit/fileapi/file_system_operation.cc:185: quota_util_helper_.reset(new ScopedQuotaUtilHelper( This feels a bit odd here, given that many filesystems won't have quota issues. Perhaps in OFSFU and QFU both, as came up in the meeting on Wednesday? http://codereview.chromium.org/7671039/diff/1/webkit/fileapi/quota_file_util.cc File webkit/fileapi/quota_file_util.cc (right): http://codereview.chromium.org/7671039/diff/1/webkit/fileapi/quota_file_util.... webkit/fileapi/quota_file_util.cc:28: class ScopedOriginUpdateHelper { Does this class now not actually do anything "scoped"?
Thank you for reviewing, Eric. http://codereview.chromium.org/7671039/diff/1/webkit/fileapi/file_system_oper... File webkit/fileapi/file_system_operation.cc (right): http://codereview.chromium.org/7671039/diff/1/webkit/fileapi/file_system_oper... webkit/fileapi/file_system_operation.cc:185: quota_util_helper_.reset(new ScopedQuotaUtilHelper( On 2011/08/19 04:12:59, ericu wrote: > This feels a bit odd here, given that many filesystems won't have quota issues. > Perhaps in OFSFU and QFU both, as came up in the meeting on Wednesday? I basically agree with that, but I guess it has a trade-off issue. If we do that both in FUs, we have to make sure that the inner pair of ++/-- is sandwiched by the outer pair of ++/-- in some way. For example, if we do an operation in the following order: 1) OFSFU's ++ 2) OFSFU path name accounting 3) OFSFU's -- 4) QFU's ++ 5) QFU file content accounting 6) QFU's -- the usage cache can have an inconsistent state. What do you think, Kinuko-san? http://codereview.chromium.org/7671039/diff/1/webkit/fileapi/quota_file_util.cc File webkit/fileapi/quota_file_util.cc (right): http://codereview.chromium.org/7671039/diff/1/webkit/fileapi/quota_file_util.... webkit/fileapi/quota_file_util.cc:28: class ScopedOriginUpdateHelper { On 2011/08/19 04:12:59, ericu wrote: > Does this class now not actually do anything "scoped"? Agreed. I'll fix it in the next patch.
http://codereview.chromium.org/7671039/diff/1/webkit/fileapi/file_system_oper... File webkit/fileapi/file_system_operation.cc (right): http://codereview.chromium.org/7671039/diff/1/webkit/fileapi/file_system_oper... webkit/fileapi/file_system_operation.cc:185: quota_util_helper_.reset(new ScopedQuotaUtilHelper( On 2011/08/19 10:21:10, Dai Mikurube wrote: > On 2011/08/19 04:12:59, ericu wrote: > > This feels a bit odd here, given that many filesystems won't have quota > issues. > > Perhaps in OFSFU and QFU both, as came up in the meeting on Wednesday? > > I basically agree with that, but I guess it has a trade-off issue. If we do > that both in FUs, we have to make sure that the inner pair of ++/-- is > sandwiched by the outer pair of ++/-- in some way. > > For example, if we do an operation in the following order: > 1) OFSFU's ++ > 2) OFSFU path name accounting > 3) OFSFU's -- > 4) QFU's ++ > 5) QFU file content accounting > 6) QFU's -- > the usage cache can have an inconsistent state. > > What do you think, Kinuko-san? I'm ok with eric's proposal iff we can make sure the ordering and that all the operations that modify both database and filesystem also mark dirty in the OFSFU. (Sounds like it's true after this path name accounting change, but I'd like to make double-sure)
On Fri, Aug 19, 2011 at 3:21 AM, <dmikurube@chromium.org> wrote: > Thank you for reviewing, Eric. > > > http://codereview.chromium.org/7671039/diff/1/webkit/fileapi/file_system_oper... > File webkit/fileapi/file_system_operation.cc (right): > > http://codereview.chromium.org/7671039/diff/1/webkit/fileapi/file_system_oper... > webkit/fileapi/file_system_operation.cc:185: > quota_util_helper_.reset(new ScopedQuotaUtilHelper( > On 2011/08/19 04:12:59, ericu wrote: >> >> This feels a bit odd here, given that many filesystems won't have > > quota issues. >> >> Perhaps in OFSFU and QFU both, as came up in the meeting on Wednesday? > > I basically agree with that, but I guess it has a trade-off issue. If > we do that both in FUs, we have to make sure that the inner pair of > ++/-- is sandwiched by the outer pair of ++/-- in some way. > > For example, if we do an operation in the following order: > 1) OFSFU's ++ > 2) OFSFU path name accounting > 3) OFSFU's -- > 4) QFU's ++ > 5) QFU file content accounting > 6) QFU's -- > the usage cache can have an inconsistent state. Agreed. We can avoid that by using a ScopedQuotaUtilHelper in both OFSFU and QFU. I expect that we can use the same class in both cases, just two different instances. > What do you think, Kinuko-san? > > http://codereview.chromium.org/7671039/diff/1/webkit/fileapi/quota_file_util.cc > File webkit/fileapi/quota_file_util.cc (right): > > http://codereview.chromium.org/7671039/diff/1/webkit/fileapi/quota_file_util.... > webkit/fileapi/quota_file_util.cc:28: class ScopedOriginUpdateHelper { > On 2011/08/19 04:12:59, ericu wrote: >> >> Does this class now not actually do anything "scoped"? > > Agreed. I'll fix it in the next patch. > > http://codereview.chromium.org/7671039/ >
On 2011/08/19 22:28:55, ericu wrote: > On Fri, Aug 19, 2011 at 3:21 AM, <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=dmikurube@chromium.org> wrote: > > Thank you for reviewing, Eric. > http://codereview.chromium.org/7671039/diff/1/webkit/fileapi/file_system_oper... > > File webkit/fileapi/file_system_operation.cc (right): > > > > > http://codereview.chromium.org/7671039/diff/1/webkit/fileapi/file_system_oper... > > webkit/fileapi/file_system_operation.cc:185: > > quota_util_helper_.reset(new ScopedQuotaUtilHelper( > > On 2011/08/19 04:12:59, ericu wrote: > >> > >> This feels a bit odd here, given that many filesystems won't have > > > > quota issues. For filesystems we don't have quota issue GetQuotaUtil(type) will return NULL, therefore we can safely skip unnecessary quota operations. > >> Perhaps in OFSFU and QFU both, as came up in the meeting on Wednesday? > > I basically agree with that, but I guess it has a trade-off issue. If > > we do that both in FUs, we have to make sure that the inner pair of > > ++/-- is sandwiched by the outer pair of ++/-- in some way. > > > > For example, if we do an operation in the following order: > > 1) OFSFU's ++ > > 2) OFSFU path name accounting > > 3) OFSFU's -- > > 4) QFU's ++ > > 5) QFU file content accounting > > 6) QFU's -- > > > the usage cache can have an inconsistent state. > Agreed. We can avoid that by using a ScopedQuotaUtilHelper in both > OFSFU and QFU. I expect that we can use the same class in both cases, > just two different instances. Dai's diagram might be a bit delusive but what we want to achieve is: 1) dirty ++ 2) DB change (possibly update usage too) 3) Filesystem change + usage update 4) dirty -- Please note that this proposal is independent from path name accounting at OFSFU. We do want to increment dirty before making DB changes regardless of whether we do update usage at OFSFU, and that's why I think incrementing/decrementing at upper layer would make more sense. (Imagine instantiating ScopedQuotaUtilHelper while we do not update usage at OFSFU-- that would look very weird) > > What do you think, Kinuko-san? > > > > > http://codereview.chromium.org/7671039/diff/1/webkit/fileapi/quota_file_util.cc > > File webkit/fileapi/quota_file_util.cc (right): > > > > > http://codereview.chromium.org/7671039/diff/1/webkit/fileapi/quota_file_util.... > > webkit/fileapi/quota_file_util.cc:28: class ScopedOriginUpdateHelper { > > On 2011/08/19 04:12:59, ericu wrote: > >> > >> Does this class now not actually do anything "scoped"? > > > > Agreed. I'll fix it in the next patch. > > > > http://codereview.chromium.org/7671039/ > >
On 2011/08/20 02:56:16, kinuko wrote: > On 2011/08/19 22:28:55, ericu wrote: > > On Fri, Aug 19, 2011 at 3:21 AM, > <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=dmikurube%40chromium.org> > wrote: > > > Thank you for reviewing, Eric. > > > http://codereview.chromium.org/7671039/diff/1/webkit/fileapi/file_system_oper... > > > File webkit/fileapi/file_system_operation.cc (right): > > > > > > > > > http://codereview.chromium.org/7671039/diff/1/webkit/fileapi/file_system_oper... > > > webkit/fileapi/file_system_operation.cc:185: > > > quota_util_helper_.reset(new ScopedQuotaUtilHelper( > > > On 2011/08/19 04:12:59, ericu wrote: > > >> > > >> This feels a bit odd here, given that many filesystems won't have > > > > > > quota issues. > > For filesystems we don't have quota issue GetQuotaUtil(type) will return NULL, > therefore we can safely skip unnecessary quota operations. > > > >> Perhaps in OFSFU and QFU both, as came up in the meeting on Wednesday? > > > > I basically agree with that, but I guess it has a trade-off issue. If > > > we do that both in FUs, we have to make sure that the inner pair of > > > ++/-- is sandwiched by the outer pair of ++/-- in some way. > > > > > > For example, if we do an operation in the following order: > > > 1) OFSFU's ++ > > > 2) OFSFU path name accounting > > > 3) OFSFU's -- > > > 4) QFU's ++ > > > 5) QFU file content accounting > > > 6) QFU's -- > > > > > the usage cache can have an inconsistent state. > > Agreed. We can avoid that by using a ScopedQuotaUtilHelper in both > > OFSFU and QFU. I expect that we can use the same class in both cases, > > just two different instances. > > Dai's diagram might be a bit delusive but what we want to achieve is: > > 1) dirty ++ > 2) DB change (possibly update usage too) > 3) Filesystem change + usage update > 4) dirty -- > > Please note that this proposal is independent from path name accounting at > OFSFU. > We do want to increment dirty before making DB changes regardless of whether we > do update usage at OFSFU, and that's why I think incrementing/decrementing at > upper layer would make more sense. > (Imagine instantiating ScopedQuotaUtilHelper while we do not update usage at > OFSFU-- that would look very weird) Anyway now that the directory accounting patch seems to be landed without StartUpdate / EndUpdate let's fix marking/clearing dirty flag logic. I prefer having the logic at higher level (because of the reasons I wrote above) but if we choose to put it at both levels please put appropriate comments about how dirty++/-- need to be ordered (and about the fact that we need ++dirty before DB change whether we update usage cache at OFSFU or not). > > > What do you think, Kinuko-san? > > > > > > > > > http://codereview.chromium.org/7671039/diff/1/webkit/fileapi/quota_file_util.cc > > > File webkit/fileapi/quota_file_util.cc (right): > > > > > > > > > http://codereview.chromium.org/7671039/diff/1/webkit/fileapi/quota_file_util.... > > > webkit/fileapi/quota_file_util.cc:28: class ScopedOriginUpdateHelper { > > > On 2011/08/19 04:12:59, ericu wrote: > > >> > > >> Does this class now not actually do anything "scoped"? > > > > > > Agreed. I'll fix it in the next patch. > > > > > > http://codereview.chromium.org/7671039/ > > >
On 2011/08/20 02:56:16, kinuko wrote: > On 2011/08/19 22:28:55, ericu wrote: > > On Fri, Aug 19, 2011 at 3:21 AM, > <https://mail.google.com/mail/?view=cm&fs=1&tf=1&to=dmikurube%40chromium.org> > wrote: > > > Thank you for reviewing, Eric. > > > http://codereview.chromium.org/7671039/diff/1/webkit/fileapi/file_system_oper... > > > File webkit/fileapi/file_system_operation.cc (right): > > > > > > > > > http://codereview.chromium.org/7671039/diff/1/webkit/fileapi/file_system_oper... > > > webkit/fileapi/file_system_operation.cc:185: > > > quota_util_helper_.reset(new ScopedQuotaUtilHelper( > > > On 2011/08/19 04:12:59, ericu wrote: > > >> > > >> This feels a bit odd here, given that many filesystems won't have > > > > > > quota issues. > > For filesystems we don't have quota issue GetQuotaUtil(type) will return NULL, > therefore we can safely skip unnecessary quota operations. > > > >> Perhaps in OFSFU and QFU both, as came up in the meeting on Wednesday? > > > > I basically agree with that, but I guess it has a trade-off issue. If > > > we do that both in FUs, we have to make sure that the inner pair of > > > ++/-- is sandwiched by the outer pair of ++/-- in some way. > > > > > > For example, if we do an operation in the following order: > > > 1) OFSFU's ++ > > > 2) OFSFU path name accounting > > > 3) OFSFU's -- > > > 4) QFU's ++ > > > 5) QFU file content accounting > > > 6) QFU's -- > > > > > the usage cache can have an inconsistent state. > > Agreed. We can avoid that by using a ScopedQuotaUtilHelper in both > > OFSFU and QFU. I expect that we can use the same class in both cases, > > just two different instances. > > Dai's diagram might be a bit delusive but what we want to achieve is: > > 1) dirty ++ > 2) DB change (possibly update usage too) > 3) Filesystem change + usage update > 4) dirty -- > > Please note that this proposal is independent from path name accounting at > OFSFU. > We do want to increment dirty before making DB changes regardless of whether we > do update usage at OFSFU, and that's why I think incrementing/decrementing at > upper layer would make more sense. > (Imagine instantiating ScopedQuotaUtilHelper while we do not update usage at > OFSFU-- that would look very weird) OK, I yield; we can put it in FileSystemOperation. > > > What do you think, Kinuko-san? > > > > > > > > > http://codereview.chromium.org/7671039/diff/1/webkit/fileapi/quota_file_util.cc > > > File webkit/fileapi/quota_file_util.cc (right): > > > > > > > > > http://codereview.chromium.org/7671039/diff/1/webkit/fileapi/quota_file_util.... > > > webkit/fileapi/quota_file_util.cc:28: class ScopedOriginUpdateHelper { > > > On 2011/08/19 04:12:59, ericu wrote: > > >> > > >> Does this class now not actually do anything "scoped"? > > > > > > Agreed. I'll fix it in the next patch. > > > > > > http://codereview.chromium.org/7671039/ > > >
I've renamed ScopedOriginUpdateHandler to OriginUpdateHandler. I guess it's ready for review. Could you please take a look at this change?
http://codereview.chromium.org/7671039/diff/10001/webkit/fileapi/file_system_... File webkit/fileapi/file_system_operation.h (right): http://codereview.chromium.org/7671039/diff/10001/webkit/fileapi/file_system_... webkit/fileapi/file_system_operation.h:96: class ScopedQuotaUtilHelper { I think we can just declare 'class ScopedQuotaUtilHelper' here and move all the class definition/impl to .cc? http://codereview.chromium.org/7671039/diff/10001/webkit/fileapi/quota_file_u... File webkit/fileapi/quota_file_util.cc (right): http://codereview.chromium.org/7671039/diff/10001/webkit/fileapi/quota_file_u... webkit/fileapi/quota_file_util.cc:110: OriginUpdateHelper helper( If all we do is calling NotifyUpdate() it looks like we can simply make the NotifyUpdate a simple function in an anonymous namespace and can get rid of the class entirely?
Thank you for the comments. http://codereview.chromium.org/7671039/diff/10001/webkit/fileapi/file_system_... File webkit/fileapi/file_system_operation.h (right): http://codereview.chromium.org/7671039/diff/10001/webkit/fileapi/file_system_... webkit/fileapi/file_system_operation.h:96: class ScopedQuotaUtilHelper { On 2011/08/25 09:12:00, kinuko wrote: > I think we can just declare 'class ScopedQuotaUtilHelper' here and move all the > class definition/impl to .cc? We have to pass this quota_util_helper from a function to its delayed function (e.g. CreateFile to DelayedCreateFileForQuota). In order to pass it, it should be a member variable of FileSystemOperation. It's the reason why this class is declared in the header file. http://codereview.chromium.org/7671039/diff/10001/webkit/fileapi/quota_file_u... File webkit/fileapi/quota_file_util.cc (right): http://codereview.chromium.org/7671039/diff/10001/webkit/fileapi/quota_file_u... webkit/fileapi/quota_file_util.cc:110: OriginUpdateHelper helper( On 2011/08/25 09:12:00, kinuko wrote: > If all we do is calling NotifyUpdate() it looks like we can simply make the > NotifyUpdate a simple function in an anonymous namespace and can get rid of the > class entirely? Ah, exactly. I'll modify it in the next change.
lgtm mod one more comment. http://codereview.chromium.org/7671039/diff/10001/webkit/fileapi/file_system_... File webkit/fileapi/file_system_operation.h (right): http://codereview.chromium.org/7671039/diff/10001/webkit/fileapi/file_system_... webkit/fileapi/file_system_operation.h:96: class ScopedQuotaUtilHelper { On 2011/08/25 09:19:46, Dai Mikurube wrote: > On 2011/08/25 09:12:00, kinuko wrote: > > I think we can just declare 'class ScopedQuotaUtilHelper' here and move all > the > > class definition/impl to .cc? > > We have to pass this quota_util_helper from a function to its delayed function > (e.g. CreateFile to DelayedCreateFileForQuota). In order to pass it, it should > be a member variable of FileSystemOperation. It's the reason why this class is > declared in the header file. If keeping it as a member var is the only concern we can do that while moving the definition to .cc: class FileSystemOperation { ... private class ScopedQuotaUtilHelper; scoped_ptr<ScopedQuotaUtilHelper> quota_util_helper_; }; in .cc: class FileSystemOperation::ScopedQuotaUtilHelper { public: ... }; (Your version definitely works and it's not a big issue, but it'd be preferable to hide unnecessary code in .cc and to keep .h files lightweight for build-time optimization)
Thank you for reviewing. I'll commit it if bots are happy. http://codereview.chromium.org/7671039/diff/10001/webkit/fileapi/file_system_... File webkit/fileapi/file_system_operation.h (right): http://codereview.chromium.org/7671039/diff/10001/webkit/fileapi/file_system_... webkit/fileapi/file_system_operation.h:96: class ScopedQuotaUtilHelper { On 2011/08/29 05:22:51, kinuko wrote: > On 2011/08/25 09:19:46, Dai Mikurube wrote: > > On 2011/08/25 09:12:00, kinuko wrote: > > > I think we can just declare 'class ScopedQuotaUtilHelper' here and move all > > the > > > class definition/impl to .cc? > > > > We have to pass this quota_util_helper from a function to its delayed function > > (e.g. CreateFile to DelayedCreateFileForQuota). In order to pass it, it > should > > be a member variable of FileSystemOperation. It's the reason why this class > is > > declared in the header file. > > If keeping it as a member var is the only concern we can do that while moving > the definition to .cc: > > class FileSystemOperation { > ... > private > class ScopedQuotaUtilHelper; > scoped_ptr<ScopedQuotaUtilHelper> quota_util_helper_; > }; > > in .cc: > > class FileSystemOperation::ScopedQuotaUtilHelper { > public: > ... > }; > > (Your version definitely works and it's not a big issue, but it'd be preferable > to hide unnecessary code in .cc and to keep .h files lightweight for build-time > optimization) Thanks for the information! Done it.
lgtm http://codereview.chromium.org/7671039/diff/16001/webkit/fileapi/file_system_... File webkit/fileapi/file_system_operation.cc (right): http://codereview.chromium.org/7671039/diff/16001/webkit/fileapi/file_system_... webkit/fileapi/file_system_operation.cc:28: explicit ScopedQuotaUtilHelper(FileSystemContext* context, no need to make this explicit?
Thanks. Updated! http://codereview.chromium.org/7671039/diff/16001/webkit/fileapi/file_system_... File webkit/fileapi/file_system_operation.cc (right): http://codereview.chromium.org/7671039/diff/16001/webkit/fileapi/file_system_... webkit/fileapi/file_system_operation.cc:28: explicit ScopedQuotaUtilHelper(FileSystemContext* context, On 2011/08/30 12:31:20, kinuko wrote: > no need to make this explicit? Done.
LGTM; sorry again for the slow response. http://codereview.chromium.org/7671039/diff/24001/webkit/fileapi/file_system_... File webkit/fileapi/file_system_operation.cc (right): http://codereview.chromium.org/7671039/diff/24001/webkit/fileapi/file_system_... webkit/fileapi/file_system_operation.cc:44: DCHECK(type != kFileSystemTypeUnknown); It would be safer to check that it's one of the known types.
Thank you for reviewing. As I wrote below, I think the pointed check can be handled in another change. I've checked the "Commit" box for this change. http://codereview.chromium.org/7671039/diff/24001/webkit/fileapi/file_system_... File webkit/fileapi/file_system_operation.cc (right): http://codereview.chromium.org/7671039/diff/24001/webkit/fileapi/file_system_... webkit/fileapi/file_system_operation.cc:44: DCHECK(type != kFileSystemTypeUnknown); On 2011/08/30 17:50:29, ericu wrote: > It would be safer to check that it's one of the known types. Thanks. I guess we want some other opinion on this. There're many DCHECKs like this. We can fix all of such DCHECKs in another change. What do you think, Kinuko-san?
http://codereview.chromium.org/7671039/diff/24001/webkit/fileapi/file_system_... File webkit/fileapi/file_system_operation.cc (right): http://codereview.chromium.org/7671039/diff/24001/webkit/fileapi/file_system_... webkit/fileapi/file_system_operation.cc:44: DCHECK(type != kFileSystemTypeUnknown); On 2011/08/31 07:12:02, Dai Mikurube wrote: > On 2011/08/30 17:50:29, ericu wrote: > > It would be safer to check that it's one of the known types. > > Thanks. I guess we want some other opinion on this. There're many DCHECKs like > this. We can fix all of such DCHECKs in another change. > > What do you think, Kinuko-san? Basically if it's one of the types that doesn't support quota GetQuotaUtil(type) should return NULL, so I think the check (type != Unknown) would be safe enough. Btw DCHECK_NE(type, kFileSystemTypeUnknown) would be a preferred way according to lint.
Change committed as 98942
On Wed, Aug 31, 2011 at 12:25 AM, <kinuko@chromium.org> wrote: > > http://codereview.chromium.org/7671039/diff/24001/webkit/fileapi/file_system_... > File webkit/fileapi/file_system_operation.cc (right): > > http://codereview.chromium.org/7671039/diff/24001/webkit/fileapi/file_system_... > webkit/fileapi/file_system_operation.cc:44: DCHECK(type != > kFileSystemTypeUnknown); > On 2011/08/31 07:12:02, Dai Mikurube wrote: >> >> On 2011/08/30 17:50:29, ericu wrote: >> > It would be safer to check that it's one of the known types. > >> Thanks. I guess we want some other opinion on this. There're many > > DCHECKs like >> >> this. We can fix all of such DCHECKs in another change. > >> What do you think, Kinuko-san? > > Basically if it's one of the types that doesn't support quota > GetQuotaUtil(type) should return NULL, so I think the check (type != > Unknown) would be safe enough. I was also thinking of the case in which the caller's forgotten to initialize type. We may have enough checks elsewhere that that's pretty unlikely, though. > Btw DCHECK_NE(type, kFileSystemTypeUnknown) would be a preferred way > according to lint. > > http://codereview.chromium.org/7671039/ > |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
