Conversation
97e659c to
5d27fac
Compare
grom72
left a comment
There was a problem hiding this comment.
@grom72 reviewed 1 of 2 files at r1.
Reviewable status: 1 of 2 files reviewed, 1 unresolved discussion (waiting on @osalyk)
a discussion (no related file):
Why do you want this change merged into stable-2.1 and not to master?
grom72
left a comment
There was a problem hiding this comment.
Reviewable status: 1 of 2 files reviewed, 2 unresolved discussions (waiting on @janekmi and @osalyk)
a discussion (no related file):
I think we should explain why we can have the first chunk of type CHUNK_TYPE_FREE.
5d27fac to
8d7a434
Compare
janekmi
left a comment
There was a problem hiding this comment.
Reviewable status: 0 of 6 files reviewed, 2 unresolved discussions (waiting on @grom72 and @osalyk)
a discussion (no related file):
Previously, grom72 (Tomasz Gromadzki) wrote…
Why do you want this change merged into
stable-2.1and not to master?
Done.
a discussion (no related file):
Previously, grom72 (Tomasz Gromadzki) wrote…
I think we should explain why we can have the first chunk of type
CHUNK_TYPE_FREE.
Done.
8d7a434 to
54a830b
Compare
54a830b to
2b6e2a2
Compare
osalyk
left a comment
There was a problem hiding this comment.
@osalyk reviewed 1 of 2 files at r1, 5 of 5 files at r3, all commit messages.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @grom72)
grom72
left a comment
There was a problem hiding this comment.
@grom72 reviewed all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @janekmi)
ChangeLog line 5 at r3 (raw file):
* Version X.X.X - Fix a very unlikely issue in the PMEMOBJ allocator with a potential to corrupt the allocator's metadata (daos-stack/pmdk#24, DAOS-18195).
Do we fix something or not?
Suggestion:
Fix an issue
janekmi
left a comment
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @grom72)
ChangeLog line 5 at r3 (raw file):
Previously, grom72 (Tomasz Gromadzki) wrote…
Do we fix something or not?
We do. Moreover, “very unlikely,” despite being vague, is a qualifier that gives the reader more insight into the problem. Considering there is no more detailed information on how frequently the problem occurs, it is the most accurate explanation we can provide.
Why exactly do you think we should not provide this information?
... chunk reinit Issue Huge memory blocks with the header chunk of type CHUNK_TYPE_FREE had not reinitialized their footer chunk. Context - Allocator divides the available space into zones and zones into smaller chunks. - All chunk headers belonging to a single zone are stored in an array. - Chunk header describes its: type, size_idx and flags. - Chunk can be of a few types. The types important for this patch are: CHUNK_TYPE_FOOTER, CHUNK_TYPE_FREE, and CHUNK_TYPE_USED. - CHUNK_TYPE_FREE and _USED are chunk types marking the beginning of so called huge memory block. This means this chunk and a number of chunks following it (size_idx) constitues a single allocation or a free memory. - The last chunk belonging to a huge memory block ought to be of type CHUNK_TYPE_FOOTER. Its size_idx allows to easily find the first chunk belonging to this huge memory block and determine the huge memory block type. - Huge memory blocks' footers are written immediately but persited lazily. It is not a problem at runtime since the footers are there at runtime. But in case of a crash the footers may be not persisted properly and missing on open. So, when the allocator is booted up it recreates footers just in case any of them is missing. Note: The huge memory block's first chunk header and the last chunk header (footer) are not written nor persisted in any way transactionally. The first chunk header occupies only 8 bytes so it is written and persisted atomically. But its footer is written independently and not explicitly persisted. Note: The first chunk header contains all the required info to recreate footer. The last chunk header is there only to make huge memory blocks coalescing easier to compute. Reading the footer allows to immediately find the memory block leaving just before the memory block which neighbours one might want to find. This patch makes sure the huge memory block's footer is recreated no matter whather its type is CHUNK_TYPE_FREE or CHUNK_TYPE_USED. The patch is inspired by work done for DAOS' DAV allocator (DAOS-18195) which is heavily based on the PMEMOBJ allocator. Signed-off-by: Jan Michalski <jan-marian.michalski@hpe.com> Inspired-by: Sherin T George <sherin-t.george@hpe.com>
2b6e2a2 to
f09bc1d
Compare
janekmi
left a comment
There was a problem hiding this comment.
Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @grom72 and @osalyk)
ChangeLog line 5 at r3 (raw file):
Previously, janekmi (Jan Michalski) wrote…
We do. Moreover, “very unlikely,” despite being vague, is a qualifier that gives the reader more insight into the problem. Considering there is no more detailed information on how frequently the problem occurs, it is the most accurate explanation we can provide.
Why exactly do you think we should not provide this information?
We agreed:
- "very unlikely" does not assume any kind of randomness. Rather it means a number of scenarios causing the problems is relatively small compared to the number of all possible scenarios.
- We also agreed since these scenarios include power-failures/crash they are very important. Since they constitute the key deliverable of the PMDK libraries as such.
janekmi
left a comment
There was a problem hiding this comment.
Reviewable status: 5 of 6 files reviewed, 1 unresolved discussion (waiting on @grom72 and @osalyk)
ChangeLog line 5 at r3 (raw file):
Previously, janekmi (Jan Michalski) wrote…
We agreed:
- "very unlikely" does not assume any kind of randomness. Rather it means a number of scenarios causing the problems is relatively small compared to the number of all possible scenarios.
- We also agreed since these scenarios include power-failures/crash they are very important. Since they constitute the key deliverable of the PMDK libraries as such.
Done.
grom72
left a comment
There was a problem hiding this comment.
@grom72 reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @janekmi)
janekmi
left a comment
There was a problem hiding this comment.
@janekmi reviewed 1 of 1 files at r4, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @janekmi)
Requires
Issue
Huge memory blocks with the header chunk of type CHUNK_TYPE_FREE had not reinitialized their footer chunk.
Context
Note: The huge memory block's first chunk header and the last chunk header (footer) are not written nor persisted in any way transactionally. The first chunk header occupies only 8 bytes so it is written and persisted atomically. But its footer is written independently and not explicitly persisted.
Note: The first chunk header contains all the required info to recreate footer. The last chunk header is there only to make huge memory blocks coalescing easier to compute. Reading the footer allows to immediately find the memory block leaving just before the memory block which neighbours one might want to find.
This patch makes sure the huge memory block's footer is recreated no matter whather its type is CHUNK_TYPE_FREE or CHUNK_TYPE_USED.
The patch is inspired by work done for DAOS' DAV allocator (DAOS-18195) which is heavily based on the PMEMOBJ allocator.
Inspired-by: Sherin T George sherin-t.george@hpe.com
This change is