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.

Sign up for GitHub

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

Jump to bottom

zfs recv hangs if max recordsize is less than received recordsize #13855

Merged
merged 2 commits into from
Sep 16, 2022
Merged

zfs recv hangs if max recordsize is less than received recordsize #13855

merged 2 commits into from
Sep 16, 2022

Conversation

ixhamza
Copy link
Member

@ixhamza ixhamza commented Sep 8, 2022

Motivation and Context

zfs recv hangs if max recordsize is less than received recordsize

Description

For large blocks, if the record size for the received stream is less than the max_record_size parameter, the receiver is deadlocked in the bqueue enqueue/dequeue due to a greater payload size than the queue initialized with. bqueue should be configured at least twice the length of the received record size for the receiver to operate correctly.

How Has This Been Tested?

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

@ixhamza
Copy link
Member Author

ixhamza commented Sep 8, 2022

cc: @freqlabs, @amotin.

amotin
amotin reviewed Sep 8, 2022
View reviewed changes
Copy link
Member

@amotin amotin left a comment

Choose a reason for hiding this comment

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

I would prefer this to be handled in a different way. It would be more reliable in general if bqueue_enqueue_impl() would not go to sleep if q->bq_size is zero, no matter what is the current block size. It temporary overflow the queue slightly, but its not a problem. It would allow small systems to not waste very limited kernel memory for excessive 32MB queue, that will likely never be required, while if really required, they will still be able to operate one block at a time.

While there, I would also change bqueue_enqueue_impl() to call cv_signal() only as "else" to "if (flush)". It makes no sense to wake up thread(s) twice.

And if go even further from original topic, I see no any reason for this code to use uint64_t. I would change all sizes in bqueue.* to size_t and fill_fraction to uint_t. Again to help small systems to not waste time on 64bit math.

@ghost ghost added the Status: Revision Needed Changes are required for the PR to be accepted label Sep 8, 2022
@ixhamza ixhamza force-pushed the NAS-113932 branch 2 times, most recently from c51fa05 to 8ebb0c2 Compare September 8, 2022 21:19
@ixhamza ixhamza requested a review from amotin September 8, 2022 21:21
amotin
amotin approved these changes Sep 8, 2022
View reviewed changes
Copy link
Member

@amotin amotin left a comment

Choose a reason for hiding this comment

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

Thank you!

@amotin amotin added Status: Code Review Needed Ready for review and testing and removed Status: Revision Needed Changes are required for the PR to be accepted labels Sep 8, 2022
@ixhamza
bqueue optimizations and fix for deadlock
656cdf8 
- Some optimizations for bqueue enqueue/dequeue.
- Added a fix to prevent deadlock when both bqueue_enqueue_impl()
and bqueue_dequeue() waits for signal to be triggered.

Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
@ixhamza
Updating comment as default value of zfs_max_recordsize is 16MB
86bb6e7 
Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
@ghost ghost added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Sep 16, 2022
@behlendorf behlendorf merged commit 577d41d into openzfs:master Sep 16, 2022
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Sep 19, 2022
@ixhamza @tonyhutter
zfs recv hangs if max recordsize is less than received recordsize
a5b0d42 
- Some optimizations for bqueue enqueue/dequeue.
- Added a fix to prevent deadlock when both bqueue_enqueue_impl()
and bqueue_dequeue() waits for signal to be triggered.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Ryan Moeller <ryan@iXsystems.com>
Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
Closes openzfs#13855
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Sep 21, 2022
@ixhamza @tonyhutter
zfs recv hangs if max recordsize is less than received recordsize
9c97c95 
- Some optimizations for bqueue enqueue/dequeue.
- Added a fix to prevent deadlock when both bqueue_enqueue_impl()
and bqueue_dequeue() waits for signal to be triggered.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Ryan Moeller <ryan@iXsystems.com>
Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
Closes openzfs#13855
tonyhutter pushed a commit to tonyhutter/zfs that referenced this pull request Sep 22, 2022
@ixhamza @tonyhutter
zfs recv hangs if max recordsize is less than received recordsize
d5105f0 
- Some optimizations for bqueue enqueue/dequeue.
- Added a fix to prevent deadlock when both bqueue_enqueue_impl()
and bqueue_dequeue() waits for signal to be triggered.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Ryan Moeller <ryan@iXsystems.com>
Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
Closes openzfs#13855
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Sep 27, 2022
@ixhamza @andrewc12
zfs recv hangs if max recordsize is less than received recordsize
3e3f26a 
- Some optimizations for bqueue enqueue/dequeue.
- Added a fix to prevent deadlock when both bqueue_enqueue_impl()
and bqueue_dequeue() waits for signal to be triggered.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Ryan Moeller <ryan@iXsystems.com>
Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
Closes openzfs#13855
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Oct 1, 2022
@ixhamza @andrewc12
zfs recv hangs if max recordsize is less than received recordsize
960fd25 
- Some optimizations for bqueue enqueue/dequeue.
- Added a fix to prevent deadlock when both bqueue_enqueue_impl()
and bqueue_dequeue() waits for signal to be triggered.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Ryan Moeller <ryan@iXsystems.com>
Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
Closes openzfs#13855
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Oct 1, 2022
@ixhamza @andrewc12
zfs recv hangs if max recordsize is less than received recordsize
76bdf87 
- Some optimizations for bqueue enqueue/dequeue.
- Added a fix to prevent deadlock when both bqueue_enqueue_impl()
and bqueue_dequeue() waits for signal to be triggered.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Ryan Moeller <ryan@iXsystems.com>
Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
Closes openzfs#13855
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Oct 1, 2022
@ixhamza @andrewc12
zfs recv hangs if max recordsize is less than received recordsize
31d2d00 
- Some optimizations for bqueue enqueue/dequeue.
- Added a fix to prevent deadlock when both bqueue_enqueue_impl()
and bqueue_dequeue() waits for signal to be triggered.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Ryan Moeller <ryan@iXsystems.com>
Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
Closes openzfs#13855
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Oct 1, 2022
@ixhamza @andrewc12
zfs recv hangs if max recordsize is less than received recordsize
7d41f5b 
- Some optimizations for bqueue enqueue/dequeue.
- Added a fix to prevent deadlock when both bqueue_enqueue_impl()
and bqueue_dequeue() waits for signal to be triggered.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Ryan Moeller <ryan@iXsystems.com>
Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
Closes openzfs#13855
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Oct 1, 2022
@ixhamza @andrewc12
zfs recv hangs if max recordsize is less than received recordsize
9c8268f 
- Some optimizations for bqueue enqueue/dequeue.
- Added a fix to prevent deadlock when both bqueue_enqueue_impl()
and bqueue_dequeue() waits for signal to be triggered.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Ryan Moeller <ryan@iXsystems.com>
Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
Closes openzfs#13855
andrewc12 pushed a commit to andrewc12/openzfs that referenced this pull request Oct 1, 2022
@ixhamza @andrewc12
zfs recv hangs if max recordsize is less than received recordsize
271645e 
- Some optimizations for bqueue enqueue/dequeue.
- Added a fix to prevent deadlock when both bqueue_enqueue_impl()
and bqueue_dequeue() waits for signal to be triggered.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Ryan Moeller <ryan@iXsystems.com>
Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
Closes openzfs#13855
snajpa pushed a commit to vpsfreecz/zfs that referenced this pull request Oct 22, 2022
@ixhamza @snajpa
zfs recv hangs if max recordsize is less than received recordsize
5cf396e 
- Some optimizations for bqueue enqueue/dequeue.
- Added a fix to prevent deadlock when both bqueue_enqueue_impl()
and bqueue_dequeue() waits for signal to be triggered.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Ryan Moeller <ryan@iXsystems.com>
Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
Closes openzfs#13855
snajpa pushed a commit to vpsfreecz/zfs that referenced this pull request Oct 22, 2022
@ixhamza @snajpa
zfs recv hangs if max recordsize is less than received recordsize
fbd0d7a 
- Some optimizations for bqueue enqueue/dequeue.
- Added a fix to prevent deadlock when both bqueue_enqueue_impl()
and bqueue_dequeue() waits for signal to be triggered.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Ryan Moeller <ryan@iXsystems.com>
Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
Closes openzfs#13855
snajpa pushed a commit to vpsfreecz/zfs that referenced this pull request Oct 23, 2022
@ixhamza @snajpa
zfs recv hangs if max recordsize is less than received recordsize
ca847ac 
- Some optimizations for bqueue enqueue/dequeue.
- Added a fix to prevent deadlock when both bqueue_enqueue_impl()
and bqueue_dequeue() waits for signal to be triggered.

Reviewed-by: Alexander Motin <mav@FreeBSD.org>
Reviewed-by: Ryan Moeller <ryan@iXsystems.com>
Signed-off-by: Ameer Hamza <ahamza@ixsystems.com>
Closes openzfs#13855
@ixhamza ixhamza deleted the NAS-113932 branch August 2, 2023 17:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@amotin amotin amotin approved these changes

Assignees
No one assigned
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ixhamza @amotin @behlendorf