git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Derrick Stolee <stolee@gmail.com>
Cc: git@vger.kernel.org, peff@peff.net, git@jeffhostetler.com,
	jonathantanmy@google.com, sbeller@google.com,
	szeder.dev@gmail.com, ramsay@ramsayjones.plus.com,
	Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH v5 04/13] csum-file: add CSUM_KEEP_OPEN flag
Date: Tue, 13 Mar 2018 14:42:34 -0700
Message-ID: <xmqqsh931wl1.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <e1264d9a-9d6c-b294-6d56-46382a28b280@gmail.com>

Derrick Stolee <stolee@gmail.com> writes:

> On 2/26/2018 9:32 PM, Derrick Stolee wrote:
>> This patch is new to the series due to the interactions with the lockfile API
>> and the hashfile API. I need to ensure the hashfile writes the hash value at
>> the end of the file, but keep the file descriptor open so the lock is valid.
>>
>> I welcome any susggestions to this patch or to the way I use it in the commit
>> that follows.
>>
>> -- >8 --
>
> I haven't gotten any feedback on this step of the patch. Could someone
> take a look and let me know what you think?

Let's follow the commit-graph writing codepath to see what happens:

	fd = hold_lock_file_for_update(&lk, graph_name, 0);
	...
	f = hashfd(lk.tempfile->fd, lk.tempfile->filename.buf);

The caller creates a lockfile, and then wraps its file descriptor in
a hashfile.

	hashwrite_be32(f, GRAPH_SIGNATURE);
	...

Then it goes on writing to the hashfile, growing the lockfile.

        ...
	write_graph_chunk_large_edges(f, commits.list, commits.nr);

	close_commit_graph();

And after writing all data out (oh by the way, why aren't we passing
commit_graph instance around and instead relying on a file-scope
static global?)...

	hashclose(f, final_hash, CSUM_CLOSE | CSUM_FSYNC | CSUM_KEEP_OPEN);

We ask for the final hash value to be written to the file (and also
returned to us---although you do not seem to use that at all).  See
a comment on this, though, at the end.

	commit_lock_file(&lk);

And then, we put the lockfile to its final place, while closing its
file descriptor.

The overall API sounds sensible, from the above.

However.

The function whose name is hashclose() that takes a flag word whose
possible bit value includes "Please close this thing" feels strange
enough (does it mean the hashclose() function does not close it if
CSUM_CLOSE is not given?), but adding another to the mix that lets
us say "Please close this (with or without FSYNC), oh by the way
please leave it open" feels a bit borderline to insanity.

I _think_ the word "close" in the name hashclose() is about closing
the (virtual) stream for the hashing that is overlayed on top of the
underlying file descriptor, and being able to choose between closing
and not closing the underlying file descriptor when "closing" the
hashing layer sort of makes sense.  So I won't complain too much
about hashclose() that takes optional CSUM_CLOSE flag.

But then what does it mean to give KEEP_OPEN and CLOSE together?

The new caller (which is the only one that wants the nominally
nonsensical CLOSE|KEEP_OPEN combination, which is shown above) wants
the final checksum of the data sent over the (virtual) stream
computed and written, and the file descriptor fsync'ed, but the file
descriptor kept open.  As we _DO_ want to keep the verbs in flags
CSUM_CLOSE and CSUM_FSYNC to be about the underlying file
descriptor, I think your new code for KEEP_OPEN that is inside the
if() block that is for CSUM_CLOSE is an ugly hack, and your asking
for improvements is very much appreciated.

Let's step back and see what different behaviours the existing code
wants to support before your patch:

    - hashclose() is always about finializing the hash computation
      over the data sent through the struct hashfile (i.e. the
      virtual stream opened by hashfd()).  The optional *result can
      be used to receive this hash value, even when the caller does
      not want to write that hash value to the output stream.

    - when CSUM_CLOSE is given, however, the hash value is written
      out as the trailing record to the output stream and the stream
      is closed.  CSUM_FSYNC can instead be used to ensure that the
      data hits the disk platter when the output stream is closed.

    - when CSUM_CLOSE nor CSUM_FSYNC is not given, hash value is not
      written to the output stream (the caller takes responsibility
      of using *result), and the output stream is left open.

I think the first mistake in the existing code is to associate
"close the underlying stream" and "write the hash out to the
underlying stream" more closely than it should.  It should be
possible to "close the underlying steam" without first writing the
hash out to the underlying stream", and vice versa.

IOW, I think

        hashclose() {
                hashflush();
                the_hash_algo->final_fn();
                if (result)             
                        hashcpy(result, f->buffer);
        +       if (flags & CSUM_HASH_IN_STREAM)
        +               flush(f, f->buffer, the_hash_algo->rawsz);
        +       if (flags & CSUM_FSYNC)
        +               fsync_or_die();
                if (flags & (CSUM_CLOSE | CSUM_FSYNC)) {
        -               flush();                
        -               if (flags & CSUM_FSYNC)
        -                       fsync_or_die();
                        if (close(f->fd))
                                die_errno();
                        fd = 0;
                } else
                        fd = f->fd;
                if (0 <= f->check_fd) {
                        ...
                }
                free(f);
                return fd;
        }

with would be a good first "preliminary preparation" step.

Existing callers that pass CSUM_FSYNC or CSUM_CLOSE now need to also
say "I want the resulting hash in the output stream", but that
allows your later caller to omit CSUM_CLOSE and then ask for
HASH_IN_STREAM alone.

Existing callers can expect that FSYNC alone means fsync and close,
but your caller wants hashclose() to compute the hash, write the hash
to the output stream, and fsync the output stream, and return
without closing the output stream.  For that, you'd make FSYNC not
to imply CLOSE, and you'd need to vet all the existing callers that
use FSYNC are OK with such a change.  And then the above would
become

        hashclose() {
                hashflush();
                the_hash_algo->final_fn();
                if (result)             
                        hashcpy(result, f->buffer);
                if (flags & CSUM_HASH_IN_STREAM)
                        flush(f, f->buffer, the_hash_algo->rawsz);
                if (flags & CSUM_FSYNC)
                        fsync_or_die();
                if (flags & CSUM_CLOSE) {
                        if (close(f->fd))
                                die_errno();
                        fd = 0;
                } else
                        fd = f->fd;
                if (0 <= f->check_fd) {
                        ...
                }
                free(f);
                return fd;
        }

Once we reach that state, the new caller in write_commit_graph()
does not have to pass nonsensical CLOSE|KEEP_OPEN combination.
Instead we can do

	hashclose(f, NULL, CSUM_HASH_IN_STREAM | CSUM_FSYNC);

or something like that, I would think, without having KEEP_OPEN.

I am actually wondering if it is worth making CSUM_FSYNC not imply
CSUM_CLOSE.  There aren't that many existing callers of hashclose()
that uses FSYNC, so vetting all of them and replacing their FSYNC
with (FSYNC|CLOSE) is not all that difficult, but if this new caller
is an oddball then another strategy may be to do the fsync_or_die()
on the caller side, something like:

                hashclose(f, NULL, CSUM_HASH_IN_STREAM);
        +       fsync_or_die(fd, get_lock_file_path(&lk));
                commit_lock_file(&lk);

And then we can keep the "FSYNC means fsync and then close" the
current set of callers rely on.  I dunno if that is a major issue,
but I do think "close this, or no, keep it open" is far worse than
"do we want the resulting hash in the stream?"

An alternative design of the above is without making
CSUM_HASH_IN_STREAM a new flag bit.  I highly suspect that the
calling codepath _knows_ whether the resulting final hash will be
written out at the end of the stream or not when it wraps an fd with
a hashfile structure, so "struct hashfile" could gain a bit to tell
hashclose() whether the resulting hash need to be written (or not).
That would be a bit larger change than what I outlined above, and I
do not know if it is worth doing, though.




  reply index

Thread overview: 109+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-02-27  2:32 [PATCH v5 00/13] Serialized Git Commit Graph Derrick Stolee
2018-02-27  2:32 ` [PATCH v5 01/13] commit-graph: add format document Derrick Stolee
2018-02-27  2:32 ` [PATCH v5 02/13] graph: add commit graph design document Derrick Stolee
2018-02-27  2:32 ` [PATCH v5 03/13] commit-graph: create git-commit-graph builtin Derrick Stolee
2018-02-27  2:32 ` [PATCH v5 04/13] csum-file: add CSUM_KEEP_OPEN flag Derrick Stolee
2018-03-12 13:55   ` Derrick Stolee
2018-03-13 21:42     ` Junio C Hamano [this message]
2018-03-14  2:26       ` Derrick Stolee
2018-03-14 17:00         ` Junio C Hamano
2018-02-27  2:32 ` [PATCH v5 05/13] commit-graph: implement write_commit_graph() Derrick Stolee
2018-02-27  2:33 ` [PATCH v5 06/13] commit-graph: implement 'git-commit-graph write' Derrick Stolee
2018-02-27  2:33 ` [PATCH v5 07/13] commit-graph: implement git commit-graph read Derrick Stolee
2018-02-27  2:33 ` [PATCH v5 08/13] commit-graph: add core.commitGraph setting Derrick Stolee
2018-02-27  2:33 ` [PATCH v5 09/13] commit-graph: close under reachability Derrick Stolee
2018-02-27  2:33 ` [PATCH v5 10/13] commit: integrate commit graph with commit parsing Derrick Stolee
2018-02-27  2:33 ` [PATCH v5 11/13] commit-graph: read only from specific pack-indexes Derrick Stolee
2018-02-27 20:15   ` Stefan Beller
2018-02-27  2:33 ` [PATCH v5 12/13] commit-graph: build graph from starting commits Derrick Stolee
2018-02-27  2:33 ` [PATCH v5 13/13] commit-graph: implement "--additive" option Derrick Stolee
2018-02-27 18:50 ` [PATCH v5 00/13] Serialized Git Commit Graph Stefan Beller
2018-03-14 19:27 ` [PATCH v6 00/14] " Derrick Stolee
2018-03-14 19:27   ` [PATCH v6 01/14] csum-file: rename hashclose() to finalize_hashfile() Derrick Stolee
2018-03-14 19:27   ` [PATCH v6 02/14] csum-file: refactor finalize_hashfile() method Derrick Stolee
2018-03-14 19:27   ` [PATCH v6 03/14] commit-graph: add format document Derrick Stolee
2018-03-14 19:27   ` [PATCH v6 04/14] graph: add commit graph design document Derrick Stolee
2018-03-14 19:27   ` [PATCH v6 05/14] commit-graph: create git-commit-graph builtin Derrick Stolee
2018-03-14 19:27   ` [PATCH v6 06/14] commit-graph: implement write_commit_graph() Derrick Stolee
2018-03-14 19:27   ` [PATCH v6 07/14] commit-graph: implement 'git-commit-graph write' Derrick Stolee
2018-03-18 13:25     ` Ævar Arnfjörð Bjarmason
2018-03-19 13:12       ` Derrick Stolee
2018-03-19 14:36         ` Ævar Arnfjörð Bjarmason
2018-03-19 18:27           ` Derrick Stolee
2018-03-19 18:48             ` Ævar Arnfjörð Bjarmason
2018-03-14 19:27   ` [PATCH v6 08/14] commit-graph: implement git commit-graph read Derrick Stolee
2018-03-14 19:27   ` [PATCH v6 09/14] commit-graph: add core.commitGraph setting Derrick Stolee
2018-03-14 19:27   ` [PATCH v6 10/14] commit-graph: close under reachability Derrick Stolee
2018-03-14 19:27   ` [PATCH v6 11/14] commit: integrate commit graph with commit parsing Derrick Stolee
2018-03-14 19:27   ` [PATCH v6 12/14] commit-graph: read only from specific pack-indexes Derrick Stolee
2018-03-15 22:50     ` SZEDER Gábor
2018-03-19 13:13       ` Derrick Stolee
2018-03-14 19:27   ` [PATCH v6 13/14] commit-graph: build graph from starting commits Derrick Stolee
2018-03-14 19:27   ` [PATCH v6 14/14] commit-graph: implement "--additive" option Derrick Stolee
2018-03-14 20:10   ` [PATCH v6 00/14] Serialized Git Commit Graph Ramsay Jones
2018-03-14 20:43   ` Junio C Hamano
2018-03-15 17:23     ` Johannes Schindelin
2018-03-15 18:41       ` Junio C Hamano
2018-03-15 21:51         ` Ramsay Jones
2018-03-16 11:50         ` Johannes Schindelin
2018-03-16 17:27           ` Junio C Hamano
2018-03-19 11:41             ` Johannes Schindelin
2018-03-16 16:28     ` Lars Schneider
2018-03-19 13:10       ` Derrick Stolee
2018-03-16 15:06   ` Ævar Arnfjörð Bjarmason
2018-03-16 16:38     ` SZEDER Gábor
2018-03-16 18:33       ` Junio C Hamano
2018-03-16 19:48         ` SZEDER Gábor
2018-03-16 20:06           ` Jeff King
2018-03-16 20:19             ` Jeff King
2018-03-19 12:55               ` Derrick Stolee
2018-03-20  1:17                 ` Derrick Stolee
2018-03-16 20:49         ` Jeff King
2018-04-02 20:34   ` [PATCH v7 " Derrick Stolee
2018-04-02 20:34     ` [PATCH v7 01/14] csum-file: rename hashclose() to finalize_hashfile() Derrick Stolee
2018-04-02 20:34     ` [PATCH v7 02/14] csum-file: refactor finalize_hashfile() method Derrick Stolee
2018-04-07 22:59       ` Jakub Narebski
2018-04-02 20:34     ` [PATCH v7 03/14] commit-graph: add format document Derrick Stolee
2018-04-07 23:49       ` Jakub Narebski
2018-04-02 20:34     ` [PATCH v7 04/14] graph: add commit graph design document Derrick Stolee
2018-04-08 11:06       ` Jakub Narebski
2018-04-02 20:34     ` [PATCH v7 05/14] commit-graph: create git-commit-graph builtin Derrick Stolee
2018-04-02 20:34     ` [PATCH v7 06/14] commit-graph: implement write_commit_graph() Derrick Stolee
2018-04-02 20:34     ` [PATCH v7 07/14] commit-graph: implement git-commit-graph write Derrick Stolee
2018-04-08 11:59       ` Jakub Narebski
2018-04-02 20:34     ` [PATCH v7 08/14] commit-graph: implement git commit-graph read Derrick Stolee
2018-04-02 21:33       ` Junio C Hamano
2018-04-03 11:49         ` Derrick Stolee
2018-04-08 12:59       ` Jakub Narebski
2018-04-02 20:34     ` [PATCH v7 09/14] commit-graph: add core.commitGraph setting Derrick Stolee
2018-04-08 13:39       ` Jakub Narebski
2018-04-02 20:34     ` [PATCH v7 10/14] commit-graph: close under reachability Derrick Stolee
2018-04-02 20:34     ` [PATCH v7 11/14] commit: integrate commit graph with commit parsing Derrick Stolee
2018-04-02 20:34     ` [PATCH v7 12/14] commit-graph: read only from specific pack-indexes Derrick Stolee
2018-04-02 20:34     ` [PATCH v7 13/14] commit-graph: build graph from starting commits Derrick Stolee
2018-04-08 13:50       ` Jakub Narebski
2018-04-02 20:34     ` [PATCH v7 14/14] commit-graph: implement "--additive" option Derrick Stolee
2018-04-05  8:27       ` SZEDER Gábor
2018-04-10 12:55     ` [PATCH v8 00/14] Serialized Git Commit Graph Derrick Stolee
2018-04-10 12:55       ` [PATCH v8 01/14] csum-file: rename hashclose() to finalize_hashfile() Derrick Stolee
2018-04-10 12:55       ` [PATCH v8 02/14] csum-file: refactor finalize_hashfile() method Derrick Stolee
2018-04-10 12:55       ` [PATCH v8 03/14] commit-graph: add format document Derrick Stolee
2018-04-10 19:10         ` Stefan Beller
2018-04-10 19:18           ` Derrick Stolee
2018-04-11 20:58         ` Jakub Narebski
2018-04-12 11:28           ` Derrick Stolee
2018-04-13 22:07             ` Jakub Narebski
2018-04-10 12:55       ` [PATCH v8 04/14] graph: add commit graph design document Derrick Stolee
2018-04-15 22:48         ` Jakub Narebski
2018-04-10 12:55       ` [PATCH v8 05/14] commit-graph: create git-commit-graph builtin Derrick Stolee
2018-04-10 12:56       ` [PATCH v8 06/14] commit-graph: implement write_commit_graph() Derrick Stolee
2018-04-10 12:56       ` [PATCH v8 07/14] commit-graph: implement git-commit-graph write Derrick Stolee
2018-04-10 12:56       ` [PATCH v8 08/14] commit-graph: implement git commit-graph read Derrick Stolee
2018-04-14 22:15         ` Jakub Narebski
2018-04-15  3:26           ` Eric Sunshine
2018-04-10 12:56       ` [PATCH v8 09/14] commit-graph: add core.commitGraph setting Derrick Stolee
2018-04-14 18:33         ` Jakub Narebski
2018-04-10 12:56       ` [PATCH v8 10/14] commit-graph: close under reachability Derrick Stolee
2018-04-10 12:56       ` [PATCH v8 11/14] commit: integrate commit graph with commit parsing Derrick Stolee
2018-04-10 12:56       ` [PATCH v8 12/14] commit-graph: read only from specific pack-indexes Derrick Stolee
2018-04-10 12:56       ` [PATCH v8 13/14] commit-graph: build graph from starting commits Derrick Stolee
2018-04-10 12:56       ` [PATCH v8 14/14] commit-graph: implement "--append" option Derrick Stolee

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqsh931wl1.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=dstolee@microsoft.com \
    --cc=git@jeffhostetler.com \
    --cc=git@vger.kernel.org \
    --cc=jonathantanmy@google.com \
    --cc=peff@peff.net \
    --cc=ramsay@ramsayjones.plus.com \
    --cc=sbeller@google.com \
    --cc=stolee@gmail.com \
    --cc=szeder.dev@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox