Closed
Bug 532323
Opened 15 years ago
Closed 15 years ago
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)
Categories
(MailNews Core :: Backend, defect)
MailNews Core
Backend
Tracking
(blocking-thunderbird3.0 -, thunderbird3.0 wanted)
RESOLVED
FIXED
Thunderbird 3.1a1
People
(Reporter: rolandtanglao, Assigned: Bienvenu)
References
()
Details
(Whiteboard: [for 3.0.x would prefer a patch for a better failure mode][gs][gssolved])
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
neil
:
review+
Bienvenu
:
superreview+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Updated•15 years ago
|
Assignee: nobody → bienvenu
Reporter | ||
Comment 1•15 years ago
|
||
Updated•15 years ago
|
Assignee: bienvenu → nobody
Component: General → Backend
Product: Thunderbird → MailNews Core
QA Contact: general → backend
Version: 3.0 → Trunk
Comment 2•15 years ago
|
||
Should this marked as dependent on https://bugzilla.mozilla.org/show_bug.cgi?id=462665?
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.
Assignee | ||
Comment 4•15 years ago
|
||
It's not a duplicate. This can be fixed without fixing the pop3 mailbox limit, I believe.
Comment 5•15 years ago
|
||
(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•15 years ago
|
||
(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.
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•15 years ago
|
||
(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•15 years ago
|
||
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.
Severity: normal → major
OS: Mac OS X → All
Hardware: x86 → All
Assignee | ||
Comment 10•15 years ago
|
||
We're going to do both 1 and 2, actually.
Assignee | ||
Comment 11•15 years ago
|
||
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.
Assignee: nobody → bienvenu
Comment 12•15 years ago
|
||
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•15 years ago
|
||
(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•15 years ago
|
||
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•15 years ago
|
||
(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•15 years ago
|
||
Adding IMAP and offline-store in bug summary to avoid confusion with Bug 462665 (for local mailbox file).
Summary: 4 GB limit breaks sync of gmail "All Mail" and other systems → 4 GB limit of IMAP offline-store breaks sync of gmail "All Mail" and other systems
Assignee | ||
Comment 17•15 years ago
|
||
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.
Attachment #421140 -
Attachment is obsolete: true
Assignee | ||
Comment 18•15 years ago
|
||
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.
Attachment #421332 -
Attachment is obsolete: true
Attachment #421368 -
Flags: superreview?(neil)
Attachment #421368 -
Flags: review?(bugzilla)
Comment 19•15 years ago
|
||
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?
Assignee | ||
Comment 20•15 years ago
|
||
Attachment #421368 -
Attachment is obsolete: true
Attachment #421461 -
Flags: superreview?(neil)
Attachment #421461 -
Flags: review?(bugzilla)
Attachment #421368 -
Flags: superreview?(neil)
Attachment #421368 -
Flags: review?(bugzilla)
Comment 21•15 years ago
|
||
(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•15 years ago
|
||
(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.
Assignee | ||
Comment 23•15 years ago
|
||
(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•15 years ago
|
||
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)
Attachment #421461 -
Flags: superreview?(neil) → superreview+
Assignee | ||
Comment 25•15 years ago
|
||
carrying forward standard8's sr
Attachment #421461 -
Attachment is obsolete: true
Attachment #421733 -
Flags: superreview+
Attachment #421733 -
Flags: review?(neil)
Attachment #421461 -
Flags: review?(bugzilla)
Assignee | ||
Comment 26•15 years ago
|
||
(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•15 years ago
|
||
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.
Attachment #421733 -
Flags: review?(neil) → review+
Updated•15 years ago
|
blocking-thunderbird3.0: --- → ?
Flags: blocking-thunderbird3.1+
Comment 29•15 years ago
|
||
(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.
Assignee | ||
Comment 30•15 years ago
|
||
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?
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Comment 31•15 years ago
|
||
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•15 years ago
|
||
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.
Assignee | ||
Comment 33•15 years ago
|
||
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•15 years ago
|
||
(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.
Assignee | ||
Comment 35•15 years ago
|
||
sid0 pointed out on IRC that FAT32 doesn't allow files > 4GB, so that's why it's failing.
Updated•15 years ago
|
Summary: 4 GB limit of IMAP offline-store breaks sync of gmail "All Mail" and other systems → 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)
Comment 36•15 years ago
|
||
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.
blocking-thunderbird3.0: ? → -
status-thunderbird3.0:
--- → wanted
Whiteboard: [for 3.0.x would prefer a patch for a better failure mode]
Updated•15 years ago
|
Blocks: tb-enterprise
Updated•15 years ago
|
No longer blocks: tb-enterprise
Updated•14 years ago
|
Whiteboard: [for 3.0.x would prefer a patch for a better failure mode] → [for 3.0.x would prefer a patch for a better failure mode][gs][gssolved]
Updated•14 years ago
|
Target Milestone: --- → Thunderbird 3.1a1
You need to log in
before you can comment on or make changes to this bug.
Description
•