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

ZIL: Call brt_pending_add() replaying TX_CLONE_RANGE #15603

Merged
merged 1 commit into from
Nov 29, 2023
Merged

ZIL: Call brt_pending_add() replaying TX_CLONE_RANGE #15603

merged 1 commit into from
Nov 29, 2023

Conversation

amotin
Copy link
Member

@amotin amotin commented Nov 29, 2023

zil_claim_clone_range() takes references on cloned blocks before ZIL replay. Later zil_free_clone_range() drops them after replay or on dataset destroy. The total balance is neutral. It means on actual replay we must take additional references, which would stay in BRT.

Without this blocks could be freed prematurely when either original file or its clone are destroyed. I've observed BRT being emptied and the feature being deactivated after ZIL replay completion, which should not have happened. With the patch I see expected stats.

How Has This Been Tested?

Created a file, synced and frozen the pool, cloned the file to create ZIL record, observed feature active, exported/imported the pool. Without the patch the BRT stats is zero and the feature is back to enabled, with the patch -- non-zero and active.

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:

@amotin amotin requested a review from behlendorf November 29, 2023 01:45
@amotin
Copy link
Member Author

amotin commented Nov 29, 2023

@pjd Please take a look.

@amotin amotin added the Status: Code Review Needed Ready for review and testing label Nov 29, 2023
@amotin amotin mentioned this pull request Nov 29, 2023
Merged
13 tasks
robn
robn approved these changes Nov 29, 2023
View reviewed changes
Copy link
Member

@robn robn left a comment

Choose a reason for hiding this comment

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

Yeah, that makes sense. Consider if you want to update the comment at the top of brt.c, just so it describes all the places where references can be taken.

@amotin
ZIL: Call brt_pending_add() replaying TX_CLONE_RANGE
d7a3568 
zil_claim_clone_range() takes references on cloned blocks before ZIL
replay.  Later zil_free_clone_range() drops them after replay or on
dataset destroy.  The total balance is neutral.  It means on actual
replay we must take additional references, which would stay in BRT.

Without this blocks could be freed prematurely when either original
file or its clone are destroyed.  I've observed BRT being emptied
and the feature being desctivated after ZIL replay completion, which
should not have happened.  With the patch I see expected stats.

Signed-off-by:	Alexander Motin <mav@FreeBSD.org>
Sponsored by:	iXsystems, Inc.
@amotin
Copy link
Member Author

amotin commented Nov 29, 2023

@robn Thanks. I've updated the comments.

behlendorf
behlendorf approved these changes Nov 29, 2023
View reviewed changes
Copy link
Contributor

@behlendorf behlendorf left a comment

Choose a reason for hiding this comment

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

Good find, this makes sense.

@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Nov 29, 2023
@behlendorf behlendorf merged commit a03ebd9 into openzfs:master Nov 29, 2023
18 of 26 checks passed
@amotin amotin deleted the brt_clone_replay branch November 29, 2023 18:52
@pjd
Copy link
Contributor

pjd commented Nov 29, 2023

Interesting... Good catch, Alex. Thank you!

amotin added a commit to amotin/zfs that referenced this pull request Nov 30, 2023
@amotin
ZIL: Remove TX_CLONE_RANGE replay for ZVOLs.
f89c5a0 
zil_claim_clone_range() takes references on cloned blocks before ZIL
replay.  Later zil_free_clone_range() drops them after replay or on
dataset destroy.  The total balance is neutral.  It means we do not
need to do anything (drop the references) for not implemented yet
TX_CLONE_RANGE replay for ZVOLs.

This is a logical follow up to openzfs#15603.

Signed-off-by:	Alexander Motin <mav@FreeBSD.org>
Sponsored by:	iXsystems, Inc.
@amotin amotin mentioned this pull request Nov 30, 2023
Merged
13 tasks
tonyhutter pushed a commit that referenced this pull request Nov 30, 2023
@amotin @tonyhutter
ZIL: Call brt_pending_add() replaying TX_CLONE_RANGE
a8c2560 
zil_claim_clone_range() takes references on cloned blocks before ZIL
replay.  Later zil_free_clone_range() drops them after replay or on
dataset destroy.  The total balance is neutral.  It means on actual
replay we must take additional references, which would stay in BRT.

Without this blocks could be freed prematurely when either original
file or its clone are destroyed.  I've observed BRT being emptied
and the feature being deactivated after ZIL replay completion, which
should not have happened.  With the patch I see expected stats.

Reviewed-by: Kay Pedersen <mail@mkwg.de>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Rob Norris <robn@despairlabs.com>
Signed-off-by:	Alexander Motin <mav@FreeBSD.org>
Sponsored by:	iXsystems, Inc.
Closes #15603
behlendorf pushed a commit that referenced this pull request Dec 1, 2023
@amotin
ZIL: Remove TX_CLONE_RANGE replay for ZVOLs.
bcd83cc 
zil_claim_clone_range() takes references on cloned blocks before ZIL
replay.  Later zil_free_clone_range() drops them after replay or on
dataset destroy.  The total balance is neutral.  It means we do not
need to do anything (drop the references) for not implemented yet
TX_CLONE_RANGE replay for ZVOLs.

This is a logical follow up to #15603.

Reviewed-by: Kay Pedersen <mail@mkwg.de>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by:	Alexander Motin <mav@FreeBSD.org>
Sponsored by:	iXsystems, Inc.
Closes #15612
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Dec 12, 2023
@amotin @lundman
ZIL: Call brt_pending_add() replaying TX_CLONE_RANGE
299793c 
zil_claim_clone_range() takes references on cloned blocks before ZIL
replay.  Later zil_free_clone_range() drops them after replay or on
dataset destroy.  The total balance is neutral.  It means on actual
replay we must take additional references, which would stay in BRT.

Without this blocks could be freed prematurely when either original
file or its clone are destroyed.  I've observed BRT being emptied
and the feature being deactivated after ZIL replay completion, which
should not have happened.  With the patch I see expected stats.

Reviewed-by: Kay Pedersen <mail@mkwg.de>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Rob Norris <robn@despairlabs.com>
Signed-off-by:	Alexander Motin <mav@FreeBSD.org>
Sponsored by:	iXsystems, Inc.
Closes openzfs#15603
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Dec 12, 2023
@amotin @lundman
ZIL: Remove TX_CLONE_RANGE replay for ZVOLs.
1ddea53 
zil_claim_clone_range() takes references on cloned blocks before ZIL
replay.  Later zil_free_clone_range() drops them after replay or on
dataset destroy.  The total balance is neutral.  It means we do not
need to do anything (drop the references) for not implemented yet
TX_CLONE_RANGE replay for ZVOLs.

This is a logical follow up to openzfs#15603.

Reviewed-by: Kay Pedersen <mail@mkwg.de>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by:	Alexander Motin <mav@FreeBSD.org>
Sponsored by:	iXsystems, Inc.
Closes openzfs#15612
mmatuska pushed a commit to mmatuska/zfs that referenced this pull request Dec 26, 2023
@amotin @mmatuska
ZIL: Remove TX_CLONE_RANGE replay for ZVOLs.
fff762e 
zil_claim_clone_range() takes references on cloned blocks before ZIL
replay.  Later zil_free_clone_range() drops them after replay or on
dataset destroy.  The total balance is neutral.  It means we do not
need to do anything (drop the references) for not implemented yet
TX_CLONE_RANGE replay for ZVOLs.

This is a logical follow up to openzfs#15603.

Reviewed-by: Kay Pedersen <mail@mkwg.de>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by:	Alexander Motin <mav@FreeBSD.org>
Sponsored by:	iXsystems, Inc.
Closes openzfs#15612
mmatuska pushed a commit to mmatuska/zfs that referenced this pull request Dec 27, 2023
@amotin @mmatuska
ZIL: Remove TX_CLONE_RANGE replay for ZVOLs.
f1227c4 
zil_claim_clone_range() takes references on cloned blocks before ZIL
replay.  Later zil_free_clone_range() drops them after replay or on
dataset destroy.  The total balance is neutral.  It means we do not
need to do anything (drop the references) for not implemented yet
TX_CLONE_RANGE replay for ZVOLs.

This is a logical follow up to openzfs#15603.

Reviewed-by: Kay Pedersen <mail@mkwg.de>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by:	Alexander Motin <mav@FreeBSD.org>
Sponsored by:	iXsystems, Inc.
Closes openzfs#15612
mmatuska pushed a commit to mmatuska/zfs that referenced this pull request Dec 27, 2023
@amotin @mmatuska
ZIL: Remove TX_CLONE_RANGE replay for ZVOLs.
46c0bfc 
zil_claim_clone_range() takes references on cloned blocks before ZIL
replay.  Later zil_free_clone_range() drops them after replay or on
dataset destroy.  The total balance is neutral.  It means we do not
need to do anything (drop the references) for not implemented yet
TX_CLONE_RANGE replay for ZVOLs.

This is a logical follow up to openzfs#15603.

Reviewed-by: Kay Pedersen <mail@mkwg.de>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by:	Alexander Motin <mav@FreeBSD.org>
Sponsored by:	iXsystems, Inc.
Closes openzfs#15612
@tonyhutter tonyhutter mentioned this pull request Jan 4, 2024
Merged
13 tasks
behlendorf pushed a commit that referenced this pull request Jan 9, 2024
@amotin @behlendorf
ZIL: Remove TX_CLONE_RANGE replay for ZVOLs.
3b8f227 
zil_claim_clone_range() takes references on cloned blocks before ZIL
replay.  Later zil_free_clone_range() drops them after replay or on
dataset destroy.  The total balance is neutral.  It means we do not
need to do anything (drop the references) for not implemented yet
TX_CLONE_RANGE replay for ZVOLs.

This is a logical follow up to #15603.

Reviewed-by: Kay Pedersen <mail@mkwg.de>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Signed-off-by:	Alexander Motin <mav@FreeBSD.org>
Sponsored by:	iXsystems, Inc.
Closes #15612
gamanakis pushed a commit to gamanakis/zfs that referenced this pull request Jan 19, 2024
@amotin @gamanakis
ZIL: Call brt_pending_add() replaying TX_CLONE_RANGE
0a89260 
zil_claim_clone_range() takes references on cloned blocks before ZIL
replay.  Later zil_free_clone_range() drops them after replay or on
dataset destroy.  The total balance is neutral.  It means on actual
replay we must take additional references, which would stay in BRT.

Without this blocks could be freed prematurely when either original
file or its clone are destroyed.  I've observed BRT being emptied
and the feature being deactivated after ZIL replay completion, which
should not have happened.  With the patch I see expected stats.

Reviewed-by: Kay Pedersen <mail@mkwg.de>
Reviewed-by: Brian Behlendorf <behlendorf1@llnl.gov>
Reviewed-by: Rob Norris <robn@despairlabs.com>
Signed-off-by:	Alexander Motin <mav@FreeBSD.org>
Sponsored by:	iXsystems, Inc.
Closes openzfs#15603
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oromenahar oromenahar oromenahar approved these changes

@behlendorf behlendorf behlendorf approved these changes

@robn robn robn 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.
5 participants
@amotin @pjd @robn @behlendorf @oromenahar