[SPDK] blobstore metadata questions, comments and potential issues

[SPDK] blobstore metadata questions, comments and potential issues

[Luse, Paul E]

[-- Attachment #1: Type: text/plain, Size: 3688 bytes --]

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 :)

-Paul






[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 13360 bytes --]

Re: [SPDK] blobstore metadata questions, comments and potential issues

[Harris, James R]

[-- Attachment #1: Type: text/plain, Size: 4628 bytes --]

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(a)lists.01.org> on behalf of Paul E Luse <paul.e.luse(a)intel.com>
Reply-To: Storage Performance Development Kit <spdk(a)lists.01.org>
Date: Monday, October 2, 2017 at 2:00 PM
To: Storage Performance Development Kit <spdk(a)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 ☺

-Paul






[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 14202 bytes --]

Re: [SPDK] blobstore metadata questions, comments and potential issues

[Luse, Paul E]

[-- Attachment #1: Type: text/plain, Size: 5487 bytes --]

Thanks for the quick response Jim! What you say will only be true after the pending patch that I mention below lands though correct?

Also, it seems like it would be good to write those masks out on disk on sync() as well as this will result in a clean MD but, of course, that only makes sense once the clean bit suggestion mentioned below is in place.  I’ll take care of that once the current patch lands just to simply review & merging since it’s not urgent.

Thx
Paul

From: SPDK [mailto:spdk-bounces(a)lists.01.org] On Behalf Of Harris, James R
Sent: Monday, October 2, 2017 2:46 PM
To: Storage Performance Development Kit <spdk(a)lists.01.org>
Subject: Re: [SPDK] blobstore metadata questions, comments and potential issues

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(a)lists.01.org<mailto:spdk-bounces(a)lists.01.org>> on behalf of Paul E Luse <paul.e.luse(a)intel.com<mailto:paul.e.luse(a)intel.com>>
Reply-To: Storage Performance Development Kit <spdk(a)lists.01.org<mailto:spdk(a)lists.01.org>>
Date: Monday, October 2, 2017 at 2:00 PM
To: Storage Performance Development Kit <spdk(a)lists.01.org<mailto:spdk(a)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 ☺

-Paul






[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 16006 bytes --]

Re: [SPDK] blobstore metadata questions, comments and potential issues

[Harris, James R]

[-- Attachment #1: Type: text/plain, Size: 6207 bytes --]

Writing the masks to disk on every sync would kill performance.  Take a 2TB SSD for example.  By default it will have 2 million metadata pages and 2 million clusters (assuming 1MB cluster size), which can be represented by a bitmask of size 256KB for each (512KB total).  We definitely don’t want to write that out on every sync operation to every blob.

From: SPDK <spdk-bounces(a)lists.01.org> on behalf of Paul E Luse <paul.e.luse(a)intel.com>
Reply-To: Storage Performance Development Kit <spdk(a)lists.01.org>
Date: Monday, October 2, 2017 at 3:13 PM
To: Storage Performance Development Kit <spdk(a)lists.01.org>
Subject: Re: [SPDK] blobstore metadata questions, comments and potential issues

Thanks for the quick response Jim! What you say will only be true after the pending patch that I mention below lands though correct?

Also, it seems like it would be good to write those masks out on disk on sync() as well as this will result in a clean MD but, of course, that only makes sense once the clean bit suggestion mentioned below is in place.  I’ll take care of that once the current patch lands just to simply review & merging since it’s not urgent.

Thx
Paul

From: SPDK [mailto:spdk-bounces(a)lists.01.org] On Behalf Of Harris, James R
Sent: Monday, October 2, 2017 2:46 PM
To: Storage Performance Development Kit <spdk(a)lists.01.org>
Subject: Re: [SPDK] blobstore metadata questions, comments and potential issues

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(a)lists.01.org<mailto:spdk-bounces(a)lists.01.org>> on behalf of Paul E Luse <paul.e.luse(a)intel.com<mailto:paul.e.luse(a)intel.com>>
Reply-To: Storage Performance Development Kit <spdk(a)lists.01.org<mailto:spdk(a)lists.01.org>>
Date: Monday, October 2, 2017 at 2:00 PM
To: Storage Performance Development Kit <spdk(a)lists.01.org<mailto:spdk(a)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 ☺

-Paul






[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 17848 bytes --]

Re: [SPDK] blobstore metadata questions, comments and potential issues

[Luse, Paul E]

[-- Attachment #1: Type: text/plain, Size: 6982 bytes --]

So you think a 512K write on a sync is a big cost? I guess I hadn’t envisioned sync being that frequent of an operation… It is a bit misleading that the sync API doesn’t do a full sync, it just marks the blob that was operated on as clean. I wonder if anyone would see any value in a new API bs_sync() that sync’d the super block + on disk masks?

Thx
Paul

From: SPDK [mailto:spdk-bounces(a)lists.01.org] On Behalf Of Harris, James R
Sent: Monday, October 2, 2017 3:18 PM
To: Storage Performance Development Kit <spdk(a)lists.01.org>
Subject: Re: [SPDK] blobstore metadata questions, comments and potential issues

Writing the masks to disk on every sync would kill performance.  Take a 2TB SSD for example.  By default it will have 2 million metadata pages and 2 million clusters (assuming 1MB cluster size), which can be represented by a bitmask of size 256KB for each (512KB total).  We definitely don’t want to write that out on every sync operation to every blob.

From: SPDK <spdk-bounces(a)lists.01.org<mailto:spdk-bounces(a)lists.01.org>> on behalf of Paul E Luse <paul.e.luse(a)intel.com<mailto:paul.e.luse(a)intel.com>>
Reply-To: Storage Performance Development Kit <spdk(a)lists.01.org<mailto:spdk(a)lists.01.org>>
Date: Monday, October 2, 2017 at 3:13 PM
To: Storage Performance Development Kit <spdk(a)lists.01.org<mailto:spdk(a)lists.01.org>>
Subject: Re: [SPDK] blobstore metadata questions, comments and potential issues

Thanks for the quick response Jim! What you say will only be true after the pending patch that I mention below lands though correct?

Also, it seems like it would be good to write those masks out on disk on sync() as well as this will result in a clean MD but, of course, that only makes sense once the clean bit suggestion mentioned below is in place.  I’ll take care of that once the current patch lands just to simply review & merging since it’s not urgent.

Thx
Paul

From: SPDK [mailto:spdk-bounces(a)lists.01.org] On Behalf Of Harris, James R
Sent: Monday, October 2, 2017 2:46 PM
To: Storage Performance Development Kit <spdk(a)lists.01.org<mailto:spdk(a)lists.01.org>>
Subject: Re: [SPDK] blobstore metadata questions, comments and potential issues

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(a)lists.01.org<mailto:spdk-bounces(a)lists.01.org>> on behalf of Paul E Luse <paul.e.luse(a)intel.com<mailto:paul.e.luse(a)intel.com>>
Reply-To: Storage Performance Development Kit <spdk(a)lists.01.org<mailto:spdk(a)lists.01.org>>
Date: Monday, October 2, 2017 at 2:00 PM
To: Storage Performance Development Kit <spdk(a)lists.01.org<mailto:spdk(a)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 ☺

-Paul






[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 19419 bytes --]

Re: [SPDK] blobstore metadata questions, comments and potential issues

[Harris, James R]

[-- Attachment #1: Type: text/plain, Size: 8275 bytes --]

It’s definitely a big cost.  For example, BlobFS saves the file length as an xattr on the blob.  So every operation on the WAL would add a 512KB I/O write.  sync() today is a per-blob operation for persisting resize and xattr operations to disk.  If we’re trying to always keep the on-disk masks up-to-date, it would need to be done for create/delete blob operations as well.

Regarding the sync API – which part is misleading?  I was just reading the header file and it seems to pretty clearly state this is a per-blob operation.  Do we just need to add more details in the theory of operation doc for how this works?

I guess a bs_sync() API could be added, if someone wants to incur the high cost of writing out the 512KB for every metadata operation on the blobstore and cannot afford the time to rebuild the masks after a dirty shutdown.  Can you elaborate on where you see this being needed?

Thanks,

-Jim


From: SPDK <spdk-bounces(a)lists.01.org> on behalf of Paul E Luse <paul.e.luse(a)intel.com>
Reply-To: Storage Performance Development Kit <spdk(a)lists.01.org>
Date: Monday, October 2, 2017 at 3:44 PM
To: Storage Performance Development Kit <spdk(a)lists.01.org>
Subject: Re: [SPDK] blobstore metadata questions, comments and potential issues

So you think a 512K write on a sync is a big cost? I guess I hadn’t envisioned sync being that frequent of an operation… It is a bit misleading that the sync API doesn’t do a full sync, it just marks the blob that was operated on as clean. I wonder if anyone would see any value in a new API bs_sync() that sync’d the super block + on disk masks?

Thx
Paul

From: SPDK [mailto:spdk-bounces(a)lists.01.org] On Behalf Of Harris, James R
Sent: Monday, October 2, 2017 3:18 PM
To: Storage Performance Development Kit <spdk(a)lists.01.org>
Subject: Re: [SPDK] blobstore metadata questions, comments and potential issues

Writing the masks to disk on every sync would kill performance.  Take a 2TB SSD for example.  By default it will have 2 million metadata pages and 2 million clusters (assuming 1MB cluster size), which can be represented by a bitmask of size 256KB for each (512KB total).  We definitely don’t want to write that out on every sync operation to every blob.

From: SPDK <spdk-bounces(a)lists.01.org<mailto:spdk-bounces(a)lists.01.org>> on behalf of Paul E Luse <paul.e.luse(a)intel.com<mailto:paul.e.luse(a)intel.com>>
Reply-To: Storage Performance Development Kit <spdk(a)lists.01.org<mailto:spdk(a)lists.01.org>>
Date: Monday, October 2, 2017 at 3:13 PM
To: Storage Performance Development Kit <spdk(a)lists.01.org<mailto:spdk(a)lists.01.org>>
Subject: Re: [SPDK] blobstore metadata questions, comments and potential issues

Thanks for the quick response Jim! What you say will only be true after the pending patch that I mention below lands though correct?

Also, it seems like it would be good to write those masks out on disk on sync() as well as this will result in a clean MD but, of course, that only makes sense once the clean bit suggestion mentioned below is in place.  I’ll take care of that once the current patch lands just to simply review & merging since it’s not urgent.

Thx
Paul

From: SPDK [mailto:spdk-bounces(a)lists.01.org] On Behalf Of Harris, James R
Sent: Monday, October 2, 2017 2:46 PM
To: Storage Performance Development Kit <spdk(a)lists.01.org<mailto:spdk(a)lists.01.org>>
Subject: Re: [SPDK] blobstore metadata questions, comments and potential issues

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(a)lists.01.org<mailto:spdk-bounces(a)lists.01.org>> on behalf of Paul E Luse <paul.e.luse(a)intel.com<mailto:paul.e.luse(a)intel.com>>
Reply-To: Storage Performance Development Kit <spdk(a)lists.01.org<mailto:spdk(a)lists.01.org>>
Date: Monday, October 2, 2017 at 2:00 PM
To: Storage Performance Development Kit <spdk(a)lists.01.org<mailto:spdk(a)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 ☺

-Paul






[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 21953 bytes --]

Re: [SPDK] blobstore metadata questions, comments and potential issues

[Luse, Paul E]

[-- Attachment #1: Type: text/plain, Size: 9624 bytes --]

Thanks again Jim, it’s probably only misleading because my past experience and assumptions about what “sync” means (usually means all metadata).  Yeah the API clearly has the word blob in it so I breezed over that but if I made the assumption that sync means I’m 100% clean I’m sure someone else who’s worked w/other filesystems before will too. The thing that seems “hidden” is that I don’t believe there’s anything documenting that there’s a price to pay on startup even if you think you’ve sync’d – I believe most would assume that they can expect consistent performance in startup time, which is important to some applications.  The bs_sync() API isn’t something anyone would *need* to call after every blob sync but at least there’d be the option.

Don’t take my questions as a sign that I think this is a big deal or anything, I’m just picking at thing to learn more than anything ☺

Thx
Paul

From: SPDK [mailto:spdk-bounces(a)lists.01.org] On Behalf Of Harris, James R
Sent: Monday, October 2, 2017 3:59 PM
To: Storage Performance Development Kit <spdk(a)lists.01.org>
Subject: Re: [SPDK] blobstore metadata questions, comments and potential issues

It’s definitely a big cost.  For example, BlobFS saves the file length as an xattr on the blob.  So every operation on the WAL would add a 512KB I/O write.  sync() today is a per-blob operation for persisting resize and xattr operations to disk.  If we’re trying to always keep the on-disk masks up-to-date, it would need to be done for create/delete blob operations as well.

Regarding the sync API – which part is misleading?  I was just reading the header file and it seems to pretty clearly state this is a per-blob operation.  Do we just need to add more details in the theory of operation doc for how this works?

I guess a bs_sync() API could be added, if someone wants to incur the high cost of writing out the 512KB for every metadata operation on the blobstore and cannot afford the time to rebuild the masks after a dirty shutdown.  Can you elaborate on where you see this being needed?

Thanks,

-Jim


From: SPDK <spdk-bounces(a)lists.01.org<mailto:spdk-bounces(a)lists.01.org>> on behalf of Paul E Luse <paul.e.luse(a)intel.com<mailto:paul.e.luse(a)intel.com>>
Reply-To: Storage Performance Development Kit <spdk(a)lists.01.org<mailto:spdk(a)lists.01.org>>
Date: Monday, October 2, 2017 at 3:44 PM
To: Storage Performance Development Kit <spdk(a)lists.01.org<mailto:spdk(a)lists.01.org>>
Subject: Re: [SPDK] blobstore metadata questions, comments and potential issues

So you think a 512K write on a sync is a big cost? I guess I hadn’t envisioned sync being that frequent of an operation… It is a bit misleading that the sync API doesn’t do a full sync, it just marks the blob that was operated on as clean. I wonder if anyone would see any value in a new API bs_sync() that sync’d the super block + on disk masks?

Thx
Paul

From: SPDK [mailto:spdk-bounces(a)lists.01.org] On Behalf Of Harris, James R
Sent: Monday, October 2, 2017 3:18 PM
To: Storage Performance Development Kit <spdk(a)lists.01.org<mailto:spdk(a)lists.01.org>>
Subject: Re: [SPDK] blobstore metadata questions, comments and potential issues

Writing the masks to disk on every sync would kill performance.  Take a 2TB SSD for example.  By default it will have 2 million metadata pages and 2 million clusters (assuming 1MB cluster size), which can be represented by a bitmask of size 256KB for each (512KB total).  We definitely don’t want to write that out on every sync operation to every blob.

From: SPDK <spdk-bounces(a)lists.01.org<mailto:spdk-bounces(a)lists.01.org>> on behalf of Paul E Luse <paul.e.luse(a)intel.com<mailto:paul.e.luse(a)intel.com>>
Reply-To: Storage Performance Development Kit <spdk(a)lists.01.org<mailto:spdk(a)lists.01.org>>
Date: Monday, October 2, 2017 at 3:13 PM
To: Storage Performance Development Kit <spdk(a)lists.01.org<mailto:spdk(a)lists.01.org>>
Subject: Re: [SPDK] blobstore metadata questions, comments and potential issues

Thanks for the quick response Jim! What you say will only be true after the pending patch that I mention below lands though correct?

Also, it seems like it would be good to write those masks out on disk on sync() as well as this will result in a clean MD but, of course, that only makes sense once the clean bit suggestion mentioned below is in place.  I’ll take care of that once the current patch lands just to simply review & merging since it’s not urgent.

Thx
Paul

From: SPDK [mailto:spdk-bounces(a)lists.01.org] On Behalf Of Harris, James R
Sent: Monday, October 2, 2017 2:46 PM
To: Storage Performance Development Kit <spdk(a)lists.01.org<mailto:spdk(a)lists.01.org>>
Subject: Re: [SPDK] blobstore metadata questions, comments and potential issues

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(a)lists.01.org<mailto:spdk-bounces(a)lists.01.org>> on behalf of Paul E Luse <paul.e.luse(a)intel.com<mailto:paul.e.luse(a)intel.com>>
Reply-To: Storage Performance Development Kit <spdk(a)lists.01.org<mailto:spdk(a)lists.01.org>>
Date: Monday, October 2, 2017 at 2:00 PM
To: Storage Performance Development Kit <spdk(a)lists.01.org<mailto:spdk(a)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 ☺

-Paul






[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 24386 bytes --]

Re: [SPDK] blobstore metadata questions, comments and potential issues

[Harris, James R]

[-- Attachment #1: Type: text/plain, Size: 10639 bytes --]

POSIX already has both sync/syncfs (which is global or per-filesystem) and fsync (flushes per-file metadata) so different calls for per-filesystem and per-file syncing are well known.

If we were going to support keeping the bitmasks up-to-date on every metadata operation, I think we’d be better served to allow for writing out pages of the bitmask individually, and then have that as an spdk_bs_init/load option so users could decide which they want.  For applications that need a guarantee on consistent startup performance even after a crash, I think they will not be able to handle a full bitmask write on every metadata operation.

-Jim


From: SPDK <spdk-bounces(a)lists.01.org> on behalf of Paul E Luse <paul.e.luse(a)intel.com>
Reply-To: Storage Performance Development Kit <spdk(a)lists.01.org>
Date: Monday, October 2, 2017 at 4:06 PM
To: Storage Performance Development Kit <spdk(a)lists.01.org>
Subject: Re: [SPDK] blobstore metadata questions, comments and potential issues

Thanks again Jim, it’s probably only misleading because my past experience and assumptions about what “sync” means (usually means all metadata).  Yeah the API clearly has the word blob in it so I breezed over that but if I made the assumption that sync means I’m 100% clean I’m sure someone else who’s worked w/other filesystems before will too. The thing that seems “hidden” is that I don’t believe there’s anything documenting that there’s a price to pay on startup even if you think you’ve sync’d – I believe most would assume that they can expect consistent performance in startup time, which is important to some applications.  The bs_sync() API isn’t something anyone would *need* to call after every blob sync but at least there’d be the option.

Don’t take my questions as a sign that I think this is a big deal or anything, I’m just picking at thing to learn more than anything ☺

Thx
Paul

From: SPDK [mailto:spdk-bounces(a)lists.01.org] On Behalf Of Harris, James R
Sent: Monday, October 2, 2017 3:59 PM
To: Storage Performance Development Kit <spdk(a)lists.01.org>
Subject: Re: [SPDK] blobstore metadata questions, comments and potential issues

It’s definitely a big cost.  For example, BlobFS saves the file length as an xattr on the blob.  So every operation on the WAL would add a 512KB I/O write.  sync() today is a per-blob operation for persisting resize and xattr operations to disk.  If we’re trying to always keep the on-disk masks up-to-date, it would need to be done for create/delete blob operations as well.

Regarding the sync API – which part is misleading?  I was just reading the header file and it seems to pretty clearly state this is a per-blob operation.  Do we just need to add more details in the theory of operation doc for how this works?

I guess a bs_sync() API could be added, if someone wants to incur the high cost of writing out the 512KB for every metadata operation on the blobstore and cannot afford the time to rebuild the masks after a dirty shutdown.  Can you elaborate on where you see this being needed?

Thanks,

-Jim


From: SPDK <spdk-bounces(a)lists.01.org<mailto:spdk-bounces(a)lists.01.org>> on behalf of Paul E Luse <paul.e.luse(a)intel.com<mailto:paul.e.luse(a)intel.com>>
Reply-To: Storage Performance Development Kit <spdk(a)lists.01.org<mailto:spdk(a)lists.01.org>>
Date: Monday, October 2, 2017 at 3:44 PM
To: Storage Performance Development Kit <spdk(a)lists.01.org<mailto:spdk(a)lists.01.org>>
Subject: Re: [SPDK] blobstore metadata questions, comments and potential issues

So you think a 512K write on a sync is a big cost? I guess I hadn’t envisioned sync being that frequent of an operation… It is a bit misleading that the sync API doesn’t do a full sync, it just marks the blob that was operated on as clean. I wonder if anyone would see any value in a new API bs_sync() that sync’d the super block + on disk masks?

Thx
Paul

From: SPDK [mailto:spdk-bounces(a)lists.01.org] On Behalf Of Harris, James R
Sent: Monday, October 2, 2017 3:18 PM
To: Storage Performance Development Kit <spdk(a)lists.01.org<mailto:spdk(a)lists.01.org>>
Subject: Re: [SPDK] blobstore metadata questions, comments and potential issues

Writing the masks to disk on every sync would kill performance.  Take a 2TB SSD for example.  By default it will have 2 million metadata pages and 2 million clusters (assuming 1MB cluster size), which can be represented by a bitmask of size 256KB for each (512KB total).  We definitely don’t want to write that out on every sync operation to every blob.

From: SPDK <spdk-bounces(a)lists.01.org<mailto:spdk-bounces(a)lists.01.org>> on behalf of Paul E Luse <paul.e.luse(a)intel.com<mailto:paul.e.luse(a)intel.com>>
Reply-To: Storage Performance Development Kit <spdk(a)lists.01.org<mailto:spdk(a)lists.01.org>>
Date: Monday, October 2, 2017 at 3:13 PM
To: Storage Performance Development Kit <spdk(a)lists.01.org<mailto:spdk(a)lists.01.org>>
Subject: Re: [SPDK] blobstore metadata questions, comments and potential issues

Thanks for the quick response Jim! What you say will only be true after the pending patch that I mention below lands though correct?

Also, it seems like it would be good to write those masks out on disk on sync() as well as this will result in a clean MD but, of course, that only makes sense once the clean bit suggestion mentioned below is in place.  I’ll take care of that once the current patch lands just to simply review & merging since it’s not urgent.

Thx
Paul

From: SPDK [mailto:spdk-bounces(a)lists.01.org] On Behalf Of Harris, James R
Sent: Monday, October 2, 2017 2:46 PM
To: Storage Performance Development Kit <spdk(a)lists.01.org<mailto:spdk(a)lists.01.org>>
Subject: Re: [SPDK] blobstore metadata questions, comments and potential issues

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(a)lists.01.org<mailto:spdk-bounces(a)lists.01.org>> on behalf of Paul E Luse <paul.e.luse(a)intel.com<mailto:paul.e.luse(a)intel.com>>
Reply-To: Storage Performance Development Kit <spdk(a)lists.01.org<mailto:spdk(a)lists.01.org>>
Date: Monday, October 2, 2017 at 2:00 PM
To: Storage Performance Development Kit <spdk(a)lists.01.org<mailto:spdk(a)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 ☺

-Paul






[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 26655 bytes --]

Re: [SPDK] blobstore metadata questions, comments and potential issues

[Luse, Paul E]

[-- Attachment #1: Type: text/plain, Size: 11213 bytes --]

So I don’t fully agree with you but I’m also not all that concerned about it.  If any when an application comes along with either requirement we can react accordingly.  For now though, I think its good to have a solid understanding of what is and isn’t sync’d, when, and what the implications are.  Thanks Jim!

From: SPDK [mailto:spdk-bounces(a)lists.01.org] On Behalf Of Harris, James R
Sent: Monday, October 2, 2017 4:28 PM
To: Storage Performance Development Kit <spdk(a)lists.01.org>
Subject: Re: [SPDK] blobstore metadata questions, comments and potential issues

POSIX already has both sync/syncfs (which is global or per-filesystem) and fsync (flushes per-file metadata) so different calls for per-filesystem and per-file syncing are well known.

If we were going to support keeping the bitmasks up-to-date on every metadata operation, I think we’d be better served to allow for writing out pages of the bitmask individually, and then have that as an spdk_bs_init/load option so users could decide which they want.  For applications that need a guarantee on consistent startup performance even after a crash, I think they will not be able to handle a full bitmask write on every metadata operation.

-Jim


From: SPDK <spdk-bounces(a)lists.01.org> on behalf of Paul E Luse <paul.e.luse(a)intel.com>
Reply-To: Storage Performance Development Kit <spdk(a)lists.01.org>
Date: Monday, October 2, 2017 at 4:06 PM
To: Storage Performance Development Kit <spdk(a)lists.01.org>
Subject: Re: [SPDK] blobstore metadata questions, comments and potential issues

Thanks again Jim, it’s probably only misleading because my past experience and assumptions about what “sync” means (usually means all metadata).  Yeah the API clearly has the word blob in it so I breezed over that but if I made the assumption that sync means I’m 100% clean I’m sure someone else who’s worked w/other filesystems before will too. The thing that seems “hidden” is that I don’t believe there’s anything documenting that there’s a price to pay on startup even if you think you’ve sync’d – I believe most would assume that they can expect consistent performance in startup time, which is important to some applications.  The bs_sync() API isn’t something anyone would *need* to call after every blob sync but at least there’d be the option.

Don’t take my questions as a sign that I think this is a big deal or anything, I’m just picking at thing to learn more than anything ☺

Thx
Paul

From: SPDK [mailto:spdk-bounces(a)lists.01.org] On Behalf Of Harris, James R
Sent: Monday, October 2, 2017 3:59 PM
To: Storage Performance Development Kit <spdk(a)lists.01.org>
Subject: Re: [SPDK] blobstore metadata questions, comments and potential issues

It’s definitely a big cost.  For example, BlobFS saves the file length as an xattr on the blob.  So every operation on the WAL would add a 512KB I/O write.  sync() today is a per-blob operation for persisting resize and xattr operations to disk.  If we’re trying to always keep the on-disk masks up-to-date, it would need to be done for create/delete blob operations as well.

Regarding the sync API – which part is misleading?  I was just reading the header file and it seems to pretty clearly state this is a per-blob operation.  Do we just need to add more details in the theory of operation doc for how this works?

I guess a bs_sync() API could be added, if someone wants to incur the high cost of writing out the 512KB for every metadata operation on the blobstore and cannot afford the time to rebuild the masks after a dirty shutdown.  Can you elaborate on where you see this being needed?

Thanks,

-Jim


From: SPDK <spdk-bounces(a)lists.01.org<mailto:spdk-bounces(a)lists.01.org>> on behalf of Paul E Luse <paul.e.luse(a)intel.com<mailto:paul.e.luse(a)intel.com>>
Reply-To: Storage Performance Development Kit <spdk(a)lists.01.org<mailto:spdk(a)lists.01.org>>
Date: Monday, October 2, 2017 at 3:44 PM
To: Storage Performance Development Kit <spdk(a)lists.01.org<mailto:spdk(a)lists.01.org>>
Subject: Re: [SPDK] blobstore metadata questions, comments and potential issues

So you think a 512K write on a sync is a big cost? I guess I hadn’t envisioned sync being that frequent of an operation… It is a bit misleading that the sync API doesn’t do a full sync, it just marks the blob that was operated on as clean. I wonder if anyone would see any value in a new API bs_sync() that sync’d the super block + on disk masks?

Thx
Paul

From: SPDK [mailto:spdk-bounces(a)lists.01.org] On Behalf Of Harris, James R
Sent: Monday, October 2, 2017 3:18 PM
To: Storage Performance Development Kit <spdk(a)lists.01.org<mailto:spdk(a)lists.01.org>>
Subject: Re: [SPDK] blobstore metadata questions, comments and potential issues

Writing the masks to disk on every sync would kill performance.  Take a 2TB SSD for example.  By default it will have 2 million metadata pages and 2 million clusters (assuming 1MB cluster size), which can be represented by a bitmask of size 256KB for each (512KB total).  We definitely don’t want to write that out on every sync operation to every blob.

From: SPDK <spdk-bounces(a)lists.01.org<mailto:spdk-bounces(a)lists.01.org>> on behalf of Paul E Luse <paul.e.luse(a)intel.com<mailto:paul.e.luse(a)intel.com>>
Reply-To: Storage Performance Development Kit <spdk(a)lists.01.org<mailto:spdk(a)lists.01.org>>
Date: Monday, October 2, 2017 at 3:13 PM
To: Storage Performance Development Kit <spdk(a)lists.01.org<mailto:spdk(a)lists.01.org>>
Subject: Re: [SPDK] blobstore metadata questions, comments and potential issues

Thanks for the quick response Jim! What you say will only be true after the pending patch that I mention below lands though correct?

Also, it seems like it would be good to write those masks out on disk on sync() as well as this will result in a clean MD but, of course, that only makes sense once the clean bit suggestion mentioned below is in place.  I’ll take care of that once the current patch lands just to simply review & merging since it’s not urgent.

Thx
Paul

From: SPDK [mailto:spdk-bounces(a)lists.01.org] On Behalf Of Harris, James R
Sent: Monday, October 2, 2017 2:46 PM
To: Storage Performance Development Kit <spdk(a)lists.01.org<mailto:spdk(a)lists.01.org>>
Subject: Re: [SPDK] blobstore metadata questions, comments and potential issues

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(a)lists.01.org<mailto:spdk-bounces(a)lists.01.org>> on behalf of Paul E Luse <paul.e.luse(a)intel.com<mailto:paul.e.luse(a)intel.com>>
Reply-To: Storage Performance Development Kit <spdk(a)lists.01.org<mailto:spdk(a)lists.01.org>>
Date: Monday, October 2, 2017 at 2:00 PM
To: Storage Performance Development Kit <spdk(a)lists.01.org<mailto:spdk(a)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 ☺

-Paul






[-- Attachment #2: attachment.html --]
[-- Type: text/html, Size: 27779 bytes --]