532323 - 4 GB limit of IMAP offline-store breaks sync of gmail "All Mail" and other systems(4GB limit is on Win, 2GB limit if Linux/Mac)

Status: RESOLVED FIXED

(MailNews Core :: Backend, defect)

Description

[Roland Tanglao needinfo please :rolandtanglao, :mohnkuchen, :adobo, :sinigang, :roland]

Many potential Thunderbird users have more than 4GB of email that needs to be synced. And with labels and allmail holding the mail it's very difficult/clumsy to move that email over to Thunderbird with the current 4GB limit. A true solution which may involve making the 4GB limit bigger is needed.

Comment 1

[Roland Tanglao needinfo please :rolandtanglao, :mohnkuchen, :adobo, :sinigang, :roland]

found in Get Satisfaction:
http://getsatisfaction.com/mozilla_messaging/topics/thunderbird_cant_handle_large_mailboxes?utm_content=topic_link&utm_medium=email&utm_source=new_topic

Comment 2

[Andrea Monni]

Should this marked as dependent on https://bugzilla.mozilla.org/show_bug.cgi?id=462665?

Comment 3

[rsx11m]

I think this is a duplicate of bug 462665, but leave it up to the others to decide on that. The underlying problem may be caused by the offline syncing; IMAP by itself shouldn't be subject to the 4GB limit as it deals only with message identifiers and not absolute message-file locations, but when >4GB are synced to the local disk, that may the point where things go wrong.

Disabling offline synchronization for All Mail may be a workaround, but then the message-body indexing of archived messages probably wouldn't work.

Comment 4

[David :Bienvenu]

It's not a duplicate. This can be fixed without fixing the pop3 mailbox limit, I believe.

Comment 5

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #3)
> but when >4GB are synced to the local disk, that may the point where things go wrong.

I've opened bug 537498 for next special case.
  - IMAP mail folder size is smaller than 4GB
  - Total synced data size exceeds 4GB before compact folder is executed
  (no need of 4GB mail in IMAP folder to test in this case)

Comment 6

[Wayne Mery (:wsmwk)]

(In reply to comment #4)
> It's not a duplicate. This can be fixed without fixing the pop3 mailbox limit,
> I believe.

bienevenu, please suggest whether this and bug 537498 should be "needed" for 1.9.1 as a stopgap for bug 462665.  It seems to me that both are needed, based on user reports, the increased potential for problems with large mail stores, and the workarounds don't seem nice from user POV. I nominated bug 537498 for 1.9.1 - but we were unsure in the phone discussion today whether one or both of these are needed.  Or, whether some other approach is warranted.

Comment 7

[Magritte]

I'm using TB 3.0 on Windows 7 64bit. I've run into issues with local folders larger than 4 GB and with uploading large amounts of messages to IMAP server, but on the other hand I've synced 6.14 GB All Mail folder from Gmail successfully and it appears to work correctly, even though local copies are saved in an mbox of 6.14 GB. No error messages or warnings were given. I'm unclear on whether this is expected behaviour or not and what are the consequences, especially in terms of potential data loss or corruption both locally and then synced back to to Gmail's IMAP server.

I think certainly if there is a real issue with syncing offline IMAP folders > 4 GB this should be fixed ASAP for the next release as there are so many Gmail users and this could potentially lead to data loss/corruption for many users. 

Frankly, I think the 4 GB limit on local folders should also be fixed ASAP as 64bit OSes and file systems with > 4GB file size limits are now common. Even if the 3 bugs referenced here are not actually interrelated, it seems to me they should all be fixed at the same time since for the majority of us who aren't familiar with the inner workings of Thunderbird, they would logically appear interrelated and cause unnecessary confusion and potential anxiety if the behaviour is different, for instance, for local folders vs local IMAP stores. If an error pops up regarding a local folder size limitation, it would be natural to assume the same limitation exists for an IMAP store, which is a serious issue for Gmail users in particular. While it is possible to prevent All Mail syncing as a workaround, this largely breaks Gmail's archiving model and therefore should be avoided.

Comment 8

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #7)
> I've run into issues with local folders larger than 4 GB (snip)
> Frankly, I think the 4 GB limit on local folders should also be fixed ASAP (snip)

This bug is for IMAP offline-store only.
Please read and post comment to Bug 462665 for local mailbox file issues.
Even if fixing of Bug 462665 shortly is very hard, fixing of this bug is possible, because IMAP offline_store !== local mbox file, although common logic is currently used. It's the reason why this bug is kept separately.

And, there is no need to state "why this bug is severe", "why this bug should be fixed," ... any more. We need to know here are;
- How this bug can be fixed, with minimum issues in compatibility with 
  older Tb.
- What kind of problems/phenomena occur by this bug.
- How to protect users from such problems,
  including patch which inhibits appending of larger than 4GB data,
  including workarounds by user.
As Tb3 already has "Disk Cache", next is an possible circumvention.
  - Disable offline use of [Gmail]/All Mail (auto-sync of this folder=off)
  - Set Disk Cache size greater than 6GB
Situation on [Gmail]/All Mail never becomes worse than Tb2 by this.

Comment 9

[WADA:World Anti-bad-Duping Agency]

Just an idea.

(A) Fixing this bug.
(1) A file per a mail as offline-store.
    (32 bits offset_1 + 32 bits offset_2) => (dir_# + UID)
    For Inbox: max_dir_#=M, max_file_#_per_dir=N
      Inbox.mdb, Inbox.mdb/mdir_1 to mdir_M,
      Inbox.mdb/mdir_X/<uid_of_mail>.eml
    Max M*N mails can be held in offline-store.
    Conceptually, no file size limitation by Tb.
    No compatibility of offline-store with old Tb versions.
    Per folder offlin-store type("unix mbox" or "mail dir") may be a solution
    of issues around compatibility with old Tb versions.
(2) 64bits integer for offset value.
    Per folder file size limitation.
    If smaller limit than 4GB(Win, 2GB for linux/mac), use current 32 bits
    integer as offset for compatibility. 
    If larger limit than 4GB(2GB), use 64 bits integer as offset.
       Note: if .msf is deleted by user, user should specify limit again.
             no compatibility of offline-store with old Tb versions.
I prefer (1), because "From line" issue won't occur.

(B) patches to protect users from current problems due to this bug.
    - patch for offline-store like Bug 387502 for local mbox file.

Comment 10

[David :Bienvenu]

We're going to do both 1 and 2, actually.

Comment 11

[David :Bienvenu]

Attachment: wip

this makes offline stores use a 64 bit offset. It is essentially untested, though everything seems to work with my existing offline stores. It is obviously not suitable for 3.0x since several interfaces had to change. Making it work for 3.0x is probably a non-starter for that reason. I think 3.0x is just going to have to not download to offline stores if the offline store is >= 4GB.

This patch actually gets us most of the way towards having local folders > 4GB, if we could live with the lack of backwards compatibility for mailboxes > 4GB. We'd have to remove the 4GB checks in 3.1, and do some more testing.

Speaking of testing, it's unclear to me if it's OK to make our xpcshell tests use > 4GB of disk space, but this area is really crying out for testing. I suppose we could pre-flight the tests with a check for available disk space.

Comment 12

[Andrew Sutherland [:asuth] (he/him)]

If we can leverage file-systems that leave 'holes' where you seek over parts of the file and do not write to them, we can probably test the 4 gig case without actually using up 4 gigs and the write burden that entails if we do things right.  I would presume this is the case for our primary platforms assuming we are using NTFS on windows.

Comment 13

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #12)

Andrew Sutherland, see bug 537498 for test of "offline-store>4GB, imap folder<4GB" case(emulation of repeated new mail/mail delete without compact).
On MS Win, big offline-store with big data of deleted mail is generated by;
  - open write append,
    write large dummy data,
    or seek to 4GB and write single byte(sparse file, NTFS only if MS Win),
    close
  - change write timestamp back to original

Comment 14

[WADA:World Anti-bad-Duping Agency]

When sparse file of NTFS of MS Win, disk spase was allocated by close(free disk space was reduced). So, Linux may be required to test with real sparse file.

Comment 15

[WADA:World Anti-bad-Duping Agency]

(In reply to comment #11)

> I think 3.0x is just going to have to not download to offline stores if the offline store is >= 4GB.
>(snip)
> We'd have to remove the 4GB checks in 3.1, and do some more testing.

Should we open separate bug for it for ease of tracking?
(Tb3.0.x only. Protect Tb 3.0.x user from problem like bug 537498, bug 538610)

Comment 16

[WADA:World Anti-bad-Duping Agency]

Adding IMAP and offline-store in bug summary to avoid confusion with Bug 462665 (for local mailbox file).

Comment 17

[David :Bienvenu]

Attachment: wip v2

This adds a unit test that passes, but doesn't seem to finish - do_test_finished() never seems to complete. It might be taking a very long time to clean out the user profile, because of the 4GB file. I can't really tell.

Comment 18

[David :Bienvenu]

Attachment: patch for review

This makes imap/news offline stores support files > 4GB. It doesn't do the same for local folders, though some of the work will help with that...

The unit test tests that we can store imap messages in offline stores > 4GB, and read them back out.

Comment 19

[[e-mail]]

Comment on attachment 421368 [details] [diff] [review]
patch for review

>     PRInt64 filePos;
>     seekableStream->Tell(&filePos);
>-    m_startOfNewMsg = (PRUint32) filePos;
>+    m_startOfNewMsg = filePos;
Would it be worth switching to PRInt64 for the message offset instead, so you avoid the mismatch with the file/network APIs? For instance, this becomes
seekableStream->Tell(&m_startOfNewMsg);

>-        smtpServer->GetTrySecAuth(&m_prefTrySecAuth);
>+        // Uncomment out the following line to turn back on sec auth probing
>+        // smtpServer->GetTrySecAuth(&m_prefTrySecAuth);
Part of another patch? ;-)

>+  // I believe 0xFFFFFFFFFFFFFFFF fits in 22 decimal digits, so 25 is plenty.
18446744073709551615 seems to be 20 digits ;-)

>         rv = NS_NewInputStreamPump(getter_AddRefs(pump), fileStream,
>-                                   nsInt64(offset), nsInt64(size));
>+                                   offset, nsInt64(size));
Can we change this to PRInt64 while we're here?

Comment 20

[David :Bienvenu]

Attachment: patch addressing Neil's comments

Comment 21

[[e-mail]]

(In reply to comment #19)
>>+  // I believe 0xFFFFFFFFFFFFFFFF fits in 22 decimal digits, so 25 is plenty.
>18446744073709551615 seems to be 20 digits ;-)
Wait, we're both wrong. We're using hex, so that's 16 digits!

Comment 22

[[e-mail]]

(In reply to comment #19)
>(From update of attachment 421368 [details] [diff] [review])
>>         rv = NS_NewInputStreamPump(getter_AddRefs(pump), fileStream,
>-                                   nsInt64(offset), nsInt64(size));
>>+                                   offset, nsInt64(size));
>Can we change this to PRInt64 while we're here?
I messed up here. I originally wrote these, then I changed it to this, forgetting that this isn't the only case of an nsInt64 - there's another NS_NewInputStreamPump later on in the patch.

>Would it be worth switching to PRInt64 for the message offset instead, so you
>avoid the mismatch with the file/network APIs? For instance, this becomes
>seekableStream->Tell(&m_startOfNewMsg);
And I confused you here too, rather than just "fixing" this one case I was thinking that it might be better not to mix PRUint64 and PRInt64 and instead stick to PRInt64 for all the message offsets in the whole codebase.

Comment 23

[David :Bienvenu]

(In reply to comment #22)
> >Would it be worth switching to PRInt64 for the message offset instead, so you
> >avoid the mismatch with the file/network APIs? For instance, this becomes
> >seekableStream->Tell(&m_startOfNewMsg);
> And I confused you here too, rather than just "fixing" this one case I was
> thinking that it might be better not to mix PRUint64 and PRInt64 and instead
> stick to PRInt64 for all the message offsets in the whole codebase.

Ah, I see - yes, now that we have 64 bits, we could use a signed value, but negative message offsets don't make sense, so I made it an unsigned value, and I'm inclined to keep it that way, despite the inconsistency with the tell() result.

Comment 24

[[e-mail]]

Comment on attachment 421461 [details] [diff] [review]
patch addressing Neil's comments

>+  PRInt64 m_localFileOffset;
...
>+  PRInt64 m_startOfNewMsg; // offset in mailbox of new message
In that case please change these back to PRUint64.

>-  nsInt64 curStorePos;
>+  PRInt64 curStorePos;
Actually you can eliminate this entirely and use tellPos throughout.
(Or call seekable->Tell(&curStorePos) instead if you prefer.)

>+      NS_ASSERTION(offset < 10000, "extremely unlikely status offset > 10000");
Doesn't an NS_ASSERTION already include the text of the condition?

>+  char  yarnBuf[100];
%llx = 16 chars + null terminator?

>   PRInt64 tellPos;
>   seekable->Tell(&tellPos);
>-  nsInt64 curStorePos = tellPos;
>-  destHdr->SetMessageOffset((PRUint32) curStorePos);
>+  PRInt64 curStorePos = tellPos;
>+  destHdr->SetMessageOffset(curStorePos);
[And again here.]

>         rv = NS_NewInputStreamPump(getter_AddRefs(pump),
>-                                   fileStream, nsInt64(offset), nsInt64(size));
>+                                   fileStream, offset, nsInt64(size));
(PRInt64)

Comment 25

[David :Bienvenu]

Attachment: addressing review comments

carrying forward standard8's sr

Comment 26

[David :Bienvenu]

(In reply to comment #24)
> (From update of attachment 421461 [details] [diff] [review])
> >+  PRInt64 m_localFileOffset;
> ...
> >+  PRInt64 m_startOfNewMsg; // offset in mailbox of new message
> In that case please change these back to PRUint64.
> 
I can't do that, and pass its address to Tell, so I've left it as is to save the local var and copy - we're still going to convert from PRInt64 to PRUint64 at some point no matter what...

I've addressed the other commments, though.

Comment 27

[[e-mail]]

Comment on attachment 421733 [details] [diff] [review]
addressing review comments

Ah, so that spot is the only place where we have the chance to Tell directly in to the variable. I don't think it's worth changing it to PRInt64 just for that, since it's then inconsistent with everywhere else. I'd also like m_localFileOffset to be PRUint64 to match the parameter. (I wonder whether there was a 2GB body search limit?) Finally test_largeOfflineStore.js contains a line with a trailing space.

Comment 29

[[e-mail]]

(In reply to comment #14)
> When sparse file of NTFS of MS Win, disk space was allocated by close (free disk
> space was reduced). So, Linux may be required to test with real sparse file.
You need to use the sparse file API to create a sparse file on Windows.

Comment 30

[David :Bienvenu]

fixed on trunk, with comments addressed - I still need to think what we can do for 3.02, i.e., w/o changing existing interfaces. I could define new interfaces implemented by nsMsgFolder and nsMsgHdr to add the 64 bit messageOffset methods, e.g., nsIMsgFolder_2 and nsIMsgHdr_2...or I could add code to 3.0x to turn off autosync and adding messages to offline store for folders >= 4GB. The latter would be a much smaller patch, but it would leave 3.0x users with giant imap folders without the ability to add newer messages to the offline store. 

Standard8, how likely are you to want a patch similar to the trunk one for 3.02?

Comment 31

[Rain [:rain1] (inactive)]

This is causing consistent failures on WINNT 5.2 comm-central check, presumably because the partition the tests are being run on is FAT32. e.g. <http://tinderbox.mozilla.org/showlog.cgi?log=Thunderbird/1264047995.1264051121.18060.gz#err1>.

gozer, I'm wondering if it would be possible to have xpcshell test runs on an NTFS partition, even if builds are still done on FAT32?

I'm still worried about skipping the test because there isn't enough free disk space -- it might just happen that we run out of space on the partition, and at some point after that we regress this.

Comment 32

[Mark Banner (:standard8)]

I just disabled the test on Windows: http://hg.mozilla.org/comm-central/rev/d093bd27df57

Whatever solution we come up with we should try and make a distinction between running it on FAT32 versus NTFS.

Comment 33

[David :Bienvenu]

Is it possible that GetDiskspaceAvailable doesn't work on FAT32? If so, then this test shouldn't run at all on FAT32, because we don't want to potentially consume all the disk space...

Comment 34

[Mark Banner (:standard8)]

(In reply to comment #33)
> Is it possible that GetDiskspaceAvailable doesn't work on FAT32? If so, then
> this test shouldn't run at all on FAT32, because we don't want to potentially
> consume all the disk space...

Well I'd have thought that GetDiskspaceAvailable would return all the disk space not just a 4GB limit which is what I believe is happening here. I did log into a box earlier and it had something like 10GB of disk space available and it had just failed the test.

If you can construct something for running in the error console, gozer or I can probably find a box to log into and run it to test.

Comment 35

[David :Bienvenu]

sid0 pointed out on IRC that FAT32 doesn't allow files > 4GB, so that's why it's failing.

Comment 36

[Mark Banner (:standard8)]

Not blocking 3.0.x on this as the patch seems a bit large and this isn't a regression, just an improvement - it also wouldn't get in until late April at the earliest at which point 3.1 should almost be out.

However, we would take a patch though that would handle the failure mode better though.