Re: compute swap_file_sz before packing it

On 09/14/2009 03:26 PM, Henrik Nordstrom wrote:
> mån 2009-09-14 klockan 02:04 -0600 skrev Alex Rousskov:
>
>> 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.
>
> Hmm... are we really packing that one? I thougt we only packed the objet
> size in STORE_META_OBJSIZE which is excluding the TLV header.
>
> Right... it's in STORE_META_STD. Just ignore that one as a design error.
> Use STORE_META_OBJSIZE instead, and calculate the "file size" as object
> size + header size if needed.

Hi Henrik,

    Can you explain what you mean by "just ignore" above? It is kind of
difficult for me to ignore the only code that seems to supply the info
Rock store needs. Do you mean we should ultimately remove STORE_META_STD
from Squid, replacing all its current uses with STORE_META_OBJSIZE?

In my case, the core part of Squid code is using STORE_META_STD as a
part of stored "file" header and Rock code needs to rebuild the index by
loading that header from each "file". There is no swap state. Are you
suggesting that the core Squid code should not be using STORE_META_STD
for stored object headers and should be using STORE_META_OBJSIZE instead?

Moreover, STORE_META_OBJSIZE has the following comment attached to its
declaration: "not implemented, squid26 compatibility" and appears to be
unused...

>> 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.
>
> Do we need that? COSS rebuildig is quite content with using
> STORE_META_OBJSIZE.

Squid3 COSS cannot rebuild from the store db, only from swap state. If
swap state is gone, COSS db is gone. See storeCossDirRebuild. COSS is
using StoreSwapLogData::swap_file_sz in swap state and that field is not
broken. Broken STORE_META_STD is in all the store db entries but COSS
does not use it so it does not know that it is broken.

UFS can rebuild from store dir, but it uses fstat() call to determine
the file size and ignores broken swap_file_sz.

Neither approach works for Rock store because Rock store does not have a
swap state file like COSS and does not use individual files like UFS.
That is why it has to rely on the "file" size information supplied by
the core. Perhaps there is a better way of getting that information, but
I do not know it.

Please clarify what your suggestion for dealing with this bug is.

Thank you,

Alex.

> === 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);
>
Received on Tue Oct 27 2009 - 19:42:34 MDT