Settings filter duplicates

[rakons]

This PR adds an option to filter duplicates when direct loading data from file and fcb backend.
The average complexity should be N ^ sqrt(2), thanks to the fact that not all the data is read on every entry, but only all the data left.

[zephyrbot]

All checks are passing now.

checkpatch (informational only, not a failure)

-:752: WARNING:LONG_LINE: line over 80 characters
#752: FILE: tests/subsys/settings/functional/src/settings_basic_test.c:511:
+	zassert_equal(strlen(ldata->v) + 1, len, "e: \"%s\", a:\"%s\"", ldata->v, buf);

-:758: WARNING:LONG_LINE: line over 80 characters
#758: FILE: tests/subsys/settings/functional/src/settings_basic_test.c:517:
+	zassert_false(strcmp(ldata->v, buf), "e: \"%s\", a:\"%s\"", ldata->v, buf);

- total: 0 errors, 2 warnings, 774 lines checked

NOTE: For some of the reported defects, checkpatch may be able to
      mechanically convert to the typical style using --fix or --fix-inplace.

Your patch has style problems, please review.

NOTE: Ignored message types: AVOID_EXTERNS BRACES CONFIG_EXPERIMENTAL CONST_STRUCT DATE_TIME FILE_PATH_CHANGES MINMAX NETWORKING_BLOCK_COMMENT_STYLE PRINTK_WITHOUT_KERN_LEVEL SPLIT_STRING VOLATILE

NOTE: If any of the errors are false positives, please report
      them to the maintainers.

Tip: The bot edits this comment instead of posting a new one, so you can check the comment's history to see earlier messages.

[Laczen]

@rakons, what do you mean by filter duplicates ? Is the idea to call the settings_handler just for the latest entry in the fs, or is it something else ?

[rakons]

@Laczen - yes. Ignore previous (old) copies and call the cb function only for the final data.

[Laczen]

@rakons, OK. In that case I think you can achieve the same results without any changes to fcb. What you are trying to achieve is similar to what is done in the compress method. In that method a copy of an entry is done when there is no later entry in the fs that has the same name. In your case you would only call the load if there is no later entry in the fs with the same name. The load can be modified very easily to do the same functionality as the compress, you can remove the fcb_walk routine completely. For the nffs case the same methodology can be used. I would propose the filter setting to be a configuration option. By introducing a load variable that is set to true at the start for every entry and then walking over the remaining fs to see if you need to set load to false (if you find another entry with the same name) it is easy to use the configuration option to decide if you need to walk over the remaining items or not.
At a high level it would look like:

while (fcb_getnext(&cf->cf_fcb, &loc1.loc) == 0) {
   read name1
   load = true;
#if enabled(SETTINGS_SFCB_FILTER_DUPLICATE)
   loc2 = loc1;
   while (fcb_getnext(&cf->cf_fcb, &loc2.loc) == 0) {
       read name2
       if name1 equal to name2 {
           load = false;
           break;
       }
    }
#endif
   if (load) {
      call settings_handler
   }
}

[rakons]

It makes a sense. Limiting callbacks levels inside settings is always good move. Shouldnt we lock fcb while processing it using fcb_getnext?

[Laczen]

It makes a sense. Limiting callbacks levels inside settings is always good move. Shouldnt we lock fcb while processing it using fcb_getnext?

@nvlsianpu, ^^^ should fcb be locked ?

[rakons]

Reworked limiting callbacks level by @Laczen sugestion.
Currently fcb is not locked during data traversing. Waiting for @nvlsianpu to feedback if it should be locked. It seems that we are missing some locking while data compressing anyway inside fcb.

[nvlsianpu]

It makes a sense. Limiting callbacks levels inside settings is always good move. Shouldnt we lock fcb while processing it using fcb_getnext?

@nvlsianpu, ^^^ should fcb be locked ?

I have been Looking deeply into code. Answer to this question is yes. Need to introduce few more lock into code for ensure multiple access compatibility

I have been Looking deeply into code. Answer to this question is yes. Need to introduce few more lock into code for ensure multiple access compatibility. I Discussed this with @rako of-channel.

[Laczen]

It makes a sense. Limiting callbacks levels inside settings is always good move. Shouldnt we lock fcb while processing it using fcb_getnext?

@nvlsianpu, ^^^ should fcb be locked ?

I have been Looking deeply into code. Answer to this question is yes. Need to introduce few more lock into code for ensure multiple access compatibility

I have been Looking deeply into code. Answer to this question is yes. Need to introduce few more lock into code for ensure multiple access compatibility. I Discussed this with @rako of-channel.

@rakons, @nvlsianpu, are you sure about the locking ? AFAIR, locking in fcb is only required when doing a write (nothing else changes the fs), when a writing is done fcb allocates the space and locks the fs (fcb_append), data is written during this lock (first unlocking it and then doing the write, afterwards lock it back again). when the writing is finished (fcb_close) the last part is written and then the fs is unlocked.

[Laczen]

The proposed addition IMO does not justify an API change. Either enable it by default or use the filter_duplicate option only on settings_load_direct().

[rakons]

It does not make a big sense to use it when all the settings are loaded at startup. It was the idea - do not change an external API. If somebody knows it is here and wishes to use it - set the specific field in the settings.

[Laczen]

You are changing an external API. Settings can be configured to use a custom backend, for these you are changing the API.

[rakons]

@nvlsianpu - What is your opinion? Anybody more we can vote if it is worth to change API between settings and backend or is is better to add another backend function?
The first solution would fail during compilation if not adjusted. The second can lead to runtime error but if the functionality is not used, it would not require any adjustment from the user.

[Laczen]

@rakons, there are other options: enable it by default (makes the load a little longer) or use config to enable it.

[nvlsianpu]

I would introduce this API change - it is no big change. Making it optional makes things harder to be maintained - I believe adaptation of a current custom back-end will be easy.

[Laczen]

@nvlsianpu, why not enable it by default ?

[nvlsianpu]

This API was introduced like ~2 weeks ago by #19077

[Laczen]

@nvlsianpu ?? There is a new change used for passing the bool filter_duplicates.

[Laczen]

See above

[Laczen]

See above

[Laczen]

shouldn't this be if (rc <= 0) ?

[rakons]

No. If 0 is returned I wish to set pass_entry to 1 and continue the loop.

[Laczen]

There is something strange with the logic here, if no duplicates are found or if a duplicate is found you want to set pass_entry to 1 (unless the description of settings_fcb_check_duplicate is wrong).

[rakons]

pass_entry = (rc == 0); - bracket added. If duplicates are found then rc = 1. If no duplicates is found - rc = 0.

[Laczen]

Ok, this makes it more clear, but I still think it is not correct. Whenever an error is occurring during the find duplicates there is a jump out of the loop, while it should just be a continue. It should still load whatever it has found.

[rakons]

Done

[Laczen]

You could easily use the exact same methodology in fcb, doing the same thing makes it easier to maintain.

[rakons]

I would rather pass the same variable name into file - is_duplicate does not explain everything and I am worried that it might be used for additional checking in the future. pass_entry explains what this variable does.

[rakons]

Done

[Laczen]

I agree, I would not use is_duplicate, but use a boolean load = true. This explains everything, you set load to false when not needed. The methodology in the file is a lot simpler than in the fcb backend, so keep this methodology.

[rakons]

I do not think so. Moreover - to much of tabs is required in files and it makes it hard to keep line length limit. Splitting it into two functions line in fcb backend.

[Laczen]

It is very easy to put this in a function for the file as well. This is no reason. Keep the methods equal! Just have this function return 0 if it finds a match, and an error code when it doesn't (a error: end of file or whatever error means loading is needed).

[rakons]

I see no reason to flatten the function return value so much. It might make sense to react in the same way when an error occur and no match is found. The functionality is the same now in both backends.

[Laczen]

@rakons, I do see a reason for both backends to flatten out the function return. At the moment settings reads as much as it can, when you don't flatten out the error settings will return no value at all because there is a error later in the backend, even when this error has nothing to do with the value being read.

[rakons]

Did you look into the current code?

[nvlsianpu]

@Laczen fcb settings compression: when one thread is appending to scratch, other thread might append to, so it might occur that it causes even scratch sector full filling before compression is done.

What i see is that the fcb instance is preserved against its state violation., but not against something like that.

[Laczen]

@Laczen fcb settings compression: when one thread is appending to scratch, other thread might append to, so it might occur that it causes even scratch sector full filling before compression is done.
What i see is that the fcb instance is preserved against its state violation., but not against something like that.

True, but fcb cannot protect against this, I think the settings lock protects against this. But of course this means you can only use the fcb fs for settings.

[Laczen]

continue ? Or even better, remove the if, when length is zero it will set name2[0] = '\0'

[rakons]

Ok, why not.

[fnde-ot]

Maybe we should consider deprecating backends storing duplicates at some point, but meanwhile this change makes sense to me. Nice job!

[Laczen]

When the filter_duplicate is a option to the load routine, how do I decide to use it or not ?

This is left to the user and a clear answer needs to be provided.

[rakons]

The output for the meeting with @carlescufi and @jhedberg:
We are moving towards the backends that does not present the entity history.
FCB stays here for backward compatibility, FILE would always be there, but we does not expect performance from FILE backend.
What we should do:

• Make an doc update that from 2.1 we does support the backends that presents the history (then we may remove some complexity from the modules that uses settings).
• Filter out deleted entries.
• Add filtering also for global loading function. Does not provide any optional flag in KConfig - this is now the requirement for the backend.

[rakons]

Seems done
@carlescufi - who can check the documentation?

[carlescufi]

Seems done
@carlescufi - who can check the documentation?

@dbkinder will, he is automatically added when you edit .rst files.

[carlescufi]

@Laczen could you take another look?

[Laczen]

storied -> stored

[rakons]

Covered by @dbkinder comment below.

[jhedberg]

What's the reason you left the filter_duplicates as a parameter for this internal API?

[rakons]

It is used to search the duplicates when storing. Storing procedure can have simpler algorithm to remove the duplicates with a * (x) complexity. It is left here for storing performance.

[jhedberg]

@rakons ok, makes sense, thanks for the clarification!

[dbkinder]

For clarity and fixing some misspellings, how about instead:

This means that the newest data entity is stored after any
older existing data entities. Starting with Zephyr 2.1, the
back-end must filter out all old entities and call the
callback with only the newest entity.

[rakons]

Fixed. Please check if this is what you meant.

stale

[carlescufi]

@rakons can you fix the checkpatch issues?