| Message ID | 878bffcdfe5ca7657f839de8f7993d9098726636.1606545878.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Minimal patches to let reftable pass the CI builds | expand |
On Sat, Nov 28, 2020 at 7:44 AM Johannes Schindelin via GitGitGadget <gitgitgadget@gmail.com> wrote: > > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > Close the file descriptors to obsolete files before trying to delete or > rename them. This is actually required on Windows. > > Note: this patch is just a band-aid to get the tests pass on Windows. > The fact that it is needed raises concerns about the overall resource > handling: are file descriptors closed properly whenever appropriate, or > are they closed much later (which can lead to rename() problems on > Windows, and risks running into ulimits)? > > Also, a `reftable_stack_destroy()` call had to be moved in > `test_reftable_stack_uptodate()` to avoid the prompt complaining that a > `.ref` file could not be deleted on Windows. This raises the question > whether the code does the right thing when two concurrent processes want > to access the reftable, and one wants to compact it. At the moment, it > does not appear to fail gracefully. Thanks for the report; I have to look more closely at these fixes; I fear they might be incorrect. The reftable spec doesn't treat this case in depth, and I think it was rather written for Unix-like semantics. In the Unix flavor, a process that wants to read can keep file descriptors open to keep reading from the ref DB at a consistent snapshot. What is the approach that the rest of Git on Windows takes in these circumstances? Consider processes P1 and P2, and the following sequence of actions P1 opens ref DB (ie. opens a set of *.ref files for read) P2 opens ref DB, executes a transaction. Post-transaction, it compacts the reftable stack. P2 exits P1 exits Currently, the compaction in P2 tries to delete the files obviated by the compaction. On Windows this currently fails, because you can't delete open files. There are several options: 1) P2 should fail the compaction. This is bad because it will lead to degraded performance over time, and it's not obvious if you can anticipate that the deletion doesn't work. 2) P2 should retry deleting until it succeeds. This is bad, because a reader can starve writers. 3) on exit, P1 should check if its *.ref files are still in use, and delete them. This smells bad, because P1 is a read-only process, yet it executes writes. Also, do we have good on-exit hooks in Git? 4) On exit, P1 does nothing. Stale *.ref files are left behind. Some sort of GC process cleans things up asynchronously. 5) The ref DB should not keep files open, and should rather open and close files as needed; this means P1 doesn't keep files open for long, and P2 can retry safely. I think 3) seems the cleanest to me (even though deleting in read process feels weird), but perhaps we could fallback to 5) on windows as well. Thoughts? What errno code does deleting an in-use file on Windows produce? > Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de> > --- > reftable/stack.c | 37 ++++++++++++++++++++++++++++--------- > reftable/stack_test.c | 2 +- > 2 files changed, 29 insertions(+), 10 deletions(-) > > diff --git a/reftable/stack.c b/reftable/stack.c > index 1d632937d7..02c6a370ba 100644 > --- a/reftable/stack.c > +++ b/reftable/stack.c > @@ -212,7 +212,6 @@ static int reftable_stack_reload_once(struct reftable_stack *st, char **names, > goto done; > > new_tables = NULL; > - st->readers_len = new_readers_len; > if (st->merged != NULL) { > merged_table_release(st->merged); > reftable_merged_table_free(st->merged); > @@ -220,6 +219,7 @@ static int reftable_stack_reload_once(struct reftable_stack *st, char **names, > if (st->readers != NULL) { > reftable_free(st->readers); > } > + st->readers_len = new_readers_len; > st->readers = new_readers; > new_readers = NULL; > new_readers_len = 0; > @@ -939,14 +939,6 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last, > strbuf_addstr(&new_table_path, "/"); > strbuf_addbuf(&new_table_path, &new_table_name); > > - if (!is_empty_table) { > - err = rename(temp_tab_file_name.buf, new_table_path.buf); > - if (err < 0) { > - err = REFTABLE_IO_ERROR; > - goto done; > - } > - } > - > for (i = 0; i < first; i++) { > strbuf_addstr(&ref_list_contents, st->readers[i]->name); > strbuf_addstr(&ref_list_contents, "\n"); > @@ -960,6 +952,32 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last, > strbuf_addstr(&ref_list_contents, "\n"); > } > > + /* > + * Now release the merged tables and readers > + */ > + if (st->merged != NULL) { > + reftable_merged_table_free(st->merged); > + st->merged = NULL; > + } > + > + if (st->readers != NULL) { > + int i = 0; > + for (i = 0; i < st->readers_len; i++) { > + reader_close(st->readers[i]); > + reftable_reader_free(st->readers[i]); > + } > + st->readers_len = 0; > + FREE_AND_NULL(st->readers); > + } > + > + if (!is_empty_table) { > + err = rename(temp_tab_file_name.buf, new_table_path.buf); > + if (err < 0) { > + err = REFTABLE_IO_ERROR; > + goto done; > + } > + } > + > err = write(lock_file_fd, ref_list_contents.buf, ref_list_contents.len); > if (err < 0) { > err = REFTABLE_IO_ERROR; > @@ -1242,6 +1260,7 @@ static int stack_check_addition(struct reftable_stack *st, > > free(refs); > reftable_iterator_destroy(&it); > + reader_close(rd); > reftable_reader_free(rd); > return err; > } > diff --git a/reftable/stack_test.c b/reftable/stack_test.c > index 11d3d30799..c35abd7301 100644 > --- a/reftable/stack_test.c > +++ b/reftable/stack_test.c > @@ -159,12 +159,12 @@ static void test_reftable_stack_uptodate(void) > err = reftable_stack_add(st2, &write_test_ref, &ref2); > EXPECT(err == REFTABLE_LOCK_ERROR); > > + reftable_stack_destroy(st1); > err = reftable_stack_reload(st2); > EXPECT_ERR(err); > > err = reftable_stack_add(st2, &write_test_ref, &ref2); > EXPECT_ERR(err); > - reftable_stack_destroy(st1); > reftable_stack_destroy(st2); > clear_dir(dir); > } > -- > gitgitgadget >
Hi Han-Wen, On Tue, 1 Dec 2020, Han-Wen Nienhuys wrote: > On Sat, Nov 28, 2020 at 7:44 AM Johannes Schindelin via GitGitGadget > <gitgitgadget@gmail.com> wrote: > > > > From: Johannes Schindelin <johannes.schindelin@gmx.de> > > > > Close the file descriptors to obsolete files before trying to delete or > > rename them. This is actually required on Windows. > > > > Note: this patch is just a band-aid to get the tests pass on Windows. > > The fact that it is needed raises concerns about the overall resource > > handling: are file descriptors closed properly whenever appropriate, or > > are they closed much later (which can lead to rename() problems on > > Windows, and risks running into ulimits)? > > > > Also, a `reftable_stack_destroy()` call had to be moved in > > `test_reftable_stack_uptodate()` to avoid the prompt complaining that a > > `.ref` file could not be deleted on Windows. This raises the question > > whether the code does the right thing when two concurrent processes want > > to access the reftable, and one wants to compact it. At the moment, it > > does not appear to fail gracefully. > > Thanks for the report; I have to look more closely at these fixes; I > fear they might be incorrect. They might be incorrect, but less so than the previous state, as testified by the previously failing PR build. > The reftable spec doesn't treat this case in depth, and I think it was > rather written for Unix-like semantics. In the Unix flavor, a process > that wants to read can keep file descriptors open to keep reading from > the ref DB at a consistent snapshot. Thanks for the explanation. I actually knew that. > What is the approach that the rest of Git on Windows takes in these > circumstances? The rest of Git (whether on Windows or not) treats this as a no-no. You cannot keep a handle open to a file that is deleted. > Consider processes P1 and P2, and the following sequence of actions > > P1 opens ref DB (ie. opens a set of *.ref files for read) > P2 opens ref DB, executes a transaction. Post-transaction, it compacts > the reftable stack. > P2 exits > P1 exits > > Currently, the compaction in P2 tries to delete the files obviated by > the compaction. On Windows this currently fails, because you can't > delete open files. Indeed. So the design needs to be fixed, if it fails. > There are several options: > > 1) P2 should fail the compaction. This is bad because it will lead to > degraded performance over time, and it's not obvious if you can > anticipate that the deletion doesn't work. > 2) P2 should retry deleting until it succeeds. This is bad, because a > reader can starve writers. > 3) on exit, P1 should check if its *.ref files are still in use, and > delete them. This smells bad, because P1 is a read-only process, yet > it executes writes. Also, do we have good on-exit hooks in Git? > 4) On exit, P1 does nothing. Stale *.ref files are left behind. Some > sort of GC process cleans things up asynchronously. > 5) The ref DB should not keep files open, and should rather open and > close files as needed; this means P1 doesn't keep files open for long, > and P2 can retry safely. > > I think 3) seems the cleanest to me (even though deleting in read > process feels weird), but perhaps we could fallback to 5) on windows > as well. Traditionally, Git would fail gracefully (i.e. with a warning) to delete the stale files, and try again at a later stage (during `git gc --auto`, for example, or after the next compaction step). > What errno code does deleting an in-use file on Windows produce? I believe it would be `EACCES`. See https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/unlink-wunlink?view=msvc-160 for the documented behavior (I believe that an in-use file is treated the same way as a read-only file in this instance). Ciao, Dscho
On Wed, Dec 2, 2020 at 1:32 PM Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote: > > Consider processes P1 and P2, and the following sequence of actions > > > > P1 opens ref DB (ie. opens a set of *.ref files for read) > > P2 opens ref DB, executes a transaction. Post-transaction, it compacts > > the reftable stack. > > P2 exits > > P1 exits > > > > Currently, the compaction in P2 tries to delete the files obviated by > > the compaction. On Windows this currently fails, because you can't > > delete open files. > > Indeed. So the design needs to be fixed, if it fails. > > > There are several options: > > > > 1) P2 should fail the compaction. This is bad because it will lead to > > degraded performance over time, and it's not obvious if you can > > anticipate that the deletion doesn't work. > > 2) P2 should retry deleting until it succeeds. This is bad, because a > > reader can starve writers. > > 3) on exit, P1 should check if its *.ref files are still in use, and > > delete them. This smells bad, because P1 is a read-only process, yet > > it executes writes. Also, do we have good on-exit hooks in Git? > > 4) On exit, P1 does nothing. Stale *.ref files are left behind. Some > > sort of GC process cleans things up asynchronously. > > 5) The ref DB should not keep files open, and should rather open and > > close files as needed; this means P1 doesn't keep files open for long, > > and P2 can retry safely. > > > > I think 3) seems the cleanest to me (even though deleting in read > > process feels weird), but perhaps we could fallback to 5) on windows > > as well. > > Traditionally, Git would fail gracefully (i.e. with a warning) to delete > the stale files, and try again at a later stage (during `git gc --auto`, > for example, or after the next compaction step). So, how does this sound: * add a void set_warn_handler(void(*handler)(char const*)) The handler is called for non-fatal errors (eg. deleting an in-use .ref file during compaction), and can provide the warning. * all .ref files will be prefixed with an 8-byte random string, to avoid that new *.ref files collide with unreferenced, stale *.ref files. * in reftable_stack_close(), after closing files, we check if *.ref files we were reading have gone out of use. If so, delete those .ref files, printing a warning on EACCESS. * an on-exit handler in git calls reftable_stack_close() * provide a reftable_stack_gc(), which loads tables.list and then deletes all unused *.ref files that are older than tables.list. > > What errno code does deleting an in-use file on Windows produce? > > I believe it would be `EACCES`. See > https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/unlink-wunlink?view=msvc-160 > for the documented behavior (I believe that an in-use file is treated the > same way as a read-only file in this instance). your commit message says "to avoid the prompt complaining that a `.ref` file could not be deleted on Windows." AFAICT, this prompt is coming from windows itself, because the reftable library doesn't issue this type of warning. Is there a way to delete a file that just returns EACCESS if it fails, without triggering a prompt? Or is the prompt part of the "failing gracefully" behavior?
On Wed, Dec 02 2020, Han-Wen Nienhuys wrote: > On Wed, Dec 2, 2020 at 1:32 PM Johannes Schindelin > <Johannes.Schindelin@gmx.de> wrote: >> > Consider processes P1 and P2, and the following sequence of actions >> > >> > P1 opens ref DB (ie. opens a set of *.ref files for read) >> > P2 opens ref DB, executes a transaction. Post-transaction, it compacts >> > the reftable stack. >> > P2 exits >> > P1 exits >> > >> > Currently, the compaction in P2 tries to delete the files obviated by >> > the compaction. On Windows this currently fails, because you can't >> > delete open files. >> >> Indeed. So the design needs to be fixed, if it fails. >> >> > There are several options: >> > >> > 1) P2 should fail the compaction. This is bad because it will lead to >> > degraded performance over time, and it's not obvious if you can >> > anticipate that the deletion doesn't work. >> > 2) P2 should retry deleting until it succeeds. This is bad, because a >> > reader can starve writers. >> > 3) on exit, P1 should check if its *.ref files are still in use, and >> > delete them. This smells bad, because P1 is a read-only process, yet >> > it executes writes. Also, do we have good on-exit hooks in Git? >> > 4) On exit, P1 does nothing. Stale *.ref files are left behind. Some >> > sort of GC process cleans things up asynchronously. >> > 5) The ref DB should not keep files open, and should rather open and >> > close files as needed; this means P1 doesn't keep files open for long, >> > and P2 can retry safely. >> > >> > I think 3) seems the cleanest to me (even though deleting in read >> > process feels weird), but perhaps we could fallback to 5) on windows >> > as well. >> >> Traditionally, Git would fail gracefully (i.e. with a warning) to delete >> the stale files, and try again at a later stage (during `git gc --auto`, >> for example, or after the next compaction step). > > So, how does this sound: > > * add a void > > set_warn_handler(void(*handler)(char const*)) > > The handler is called for non-fatal errors (eg. deleting an in-use > .ref file during compaction), and can provide the warning. > > * all .ref files will be prefixed with an 8-byte random string, to > avoid that new *.ref files collide with unreferenced, stale *.ref > files. Just trying to follow along here. Do you mean prefix the file name or the content of those *.ref files? In any case isn't this synonymous with proposing moving beyond reftable v1 (to the WIP v2, or a v1.1 with just this change?). The current spec seems to preclude prefixing either the file content or filename, but maybe I'm misreading it: https://googlers.googlesource.com/sop/jgit/+/reftable/Documentation/technical/reftable.md#update-transactions https://googlers.googlesource.com/sop/jgit/+/reftable/Documentation/technical/reftable.md#header When the reftable format was initially being discussed I suggested somewhat flippantly (why not SQLite?). Partially because in trying to invent what's essentially a portable and concurrently updated database format we'd be likely to reinvent much of the same wheel. So not to drag that whole thing up again as a proposal for a format replacement, but I wonder what SQLite would do in this scenario & others where desired DB semantics / transactions and FS/portability semantics clash. AFAIK the reftable has only been battle-tested in production under JGit so far (presumably Linux-only). Whereas SQLite has been ported to and widely used on Windows, HP/UX and probably other systems where Linux-specific assumptions don't apply. > * in reftable_stack_close(), after closing files, we check if *.ref > files we were reading have gone out of use. If so, delete those .ref > files, printing a warning on EACCESS. > > * an on-exit handler in git calls reftable_stack_close() > > * provide a reftable_stack_gc(), which loads tables.list and then > deletes all unused *.ref files that are older than tables.list. > >> > What errno code does deleting an in-use file on Windows produce? >> >> I believe it would be `EACCES`. See >> https://docs.microsoft.com/en-us/cpp/c-runtime-library/reference/unlink-wunlink?view=msvc-160 >> for the documented behavior (I believe that an in-use file is treated the >> same way as a read-only file in this instance). > > your commit message says "to avoid the prompt complaining that a > `.ref` file could not be deleted on Windows." AFAICT, this prompt is > coming from windows itself, because the reftable library doesn't issue > this type of warning. Is there a way to delete a file that just > returns EACCESS if it fails, without triggering a prompt? Or is the > prompt part of the "failing gracefully" behavior? > > -- > Han-Wen Nienhuys - Google Munich > I work 80%. Don't expect answers from me on Fridays.
On Thu, Dec 3, 2020 at 1:24 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote: > >> Indeed. So the design needs to be fixed, if it fails. > >> > >> > There are several options: > >> > > >> > 1) P2 should fail the compaction. This is bad because it will lead to > >> > degraded performance over time, and it's not obvious if you can > >> > anticipate that the deletion doesn't work. > >> > 2) P2 should retry deleting until it succeeds. This is bad, because a > >> > reader can starve writers. > >> > 3) on exit, P1 should check if its *.ref files are still in use, and > >> > delete them. This smells bad, because P1 is a read-only process, yet > >> > it executes writes. Also, do we have good on-exit hooks in Git? > >> > 4) On exit, P1 does nothing. Stale *.ref files are left behind. Some > >> > sort of GC process cleans things up asynchronously. > >> > 5) The ref DB should not keep files open, and should rather open and > >> > close files as needed; this means P1 doesn't keep files open for long, > >> > and P2 can retry safely. > >> > > >> > I think 3) seems the cleanest to me (even though deleting in read > >> > process feels weird), but perhaps we could fallback to 5) on windows > >> > as well. > >> > >> Traditionally, Git would fail gracefully (i.e. with a warning) to delete > >> the stale files, and try again at a later stage (during `git gc --auto`, > >> for example, or after the next compaction step). > > > > So, how does this sound: > > > > * add a void > > > > set_warn_handler(void(*handler)(char const*)) > > > > The handler is called for non-fatal errors (eg. deleting an in-use > > .ref file during compaction), and can provide the warning. > > > > * all .ref files will be prefixed with an 8-byte random string, to > > avoid that new *.ref files collide with unreferenced, stale *.ref > > files. > > Just trying to follow along here. Do you mean prefix the file name or > the content of those *.ref files? In any case isn't this synonymous with > proposing moving beyond reftable v1 (to the WIP v2, or a v1.1 with just > this change?). The current spec seems to preclude prefixing either the > file content or filename, but maybe I'm misreading it: This is about prefixing the filename with an uniquifying prefix. This is because the range of timestamps encompassed by the table doesn't uniquely identify the table. Consider the following case: State: 1 table "00001.ref" Transaction 1: add ref xyz at timestamp 2 add ref abc at timestamp 2 => 00002.ref Transaction 2: add delete ref xyz at timestamp 3 => 00003.ref On compacting 00002 and 00003 together, you'll end up with a table for only "ref abc at timestamp 2". If you call that result 00002.ref again, there are two versions 00002.ref. In the current spec with unix semantics, that's not a problem, because out of date tables are removed, and disappear once the last reader closes the file, but if we have to cater for a system where removal that doesn't always work, we'd have to fix up things afterwards. That means we have to be able to tell apart these two versions of 00002.ref, and this is where a random prefix would help. The exact naming of the tables is not central to the spec, as the table names to read are listed in tables.list, and systems can write filenames as they please. Maybe recommendations around filenames should be phrased as a "SHOULD" clause, with a "MUST" that each table has a globally unique name. > https://googlers.googlesource.com/sop/jgit/+/reftable/Documentation/technical/reftable.md#update-transactions > https://googlers.googlesource.com/sop/jgit/+/reftable/Documentation/technical/reftable.md#header > > When the reftable format was initially being discussed I suggested > somewhat flippantly (why not SQLite?). Partially because in trying to > invent what's essentially a portable and concurrently updated database > format we'd be likely to reinvent much of the same wheel. > > So not to drag that whole thing up again as a proposal for a format > replacement, but I wonder what SQLite would do in this scenario & others > where desired DB semantics / transactions and FS/portability semantics > clash. AFAIK the reftable has only been battle-tested in production > under JGit so far (presumably Linux-only). Whereas SQLite has been > ported to and widely used on Windows, HP/UX and probably other systems > where Linux-specific assumptions don't apply. I think it would be an interesting exercise to build a ref database on SQLite, and I wish it had been done before, because large part of the work of getting reftable to work with git proper, is disentangling the code with assumptions about the underlying ref storage system. I agree that some of this is reinventing the wheel, and I wish that reftable had been based on another format (say, LevelDB) to get away from bit mangling, but I think SQlite is a tricky proposition, because SQLite is a system with considerable complexity of its own. You'd force all the other Git implementations (libgit2, Microsoft's C#, go-git, JGit, Dulwhich etc.) to find a way to implement SQLite support. That will be a difficult proposition for the implementations that try to avoid FFI. You could also look at this from the positive side, which is that the transaction semantics of reftable are much easier to understand and verify than those of the files backend. -- Han-Wen Nienhuys - Google Munich I work 80%. Don't expect answers from me on Fridays. -- Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Paul Manicle, Halimah DeLaine Prado -- Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Paul Manicle, Halimah DeLaine Prado
diff --git a/reftable/stack.c b/reftable/stack.c index 1d632937d7..02c6a370ba 100644 --- a/reftable/stack.c +++ b/reftable/stack.c @@ -212,7 +212,6 @@ static int reftable_stack_reload_once(struct reftable_stack *st, char **names, goto done; new_tables = NULL; - st->readers_len = new_readers_len; if (st->merged != NULL) { merged_table_release(st->merged); reftable_merged_table_free(st->merged); @@ -220,6 +219,7 @@ static int reftable_stack_reload_once(struct reftable_stack *st, char **names, if (st->readers != NULL) { reftable_free(st->readers); } + st->readers_len = new_readers_len; st->readers = new_readers; new_readers = NULL; new_readers_len = 0; @@ -939,14 +939,6 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last, strbuf_addstr(&new_table_path, "/"); strbuf_addbuf(&new_table_path, &new_table_name); - if (!is_empty_table) { - err = rename(temp_tab_file_name.buf, new_table_path.buf); - if (err < 0) { - err = REFTABLE_IO_ERROR; - goto done; - } - } - for (i = 0; i < first; i++) { strbuf_addstr(&ref_list_contents, st->readers[i]->name); strbuf_addstr(&ref_list_contents, "\n"); @@ -960,6 +952,32 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last, strbuf_addstr(&ref_list_contents, "\n"); } + /* + * Now release the merged tables and readers + */ + if (st->merged != NULL) { + reftable_merged_table_free(st->merged); + st->merged = NULL; + } + + if (st->readers != NULL) { + int i = 0; + for (i = 0; i < st->readers_len; i++) { + reader_close(st->readers[i]); + reftable_reader_free(st->readers[i]); + } + st->readers_len = 0; + FREE_AND_NULL(st->readers); + } + + if (!is_empty_table) { + err = rename(temp_tab_file_name.buf, new_table_path.buf); + if (err < 0) { + err = REFTABLE_IO_ERROR; + goto done; + } + } + err = write(lock_file_fd, ref_list_contents.buf, ref_list_contents.len); if (err < 0) { err = REFTABLE_IO_ERROR; @@ -1242,6 +1260,7 @@ static int stack_check_addition(struct reftable_stack *st, free(refs); reftable_iterator_destroy(&it); + reader_close(rd); reftable_reader_free(rd); return err; } diff --git a/reftable/stack_test.c b/reftable/stack_test.c index 11d3d30799..c35abd7301 100644 --- a/reftable/stack_test.c +++ b/reftable/stack_test.c @@ -159,12 +159,12 @@ static void test_reftable_stack_uptodate(void) err = reftable_stack_add(st2, &write_test_ref, &ref2); EXPECT(err == REFTABLE_LOCK_ERROR); + reftable_stack_destroy(st1); err = reftable_stack_reload(st2); EXPECT_ERR(err); err = reftable_stack_add(st2, &write_test_ref, &ref2); EXPECT_ERR(err); - reftable_stack_destroy(st1); reftable_stack_destroy(st2); clear_dir(dir); }