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

spa_min_alloc should be GCD, not min #15067

Merged
merged 1 commit into from
Jul 20, 2023
Merged

Conversation

ixhamza
Copy link
Contributor

@ixhamza ixhamza commented Jul 13, 2023

Motivation and Context

Since spa_min_alloc may not be a power of 2, unlike ashifts, in the case of DRAID, we should not select the minimal value among several vdevs. Rounding to a multiple of it is unlikely to work for other vdevs. Instead, using the greatest common divisor produces smaller yet more reasonable results.

How Has This Been Tested?

# Create disks
truncate -s 256m /var/tmp/disk1
truncate -s 256m /var/tmp/disk2
truncate -s 256m /var/tmp/disk3
truncate -s 256m /var/tmp/disk4
truncate -s 256m /var/tmp/disk5

# Create a draid pool with ashift=9
echo 9 > /sys/module/zfs/parameters/vdev_file_logical_ashift
echo 9 > /sys/module/zfs/parameters/vdev_file_physical_ashift
zpool create tank draid1 /var/tmp/disk1 /var/tmp/disk2 /var/tmp/disk3 /var/tmp/disk4 -f

# spa_min_alloc will be 1536 [(vdc->vdc_ndata << vd->vdev_ashift) =>
# (3 << 512) => 1536]

# Add another top-level vdev with ashift=10, which produces min_alloc
# 1024 [(1 << ashift) => (1 << 10) => 1024] 
echo 10 > /sys/module/zfs/parameters/vdev_file_logical_ashift
echo 10 > /sys/module/zfs/parameters/vdev_file_physical_ashift
zpool add tank /var/tmp/disk5 -f

# New spa_min_alloc will now be 512 [gcd(1536, 1024) => 512] instead
# of 1024 [min(1536, 1024)] 

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Performance enhancement (non-breaking change which improves efficiency)
  • Code cleanup (non-breaking change which makes code smaller or more readable)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Library ABI change (libzfs, libzfs_core, libnvpair, libuutil and libzfsbootenv)
  • Documentation (a change to man pages or other documentation)

Checklist:

@ixhamza
Copy link
Contributor Author

ixhamza commented Jul 13, 2023

cc: @amotin

@amotin amotin requested a review from behlendorf July 13, 2023 18:26
@amotin amotin added the Status: Code Review Needed Ready for review and testing label Jul 13, 2023
@amotin
Copy link
Member

amotin commented Jul 13, 2023

For a note, in #14909 I am rounding ZIL block allocation size to spa_min_alloc, and previous min value could result in allocation bigger than other vdev where allocation finally ended up can efficiently handle.

@amotin
Copy link
Member

amotin commented Jul 13, 2023

Thinking more about it, I think we should both keep old min value and just add new gcd. If allocation size is smaller or equal than spa_min_alloc, then it should be set to spa_min_alloc and suffer whatever inefficiency happen on bigger vdev. If allocation is bigger than spa_min_alloc, then we should use roundup(s, spa_gcd_alloc). We should probably add a function/macro implementing this logic and use it in two places where spa_min_alloc is used now.

@ixhamza
Copy link
Contributor Author

ixhamza commented Jul 13, 2023

Thank you @amotin for the feedback. Updated.

module/zfs/vdev.c Outdated Show resolved Hide resolved
module/zfs/zio.c Outdated Show resolved Hide resolved
@ixhamza
Copy link
Contributor Author

ixhamza commented Jul 13, 2023

Thanks, @amotin. I will address your feedback.

module/zfs/zio.c Outdated Show resolved Hide resolved
module/zfs/zio.c Outdated Show resolved Hide resolved
module/zfs/zio.c Outdated Show resolved Hide resolved
Since spa_min_alloc may not be a power of 2, unlike ashifts, in the
case of DRAID, we should not select the minimal value among several
vdevs. Rounding to a multiple of it is unlikely to work for other
vdevs. Instead, using the greatest common divisor produces smaller
yet more reasonable results.

Signed-off-by: Ameer Hamza <[email protected]>
@behlendorf behlendorf merged commit d9bb583 into openzfs:master Jul 20, 2023
@behlendorf behlendorf added Status: Accepted Ready to integrate (reviewed, tested) and removed Status: Code Review Needed Ready for review and testing labels Jul 20, 2023
behlendorf pushed a commit to behlendorf/zfs that referenced this pull request Jul 21, 2023
Since spa_min_alloc may not be a power of 2, unlike ashifts, in the
case of DRAID, we should not select the minimal value among several
vdevs. Rounding to a multiple of it is unlikely to work for other
vdevs. Instead, using the greatest common divisor produces smaller
yet more reasonable results.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ameer Hamza <[email protected]>
Closes openzfs#15067
behlendorf pushed a commit that referenced this pull request Jul 21, 2023
Since spa_min_alloc may not be a power of 2, unlike ashifts, in the
case of DRAID, we should not select the minimal value among several
vdevs. Rounding to a multiple of it is unlikely to work for other
vdevs. Instead, using the greatest common divisor produces smaller
yet more reasonable results.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ameer Hamza <[email protected]>
Closes #15067
ixhamza added a commit to truenas/zfs that referenced this pull request Jul 24, 2023
Since spa_min_alloc may not be a power of 2, unlike ashifts, in the
case of DRAID, we should not select the minimal value among several
vdevs. Rounding to a multiple of it is unlikely to work for other
vdevs. Instead, using the greatest common divisor produces smaller
yet more reasonable results.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ameer Hamza <[email protected]>
Closes openzfs#15067
@ixhamza ixhamza deleted the NAS-122880 branch August 2, 2023 17:35
lundman pushed a commit to openzfsonwindows/openzfs that referenced this pull request Dec 12, 2023
Since spa_min_alloc may not be a power of 2, unlike ashifts, in the
case of DRAID, we should not select the minimal value among several
vdevs. Rounding to a multiple of it is unlikely to work for other
vdevs. Instead, using the greatest common divisor produces smaller
yet more reasonable results.

Reviewed-by: Brian Behlendorf <[email protected]>
Reviewed-by: Alexander Motin <[email protected]>
Signed-off-by: Ameer Hamza <[email protected]>
Closes openzfs#15067
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Accepted Ready to integrate (reviewed, tested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants