[parted-devel] [PATCH 6/6] libparted: fix optimal IO alignment

[Brian C. Lane]

On Mon, Oct 15, 2012 at 12:00:03AM -0400, Phillip Susi wrote:
> There were several apparently incorrect tests that would
> cause certain kernel supplied optimal io size to be discarded in
> favor of the default 1MB alignment, such as 1.5 MB.  Remove these
> tests and accept the kernel value if it is non zero.
> ---
>  libparted/arch/linux.c |   10 +---------
>  1 file changed, 1 insertion(+), 9 deletions(-)
> 
> diff --git a/libparted/arch/linux.c b/libparted/arch/linux.c
> index fd4ba32..bbb346a 100644
> --- a/libparted/arch/linux.c
> +++ b/libparted/arch/linux.c
> @@ -2986,15 +2986,7 @@ linux_get_optimum_alignment(const PedDevice *dev)
>             previous logic. */
>          unsigned long optimal_io = blkid_topology_get_optimal_io_size(tp);
>          unsigned long minimum_io = blkid_topology_get_minimum_io_size(tp);
> -        if (
> -            (!optimal_io && !minimum_io)
> -	    || (optimal_io && PED_DEFAULT_ALIGNMENT % optimal_io == 0
> -		&& minimum_io && PED_DEFAULT_ALIGNMENT % minimum_io == 0)
> -	    || (!minimum_io && optimal_io
> -		&& PED_DEFAULT_ALIGNMENT % optimal_io == 0)
> -	    || (!optimal_io && minimum_io
> -		&& PED_DEFAULT_ALIGNMENT % minimum_io == 0)
> -           ) {
> +	if (!optimal_io) {
>              /* DASD needs to use minimum alignment */
>              if (dev->type == PED_DEVICE_DASD)
>                  return linux_get_minimum_alignment(dev);

After re-reading the bug and the code (again) I think I have a better
handle on this.

1. get_optimum_alignment is only used to drive the 'not aligned' warning
in parted. It doesn't actually change anything. Anaconda uses it to
determine how it should align partitions which was the main reason for
the changes.

2. The code inside the block is only used if the *_io value is <
PED_DEFAULT_ALIGNMENT and is evenly divisible. The modulo check won't
return 0 for any value larger than PED_DEFAULT_ALIGNMENT so there is no
need for an additional check.

3. Hardware may report bogus values (eg. 512b on a 4k disk), the kernel
just passes this along without any way to tell so it is safer to align
on 1MiB unless we have better information.

4. This is pretty well tested code on a variety of storage systems. I
don't think that we can be sure that ripping it out won't break
something.

So, count me as a NAK to this change.

-- 
Brian C. Lane | Anaconda Team | IRC: bcl #anaconda | Port Orchard, WA (PST8PDT)
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 482 bytes
Desc: not available
URL: <http://lists.alioth.debian.org/pipermail/parted-devel/attachments/20130104/325c2c81/attachment.pgp>