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.

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

tcmur: fix check_iovec_length #481

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

tcmur: fix check_iovec_length #481

wants to merge 1 commit into from

Conversation

lxbsz
Copy link
Collaborator

@lxbsz lxbsz commented Sep 29, 2018

readsector0 checker for multipath is not working, and the log like:

[DEBUG_SCSI_CMD] tcmu_print_cdb_info:1069 glfs/block0: 28 0 0 0 0 0 0 0 1 0
[ERROR] check_lba_and_length:107: iov len mismatch: iov len 4096, xfer len 1, block size 512
check_lba_and_length:107: iov len mismatch: iov len 4096, xfer len 1, block size 512

This is because in kernel space the sg->length is aligned to the
page size, and also the ringbufer data area's block size. So here
we need to make sure that the iov len is not less than the scsi
command's require.

Signed-off-by: Xiubo Li xiubli@redhat.com

readsector0 checker for multipath is not working, and the log like:

[DEBUG_SCSI_CMD] tcmu_print_cdb_info:1069 glfs/block0: 28 0 0 0 0 0 0 0 1 0
[ERROR] check_lba_and_length:107: iov len mismatch: iov len 4096, xfer len 1, block size 512
check_lba_and_length:107: iov len mismatch: iov len 4096, xfer len 1, block size 512

This is because in kernel space the sg->length is aligned to the
page size, and also the ringbufer data area's block size. So here
we need to make sure that the iov len is not less than the scsi
command's require.

Signed-off-by: Xiubo Li <xiubli@redhat.com>
@lxbsz
Copy link
Collaborator Author

lxbsz commented Sep 29, 2018

@bgly Thanks.

@mikechristie
Copy link
Collaborator

If the value returned by tcmu_iovec_length does not match the length of the command we have issues with all the rw paths, because are using the iovec length to perform IOs. I think with your patch in the write path for example, we will end up writing 4096 bytes instead of only 512 which is going to cause corruption.

@mikechristie
Copy link
Collaborator

Oh yeah here is GH issue you might want to take into consideration when fixing this issue:
#476
Basically, you can end up with weird iov splits.

@bgly
Copy link
Contributor

bgly commented Oct 2, 2018

Each handler would probably need to make sure they do not over-write past what they are supposed to? The function @lxbsz is modifying only checks validity and a command is still valid if iov_length < sectors * tcmu_get_dev_block_size(dev) ?

@lxbsz
Copy link
Collaborator Author

lxbsz commented Oct 2, 2018

Each handler would probably need to make sure they do not over-write past what they are supposed to? The function @lxbsz is modifying only checks validity and a command is still valid if iov_length < sectors * tcmu_get_dev_block_size(dev) ?

Yeah, in kernel space the iov_length is aligned to 4096(if page size is 4k), so the iov_length will always larger or equal to sectors * tcmu_get_dev_block_size(dev).

If the value returned by tcmu_iovec_length does not match the length of the command we have issues with all the rw paths, because are using the iovec length to perform IOs. I think with your patch in the write path for example, we will end up writing 4096 bytes instead of only 512 which is going to cause corruption.

This change won't avoid the corruption then.
Currently, we are using "tcmu_iovec_length(cmd->iovec, cmd->iov_cnt)", so we should use the actual xfer length "sectors * tcmu_get_dev_block_size(dev)" instead, will it be right ?

 160 static int write_work_fn(struct tcmu_device *dev,
 161                                 struct tcmulib_cmd *cmd)            
 162 { 
 163         struct tcmur_handler *rhandler = tcmu_get_runner_handler(dev);
 164         uint32_t block_size = tcmu_get_dev_block_size(dev);
 165   
 166         return rhandler->write(dev, cmd, cmd->iovec, cmd->iov_cnt,
 167                                 tcmu_iovec_length(cmd->iovec, cmd->iov_cnt),                                                                                            
 168                                 block_size * tcmu_get_lba(cmd->cdb));
 169 } 

@mikechristie @bgly Thanks.

@mikechristie
Copy link
Collaborator

Currently, we are using "tcmu_iovec_length(cmd->iovec, cmd->iov_cnt)", so we should use the actual xfer length "sectors * tcmu_get_dev_block_size(dev)" instead, will it be right ?

I am not 100% sure. Yes that will fix the issue, but I think iov_len will be buggy where sometimes it describes the size of the buffer pointed to by iov_base and sometimes it is the number of bytes that are in the buffer at iov_base.

I am still looking through the kernel and user space code. I think that the original code meant for iov_len to be the number of bytes in the buffer at iov_base that were supposed to be transferred and somewhere a long the way we messed that up. If that is the case we should fix the kernel because that bug will cause issues with other user space apps.

@mikechristie
Copy link
Collaborator

@lxbsz

What target driver did you hit this with? Did you hit it with loop?

Yeah, in kernel space the iov_length is aligned to 4096(if page size is 4k), so the iov_length will always larger or equal to sectors * tcmu_get_dev_block_size(dev).

For drivers that go through target_alloc_sgl won't the sg->length be 512 bytes for a 512 byte RW?

cmd->data_length =512 so sgl_alloc_order gets passed 512. It then does min(512, PAGE_SIZE << order) which would be 512 for elem_len which gets set to sg->length in sg_set_page.

So later when target_core_user does a min(sg_remaining, block_remaining) we should get iov_len = 512.

@lxbsz
Copy link
Collaborator Author

lxbsz commented Oct 4, 2018

@lxbsz

What target driver did you hit this with? Did you hit it with loop?

The LIO/TCMU for the gluster-block.

[...]

For drivers that go through target_alloc_sgl won't the sg->length be 512 bytes for a 512 byte RW?

cmd->data_length =512 so sgl_alloc_order gets passed 512. It then does min(512, PAGE_SIZE << order) which would be 512 for elem_len which gets set to sg->length in sg_set_page.

So later when target_core_user does a min(sg_remaining, block_remaining) we should get iov_len = 512.

This should be what we are expecting. Currently, it seems the sg->length is the page size. I need to check the LIO core code about this.

@mikechristie
Copy link
Collaborator

What target driver did you hit this with? Did you hit it with loop?
The LIO/TCMU for the gluster-block.

Does gluster-block always use iscsi? I am just asking because some fabric drivers like loop allocates scatter gather entries differently than iscsi because the sg list comes from the scsi/block initiator layer.

This should be what we are expecting. Currently, it seems the sg->length is the page size. I need to check the LIO core code about this.

What kernel is this with? I am actually seeing the iov_len at 512 for 512 byte RWs.

@lxbsz
Copy link
Collaborator Author

lxbsz commented Oct 8, 2018

What target driver did you hit this with? Did you hit it with loop?
The LIO/TCMU for the gluster-block.

Does gluster-block always use iscsi? I am just asking because some fabric drivers like loop allocates scatter gather entries differently than iscsi because the sg list comes from the scsi/block initiator layer.

Sorry for late. Yes currently it is, only iscsi is support.

This should be what we are expecting. Currently, it seems the sg->length is the page size. I need to check the LIO core code about this.

What kernel is this with? I am actually seeing the iov_len at 512 for 512 byte RWs.

The kernel version is 3.10.0-862.14.2.el7.x86_64.debug.
I have tried the newest upstream version, yeah you're right, it's already 512 now.

@lxbsz lxbsz changed the base branch from master to main August 10, 2022 00:23
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.

3 participants