statuscache and fatal
Hello,
I wonder why is a fatal function called when the statuscache backend
can't be opened. This makes the process terminate (then fork again,
then terminate, then...). This is not logical, because statuscache
is... a cache ! Cyrus should work fine without (and actually, it does,
with the patch attached).
As we use an sql backend for statuscache, when the sql server is not
available, we prefer that cyrus-imapd works without cache than a total
breakdown.
--
Cyril
--- imap/statuscache_db.c 30 Mar 2009 07:52:37 - 1.1.1.2
+++ imap/statuscache_db.c 4 Nov 2009 10:17:11 -
@@ -94,7 +94,7 @@
if (ret != 0) {
syslog(LOG_ERR, DBERROR: opening %s: %s, fname,
cyrusdb_strerror(ret));
+ syslog(LOG_ERR, statuscache in degraded mode);
- fatal(can't read statuscache file, EC_TEMPFAIL);
}
if (tofree) free(tofree);
statuscache
Looking (again) at integrating this. Two observations, the first somewhat minor, and the second somewhat major: - statuscache v.2 doesn't have support for HIGHESTMODSEQ. This is trivial to add to a v.3. - statuscache v.2 stores statuscache_data as a binary blob, which is platform dependent (byte-order word size). This make statuscache.db non-portable. I believe that this might be the first cyrusdb that would be non-portable. Does anybody care? -- Kenneth Murchison Systems Programmer Project Cyrus Developer/Maintainer Carnegie Mellon University
Re: statuscache
Bron Gondwana wrote: On Thu, Jan 17, 2008 at 02:47:27PM -0500, Ken Murchison wrote: Looking (again) at integrating this. Two observations, the first somewhat minor, and the second somewhat major: - statuscache v.2 doesn't have support for HIGHESTMODSEQ. This is trivial to add to a v.3. Yeah, shouldn't be a big problem. - statuscache v.2 stores statuscache_data as a binary blob, which is platform dependent (byte-order word size). This make statuscache.db non-portable. I believe that this might be the first cyrusdb that would be non-portable. Does anybody care? Hmm - it would be a very minor cost to make it platform independent. On the flip side, it's not supposed to persist over shutdowns of Cyrus anyway. Still, I would vote for fixing that too - might as well keep the platform safeness. Processor time is cheap compared to disk IO at least in our setup. The only tricky part is going to be upgrading from v.2 to v.3. If its not designed to be persistent and/or migrated between platforms, I'm not sure I care, since your design is definitely quicker. I'm in the process of refactoring your patch a little, and I'm going to leave the storage method alone for now. I'll send you my patch when complete. I'll have it done later tonight, after I make dinner for my kids. BTW, is statuscache:log necessary, or is it just for debugging? -- Kenneth Murchison Systems Programmer Project Cyrus Developer/Maintainer Carnegie Mellon University
Re: statuscache
- statuscache v.2 stores statuscache_data as a binary blob, which is platform dependent (byte-order word size). This make statuscache.db non-portable. I believe that this might be the first cyrusdb that would be non-portable. Does anybody care? The only tricky part is going to be upgrading from v.2 to v.3. If its not designed to be persistent and/or migrated between platforms, I'm not sure I care, since your design is definitely quicker. Basically our startup scripts completely delete the statuscache.db file on startup, so migrating isn't an issue for us at all. So if you want to make it platform independent for others in the future, I'm fine with that. BTW, is statuscache:log necessary, or is it just for debugging? It's not necessary, it was just helpful for debugging rather than having to recompile everything. From memory, clients make SO many status calls, that it can completely flood the log if you leave the syslog calls in. Rob
Re: statuscache
Rob Mueller wrote: Basically our startup scripts completely delete the statuscache.db file on startup, so migrating isn't an issue for us at all. Why do you delete the cache? I doesn't appear from the code that the cache entries would be invalid if the mailbox and seen state aren't touched. BTW, is statuscache:log necessary, or is it just for debugging? It's not necessary, it was just helpful for debugging rather than having to recompile everything. From memory, clients make SO many status calls, that it can completely flood the log if you leave the syslog calls in. I'm changing the statuscache option to a switch and leaving the logging in as LOG_DEBUG -- Kenneth Murchison Systems Programmer Project Cyrus Developer/Maintainer Carnegie Mellon University
Re: statuscache
Why do you delete the cache? I doesn't appear from the code that the cache entries would be invalid if the mailbox and seen state aren't touched. Historical. We only use berkeley db for statuscache and duplicate delivery, and because of problems in the past with berkeley db's corrupting themselves, we found it easier to just delete them on startup. It avoids the case of cyrus failing to start if one of the bdb's is corrupted I'm changing the statuscache option to a switch and leaving the logging in as LOG_DEBUG My only concern with this is that it can cause a LOT of log messages. I've never been entirely sure how well syslog handles the case of lots of log messages. I think it still has to send them over the socket to the syslogd. Most people then have a syslog.conf that discards debug messages, so it always seemed like a possible way to overflow the syslog socket buffer and loose important messages (i'm pretty sure syslog never guarantees that a message will be logged, which is why djb implemented his own way that uses streams rather than dgrams). This is probably all entirely academic and not a real issue... Rob
Re: statuscache
)) {
+ statuscache_close();
+ statuscache_done();
+}
+
if (imapd_in) {
/* Flush the incoming buffer */
prot_NONBLOCK(imapd_in);
@@ -6747,21 +6757,19 @@
void cmd_status(char *tag, char *name)
{
int c;
-int statusitems = 0;
+unsigned statusitems = 0;
static struct buf arg;
-struct mailbox mailbox;
char mailboxname[MAX_MAILBOX_NAME+1];
int mbtype;
-char *server;
+char *server, *acl;
int r = 0;
-int doclose = 0;
r = (*imapd_namespace.mboxname_tointernal)(imapd_namespace, name,
imapd_userid, mailboxname);
if (!r) {
r = mlookup(tag, name, mailboxname, mbtype, NULL, NULL,
- server, NULL, NULL);
+ server, acl, NULL);
}
if (r == IMAP_MAILBOX_MOVED) {
/* Eat the argument */
@@ -6864,24 +6872,18 @@
}
if (!r) {
- r = mailbox_open_header(mailboxname, imapd_authstate, mailbox);
-}
+ int myrights = cyrus_acl_myrights(imapd_authstate, acl);
-if (!r) {
- doclose = 1;
- r = mailbox_open_index(mailbox);
-}
-if (!r !(mailbox.myrights ACL_READ)) {
- r = (imapd_userisadmin || (mailbox.myrights ACL_LOOKUP)) ?
- IMAP_PERMISSION_DENIED : IMAP_MAILBOX_NONEXISTENT;
+ if (!(myrights ACL_READ)) {
+ r = (imapd_userisadmin || (myrights ACL_LOOKUP)) ?
+ IMAP_PERMISSION_DENIED : IMAP_MAILBOX_NONEXISTENT;
+ }
}
if (!r) {
- r = index_status(mailbox, name, statusitems);
+ r = index_status(mailboxname, name, statusitems);
}
-if (doclose) mailbox_close(mailbox);
-
if (r) {
prot_printf(imapd_out, %s NO %s\r\n, tag, error_message(r));
return;
Index: imap/imapd.h
===
RCS file: /afs/andrew/system/cvs/src/cyrus/imap/imapd.h,v
retrieving revision 1.69
diff -u -r1.69 imapd.h
--- imap/imapd.h 17 Jan 2008 13:07:40 - 1.69
+++ imap/imapd.h 18 Jan 2008 04:16:12 -
@@ -272,8 +272,7 @@
struct searchargs *searchargs, int usinguid);
extern int index_copy(struct mailbox *mailbox, char *sequence,
int usinguid, char *name, char **copyuidp, int nolink);
-extern int index_status(struct mailbox *mailbox, char *name,
- int statusitems);
+extern int index_status(char *mboxname, char *name, unsigned statusitems);
extern int index_getuids(struct mailbox *mailbox, unsigned lowuid);
extern int index_getstate(struct mailbox *mailbox);
Index: imap/index.c
===
RCS file: /afs/andrew/system/cvs/src/cyrus/imap/index.c,v
retrieving revision 1.241
diff -u -r1.241 index.c
--- imap/index.c 17 Jan 2008 13:07:40 - 1.241
+++ imap/index.c 18 Jan 2008 04:16:12 -
@@ -69,10 +69,12 @@
#include lsort.h
#include mailbox.h
#include map.h
+#include mboxlist.h
#include message.h
#include parseaddr.h
#include search_engines.h
#include seen.h
+#include statuscache.h
#include strhash.h
#include stristr.h
#include util.h
@@ -1550,53 +1552,107 @@
/*
* Performs a STATUS command
*/
-int
-index_status(mailbox, name, statusitems)
-struct mailbox *mailbox;
-char *name;
-int statusitems;
+int index_status(char *mboxname, char *name, unsigned statusitems)
{
int r;
-struct seen *status_seendb;
-time_t last_read, last_change = 0;
-unsigned last_uid;
-char *last_seenuids;
+struct statuscache_data scdata;
+struct mailbox mailbox;
+int doclose = 0;
int num_recent = 0;
int num_unseen = 0;
int sepchar;
static struct seq_set seq_set = { NULL, 0, 0, 0 , NULL};
-if (mailbox-exists != 0
- (statusitems
- (STATUS_RECENT | STATUS_UNSEEN))) {
- r = seen_open(mailbox,
- (mailbox-options OPT_IMAP_SHAREDSEEN) ? anyone :
+/* Check status cache if possible */
+if (config_getswitch(IMAPOPT_STATUSCACHE)) {
+ /* Do actual lookup of cache item. */
+ r = statuscache_lookup(mboxname, imapd_userid, scdata);
+
+ /* We need to find everything we're looking for in the cache,
+ otherwise we fallback to the regular method */
+ if (!r (scdata.statusitems statusitems) != statusitems) {
+ r = IMAP_NO_NOSUCHMSG;
+ }
+
+ if (!r) {
+ /* If index file has changed, fallback to regular method */
+ char *path, *mpath;
+ struct stat index;
+
+ /* Get path to mailbox */
+ r = mboxlist_detail(mboxname, NULL, path, mpath, NULL, NULL, NULL);
+ if (!r) r = mailbox_stat(path, mpath, NULL, index, NULL);
+
+ if (!r
+ (index.st_mtime != scdata.message_mtime ||
+ index.st_ino != scdata.message_ino ||
+ index.st_size != scdata.message_size)) {
+ r = IMAP_NO_NOSUCHMSG;
+ }
+
+ /* Seen/recent status uses push invalidation events from
+ * seen_db.c. This avoids needing to open cyrus.header to get
+ * the mailbox uniqueid to open the seen db and get the
+ * unseen_mtime and recentuid.
+ */
+ }
+
+ if (!r) {
+ syslog(LOG_DEBUG, statuscache, '%s', '%s', '0x%02x', 'yes',
+ mboxname, imapd_userid, statusitems);
+ goto statusdone
Re: RHEL5 x64, skiplist statuscache SIGV
On Sun, 23 Sep 2007 21:58:57 +0200, Michael Glad [EMAIL PROTECTED] said:
Hello Bron, I've tested Cyrus 2.3.9 + your FM patches as of September 20
on a RHEL5 x64 Opteron virtual machine.
We don't have any experience (yet) with 64 bit. I believe it's less
well tested.
As I'm a BDB skepticist I use skiplists where ever possible including
the statuscache.
From memory it's not a good match because of the various strengths and
weaknesses of the the database methods used. Skiplist is good for heavy
read loads, not so good for heavy write loads.
But Imapd SIGV's in the skiplist mydelete routine (invoked from
statuscache_invalidate) when the first 'select INBOX' command is
received.
I've looked into the cause and would to hear your opinion before I
post the info below to the devel list.
It seems to me that the skiplist routine is to blame. The SIGV
arises from line 1305:
newoffset = htonl(FORWARD(ptr, i));
But ptr at the time of the crash points to db-map_base which is the
initial skiplist header = Kaboom.
When I apply the patch:
@@ -1292,6 +1292,11 @@
ptr = find_node(db, key, keylen, updateoffsets);
if (ptr == db-map_base ||
!db-compar(KEY(ptr), KEYLEN(ptr), key, keylen)) {
+/* start glad */
+if(ptr == db-map_base) {
+ ptr = DUMMY_PTR(db);
+}
+/* end glad */
/* gotcha */
offset = ptr - db-map_base;
things doesn't SIGV and output from
/usr/lib/cyrus-imapd/cyr_dbtool /var/lib/imap/statuscache.db skiplist
show
looks sensible -- it is partly garbled on my nonpatched RHEL4
x32 production system.
I have CC'd this to the cyrus development list, as there are probably people
with
more skiplist and 64 bit programming experience who can tell you if this is the
right approach, and hopefully push the fix into the next Cyrus release!
Bron.
--
Bron Gondwana
[EMAIL PROTECTED]
