| Message ID | 3b81d8f5aeffb73a32b0bff0da947f023a3df517.1646905589.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | A design for future-proofing fsync() configuration | expand |
Patrick Steinhardt <ps@pks.im> writes: > diff --git a/refs/packed-backend.c b/refs/packed-backend.c > index 27dd8c3922..32d6635969 100644 > --- a/refs/packed-backend.c > +++ b/refs/packed-backend.c > @@ -1262,7 +1262,8 @@ static int write_with_updates(struct packed_ref_store *refs, > goto error; > } > > - if (close_tempfile_gently(refs->tempfile)) { > + if (fsync_component(FSYNC_COMPONENT_PACKED_REFS, get_tempfile_fd(refs->tempfile)) || > + close_tempfile_gently(refs->tempfile)) { > strbuf_addf(err, "error closing file %s: %s", > get_tempfile_path(refs->tempfile), > strerror(errno)); I do not necessarily agree with the organization to have it as a component that is separate from other ref backends, but it is very pleasing to see that there is only one fsync necessary for the packed backend. Nice.
On Thu, Mar 10, 2022 at 10:28:57AM -0800, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > > diff --git a/refs/packed-backend.c b/refs/packed-backend.c > > index 27dd8c3922..32d6635969 100644 > > --- a/refs/packed-backend.c > > +++ b/refs/packed-backend.c > > @@ -1262,7 +1262,8 @@ static int write_with_updates(struct packed_ref_store *refs, > > goto error; > > } > > > > - if (close_tempfile_gently(refs->tempfile)) { > > + if (fsync_component(FSYNC_COMPONENT_PACKED_REFS, get_tempfile_fd(refs->tempfile)) || > > + close_tempfile_gently(refs->tempfile)) { > > strbuf_addf(err, "error closing file %s: %s", > > get_tempfile_path(refs->tempfile), > > strerror(errno)); > > I do not necessarily agree with the organization to have it as a > component that is separate from other ref backends, but it is > very pleasing to see that there is only one fsync necessary for the > packed backend. > > Nice. I was mostly adapting to the precedent set by Neeraj, where we also distinguish loose objects and packed objects. Personally I don't mind much whether we want to discern those two cases, and I'd be happy to just merge them into a single "refs" knob. We can still split these up at a later point if the need ever arises. Patrick
diff --git a/Documentation/config/core.txt b/Documentation/config/core.txt index b67d3c340e..3fd466f955 100644 --- a/Documentation/config/core.txt +++ b/Documentation/config/core.txt @@ -565,9 +565,10 @@ core.fsync:: * `commit-graph` hardens the commit graph file. * `index` hardens the index when it is modified. * `loose-ref` hardens references modified in the repo in loose-ref form. +* `packed-refs` hardens references modified in the repo in packed-refs form. * `objects` is an aggregate option that is equivalent to `loose-object,pack`. -* `refs` is an aggregate option that is equivalent to `loose-ref`. +* `refs` is an aggregate option that is equivalent to `loose-ref,packed-refs`. * `derived-metadata` is an aggregate option that is equivalent to `pack-metadata,commit-graph`. * `default` is an aggregate option that is equivalent to diff --git a/cache.h b/cache.h index b56a56f539..9b7c282fa5 100644 --- a/cache.h +++ b/cache.h @@ -1006,12 +1006,14 @@ enum fsync_component { FSYNC_COMPONENT_COMMIT_GRAPH = 1 << 3, FSYNC_COMPONENT_INDEX = 1 << 4, FSYNC_COMPONENT_LOOSE_REF = 1 << 5, + FSYNC_COMPONENT_PACKED_REFS = 1 << 6, }; #define FSYNC_COMPONENTS_OBJECTS (FSYNC_COMPONENT_LOOSE_OBJECT | \ FSYNC_COMPONENT_PACK) -#define FSYNC_COMPONENTS_REFS (FSYNC_COMPONENT_LOOSE_REF) +#define FSYNC_COMPONENTS_REFS (FSYNC_COMPONENT_LOOSE_REF | \ + FSYNC_COMPONENT_PACKED_REFS) #define FSYNC_COMPONENTS_DERIVED_METADATA (FSYNC_COMPONENT_PACK_METADATA | \ FSYNC_COMPONENT_COMMIT_GRAPH) @@ -1030,7 +1032,8 @@ enum fsync_component { FSYNC_COMPONENT_PACK_METADATA | \ FSYNC_COMPONENT_COMMIT_GRAPH | \ FSYNC_COMPONENT_INDEX | \ - FSYNC_COMPONENT_LOOSE_REF) + FSYNC_COMPONENT_LOOSE_REF | \ + FSYNC_COMPONENT_PACKED_REFS) /* * A bitmask indicating which components of the repo should be fsynced. diff --git a/config.c b/config.c index b5d3e6e404..b4a2ee3a8c 100644 --- a/config.c +++ b/config.c @@ -1333,6 +1333,7 @@ static const struct fsync_component_entry { { "commit-graph", FSYNC_COMPONENT_COMMIT_GRAPH }, { "index", FSYNC_COMPONENT_INDEX }, { "loose-ref", FSYNC_COMPONENT_LOOSE_REF }, + { "packed-refs", FSYNC_COMPONENT_PACKED_REFS }, { "objects", FSYNC_COMPONENTS_OBJECTS }, { "refs", FSYNC_COMPONENTS_REFS }, { "derived-metadata", FSYNC_COMPONENTS_DERIVED_METADATA }, diff --git a/refs/packed-backend.c b/refs/packed-backend.c index 27dd8c3922..32d6635969 100644 --- a/refs/packed-backend.c +++ b/refs/packed-backend.c @@ -1262,7 +1262,8 @@ static int write_with_updates(struct packed_ref_store *refs, goto error; } - if (close_tempfile_gently(refs->tempfile)) { + if (fsync_component(FSYNC_COMPONENT_PACKED_REFS, get_tempfile_fd(refs->tempfile)) || + close_tempfile_gently(refs->tempfile)) { strbuf_addf(err, "error closing file %s: %s", get_tempfile_path(refs->tempfile), strerror(errno));
Similar to the preceding commit, this commit adds a new option to harden packed references so that users and admins can avoid data loss when we commit a new packed-refs file. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- Documentation/config/core.txt | 3 ++- cache.h | 7 +++++-- config.c | 1 + refs/packed-backend.c | 3 ++- 4 files changed, 10 insertions(+), 4 deletions(-)