Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Stable 1.12 #5508

Merged
merged 24 commits into from
Nov 29, 2022
Merged

Stable 1.12 #5508

merged 24 commits into from
Nov 29, 2022

Conversation

lplewa
Copy link
Member

@lplewa lplewa commented Oct 28, 2022

This change is Reviewable

pbalcer and others added 23 commits July 20, 2022 12:32
This patch fixes a bug where an invalidated undo log might have not been
properly cleaned up after a crash. Such a log would show that it has
no entries, so seemingly nothing to cleanup. However, a single log
can be composed of multiple allocations in a singly-linked list.
Typically all allocations but the first one are deallocated as part
of the cleanup process on recovery. And the log creation logic
makes decisions based on that assumption. And that assumption
was being violated in certain circumstanes where applications
crashed in large transactions, after a commit but before the undo log
was fully invalidated.

Reported-by: @task3r
Right now pmem2_xyz_async family of functions don't use
VDM_F_MEM_DURABLE vdm flags, they merely check if they
are supported by the vdm. This might lead to a situation
where a vdm supports durable writes, but pmem2 doesn't
take advantage of that support, even when it should.
This patch addresses that by using the durable flag
whenever that's appropriate, and handling the flag
correctly in pmem2 mover implementation.
obj: always perform cleanup of logs with leftover state
pmem2: make sure async functions use proper vdm flags
A recent tried to globally add a new flag
in the makefiles, which inadvertently caused
the build system to stop adding important
compile flags like '-O2'...
common: fix default cflags in makefiles
pmem2: enable movdir64b compilation but disable it in the runtime
This patch fixes a bug where free prior to any allocs,
combined with reservations, could have led to overlapping
allocations - which ultimately was causing corrupted heap
and incorrect statistics.

This problem is caused by lazy heap runtime state reclamation.
Heap runtime state is rebuilt lazily whenever required
to serve allocation requests. Deallocations (free) simply update
persistent metadata and, in case of huge allocations, inserts
the freed chunk into a container of free chunks. On reclaim,
all free chunks not already deallocated are inserted into a freelist.

This would have been fine, but libpmemobj's allocator enables
software to reserve a chunk, removing it from the heap runtime state
without updating the persistent on-media layout.
This means that software can deallocate a chunk, reserve
that same chunk, allocate something normally - triggering
heap zone reclamation, and then it can finally
publish (actually persistently allocate) that reserved chunk.
This can lead to the same chunk being potentially allocated twice...

This patch fixes this problem by ensuring that object's
zone is fully processed and reclaimed prior to deallocation.

Reported-by: @jolivier23
obj: ensure zones are reclaimed prior to free (1.11)
f3b809217 Merge pull request pmem#110 from lplewa/master
11fddd440 masync: suppres unused parameters in future.h
1c6ba8d18 masync: 0.2.0-rc2 release
76ba3a10f Merge pull request pmem#108 from pbalcer/fix-use-after-free-membuf
c2885af40 common: fix use-after-free in membuf in mt scenarios
1914de3de Merge pull request pmem#106 from DamianDuy/updateAsyncDoc
ffedbde35 doc: update async property documentation
12e9fa42c Merge pull request pmem#105 from DamianDuy/addAsyncDoc
cb1734572 doc: add async property to documentation
3b44a2cb9 Merge pull request pmem#100 from DamianDuy/addAsyncFlag
9f740e491 masync: add flag indicating that future is async
b7216cab1 Merge pull request pmem#90 from DamianDuy/addFlushOp
01ef54aa4 masync: add flush operation for DML data mover
c0c8f9ddf Merge pull request pmem#104 from lukaszstolarczuk/update-docs
eb41ffcee doc: update code styling to properly appear in manpage
e573f7e89 Merge pull request pmem#102 from lplewa/readback
c98664d80 Merge pull request pmem#103 from pbalcer/atomic-load-ringbuf
2490b0cee common: use correct atomics in ringbuf
428ecdaf9 masync: add destination_readback flag to persistent memory writes
f1203f533 Merge pull request pmem#71 from kswiecicki/masync-example-hashmap
bc1d04ef7 masync: add hashmap example
352b4c1aa Merge pull request pmem#92 from lukaszstolarczuk/add-macro-is-init
a6e312918 Merge pull request pmem#96 from kilobyte/no-arch-ifdefs
27e8795a8 masync: add macro to check if chain entry was initialized
790bb3522 Merge pull request pmem#99 from lukaszstolarczuk/fix-manpage-header
af2e47738 common: replace improper 'RPMA' in man pages with 'MINIASYNC'
01e68810b masync: drop unused #ifdefs that block portability

git-subtree-dir: src/deps/miniasync
git-subtree-split: f3b809217fc6f4401c227e707708a803725290bc
a8d51f0a4 Merge pull request pmem#111 from kswiecicki/kw-issues
b1efa1c99 Merge pull request pmem#109 from KFilipek/add-casting
dc88b63a0 masync: fix hashmap memory allocation
97055b841 masync: assert hashmap example copied value
701844c99 masync: fix hashmap example allocation checking
93b1698ff masync: fix hashmap example cleanup
7fa75e02f masync: avoid casting error on C++ compilation

git-subtree-dir: src/deps/miniasync
git-subtree-split: a8d51f0a46d8143cbbc5ac8ca272ad3ba19fed6e
@codecov
Copy link

codecov bot commented Oct 28, 2022

Codecov Report

Merging #5508 (64d2aa4) into master (a664a9f) will decrease coverage by 0.25%.
The diff coverage is 89.47%.

@@            Coverage Diff             @@
##           master    #5508      +/-   ##
==========================================
- Coverage   72.12%   71.87%   -0.26%     
==========================================
  Files         193      195       +2     
  Lines       30866    30727     -139     
==========================================
- Hits        22263    22084     -179     
- Misses       8603     8643      +40     

Copy link
Member

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it looks 🆗 to me, but I'm wondering about the last commit: "Merge remote-tracking branch 'upstream/master' into stable-1.12". It seems like the master was merged into the stable branch... Commits range in this PR looks ok, so I'm not sure if we should worry 😉

Reviewed 9 of 93 files at r1.
Reviewable status: 9 of 93 files reviewed, all discussions resolved (waiting on @wlemkows)

@lukaszstolarczuk
Copy link
Member

lukaszstolarczuk commented Oct 31, 2022

I merged such PR into my master branch, and it looks acceptable, but I would still re-do this merge, as expected. Meaning, "the other way around" - you should rather merge a stable branch into master.

Right now, after merging this PR you'd have an extra "connection" between master and stable, see magenta and blue lines on the image below:

image

@lplewa
Copy link
Member Author

lplewa commented Nov 7, 2022

Well, this is correct. I created merge commit as i had to resolve conflict. GH cannon do FF merge, that's why after "merge pull reqest" thru GH interface you have a second merge commit. This is intended, but if you really don't want this you can "merge this PR by hand", and close this PR. Theoretically i could do the merge in other way, but this would change only parent order, which doesn't matter.

@lukaszstolarczuk
Copy link
Member

I've done my share of manual "merges" and I agree, you should see the "merge" commit in here, but it should be the other way around: not "Merge remote-tracking branch 'upstream/master' into stable-1.12" but rather "Merge 'upstream/stable-1.12' into master".

I believe, what you did, you executed:

git checkout stable-1.12
git merge upstream/master

and I believe, you should do:

git checkout master
git merge upstream/stable-1.12

😉

Copy link
Member

@lukaszstolarczuk lukaszstolarczuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Much better 😄 Thanks! 👍

Reviewed all commit messages.
Reviewable status: 9 of 93 files reviewed, all discussions resolved (waiting on @wlemkows)

@lplewa
Copy link
Member Author

lplewa commented Nov 15, 2022

Any update? @wlemkows can you please review/merge this PR?

@wlemkows wlemkows merged commit 5608478 into pmem:master Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants