Closed Bug 666611 Opened 9 years ago Closed 9 years ago

Do not set chunksize if less than 500MiB of storage is available

Categories

(Toolkit :: Storage, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: taras.mozilla, Assigned: int3)

References

Details

Attachments

(1 file, 7 obsolete files)

This will avoid immediate problems with ill-chosen quotas
100MB is a bit of a guessed random number, a system with 150 or 200MB would not have less issues. I think it may make more sense use a 1GB limit.  Also, I'm not sure if our libs can correctly detect available storage on a disk managed with quota, this may be fixable though.
I guess if we may come with something more general to define a low-storage-device, like (total_storage<250GB || available_storage<1GB) and get rid of the mobile ifdefs, that fail on the Pre or other experimental mobile ports. Or even do that in addition.
The other way to do this would be to detect ENOSPACE during chunk allocation and back off, but that requires modifications to sqlite :)
Attached patch Patch v1 (obsolete) — Splinter Review
I'm not sure whether my error handling is done correctly. Feedback appreciated.
Attachment #544948 - Flags: feedback?(tglek)
Comment on attachment 544948 [details] [diff] [review]
Patch v1

Does GetDiskSpaceAvailable do synchronous disk I/O to figure out how much space is available?  If so, we need to do that check on a different thread.
Comment on attachment 544948 [details] [diff] [review]
Patch v1

Thanks! This looks good to me. Need to update docs for SetGrowthIncrement to mention this
Attachment #544948 - Flags: review?(mak77)
Attachment #544948 - Flags: feedback?(tglek)
Attachment #544948 - Flags: feedback+
(In reply to comment #4)
> Comment on attachment 544948 [details] [diff] [review] [review]
> Patch v1
> 
> Does GetDiskSpaceAvailable do synchronous disk I/O to figure out how much
> space is available?  If so, we need to do that check on a different thread.

It doesn't.
(In reply to comment #6)
> It doesn't.
What does it do to establish that information?
(In reply to comment #7)
> (In reply to comment #6)
> > It doesn't.
> What does it do to establish that information?

Because in the worst case it does a stat() on a file that is already cached.
Attached patch Patch v2 (obsolete) — Splinter Review
Update patch to fix docs for SetGrowthIncrement as well.
Attachment #544948 - Attachment is obsolete: true
Attachment #545333 - Flags: review?(mak77)
Attachment #544948 - Flags: review?(mak77)
Comment on attachment 545333 [details] [diff] [review]
Patch v2

Review of attachment 545333 [details] [diff] [review]:
-----------------------------------------------------------------

Considering all of this, it looks good enough. I have some comment though.

::: storage/public/mozIStorageConnection.idl
@@ +362,5 @@
>    /**
>     * Controls SQLITE_FCNTL_CHUNK_SIZE setting in sqlite. This helps avoid fragmentation
> +   * by growing/shrinking the database file in SQLITE_FCNTL_CHUNK_SIZE increments. To
> +   * conserve memory on systems short on storage space, this function will have no effect
> +   * if less than 100MB of space is left available.

, or on mobile devices.

@@ +367,3 @@
>     *
>     * @param aIncrement
>     *        The database file will grow in multiples of chunkSize.

the idl should specify that this @throws NS_ERROR_FILE_TOO_BIG if the system is short on storage space.

::: storage/src/mozStorageConnection.cpp
@@ +1250,4 @@
>    // so don't preallocate space. This is also not effective
>    // on log structured file systems used by Android devices
>  #if !defined(ANDROID) && !defined(MOZ_PLATFORM_MAEMO)
> +  // Don't preallocate if less than 100MB is available

nit: end comment with a period please, also this is 100MiB. We don't expose new units to users, but internal comments may use them.

@@ +1252,5 @@
>  #if !defined(ANDROID) && !defined(MOZ_PLATFORM_MAEMO)
> +  // Don't preallocate if less than 100MB is available
> +  nsresult rv;
> +  nsCOMPtr<nsILocalFile> localFile = do_QueryInterface(mDatabaseFile, &rv);
> +  NS_ENSURE_SUCCESS(rv, rv);

let's just NS_ENSURE_STATE(localFile); and declare rv later on first use

@@ +1254,5 @@
> +  nsresult rv;
> +  nsCOMPtr<nsILocalFile> localFile = do_QueryInterface(mDatabaseFile, &rv);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  PRInt64 bytesAvailable;
> +  rv = localFile->GetDiskSpaceAvailable(&bytesAvailable);

My main question is how good is this method to correctly detect the available disk space.

on Windows it uses GetDiskFreeSpaceExW, that seems good enough taking into account bytes available to the specific user
on OS2 it uses DosQueryFSInfo, looks like it's rather returning global free space on disk, but I've not found a lot of documentation. Doesn't seem good enough, but it's not a tier1 platform so we may stick to what we had for now.
on Unix it uses statfs, I've not found good docs saying f_bavail takes in count quota, but statfs does, so I suppose it does.
on Linux there is a specific code for quotactl too, that's nice.

looks like it should work on the most common cases, I suppose some network fs may fail, but we have bigger issues there usually.

@@ +1257,5 @@
> +  PRInt64 bytesAvailable;
> +  rv = localFile->GetDiskSpaceAvailable(&bytesAvailable);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  PRInt64 MBAvail = bytesAvailable / (1024 * 1024);
> +  if (MBAvail < 100)

I'd prefer if you
#define BYTES_PER_MEBIBYTE 1048576
and
if (bytesAvailable < 100 * BYTES_PER_MEBIBYTE) {

@@ +1258,5 @@
> +  rv = localFile->GetDiskSpaceAvailable(&bytesAvailable);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  PRInt64 MBAvail = bytesAvailable / (1024 * 1024);
> +  if (MBAvail < 100)
> +    return NS_ERROR_FILE_TOO_BIG;

also please brace this if
Attachment #545333 - Flags: review?(mak77) → review-
I suppose there is no easy way to make a test here, since it may be hard to sym a low disk space situation. Ideas?
(In reply to comment #11)
> I suppose there is no easy way to make a test here, since it may be hard to
> sym a low disk space situation. Ideas?

Short of simulating it via a dummy pref, observer, etc there isn't much we can do.
Attached patch Patch v3 (obsolete) — Splinter Review
Patch with review comments addressed.

I'm not sure how good the method of detecting disk space is, but it's the same one being used by the network cache code to determine the default cache size.
Assignee: nobody → jezreel
Attachment #545333 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #547360 - Flags: review?(mak77)
Comment on attachment 547360 [details] [diff] [review]
Patch v3

Review of attachment 547360 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with these small glitches fixed

::: storage/public/mozIStorageConnection.idl
@@ +367,4 @@
>     *
>     * @param aIncrement
>     *        The database file will grow in multiples of chunkSize.
> +   *        @throws NS_ERROR_FILE_TOO_BIG if the system is short on storage space.

ehr, maybe I was unclear (sorry), this should be after all params and at the same level, as in any other javadoc declaration.

@param
@param
@return
@throws
@note
...

::: storage/src/mozStorageConnection.cpp
@@ +1250,5 @@
> +  nsCOMPtr<nsILocalFile> localFile = do_QueryInterface(mDatabaseFile);
> +  NS_ENSURE_STATE(localFile);
> +  PRInt64 bytesAvailable;
> +  nsresult rv;
> +  rv = localFile->GetDiskSpaceAvailable(&bytesAvailable);

assign on declaration
nsresult rv = localFile...

@@ +1252,5 @@
> +  PRInt64 bytesAvailable;
> +  nsresult rv;
> +  rv = localFile->GetDiskSpaceAvailable(&bytesAvailable);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +# define BYTES_PER_MEBIBYTE 1048576

this define should be at the top of the file after includes, as any other define, no space between # and define then
Attachment #547360 - Flags: review?(mak77) → review+
Attached patch Patch v4 (obsolete) — Splinter Review
Patch with comments addressed. Sorry about the silly mistakes in the last patch, shouldn't have dashed it off so quickly..
Attachment #547360 - Attachment is obsolete: true
Keywords: checkin-needed
Keywords: checkin-needed
Whiteboard: [inbound]
Didn't compile, backed out.
Whiteboard: [inbound]
Strange, the compile failure is Windows only, other platforms did compile fine. I guess if it's the include (regardless you should probably include nsILocalFile.h and not nsLocalFile.h)
Duplicate of this bug: 674514
Attached patch Patch v5 (obsolete) — Splinter Review
I guess it was a problem with the nsILocalFile.h. I modified the patch to add that and all platforms compiled cleanly on Try [1], though I did run into one intermittent orange on Linux (bug 673017).

[1]: http://tbpl.mozilla.org/?tree=Try&rev=ff95b14e57c7
Attachment #547533 - Attachment is obsolete: true
Keywords: checkin-needed
(In reply to comment #20)
> I guess it was a problem with the nsILocalFile.h. I modified the patch to
> add that and all platforms compiled cleanly on Try [1], though I did run

that's a known orange, it's fine
Keywords: checkin-needed
Whiteboard: [inbound]
I had to backout due to failure on windows xpcshell.
Looks like GetDiskSpaceAvailable there returns something smaller than 100MB, and tests\storage\test\unit\test_chunk_growth.js throws
I don't know if windows tinderboxes have some sort of quota enabled or a broken controller, something is wrong though.
Whiteboard: [inbound]
Depends on: 676521
I filed bug 676521 to ask releng if they have any idea why Windows reports such small space.
regardless, to avoid random failures, we should probably catch NS_ERROR_FILE_TOO_BIG in the test, info() that and bailout, since a slave may run out of space.
So, for some reason on windows GetDiskSpaceAvailable works only on folders, if you use it on a nsILocalFile representing a file, GetDiskFreeSpaceExW fails.
If you change this code to che on the profile folder rather than the database file it will work.
We should then file a bug to fix GetDiskSpaceAvailable
Depends on: 681660
and rethinking about that, it's better to fix the underlying bug.
I've attached patch to fix the nsILocalFile problem, could you please update the patch to address comment 25 (update the test for SetGrowthIncrement), so we don't random fail if a slave runs out of space.
As an additional thing, I'm starting considering we may enlarge a bit the value, anything below 500MB today is pretty low in space.  What do you think Taras?
(In reply to Marco Bonardo [:mak] from comment #29)
> I've attached patch to fix the nsILocalFile problem, could you please update
> the patch to address comment 25 (update the test for SetGrowthIncrement), so
> we don't random fail if a slave runs out of space.
> As an additional thing, I'm starting considering we may enlarge a bit the
> value, anything below 500MB today is pretty low in space.  What do you think
> Taras?

Lets do 100mb see if people complain. Since chunk_size doesn't persist when free space shrinks to 100mb, sqlite will disable chunk_size trim itself on the next restart.
I don't think it will trim without a vacuum, that may happen up to a month later.
(In reply to Marco Bonardo [:mak] from comment #31)
> I don't think it will trim without a vacuum, that may happen up to a month
> later.

I seem to recall it trimming during normal operations. If you want to be conservative, 500mb is fine.
Attached patch Patch v6 (obsolete) — Splinter Review
Updated patch as per comment 25. I've left the threshold at 100MB, let me know if you decide to go with 500MB.

Just a thought -- wouldn't wrapping the setGrowthIncrement in a try..catch block hide potentially real errors such as the one we discovered with the GetDiskSpaceAvailable implementation?
Attachment #550175 - Attachment is obsolete: true
Attachment #555635 - Flags: review?(mak77)
(In reply to Jez Ng [:int3] from comment #33)
> Just a thought -- wouldn't wrapping the setGrowthIncrement in a try..catch
> block hide potentially real errors such as the one we discovered with the
> GetDiskSpaceAvailable implementation?

Yes it will cover GDSA errors, but GDSA is getting its own test and is xpcom task to test it, not ours.  Unfortunately I can't think of a way to avoid random oranges due to real low disk space, unless we use GDSA to check first, and that would still cover any possible issue.
Comment on attachment 555635 [details] [diff] [review]
Patch v6

Review of attachment 555635 [details] [diff] [review]:
-----------------------------------------------------------------

yes, I prefer it being some larger, let's go for 500. Remember to update the idl comment too.

::: storage/test/unit/test_chunk_growth.js
@@ +28,5 @@
>    var d = getDatabase(new_file(filename));
> +  try {
> +    d.setGrowthIncrement(CHUNK_SIZE, "");
> +  } catch (e if e.result == Cr.NS_ERROR_FILE_TOO_BIG) {
> +    ok(true, "Too little free space to set CHUNK_SIZE!");

ehr, wait this is an xpcshell test, it has no ok(), you probably just want a print(msg); and a do_check_true(true); (so there is at least 1 test, even if fake)

btw, if this would have been a b-c test, you should have used info(msg)
Attachment #555635 - Flags: review?(mak77)
Summary: Do not set chunksize if less than 100MB of storage is available → Do not set chunksize if less than 500MiB of storage is available
Attached patch Patch v7 (obsolete) — Splinter Review
Fixed the ok() and changed 100MiB to 500MiB.

I tried running the test without an additional do_check_true, and unlike mochitests it seems to pass even when no checks are done, merely emitting an info message to say that no checks were found. So I've left out that extra check.
Attachment #555635 - Attachment is obsolete: true
Attachment #556050 - Flags: review?(mak77)
Comment on attachment 556050 [details] [diff] [review]
Patch v7

Review of attachment 556050 [details] [diff] [review]:
-----------------------------------------------------------------

to me looks fine,

::: storage/src/mozStorageConnection.cpp
@@ +69,5 @@
>  
>  #include "prlog.h"
>  #include "prprf.h"
>  
> +#define BYTES_PER_MEBIBYTE 1048576

sorry, I think I changed my mind, I know you are about to kill me, but this won't preclude a positive review before that.

make this:
#define MIN_AVAILABLE_BYTES_PER_CHUNKED_GROWTH 524288000 // 500MiB

and directly use that in the if, should be crystal clear then.
Attachment #556050 - Flags: review?(mak77) → review+
Attached patch Patch v8Splinter Review
Attachment #556050 - Attachment is obsolete: true
Keywords: checkin-needed
In my queue :-)
https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=feaf94ae787f
Keywords: checkin-needed
OS: Linux → All
Hardware: x86 → All
Version: unspecified → Trunk
Attempt 2 (one of the other checkin-neededs broke that try push):
https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=189de6fe4c76
http://hg.mozilla.org/try/rev/08c883d1baa2
Target Milestone: --- → mozilla9
http://hg.mozilla.org/mozilla-central/rev/89d9b45ef241
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.