Changes to accomodate disk write failures:

0) The problem: file_close() doesn't always close the file immediately;
   i.e. when there are pending buffers to write.  However, after
   calling file_close, we were also calling storeUnlockObject and
   the lock count could become zero then.  Bad things would happen
   if we got a callback from a file_write after the lock count went
   to zero.  We need a way to prevent the file_write callback from
   happening when we "close" the swapout FD.

1) the file_write() callback data must now be in the callback
   database (cbdata).  We can't add StoreEntry's to cbdata for a couple
   of reasons.  So now we use the swapout_ctrl_t structure for the
   callback data.  Previously this was used only for the file_open
   process, but now we keep it around for the entire swapout duration.
   A new element MemObject->swapout.ctrl points to this data strucutre
   so we can re-access it from storeCheckSwapOut.

2) Changed the way write errors are handled by diskHandleWrite.
   If there is no callback function, now we exit with a fatal
   message under the assumption that the file in question is a log
   file or IPC pipe.  Otherwise, we flush all the pending write
   buffers (so we don't see multiple repeated write errors from
   the same descriptor) and let the upper layer decide how to handle
   the failure.

3) Fixes to storeDirWriteCleanLogs.  A write failure was leaving some
   empty swap.state files, even though it tells us that its "not
   replacing the file."  Don't flush/rename logs which we have
   prematurely closed due to write failures, indiciated by
   fd[dirn] == -1.  Close these files LAST, not before renaming.

diff --git a/src/disk.cc b/src/disk.cc
index b3e2085627db929a3052284b4e476de8d1448153..2e843fb8056ee8ddd9f7aefd182d69bd4ead70e3 100644 (file)
--- a/src/disk.cc
+++ b/src/disk.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: disk.cc,v 1.103 1998/02/09 21:17:25 wessels Exp $
+ * $Id: disk.cc,v 1.104 1998/02/10 00:55:03 wessels Exp $
  *
  * DEBUG: section 6     Disk I/O Routines
  * AUTHOR: Harvest Derived
@@ -200,10 +200,14 @@ void
 file_close(int fd)
 {
     fde *F = &fd_table[fd];
+#if USE_ASYNC_IO
     if (fd < 0) {
        debug(6, 0) ("file_close: FD less than zero: %d\n", fd);
        return;
     }
+#else
+    assert(fd >= 0);
+#endif
     assert(F->open);
     if (EBIT_TEST(F->flags, FD_WRITE_DAEMON)) {
        EBIT_SET(F->flags, FD_CLOSE_REQUEST);
@@ -304,15 +308,32 @@ diskHandleWriteComplete(void *data, int len, int errcode)
            status = errno == ENOSPC ? DISK_NO_SPACE_LEFT : DISK_ERROR;
            debug(50, 1) ("diskHandleWrite: FD %d: disk write error: %s\n",
                fd, xstrerror());
-           if (fdd->wrt_handle == NULL || status != DISK_NO_SPACE_LEFT) {
-               /* FLUSH PENDING BUFFERS */
-               do {
-                   fdd->write_q = q->next;
-                   if (q->free_func)
-                       (q->free_func) (q->buf);
-                   safe_free(q);
-               } while ((q = fdd->write_q));
-           }
+           /*
+            * If there is no write callback, then this file is
+            * most likely something important like a log file, or
+            * an interprocess pipe.  Its not a swapfile.  We feel
+            * that a write failure on a log file is rather important,
+            * and Squid doesn't otherwise deal with this condition.
+            * So to get the administrators attention, we exit with
+            * a fatal message.
+            */
+           if (fdd->wrt_handle == NULL)
+               fatal("Write failure -- check your disk space and cache.log");
+           /*
+            * If there is a write failure, then we notify the
+            * upper layer via the callback, at the end of this
+            * function.  Meanwhile, flush all pending buffers
+            * here.  Let the upper layer decide how to handle the
+            * failure.  This will prevent experiencing multiple,
+            * repeated write failures for the same FD because of
+            * the queued data.
+            */
+           do {
+               fdd->write_q = q->next;
+               if (q->free_func)
+                   (q->free_func) (q->buf);
+               safe_free(q);
+           } while ((q = fdd->write_q));
        }
        len = 0;
     }
@@ -337,11 +358,16 @@ diskHandleWriteComplete(void *data, int len, int errcode)
        EBIT_CLR(F->flags, FD_WRITE_DAEMON);
     } else {
        /* another block is queued */
+       cbdataLock(fdd->wrt_handle_data);
        commSetSelect(fd, COMM_SELECT_WRITE, diskHandleWrite, NULL, 0);
        EBIT_SET(F->flags, FD_WRITE_DAEMON);
     }
-    if (fdd->wrt_handle)
-       fdd->wrt_handle(fd, status, len, fdd->wrt_handle_data);
+    if (fdd->wrt_handle) {
+       if (fdd->wrt_handle_data == NULL || cbdataValid(fdd->wrt_handle_data))
+           fdd->wrt_handle(fd, status, len, fdd->wrt_handle_data);
+       if (fdd->wrt_handle_data != NULL)
+           cbdataUnlock(fdd->wrt_handle_data);
+    }
     if (EBIT_TEST(F->flags, FD_CLOSE_REQUEST))
        file_close(fd);
 }
@@ -382,6 +408,7 @@ file_write(int fd,
        F->disk.write_q_tail = wq;
     }
     if (!EBIT_TEST(F->flags, FD_WRITE_DAEMON)) {
+       cbdataLock(F->disk.wrt_handle_data);
 #if USE_ASYNC_IO
        diskHandleWrite(fd, NULL);
 #else

diff --git a/src/protos.h b/src/protos.h
index 9eda3bae9c4c5adf838217a356ed2a091987db43..1a2d0e047de4944e9591e262366721214cf2016e 100644 (file)
--- a/src/protos.h
+++ b/src/protos.h
@@ -559,7 +559,6 @@ extern void storeSwapOutStart(StoreEntry * e);
 extern void storeSwapOutHandle(int fdnotused, int flag, size_t len, void *data);
 extern void storeCheckSwapOut(StoreEntry * e);
 extern void storeSwapOutFileClose(StoreEntry * e);
-extern void storeSwapOutFileClose(StoreEntry * e);
 
 /*
  * store_client.c

diff --git a/src/store_dir.cc b/src/store_dir.cc
index bdc883b41bd00390595aea94db9c925f170e3f96..81709f45247177ce70c97fe5793949783c3593f9 100644 (file)
--- a/src/store_dir.cc
+++ b/src/store_dir.cc
@@ -1,6 +1,6 @@
 
 /*
- * $Id: store_dir.cc,v 1.48 1998/02/06 18:54:10 wessels Exp $
+ * $Id: store_dir.cc,v 1.49 1998/02/10 00:55:04 wessels Exp $
  *
  * DEBUG: section 47    Store Directory Routines
  * AUTHOR: Duane Wessels
@@ -519,6 +519,8 @@ storeDirWriteCleanLogs(int reopen)
            debug(50, 0) ("storeDirWriteCleanLogs: %s: %s\n", new[dirn], xstrerror());
            continue;
        }
+        debug(20, 3) ("storeDirWriteCleanLogs: opened %s, FD %d\n",
+               new[dirn], fd[dirn]);
 #if HAVE_FCHMOD
        if (stat(cur[dirn], &sb) == 0)
            fchmod(fd[dirn], sb.st_mode);
@@ -564,12 +566,12 @@ storeDirWriteCleanLogs(int reopen)
        /* buffered write */
        if (outbufoffset[dirn] + sizeof(storeSwapData) > CLEAN_BUF_SZ) {
            if (write(fd[dirn], outbuf[dirn], outbufoffset[dirn]) < 0) {
-               debug(50, 0) ("storeDirWriteCleanLogs: %s: %s\n",
+               debug(50, 0) ("storeDirWriteCleanLogs: %s: write: %s\n",
                    new[dirn], xstrerror());
                debug(20, 0) ("storeDirWriteCleanLogs: Current swap logfile not replaced.\n");
                file_close(fd[dirn]);
                fd[dirn] = -1;
-               unlink(cln[dirn]);
+               unlink(new[dirn]);
                continue;
            }
            outbufoffset[dirn] = 0;
@@ -583,22 +585,25 @@ storeDirWriteCleanLogs(int reopen)
     for (dirn = 0; dirn < Config.cacheSwap.n_configured; dirn++) {
        if (outbufoffset[dirn] == 0)
            continue;
+       if (fd[dirn] < 0)
+           continue;
        if (write(fd[dirn], outbuf[dirn], outbufoffset[dirn]) < 0) {
-           debug(50, 0) ("storeDirWriteCleanLogs: %s: %s\n", new[dirn], xstrerror());
+           debug(50, 0) ("storeDirWriteCleanLogs: %s: write: %s\n",
+               new[dirn], xstrerror());
            debug(20, 0) ("storeDirWriteCleanLogs: Current swap logfile not replaced.\n");
            file_close(fd[dirn]);
            fd[dirn] = -1;
-           unlink(cln[dirn]);
+           unlink(new[dirn]);
            continue;
        }
        safe_free(outbuf[dirn]);
     }
     safe_free(outbuf);
     safe_free(outbufoffset);
-    /* close */
+    /* rename */
     for (dirn = 0; dirn < Config.cacheSwap.n_configured; dirn++) {
-       file_close(fd[dirn]);
-       fd[dirn] = -1;
+       if (fd[dirn] < 0)
+           continue;
        if (rename(new[dirn], cur[dirn]) < 0) {
            debug(50, 0) ("storeDirWriteCleanLogs: rename failed: %s, %s -> %s\n",
                xstrerror(), new[dirn], cur[dirn]);
@@ -613,13 +618,23 @@ storeDirWriteCleanLogs(int reopen)
     debug(20, 1) ("  Took %d seconds (%6.1lf entries/sec).\n",
        r > 0 ? r : 0, (double) n / (r > 0 ? r : 1));
     /* touch a timestamp file if we're not still validating */
-    for (dirn = 0; dirn < Config.cacheSwap.n_configured; dirn++) {
-       if (!store_rebuilding)
+    if (!store_rebuilding) {
+        for (dirn = 0; dirn < Config.cacheSwap.n_configured; dirn++) {
+           if (fd[dirn] < 0)
+               continue;
            file_close(file_open(cln[dirn],
-                   O_WRONLY | O_CREAT | O_TRUNC, NULL, NULL, NULL));
+               O_WRONLY | O_CREAT | O_TRUNC, NULL, NULL, NULL));
+        }
+    }
+    /* close */
+    for (dirn = 0; dirn < Config.cacheSwap.n_configured; dirn++) {
        safe_free(cur[dirn]);
        safe_free(new[dirn]);
        safe_free(cln[dirn]);
+       if (fd[dirn] < 0)
+           continue;
+       file_close(fd[dirn]);
+       fd[dirn] = -1;
     }
     safe_free(cur);
     safe_free(new);

diff --git a/src/store_rebuild.cc b/src/store_rebuild.cc
index a8744cf582829fc43b755500d514e184d3cfac71..56054e5bf2907705af8e02eef8fa88648c496968 100644 (file)
--- a/src/store_rebuild.cc
+++ b/src/store_rebuild.cc
@@ -72,12 +72,13 @@ storeRebuildFromDirectory(rebuild_dir * d)
     tlv *t;
     double x;
     assert(d != NULL);
-    debug(20, 3) ("storeRebuildFromDirectory: DIR #%s\n", d->dirn);
+    debug(20, 3) ("storeRebuildFromDirectory: DIR #%d\n", d->dirn);
     for (count = 0; count < d->speed; count++) {
        assert(fd == -1);
        fd = storeGetNextFile(d, &sfileno, &size);
        if (fd == -2) {
-           debug(20, 1) ("storeRebuildFromDirectory: done!\n");
+           debug(20, 1) ("storeRebuildFromDirectory: DIR #%d done!\n", d->dirn);
+           storeDirCloseTmpSwapLog(d->dirn);
            store_rebuilding = 0;
            return -1;
        } else if (fd < 0) {

diff --git a/src/store_swapout.cc b/src/store_swapout.cc
index 0c237cb94170192c07103c01b6c942cf7918502b..9122140c000953d9cc01e8419fee68fc1aa5353d 100644 (file)
--- a/src/store_swapout.cc
+++ b/src/store_swapout.cc
@@ -12,26 +12,28 @@ static FOCB storeSwapOutFileOpened;
 void
 storeSwapOutStart(StoreEntry * e)
 {
-    swapout_ctrl_t *ctrlp;
-    LOCAL_ARRAY(char, swapfilename, SQUID_MAXPATHLEN);
+    swapout_ctrl_t *ctrlp = xmalloc(sizeof(swapout_ctrl_t));
+    assert(e->mem_obj);
+    cbdataAdd(ctrlp, MEM_NONE);
     storeLockObject(e);
     e->swap_file_number = storeDirMapAllocate();
-    storeSwapFullPath(e->swap_file_number, swapfilename);
-    ctrlp = xmalloc(sizeof(swapout_ctrl_t));
-    ctrlp->swapfilename = xstrdup(swapfilename);
+    ctrlp->swapfilename = xstrdup(storeSwapFullPath(e->swap_file_number, NULL));
     ctrlp->e = e;
     ctrlp->oldswapstatus = e->swap_status;
     e->swap_status = SWAPOUT_OPENING;
-    file_open(swapfilename,
+    e->mem_obj->swapout.ctrl = ctrlp;
+    file_open(ctrlp->swapfilename,
        O_WRONLY | O_CREAT | O_TRUNC,
        storeSwapOutFileOpened,
-       ctrlp, e);
+       ctrlp,
+       e);
 }
 
 void
 storeSwapOutHandle(int fdnotused, int flag, size_t len, void *data)
 {
-    StoreEntry *e = data;
+    swapout_ctrl_t *ctrlp = data;
+    StoreEntry *e = ctrlp->e;
     MemObject *mem = e->mem_obj;
     debug(20, 3) ("storeSwapOutHandle: '%s', len=%d\n", storeKeyText(e->key), (int) len);
     if (flag < 0) {
@@ -187,7 +189,7 @@ storeCheckSwapOut(StoreEntry * e)
        swap_buf,
        swap_buf_len,
        storeSwapOutHandle,
-       e,
+       mem->swapout.ctrl,
        memFreeDISK);
 }
 
@@ -195,50 +197,54 @@ void
 storeSwapOutFileClose(StoreEntry * e)
 {
     MemObject *mem = e->mem_obj;
+    swapout_ctrl_t *ctrlp;
     assert(mem != NULL);
-    if (mem->swapout.fd > -1) {
-       file_close(mem->swapout.fd);
-       mem->swapout.fd = -1;
-       storeUnlockObject(e);
-    }
+    if (mem->swapout.fd < 0) {
 #if USE_ASYNC_IO
-    else
        aioCancel(-1, e);       /* Make doubly certain pending ops are gone */
 #endif
+       return;
+    }
+    ctrlp = mem->swapout.ctrl;
+    file_close(mem->swapout.fd);
+    mem->swapout.fd = -1;
+    xfree(ctrlp->swapfilename);
+    cbdataFree(ctrlp);
+    mem->swapout.ctrl = NULL;
+    storeUnlockObject(e);
 }
 
 static void
 storeSwapOutFileOpened(void *data, int fd, int errcode)
 {
     swapout_ctrl_t *ctrlp = data;
-    int oldswapstatus = ctrlp->oldswapstatus;
-    char *swapfilename = ctrlp->swapfilename;
     StoreEntry *e = ctrlp->e;
-    MemObject *mem;
+    MemObject *mem = e->mem_obj;
     int swap_hdr_sz = 0;
     tlv *tlv_list;
     char *buf;
-    xfree(ctrlp);
     if (fd == -2 && errcode == -2) {   /* Cancelled - Clean up */
-       xfree(swapfilename);
+       xfree(ctrlp->swapfilename);
+       cbdataFree(ctrlp);
+       mem->swapout.ctrl = NULL;
        return;
     }
     assert(e->swap_status == SWAPOUT_OPENING);
     if (fd < 0) {
        debug(20, 0) ("storeSwapOutFileOpened: Unable to open swapfile: %s\n",
-           swapfilename);
+           ctrlp->swapfilename);
        storeDirMapBitReset(e->swap_file_number);
        e->swap_file_number = -1;
-       e->swap_status = oldswapstatus;
-       xfree(swapfilename);
+       e->swap_status = ctrlp->oldswapstatus;
+       xfree(ctrlp->swapfilename);
+       cbdataFree(ctrlp);
+       mem->swapout.ctrl = NULL;
        return;
     }
-    mem = e->mem_obj;
     mem->swapout.fd = (short) fd;
     e->swap_status = SWAPOUT_WRITING;
     debug(20, 5) ("storeSwapOutFileOpened: Begin SwapOut '%s' to FD %d '%s'\n",
-       storeUrl(e), fd, swapfilename);
-    xfree(swapfilename);
+       storeUrl(e), fd, ctrlp->swapfilename);
     debug(20, 5) ("swap_file_number=%08X\n", e->swap_file_number);
     tlv_list = storeSwapMetaBuild(e);
     buf = storeSwapMetaPack(tlv_list, &swap_hdr_sz);
@@ -249,6 +255,6 @@ storeSwapOutFileOpened(void *data, int fd, int errcode)
        buf,
        mem->swap_hdr_sz,
        storeSwapOutHandle,
-       e,
+       ctrlp,
        xfree);
 }

diff --git a/src/structs.h b/src/structs.h
index ed473d9595a745b9c92666a2139c03455590c358..06654e8de9a10254c1c7c1f013fac75f51d2fc2d 100644 (file)
--- a/src/structs.h
+++ b/src/structs.h
@@ -805,6 +805,7 @@ struct _MemObject {
        off_t queue_offset;     /* relative to in-mem data */
        off_t done_offset;      /* relative to swap file with meta headers! */
        int fd;
+       void *ctrl;
     } swapout;
     struct _http_reply *reply;
     request_t *request;