Skip to content

Improve snapshot listing times#1963

Open
tiagolobocastro wants to merge 14 commits intodevelopfrom
snap-perf
Open

Improve snapshot listing times#1963
tiagolobocastro wants to merge 14 commits intodevelopfrom
snap-perf

Conversation

@tiagolobocastro
Copy link
Copy Markdown
Member

test: add multi-replica and clone check to lvol_snap_list

Ensures the num_clones is set correctly
Spreads snapshots across a number of replicas
Creates a clone for each snapshot.

todo: investigate warn message:
Using passed in clear method 0x2 instead of stored value of 0x0

fix(lvs): don't query snapshot parent for listing

I'm not sure why this was being done as we can simply return the uuid
itself, rather than query only to return the same value...
It's possible that we wanted to return the uuid only if the parent
still existings, but it's not clear from the api and would be a strange
behaviour...

So we now drop this, and simply return the parent uuid.

refactor(lvs): reorder xattr to list parent id as the first

This makes snapshot searching quicker, as we don't need to load
the other attributes when it's a mismatch.

refactor(lvs): clarify is_snapshot_clone api

Renames is_snapshot_clone into clone_source, where the Lvol snapshot
is returned if found.
Added new api method is_clone which checks if the current Lvol is a
cloned lvol.

This both clarifies the previous method and also makes it more efficient
as we don't need to lookup the source snapshot all the times.

refactor(lvs): replace list clones logic with spdk immediate clone

Rather than looping all bdevs, we get the list of clones directly
from the snapshot using spdk_lvol_iter_immediate_clones.
Further we trim this down to "real_clones" by filtering out snapshots
from the returned list.

todo: can we optimize this process, we don't need 2 lists?

refactor(lvs): compare snapshot uuid_str with potential clone parent

When listing all potential bdevs to find matching clones, don't bother
looking up the snapshot from each potential clone.
Instead, simply compare the uuid with the snapshot.

refactor(lvs): add lvol uuid lookup function

Lookup lvol directly, rather than iterating all bdevs.

refactor(lvs): count snapshot clones using spdk_blob_get_clones

This api is much faster than iterating all bdevs, map them to lvols and then
iterating again all bdevs to find the clones...

This reduces the lvol_snap_list all the way to 50ms!

refactor(lvs): simplify bdev to Lvol mappings

Simplify all the iterations which try to load a bdev as an lvol.

Also when comparing lvol's when listing clones, don't use the uuid
as the comparisson method as it's an expensive operation requiring
allocation of memory and string copies.
Rather, we can simply compare the spdk_lvol pointer.

This further reduces the slvol_snap_list test by another ~.5s

refactor(lvs/xattr): change blob attr api to use compile constants

Using a type which can yield &CStr avoids having to convert strings
to CString at runtime.
This reduces the snapshot list test by ~0.5s.

refactor: compare bdev uuid bytes rather than string

It's faster to compare the 16bytes rather than the stringified version.

refactor: use new api for blob xattr reading

We don't always need an owned string for the attribute values, as sometimes
we only want to check/compare the value.
I refactored the apis to returned a string with the lifetime of the lvol or
the specified blob.
The internal function is marked as unsafe and should not be directly used
as the returned value is a pointer from the lvol, and must not be used
beyong the blob's lifetime.

test: add lvs snapshot list timing test

Creates 3000 snapshots from a single replica and time out long it takes
to list all snapshots.

@tiagolobocastro tiagolobocastro requested a review from a team as a code owner April 7, 2026 19:44
@tiagolobocastro tiagolobocastro changed the title Snap perf Improve snapshot listing times Apr 7, 2026
@tiagolobocastro
Copy link
Copy Markdown
Member Author

bors try

bors-openebs-mayastor bot pushed a commit that referenced this pull request Apr 7, 2026
@bors-openebs-mayastor
Copy link
Copy Markdown

try

Build failed:

Creates 3000 snapshots from a single replica and time out long it takes
to list all snapshots.

Signed-off-by: Tiago Castro <tiagolobocastro@gmail.com>
We don't always need an owned string for the attribute values, as sometimes
we only want to check/compare the value.
I refactored the apis to returned a string with the lifetime of the lvol or
the specified blob.
The internal function is marked as unsafe and should not be directly used
as the returned value is a pointer from the lvol, and must not be used
beyong the blob's lifetime.

Signed-off-by: Tiago Castro <tiagolobocastro@gmail.com>
It's faster to compare the 16bytes rather than the stringified version.

Signed-off-by: Tiago Castro <tiagolobocastro@gmail.com>
Using a type which can yield &CStr avoids having to convert strings
to CString at runtime.
This reduces the snapshot list test by ~0.5s.

Signed-off-by: Tiago Castro <tiagolobocastro@gmail.com>
Simplify all the iterations which try to load a bdev as an lvol.

Also when comparing lvol's when listing clones, don't use the uuid
as the comparisson method as it's an expensive operation requiring
allocation of memory and string copies.
Rather, we can simply compare the spdk_lvol pointer.

This further reduces the slvol_snap_list test by another ~.5s

Signed-off-by: Tiago Castro <tiagolobocastro@gmail.com>
This api is much faster than iterating all bdevs, map them to lvols and then
iterating again all bdevs to find the clones...

This reduces the lvol_snap_list all the way to <100ms!

Signed-off-by: Tiago Castro <tiagolobocastro@gmail.com>
Lookup lvol directly, rather than iterating all bdevs.

Signed-off-by: Tiago Castro <tiagolobocastro@gmail.com>
When listing all potential bdevs to find matching clones, don't bother
looking up the snapshot from each potential clone.
Instead, simply compare the uuid with the snapshot.

Signed-off-by: Tiago Castro <tiagolobocastro@gmail.com>
Rather than looping all bdevs, we get the list of clones directly
from the snapshot using spdk_lvol_iter_immediate_clones.
Further we trim this down to "real_clones" by filtering out snapshots
from the returned list.

todo: can we optimize this process, we don't need 2 lists?

Signed-off-by: Tiago Castro <tiagolobocastro@gmail.com>
Renames is_snapshot_clone into clone_source, where the Lvol snapshot
is returned if found.
Added new api method is_clone which checks if the current Lvol is a
cloned lvol.

This both clarifies the previous method and also makes it more efficient
as we don't need to lookup the source snapshot all the times.

Signed-off-by: Tiago Castro <tiagolobocastro@gmail.com>
This makes snapshot searching quicker, as we don't need to load
the other attributes when it's a mismatch.

Signed-off-by: Tiago Castro <tiagolobocastro@gmail.com>
I'm not sure why this was being done as we can simply return the uuid
itself, rather than query only to return the same value...
It's possible that we wanted to return the uuid only if the parent
still existings, but it's not clear from the api and would be a strange
behaviour...

So we now drop this, and simply return the parent uuid.

Signed-off-by: Tiago Castro <tiagolobocastro@gmail.com>
Ensures the num_clones is set correctly
Spreads snapshots across a number of replicas
Creates a clone for each snapshot.

todo: investigate warn message:
Using passed in clear method 0x2 instead of stored value of 0x0

Signed-off-by: Tiago Castro <tiagolobocastro@gmail.com>
Signed-off-by: Tiago Castro <tiagolobocastro@gmail.com>
@tiagolobocastro
Copy link
Copy Markdown
Member Author

bors merge

bors-openebs-mayastor bot pushed a commit that referenced this pull request Apr 10, 2026
1963: Improve snapshot listing times r=tiagolobocastro a=tiagolobocastro

    test: add multi-replica and clone check to lvol_snap_list
    
    Ensures the num_clones is set correctly
    Spreads snapshots across a number of replicas
    Creates a clone for each snapshot.
    
    todo: investigate warn message:
    Using passed in clear method 0x2 instead of stored value of 0x0
    
---

    fix(lvs): don't query snapshot parent for listing
    
    I'm not sure why this was being done as we can simply return the uuid
    itself, rather than query only to return the same value...
    It's possible that we wanted to return the uuid only if the parent
    still existings, but it's not clear from the api and would be a strange
    behaviour...
    
    So we now drop this, and simply return the parent uuid.

---

    refactor(lvs): reorder xattr to list parent id as the first
    
    This makes snapshot searching quicker, as we don't need to load
    the other attributes when it's a mismatch.
    
---

    refactor(lvs): clarify is_snapshot_clone api
    
    Renames is_snapshot_clone into clone_source, where the Lvol snapshot
    is returned if found.
    Added new api method is_clone which checks if the current Lvol is a
    cloned lvol.
    
    This both clarifies the previous method and also makes it more efficient
    as we don't need to lookup the source snapshot all the times.
    
---

    refactor(lvs): replace list clones logic with spdk immediate clone
    
    Rather than looping all bdevs, we get the list of clones directly
    from the snapshot using spdk_lvol_iter_immediate_clones.
    Further we trim this down to "real_clones" by filtering out snapshots
    from the returned list.
    
    todo: can we optimize this process, we don't need 2 lists?
    
---

    refactor(lvs): compare snapshot uuid_str with potential clone parent
    
    When listing all potential bdevs to find matching clones, don't bother
    looking up the snapshot from each potential clone.
    Instead, simply compare the uuid with the snapshot.
    
---

    refactor(lvs): add lvol uuid lookup function
    
    Lookup lvol directly, rather than iterating all bdevs.

---

    refactor(lvs): count snapshot clones using spdk_blob_get_clones
    
    This api is much faster than iterating all bdevs, map them to lvols and then
    iterating again all bdevs to find the clones...
    
    This reduces the lvol_snap_list all the way to 50ms!

---

    refactor(lvs): simplify bdev to Lvol mappings
    
    Simplify all the iterations which try to load a bdev as an lvol.
    
    Also when comparing lvol's when listing clones, don't use the uuid
    as the comparisson method as it's an expensive operation requiring
    allocation of memory and string copies.
    Rather, we can simply compare the spdk_lvol pointer.
    
    This further reduces the slvol_snap_list test by another ~.5s
    
---

    refactor(lvs/xattr): change blob attr api to use compile constants
    
    Using a type which can yield &CStr avoids having to convert strings
    to CString at runtime.
    This reduces the snapshot list test by ~0.5s.
    
---

    refactor: compare bdev uuid bytes rather than string
    
    It's faster to compare the 16bytes rather than the stringified version.
    
---

    refactor: use new api for blob xattr reading
    
    We don't always need an owned string for the attribute values, as sometimes
    we only want to check/compare the value.
    I refactored the apis to returned a string with the lifetime of the lvol or
    the specified blob.
    The internal function is marked as unsafe and should not be directly used
    as the returned value is a pointer from the lvol, and must not be used
    beyong the blob's lifetime.
    
---

    test: add lvs snapshot list timing test
    
    Creates 3000 snapshots from a single replica and time out long it takes
    to list all snapshots.


Co-authored-by: Tiago Castro <tiagolobocastro@gmail.com>
@bors-openebs-mayastor
Copy link
Copy Markdown

Build failed:

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