Skip to content

fix(initiator): volume expansion avoid target disconnect & sync dm size#252

Merged
derekbit merged 1 commit into
longhorn:mainfrom
davidcheng0922:issue-12359-encrypt-volume-expansion
Jan 2, 2026
Merged

fix(initiator): volume expansion avoid target disconnect & sync dm size#252
derekbit merged 1 commit into
longhorn:mainfrom
davidcheng0922:issue-12359-encrypt-volume-expansion

Conversation

@davidcheng0922
Copy link
Copy Markdown
Contributor

Which issue(s) this PR fixes:

longhorn/longhorn#12359

What this PR does / why we need it:

  • Option for not stopping target in StartNvmeTCPInitiator:

    During volume expansion, we avoid disconnect the target. Any disconnect / connect process may create orphan device.

  • Add function SyncDmDeviceSize:

    Kernel takes time for getting the latest size, the dm table may stay previous size if we don't wait for it

Special notes for your reviewer:

Additional documentation or context

@codecov
Copy link
Copy Markdown

codecov Bot commented Dec 30, 2025

Codecov Report

❌ Patch coverage is 0% with 30 lines in your changes missing coverage. Please review.
✅ Project coverage is 0.00%. Comparing base (7df556b) to head (42d3288).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pkg/initiator/initiator.go 0.00% 29 Missing ⚠️
app/cmd/nvmecli/nvmecli.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##            main    #252   +/-   ##
=====================================
  Coverage   0.00%   0.00%           
=====================================
  Files         38      38           
  Lines       4874    4898   +24     
=====================================
- Misses      4874    4898   +24     
Flag Coverage Δ
unittests 0.00% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

DefaultUblkQueueDepth = 128
DefaultUblkNumberOfQueue = 1

DmSectorSize = 512
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.

default hard-coded value?

Copy link
Copy Markdown
Contributor Author

@davidcheng0922 davidcheng0922 Dec 31, 2025

Choose a reason for hiding this comment

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

Investigate it

Copy link
Copy Markdown
Contributor Author

@davidcheng0922 davidcheng0922 Dec 31, 2025

Choose a reason for hiding this comment

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

After investigating, it seems in linux kernel and dmsetup, it should always be 512, no matter the sector size of physical device.

/**
 * The type used for indexing onto a disc or disc partition.
 *
 * Linux always considers sectors to be 512 bytes long independently
 * of the devices real block size.
 *
 * blkcnt_t is the type of the inode's block count.
 */
typedef u64 sector_t;

ref: https://github.com/torvalds/linux/blob/master/include/linux/types.h#L130-L138

/* FIXME Should be elsewhere */
#define SECTOR_SHIFT 9L

ref: https://android.googlesource.com/platform/external/lvm2/+/refs/heads/main/tools/dmsetup.c#103

Do I still implement a helper function for this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

If I'm wrong, please correct me, thank you

Comment thread pkg/initiator/initiator.go Outdated
Signed-off-by: David Cheng <davidcheng0922@gmail.com>
@davidcheng0922 davidcheng0922 force-pushed the issue-12359-encrypt-volume-expansion branch from 05dbd78 to 42d3288 Compare December 31, 2025 07:08
Copy link
Copy Markdown
Member

@derekbit derekbit left a comment

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 3ace160 into longhorn:main Jan 2, 2026
8 checks passed
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.

2 participants