Re: [PATCH v2 2/4] Consolidate code to close a pack's file descriptor
- From
Johannes Schindelin <johannes.schindelin@gmx.de>
- Date
- Oct 5, 2015, 21:52 UTC
- Message-ID
- <b7d0f89bb94f88cb8d600a461dfe7ea1@dscho.org>
- In-Reply-To
- <xmqqio6lxcci.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On 2015-10-05 22:57, Junio C Hamano wrote:
Show 38 quoted lines
> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>
>> There was a lot of repeated code to close the file descriptor of
>> a given pack. Let's just refactor this code into a single function.
>
> That is a very good idea, but...
>
>> +static int close_pack_fd(struct packed_git *p)
>> +{
>> + if (p->pack_fd < 0)
>> + return 0;
>
> Is this "return 0" compatible with ...
>
>> + close(p->pack_fd);
>> + pack_open_fds--;
>> + p->pack_fd = -1;
>> +
>> + return 1;
>> +}
>> +
>> /*
>> * 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.
>> @@ -853,12 +865,8 @@ static int close_one_pack(void)
>> 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;
>> - }
>> + if (lru_p)
>> + return close_pack_fd(lru_p);
>
> ... what is returned from here?Yes. At this point, `lru_p` can only be non-NULL if lru_p->pack_fd is not larger than 0 (hence the call to `close(lru_p->pack_fd)` does not fail all the time, and hence the `pack_open_fds--` does not result in inconsistent state).
> It seems to me that it would be a bug if (p->pack_fd < 0) in this > codepath, so in practice nobody will receive a newly invented return > value 0 from this function, but it feels wrong.
Yes, it would be a bug. And more subtle things would go wrong if that was the case, too.
Show 17 quoted lines
>
>> return 0;
>> }
>> @@ -899,10 +907,7 @@ void free_pack_by_name(const char *pack_name)
>> if (strcmp(pack_name, p->pack_name) == 0) {
>> clear_delta_base_cache();
>> close_pack_windows(p);
>> - if (p->pack_fd != -1) {
>> - close(p->pack_fd);
>> - pack_open_fds--;
>> - }
>> + close_pack_fd(p);
>
> And here, the closer _must_ be (and it currently is) very aware that
> the pack chosen may not be open anymore. By giving a function that
> conditionally closes if the pack is still open too bland a name,
> that distinction is lost at these two call sites.Please note that the `close_pack_fd(p)` call does not invalidate the data. It is the caller (`free_pack_by_name()`) that does. Which is safe.
> Also closing "fd" is not the only thing the new helper does, so in > that sense its name is suboptimal, too.
Yes, it also decrements the counter. But that is a necessary consequence of closing the file descriptor, otherwise the state would be inconsistent.
> Perhaps a new helper function that is close_pack(), which is > unconditional, with another close_pack_if_open() around it?
Next patch. And that `close_pack()` actually does do more than closing the file descriptor.
Show 12 quoted lines
>> if (!win->offset && win->len == p->pack_size
>> - && !p->do_not_close) {
>> - close(p->pack_fd);
>> - pack_open_fds--;
>> - p->pack_fd = -1;
>> - }
>> + && !p->do_not_close)
>> + close_pack_fd(p);
>
> I wonder how this do_not_close bit should interact with "we are
> shutting down (or we are spawning another to do the real work, while
> we won't do anything useful anymore), so close everything down".The `close_all_packs()` function is meant for the use case where you really close all the packs, no question asked.
I cannot think of a use case where it would make sense to try to be gentle, yet still ask for *all* of the packs to be closed. Maybe you can think of one to convince me that it might make sense to respect the `do_not_close` flag in such a function? Because so far, I am totally unconvinced.
Ciao, Johannes