[PATCH v2 0/2] disk: use maximum number of sectors for LBA

[PATCH v2 0/2] disk: use maximum number of sectors for LBA

[ValdikSS via Grub-devel]

GRUB2 limits number of sectors read at once in LBA mode to the
number reported in CHS disk geometry by BIOS.
This is unnecessary, as IBM/MS INT13 Extensions does not have
such limit and it is safe to read up to 127 sectors in a single call.

This fixes greatly increased boot times on WYSE C10LE x86 thin
client with Phoenix bios.

See: https://lore.kernel.org/grub-devel/d42a11fa-2a59-b5e7-08b1-d2c60444bb99@valdikss.org.ru/

ValdikSS (2):
  disk: read up to 63 sectors in LBA mode
  disk: increase sector size up to 127 for LBA reads

 grub-core/disk/i386/pc/biosdisk.c | 5 ++++-
 include/grub/disk.h               | 7 +++++--
 2 files changed, 9 insertions(+), 3 deletions(-)

-- 
2.41.0


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

[PATCH v2 1/2] disk: read up to 63 sectors in LBA mode

[ValdikSS via Grub-devel]

Current code impose limitations on the amount of sectors read
in a single call according to CHS layout of the disk, even in LBA
read mode.

There's no need to obey CHS layout restrictions for LBA reads
on LBA disks, it only slows down booting process.

See: https://lore.kernel.org/grub-devel/d42a11fa-2a59-b5e7-08b1-d2c60444bb99@valdikss.org.ru/

Signed-off-by: ValdikSS <iam@valdikss.org.ru>
---
 grub-core/disk/i386/pc/biosdisk.c | 5 ++++-
 include/grub/disk.h               | 3 +++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/grub-core/disk/i386/pc/biosdisk.c b/grub-core/disk/i386/pc/biosdisk.c
index f9362d17c..1d6788950 100644
--- a/grub-core/disk/i386/pc/biosdisk.c
+++ b/grub-core/disk/i386/pc/biosdisk.c
@@ -471,7 +471,7 @@ grub_biosdisk_rw (int cmd, grub_disk_t disk,
       struct grub_biosdisk_dap *dap;
 
       dap = (struct grub_biosdisk_dap *) (GRUB_MEMORY_MACHINE_SCRATCH_ADDR
-					  + (data->sectors
+					  + (GRUB_DISK_MAX_LBA_SECTORS
 					     << disk->log_sector_size));
       dap->length = sizeof (*dap);
       dap->reserved = 0;
@@ -561,6 +561,9 @@ get_safe_sectors (grub_disk_t disk, grub_disk_addr_t sector)
   struct grub_biosdisk_data *data = disk->data;
   grub_uint32_t sectors = data->sectors;
 
+  if (data->flags & GRUB_BIOSDISK_FLAG_LBA)
+    sectors = GRUB_DISK_MAX_LBA_SECTORS;
+
   /* OFFSET = SECTOR % SECTORS */
   grub_divmod64 (sector, sectors, &offset);
 
diff --git a/include/grub/disk.h b/include/grub/disk.h
index a4b5d13f3..be032a72c 100644
--- a/include/grub/disk.h
+++ b/include/grub/disk.h
@@ -190,6 +190,9 @@ typedef struct grub_disk_memberlist *grub_disk_memberlist_t;
 
 #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
+
 /* 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
https://lists.gnu.org/mailman/listinfo/grub-devel

[PATCH v2 2/2] disk: increase sector size up to 127 for LBA reads

[ValdikSS via Grub-devel]

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 and OSDev, 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 the limit, but it was no-op due to other
constraints. This value is also used in syslinux.

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).

See: https://en.wikipedia.org/wiki/INT_13H#INT_13h_AH=42h:_Extended_Read_Sectors_From_Drive
See: https://wiki.osdev.org/Disk_access_using_the_BIOS_(INT_13h)#LBA_in_Extended_Mode
See: https://github.com/rhboot/grub2/blob/10f8ffc133553209ec1ddaadc6f4a8a25d3dea4e/grub-core/disk/i386/pc/biosdisk.c#L434
See: https://github.com/geneC/syslinux/blob/5e426532210bb830d2d7426eb8d8c154d9dfcba6/core/fs/diskio_bios.c#L349

Signed-off-by: ValdikSS <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
https://lists.gnu.org/mailman/listinfo/grub-devel

Re: [PATCH v2 2/2] disk: increase sector size up to 127 for LBA reads

[Glenn Washburn]

On Fri,  6 Oct 2023 20:10:13 +0300
ValdikSS via Grub-devel <grub-devel@gnu.org> wrote:

> 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 and OSDev, 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 the limit, but it was no-op due to other
> constraints. This value is also used in syslinux.
> 
> 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).

Much better explanation, thanks.

> 
> See: https://en.wikipedia.org/wiki/INT_13H#INT_13h_AH=42h:_Extended_Read_Sectors_From_Drive
> See: https://wiki.osdev.org/Disk_access_using_the_BIOS_(INT_13h)#LBA_in_Extended_Mode
> See: https://github.com/rhboot/grub2/blob/10f8ffc133553209ec1ddaadc6f4a8a25d3dea4e/grub-core/disk/i386/pc/biosdisk.c#L434

Commit messages should not use links to downstream projects. The commit
hash referred to in that link does not exist in GRUB master. Here's a
more appropriate link:

https://git.savannah.gnu.org/cgit/grub.git/tree/grub-core/disk/i386/pc/biosdisk.c?h=grub-2.12-rc1#n430

> See: https://github.com/geneC/syslinux/blob/5e426532210bb830d2d7426eb8d8c154d9dfcba6/core/fs/diskio_bios.c#L349

I would also change this to link to the official syslinux repository,
not an unofficial mirror that may be gone tomorrow. I'd use this link
instead:

https://repo.or.cz/syslinux.git/blob/refs/tags/syslinux-6.03:/core/fs/diskio_bios.c#l349

Also, the commit message could be improved by using the reference syntax
(I think its markdown). This will show where the link is relevant. So
this link would be [n], where n is an integer usually starting from 1.
Then above I would use "syslinux[n]". This way its more clear in
context why this link is here. It would be nice to have all the links
this way. Look at other commit messages for guidance.

Glenn

> 
> Signed-off-by: ValdikSS <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

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

Re: [PATCH v2 2/2] disk: increase sector size up to 127 for LBA reads

[ValdikSS via Grub-devel]

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

On 06.10.2023 21:21, Glenn Washburn wrote:
> Commit messages should not use links to downstream projects. The commit
> hash referred to in that link does not exist in GRUB master. Here's a
> more appropriate link:
> 
> https://git.savannah.gnu.org/cgit/grub.git/tree/grub-core/disk/i386/pc/biosdisk.c?h=grub-2.12-rc1#n430
> 
>> See: https://github.com/geneC/syslinux/blob/5e426532210bb830d2d7426eb8d8c154d9dfcba6/core/fs/diskio_bios.c#L349
> 
> I would also change this to link to the official syslinux repository,
> not an unofficial mirror that may be gone tomorrow. I'd use this link
> instead:
> 
> https://repo.or.cz/syslinux.git/blob/refs/tags/syslinux-6.03:/core/fs/diskio_bios.c#l349

Indeed, I just took the first links without validation, thanks.
I'll wait for other comments, and if there aren't any, will send updated 
patchset.

Note that 63 vs 127 patch (2/2) have only marginal speedup and thus not 
that important. Since it causes lots of questions and fear of breaking 
things from you and Vladimir, I guess we should just not apply it.

[-- 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

Re: [PATCH v2 2/2] disk: increase sector size up to 127 for LBA reads

[Vladimir 'phcoder' Serbinenko]

[-- Attachment #1.1: Type: text/plain, Size: 3135 bytes --]

Le ven. 6 oct. 2023, 19:13, ValdikSS via Grub-devel <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.
>

These are 2 independent changes and need to be discussed separately.
E.g. it has an unintentional effect of doubling the cache size. I wonder if
this alone might explain your speed-up. Yet this might be bad for low RAM
scenarios including the cases like sparc64 where only few MiBs of RAM are
available to us independently of total RAM size

> 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 and OSDev, 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 the limit, but it was no-op due to other
> constraints. This value is also used in syslinux.
>
> 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).
>
> See:
> https://en.wikipedia.org/wiki/INT_13H#INT_13h_AH=42h:_Extended_Read_Sectors_From_Drive
> See:
> https://wiki.osdev.org/Disk_access_using_the_BIOS_(INT_13h)#LBA_in_Extended_Mode
> See:
> https://github.com/rhboot/grub2/blob/10f8ffc133553209ec1ddaadc6f4a8a25d3dea4e/grub-core/disk/i386/pc/biosdisk.c#L434
> See:
> https://github.com/geneC/syslinux/blob/5e426532210bb830d2d7426eb8d8c154d9dfcba6/core/fs/diskio_bios.c#L349
>
> Signed-off-by: ValdikSS <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
> https://lists.gnu.org/mailman/listinfo/grub-devel
>

[-- Attachment #1.2: Type: text/html, Size: 4681 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

Re: [PATCH v2 0/2] disk: use maximum number of sectors for LBA

[Daniel Kiper]

On Fri, Oct 06, 2023 at 08:10:11PM +0300, ValdikSS via Grub-devel wrote:
> GRUB2 limits number of sectors read at once in LBA mode to the
> number reported in CHS disk geometry by BIOS.
> This is unnecessary, as IBM/MS INT13 Extensions does not have
> such limit and it is safe to read up to 127 sectors in a single call.
>
> This fixes greatly increased boot times on WYSE C10LE x86 thin
> client with Phoenix bios.
>
> See: https://lore.kernel.org/grub-devel/d42a11fa-2a59-b5e7-08b1-d2c60444bb99@valdikss.org.ru/
>
> ValdikSS (2):
>   disk: read up to 63 sectors in LBA mode
>   disk: increase sector size up to 127 for LBA reads

After reading whole discussion about this patch set I think it is quite
safe to take both patches into the release. If we realy want to be on the
safe side we can drop the second one and merge it after the release.

Daniel

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

Re: [PATCH v2 2/2] disk: increase sector size up to 127 for LBA reads

[ValdikSS via Grub-devel]

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

On 06.10.2023 23:54, Vladimir 'phcoder' Serbinenko wrote:
> 
> 
> Le ven. 6 oct. 2023, 19:13, 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.
> 
> 
> These are 2 independent changes and need to be discussed separately.
> E.g. it has an unintentional effect of doubling the cache size. I wonder 
> if this alone might explain your speed-up.

It is not. That was probably the first thing I have tried, as I found 
somewhat similar issues with UEFI loading from 2012 which were solved by 
increased cache:
https://bugs.launchpad.net/ubuntu/+source/grub2/+bug/944347

> The performance problem appears to be due to overhead of the individual disk read calls...
> 
> grub2's disk I/O layer buffers 8 disk sectors per read, hard-coded in include/grub/disk.h as GRUB_DISK_CACHE_SIZE.
> 
> Increasing GRUB_DISK_CACHE_SIZE to 1024 (patch attached) dramatically improves the load performance for my Intel Tunnel Mountain system booting with UEFI: total load time for linux and initrd is reduced from about 53 seconds to about 1 second.

But that is not the reason for my issue.

P.S. Vladimir, please send emails to me with my email address in To/CC, 
I'm not subscribed to every message in the mail list.


[-- 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

Re: [PATCH v2 0/2] disk: use maximum number of sectors for LBA

[Daniel Kiper]

On Mon, Oct 09, 2023 at 05:08:21PM +0200, Daniel Kiper wrote:
> On Fri, Oct 06, 2023 at 08:10:11PM +0300, ValdikSS via Grub-devel wrote:
> > GRUB2 limits number of sectors read at once in LBA mode to the
> > number reported in CHS disk geometry by BIOS.
> > This is unnecessary, as IBM/MS INT13 Extensions does not have
> > such limit and it is safe to read up to 127 sectors in a single call.
> >
> > This fixes greatly increased boot times on WYSE C10LE x86 thin
> > client with Phoenix bios.
> >
> > See: https://lore.kernel.org/grub-devel/d42a11fa-2a59-b5e7-08b1-d2c60444bb99@valdikss.org.ru/
> >
> > ValdikSS (2):
> >   disk: read up to 63 sectors in LBA mode
> >   disk: increase sector size up to 127 for LBA reads
>
> After reading whole discussion about this patch set I think it is quite
> safe to take both patches into the release. If we realy want to be on the
> safe side we can drop the second one and merge it after the release.

ValdikSS, Vladimir and I agreed we will take patch #1 into the release.
The patch #2 will be merged after the release. Though please update both
patches and post v3 now.

Thank you for debugging and fixing this issue!

Daniel

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

Re: [PATCH v2 0/2] disk: use maximum number of sectors for LBA

[ValdikSS via Grub-devel]

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

On 10.10.2023 14:34, Daniel Kiper wrote:
> ValdikSS, Vladimir and I agreed we will take patch #1 into the release.
> The patch #2 will be merged after the release. Though please update both
> patches and post v3 now.
> 
> Thank you for debugging and fixing this issue!
> 
> Daniel

Done, check v3.

[-- 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