settings fs: Adding support for csi_save_start/end handlers #42028

optical-o · 2022-01-21T09:29:27Z

Prevents repeated flushing and writing of file during settings_save.

Should increase performance and reduce write cycles when used with littlefs and other fs. #34554
Note i have not tested the compression feature of this fix. I'm using modified version of this module without the compression.

Laczen · 2022-01-21T09:48:33Z

@optical-o, thanks for providing the PR.

IMO by keeping the file open it is no longer guarantee that when a settings_save_one() is succesfully finished that the data is stored. A power interrupt will create a loss of data.

optical-o · 2022-01-21T09:53:59Z

Yes you are correct. The current proposed implementation will break when used with settings_save_one(). This indeed needs to be addressed.
Then i find the api for csi very strange.

	int (*csi_save_start)(struct settings_store *cs);
	/**< Handler called before an export operation.
	 *
	 * Parameters:
	 *  - cs - Corresponding backend handler node
	 */

However i think that is a problem with the implementation of the settings_store api. The redundant writes created by this implementation are in my opinion quite serious because 14s ->200ms when saving around 200 settings options will create enormous amount of erases and writes on the flash. Which will rapidly degrade its life time.

The implementation of settings_save_one should be propably surrounded by:

		cs->cs_itf->csi_save_start(cs);
...
		cs->cs_itf->csi_save_end(cs);

Same as the settings_save is.

Laczen · 2022-01-21T10:30:24Z

Yes you are correct. The current proposed implementation will break when used with settings_save_one(). This indeed needs to be addressed. Then i find very strange the api for csi.
	int (*csi_save_start)(struct settings_store *cs);
	/**< Handler called before an export operation.
	 *
	 * Parameters:
	 *  - cs - Corresponding backend handler node
	 */
However i think that is a problem with the implementation of the settings_store api. The redundant writes created by this implementation are in my opinion quite serious because 14s ->200ms when saving around 200 settings options will create enormous amount of erases and writes on the flash. Which will rapidly degrade its life time.

The implementation of settings_save_one should be propably surrounded by:
		cs->cs_itf->csi_save_start(cs);
...
		cs->cs_itf->csi_save_end(cs);
Same as the settings_save.

Yes we could use the csi_save_start(cs) and csi_save_end(cs) to open and close the file. And this would also make sense, we know that an export will be doing a lot of writes. At the moment I don't think there is a backend that uses these routines. These routines could also be used for something completely different like changing where to store, ... (let your imagination work).

Another way to utilize these routines would be to set a boolean that avoids opening/closing the file when a settings_save_one() is called.

The extensive time savings you report would however only apply when a export is performed.

Anyhow, as it is I don't think it is an acceptable proposal.

nvlsianpu

Nice patch, thanks!

Have a few nits.
Please also follow the project commit message guide.

nvlsianpu · 2022-01-21T10:25:40Z

subsys/settings/src/settings_file.c

-			rc = rc2;
-		}
-	}
+    entry_ctx.stor_ctx = &cf->file;


please use tabs (set to 8 spaces wide) for code indentation.

nvlsianpu · 2022-01-21T10:28:22Z

subsys/settings/src/settings_file.c

@@ -484,9 +498,6 @@ static int write_handler(void *ctx, off_t off, char const *buf, size_t len)
 	struct fs_file_t *file = entry_ctx->stor_ctx;
 	int rc;

-	/* append to file only */


Can you explain this removal?

nvlsianpu · 2022-01-21T10:36:17Z

@optical-o, thanks for providing the PR.

IMO by keeping the file open it is no longer guarantee that when a settings_save_one() is succesfully finished that the data is stored. A power interrupt will create a loss of data.

fs_sync() can be used for mitigating this - for littlefs there will be no timing cost as it is strict-write FS.

Laczen · 2022-01-21T10:59:54Z

@optical-o, thanks for providing the PR.

IMO by keeping the file open it is no longer guarantee that when a settings_save_one() is succesfully finished that the data is stored. A power interrupt will create a loss of data.

fs_sync() can be used for mitigating this - for littlefs there will be no timing cost as it is strict-write FS.

If fs_sync() is used I don't mind keeping the file open to avoid opening and closing the file. There is no need to use the csi_save_start() and csi_save_end, it is just the file backend that needs to be modified to keep the file open.

optical-o · 2022-01-21T11:43:49Z

@nvlsianpu I will correct the code and prepare it for merging when the final implementation course will be clear.

optical-o · 2022-01-21T11:53:21Z

Just to clarify the full scope of this PR. I have an issue with settings_save having very poor performance when dealing with larger number of setting entries.

As of writing this i see these issues which should be resolved:

Inconsistency of csi_save_start/end usage inside settings_store.c These should be according to documentation called always when value is about to be exported. The save_settings_one does not call these, which is a issue. At least by the my opinion. Or atleast the API should then say: "these are called only when" which is always a bad design if the function is not named properly.
The actual issue regarding poor performance of the settings_fs module. Is connected to excessive file flushing of the exported values. This happens in two cases. Firstly by opening and closing the file for every exported value. Secondly by seeking to the end of the file each time a value is written into the file. (@nvlsianpu Reason behind removing the fs_seek. The file is already at the end. There is no need to seek again.)

I think that proper way of solving this is to utilize the csi_save_start and csi_save_end callback for opening and closing/syncing the file. For this however the issue no. 1 must be resolved due to the settings_save_one not utilizing the csi_save_start/end callbacks.

@Laczen I think that other solution would still require modification of the settings_store.c to signal the csi that there will be additional export after this one to prevent the immediate flushing/syncing. In case of settings_save()

Laczen · 2022-01-21T12:35:05Z

Just to clarify the full scope of this PR. I have an issue with settings_save having very poor performance when dealing with larger number of setting entries.

As of writing this i see these issues which should be resolved:

Inconsistency of csi_save_start/end usage inside settings_store.c These should be according to documentation called always when value is about to be exported. The save_settings_one does not call these, which is a issue. At least by the my opinion. Or atleast the API should then say: "these are called only when" which is always a bad design if the function is not named properly.

There is a difference between exporting and calling settings_save_one(). At the moment there is no backend registering the csi_save_start() and csi_save_end() routines. The naming is indeed not good,
they should be called csi_export_start() and csi_export_end().

The actual issue regarding poor performance of the settings_fs module. Is connected to excessive file flushing of the exported values. This happens in two cases. Firstly by opening and closing the file for every exported value. Secondly by seeking to the end of the file each time a value is written into the file. (@nvlsianpu Reason behind removing the fs_seek. The file is already at the end. There is no need to seek again.)

This is the real issue and we should focus on this. It is possible (as your PR proposes) to avoid opening and closing the settings (write) file, I am still wondering what is causing the speedup, it is not the opening and closing of the files (as this is done to check for duplicate values anyhow). It could be the fs_seek that can be avoided by keeping the file open (and definitively does not need to be called twice), or the flushing to flash (that will be need to be inserted anyhow for settings_save_one).

I think that proper way of solving this is to utilize the csi_save_start and csi_save_end callback for opening and closing/syncing the file. For this however the issue no. 1 must be resolved due to the settings_save_one not utilizing the csi_save_start/end callbacks.

@Laczen I think that other solution would still require modification of the settings_store.c to signal the csi that there will be additional export after this one to prevent the immediate flushing/syncing. In case of settings_save()

What about using the csi_save_start() to disable the flushing (do not call fs_sync()) and csi_save_end() to flush and reenable the flushing.

de-nordic · 2022-01-21T12:39:13Z

subsys/settings/src/settings_file.c

+	int rc;
+
+	fs_file_t_init(&cf->file);
+	rc = fs_open(&cf->file, cf->cf_name, FS_O_CREATE | FS_O_RDWR);


The code is always appending to the end of file so the FS_O_APPEND could probably be used here, with no need for fs_seek invocation bellow.

I'm tried to add FS_O_APPEND instead of FS_O_CREATE. The fs_open returned error -2.
I have not tried to know what was the cause. I can try it again an investigate further. It might be that the FS_O_RDWR is not supported when FS_O_APPEND is selected. Any way the RW options should by propably only WR.

I'm tried to add FS_O_APPEND instead of FS_O_CREATE. The fs_open returned error -2. I have not tried to know what was the cause. I can try it again an investigate further.

FS_O_CREATE allows fs_open to create file if such does not exist, you should use both.

Oh yes, you are correct. I will change the implementation to use the APPEND option instead of seek. That will solve the littlefs seek flush issue.
Thank you for your input

This change could be done to the existing implementation as well. Is this the cause of the slow performance?

Partially yes. This needs to be solved as well, otherwise the closing of the file(flushing) wont be effective alone.

So with fs_sync added after the write you are still getting a dramatic improvement in performance ?

optical-o · 2022-01-21T12:41:46Z

@Laczen If the naming should be changed. It that case your solution appears to be the best one.
I can implement it that way if you agree.

optical-o · 2022-01-21T13:27:24Z

I have added @Laczen basic implementation which i will need to test. Currently it is untested. However you can see the implementation using state variable syncing exactly as you proposed. Could you verify that i implemented it as you intended ?

Laczen · 2022-01-24T09:36:32Z

I have added @Laczen basic implementation which i will need to test. Currently it is untested. However you can see the implementation using state variable syncing exactly as you proposed. Could you verify that i implemented it as you intended ?

Yes, this is how I intended.

I am still wondering if the proposed change still gives the drastic improvement from 14s to 200ms?

You should anyhow still register the csi_save_start() and csi_save_end() routines as they would not be called otherwise.

optical-o · 2022-01-24T09:44:40Z

I have added @Laczen basic implementation which i will need to test. Currently it is untested. However you can see the implementation using state variable syncing exactly as you proposed. Could you verify that i implemented it as you intended ?

Yes, this is how I intended.

I am still wondering if the proposed change still gives the drastic improvement from 14s to 200ms?

You should anyhow still register the csi_save_start() and csi_save_end() routines as they would not be called otherwise.

I'm swtiching between my branch with older zephyr to this one and missing some lines. sorry about that.
Yes this gives the still the drastic improvement.

optical-o · 2022-01-24T09:46:15Z

And i have close the issue by mistake. What a day.

Laczen · 2022-01-24T10:13:41Z

I'm swtiching between my branch with older zephyr to this one and missing some lines. sorry about that. Yes this gives the still the drastic improvement.

I have a problem understanding this, since every write does a sync the fs_close should be almost a nop, the opening and closing of a file (without writing) is not the problem since this was done for the read anyhow. There is some magic going on.

optical-o · 2022-01-24T10:19:59Z

fs_close

The issue only occurs when settings_save is called. The is an large amount of settings entries into the file and every single one was flushed either by closing the file or in case of little fs fs_seek(END) the little fs in seek operation flushes the buffer.

Now when the syncing flag is off and the file is opened all the time and syncing occurs only at the end of the settings_save so the little fs can utilize its caching capability. Otherwise it would flush around ~4 time for each settings write due to the nature of settings_line write handling (for fs_seek), with the seek fixed(changed to FS_APPEND) it would flush only once per entry. However that is still inefficient when calling settings_save.

nvlsianpu

I would expect that settings_file_save_end() should call fs_sync() .

optical-o · 2022-01-24T11:45:21Z

I would expect that settings_file_save_end() should call fs_sync() .

Yes, you are correct i made a mistake i had the sync inside start() instead of end. My mistake.
I fixed it now.

Laczen · 2022-01-24T12:05:48Z

@optical-o, I am still feeling like the change is only getting a speedup from using cache. While this is allowed and would be a good addition from the settings_save() it should not be done for the settings_save_one(). If this is valid (this removes the magic) a different approach to the PR should be taken. Indeed the csi_save_start() and csi_save_end() should be used to disable any flushing in the settings_save_one() but there is no need to keep the file open during the settings_save_one(). The speedup should then only be for the case where settings_save() is called, which is in line with my expectations.

Fixes unnecessary flushes writes of settings file during settings_save call. Replace fs_seek with new fs_open option FS_O_APPEND. Signed-off-by: Tomas Benes <opt@dcw.cz>

Laczen · 2022-02-21T07:50:05Z

@optical-o, why are you closing this PR?

optical-o requested a review from nvlsianpu as a code owner January 21, 2022 09:29

github-actions bot added the area: Settings Settings subsystem label Jan 21, 2022

optical-o force-pushed the settings_fs_csi_save branch from 2d9859a to dbe94fa Compare January 21, 2022 09:30

nvlsianpu requested changes Jan 21, 2022

View reviewed changes

optical-o force-pushed the settings_fs_csi_save branch 6 times, most recently from 2c0990d to e6a834e Compare January 21, 2022 12:30

de-nordic reviewed Jan 21, 2022

View reviewed changes

optical-o force-pushed the settings_fs_csi_save branch from e6a834e to 41d6003 Compare January 21, 2022 13:25

optical-o force-pushed the settings_fs_csi_save branch 4 times, most recently from fd06b18 to 15ed8b2 Compare January 24, 2022 09:32

optical-o closed this Jan 24, 2022

optical-o reopened this Jan 24, 2022

optical-o force-pushed the settings_fs_csi_save branch 2 times, most recently from e8d752a to e7c00f5 Compare January 24, 2022 09:54

nvlsianpu reviewed Jan 24, 2022

View reviewed changes

optical-o force-pushed the settings_fs_csi_save branch 3 times, most recently from 0174fc5 to 25142c7 Compare January 24, 2022 11:57

zephyrbot assigned nvlsianpu Jan 24, 2022

optical-o force-pushed the settings_fs_csi_save branch from 25142c7 to 46b9ed8 Compare January 26, 2022 08:33

settings: file: adding support for csi_save_start/end handlers

d0a690a

Fixes unnecessary flushes writes of settings file during settings_save call. Replace fs_seek with new fs_open option FS_O_APPEND. Signed-off-by: Tomas Benes <opt@dcw.cz>

optical-o force-pushed the settings_fs_csi_save branch from 46b9ed8 to d0a690a Compare January 26, 2022 08:37

optical-o closed this Feb 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

settings fs: Adding support for csi_save_start/end handlers #42028

settings fs: Adding support for csi_save_start/end handlers #42028

optical-o commented Jan 21, 2022

Laczen commented Jan 21, 2022

optical-o commented Jan 21, 2022 •

edited

Laczen commented Jan 21, 2022

nvlsianpu left a comment

nvlsianpu Jan 21, 2022

nvlsianpu Jan 21, 2022

nvlsianpu commented Jan 21, 2022

Laczen commented Jan 21, 2022 •

edited

optical-o commented Jan 21, 2022

optical-o commented Jan 21, 2022 •

edited

Laczen commented Jan 21, 2022

de-nordic Jan 21, 2022

optical-o Jan 21, 2022 •

edited

de-nordic Jan 21, 2022

optical-o Jan 21, 2022 •

edited

Laczen Jan 21, 2022

optical-o Jan 21, 2022

Laczen Jan 21, 2022

optical-o commented Jan 21, 2022 •

edited

optical-o commented Jan 21, 2022

Laczen commented Jan 24, 2022 •

edited

optical-o commented Jan 24, 2022 •

edited

optical-o commented Jan 24, 2022

Laczen commented Jan 24, 2022

optical-o commented Jan 24, 2022 •

edited

nvlsianpu left a comment

optical-o commented Jan 24, 2022

Laczen commented Jan 24, 2022

Laczen commented Feb 21, 2022

settings fs: Adding support for csi_save_start/end handlers #42028

settings fs: Adding support for csi_save_start/end handlers #42028

Conversation

optical-o commented Jan 21, 2022

Laczen commented Jan 21, 2022

optical-o commented Jan 21, 2022 • edited

Laczen commented Jan 21, 2022

nvlsianpu left a comment

Choose a reason for hiding this comment

nvlsianpu Jan 21, 2022

Choose a reason for hiding this comment

nvlsianpu Jan 21, 2022

Choose a reason for hiding this comment

nvlsianpu commented Jan 21, 2022

Laczen commented Jan 21, 2022 • edited

optical-o commented Jan 21, 2022

optical-o commented Jan 21, 2022 • edited

Laczen commented Jan 21, 2022

de-nordic Jan 21, 2022

Choose a reason for hiding this comment

optical-o Jan 21, 2022 • edited

Choose a reason for hiding this comment

de-nordic Jan 21, 2022

Choose a reason for hiding this comment

optical-o Jan 21, 2022 • edited

Choose a reason for hiding this comment

Laczen Jan 21, 2022

Choose a reason for hiding this comment

optical-o Jan 21, 2022

Choose a reason for hiding this comment

Laczen Jan 21, 2022

Choose a reason for hiding this comment

optical-o commented Jan 21, 2022 • edited

optical-o commented Jan 21, 2022

Laczen commented Jan 24, 2022 • edited

optical-o commented Jan 24, 2022 • edited

optical-o commented Jan 24, 2022

Laczen commented Jan 24, 2022

optical-o commented Jan 24, 2022 • edited

nvlsianpu left a comment

Choose a reason for hiding this comment

optical-o commented Jan 24, 2022

Laczen commented Jan 24, 2022

Laczen commented Feb 21, 2022

optical-o commented Jan 21, 2022 •

edited

Laczen commented Jan 21, 2022 •

edited

optical-o commented Jan 21, 2022 •

edited

optical-o Jan 21, 2022 •

edited

optical-o Jan 21, 2022 •

edited

optical-o commented Jan 21, 2022 •

edited

Laczen commented Jan 24, 2022 •

edited

optical-o commented Jan 24, 2022 •

edited

optical-o commented Jan 24, 2022 •

edited