Re: [PATCH v3 2/2] disk: increase sector size up to 127 for LBA reads - ValdikSS via Grub-devel

From: ValdikSS via Grub-devel <grub-devel@gnu.org>
To: The development of GNU GRUB <grub-devel@gnu.org>,
	Vladimir 'phcoder' Serbinenko <phcoder@gmail.com>
Cc: ValdikSS <iam@valdikss.org.ru>
Subject: Re: [PATCH v3 2/2] disk: increase sector size up to 127 for LBA reads
Date: Fri, 9 Feb 2024 20:58:56 +0300	[thread overview]
Message-ID: <f91c8374-0ea9-4f50-9258-2d820c1c66c0@valdikss.org.ru> (raw)
In-Reply-To: <CAEaD8JMf2AnUoNaU16r4MLoTYShkttnRFzJKyqj55EB48fHF-w@mail.gmail.com>

[-- Attachment #1.1.1: Type: text/plain, Size: 5096 bytes --]

On 09.02.2024 05:01, Vladimir 'phcoder' Serbinenko wrote:
> I have few concerns:
> 1. The name of your patch is misleading. You don't increase sector size

+
Do you want me to resend the patch or you'll rename it yourself?

> 2. This increases memory pressure on systems that are already low on 
> usable memory. Sparc64 comes to mind. Due to the way how off handles 
> memory map there, even if you have a terabyte of RAM, only few MiB are 
> usable at boot. We may need to do it dynamically, otherwise we either 
> leave a lot of gain on systems that can read megabytes at once or we 
> fail on low memory systems

I don't know anything about boot process of SPARC64 (I don't know about 
x86 much to begin with).

> 3. Firmware bugs. Not even just BIOS. This changes all the systems. We 
> need to review all the drivers

If the patch may break some systems, better not to merge it. It doesn't 
bring that major speed gain, and measurably speeds the booting process 
only on legacy machines with not very optimized BIOS calls.

It's not worth it.

> 4. You mentioned that other implementations that increase the size of 
> single read have a fallback. Do we have one?

Yes, CHR reading is used if MBR has failed.

P.S. please include my email when you send me the messages, I saw this 
maillist-only post only by pure coincidence.

> 
> Le mar. 10 oct. 2023, 21:30, ValdikSS via Grub-devel <grub-devel@gnu.org 
> <mailto:grub-devel@gnu.org>> a écrit :
> 
>     Increase the value from 63 to speed up reading process.
> 
>     This commit increases two limits: the low-level int 13h reading code
>     and a high-level reading code with disk cache.
>     The disk cache imposes an overall limitation of a higher-layer reading
>     code. The original comment regarding 16K is incorrect, it was
>     512<<6 = 32768, and now it is 512<<7 = 65536.
> 
>     According to [Wikipedia][1] and [OSDev][2], the upper safe value for LBA
>     read using IBM/MS INT13 Extensions is 127 sectors due to the
>     limitations of some BIOSes.
>     GRUB [already enforced][3] the limit, but it was no-op due to other
>     constraints. This value is also used in [syslinux][4].
> 
>     As we're now reading up to 127 sectors of 512 bytes, we need to be able
>     to store in the cache up to 65024 bytes. Without this change, GRUB
>     wouldn't try to read more than 64 sectors at once
>     (even if the lower reading layer allows it).
> 
>     [1]:
>     https://en.wikipedia.org/wiki/INT_13H#INT_13h_AH=42h:_Extended_Read_Sectors_From_Drive <https://en.wikipedia.org/wiki/INT_13H#INT_13h_AH=42h:_Extended_Read_Sectors_From_Drive>
>     [2]:
>     https://wiki.osdev.org/Disk_access_using_the_BIOS_(INT_13h)#LBA_in_Extended_Mode <https://wiki.osdev.org/Disk_access_using_the_BIOS_(INT_13h)#LBA_in_Extended_Mode>
>     [3]:
>     https://git.savannah.gnu.org/cgit/grub.git/tree/grub-core/disk/i386/pc/biosdisk.c?h=grub-2.12-rc1#n430 <https://git.savannah.gnu.org/cgit/grub.git/tree/grub-core/disk/i386/pc/biosdisk.c?h=grub-2.12-rc1#n430>
>     [4]:
>     https://repo.or.cz/syslinux.git/blob/refs/tags/syslinux-6.03:/core/fs/diskio_bios.c#l349 <https://repo.or.cz/syslinux.git/blob/refs/tags/syslinux-6.03:/core/fs/diskio_bios.c#l349>
> 
>     Signed-off-by: ValdikSS <iam@valdikss.org.ru
>     <mailto:iam@valdikss.org.ru>>
>     ---
>       include/grub/disk.h | 6 +++---
>       1 file changed, 3 insertions(+), 3 deletions(-)
> 
>     diff --git a/include/grub/disk.h b/include/grub/disk.h
>     index be032a72c..608deb034 100644
>     --- a/include/grub/disk.h
>     +++ b/include/grub/disk.h
>     @@ -184,14 +184,14 @@ typedef struct grub_disk_memberlist
>     *grub_disk_memberlist_t;
>       #define GRUB_MDRAID_MAX_DISKS  4096
> 
>       /* The size of a disk cache in 512B units. Must be at least as big
>     as the
>     -   largest supported sector size, currently 16K.  */
>     -#define GRUB_DISK_CACHE_BITS   6
>     +   largest supported sector size, currently 64K.  */
>     +#define GRUB_DISK_CACHE_BITS   7
>       #define GRUB_DISK_CACHE_SIZE   (1 << GRUB_DISK_CACHE_BITS)
> 
>       #define GRUB_DISK_MAX_MAX_AGGLOMERATE ((1 << (30 -
>     GRUB_DISK_CACHE_BITS - GRUB_DISK_SECTOR_BITS)) - 1)
> 
>       /* Maximum number of sectors to read in LBA mode at once */
>     -#define GRUB_DISK_MAX_LBA_SECTORS 63
>     +#define GRUB_DISK_MAX_LBA_SECTORS 127
> 
>       /* Return value of grub_disk_native_sectors() in case disk size is
>     unknown. */
>       #define GRUB_DISK_SIZE_UNKNOWN  0xffffffffffffffffULL
>     -- 
>     2.41.0
> 
> 
>     _______________________________________________
>     Grub-devel mailing list
>     Grub-devel@gnu.org <mailto:Grub-devel@gnu.org>
>     https://lists.gnu.org/mailman/listinfo/grub-devel
>     <https://lists.gnu.org/mailman/listinfo/grub-devel>
> 
> 
> _______________________________________________
> Grub-devel mailing list
> Grub-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/grub-devel

[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

[-- Attachment #2: Type: text/plain, Size: 141 bytes --]

_______________________________________________
Grub-devel mailing list
Grub-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/grub-devel