Hello,
A few weeks ago, I stumbled upon swap entry size calculation and
store dir selection bug. I found more problems with the related code.
One of them is discussed in this email.
Apparently, we are packing StoreEntry.swap_file_sz into the stored
entry header before we compute its value. This happens because to
compute swap_file_sz, we need to know swap_hdr_sz, the size of the
stored entry header. To compute swap_hdr_sz, we need to pack the
header, but we cannot pack the header because swap_file_sz field has
not been computed yet. Chicken and egg, hidden behind a bunch of void*
memcopies.
We have not noticed the problem because UFS ignores swap_file_sz (or
supplies its own) in most contexts and COSS simply lacks the part of
the code that would expose the bug (no store rebuilding from the
disk db file).
The attached patch is a "work in progress" hack to facilitate the
discussion. The patch computes the future swap_hdr_sz without packing
the header, sets swap_file_sz, and then packs the header. It is not
very efficient but appears to work.
If I am on the wrong path here, please shout.
Thank you,
Alex.
Work in progress: compute swap_file_sz before packing it
=== modified file 'src/StoreMeta.h'
--- src/StoreMeta.h 2009-02-01 10:09:23 +0000
+++ src/StoreMeta.h 2009-09-13 10:26:18 +0000
@@ -162,6 +162,8 @@
/// \ingroup SwapStoreAPI
SQUIDCEXTERN char *storeSwapMetaPack(tlv * tlv_list, int *length);
/// \ingroup SwapStoreAPI
+SQUIDCEXTERN size_t storeSwapMetaSize(const StoreEntry * e);
+/// \ingroup SwapStoreAPI
SQUIDCEXTERN tlv *storeSwapMetaBuild(StoreEntry * e);
/// \ingroup SwapStoreAPI
SQUIDCEXTERN void storeSwapTLVFree(tlv * n);
=== modified file 'src/store.cc'
--- src/store.cc 2009-09-02 05:42:24 +0000
+++ src/store.cc 2009-09-13 11:06:37 +0000
@@ -1801,12 +1801,18 @@
char const *
StoreEntry::getSerialisedMetaData()
{
+ const size_t swap_hdr_sz0 = storeSwapMetaSize(this);
+ assert (swap_hdr_sz0 >= 0);
+ mem_obj->swap_hdr_sz = (size_t) swap_hdr_sz0;
+ // now we can use swap_hdr_sz to calculate swap_file_sz
+ // so that storeSwapMetaBuild/Pack can pack corrent swap_file_sz
+ swap_file_sz = objectLen() + mem_obj->swap_hdr_sz;
+
StoreMeta *tlv_list = storeSwapMetaBuild(this);
int swap_hdr_sz;
char *result = storeSwapMetaPack(tlv_list, &swap_hdr_sz);
+ assert(static_cast<int>(swap_hdr_sz0) == swap_hdr_sz);
storeSwapTLVFree(tlv_list);
- assert (swap_hdr_sz >= 0);
- mem_obj->swap_hdr_sz = (size_t) swap_hdr_sz;
return result;
}
=== modified file 'src/store_swapmeta.cc'
--- src/store_swapmeta.cc 2009-02-01 10:09:23 +0000
+++ src/store_swapmeta.cc 2009-09-13 11:12:46 +0000
@@ -52,6 +52,30 @@
}
/*
+ * Calculate TLV list size for a StoreEntry
+ * XXX: Must match the actual storeSwapMetaBuild result size
+ */
+size_t
+storeSwapMetaSize(const StoreEntry * e)
+{
+ size_t size = 0;
+ ++size; // STORE_META_OK
+ size += sizeof(int); // size of header to follow
+
+ const size_t pfx = sizeof(char) + sizeof(int); // in the start of list
entries
+
+ size += pfx + SQUID_MD5_DIGEST_LENGTH;
+ size += pfx + STORE_HDR_METASIZE;
+ size += pfx + strlen(e->url()) + 1;
+
+ if (const char *vary = e->mem_obj->vary_headers)
+ size += pfx + strlen(vary) + 1;
+
+ debugs(20, 3, "storeSwapMetaSize(" << e->url() << "): " << size);
+ return size;
+}
+
+/*
* Build a TLV list for a StoreEntry
*/
tlv *
=== modified file 'src/store_swapout.cc'
--- src/store_swapout.cc 2009-09-02 05:53:27 +0000
+++ src/store_swapout.cc 2009-09-13 09:24:16 +0000
@@ -345,7 +345,8 @@
debugs(20, 3, "storeSwapOutFileClosed: SwapOut complete: '" <<
e->url() << "' to " <<
e->swap_dirn << ", " << std::hex << std::setw(8) <<
std::setfill('0') <<
std::uppercase << e->swap_filen);
- e->swap_file_sz = e->objectLen() + mem->swap_hdr_sz;
+// e->swap_file_sz = e->objectLen() + mem->swap_hdr_sz;
+assert(e->swap_file_sz == static_cast<uint64_t>(e->objectLen() +
mem->swap_hdr_sz));
e->swap_status = SWAPOUT_DONE;
e->store()->updateSize(e->swap_file_sz, 1);
