854791 - Free disk space size has to be checked before start Compact of Berkley Mbox file, to reduce "Disk Full while Compact" as many as possible

Status: RESOLVED FIXED

(MailNews Core :: Database, enhancement)

Description

[WADA:World Anti-bad-Duping Agency]

+++ This bug was initially created as a clone of Bug #794303 +++
This is spin-off of bug 794303 comment #36.

Free disk space size has to be checked before start Compact of Berkley Mbox file, to reduce "Disk Full while Compact" as many as possible.

I filled my VALLUABLE(only 9.5GB free!) HDD by 4GB, 2GB, 2GB files(each contains only "hello" at end, all other is 0x00 padded by MS Win). Free space is 1.5GB. Then deleted one 4MB mail at offset=0 of more than 4GB Tb 17.0.2's Inbox, and requested Tb to Do Compact. 
As expected, Tb started Compact blindly, and Compact failed with next dialog because of HDD full as expected.
> !
> The folder 'Inbox' could not be compacted because writing to folder failed.
> Verify that you have enough disk space, and that you have write privileges to the file system, then try again.
>    [ OK ]

Required size is obtained by simply caluculate summation of mesageSize.
> Assume msgFolder contains currently processed folder object.
> var required_size=0;
> try{
> var Enumerator=msgFolder.messages ; // if no messages, Enumerator may be undefined
> var cnt_max = 9999999999 ; // avoid infinite loop by my coding error
> for(var cnt=1; Enumerator && Enumerator.hasMoreElements() && cnt<cnt_max; cnt++)
> {
>    var msgDBHdr = Enumerator.getNext().QueryInterface(Components.interfaces.nsIMsgDBHdr) ;
>    if( true==msgDBHdr.IMAPDeleted ){ continue; }
>    required_size ++= = msgDBHdr.messageSize;
> }
> }catch(e){ alert("Compact Error"+"\n"+e); }
Current "m_totalMsgSize += msgSize; in EndCopy" is for;
(a) surely detect 4KB buffer write error during "copy a message". 
But it does do next at same time.
(b) count increase of size by adding X-Mozilla-Status:, X-Mozilla-Status2: and by adding/expanding X-Mozilla-Keys:, and by other size changes during Compact.
So, "m_totalMsgSize(==final SizeOnDisk value) - required_size" is usable to inform taken actions by Compact to Activity Manager.

Comment 1

[:aceman]

I'll try this.

Comment 2

[:aceman]

Attachment: patch

This should do it.
It needs UI review for the decision that the message is only shown for the first folder that can't be compacted. If there are more found later on, they are silently skipped. I didn't want the user to get a ton of messages if all the folders are uncompactable.

Comment 3

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8560917 [details] [diff] [review]
patch

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

I only really checked the string, because I have way too much disk space to test the error, but I'm mostly happy with it.  :)

ui-r=me with a tiny change as described below.

::: mail/locales/en-US/chrome/messenger/messenger.properties
@@ +82,5 @@
>  deletingMsgsFailed=Unable to delete messages in folder %S because it is in use by some other operation. Please wait for that operation to finish and then try again.
>  alertFilterCheckbox=Do not warn me again.
>  compactFolderDeniedLock=The folder '%S' cannot be compacted because another operation is in progress. Please try again later.
>  compactFolderWriteFailed=The folder '%S' could not be compacted because writing to folder failed. Verify that you have enough disk space, and that you have write privileges to the file system, then try again.
> +compactFolderInsufficientSpace=The folder '%S' cannot be compacted because there is not enough free disk space.

I think it would be nice to add another bit at the end which said something like "Please delete some files and try again." so that the user knows what they can do to fix the error.

::: suite/locales/en-US/chrome/mailnews/messenger.properties
@@ +79,5 @@
>  deletingMsgsFailed=Unable to delete messages in folder %S because it is in use by some other operation. Please wait for that operation to finish and then try again.
>  alertFilterCheckbox=Do not warn me again.
>  compactFolderDeniedLock=The folder '%S' cannot be compacted because another operation is in progress. Please try again later.
>  compactFolderWriteFailed=The folder '%S' could not be compacted because writing to folder failed. Verify that you have enough disk space, and that you have write privileges to the file system, then try again.
> +compactFolderInsufficientSpace=The folder '%S' cannot be compacted because there is not enough free disk space.

(And don't forget this string while you're changing the other one.  ;)

Comment 4

[Blake Winton (:bwinton) (:☕️)]

> It needs UI review for the decision that the message is only shown for the
> first folder that can't be compacted. If there are more found later on, they
> are silently skipped. I didn't want the user to get a ton of messages if all
> the folders are uncompactable.

Yes.  That's definitely the right thing to do.

If you wanted to be all fancy, you could list some of the folders that couldn't be compacted, but I probably wouldn't bother.

Oh, maybe change the string to "Some folders (including '%S') cannot be compacted…", so that people don't think that only one folder failed.

Comment 5

[:aceman]

I show the message as soon as the first folder is found so I don't know if any more folders will fail. We could probably collect all the failed folders and show the message at the end, but it would add some complexity. Not to mention localization problems on how to concatenate the folder names :)

Comment 6

[:aceman]

Attachment: 854791.patch v1.1

The catch is, even if the user does not delete any file, subsequent compact attempts may suddenly succeed (as folders that could be compacted in the first run have freed enough space:))

Comment 7

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8561051 [details] [diff] [review]
854791.patch v1.1

(You could have carried forward the ui-r=me, for the record.  ;)

Comment 8

[Kent James (:rkent)]

Comment on attachment 8561051 [details] [diff] [review]
854791.patch v1.1

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

To test this, I loaded the patch into a VM where I could easily add another, small disk, and checked that it worked as advertised.

Overall this looks great. I should look at it one more time after you incorporate the suggestion about estimating the size of the folder db in your needed space calculation.

::: mailnews/base/src/nsMsgFolderCompactor.cpp
@@ +228,5 @@
> +
> +   // Let's try to not even start compact if there is really low free space.
> +   // It may still fail later as we do not know how big the folder DB will
> +   // end up being.
> +   if (diskFree < diskSize - expunged)

Although you do not know how big exactly the folder DB is going to be, you can make a pretty good guess based on the number of messages in the folder.  In my system, the size of the db is approximately 0.75 kilobytes per message.

The calculation that you give here is guaranteed to fail for folders that will expand to near the available space available. You probably don't want to use up the very last byte of disk space anyway.

So what I would suggest is that you also allow 1 kilobyte for each message in the folder when you calculate the required disk space, as an approximation for the size requirement of the message db.

Comment 9

[:aceman]

Attachment: 854791.patch v1.2

OK, what about this? The new DB size should be at most equal to the current DB size.

Comment 10

[Kent James (:rkent)]

Comment on attachment 8564618 [details] [diff] [review]
854791.patch v1.2

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

r=me with requested changes

::: mailnews/base/src/nsMsgFolderCompactor.cpp
@@ +229,5 @@
> +   // It may still fail later as we do not know how big exactly the folder DB will
> +   // end up being.
> +   // The DB already doesn't contain references to messages that are already deleted.
> +   // So theoretically it shouldn't shrink with compact. But in practice,
> +   // The automatic shrinking of the DB may still have not yet happened.

nit: the automatic ...

::: mailnews/db/msgdb/public/nsIMsgDatabase.idl
@@ +255,5 @@
>    void resetHdrCacheSize(in unsigned long size);
>  
>    readonly attribute nsIDBFolderInfo  dBFolderInfo;
>  
> +  readonly attribute long long databaseSize;

You'll need a one line documentation of this, like:

/// Size of the database file in bytes.

::: mailnews/db/msgdb/src/nsMsgDatabase.cpp
@@ +591,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  nsCOMPtr<nsIFile> summaryFilePath;
> +  rv = msgStore->GetSummaryFile(m_folder, getter_AddRefs(summaryFilePath));
> +  NS_ENSURE_SUCCESS(rv, rv);

Rather than going through the store to get the file (which is backwards anyway since lower-level routines like db should avoid accessing higher-level routines whenever possible, plus this code will be obsoleted by another patch I'll be landing soon) just do this:
nsCOMPtr<nsIFile> summaryFilePath = do_CreateInstance(NS_LOCAL_FILE_CONTRACTID, &rv);
NS_ENSURE_SUCCESS(rv, rv);
rv = summaryFilePath->InitWithNativePath(m_dbName);
NS_ENSURE_SUCCESS(rv, rv);

Comment 11

[:aceman]

Attachment: 854791.patch v1.3

OK, that looks good.

Comment 12

[Philip Chee]

Comment on attachment 8565211 [details] [diff] [review]
854791.patch v1.3

a=Ratty for SeaMonkey CLOSED TREE

Comment 13

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Pushed as https://hg.mozilla.org/comm-central/rev/a85a4ebc9890