Skip to content

Commit

Permalink
blockdev: query physical sector size, not logical
Browse files Browse the repository at this point in the history
When determining whether to install the 512b or 4k image, we should be
comparing against the physical sector size of the drive, not its logical
sector size. Some 4k drives report themselves as having a logical sector
size of 512 bytes for backwards compatibility with OSes and software
that can't do 4k blocks (these are also called "512e" drives). However,
actually using the drive as a 512b one provides reduced performance
because the drive needs to translate all requests.

For comparison, `mkfs.xfs` will use a sector size of 4096 on 512e
drives:

https://git.kernel.org/pub/scm/fs/xfs/xfsprogs-dev.git/tree/mkfs/xfs_mkfs.c?id=3498b68025ce#n1932

One can also experimentally verify this:

```
$ qemu-img create -f qcow2 storage-512.qcow2 1G
$ qemu-img create -f qcow2 storage-512e.qcow2 1G
$ qemu-img create -f qcow2 storage-4kn.qcow2 1G
$ cosa run -- \
    -drive if=none,id=storage-512,format=qcow2,file=storage-512.qcow2 \
    -device virtio-blk,drive=storage-512,physical_block_size=512,logical_block_size=512,serial=storage-512 \
    -drive if=none,id=storage-512e,format=qcow2,file=storage-512e.qcow2 \
    -device virtio-blk,drive=storage-512e,physical_block_size=4096,logical_block_size=512,serial=storage-512e \
    -drive if=none,id=storage-4kn,format=qcow2,file=storage-4kn.qcow2 \
    -device virtio-blk,drive=storage-4kn,physical_block_size=4096,logical_block_size=4096,serial=storage-4kn
[core@cosa-devsh ~]$ sudo mkfs.xfs /dev/disk/by-id/virtio-storage-512 | grep sectsz
         =                       sectsz=512   attr=2, projid32bit=1
         =                       sectsz=512   sunit=0 blks, lazy-count=1
[core@cosa-devsh ~]$ sudo mkfs.xfs /dev/disk/by-id/virtio-storage-512e | grep sectsz
         =                       sectsz=4096  attr=2, projid32bit=1
         =                       sectsz=4096  sunit=1 blks, lazy-count=1
[core@cosa-devsh ~]$ sudo mkfs.xfs /dev/disk/by-id/virtio-storage-4kn | grep sectsz
         =                       sectsz=4096  attr=2, projid32bit=1
         =                       sectsz=4096  sunit=1 blks, lazy-count=1
```

To match this, we only need to change `get_sector_size_for_path` to use
`BLKPBSZGET` instead of `BLKSSZGET`.
  • Loading branch information
jlebon committed Nov 26, 2022
1 parent 0b11ae0 commit b889c59
Show file tree
Hide file tree
Showing 2 changed files with 3 additions and 2 deletions.
1 change: 1 addition & 0 deletions docs/release-notes.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ Major changes:
Minor changes:

- install: Avoid osmet performance regression in debug builds on Rust 1.64+
- install: Install 4Kn image on 512e disks

Internal changes:

Expand Down
4 changes: 2 additions & 2 deletions src/blockdev.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1085,7 +1085,7 @@ pub fn get_sector_size_for_path(device: &Path) -> Result<NonZeroU32> {
pub fn get_sector_size(file: &File) -> Result<NonZeroU32> {
let fd = file.as_raw_fd();
let mut size: c_int = 0;
match unsafe { ioctl::blksszget(fd, &mut size) } {
match unsafe { ioctl::blkpbszget(fd, &mut size) } {
Ok(_) => {
let size_u32: u32 = size
.try_into()
Expand Down Expand Up @@ -1194,7 +1194,7 @@ mod ioctl {
use super::c_int;
use nix::{ioctl_none, ioctl_read, ioctl_read_bad, request_code_none};
ioctl_none!(blkrrpart, 0x12, 95);
ioctl_read_bad!(blksszget, request_code_none!(0x12, 104), c_int);
ioctl_read_bad!(blkpbszget, request_code_none!(0x12, 123), c_int);
ioctl_read!(blkgetsize64, 0x12, 114, libc::size_t);
}

Expand Down

0 comments on commit b889c59

Please sign in to comment.