Conversation
Three places matched the message with a string prefix: the one that builds it, the reload that has to bring the mount down when the volume is gone, and format, which decides between creating and updating from it. Rewording the message would have quietly turned the reload into a warning and left the mount running against a volume that no longer exists, with nothing failing to build. Export the error and match it with errors.Is. Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
The format is published through setFormat and read concurrently through getFormat, so an update has to be published as a new value. The atomic pointer added for the format race guards the pointer, not the struct it points at. Load published the format before refresh handed it to the reload callbacks, and the mount's callback writes the command line overrides (Bucket, Storage, Tiers, UploadLimit) into it on every heartbeat. Split the parsing out of Load as loadFormat, and let reloadFormat run the callbacks while the format is still private, publishing it once they are done. The window where readers saw the stored values before the overrides were applied is gone too. loadFormat also gives the callers that only want to read the format a way to do so without publishing it, which the next changes use. Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
Turning DirStats or UserGroupQuota on wrote the flag straight into the format every reader shares, then passed that same pointer to be stored. On a mount or an SDK client that format is the one the reload callbacks have patched with the options the client was started with, so the bucket and the limits of a single client went to every other one. Two such calls also raced: each started from the same format, so whichever stored last dropped the flag the other had set, and both reported success. Load the stored format instead, set the flag on that, and hold a lock across the load and the store. The three cases only differed in the flag, so they collapse into one helper and one shared case. Failing to store the flag is now an error rather than a warning: the quota would otherwise be created but never accounted, since updateDirQuota gives up as soon as it sees the flag disabled. It carries EIO, because a store that was briefly unreachable is worth retrying. Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
HandleQuota turned every error it did not recognise into EINVAL, so a metadata store that was briefly unreachable reached a Java caller as "invalid argument" and looked like a request worth giving up on rather than retrying. Pass through whichever errno the meta layer reported, and fall back to EINVAL only when there is none. Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
Every engine published the format itself, and each one picked its own moment: redis between storing the setting and creating the root inode of a fresh volume, the other two before the transaction that stores it. So a caller that got an error back could be left running on a format the volume never received, and whether that happened depended on the engine. Publish in Init, once the engine reports success. The engines just store now, and a fourth one cannot get the ordering wrong. The KV case is covered through a client whose writes fail, the SQL one through a read-only client: its txn refuses to run while the read doInit does first still goes through simpleTxn. The latter fails with the setFormat call sql.go had before its transaction. Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
GetFormat returns the format by value, which reads as a private copy, but Tiers is a map so the copy keeps pointing at the live one. Reading the .config file of a mount takes such a copy and hands it to the mount's patcher, which writes Tiers[0] straight back into the map the published format owns, while the heartbeat walks it in DeepEqual. That is a concurrent map read and write, which takes the whole process down rather than just racing. Give Format a Clone that copies the map, and hand that out. Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
Status loads the format through Load, which publishes it. On a client that is only reporting, that overwrites the format the reload callbacks had patched with this client's own options, and the next heartbeat then finds the stored format equal to the published one, skips the callbacks and never puts the patches back. It also scrubbed the secrets out of the format it had just published, replacing SecretKey, SessionToken and EncryptKey with "removed" for every reader of the volume. From a long lived client (the Java SDK calls Status on a mounted session) a later quota set would then store the placeholders in the volume setting for good. Read the format without publishing it and scrub a copy, so only the reported Sections carries the placeholders. A client that has nothing published yet still needs one for the StatFS below, which is the case of the status command and its fresh client. Signed-off-by: Git'Fellow <12234510+solracsf@users.noreply.github.com>
solracsf
force-pushed
the
fix/meta-format-mutation
branch
from
August 16, 2026 07:18
2bf8b6b to
44810a9
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #7378, which stored
baseMeta.fmtas anatomic.Pointer. That guards the pointer, not the struct it points at, and several call sites still wrote into the format every reader shares.The reload path published too early:
Loadpublished the format beforerefreshhanded it to the reload callbacks, and the mount's callback writes the command line overrides (Bucket,Storage,Tiers,UploadLimit) into it on every heartbeat. The parsing is split out ofLoadasloadFormat, andreloadFormatruns the callbacks while the format is still private, publishing it once they are done. The window where readers saw the stored values before the overrides were applied is gone too.Status(pkg/meta/status.go) calledLoad, which publishes the format, and thenRemoveSecreton that same pointer, replacingSecretKey,SessionTokenandEncryptKeywithremovedfor every reader of the volume. From a long lived client (the Java SDK callsStatuson a mounted session) a later quota set would store the placeholders in the volume setting for good. Publishing there also dropped the patches the reload callbacks had applied, and because the next reload then finds the stored format unchanged and skips the callbacks, the client kept running without its own overrides until the stored format changed. It now reads throughloadFormat, which does not publish, and scrubs a copy, so only the reportedSectionscarries the placeholders.handleQuotaSet(pkg/meta/quota.go) turnedDirStatsorUserGroupQuotaon in the format it had just read fromgetFormat(), then passed that same pointer on to be stored. On a mount or an SDK client that format is the one the reload callbacks have patched, so the bucket and the limits of a single client went into the volume setting for every other client. Two such calls also raced: each started from the same format, so whichever stored last dropped the flag the other had set, and both reported success. It now loads the stored format, sets the flag on that, and holds a lock across the load and the store.GetFormatreturns the format by value, which reads as a private copy, butTiersis a map so the copy keeps pointing at the live one. Reading.configon a mount takes such a copy and hands it to the mount's patcher, which writesTiers[0]straight back into the map the published format owns, while the heartbeat walks it inreflect.DeepEqual. That is a concurrent map read and write, which takes the process down rather than just racing.Format.Clonecopies the map, andGetFormathands that out.Three related changes fell out of this.
handleQuotaSetonly warned when thedoInitenabling those flags failed, and created the quota anyway. Since the flag is no longer forced on locally,updateDirQuotasees it disabled and returns immediately, so the quota exists but is never accounted or enforced while the command reports success. It returns the error now, carryingEIO, andfs.FileSystem.HandleQuotapasses that errno through instead of turning everything it does not recognise intoEINVAL, so a metadata store that was briefly unreachable no longer reaches a Java caller as an invalid request.Publishing moved out of the engines into
Init. Each engine picked its own moment: redis between storing the setting and creating the root inode of a fresh volume, the SQL and KV engines before the transaction that stores it. A caller that got an error back could therefore be left running on a format the volume never received, and whether that happened depended on the engine.Initpublishes oncedoInitreports success, so the engines only store and a fourth one cannot get the ordering wrong.ErrNotFormattedreplaces the message prefix that three places matched on with a string compare. Rewording that message would have quietly turned the reload'sos.Exit(UmountCode)into a warning, leaving a mount running against a volume that no longer exists, with nothing failing to build.Tests
TestPublishedFormatIsNotMutatedcovers the invariant in seven subtests (quota set, quota set keeps the local overrides local, tiers are copied, failed init, failed init (sql), status, reload callbacks), andTestQuotaSetFailsWhenFormatUpdateFailscovers the error propagation. Each case was run against the code without its fix first and fails there, for example:The KV
failed initcase injects the failure through atkvClientwrapper that failstxnbut notsimpleTxn, sodoInitgets past its initial read and fails exactly where the setting would be stored. It changesCapacityrather than a flag, because enablingDirStatsorUserGroupQuotaruns a cleanup transaction that would fail earlier and hide the case under test. The SQL case gets the same reads pass, writes fail split from a read only client, sincedbMeta.txnrefuses to run while the initial read still goes throughsimpleTxn. That case fails with thesetFormatcallsql.goused to have before its transaction.