On 21 Feb 2013 00:55:00 +0100 neilb@xxxxxxx wrote:
> On 20 Feb 2013 14:57:07 +0100 bstroesser@xxxxxxxxxxxxxx wrote:
>
> > On 20 Feb 2013 04:09:00 +0100 neilb@xxxxxxx wrote:
snip
> > > Maybe:
> > >
> > > switch(cache_make_upcall(detail, h)) {
> > > case -EINVAL:
> > > if (rv) {
> > > set_bit(CACHE_NEGATIVE, &h->flags);
> > > cache_fresh_locked(h, get_seconds() + CACHE_NEW_EXPIRY);
> > > rv = -ENOENT;
> > > }
> > > /* FALLTHROUGH */
> > > case -EAGAIN:
> > > cache_fresh_unlocked(h, detail);
> > > }
> >
> > I agree, your patch is obviously better than the mine.
> > But let me suggest one little change: I would like to substitute
> > cache_fresh_unlocked() by clear_bit() and cache_revisit_request(),
> > as the call to cache_dequeue() in cache_fresh_unlocked() seems to
> > be obsolete here:
>
> It is exactly this sort of thinking (on my part) that got us into this mess
> in the first place. I reasoned that the full locking/testing/whatever wasn't
> necessary and took a short cut. It wasn't a good idea.
Maybe I'm totally wrong, but AFAICS, calling cache_dequeue() here in extreme
situations (two threads in parallel calling check_cache() while a first reader
is going to open cache access) could again cause a race (?)
BTW: if there is a reader for a cache, is there any protection against many
upcalls being queued for the same cache entry?
Bodo
>
> Given that this is obviously difficult code to get right, we should make it
> as easy to review as possible. Have "cache_fresh_unlocked" makes it more
> obviously correct, and that is a good thing.
> Maybe cache_dequeue isn't needed here, but it won't hurt so I'd much rather
> have the clearer code.
> In fact, I'd also like to change
>
> if (test_and_clear_bit(CACHE_PENDING, &ch->flags))
> cache_dequeue(current_detail, ch);
> cache_revisit_request(ch);
>
> near the end of cache_clean to call cache_fresh_unlocked().
>
ÿôèº{.nÇ+?·?®??+%?Ëÿ±éݶ¥?wÿº{.nÇ+?·¥?{±þwìþ)í?æèw* jg¬±¨ ¶????Ý¢jÿ¾«þG«?éÿ¢¸ ¢·¦j:+v?¨?wèjØm¶?ÿþø ¯ù ®w¥þ?àþf£¢·h??â?úÿ?Ù¥
