{"thread":{"id":"60814","subject":"[PATCH] reftable/stack: fsync \"tables.list\" during compaction","startedAt":"2024-01-30T05:22:54Z","lastAt":"2024-01-31T05:46:44Z","messageCount":3,"participants":["Patrick Steinhardt","Justin Tobler"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"487575","messageId":"7bdafc9bd7f53f38a24d69a563615b6ad484e1ba.1706592127.git.ps@pks.im","threadId":"60814","inReplyTo":null,"subject":"[PATCH] reftable/stack: fsync \"tables.list\" during compaction","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-30T05:22:47Z","receivedAt":"2024-01-30T05:22:54Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In 1df18a1c9a (reftable: honor core.fsync, 2024-01-23), we have added\ncode to fsync both newly written reftables as well as \"tables.list\" to\ndisk. But there are two code paths where \"tables.list\" is being written:\n\n  - When appending a new table due to a normal ref update.\n\n  - When compacting a range of tables during compaction.\n\nWe have only addressed the former code path, but do not yet sync the new\n\"tables.list\" file in the latter. Fix this ommission.\n\nNote that we are not yet adding any tests. These tests will be added\nonce the \"reftable\" backend has been upstreamed.\n\nSigned-off-by: Patrick Steinhardt <ps@pks.im>\n---\n reftable/stack.c | 8 ++++++++\n 1 file changed, 8 insertions(+)\n\ndiff --git a/reftable/stack.c b/reftable/stack.c\nindex ab295341cc..01d05933f6 100644\n--- a/reftable/stack.c\n+++ b/reftable/stack.c\n@@ -1018,6 +1018,14 @@ static int stack_compact_range(struct reftable_stack *st, int first, int last,\n \t\tunlink(new_table_path.buf);\n \t\tgoto done;\n \t}\n+\n+\terr = fsync_component(FSYNC_COMPONENT_REFERENCE, lock_file_fd);\n+\tif (err < 0) {\n+\t\terr = REFTABLE_IO_ERROR;\n+\t\tunlink(new_table_path.buf);\n+\t\tgoto done;\n+\t}\n+\n \terr = close(lock_file_fd);\n \tlock_file_fd = -1;\n \tif (err < 0) {\n-- \n2.43.GIT\n\n"},{"id":"487607","messageId":"CAGAWz+4wyd13UYRH3ZSSZEq1Y=5HE_p+qaXeOJS-ENMpWoLabA@mail.gmail.com","threadId":"60814","inReplyTo":"7bdafc9bd7f53f38a24d69a563615b6ad484e1ba.1706592127.git.ps@pks.im","subject":"Re: [PATCH] reftable/stack: fsync \"tables.list\" during compaction","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2024-01-30T19:04:46Z","receivedAt":"2024-01-30T19:05:01Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On Mon, Jan 29, 2024 at 11:23 PM Patrick Steinhardt <ps@pks.im> wrote:\n>\n> In 1df18a1c9a (reftable: honor core.fsync, 2024-01-23), we have added\n> code to fsync both newly written reftables as well as \"tables.list\" to\n> disk. But there are two code paths where \"tables.list\" is being written:\n>\n>   - When appending a new table due to a normal ref update.\n>\n>   - When compacting a range of tables during compaction.\n>\n> We have only addressed the former code path, but do not yet sync the new\n> \"tables.list\" file in the latter. Fix this ommission.\n\nnit: s/ommission/omission\n\n>\n> Note that we are not yet adding any tests. These tests will be added\n> once the \"reftable\" backend has been upstreamed.\n>\n\nNice catch! I noticed a small typo in the commit message but otherwise looks\ngood to me.\n\n-Justin\n"},{"id":"487628","messageId":"ZbnevPIN5cudLU5w@tanuki","threadId":"60814","inReplyTo":"CAGAWz+4wyd13UYRH3ZSSZEq1Y=5HE_p+qaXeOJS-ENMpWoLabA@mail.gmail.com","subject":"Re: [PATCH] reftable/stack: fsync \"tables.list\" during compaction","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-01-31T05:46:36Z","receivedAt":"2024-01-31T05:46:44Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Jan 30, 2024 at 01:04:46PM -0600, Justin Tobler wrote:\n> On Mon, Jan 29, 2024 at 11:23 PM Patrick Steinhardt <ps@pks.im> wrote:\n> >\n> > In 1df18a1c9a (reftable: honor core.fsync, 2024-01-23), we have added\n> > code to fsync both newly written reftables as well as \"tables.list\" to\n> > disk. But there are two code paths where \"tables.list\" is being written:\n> >\n> >   - When appending a new table due to a normal ref update.\n> >\n> >   - When compacting a range of tables during compaction.\n> >\n> > We have only addressed the former code path, but do not yet sync the new\n> > \"tables.list\" file in the latter. Fix this ommission.\n> \n> nit: s/ommission/omission\n\nI knew this looked weird when writing it! Should've looked it up.\n\n> > Note that we are not yet adding any tests. These tests will be added\n> > once the \"reftable\" backend has been upstreamed.\n> >\n> \n> Nice catch! I noticed a small typo in the commit message but otherwise looks\n> good to me.\n\nThanks. I'll refrain from sending out a new version for this typo alone,\nbut will fix it if other feedback requires a second iteration.\n\nPatrick\n"}]}