biosdisk / open_device() messing up offsets
PATCH 1/1 biosdisk / open_device() messing up offsets
It seems that open_device() in biosdisk is messing up offsets when accessing partitions. For example if you try:
grub> hexdump (hd0,1)
in grub-emu, you'll get a message saying lseek failed. The problem is that it substracts to the sector offset, so all accesses get to wrong data and accessing sector 0 results in underflow.
I would think those lines (see patch) are plainly wrong, but they appear to be very old, and it is strange that this wasn't noticed earlier. Maybe we have changed behaviour around partition/disk offsets, causing this breakage without noticing?
Signature · 5 lines
Robert Millan <GPLv2> I know my rights; I want my phone call! <DRM> What good is a phone call… if you are unable to speak? (as seen on /.)
util/biosdisk.c renamed from grub2/util/biosdisk.c +0 -3
@@ -298,9 +298,6 @@ open_device (const grub_disk_t disk, gru/* Make the buffer cache consistent with the physical disk. */ ioctl (fd, BLKFLSBUF, 0); - -if (is_partition) - sector -= disk->partition->start; }#else /* ! __linux__ */ fd =open (map[disk->id].device, flags);
Quoted 5 lines · Robert Millan
It seems that
open_device()in biosdisk is messing up offsets when accessing partitions. For example if you try:grub> hexdump (hd0,1)
Quoted 8 lines
in grub-emu, you'll get a message saying lseek failed. The problem is that it substracts to the sector offset, so all accesses get to wrong data and accessing sector 0 results in underflow.
I would think those lines (see patch) are plainly wrong, but they appear to be very old, and it is strange that this wasn't noticed earlier. Maybe we have changed behaviour around partition/disk offsets, causing this breakage without noticing?
The question whether sector is relative to the partition or to the whole disk. From what I see in disk/memdisk.c or it disk/i386/pc/biosdisk.c, the sector is relative to the whole disk. There is no compensation for partition offsets. That's low-level code that doesn't know about partitions, and util/biosdisk.c correctly tries to emulate that.
I guess grub-emu gets it wrong somewhere.
Signature · 2 lines
Regards, Pavel Roskin
Quoted 15 lines · Pavel Roskin
Quoted 5 lines · Robert Millan
It seems that
open_device()in biosdisk is messing up offsets when accessing partitions. For example if you try:grub> hexdump (hd0,1)
Quoted 8 lines
in grub-emu, you'll get a message saying lseek failed. The problem is that it substracts to the sector offset, so all accesses get to wrong data and accessing sector 0 results in underflow.
I would think those lines (see patch) are plainly wrong, but they appear to be very old, and it is strange that this wasn't noticed earlier. Maybe we have changed behaviour around partition/disk offsets, causing this breakage without noticing?
Quoted 7 lines
The question whether sector is relative to the partition or to the whole disk. From what I see in
disk/memdisk.cor itdisk/i386/pc/biosdisk.c, the sector is relative to the whole disk. There is no compensation for partition offsets. That's low-level code that doesn't know about partitions, andutil/biosdisk.ccorrectly tries to emulate that.I guess grub-emu gets it wrong somewhere.
grub-emu itself does nothing about this AFAIK, it just relies on util/biosdisk.c to do the right thing.
Signature · 5 lines
Robert Millan <GPLv2> I know my rights; I want my phone call! <DRM> What good is a phone call… if you are unable to speak? (as seen on /.)
Committed.
Quoted 40 lines · Robert Millan
It seems that
open_device()in biosdisk is messing up offsets when accessing partitions. For example if you try:grub> hexdump (hd0,1)
in grub-emu, you'll get a message saying lseek failed. The problem is that it substracts to the sector offset, so all accesses get to wrong data and accessing sector 0 results in underflow.
I would think those lines (see patch) are plainly wrong, but they appear to be very old, and it is strange that this wasn't noticed earlier. Maybe we have changed behaviour around partition/disk offsets, causing this breakage without noticing?
Signature · 7 lines
Robert Millan <GPLv2> I know my rights; I want my phone call! <DRM> What good is a phone call… if you are unable to speak? (as seen on /.) diff -x ChangeLog -x configure -x config.h.in -x CVS -x '*~' -x '*.mk' -urp ../grub2/util/biosdisk.c ./util/biosdisk.cutil/biosdisk.c renamed from grub2/util/biosdisk.c +0
-3@@ -298,9 +298,6 @@ open_device (const grub_disk_t disk, gru/* Make the buffer cache consistent with the physical disk. */ ioctl (fd, BLKFLSBUF, 0); - -if (is_partition) - sector -= disk->partition->start; }#else /* ! __linux__ */ fd =open (map[disk->id].device, flags);_______________________________________________
Grub-devel mailing list
[email protected]
http://lists.gnu.org/mailman/listinfo/grub-devel
Signature · 5 lines
Robert Millan <GPLv2> I know my rights; I want my phone call! <DRM> What good is a phone call… if you are unable to speak? (as seen on /.)
Quoted 1 line · Robert Millan
Committed.
I believe it's wrong. util/biosdisk.c is a low level disk driver. It's supposed to read data relative to the disk, like other drivers do.
Besides, is_partition becomes write-only. It means that the result of linux_find_partition() is ignored. Please note that linux_find_partition() modifies its first argument on success. It rewrites the disk device (e.g. /dev/sda) with the partition device (e.g. /dev/sda1).
If the result of linux_find_partition() is not checked, nobody knows whether fd is a handle to the partition or to the whole disk. Yet the sector variable is used to seek on that device. Needless to say that different values would be obtained.
Also, grub-emu is not the only user of util/biosdisk.c. It is also used by grub-setup, which is working correctly, as far as I can tell.
By the way, the current GRUB just failed to install on my test system. After I rebooted it, GRUB hung on startup after showing "GRUB".
It may be something else. It was an unclean reboot after a panic for unrelated reasons, so the hanging problem may be an issue with ext3. Anyway, extreme caution is required with the current code!
Signature · 2 lines
Regards, Pavel Roskin
Quoted 4 lines · Pavel Roskin
Quoted 1 line · Robert Millan
Committed.
I believe it's wrong.
You mean that the lines I removed were wrong, or that my commit is wrong?
I thought you agreed that those lines were bogus. If not, then we'd have to figure out why sector 0 of the partition can be accessed directly via hexdump.
Quoted 2 lines
By the way, the current GRUB just failed to install on my test system. After I rebooted it, GRUB hung on startup after showing "GRUB".
Please run a regression test, if you can.
Signature · 5 lines
Robert Millan <GPLv2> I know my rights; I want my phone call! <DRM> What good is a phone call… if you are unable to speak? (as seen on /.)
Quoting Robert Millan <[email protected]>:
Quoted 7 lines · Pavel Roskin
Quoted 4 lines · Robert Millan
Quoted 1 line
Committed.
I believe it's wrong.
You mean that the lines I removed were wrong, or that my commit is wrong?
The commit is wrong.
Quoted 1 line
I thought you agreed that those lines were bogus.
No, I explained why they were needed. Sorry, I must have been unclear.
Quoted 3 lines
If not, then we'd have
to figure out why sector 0 of the partition can be accessed directly via hexdump.
Absolutely.
Quoted 4 lines
Quoted 2 lines
By the way, the current GRUB just failed to install on my test system. After I rebooted it, GRUB hung on startup after showing "GRUB".
Please run a regression test, if you can.
I could boot from the rescue CD, so it's not that bad.
I'm looking at the problem now.
Signature · 2 lines
Regards, Pavel Roskin
Quoted 11 lines · Pavel Roskin
Quoting Robert Millan <[email protected]>:
Quoted 7 lines · Pavel Roskin
Quoted 4 lines · Robert Millan
Quoted 1 line
Committed.
I believe it's wrong.
You mean that the lines I removed were wrong, or that my commit is wrong?
The commit is wrong.
Woops, sorry for the confusion then.
Signature · 5 lines
Robert Millan <GPLv2> I know my rights; I want my phone call! <DRM> What good is a phone call… if you are unable to speak? (as seen on /.)
Quoting Robert Millan <[email protected]>:
Quoted 3 lines
If not, then we'd have
to figure out why sector 0 of the partition can be accessed directly via hexdump.
As I understand, "(hd0,1)" doesn't refer to an extent of data, i.e. it not a file or a block list that can be opened like a file. "(hd0,1)+1" is the first sector of the partition. But "(hd0,1)" is not all sectors of the partition. It the partition name. It can be used in the "ls" command, whereas "(hd0,1)+1" cannot.
Quoted 4 lines
Quoted 2 lines
By the way, the current GRUB just failed to install on my test system. After I rebooted it, GRUB hung on startup after showing "GRUB".
Please run a regression test, if you can.
The installation was broken by that patch, so I reverted it. It's easy to check the installation by doing it on a mounted flash drive, so that the real boot device is not at risk.
Signature · 2 lines
Regards, Pavel Roskin
Quoted 12 lines · Pavel Roskin
Quoting Robert Millan <[email protected]>:
Quoted 3 lines
If not, then we'd have
to figure out why sector 0 of the partition can be accessed directly via hexdump.As I understand, "(hd0,1)" doesn't refer to an extent of data, i.e. it not a file or a block list that can be opened like a file. "(hd0,1)+1" is the first sector of the partition. But "(hd0,1)" is not all sectors of the partition. It the partition name. It can be used in the "ls" command, whereas "(hd0,1)+1" cannot.
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
By the way, the current GRUB just failed to install on my test system. After I rebooted it, GRUB hung on startup after showing "GRUB".
Please run a regression test, if you can.
The installation was broken by that patch, so I reverted it. It's easy to check the installation by doing it on a mounted flash drive, so that the real boot device is not at risk.
Hi,
I believe the problem is with hexdump. It open the device with grub_disk_open, which returns the disk object related to the beginning of partition. However, it read it using disk->dev->read, which is a low level api that use absolute address. It should be using grub_disk_read instead.
BTW, I also find an ingesting situation when writing the load_env command. The read hook return absolute address, so grub_disk_read would not work. In that case, we have to use the low level disk->dev->read.
Signature · 1 line
Bean
Quoted 5 lines · Bean
I believe the problem is with hexdump. It open the device with
grub_disk_open, which returns the disk object related to the beginning of partition. However, it read it usingdisk->dev->read, which is a low level api that use absolute address. It should be usinggrub_disk_readinstead.
Nice catch! Indeed, hexdump has special handling for the whole partitions. And it actually tries to satisfy the low level API by converting offset to the sector number and skipping the remainder. I guess it could be simplified if grub_disk_read() is used.
Quoted 4 lines
BTW, I also find an ingesting situation when writing the
load_envcommand. The read hook return absolute address, sogrub_disk_readwould not work. In that case, we have to use the low leveldisk->dev->read.
In that case it may be justified.
Signature · 2 lines
Regards, Pavel Roskin
Quoted 8 lines · Pavel Roskin
Quoted 5 lines · Bean
I believe the problem is with hexdump. It open the device with
grub_disk_open, which returns the disk object related to the beginning of partition. However, it read it usingdisk->dev->read, which is a low level api that use absolute address. It should be usinggrub_disk_readinstead.
Quoted 4 lines
Nice catch! Indeed, hexdump has special handling for the whole partitions. And it actually tries to satisfy the low level API by converting offset to the sector number and skipping the remainder. I guess it could be simplified if
grub_disk_read()is used.
The reason I add device support for hexdump is to debug the nand driver. I need to go around the disk cache and call the underlying disk driver directly, so I use disk->dev->read. For (nand), there is just one partition, so I didn't notice the problem then.
Signature · 1 line
Bean
Quoted 14 lines · Bean
Quoted 8 lines · Pavel Roskin
Quoted 5 lines · Bean
I believe the problem is with hexdump. It open the device with
grub_disk_open, which returns the disk object related to the beginning of partition. However, it read it usingdisk->dev->read, which is a low level api that use absolute address. It should be usinggrub_disk_readinstead.Quoted 4 lines
Nice catch! Indeed, hexdump has special handling for the whole partitions. And it actually tries to satisfy the low level API by converting offset to the sector number and skipping the remainder. I guess it could be simplified if
grub_disk_read()is used.
Quoted 4 lines
The reason I add device support for hexdump is to debug the nand driver. I need to go around the disk cache and call the underlying disk driver directly, so I use
disk->dev->read. For (nand), there is just one partition, so I didn't notice the problem then.
Here's the patch. Everything seems to be OK. "--skip=N" is not recognized, but it's something in the option parsing code. "-s N" is working.
Please feel free to apply.
commands/hexdump.c +5 -13
@@ -99,35 +99,27 @@ grub_cmd_hexdump (struct grub_arg_list *state, int argc, char **args)else if ((args[0 ][0 ] =='(' ) && (args[0 ][namelen -1 ] ==')' )) {grub_disk_t disk; -grub_disk_addr_t sector; -grub_size_t ofs; args[0 ][namelen -1 ] =0 ; disk =grub_disk_open (&args[0 ][1 ]);if (! disk)return 0 ; - sector = (skip >> (GRUB_DISK_SECTOR_BITS +2 )) *4 ; - ofs = skip & (GRUB_DISK_SECTOR_SIZE *4 -1 );while (length) { -grub_size_t len, n; +grub_size_t len; len = length; -if (ofs + len >sizeof (buf)) - len =sizeof (buf) - ofs; +if (len >sizeof (buf)) + len =sizeof (buf); - n = ((ofs + len +GRUB_DISK_SECTOR_SIZE -1 ) - >>GRUB_DISK_SECTOR_BITS ); -if (disk->dev->read (disk, sector, n, buf)) +if (grub_disk_read (disk,0 , skip, len, buf))break ; -hexdump (skip, &buf[ofs], len); +hexdump (skip, buf, len); - ofs =0 ; skip += len; length -= len; - sector +=4 ; }grub_disk_close (disk);
Signature · 2 lines
Regards, Pavel Roskin
Quoted 11 lines · Pavel Roskin
Quoted 5 lines
The reason I add device support for hexdump is to debug the nand driver. I need to go around the disk cache and call the underlying disk driver directly, so I use
disk->dev->read. For (nand), there is just one partition, so I didn't notice the problem then.Here's the patch. Everything seems to be OK. "--skip=N" is not recognized, but it's something in the option parsing code. "-s N" is working.
Please feel free to apply.
Perhaps we should keep the low level api, just fix the offset. Hexdump is normally used for debugging, and the disk cache is quite annoying.
btw, please try the patch, it dumps the journal information. There could be quite some output, you can use grub-fstest to capture it.
Signature · 1 line
Bean
fs/ext2.c +15 -1
@@ -443,14 +443,19 @@ grub_ext3_get_journal (struct grub_ext2_data *data) log->last_block =grub_be_to_cpu32 (jsb->maxlen); log->start_block =grub_be_to_cpu32 (jsb->start); +grub_printf ("header: %d %d %d\n" , log->first_block, log->last_block, log->start_block); + last_num = num =0 ; block = log->start_block; seq =grub_be_to_cpu32 (jsb->sequence); +grub_printf ("seq: %d\n" , seq);while (1 ) {struct grub_ext3_journal_header *jh; +grub_printf ("block: %d\n" , block); + if (grub_fshelp_read_file (data->disk, &data->logfile, 0, block << (log2bs + 9), sizeof (buf), buf, grub_ext2_read_block,
@@ -476,6 +481,7 @@ grub_ext3_get_journal (struct grub_ext2_data *data)struct grub_ext3_journal_block_tag *tag;int ofs, flags; +grub_printf ("dblock: " ); ofs =sizeof (struct grub_ext3_journal_header ); do
@@ -491,15 +497,19 @@ grub_ext3_get_journal (struct grub_ext2_data *data) ofs +=16 ; log->mapping[num++] =grub_be_to_cpu32 (tag->block); +grub_printf ("%d " ,grub_be_to_cpu32 (tag->block));next_block (); }while (! (flags &EXT3_JOURNAL_FLAG_LAST_TAG )); +grub_printf ("\n" ); +continue ; }case EXT3_JOURNAL_COMMIT_BLOCK : { +grub_printf ("cblock: %d\n" , seq); seq++; last_num = num -1 ;continue ;
@@ -510,6 +520,8 @@ grub_ext3_get_journal (struct grub_ext2_data *data)struct grub_ext3_journal_revoke_header *jrh;grub_uint32_t i; +grub_printf ("rblock: %d " , jrh->count); + jrh = (struct grub_ext3_journal_revoke_header *) jh;for (i =0 ; i <grub_be_to_cpu32 (jrh->count); i++)
@@ -518,11 +530,12 @@ grub_ext3_get_journal (struct grub_ext2_data *data)grub_uint32_t map; map =grub_be_to_cpu32 (jrh->data[i]); +grub_printf ("%d " , map);for (j =0 ; j < num; j++)if (log->mapping[j] == map) log->mapping[j] =GRUB_FSHELP_JOURNAL_UNUSED_MAPPING ; } - +grub_printf ("\n" );continue ; }default :
@@ -538,6 +551,7 @@ quit: {int size; +grub_printf ("num: %d\n" , last_num); size =sizeof (struct grub_fshelp_journal ) + last_num *sizeof (grub_disk_addr_t);
Quoted 7 lines · Bean
Quoted 6 lines · Pavel Roskin
Quoted 5 lines
The reason I add device support for hexdump is to debug the nand driver. I need to go around the disk cache and call the underlying disk driver directly, so I use
disk->dev->read. For (nand), there is just one partition, so I didn't notice the problem then.
Quoted 8 lines
Quoted 5 lines
Here's the patch. Everything seems to be OK. "--skip=N" is not recognized, but it's something in the option parsing code. "-s N" is working.
Please feel free to apply.
Perhaps we should keep the low level api, just fix the offset. Hexdump is normally used for debugging, and the disk cache is quite annoying.
OK, it's your choice.
Quoted 2 lines
btw, please try the patch, it dumps the journal information. There could be quite some output, you can use grub-fstest to capture it.
It breaks installation:
# grub-install /dev/sda grub-mkimage: error: cannot stat /usr/local/lib/grub/i386-pc/header:.mod
"header:" must be from the patch.
I cannot generate a short image easily because loopback devices are not supported. Perhaps I'll try making a real partition initialized with zeroes or something like that.
Signature · 2 lines
Regards, Pavel Roskin
Quoted 5 lines · Pavel Roskin
Quoted 2 lines
btw, please try the patch, it dumps the journal information. There could be quite some output, you can use grub-fstest to capture it.
It breaks installation:
Quoted 8 lines
# grub-install /dev/sda grub-mkimage: error: cannot stat /usr/local/lib/grub/i386-pc/header:.mod"header:" must be from the patch.
I cannot generate a short image easily because loopback devices are not supported. Perhaps I'll try making a real partition initialized with zeroes or something like that.
Can you use grub-fstest ?
Also, the header is from
+ grub_printf ("header: %d %d %d\n", log->first_block, log->last_block, log->start_block);
I don't know why the number doesn't show, unless log is invalid pointer. Perhaps the size is invalid, try to add a debug line to show the allocated size of log:
log = grub_malloc (sizeof (struct grub_fshelp_journal) +
grub_be_to_cpu32 (jsb->maxlen) * sizeof
(grub_disk_addr_t));
Signature · 1 line
Bean
Quoted 7 lines · Bean
Can you use grub-fstest ?
Also, the header is from
+
grub_printf("header: %d %d %d\n",log->first_block,log->last_block,log->start_block);
Quoted 7 lines
I don't know why the number doesn't show, unless log is invalid pointer. Perhaps the size is invalid, try to add a debug line to show the allocated size of log:
log =
grub_malloc(sizeof (structgrub_fshelp_journal) +grub_be_to_cpu32 (jsb->maxlen) * sizeof(
grub_disk_addr_t));
The debug output must be getting split somewhere. If I replace colons and spaces with underscores, I get some numbers. Anyway, the installation with the patch is unnecessary.
I made a short partition and installed grub on it, but there are no problems with it in qemu.
But I can reproduce the problem with grub-fstest on the root partition.
[root@dv grub2]# grub-fstest /dev/sda1 ls /
lost+found/ bin/ boot/ data/ dev/ etc/ home/ lib/ lib64/ media/ mnt/ opt/ proc/ root/ sbin/ scratchbox/ selinux/ srv/ sys/ tftpboot/ tmp/ usr/ var/ debug/ tftpboot;4834754b
[root@dv grub2]# grub-fstest /dev/sda1 ls /boot
[root@dv grub2]# ls /
bin debug home lost+found opt sbin srv
tftpboot;4834754b var
boot dev lib media proc scratchbox sys tmp data etc lib64 mnt root selinux tftpboot usr
[root@dv grub2]# ls /boot
System.map grub vmlinuz
System.map-2.6.26-rc4-wl initrd-2.6.26-rc4-wl.img
vmlinuz-2.6.26-rc4-wl
System.map-2.6.26-rc4-wl.old memtest86+-2.01
vmlinuz-2.6.26-rc4-wl.old
[root@dv grub2]#
As you can see, "grub-fstest /dev/sda1 ls /boot" fails to find any files. Then I patched grub with your patch. The output of the same command is attached (compressed).
Signature · 2 lines
Regards, Pavel Roskin
Quoted 45 lines · Pavel Roskin
Quoted 7 lines · Bean
Can you use grub-fstest ?
Also, the header is from
+
grub_printf("header: %d %d %d\n",log->first_block,log->last_block,log->start_block);Quoted 7 lines
I don't know why the number doesn't show, unless log is invalid pointer. Perhaps the size is invalid, try to add a debug line to show the allocated size of log:
log =
grub_malloc(sizeof (structgrub_fshelp_journal) +grub_be_to_cpu32 (jsb->maxlen) * sizeof(
grub_disk_addr_t));The debug output must be getting split somewhere. If I replace colons and spaces with underscores, I get some numbers. Anyway, the installation with the patch is unnecessary.
I made a short partition and installed grub on it, but there are no problems with it in qemu.
But I can reproduce the problem with grub-fstest on the root partition.
[root@dv grub2]# grub-fstest
/dev/sda1ls /
lost+found/ bin/ boot/ data/ dev/ etc/ home/lib/lib64/ media/ mnt/ opt/ proc/ root/ sbin/ scratchbox/ selinux/ srv/ sys/ tftpboot/ tmp/usr/var/ debug/ tftpboot;4834754b
[root@dv grub2]# grub-fstest/dev/sda1ls /boot[root@dv grub2]# ls /
bin debug home lost+found opt sbin srv
tftpboot;4834754b varboot dev lib media proc scratchbox sys tmp data etc lib64 mnt root selinux tftpboot usr[root@dv grub2]# ls /boot
System.map grub vmlinuz
System.map-2.6.26-rc4-wl initrd-2.6.26-rc4-wl.img
vmlinuz-2.6.26-rc4-wl
System.map-2.6.26-rc4-wl.old memtest86+-2.01
vmlinuz-2.6.26-rc4-wl.old
[root@dv grub2]#
Quoted 3 lines
As you can see, "grub-fstest
/dev/sda1ls /boot" fails to find any files. Then I patched grub with your patch. The output of the same command is attached (compressed).
Hi,
Thanks, can you also give me the output of dumpe2fs ?
dumpe2fs /dev/sda1
Signature · 1 line
Bean
Quoted 3 lines · Bean
Thanks, can you also give me the output of dumpe2fs ?
dumpe2fs
/dev/sda1
I'll send it privately, as it won't compress much.
I was trying to find out which change breaks the ext3 support. It turns out grub-fstest starts working if I comment out this line in fs/ext2.c:
log->mapping[num++] = grub_be_to_cpu32 (tag->block);
Alternatively, I could replace it with
log->mapping[num++] = GRUB_FSHELP_JOURNAL_UNUSED_MAPPING;
and grub-fstest would list /boot properly. I added some debug prints, to see which states it goes through, and here's the end of the output:
descriptor
commit
descriptor
commit
descriptor
commit
descriptor
commit
descriptor
commit
descriptor
commit
descriptor
commit
revoke
descriptor
commit
num = 15628, last_num = 15627
grub/ System.map vmlinuz-2.6.26-rc4-wl.old vmlinuz System.map-2.6.26-rc4-wl.old vmlinuz-2.6.26-rc4-wl System.map-2.6.26-rc4-wl initrd-2.6.26-rc4-wl.img memtest86+-2.01
The number of the blocks is quite large, but we are never hitting the "default" case. "revoke" wasn't present in subsequent runs. The numbers (num and last_num) keep growing every time by about 20.
I also tried to see what happens on the fshelp.c side. So I applied this patch:
fs/fshelp.c +8 -2
@@ -329,12 +329,18 @@ grub_fshelp_map_block (grub_fshelp_journal_t log, grub_disk_addr_t block)if (map_block <0 )return block; +grub_printf ("map_block = %ld, start_block = %ld, first_block = %ld, last_block = %d\n" , + map_block, log->start_block, log->first_block, log->last_block); map_block += log->start_block;if (map_block >= log->last_block) map_block -= log->last_block - log->first_block;if (log->type ==GRUB_FSHELP_JOURNAL_TYPE_BLOCK )return log->blkno + map_block; -else -return log->get_block (log->node, map_block); +else { +grub_disk_addr_t ret; + ret = log->get_block (log->node, map_block); +grub_printf ("map_block = %ld, block = %ld, ret = %ld\n" , map_block, block, ret); +return ret; + } }
That's what I get:
# ./grub-fstest /dev/sda1 ls /boot map_block = 18988, start_block = 23124, first_block = 1, last_block = 32768 map_block = 9345, block = 1, ret = 10901 map_block = 18780, start_block = 23124, first_block = 1, last_block = 32768 map_block = 9137, block = 1027, ret = 10692 map_block = 18988, start_block = 23124, first_block = 1, last_block = 32768 map_block = 9345, block = 1, ret = 10901 map_block = 18780, start_block = 23124, first_block = 1, last_block = 32768 map_block = 9137, block = 1027, ret = 10692 map_block = 18988, start_block = 23124, first_block = 1, last_block = 32768 map_block = 9345, block = 1, ret = 10901
But if I change "return ret;" to "return block;" to ignore the lookup, I get this:
# ./grub-fstest /dev/sda1 ls /boot map_block = 19373, start_block = 23124, first_block = 1, last_block = 32768 map_block = 9730, block = 1, ret = 11286 map_block = 19209, start_block = 23124, first_block = 1, last_block = 32768 map_block = 9566, block = 1027, ret = 11122 map_block = 19373, start_block = 23124, first_block = 1, last_block = 32768 map_block = 9730, block = 1, ret = 11286 map_block = 19209, start_block = 23124, first_block = 1, last_block = 32768 map_block = 9566, block = 1027, ret = 11122 map_block = 19373, start_block = 23124, first_block = 1, last_block = 32768 map_block = 9730, block = 1, ret = 11286 map_block = 12763, start_block = 23124, first_block = 1, last_block = 32768 map_block = 3120, block = 15237122, ret = 4670 grub/ System.map vmlinuz-2.6.26-rc4-wl.old vmlinuz System.map-2.6.26-rc4-wl.old vmlinuz-2.6.26-rc4-wl System.map-2.6.26-rc4-wl initrd-2.6.26-rc4-wl.img memtest86+-2.01
One thing that surprises me is that the value we are supposed to return is always below 12000.
Then I created file /boot/foo, ran sync several times, and that's what I get if "block" is returned:
# ./grub-fstest /dev/sda1 ls /boot map_block = 19438, start_block = 23124, first_block = 1, last_block = 32768 map_block = 9795, block = 1, ret = 11351 map_block = 19630, start_block = 23124, first_block = 1, last_block = 32768 map_block = 9987, block = 1027, ret = 11543 map_block = 19438, start_block = 23124, first_block = 1, last_block = 32768 map_block = 9795, block = 1, ret = 11351 map_block = 19630, start_block = 23124, first_block = 1, last_block = 32768 map_block = 9987, block = 1027, ret = 11543 map_block = 19438, start_block = 23124, first_block = 1, last_block = 32768 map_block = 9795, block = 1, ret = 11351 map_block = 19627, start_block = 23124, first_block = 1, last_block = 32768 map_block = 9984, block = 15237122, ret = 11540 map_block = 19628, start_block = 23124, first_block = 1, last_block = 32768 map_block = 9985, block = 15239168, ret = 11541 map_block = 19628, start_block = 23124, first_block = 1, last_block = 32768 map_block = 9985, block = 15239168, ret = 11541 map_block = 19628, start_block = 23124, first_block = 1, last_block = 32768 map_block = 9985, block = 15239168, ret = 11541 map_block = 19628, start_block = 23124, first_block = 1, last_block = 32768 map_block = 9985, block = 15239168, ret = 11541 map_block = 19628, start_block = 23124, first_block = 1, last_block = 32768 map_block = 9985, block = 15239168, ret = 11541 map_block = 19628, start_block = 23124, first_block = 1, last_block = 32768 map_block = 9985, block = 15239168, ret = 11541 grub/ map_block = 19628, start_block = 23124, first_block = 1, last_block = 32768 map_block = 9985, block = 15239168, ret = 11541 map_block = 19628, start_block = 23124, first_block = 1, last_block = 32768 map_block = 9985, block = 15239168, ret = 11541 System.map map_block = 19628, start_block = 23124, first_block = 1, last_block = 32768 map_block = 9985, block = 15239168, ret = 11541 map_block = 19628, start_block = 23124, first_block = 1, last_block = 32768 map_block = 9985, block = 15239168, ret = 11541 vmlinuz-2.6.26-rc4-wl.old map_block = 19628, start_block = 23124, first_block = 1, last_block = 32768 map_block = 9985, block = 15239168, ret = 11541 map_block = 19628, start_block = 23124, first_block = 1, last_block = 32768 map_block = 9985, block = 15239168, ret = 11541 vmlinuz map_block = 19628, start_block = 23124, first_block = 1, last_block = 32768 map_block = 9985, block = 15239168, ret = 11541 map_block = 19628, start_block = 23124, first_block = 1, last_block = 32768 map_block = 9985, block = 15239168, ret = 11541 System.map-2.6.26-rc4-wl.old map_block = 19628, start_block = 23124, first_block = 1, last_block = 32768 map_block = 9985, block = 15239168, ret = 11541 map_block = 19628, start_block = 23124, first_block = 1, last_block = 32768 map_block = 9985, block = 15239168, ret = 11541 vmlinuz-2.6.26-rc4-wl map_block = 19628, start_block = 23124, first_block = 1, last_block = 3276 8 map_block = 9985, block = 15239168, ret = 11541 map_block = 19628, start_block = 23124, first_block = 1, last_block = 32768 map_block = 9985, block = 15239168, ret = 11541 System.map-2.6.26-rc4-wl map_block = 19628, start_block = 23124, first_block = 1, last_block = 3 2768 map_block = 9985, block = 15239168, ret = 11541 map_block = 19628, start_block = 23124, first_block = 1, last_block = 32768 map_block = 9985, block = 15239168, ret = 11541 foo map_block = 19628, start_block = 23124, first_block = 1, last_block = 32768 map_block = 9985, block = 15239168, ret = 11541 map_block = 19628, start_block = 23124, first_block = 1, last_block = 32768 map_block = 9985, block = 15239168, ret = 11541 initrd-2.6.26-rc4-wl.img map_block = 19628, start_block = 23124, first_block = 1, last_block = 3 2768 map_block = 9985, block = 15239168, ret = 11541 map_block = 19628, start_block = 23124, first_block = 1, last_block = 32768 map_block = 9985, block = 15239168, ret = 11541 memtest86+-2.01
But I get this if "ret" is returned:
# ./grub-fstest /dev/sda1 ls /boot map_block = 19438, start_block = 23124, first_block = 1, last_block = 32768 map_block = 9795, block = 1, ret = 11351 map_block = 19656, start_block = 23124, first_block = 1, last_block = 32768 map_block = 10013, block = 1027, ret = 11569 map_block = 19438, start_block = 23124, first_block = 1, last_block = 32768 map_block = 9795, block = 1, ret = 11351 map_block = 19656, start_block = 23124, first_block = 1, last_block = 32768 map_block = 10013, block = 1027, ret = 11569 map_block = 19438, start_block = 23124, first_block = 1, last_block = 32768 map_block = 9795, block = 1, ret = 11351
"ret" is still in the same region.
Signature · 2 lines
Regards, Pavel Roskin
Hi,
It's a little strange, the journal seems normal, I wonder where does it go wrong.
btw, can you use debugfs to gather more information. You can start it with
debugfs /dev/sda1
Then type in the command
logdump -a output_file
Signature · 1 line
Bean
Hi,
Ok, I fix some bugs, please see if the new patch works.
Signature · 1 line
Bean
fs/ext2.c +23 -9
@@ -443,20 +443,25 @@ grub_ext3_get_journal (struct grub_ext2_data *data) log->last_block =grub_be_to_cpu32 (jsb->maxlen); log->start_block =grub_be_to_cpu32 (jsb->start); +grub_printf ("header: %d %d %d\n" , log->first_block, log->last_block, log->start_block); + last_num = num =0 ; block = log->start_block; seq =grub_be_to_cpu32 (jsb->sequence); +grub_printf ("seq: %d\n" , seq);while (1 ) {struct grub_ext3_journal_header *jh; -if (grub_fshelp_read_file (data->disk, &data->logfile,0 , - block << (log2bs +9 ),sizeof (buf), - buf, grub_ext2_read_block, - log->last_block << (log2bs +9 ), - log2bs) != - (int )sizeof (buf)) +grub_printf ("block: %d\n" , block); + +grub_fshelp_read_file (data->disk, &data->logfile,0 , + block << (log2bs +9 ),sizeof (buf), + buf, grub_ext2_read_block, + log->last_block << (log2bs +9 ), + log2bs); +if (grub_errno)break ; jh = (struct grub_ext3_journal_header *) &buf[0 ];
@@ -476,6 +481,7 @@ grub_ext3_get_journal (struct grub_ext2_data *data)struct grub_ext3_journal_block_tag *tag;int ofs, flags; +grub_printf ("dblock: " ); ofs =sizeof (struct grub_ext3_journal_header ); do
@@ -491,15 +497,19 @@ grub_ext3_get_journal (struct grub_ext2_data *data) ofs +=16 ; log->mapping[num++] =grub_be_to_cpu32 (tag->block); +grub_printf ("%d " ,grub_be_to_cpu32 (tag->block));next_block (); }while (! (flags &EXT3_JOURNAL_FLAG_LAST_TAG )); +grub_printf ("\n" ); +continue ; }case EXT3_JOURNAL_COMMIT_BLOCK : { +grub_printf ("cblock: %d\n" , seq); seq++; last_num = num -1 ;continue ;
@@ -508,21 +518,24 @@ grub_ext3_get_journal (struct grub_ext2_data *data)case EXT3_JOURNAL_REVOKE_BLOCK : {struct grub_ext3_journal_revoke_header *jrh; -grub_uint32_t i; +grub_uint32_t i, cnt; jrh = (struct grub_ext3_journal_revoke_header *) jh; + cnt = (grub_be_to_cpu32 (jrh->count) - +sizeof (struct grub_ext3_journal_revoke_header )) >>2 ; -for (i =0 ; i <grub_be_to_cpu32 (jrh->count); i++) +for (i =0 ; i < cnt; i++) {int j;grub_uint32_t map; map =grub_be_to_cpu32 (jrh->data[i]); +grub_printf ("%d " , map);for (j =0 ; j < num; j++)if (log->mapping[j] == map) log->mapping[j] =GRUB_FSHELP_JOURNAL_UNUSED_MAPPING ; } - +grub_printf ("\n" );continue ; }default :
@@ -538,6 +551,7 @@ quit: {int size; +grub_printf ("num: %d\n" , last_num); size =sizeof (struct grub_fshelp_journal ) + last_num *sizeof (grub_disk_addr_t);
Quoted 3 lines · Bean
Hi,
Ok, I fix some bugs, please see if the new patch works.
It doesn't work. i sent the logs privately. I also checked the logs and it looks like that GRUB is reading the log properly. Then I guess we are not using the log correctly.
I changed fs/ext2.c to see how the results would differ if grub_fshelp_map_block() is bypassed:
fs/ext2.c +16 -4
@@ -257,12 +257,24 @@ inline static grub_err_t grub_ext2_blockgroup (struct grub_ext2_data *data, int group, struct grub_ext2_block_group *blkgrp) { - return grub_disk_read (data->disk, - (grub_fshelp_map_block (data->journal, - grub_le_to_cpu32 (data->sblock.first_data_block) + 1) - << LOG2_EXT2_BLOCK_SIZE (data)), + grub_err_t ret; +int block; + + block =grub_le_to_cpu32 (data->sblock.first_data_block) +1 ; + ret =grub_disk_read (data->disk, + block <<LOG2_EXT2_BLOCK_SIZE (data), + group *sizeof (struct grub_ext2_block_group ), +sizeof (struct grub_ext2_block_group ), (char *) blkgrp); +grub_printf ("direct: ret = %d, block = %d, inode_table_id = %d\n" , ret, block, + blkgrp->inode_table_id); + block =grub_fshelp_map_block (data->journal, block); + ret =grub_disk_read (data->disk, + block <<LOG2_EXT2_BLOCK_SIZE (data), group *sizeof (struct grub_ext2_block_group ),sizeof (struct grub_ext2_block_group ), (char *) blkgrp); +grub_printf ("journal: ret = %d, block = %d, inode_table_id = %d\n" , ret, block, + blkgrp->inode_table_id); +return ret; }
That's the output:
direct: ret = 0, block = 1, inode_table_id = 1027 journal: ret = 0, block = 1, inode_table_id = 1027 direct: ret = 0, block = 1, inode_table_id = 1027 journal: ret = 0, block = 17707, inode_table_id = 1027 direct: ret = 0, block = 1, inode_table_id = 1027 journal: ret = 0, block = 1, inode_table_id = 1027 direct: ret = 0, block = 1, inode_table_id = 1027 journal: ret = 0, block = 17707, inode_table_id = 1027 direct: ret = 0, block = 1, inode_table_id = 15237122 journal: ret = 0, block = 17707, inode_table_id = 6895416
So, originally we didn't have the journal opened, so the mapping was trivial, then 1 became mapped to 17707. But at some point the data in that block became different from what it used to be and different in the journaled and non-journaled blocks.
Signature · 2 lines
Regards, Pavel Roskin
Hi,
I spot another bug, please try the new patch.
Signature · 1 line
Bean
fs/ext2.c +32 -12
@@ -257,12 +257,18 @@ inline static grub_err_t grub_ext2_blockgroup (struct grub_ext2_data *data, int group, struct grub_ext2_block_group *blkgrp) { + int blkno, blkoff; + + group *=sizeof (struct grub_ext2_block_group ); + blkno = group >>LOG2_EXT2_BLOCK_SIZE (data); + blkoff = group - (blkno <<LOG2_EXT2_BLOCK_SIZE (data)); +return grub_disk_read (data->disk, (grub_fshelp_map_block (data->journal, - grub_le_to_cpu32 (data->sblock.first_data_block) + 1) +grub_le_to_cpu32 (data->sblock.first_data_block) +1 + blkno) <<LOG2_EXT2_BLOCK_SIZE (data)), - group * sizeof (struct grub_ext2_block_group), - sizeof (struct grub_ext2_block_group), (char *) blkgrp); + blkoff, +sizeof (struct grub_ext2_block_group ), (char *) blkgrp); }
@@ -443,20 +449,25 @@ grub_ext3_get_journal (struct grub_ext2_data *data) log->last_block =grub_be_to_cpu32 (jsb->maxlen); log->start_block =grub_be_to_cpu32 (jsb->start); +grub_printf ("header: %d %d %d\n" , log->first_block, log->last_block, log->start_block); + last_num = num =0 ; block = log->start_block; seq =grub_be_to_cpu32 (jsb->sequence); +grub_printf ("seq: %d\n" , seq);while (1 ) {struct grub_ext3_journal_header *jh; -if (grub_fshelp_read_file (data->disk, &data->logfile,0 , - block << (log2bs +9 ),sizeof (buf), - buf, grub_ext2_read_block, - log->last_block << (log2bs +9 ), - log2bs) != - (int )sizeof (buf)) +grub_printf ("block: %d\n" , block); + +grub_fshelp_read_file (data->disk, &data->logfile,0 , + block << (log2bs +9 ),sizeof (buf), + buf, grub_ext2_read_block, + log->last_block << (log2bs +9 ), + log2bs); +if (grub_errno)break ; jh = (struct grub_ext3_journal_header *) &buf[0 ];
@@ -476,6 +487,7 @@ grub_ext3_get_journal (struct grub_ext2_data *data)struct grub_ext3_journal_block_tag *tag;int ofs, flags; +grub_printf ("dblock: " ); ofs =sizeof (struct grub_ext3_journal_header ); do
@@ -491,15 +503,19 @@ grub_ext3_get_journal (struct grub_ext2_data *data) ofs +=16 ; log->mapping[num++] =grub_be_to_cpu32 (tag->block); +grub_printf ("%d " ,grub_be_to_cpu32 (tag->block));next_block (); }while (! (flags &EXT3_JOURNAL_FLAG_LAST_TAG )); +grub_printf ("\n" ); +continue ; }case EXT3_JOURNAL_COMMIT_BLOCK : { +grub_printf ("cblock: %d\n" , seq); seq++; last_num = num -1 ;continue ;
@@ -508,21 +524,24 @@ grub_ext3_get_journal (struct grub_ext2_data *data)case EXT3_JOURNAL_REVOKE_BLOCK : {struct grub_ext3_journal_revoke_header *jrh; -grub_uint32_t i; +grub_uint32_t i, cnt; jrh = (struct grub_ext3_journal_revoke_header *) jh; + cnt = (grub_be_to_cpu32 (jrh->count) - +sizeof (struct grub_ext3_journal_revoke_header )) >>2 ; -for (i =0 ; i <grub_be_to_cpu32 (jrh->count); i++) +for (i =0 ; i < cnt; i++) {int j;grub_uint32_t map; map =grub_be_to_cpu32 (jrh->data[i]); +grub_printf ("%d " , map);for (j =0 ; j < num; j++)if (log->mapping[j] == map) log->mapping[j] =GRUB_FSHELP_JOURNAL_UNUSED_MAPPING ; } - +grub_printf ("\n" );continue ; }default :
@@ -538,6 +557,7 @@ quit: {int size; +grub_printf ("num: %d\n" , last_num); size =sizeof (struct grub_fshelp_journal ) + last_num *sizeof (grub_disk_addr_t);
Quoted 3 lines · Bean
Hi,
I spot another bug, please try the new patch.
That didn't work. Moreover, grub-fstest failed to read /boot on read-only filesystem.
Thanks to remounting the filesystem and using lzma, I could reduce the compressed logs to a reasonable size, so I'm posting them here together with the script that produced them. lzma should be available in all modern distros.
Signature · 2 lines
Regards, Pavel Roskin
Quoted 3 lines · Bean
Hi,
I spot another bug, please try the new patch.
You probably meant to use (LOG2_EXT2_BLOCK_SIZE (data) + 9) in your patch because the block size is in 512 bytes units, unlike sizeof:
fs/ext2.c +2 -2
@@ -260,8 +260,8 @@ grub_ext2_blockgroup (struct grub_ext2_data *data, int group,int blkno, blkoff; group *=sizeof (struct grub_ext2_block_group ); - blkno = group >>LOG2_EXT2_BLOCK_SIZE (data); - blkoff = group - (blkno <<LOG2_EXT2_BLOCK_SIZE (data)); + blkno = group >> (LOG2_EXT2_BLOCK_SIZE (data) +9 ); + blkoff = group - (blkno << (LOG2_EXT2_BLOCK_SIZE (data) +9 )); return grub_disk_read (data->disk, (grub_fshelp_map_block (data->journal,
That appears to fix everything!!! grub-fstest is loading, GRUB is working in qemu on the live filesystem, and the normal boot is still working.
I think we should consider having a journaling layer above grub_disk_read() that would do the substitution and avoid such bugs in the future.
Signature · 2 lines
Regards, Pavel Roskin
Hello!
Here's the patch with some cleanups and nice wrapped lines. I leaving out other fixes from e3.diff, they should be applied separately and don't appear to be critical for my setup.
Changelog:
* fs/ext2.c (grub_ext2_blockgroup): Call grub_fshelp_map_block() for the block that contains data for the requested group.
fs/ext2.c +10 -4
@@ -257,12 +257,18 @@ inline static grub_err_t grub_ext2_blockgroup (struct grub_ext2_data *data, int group, struct grub_ext2_block_group *blkgrp) { + int blkno, blkoff; + + blkoff = group *sizeof (struct grub_ext2_block_group ); + blkno = blkoff >> (LOG2_EXT2_BLOCK_SIZE (data) +9 ); + blkoff -= (blkno << (LOG2_EXT2_BLOCK_SIZE (data) +9 )); + blkno +=grub_le_to_cpu32 (data->sblock.first_data_block) +1 ; +return grub_disk_read (data->disk, - (grub_fshelp_map_block (data->journal, - grub_le_to_cpu32 (data->sblock.first_data_block) + 1) + (grub_fshelp_map_block (data->journal, blkno) <<LOG2_EXT2_BLOCK_SIZE (data)), - group * sizeof (struct grub_ext2_block_group), - sizeof (struct grub_ext2_block_group), (char *) blkgrp); + blkoff,sizeof (struct grub_ext2_block_group ), + (char *) blkgrp); }
Signature · 2 lines
Regards, Pavel Roskin
Quoted 24 lines · Pavel Roskin
Quoted 3 lines · Bean
Hi,
I spot another bug, please try the new patch.
You probably meant to use (
LOG2_EXT2_BLOCK_SIZE(data) + 9) in your patch because the block size is in 512 bytes units, unlike sizeof:fs/ext2.c +2
-2@@ -260,8 +260,8 @@ grub_ext2_blockgroup (struct grub_ext2_data *data, int group,int blkno, blkoff; group *=sizeof (struct grub_ext2_block_group ); - blkno = group >>LOG2_EXT2_BLOCK_SIZE (data); - blkoff = group - (blkno <<LOG2_EXT2_BLOCK_SIZE (data)); + blkno = group >> (LOG2_EXT2_BLOCK_SIZE (data) +9 ); + blkoff = group - (blkno << (LOG2_EXT2_BLOCK_SIZE (data) +9 )); return grub_disk_read (data->disk, (grub_fshelp_map_block (data->journal,
Quoted 4 lines
That appears to fix everything!!! grub-fstest is loading, GRUB is working in qemu on the live filesystem, and the normal boot is still working.
Oh, nice catch, I make a silly mistake again, :)
Quoted 3 lines
I think we should consider having a journaling layer above
grub_disk_read()that would do the substitution and avoid such bugs in the future.
This should be all right, but we need to move the journal support from fshelp.c to disk.c, anyone objects ?
Signature · 1 line
Bean
Quoting Bean <[email protected]>:
Quoted 6 lines
Quoted 3 lines
I think we should consider having a journaling layer above
grub_disk_read()that would do the substitution and avoid such bugs in the future.This should be all right, but we need to move the journal support from
fshelp.ctodisk.c, anyone objects ?
No objection from me. But if it's complicated, we can start with an ext2 specific function that would translate offset and size to block numbers, apply journal mappings and read the data.
Signature · 2 lines
Regards, Pavel Roskin
Quoting Bean <[email protected]>:
Quoted 2 lines
This should be all right, but we need to move the journal support from
fshelp.ctodisk.c, anyone objects ?
I've converted ext2.c. The patch is attached. The interface should be OK for reiserfs. It looks like the call to grub_fshelp_map_block() from grub_ext2_read_inode() was also wrong. I'm afraid there are some wrong places in reiserfs as well.
I would like to commit the patch as soon as possible. It's a serious issue, and we don't want to scare away potential testers and contributors.
ChangeLog:
* fs/fshelp.c (grub_fshelp_read): New function. Implement linear disk read with journal translation.
* fs/ext2.c: Use grub_fshelp_read() instead of grub_disk_read().
* include/grub/fshelp.h: Declare grub_fshelp_read().
Signature · 2 lines
Regards, Pavel Roskin
fs/ext2.c +19 -27
@@ -257,12 +257,11 @@ inline static grub_err_t grub_ext2_blockgroup (struct grub_ext2_data *data, int group, struct grub_ext2_block_group *blkgrp) { - return grub_disk_read (data->disk, - (grub_fshelp_map_block (data->journal, - grub_le_to_cpu32 (data->sblock.first_data_block) + 1) - << LOG2_EXT2_BLOCK_SIZE (data)), - group * sizeof (struct grub_ext2_block_group), - sizeof (struct grub_ext2_block_group), (char *) blkgrp); + return grub_fshelp_read (data->disk, data->journal, + grub_le_to_cpu32 (data->sblock.first_data_block) + 1, + group * sizeof (struct grub_ext2_block_group), + sizeof (struct grub_ext2_block_group), + (char *) blkgrp, LOG2_EXT2_BLOCK_SIZE (data )); }
@@ -283,11 +282,9 @@ grub_ext2_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock) {grub_uint32_t indir[blksz /4 ]; -if (grub_disk_read (data->disk, -grub_fshelp_map_block (data->journal, -grub_le_to_cpu32 (inode->blocks.indir_block)) - << log2_blksz, -0 , blksz, (char *) indir)) +if (grub_fshelp_read (data->disk, data->journal, +grub_le_to_cpu32 (inode->blocks.indir_block), +0 , blksz, (char *) indir, log2_blksz))return grub_errno; blknr =grub_le_to_cpu32 (indir[fileblock -INDIRECT_BLOCKS ]);
@@ -300,18 +297,14 @@ grub_ext2_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock) + blksz / 4);grub_uint32_t indir[blksz /4 ]; -if (grub_disk_read (data->disk, -grub_fshelp_map_block (data->journal, -grub_le_to_cpu32 (inode->blocks.double_indir_block)) - << log2_blksz, -0 , blksz, (char *) indir)) +if (grub_fshelp_read (data->disk, data->journal, +grub_le_to_cpu32 (inode->blocks.double_indir_block), +0 , blksz, (char *) indir, log2_blksz))return grub_errno; -if (grub_disk_read (data->disk, -grub_fshelp_map_block (data->journal, -grub_le_to_cpu32 (indir[rblock / perblock])) - << log2_blksz, -0 , blksz, (char *) indir)) +if (grub_fshelp_read (data->disk, data->journal, +grub_le_to_cpu32 (indir[rblock / perblock]), +0 , blksz, (char *) indir, log2_blksz))return grub_errno;
@@ -372,12 +365,11 @@ grub_ext2_read_inode (struct grub_ext2_data *data, % inodes_per_block;/* Read the inode. */ -if (grub_disk_read (data->disk, -grub_fshelp_map_block (data->journal, -grub_le_to_cpu32 (blkgrp.inode_table_id) + blkno) - <<LOG2_EXT2_BLOCK_SIZE (data), -EXT2_INODE_SIZE (data) * blkoff, -sizeof (struct grub_ext2_inode ), (char *) inode)) +if (grub_fshelp_read (data->disk, data->journal, +grub_le_to_cpu32 (blkgrp.inode_table_id) + blkno, +EXT2_INODE_SIZE (data) * blkoff, +sizeof (struct grub_ext2_inode ), (char *) inode, +LOG2_EXT2_BLOCK_SIZE (data)))return grub_errno;return 0 ;
fs/fshelp.c +39 -0
@@ -215,6 +215,45 @@ grub_fshelp_find_file (const char *path, grub_fshelp_node_t rootnode, } +/* Read LEN bytes from the block BLOCK on disk DISK into the buffer BUF, +beginning with the block POS. Apply mappings from LOG. The blocks +have a size of LOG2BLOCKSIZE (in log2). */ +grub_err_t grub_fshelp_read (grub_disk_t disk,grub_fshelp_journal_t log, +grub_disk_addr_t block,grub_off_t pos, +grub_size_t len,char *buf,int log2blocksize) +{ +grub_off_t relblk; + + relblk = pos >> (log2blocksize +GRUB_DISK_SECTOR_BITS ); + block += relblk; + pos -= relblk << (log2blocksize +GRUB_DISK_SECTOR_BITS ); + +while (len >0 ) + { +grub_err_t ret; +grub_size_t size; + + size = (GRUB_DISK_SECTOR_SIZE << log2blocksize) - pos; +if (size > len) + size = len; + + ret =grub_disk_read (disk, +grub_fshelp_map_block (log, block) << log2blocksize, + pos, size, buf); + +if (ret) +return ret; + + block++; + pos =0 ; + buf += size; + len -= size; + } + +return 0 ; +} + +/* Read LEN bytes from the file NODE on disk DISK into the buffer BUF, beginning with the block POS. READ_HOOK should be set before reading a block from the file. GET_BLOCK is used to translate file
include/grub/fshelp.h +9 -0
@@ -84,6 +84,15 @@ EXPORT_FUNC(grub_fshelp_find_file) (const char *path, enum grub_fshelp_filetype expect); +/* Read LEN bytes from the block BLOCK on disk DISK into the buffer BUF, +beginning with the block POS. Apply mappings from LOG. The blocks +have a size of LOG2BLOCKSIZE (in log2). */ +grub_err_t +EXPORT_FUNC (grub_fshelp_read ) (grub_disk_t disk,grub_fshelp_journal_t log, +grub_disk_addr_t block,grub_off_t pos, +grub_size_t len,char *buf,int log2blocksize); + +/* Read LEN bytes from the file NODE on disk DISK into the buffer BUF, beginning with the block POS. READ_HOOK should be set before reading a block from the file. GET_BLOCK is used to translate file
Quoted 9 lines · Pavel Roskin
Quoting Bean <[email protected]>:
Quoted 2 lines
This should be all right, but we need to move the journal support from
fshelp.ctodisk.c, anyone objects ?I've converted
ext2.c. The patch is attached. The interface should be OK for reiserfs. It looks like the call togrub_fshelp_map_block()fromgrub_ext2_read_inode()was also wrong. I'm afraid there are some wrong places in reiserfs as well.
The call in grub_ext2_read_inode should be ok, blkoff would limit the read access to within a block.
Quoted 9 lines
I would like to commit the patch as soon as possible. It's a serious issue, and we don't want to scare away potential testers and contributors.
ChangeLog:
*fs/fshelp.c(grub_fshelp_read): New function. Implement linear disk read with journal translation.
*fs/ext2.c: Usegrub_fshelp_read()instead ofgrub_disk_read().
*include/grub/fshelp.h: Declaregrub_fshelp_read().
Seems fine to me. grub_ext3_get_journal still have some issue, but I can fix it after you commit it.
Signature · 1 line
Bean
Quoted 2 lines · Bean
The call in
grub_ext2_read_inodeshould be ok, blkoff would limit the read access to within a block.
You are right. Now that we have grub_fshelp_read(), we don't need that code, so I'm removing it. The block normalization should be done in one place.
Quoted 1 line
Seems fine to me.
Committed.
Quoted 2 lines
grub_ext3_get_journalstill have some issue, but I
can fix it after you commit it.
Please go ahead. And please take care of reiserfs if you can.
Signature · 2 lines
Regards, Pavel Roskin
Quoted 9 lines · Pavel Roskin
Quoted 2 lines · Bean
The call in
grub_ext2_read_inodeshould be ok, blkoff would limit the read access to within a block.You are right. Now that we have
grub_fshelp_read(), we don't need that code, so I'm removing it. The block normalization should be done in one place.
Quoted 8 lines
Quoted 1 line
Seems fine to me.
Committed.
Quoted 2 lines
grub_ext3_get_journalstill have some issue, but I
can fix it after you commit it.Please go ahead. And please take care of reiserfs if you can.
Hi,
After more thoughts, I think the current method still have problem. grub_disk_read reads up to 8192 bytes at a time, which can be larger than the block size. Unless all or none of the sectors in the same block is mapped, we end up reading the wrong sector. So we need to do the mapping inside grub_disk_read.
Signature · 1 line
Bean
Quoting Bean <[email protected]>:
Quoted 3 lines
After more thoughts, I think the current method still have problem.
grub_disk_readreads up to 8192 bytes at a time, which can be larger than the block size.
But we never ask grub_disk_read() to read across the block boundary in grub_fshelp_read(). The code takes care of it. I assume grub_disk_read() would not write more than requested to the buffer.
Quoted 3 lines
Unless all or none of the sectors in the same
block is mapped, we end up reading the wrong sector. So we need to do the mapping insidegrub_disk_read.
But mapping is done for blocks, not for sectors. grub_fshelp_map_block() clearly assumes that for every filesystem.
Signature · 2 lines
Regards, Pavel Roskin
Quoted 9 lines · Pavel Roskin
Quoting Bean <[email protected]>:
Quoted 3 lines
After more thoughts, I think the current method still have problem.
grub_disk_readreads up to 8192 bytes at a time, which can be larger than the block size.But we never ask
grub_disk_read()to read across the block boundary ingrub_fshelp_read(). The code takes care of it. I assumegrub_disk_read()would not write more than requested to the buffer.
grub_disk_read would attempt to read up to GRUB_DISK_CACHE_SIZE and put the result in a cache, then copy the necessary part to output.
Quoted 7 lines
Quoted 4 lines
Unless all or none of the sectors in the same
block is mapped, we end up reading the wrong sector. So we need to do the mapping insidegrub_disk_read.But mapping is done for blocks, not for sectors.
grub_fshelp_map_block()clearly assumes that for every filesystem.
block size is fs related, but most are less than 8192 bytes.
Signature · 1 line
Bean
Quoting Bean <[email protected]>:
Quoted 7 lines · Pavel Roskin
Quoted 6 lines
Quoting Bean <[email protected]>:
Quoted 3 lines
After more thoughts, I think the current method still have problem.
grub_disk_readreads up to 8192 bytes at a time, which can be larger than the block size.
Quoted 6 lines
Quoted 3 lines
But we never ask
grub_disk_read()to read across the block boundary ingrub_fshelp_read(). The code takes care of it. I assumegrub_disk_read()would not write more than requested to the buffer.
grub_disk_readwould attempt to read up toGRUB_DISK_CACHE_SIZEand put the result in a cache, then copy the necessary part to output.
Then I don't see any problem with it. Maybe you could give an example?
Quoted 8 lines
Quoted 6 lines
Quoted 3 lines
Unless all or none of the sectors in the same
block is mapped, we end up reading the wrong sector. So we need to do the mapping insidegrub_disk_read.But mapping is done for blocks, not for sectors.
grub_fshelp_map_block()clearly assumes that for every filesystem.block size is fs related, but most are less than 8192 bytes.
I don't understand why caching would have any effect. It should be transparent for higher layers, like caching inside the hard drive. If caching is not transparent, then it's broken.
Signature · 2 lines
Regards, Pavel Roskin
Quoted 24 lines · Pavel Roskin
Quoting Bean <[email protected]>:
Quoted 8 lines · Pavel Roskin
Quoted 7 lines
Quoting Bean <[email protected]>:
Quoted 3 lines
After more thoughts, I think the current method still have problem.
grub_disk_readreads up to 8192 bytes at a time, which can be larger than the block size.Quoted 7 lines
Quoted 4 lines
But we never ask
grub_disk_read()to read across the block boundary ingrub_fshelp_read(). The code takes care of it. I assumegrub_disk_read()
would not write more than requested to the buffer.
grub_disk_readwould attempt to read up toGRUB_DISK_CACHE_SIZEand put the result in a cache, then copy the necessary part to output.Then I don't see any problem with it. Maybe you could give an example?
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
Unless all or none of the sectors in the same
block is mapped, we end up reading the wrong sector. So we need to do the mapping insidegrub_disk_read.
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
But mapping is done for blocks, not for sectors.
grub_fshelp_map_block()clearly assumes that for every filesystem.block size is fs related, but most are less than 8192 bytes.
I don't understand why caching would have any effect. It should be transparent for higher layers, like caching inside the hard drive. If caching is not transparent, then it's broken.
Hi,
I'm sorry I got a little confused. Your method does work, no need to worry about it now.
Signature · 1 line
Bean
Quoted 4 lines · Pavel Roskin
It may be something else. It was an unclean reboot after a panic for unrelated reasons, so the hanging problem may be an issue with ext3. Anyway, extreme caution is required with the current code!
btw, any chance your cpu is big endian ? The code might have portable issue in big endian machine.
Signature · 1 line
Bean
Quoted 8 lines · Bean
Quoted 4 lines · Pavel Roskin
It may be something else. It was an unclean reboot after a panic for unrelated reasons, so the hanging problem may be an issue with ext3. Anyway, extreme caution is required with the current code!
btw, any chance your cpu is big endian ? The code might have portable issue in big endian machine.
The test machine is i386. The problems I'm seeing in qemu are observed on x86_64.
Signature · 2 lines
Regards, Pavel Roskin