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

Status: RESOLVED FIXED

(Core :: SQLite and Embedded Database Bindings, defect)

Description

[(dormant account)]

This will avoid immediate problems with ill-chosen quotas

Comment 1

[Marco Bonardo [:mak]]

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.

Comment 2

[(dormant account)]

The other way to do this would be to detect ENOSPACE during chunk allocation and back off, but that requires modifications to sqlite :)

Comment 3

[Jez Ng [:int3]]

Attachment: Patch v1

I'm not sure whether my error handling is done correctly. Feedback appreciated.

Comment 4

[Shawn Wilsher :sdwilsh]

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 5

[(dormant account)]

Comment on attachment 544948 [details] [diff] [review]
Patch v1

Thanks! This looks good to me. Need to update docs for SetGrowthIncrement to mention this

Comment 6

[(dormant account)]

(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.

Comment 7

[Shawn Wilsher :sdwilsh]

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

Comment 8

[(dormant account)]

(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.

Comment 9

[Jez Ng [:int3]]

Attachment: Patch v2

Update patch to fix docs for SetGrowthIncrement as well.

Comment 10

[Marco Bonardo [:mak]]

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

Comment 11

[Marco Bonardo [:mak]]

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?

Comment 12

[(dormant account)]

(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.

Comment 13

[Jez Ng [:int3]]

Attachment: Patch v3

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.

Comment 14

[Marco Bonardo [:mak]]

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

Comment 15

[Jez Ng [:int3]]

Attachment: Patch v4

Patch with comments addressed. Sorry about the silly mistakes in the last patch, shouldn't have dashed it off so quickly..

Comment 16

[Dão Gottwald [:dao]]

Didn't compile, backed out.

Comment 17

[Dão Gottwald [:dao]]

http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla-Inbound/1311456465.1311457000.17420.gz

Comment 18

[Marco Bonardo [:mak]]

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)

Comment 20

[Jez Ng [:int3]]

Attachment: Patch v5

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

Comment 21

[Marco Bonardo [:mak]]

(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

Comment 22

[Marco Bonardo [:mak]]

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.

Comment 23

[Marco Bonardo [:mak]]

for reference
http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla-Inbound/1312455235.1312458442.11624.gz
http://tinderbox.mozilla.org/showlog.cgi?log=Mozilla-Inbound/1312455237.1312457870.8645.gz

Comment 24

[Marco Bonardo [:mak]]

I filed bug 676521 to ask releng if they have any idea why Windows reports such small space.

Comment 25

[Marco Bonardo [:mak]]

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.

Comment 26

[Marco Bonardo [:mak]]

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

Comment 27

[Marco Bonardo [:mak]]

filed bug 681660

Comment 28

[Marco Bonardo [:mak]]

and rethinking about that, it's better to fix the underlying bug.

Comment 29

[Marco Bonardo [:mak]]

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?

Comment 30

[(dormant account)]

(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.

Comment 31

[Marco Bonardo [:mak]]

I don't think it will trim without a vacuum, that may happen up to a month later.

Comment 32

[(dormant account)]

(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.

Comment 33

[Jez Ng [:int3]]

Attachment: Patch v6

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?

Comment 34

[Marco Bonardo [:mak]]

(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 35

[Marco Bonardo [:mak]]

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)

Comment 36

[Jez Ng [:int3]]

Attachment: Patch v7

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.

Comment 37

[Marco Bonardo [:mak]]

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.

Comment 38

[Jez Ng [:int3]]

Attachment: Patch v8

Comment 39

[Ed Morley [:emorley]]

In my queue :-)
https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=feaf94ae787f

Comment 40

[Ed Morley [:emorley]]

Attempt 2 (one of the other checkin-neededs broke that try push):
https://tbpl.mozilla.org/?usebuildbot=1&tree=Try&rev=189de6fe4c76

Comment 41

[Ed Morley [:emorley]]

http://hg.mozilla.org/try/rev/08c883d1baa2

Comment 42

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/89d9b45ef241