Hey Paul,
The masks on disk are solely an optimization during load. We only write the masks out to disk when a Blobstore is unloaded. If Blobstore is not unloaded cleanly, we can rebuild both masks with a full walk of all of the metadata pages.
This avoids an extra page write on every metadata operation (create/delete/resize/xattr).
I agree that we should defer clearing the clean bit until the first metadata operation. That is not in the I/O path so an extra step on create/delete/resize/xattr would not be cumbersome.
-Jim
From:
SPDK <spdk-bounces@lists.01.org> on behalf of Paul E Luse <paul.e.luse@intel.com>
Reply-To: Storage Performance Development Kit <spdk@lists.01.org>
Date: Monday, October 2, 2017 at 2:00 PM
To: Storage Performance Development Kit <spdk@lists.01.org>
Subject: [SPDK] blobstore metadata questions, comments and potential issues
Guys-
Going through the finer details on MD handling (for the first time so forgive misunderstandings) before looking more closely at Cunyn’s patch (https://review.gerrithub.io/#/c/376470/ ) and I’m seeing some unexpected,
well IMHO, things that I wanted to cover real quick. Mostly they relate to my very early comments on the patch where we never updated the clean bit in MD.
Forget blobstore or the patch for a minute, in general I would argue that the most efficient way to manage clean/dirty metadata in a system like this is:
-
When you init the system you are clean by definition so your on-disk and in-memory structs should match and your ‘clean’ bit should be 1 in both cases
-
When you encounter an event that requires a metadata change:
o
If your in-memory clean==0 do nothing wrt on disk sync’ing because you’re already dirty
o
If your in-memory clean==1 then set to 0 and write the on-disk clean to 0 so that (a) you know you don’t need to update on disk clean w/next MD update and (b) you are protected from power fail because on disk clean is now 1.
Do not sync metadata, this is just an update to that bit (or in our case a superblock-only write)
-
In each of these following scenarios do a full sync of the metadata and set clean to 1 both in memory and on disk
o
Just loaded or initialized the system
o
Unloading the system
o
Just recovered the system after loading and recognizing that clean was 0 so you rebuilt everything
So when I look through blobstore and see the following I can certainly continue to review the patch as it builds on this but would like to follow it up with a patch to make it work like above so that we don’t have
windows where we are dirty for no reason.
But, before that I also have a few questions for things I just don’t understand.
-
When we init a BS we claim the MD used cluster in memory but never sync. I assume this is not an oversight because clean is 0 (but if we do the stuff I mention above this would end with a superblock only update to set clean to
1)
-
When we load a BS, (with the patch) we are going to immediately change clean to 0 (assuming we didn’t come up dirty) and then turn right around and not only update the superblock to set clean to 0 (just explained how I feel about
that) but then we recalc and then rewrite, to disk the masks that we just read when nothing could have changed? If I read this correctly, besides my comment about us not actually being dirty yet, why are we updating the on disks masks again?
-
When an app calls spdk_bs_md_sync_blob after a resize or something, I don’t see where the masks on disk immediately following the superblock are ever updated. It
appears at least that we are only updating he metadata for the blob itself and the in memory masks (in the bs struct) but not the on disk masks. So any scenario where we do a resize & sync (or xattr and sync) and have a dirty shutdown, even with the
recovery code, is going to end up corrupted right? It seems like the on disk masks that are stored just after the superblock need to be written as part of
spdk_bs_md_sync_blob() and from what I can tell they’re only updated with
unload today. This is, of course, independent of Cunyin’s patch but I wanted to ask about it since that’s the context I’m looking at it from
Sorry for the length email and all the claims/questions. I’m certain I have mis-read some of this stuff at least but w/o understanding these points I can’t really responsibly review Cunyin’s patch which is
critical I think… well, his patch is critical I mean, not my review of it
J
-Paul