add curvebs volume target blocksize #161
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I have to enter the container to debug the code |
|
It should be that you made an error executing the command in the container, and you can enter the container to execute the corresponding command. Or check whether the incoming command is wrong. The way to execute the command is: docker exec -t xxx xxx. |
|
issues #150 Signed-off-by: mfordjody mfordjody@gmail.com |
cw123
reviewed
Nov 22, 2022
internal/task/scripts/target.go
Outdated
| @@ -43,14 +45,13 @@ mkdir -p /curvebs/nebd/data/lock | |||
| touch /etc/curve/curvetab | |||
|
|
|||
| if [ $g_create == "true" ]; then | |||
| output=$(curve_ops_tool create -userName=$g_user -fileName=$g_volume -fileLength=$g_size) | |||
| output=$(curve_ops_tool create -userName=$g_user -fileName=$g_volume -fileLength=$g_size -fileLength=$g_blocksize) | |||
There was a problem hiding this comment.
-fileLength=$g_blocksize
这里是不是写错了？Is the logic wrong here?
two file length？
There was a problem hiding this comment.
If I output an error
curveadm target add curve:/test --host server-hosts --create --blocksize 4096byte
There is this -fileLength=$g_blocksize，Will print according to the size
$ cat ~/.curveadm/logs/curveadm-2022-11-21_17-45-44.log
remoteAddr: curve@192.168.10.136:22
command: sudo docker exec curvebs-target-daemon /bin/bash /curvebs/tools/sbin/target.sh curve /data true 10 4096
output:
error: Process exited with status 1
If don't have this -fileLength=$g_blocksize，
$ cat ~/.curveadm/logs/curveadm-2022-11-21_17-47-23.log
remoteAddr: curve@192.168.10.136:22
command: sudo docker exec curvebs-target-daemon /bin/bash /curvebs/tools/sbin/target.sh curve /data true 10 512
output:
error: Process exited with status 1
|
Merge multiple commits into one. |
|
blocksize 不是在curve_ops_tool create的时候指定的，curve_ops_tool没有指定blocksize的地方。 The blocksize is not specified when the curve_ops_tool create, and the curve_ops_tool does not have the blocksize param. blocksize是在这里指定的，在这个命令的后面添加一个--blocksize $g_blocksize blocksize is specified here, add a --blocksize $g_blocksize after this command 为什么要这样修改，因为curvebs在一些版本需要保证下发的请求是4KB对齐的请求，如果用户下发一个非4KB对齐的请求到curvebs，就会出错。通过nbd挂载的设备可以保证，nbd给到curvebs的请求是4KB对齐的，而现在使用target的方式挂载的设备没有对这个做一个保证。在target 添加lun的时候，指定blocksize，可以保证所有的请求都是指定blocksize下来的。 希望这个值默认是4KB。添加target的时候，如果不指定这个命令，就按照4096字节作为blocksize。如果指定blocksize，就按照指定的blocksize。 Why do we want to modify it like this, because in some versions of curvebs, you need to ensure that the issued request is a 4KB-aligned request. If the user sends a non-4KB-aligned request to curvebs, an error will occur. The device mounted through nbd can guarantee that the request from nbd to curvebs is 4KB aligned, but the device mounted using the target method does not make a guarantee for this. When adding a lun to the target, specify the blocksize to ensure that all requests come from the specified blocksize. Hopefully this value defaults to 4KB. When adding a target, if this command is not specified, 4096 bytes will be used as the blocksize. If blocksize is specified, follow the specified blocksize. 使用的地方参考文档 Reference documents |
Cyber-SiKu
reviewed
Nov 22, 2022
cli/command/client/map.go
Outdated
| @@ -106,11 +107,30 @@ func ParseSize(size string) (int, error) { | |||
| return n, nil | |||
| } | |||
|
|
|||
| func ParseBlockSize(blocksize string) (int, error) { | |||
There was a problem hiding this comment.
You can use the package go-humanize to parse.
|
如果未指定blocksize，fdisk -l 看到的可能是这样的 If blocksize is not specified, fdisk -l may see something like this
target 添加lun的时候指定blocksize，看到的是这样的 Target specifies the blocksize when adding lun, what you see is this
|
Wine93
reviewed
Nov 22, 2022
cli/command/target/add.go
Outdated
| flags.StringVar(&options.size, "size", "10GB", "Specify volume size") | ||
| flags.StringVarP(&options.filename, "conf", "c", "client.yaml", "Specify client configuration file") | ||
|
|
||
| flags.StringVar(&options.blocksize, "blocksize", "512byte", "Specify volume block size") |
There was a problem hiding this comment.
The default unit of blocksize should be Byte, and if we want to provide more units for convenience, we can use KB、MB and etc, like:
--blocksize 512 # Byte, also default unit
--blocksize 1KB # KB
--blocksize 1MB # MB
|
BTW: there are many commits in this PR, maybe we can rebase them into one :) |
|
Cyber-SiKu
requested changes
Nov 24, 2022
Comment on lines
+112
to
+114
| if !strings.HasSuffix(blocksize, "B") { | ||
| return 0, errno.ERR_VOLUME_BLOCKSIZE_MUST_END_WITH_BYTE_SUFFIX. | ||
| F("blocksize: %s", blocksize) | ||
| } |
There was a problem hiding this comment.
It is not needed here.
|
Cyber-SiKu
requested changes
Nov 24, 2022
| @@ -106,11 +108,25 @@ func ParseSize(size string) (int, error) { | |||
| return n, nil | |||
| } | |||
|
|
|||
| func ParseBlockSize(blocksize string) (uint64, error) { | |||
| blocksize = strings.TrimSuffix(blocksize, "B") | |||
There was a problem hiding this comment.
No need to remove B.
// You can edit this code!
// Click here and start typing.
package main
import (
"fmt"
"github.com/dustin/go-humanize"
)
func main() {
blocksize := "1024B"
str, _ := humanize.ParseBytes(blocksize)
fmt.Println(str)
}
the output is:
1024
cli/command/client/map.go
Outdated
| m, err := humanize.ParseBytes(blocksize) | ||
| if err != nil || m <= 0 { | ||
| return 0, errno.ERR_VOLUME_BLOCKSIZE_REQUIRES_POSITIVE_INTEGER. | ||
| F("blocksize: %s", humanize.Bytes(m)) |
There was a problem hiding this comment.
Learn the difference between Bytes and IBytes:Multiple-byte units
cli/command/client/map.go
Outdated
| F("blocksize: %s", humanize.Bytes(m)) | ||
| } else if m%512 != 0 { | ||
| return 0, errno.ERR_VOLUME_BLOCKSIZE_BE_MULTIPLE_OF_512. | ||
| F("blocksize: %s", humanize.Bytes(m)) |
| @@ -106,11 +108,25 @@ func ParseSize(size string) (int, error) { | |||
| return n, nil | |||
| } | |||
|
|
|||
| func ParseBlockSize(blocksize string) (uint64, error) { | |||
| blocksize = strings.TrimSuffix(blocksize, "B") | |||
| m, err := humanize.ParseBytes(blocksize) | |||
There was a problem hiding this comment.
You can look at its source code
cli/command/client/map.go
Outdated
| @@ -142,7 +158,7 @@ func NewMapCommand(curveadm *cli.CurveAdm) *cobra.Command { | |||
| flags.BoolVar(&options.noExclusive, "no-exclusive", false, "Map volume non exclusive") | |||
| flags.StringVar(&options.size, "size", "10GB", "Specify volume size") | |||
cli/command/target/add.go
Outdated
| @@ -84,10 +87,10 @@ func NewAddCommand(curveadm *cli.CurveAdm) *cobra.Command { | |||
|
|
|||
| flags := cmd.Flags() | |||
| flags.StringVar(&options.host, "host", "localhost", "Specify target host") | |||
| flags.BoolVar(&options.create, "create", false, "Create volume iff not exist") | |||
| flags.BoolVar(&options.create, "create", false, "Create volume if not exist") | |||
| flags.StringVar(&options.size, "size", "10GB", "Specify volume size") | |||
internal/errno/errno.go
Outdated
| ERR_VOLUME_NAME_MUST_START_WITH_SLASH_PREFIX = EC(221002, "volume name must start with \"/\" prefix") | ||
| ERR_VOLUME_SIZE_MUST_END_WITH_GB_SUFFIX = EC(221003, "volume size must end with \"GB\" suffix") | ||
| ERR_VOLUME_SIZE_REQUIRES_POSITIVE_INTEGER = EC(221004, "volume size requires a positive integer") | ||
| ERR_VOLUME_SIZE_MUST_BE_MULTIPLE_OF_10_GB = EC(221005, "volume size must be a multiple of 10GB, like 10GB, 20GB, 30GB...") |
internal/errno/errno.go
Outdated
| ERR_VOLUME_NAME_CAN_NOT_CONTAIN_UNDERSCORE = EC(221008, "volume name can't contain \"_\" symbol") | ||
| ERR_VOLUME_BLOCKSIZE_MUST_END_WITH_BYTE_SUFFIX = EC(201009, "volume block size must end with \"B\" suffix") | ||
| ERR_VOLUME_BLOCKSIZE_REQUIRES_POSITIVE_INTEGER = EC(221010, "volume block size requires a positive integer") | ||
| ERR_VOLUME_BLOCKSIZE_BE_MULTIPLE_OF_512 = EC(221011, "volume block size be a multiple of 512B,like 1.024kB, 2.048kB,3.072kB...") |
|
|
Cyber-SiKu
reviewed
Nov 25, 2022
|
done |
cw123
reviewed
Nov 28, 2022
cli/command/client/map.go
Outdated
| flags.StringVarP(&options.filename, "conf", "c", "client.yaml", "Specify client configuration file") | ||
|
|
||
| flags.StringVar(&options.blocksize, "blocksize", "512B", "Specify volume blocksize") |
|
看起来有两个地方都增加了blocksize的改动，一个map的时候，一个是add target的时候。 map的地方，map命令使用curve-nbd map，这个接收的参数不包含blocksize，而且这个命令没有测试结果。 另一个是add target的地方，看起主要逻辑是正确的，遗漏了一点，blocksize默认应该是4096B。 请确认一下，map.go中的逻辑是否正确。 It seems that there are two places where the changes of blocksize have been added, one for map and one for add target. For map, the map command uses curve-nbd map, the received parameter does not include blocksize, and this command has no test results. The other is add target. It seems that the main logic is correct. One thing is missing. The default blocksize should be 4096B. Please confirm whether the logic in map.go is correct. |
|
Blocksize not specified |
|
Blocksize specified |
|
Signed-off-by: mfordjody <11638005@qq.com> fix int Signed-off-by: mfordjody <11638005@qq.com> fix map Signed-off-by: mfordjody <11638005@qq.com> add target.sh Signed-off-by: mfordjody <11638005@qq.com> Update add.go Signed-off-by: mfordjody <mfordjody@gmail.com> Update add.go Signed-off-by: mfordjody <mfordjody@gmail.com> Update add.go Signed-off-by: mfordjody <mfordjody@gmail.com> Update add_target.go Signed-off-by: mfordjody <mfordjody@gmail.com> Update errno.go Signed-off-by: mfordjody <mfordjody@gmail.com> Update add.go Signed-off-by: mfordjody <mfordjody@gmail.com> Update add.go Signed-off-by: mfordjody <mfordjody@gmail.com> fix target Signed-off-by: mfordjody <mfordjody@gmail.com> Update target.go Signed-off-by: mfordjody <mfordjody@gmail.com> Update target.go Signed-off-by: mfordjody <mfordjody@gmail.com> Update target.go Signed-off-by: mfordjody <mfordjody@gmail.com> Update target.go Signed-off-by: mfordjody <mfordjody@gmail.com> Update add.go Signed-off-by: mfordjody <mfordjody@gmail.com> Update errno.go Signed-off-by: mfordjody <mfordjody@gmail.com> Update add_target.go Signed-off-by: mfordjody <mfordjody@gmail.com> Update map.go Signed-off-by: mfordjody <mfordjody@gmail.com> Update map.go Signed-off-by: mfordjody <mfordjody@gmail.com> Update map.go Signed-off-by: mfordjody <mfordjody@gmail.com> Update add.go Signed-off-by: mfordjody <mfordjody@gmail.com> Update errno.go Signed-off-by: mfordjody <mfordjody@gmail.com> Update map.go Signed-off-by: mfordjody <mfordjody@gmail.com> Update add.go Signed-off-by: mfordjody <mfordjody@gmail.com> delete map blocksize Signed-off-by: mfordjody <11638005@qq.com>
cw123
approved these changes
Dec 1, 2022
Cyber-SiKu
approved these changes
Dec 1, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
issues #150
Signed-off-by: mfordjody mfordjody@gmail.com
I won't solve the container error