Skip to content

Implement bdev_lvol_start_deep_copy and bdev_lvol_check_deep_copy API#61

Merged
derekbit merged 8 commits into
longhorn:longhorn-v25.05from
PhanLe1010:7794-cloning-longhorn-v25.05
Aug 6, 2025
Merged

Implement bdev_lvol_start_deep_copy and bdev_lvol_check_deep_copy API#61
derekbit merged 8 commits into
longhorn:longhorn-v25.05from
PhanLe1010:7794-cloning-longhorn-v25.05

Conversation

@PhanLe1010

@PhanLe1010 PhanLe1010 commented Jul 23, 2025 •
edited
Loading

Copy link
Copy Markdown
  1. bdev_lvol_start_deep_copy: Start a deep copy of a lvol over a given bdev
  2. bdev_lvol_check_deep_copy: Get deep copy status

longhorn/longhorn#7794

@PhanLe1010 PhanLe1010 changed the title Implement bdev_lvol_start_deep_copy API Implement bdev_lvol_start_deep_copy and bdev_lvol_check_deep_copy API Jul 23, 2025
@PhanLe1010

PhanLe1010 commented Jul 23, 2025

Copy link
Copy Markdown
Author

Still need to add some unit tests and code refinement but the logic is ready for reviewing

@DamiaSan

DamiaSan commented Jul 23, 2025

Copy link
Copy Markdown

At first glance, you should break each of these 2 commits into multiple commits, one for each layer (blob, lvol, vbdev_lvol, rpc). Every commit should have the corresponding unit test and the commit over the rpc layer (vbdev_lvol_rpc.c, lvol.py and rpc.py) should contain an e2e test.
See SPDK Development.

@PhanLe1010 PhanLe1010 force-pushed the 7794-cloning-longhorn-v25.05 branch from 211d77e to 6eecaa4 Compare July 24, 2025 00:53
@PhanLe1010

PhanLe1010 commented Jul 24, 2025 •
edited
Loading

Copy link
Copy Markdown
Author

Thanks @DamiaSan for the feedback. I am working on breaking down the commits and adding tests. In the mean time, the logic of the PR is ready for review. Thank you!

DamiaSan
DamiaSan reviewed Jul 25, 2025
View reviewed changes
Comment thread lib/blob/blobstore.c Outdated
Comment thread lib/blob/blobstore.c Outdated
Comment thread lib/blob/blobstore.c
Comment thread lib/blob/blobstore.c
@PhanLe1010 PhanLe1010 force-pushed the 7794-cloning-longhorn-v25.05 branch from 1357f46 to ab2a5cd Compare July 29, 2025 02:15
@PhanLe1010

PhanLe1010 commented Jul 29, 2025

Copy link
Copy Markdown
Author

Thanks @DamiaSan for the feedback. I am working on breaking down the commits and adding tests. In the mean time, the logic of the PR is ready for review. Thank you!

All completed

Could you help to review @derekbit @DamiaSan @shuo-wu @c3y1huang ? Thank you

@PhanLe1010 PhanLe1010 force-pushed the 7794-cloning-longhorn-v25.05 branch from ab2a5cd to face4f9 Compare July 29, 2025 02:22
@PhanLe1010
test/lvol: fix formatting
4ec8bfe 
These are detected by running scripts/check_format.sh

Signed-off-by: Phan Le <phan.le@suse.com>
@PhanLe1010 PhanLe1010 force-pushed the 7794-cloning-longhorn-v25.05 branch 3 times, most recently from 54420ca to c807f55 Compare July 29, 2025 03:42
@PhanLe1010 PhanLe1010 requested a review from DamiaSan July 29, 2025 03:48
DamiaSan
DamiaSan previously approved these changes Aug 5, 2025
View reviewed changes

@DamiaSan DamiaSan left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

LGTM

@PhanLe1010

PhanLe1010 commented Aug 5, 2025

Copy link
Copy Markdown
Author

LGTM

Thanks @DamiaSan for the review. Should we contribute to upstream?

shuo-wu
shuo-wu reviewed Aug 5, 2025
View reviewed changes

@shuo-wu shuo-wu left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

In general LGTM

Comment thread lib/blob/blobstore.c
derekbit
derekbit reviewed Aug 6, 2025
View reviewed changes

@derekbit derekbit left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I will continue reviewing the PR later today. More on the error handling.

Comment thread test/unit/lib/bdev/vbdev_lvol.c/vbdev_lvol_ut.c
Comment thread lib/blob/blobstore.c
Comment thread lib/blob/blobstore.c Outdated
@PhanLe1010 PhanLe1010 force-pushed the 7794-cloning-longhorn-v25.05 branch from c807f55 to 4c08580 Compare August 6, 2025 00:46
shuo-wu
shuo-wu previously approved these changes Aug 6, 2025
View reviewed changes
@PhanLe1010
blob: add deep copy over a given device
6fbdd7e 
A deep copy will copy over the destination device the
cluster allocated to the blob or blob's ancestors

Signed-off-by: Phan Le <phan.le@suse.com>
@PhanLe1010
lvol: add deep copy over a given device
7cb7e55 
This is mostly a wrapper around spdk_bs_blob_deep_copy(), but it also
performs some checks over lvol, bs_dev and their compatibility.

Signed-off-by: Phan Le <phan.le@suse.com>
@PhanLe1010
vbdev_lvol: add deep copy over a given bdev
5cb7650 
This is mostly a wrapper around spdk_lvol_deep_copy(), but it
also has the aim to create the spdk_bs_dev device from the given
bdev.

Signed-off-by: Phan Le <phan.le@suse.com>
@PhanLe1010
vbdev_lvol: add deep copy RPC
2d13764 
Add RPC interface to start a deep copy of a read only
lvol over a given bdev. Next patch will add another RPC
to check the status of this operation.

Signed-off-by: Phan Le <phan.le@suse.com>
@PhanLe1010
vbdev_lvol: add deep copy status RPC
8376419 
Add RPC interface to get info about a deep copy operation,
like the total number of clusters to be processed, the
actual number of clusters processed and the result of the operation.

Signed-off-by: Phan Le <phan.le@suse.com>
@PhanLe1010
test/lvol: add deep copy test
9ddf57d 
Added test to check deep copy and deep copy status behavior.

Signed-off-by: Phan Le <phan.le@suse.com>
@PhanLe1010
doc: add lvol/blob deep copy descriptions
2e0af6c 
CHANGELOG.md, lvol.md and blob.md have been updated with
informations about the deep copy functionality.

Signed-off-by: Phan Le <phan.le@suse.com>
@PhanLe1010 PhanLe1010 force-pushed the 7794-cloning-longhorn-v25.05 branch from 4c08580 to 2e0af6c Compare August 6, 2025 01:22
@DamiaSan

DamiaSan commented Aug 6, 2025

Copy link
Copy Markdown

LGTM

Thanks @DamiaSan for the review. Should we contribute to upstream?

It would be a great thing.
I suggest starting on spdk slack channel to understand if the development can be of interest by core maintainers, and then put it on spdk gerrit for the review process.

derekbit
derekbit approved these changes Aug 6, 2025
View reviewed changes

@derekbit derekbit left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

@derekbit derekbit merged commit ea6cb0b into longhorn:longhorn-v25.05 Aug 6, 2025
2 checks passed
@PhanLe1010 PhanLe1010 mentioned this pull request Sep 11, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@derekbit derekbit derekbit approved these changes
@shuo-wu shuo-wu shuo-wu left review comments
@c3y1huang c3y1huang Awaiting requested review from c3y1huang
@innobead innobead Awaiting requested review from innobead
+1 more reviewer
@DamiaSan DamiaSan DamiaSan left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@PhanLe1010 @DamiaSan @derekbit @shuo-wu