Re: [PATCH v3] sha1_file: introduce close_one_pack() to close packs on fd pressure

[Brandon Casey <drafnel@gmail.com>]

On Fri, Aug 2, 2013 at 9:26 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Brandon Casey <drafnel@gmail.com> writes:
>
>> +/*
>> + * The LRU pack is the one with the oldest MRU window, preferring packs
>> + * with no used windows, or the oldest mtime if it has no windows allocated.
>> + */
>> +static void find_lru_pack(struct packed_git *p, struct packed_git **lru_p, struct pack_window **mru_w, int *accept_windows_inuse)
>> +{
>> +     struct pack_window *w, *this_mru_w;
>> +     int has_windows_inuse = 0;
>> +
>> +     /*
>> +      * Reject this pack if it has windows and the previously selected
>> +      * one does not.  If this pack does not have windows, reject
>> +      * it if the pack file is newer than the previously selected one.
>> +      */
>> +     if (*lru_p && !*mru_w && (p->windows || p->mtime > (*lru_p)->mtime))
>> +             return;
>> +
>> +     for (w = this_mru_w = p->windows; w; w = w->next) {
>> +             /*
>> +              * Reject this pack if any of its windows are in use,
>> +              * but the previously selected pack did not have any
>> +              * inuse windows.  Otherwise, record that this pack
>> +              * has windows in use.
>> +              */
>> +             if (w->inuse_cnt) {
>> +                     if (*accept_windows_inuse)
>> +                             has_windows_inuse = 1;
>> +                     else
>> +                             return;
>> +             }
>> +
>> +             if (w->last_used > this_mru_w->last_used)
>> +                     this_mru_w = w;
>> +
>> +             /*
>> +              * Reject this pack if it has windows that have been
>> +              * used more recently than the previously selected pack.
>> +              * If the previously selected pack had windows inuse and
>> +              * we have not encountered a window in this pack that is
>> +              * inuse, skip this check since we prefer a pack with no
>> +              * inuse windows to one that has inuse windows.
>> +              */
>> +             if (*mru_w && *accept_windows_inuse == has_windows_inuse &&
>> +                 this_mru_w->last_used > (*mru_w)->last_used)
>> +                     return;
>
> The "*accept_windows_inuse == has_windows_inuse" part is hard to
> grok, together with the fact that this statement is evaluated for
> each and every "w", even though it is about this_mru_w and that
> variable is not updated in every iteration of the loop.  Can you
> clarify/simplify this part of the code a bit more?
>
> For example, would the above be equivalent to this?
>
>                 if (w->last_used < this_mru_w->last_used)
>                         continue;
>
>                 this_mru_w = w;
>                 if (has_windows_inuse && *mru_w &&
>                     w->last_used > (*mru_w)->last_used)
>                         return;
>
> That is, if we already know a more recently used window in this
> pack, we do not have to do anything to maintain mru_w.  Otherwise,
> remember that this window is the most recently used one in this
> pack, and if it is newer than the newest one from the pack we are
> going to pick, we refrain from picking this pack.
>
> But we do not reject ourselves if we haven't seen a window that is
> in use (yet).

No that wouldn't be the same.  The function of "*accept_windows_inuse
== has_windows_inuse" and the testing of this_mru_w in every loop
rather than w, is too subtle.  I tried to draw attention to it in the
comment, but I agree it's not enough.

The case that your example would not catch is when the new pack's mru
window has already been found, but has_windows_inuse is not set until
later.  When has_windows_inuse is later set, we need to test
this_mru_w regardless of whether we have just assigned it.

For example, if mru_w points to a pack with last_used == 11 and
*accept_windows_inuse = 1, and p->windows looks like this:

   last_used  in_use
   12         0
   10         1

Then the first time through the loop, this_mru_w would be set to the
first window with last_used equal to 12.  The if statement that tests
"this_mru_w->last_used > (*mru_w)->last_used" would be skipped since
has_windows_inuse would still be 0.  The second time through the loop,
this_mru_w would _not_ be reset, but has_windows_inuse _would_ be set.
 This time, we would want to enter the last for loop so that we can
reject the pack.

I'll try to rework this loop or add comments to clarify.

-Brandon


>> +     }
>> +
>> +     /*
>> +      * Select this pack.
>> +      */
>> +     *mru_w = this_mru_w;
>> +     *lru_p = p;
>> +     *accept_windows_inuse = has_windows_inuse;
>> +}
>> +
>> +static int close_one_pack(void)
>> +{
>> +     struct packed_git *p, *lru_p = NULL;
>> +     struct pack_window *mru_w = NULL;
>> +     int accept_windows_inuse = 1;
>> +
>> +     for (p = packed_git; p; p = p->next) {
>> +             if (p->pack_fd == -1)
>> +                     continue;
>> +             find_lru_pack(p, &lru_p, &mru_w, &accept_windows_inuse);
>> +     }
>> +
>> +     if (lru_p) {
>> +             close(lru_p->pack_fd);
>> +             pack_open_fds--;
>> +             lru_p->pack_fd = -1;
>> +             return 1;
>> +     }
>> +
>> +     return 0;
>> +}
>> +
>>  void unuse_pack(struct pack_window **w_cursor)
>>  {
>>       struct pack_window *w = *w_cursor;
>> @@ -768,7 +845,7 @@ static int open_packed_git_1(struct packed_git *p)
>>                       pack_max_fds = 1;
>>       }
>>
>> -     while (pack_max_fds <= pack_open_fds && unuse_one_window(NULL, -1))
>> +     while (pack_max_fds <= pack_open_fds && close_one_pack())
>>               ; /* nothing */
>>
>>       p->pack_fd = git_open_noatime(p->pack_name);