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

nvme/opts: parse timeouts in humantime #1548

Merged
merged 8 commits into from
Nov 24, 2023
Merged

nvme/opts: parse timeouts in humantime #1548

merged 8 commits into from
Nov 24, 2023

Conversation

tiagolobocastro
Copy link
Contributor

@tiagolobocastro tiagolobocastro commented Nov 23, 2023 •
edited
Loading 

ci(bors): add fake DCO for bors merge requests

DCO is already validated on PR and bors itself does not need it as it's a bot..

Signed-off-by: Tiago Castro <[email protected]>

fix(nvme/opts): remove outdated async parameter

Remove unused NVME_QPAIR_CONNECT_ASYNC opts parameter and add
generated uuid parameter.

Signed-off-by: Tiago Castro <[email protected]>

fix(nvme/kato): adjust to spdk minimum

SPDK minimum KATO is 10s, so setting 1s is pointless.
Also the spec does suggest setting a sufficiently high KATO on a fabric, so perhaps
we might want to consider increasing it in the future.

Signed-off-by: Tiago Castro <[email protected]>

fix(iobuf): increase large iobuf pool size

Addresses warning from spdk itself:
mayastor::spdk:transport.c:282]
The num_shared_buffers value (2048) is larger than the available iobuf pool size (1024).
 Please increase the iobuf pool sizes.

Also reduce size to power of 2 - 1 as recommended in spdk ticket:
It’s always best to specify a value for –n that is one less than a power of two (2^x -1)
because of the way rings work in DPDK. You end up allocating a ring twice the size for
the one extra element between 511 and 512 for example.

Signed-off-by: Tiago Castro <[email protected]>

chore: silence flush not supported IO

We don't keep any caching, so we don't actually need to make use of it anyway.
TODO: do we need to even send flush?

Signed-off-by: Tiago Castro <[email protected]>

chore: show starting configuration as info

Signed-off-by: Tiago Castro <[email protected]>

feat(nvme/opts): parse timeouts in humantime

This allows us to specify, for example, NVME_TIMEOUT=60s

Signed-off-by: Tiago Castro <[email protected]>

@auto-assign auto-assign bot requested a review from chriswldenyer November 23, 2023 11:12
Abhinandan-Purkait
Abhinandan-Purkait approved these changes Nov 23, 2023
View reviewed changes
Copy link
Member

@Abhinandan-Purkait Abhinandan-Purkait left a comment

Choose a reason for hiding this comment

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

LGTM

io-engine/src/subsys/config/mod.rs Show resolved Hide resolved
@tiagolobocastro
feat(nvme/opts): parse timeouts in humantime
93be755 
This allows us to specify, for example, NVME_TIMEOUT=60s

Signed-off-by: Tiago Castro <[email protected]>
@tiagolobocastro
chore: show starting configuration as info
8afc6cf 
Signed-off-by: Tiago Castro <[email protected]>
@tiagolobocastro
chore: silence flush not supported IO
d7d71f9 
We don't keep any caching, so we don't actually need to make use of it anyway.
TODO: do we need to even send flush?

Signed-off-by: Tiago Castro <[email protected]>
@tiagolobocastro
fix(iobuf): fixup large iobuf pool size
5bf8f36 
Addresses warning from spdk itself:
mayastor::spdk:transport.c:282]
The num_shared_buffers value (2048) is larger than the available iobuf pool size (1024).
 Please increase the iobuf pool sizes.

Also reduce size to power of 2 - 1 as recommended in spdk ticket:
It’s always best to specify a value for –n that is one less than a power of two (2^x -1)
because of the way rings work in DPDK. You end up allocating a ring twice the size for
the one extra element between 511 and 512 for example.

Signed-off-by: Tiago Castro <[email protected]>
@tiagolobocastro
fix(nvme/kato): adjust to spdk minimum
42c6bc8 
SPDK minimum KATO is 10s, so setting 1s is pointless.
Also the spec does suggest setting a sufficiently high KATO on a fabric, so perhaps
we might want to consider increasing it in the future.

Signed-off-by: Tiago Castro <[email protected]>
@tiagolobocastro
fix(io-engine/client): jsonrpc does not use subcommand
755cfb4 
Signed-off-by: Tiago Castro <[email protected]>
@tiagolobocastro
Copy link
Contributor Author

bors merge

bors-openebs-mayastor bot pushed a commit that referenced this pull request Nov 24, 2023
@tiagolobocastro
Merge #1548
ac84f2e 
1548: nvme/opts: parse timeouts in humantime r=tiagolobocastro a=tiagolobocastro

    fix(nvme/kato): adjust to spdk minimum
    
    SPDK minimum KATO is 10s, so setting 1s is pointless.
    Also the spec does suggest setting a sufficiently high KATO on a fabric, so perhaps
    we might want to consider increasing it in the future.
    
    Signed-off-by: Tiago Castro <[email protected]>

---

    fix(iobuf): increase large iobuf pool size
    
    Addresses warning from spdk itself:
    mayastor::spdk:transport.c:282]
    The num_shared_buffers value (2048) is larger than the available iobuf pool size (1024).
     Please increase the iobuf pool sizes.

    Also reduce size to power of 2 - 1 as recommended in spdk ticket:
    It’s always best to specify a value for –n that is one less than a power of two (2^x -1)
    because of the way rings work in DPDK. You end up allocating a ring twice the size for
    the one extra element between 511 and 512 for example.
    
    Signed-off-by: Tiago Castro <[email protected]>

---

    chore: silence flush not supported IO
    
    We don't keep any caching, so we don't actually need to make use of it anyway.
    TODO: do we need to even send flush?
    
    Signed-off-by: Tiago Castro <[email protected]>

---

    chore: show starting configuration as info
    
    Signed-off-by: Tiago Castro <[email protected]>

---

    feat(nvme/opts): parse timeouts in humantime
    
    This allows us to specify, for example, NVME_TIMEOUT=60s
    
    Signed-off-by: Tiago Castro <[email protected]>


Co-authored-by: Tiago Castro <[email protected]>
@tiagolobocastro
Copy link
Contributor Author

bors cancel

@bors-openebs-mayastor
Copy link

Canceled.

@tiagolobocastro
fix(nvme/opts): remove outdated async parameter
67dd040 
Remove unused NVME_QPAIR_CONNECT_ASYNC opts parameter and add
generated uuid parameter.

Signed-off-by: Tiago Castro <[email protected]>
@tiagolobocastro
Copy link
Contributor Author

bors merge

bors-openebs-mayastor bot pushed a commit that referenced this pull request Nov 24, 2023
@tiagolobocastro
Merge #1548
48d305a 
1548: nvme/opts: parse timeouts in humantime r=tiagolobocastro a=tiagolobocastro

    fix(nvme/kato): adjust to spdk minimum
    
    SPDK minimum KATO is 10s, so setting 1s is pointless.
    Also the spec does suggest setting a sufficiently high KATO on a fabric, so perhaps
    we might want to consider increasing it in the future.
    
    Signed-off-by: Tiago Castro <[email protected]>

---

    fix(iobuf): increase large iobuf pool size
    
    Addresses warning from spdk itself:
    mayastor::spdk:transport.c:282]
    The num_shared_buffers value (2048) is larger than the available iobuf pool size (1024).
     Please increase the iobuf pool sizes.

    Also reduce size to power of 2 - 1 as recommended in spdk ticket:
    It’s always best to specify a value for –n that is one less than a power of two (2^x -1)
    because of the way rings work in DPDK. You end up allocating a ring twice the size for
    the one extra element between 511 and 512 for example.
    
    Signed-off-by: Tiago Castro <[email protected]>

---

    chore: silence flush not supported IO
    
    We don't keep any caching, so we don't actually need to make use of it anyway.
    TODO: do we need to even send flush?
    
    Signed-off-by: Tiago Castro <[email protected]>

---

    chore: show starting configuration as info
    
    Signed-off-by: Tiago Castro <[email protected]>

---

    feat(nvme/opts): parse timeouts in humantime
    
    This allows us to specify, for example, NVME_TIMEOUT=60s
    
    Signed-off-by: Tiago Castro <[email protected]>


Co-authored-by: Tiago Castro <[email protected]>
@tiagolobocastro
ci(bors): add fake DCO for bors merge requests
5223f3e 
DCO is already validated on PR and bors itself does not need it as it's a bot..

Signed-off-by: Tiago Castro <[email protected]>
@tiagolobocastro
Copy link
Contributor Author

bors merge

bors-openebs-mayastor bot pushed a commit that referenced this pull request Nov 24, 2023
@tiagolobocastro
Merge #1548
7bd1d29 
1548: nvme/opts: parse timeouts in humantime r=tiagolobocastro a=tiagolobocastro

    ci(bors): add fake DCO for bors merge requests
    
    DCO is already validated on PR and bors itself does not need it as it's a bot..
    
    Signed-off-by: Tiago Castro <[email protected]>

---

    fix(nvme/opts): remove outdated async parameter
    
    Remove unused NVME_QPAIR_CONNECT_ASYNC opts parameter and add
    generated uuid parameter.
    
    Signed-off-by: Tiago Castro <[email protected]>

---

    fix(nvme/kato): adjust to spdk minimum
    
    SPDK minimum KATO is 10s, so setting 1s is pointless.
    Also the spec does suggest setting a sufficiently high KATO on a fabric, so perhaps
    we might want to consider increasing it in the future.
    
    Signed-off-by: Tiago Castro <[email protected]>

---

    fix(iobuf): increase large iobuf pool size
    
    Addresses warning from spdk itself:
    mayastor::spdk:transport.c:282]
    The num_shared_buffers value (2048) is larger than the available iobuf pool size (1024).
     Please increase the iobuf pool sizes.

    Also reduce size to power of 2 - 1 as recommended in spdk ticket:
    It’s always best to specify a value for –n that is one less than a power of two (2^x -1)
    because of the way rings work in DPDK. You end up allocating a ring twice the size for
    the one extra element between 511 and 512 for example.
    
    Signed-off-by: Tiago Castro <[email protected]>

---

    chore: silence flush not supported IO
    
    We don't keep any caching, so we don't actually need to make use of it anyway.
    TODO: do we need to even send flush?
    
    Signed-off-by: Tiago Castro <[email protected]>

---

    chore: show starting configuration as info
    
    Signed-off-by: Tiago Castro <[email protected]>

---

    feat(nvme/opts): parse timeouts in humantime
    
    This allows us to specify, for example, NVME_TIMEOUT=60s
    
    Signed-off-by: Tiago Castro <[email protected]>


Co-authored-by: Tiago Castro <[email protected]>
@tiagolobocastro
Copy link
Contributor Author

Had to enable DCO from any source...
bors merge

@bors-openebs-mayastor
Copy link

Build succeeded:

@bors-openebs-mayastor bors-openebs-mayastor bot merged commit 8212046 into develop Nov 24, 2023
6 checks passed
@bors-openebs-mayastor bors-openebs-mayastor bot deleted the humantime branch November 24, 2023 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Abhinandan-Purkait Abhinandan-Purkait Abhinandan-Purkait approved these changes

@dsharma-dc dsharma-dc dsharma-dc approved these changes

@dsavitskiy dsavitskiy Awaiting requested review from dsavitskiy

@hrudaya21 hrudaya21 Awaiting requested review from hrudaya21

@chriswldenyer chriswldenyer Awaiting requested review from chriswldenyer

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@tiagolobocastro @Abhinandan-Purkait @dsharma-dc