Some concern about the journal support

Some concern about the journal support

[Bean]

Hi,

I think we need to disable journal sometimes. Tools like grub-setup
and grub-install is run in an active system, that means sectors could
easily end up in the journal. However, journal is a temperately
buffer, space can be reused after a while. In this case, we should
bypass the journal and access the underlying file system directly.
Perhaps we can use a variable like no_journal to control the journal
support, any suggestions ?

-- 
Bean

Re: Some concern about the journal support

[Bean]

On Fri, Jun 13, 2008 at 5:05 PM, Bean <bean123ch@gmail.com> wrote:
> Hi,
>
> I think we need to disable journal sometimes. Tools like grub-setup
> and grub-install is run in an active system, that means sectors could
> easily end up in the journal. However, journal is a temperately
> buffer, space can be reused after a while. In this case, we should
> bypass the journal and access the underlying file system directly.
> Perhaps we can use a variable like no_journal to control the journal
> support, any suggestions ?

More thoughts about this issue. I think the problem is in the
read_hook. If we're just reading, it make no difference whether it's
from journal or real location, however, if the read_hook is not empty,
we also need to pass the sector location to upper layer. Perhaps we
can disable journal when the read hook is not null ?

-- 
Bean

Re: Some concern about the journal support

[Bean]

[-- Attachment #1: Type: text/plain, Size: 991 bytes --]

On Fri, Jun 13, 2008 at 6:01 PM, Bean <bean123ch@gmail.com> wrote:
> On Fri, Jun 13, 2008 at 5:05 PM, Bean <bean123ch@gmail.com> wrote:
>> Hi,
>>
>> I think we need to disable journal sometimes. Tools like grub-setup
>> and grub-install is run in an active system, that means sectors could
>> easily end up in the journal. However, journal is a temperately
>> buffer, space can be reused after a while. In this case, we should
>> bypass the journal and access the underlying file system directly.
>> Perhaps we can use a variable like no_journal to control the journal
>> support, any suggestions ?
>
> More thoughts about this issue. I think the problem is in the
> read_hook. If we're just reading, it make no difference whether it's
> from journal or real location, however, if the read_hook is not empty,
> we also need to pass the sector location to upper layer. Perhaps we
> can disable journal when the read hook is not null ?

Hi,

This patch should fix the above problem.

-- 
Bean

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: e1.diff --]
[-- Type: text/x-diff; name=e1.diff, Size: 3385 bytes --]

diff --git a/fs/ext2.c b/fs/ext2.c
index 184b973..0b96909 100644
--- a/fs/ext2.c
+++ b/fs/ext2.c
@@ -318,7 +318,7 @@ grub_ext2_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
       blknr = -1;
     }
 
-  return grub_fshelp_map_block (data->journal, blknr);
+  return blknr;
 }
 
 /* Read LEN bytes from the file described by DATA starting with byte
@@ -329,7 +329,8 @@ grub_ext2_read_file (grub_fshelp_node_t node,
 					unsigned offset, unsigned length),
 		     int pos, grub_size_t len, char *buf)
 {
-  return grub_fshelp_read_file (node->data->disk, node, read_hook,
+  return grub_fshelp_read_jour (node->data->journal,
+				node->data->disk, node, read_hook,
 				pos, len, buf, grub_ext2_read_block,
 				node->inode.size,
 				LOG2_EXT2_BLOCK_SIZE (node->data));
diff --git a/fs/fshelp.c b/fs/fshelp.c
index 2fae065..23a019e 100644
--- a/fs/fshelp.c
+++ b/fs/fshelp.c
@@ -269,6 +269,21 @@ grub_fshelp_read_file (grub_disk_t disk, grub_fshelp_node_t node,
                                                       grub_disk_addr_t block),
 		       grub_off_t filesize, int log2blocksize)
 {
+  return grub_fshelp_read_jour (0, disk, node, read_hook, pos, len, buf, get_block,
+                                filesize, log2blocksize);
+}
+
+grub_ssize_t
+grub_fshelp_read_jour (grub_fshelp_journal_t log,
+		       grub_disk_t disk, grub_fshelp_node_t node,
+		       void NESTED_FUNC_ATTR (*read_hook) (grub_disk_addr_t sector,
+                                                           unsigned offset,
+                                                           unsigned length),
+		       grub_off_t pos, grub_size_t len, char *buf,
+		       grub_disk_addr_t (*get_block) (grub_fshelp_node_t node,
+                                                      grub_disk_addr_t block),
+		       grub_off_t filesize, int log2blocksize)
+{
   grub_disk_addr_t i, blockcnt;
   int blocksize = 1 << (log2blocksize + GRUB_DISK_SECTOR_BITS);
 
@@ -314,6 +329,8 @@ grub_fshelp_read_file (grub_disk_t disk, grub_fshelp_node_t node,
       if (blknr)
 	{
 	  disk->read_hook = read_hook;	  
+	  if (! read_hook)
+	    blknr = grub_fshelp_map_block (log, blknr);
 
 	  grub_disk_read (disk, blknr, skipfirst,
 			  blockend, buf);
diff --git a/include/grub/fshelp.h b/include/grub/fshelp.h
index 847f78e..da70e4e 100644
--- a/include/grub/fshelp.h
+++ b/include/grub/fshelp.h
@@ -108,6 +108,17 @@ EXPORT_FUNC(grub_fshelp_read_file) (grub_disk_t disk, grub_fshelp_node_t node,
                                                                    grub_disk_addr_t block),
 				    grub_off_t filesize, int log2blocksize);
 
+grub_ssize_t
+EXPORT_FUNC(grub_fshelp_read_jour) (grub_fshelp_journal_t log,
+				    grub_disk_t disk, grub_fshelp_node_t node,
+				    void NESTED_FUNC_ATTR (*read_hook) (grub_disk_addr_t sector,
+                                                                        unsigned offset,
+                                                                        unsigned length),
+				    grub_off_t pos, grub_size_t len, char *buf,
+				    grub_disk_addr_t (*get_block) (grub_fshelp_node_t node,
+                                                                   grub_disk_addr_t block),
+				    grub_off_t filesize, int log2blocksize);
+
 unsigned int
 EXPORT_FUNC(grub_fshelp_log2blksize) (unsigned int blksize,
 				      unsigned int *pow);

Re: Some concern about the journal support

[Pavel Roskin]

On Fri, 2008-06-13 at 17:05 +0800, Bean wrote:
> Hi,
> 
> I think we need to disable journal sometimes. Tools like grub-setup
> and grub-install is run in an active system, that means sectors could
> easily end up in the journal. However, journal is a temperately
> buffer, space can be reused after a while. In this case, we should
> bypass the journal and access the underlying file system directly.
> Perhaps we can use a variable like no_journal to control the journal
> support, any suggestions ?

If we are going to hardcode block locations somewhere, hardcoding a
journal location is a serious bug.  It will be overwritten.

If we are just reading from a live filesystem, there is no 100% correct
solution, but avoiding the journal seems safer to me.  We load the
mappings once, but we read from the journal when the need arises.  The
journal can be overwritten by background activity that the administrator
doesn't control.  If we ignore the journal, inconsistencies would
normally arise only if any files used by grub are modified in the
meantime.  Those should be owned by root, and no reasonable
administrator would touch them while grub-install is running.

We still want journal support for testing purposes, so perhaps
grub-fstest should have a switch to use the journal.

-- 
Regards,
Pavel Roskin

Re: Some concern about the journal support

[Bean]

On Fri, Jun 13, 2008 at 11:55 PM, Pavel Roskin <proski@gnu.org> wrote:
> On Fri, 2008-06-13 at 17:05 +0800, Bean wrote:
>> Hi,
>>
>> I think we need to disable journal sometimes. Tools like grub-setup
>> and grub-install is run in an active system, that means sectors could
>> easily end up in the journal. However, journal is a temperately
>> buffer, space can be reused after a while. In this case, we should
>> bypass the journal and access the underlying file system directly.
>> Perhaps we can use a variable like no_journal to control the journal
>> support, any suggestions ?
>
> If we are going to hardcode block locations somewhere, hardcoding a
> journal location is a serious bug.  It will be overwritten.
>
> If we are just reading from a live filesystem, there is no 100% correct
> solution, but avoiding the journal seems safer to me.  We load the
> mappings once, but we read from the journal when the need arises.  The
> journal can be overwritten by background activity that the administrator
> doesn't control.  If we ignore the journal, inconsistencies would
> normally arise only if any files used by grub are modified in the
> meantime.  Those should be owned by root, and no reasonable
> administrator would touch them while grub-install is running.
>
> We still want journal support for testing purposes, so perhaps
> grub-fstest should have a switch to use the journal.

Please see if my last patch works. Basically, it use read_hook as an
indicator. If read_hook is not null, which means we need to get a
block location, it use the fs address. If read_hook is null, which
means we don't care about the location, it use the journal address.
This should fit the problem.

btw, the reiserfs driver is a little strange, it don't follow the
normal path of grub_fshelp_read_file, perhaps we just disable the
journal at the moment.

-- 
Bean

Re: Some concern about the journal support

[Isaac Dupree]

can you send a message when GRUB2 CVS is considered no longer broken for 
use with ext3? (so I'll know when is a good time for me to install a 
newer version -- since I'm not in a hurry -- or is it a better bet 
stability-wise within the next month, to go with a revision from May 
instead?)

-Isaac

Re: Some concern about the journal support

[Pavel Roskin]

On Fri, 2008-06-13 at 15:45 -0400, Isaac Dupree wrote:
> can you send a message when GRUB2 CVS is considered no longer broken for 
> use with ext3? (so I'll know when is a good time for me to install a 
> newer version -- since I'm not in a hurry -- or is it a better bet 
> stability-wise within the next month, to go with a revision from May 
> instead?)

I'm afraid it's not going to work this way.  I can imagine that some
fixes will be applied, but it's also possible that they will be
incomplete.  That would require further fixes.  I don't think anybody
would want to be in the position to announce that ext3 is fixed and then
announce the opposite, possibly within hours.

The true stability is proven by testing and time.

-- 
Regards,
Pavel Roskin

Re: Some concern about the journal support

[Isaac Dupree]

Pavel Roskin wrote:
> On Fri, 2008-06-13 at 15:45 -0400, Isaac Dupree wrote:
>> can you send a message when GRUB2 CVS is considered no longer broken for 
>> use with ext3? (so I'll know when is a good time for me to install a 
>> newer version -- since I'm not in a hurry -- or is it a better bet 
>> stability-wise within the next month, to go with a revision from May 
>> instead?)
> 
> I'm afraid it's not going to work this way.  I can imagine that some
> fixes will be applied, but it's also possible that they will be
> incomplete.

true enough: grub's ext3 support is relatively unstable for now, so if I 
use it I should be prepared to report problems and help debug them. 
Partly I just get confused (although patches are frequently discussed on 
the list) which patches have actually been applied to CVS, and whether 
any of the *known* issues are still known to be unfixed in CVS. (That 
is, I'm usually willing to be a tester, but it's generally not useful to 
anyone to test things that are known to be broken)

-Isaac

Re: Some concern about the journal support

[Pavel Roskin]

On Fri, 2008-06-13 at 17:49 -0400, Isaac Dupree wrote:

> true enough: grub's ext3 support is relatively unstable for now, so if I 
> use it I should be prepared to report problems and help debug them. 
> Partly I just get confused (although patches are frequently discussed on 
> the list) which patches have actually been applied to CVS, and whether 
> any of the *known* issues are still known to be unfixed in CVS. (That 
> is, I'm usually willing to be a tester, but it's generally not useful to 
> anyone to test things that are known to be broken)

You may have two kinds of problems.

If /boot/grub/core.img cannot be embedded in the first sectors of the
drive and /boot/grub is on ext3, GRUB is unlikely to boot.  There is a
patch that is likely to fix it, but it wasn't applied.

If /boot is on ext3 partition that wasn't cleanly unmounted, GRUB may
fail to boot.  This is less likely than it used to be, but it's still
possible.  It looks like some corner cases still need fixing.

If /boot is on ext2 (i.e. there is no journal), no problems are
expected.

There are still things that need to be tested.  You could try to create
a small disk image that would exhibit problems with accessing the
filesystem.  Or you could check that the last patch by Bean would work
if core.img is not embedded and the filesystem is heavily used (e.g.
more that the journal size of data is written to the filesystem).  Or if
you don't like ext3 filesystem testing, put /boot on some other
filesystem (I'm quite sure that reiserfs is broken, but the rest should
be OK).

-- 
Regards,
Pavel Roskin

Re: Some concern about the journal support

[Bean]

On Sat, Jun 14, 2008 at 4:40 AM, Pavel Roskin <proski@gnu.org> wrote:
> On Fri, 2008-06-13 at 15:45 -0400, Isaac Dupree wrote:
>> can you send a message when GRUB2 CVS is considered no longer broken for
>> use with ext3? (so I'll know when is a good time for me to install a
>> newer version -- since I'm not in a hurry -- or is it a better bet
>> stability-wise within the next month, to go with a revision from May
>> instead?)
>
> I'm afraid it's not going to work this way.  I can imagine that some
> fixes will be applied, but it's also possible that they will be
> incomplete.  That would require further fixes.  I don't think anybody
> would want to be in the position to announce that ext3 is fixed and then
> announce the opposite, possibly within hours.
>
> The true stability is proven by testing and time.

Hi,

You're right, the journal code is not quite stable right now. Perhaps
we can use variable journal to control it. By default, it's not set so
that journal is disabled. After all, even if the fs is uncleaned, it
can still be bootable. If automatic booting fails, users can set
journal=1 and retry, if they're lucky, it could work without using fs
recover tools.

-- 
Bean

Re: Some concern about the journal support

[Bean]

[-- Attachment #1: Type: text/plain, Size: 1319 bytes --]

On Sat, Jun 14, 2008 at 11:32 AM, Bean <bean123ch@gmail.com> wrote:
> On Sat, Jun 14, 2008 at 4:40 AM, Pavel Roskin <proski@gnu.org> wrote:
>> On Fri, 2008-06-13 at 15:45 -0400, Isaac Dupree wrote:
>>> can you send a message when GRUB2 CVS is considered no longer broken for
>>> use with ext3? (so I'll know when is a good time for me to install a
>>> newer version -- since I'm not in a hurry -- or is it a better bet
>>> stability-wise within the next month, to go with a revision from May
>>> instead?)
>>
>> I'm afraid it's not going to work this way.  I can imagine that some
>> fixes will be applied, but it's also possible that they will be
>> incomplete.  That would require further fixes.  I don't think anybody
>> would want to be in the position to announce that ext3 is fixed and then
>> announce the opposite, possibly within hours.
>>
>> The true stability is proven by testing and time.
>
> Hi,
>
> You're right, the journal code is not quite stable right now. Perhaps
> we can use variable journal to control it. By default, it's not set so
> that journal is disabled. After all, even if the fs is uncleaned, it
> can still be bootable. If automatic booting fails, users can set
> journal=1 and retry, if they're lucky, it could work without using fs
> recover tools.

Hi,

This is the patch.

-- 
Bean

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: e2.diff --]
[-- Type: text/x-diff; name=e2.diff, Size: 6465 bytes --]

diff --git a/fs/ext2.c b/fs/ext2.c
index 184b973..e2b1a6f 100644
--- a/fs/ext2.c
+++ b/fs/ext2.c
@@ -50,6 +50,7 @@
 #include <grub/dl.h>
 #include <grub/types.h>
 #include <grub/fshelp.h>
+#include <grub/env.h>
 
 /* Log2 size of ext2 block in 512 blocks.  */
 #define LOG2_EXT2_BLOCK_SIZE(data)			\
@@ -318,7 +319,7 @@ grub_ext2_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
       blknr = -1;
     }
 
-  return grub_fshelp_map_block (data->journal, blknr);
+  return blknr;
 }
 
 /* Read LEN bytes from the file described by DATA starting with byte
@@ -329,7 +330,8 @@ grub_ext2_read_file (grub_fshelp_node_t node,
 					unsigned offset, unsigned length),
 		     int pos, grub_size_t len, char *buf)
 {
-  return grub_fshelp_read_file (node->data->disk, node, read_hook,
+  return grub_fshelp_read_jour (node->data->journal,
+				node->data->disk, node, read_hook,
 				pos, len, buf, grub_ext2_read_block,
 				node->inode.size,
 				LOG2_EXT2_BLOCK_SIZE (node->data));
@@ -385,8 +387,6 @@ grub_ext3_get_journal (struct grub_ext2_data *data)
         block = log->first_block;
     }
 
-  data->journal = 0;
-
   if (! (data->sblock.feature_compatibility & EXT3_FEATURE_COMPAT_HAS_JOURNAL))
     return;
 
@@ -553,7 +553,10 @@ grub_ext2_mount (grub_disk_t disk)
     goto fail;
   
   data->disk = disk;
-  grub_ext3_get_journal (data);
+  data->journal = 0;
+
+  if (grub_env_get ("journal"))
+    grub_ext3_get_journal (data);
 
   data->diropen.data = data;
   data->diropen.ino = 2;
diff --git a/fs/fshelp.c b/fs/fshelp.c
index 2fae065..23a019e 100644
--- a/fs/fshelp.c
+++ b/fs/fshelp.c
@@ -269,6 +269,21 @@ grub_fshelp_read_file (grub_disk_t disk, grub_fshelp_node_t node,
                                                       grub_disk_addr_t block),
 		       grub_off_t filesize, int log2blocksize)
 {
+  return grub_fshelp_read_jour (0, disk, node, read_hook, pos, len, buf, get_block,
+                                filesize, log2blocksize);
+}
+
+grub_ssize_t
+grub_fshelp_read_jour (grub_fshelp_journal_t log,
+		       grub_disk_t disk, grub_fshelp_node_t node,
+		       void NESTED_FUNC_ATTR (*read_hook) (grub_disk_addr_t sector,
+                                                           unsigned offset,
+                                                           unsigned length),
+		       grub_off_t pos, grub_size_t len, char *buf,
+		       grub_disk_addr_t (*get_block) (grub_fshelp_node_t node,
+                                                      grub_disk_addr_t block),
+		       grub_off_t filesize, int log2blocksize)
+{
   grub_disk_addr_t i, blockcnt;
   int blocksize = 1 << (log2blocksize + GRUB_DISK_SECTOR_BITS);
 
@@ -314,6 +329,8 @@ grub_fshelp_read_file (grub_disk_t disk, grub_fshelp_node_t node,
       if (blknr)
 	{
 	  disk->read_hook = read_hook;	  
+	  if (! read_hook)
+	    blknr = grub_fshelp_map_block (log, blknr);
 
 	  grub_disk_read (disk, blknr, skipfirst,
 			  blockend, buf);
diff --git a/fs/reiserfs.c b/fs/reiserfs.c
index 9a059e3..94754aa 100644
--- a/fs/reiserfs.c
+++ b/fs/reiserfs.c
@@ -39,6 +39,7 @@
 #include <grub/dl.h>
 #include <grub/types.h>
 #include <grub/fshelp.h>
+#include <grub/env.h>
 
 #define MIN(a, b) \
   ({ typeof (a) _a = (a); \
@@ -694,8 +695,6 @@ grub_reiserfs_get_journal (struct grub_reiserfs_data *data)
   int base_block = grub_le_to_cpu32 (data->superblock.journal_block);
   int last_num, num, block;
 
-  data->journal = 0;
-
   if (! data->superblock.journal_block)
     return;
 
@@ -819,7 +818,11 @@ grub_reiserfs_mount (grub_disk_t disk)
       goto fail;
     }
   data->disk = disk;
-  grub_reiserfs_get_journal (data);
+  data->journal = 0;
+
+  if (grub_env_get ("journal"))
+    grub_reiserfs_get_journal (data);
+
   return data;
 
  fail:
diff --git a/include/grub/fshelp.h b/include/grub/fshelp.h
index 847f78e..da70e4e 100644
--- a/include/grub/fshelp.h
+++ b/include/grub/fshelp.h
@@ -108,6 +108,17 @@ EXPORT_FUNC(grub_fshelp_read_file) (grub_disk_t disk, grub_fshelp_node_t node,
                                                                    grub_disk_addr_t block),
 				    grub_off_t filesize, int log2blocksize);
 
+grub_ssize_t
+EXPORT_FUNC(grub_fshelp_read_jour) (grub_fshelp_journal_t log,
+				    grub_disk_t disk, grub_fshelp_node_t node,
+				    void NESTED_FUNC_ATTR (*read_hook) (grub_disk_addr_t sector,
+                                                                        unsigned offset,
+                                                                        unsigned length),
+				    grub_off_t pos, grub_size_t len, char *buf,
+				    grub_disk_addr_t (*get_block) (grub_fshelp_node_t node,
+                                                                   grub_disk_addr_t block),
+				    grub_off_t filesize, int log2blocksize);
+
 unsigned int
 EXPORT_FUNC(grub_fshelp_log2blksize) (unsigned int blksize,
 				      unsigned int *pow);
diff --git a/util/grub-fstest.c b/util/grub-fstest.c
index 3872ff1..e21fbdb 100644
--- a/util/grub-fstest.c
+++ b/util/grub-fstest.c
@@ -331,6 +331,7 @@ static struct option options[] = {
   {"length", required_argument, 0, 'n'},
   {"debug", required_argument, 0, 'd'},
   {"raw", no_argument, 0, 'r'},
+  {"journal", no_argument, 0, 'j'},
   {"long", no_argument, 0, 'l'},
   {"help", no_argument, 0, 'h'},
   {"version", no_argument, 0, 'V'},
@@ -361,6 +362,7 @@ Debug tool for filesystem driver.\n\
   -n, --length=N            handle N bytes in output file\n\
   -d, --debug=S             Set debug environment variable\n\
   -r, --raw                 disable auto decompression\n\
+  -j, --journal             enable journal support\n\
   -l, --long                show long directory list\n\
   -h, --help                display this message and exit\n\
   -V, --version             print version information and exit\n\
@@ -375,7 +377,7 @@ int
 main (int argc, char *argv[])
 {
   char *image_path, *debug_str = 0;
-  int cmd, is_raw = 0, is_long = 0;
+  int cmd, is_raw = 0, use_journal = 0, is_long = 0;
 
   progname = "grub-fstest";
 
@@ -409,6 +411,10 @@ main (int argc, char *argv[])
 	    is_raw = 1;
 	    break;
 
+	  case 'j':
+	    use_journal = 1;
+	    break;
+
 	  case 'l':
 	    is_long = 1;
 	    break;
@@ -506,6 +512,9 @@ main (int argc, char *argv[])
   if (is_raw)
     grub_env_set ("filehook", "0");
 
+  if (use_journal)
+    grub_env_set ("journal", "1");
+
   if (debug_str)
     grub_env_set ("debug", debug_str);
 

Re: Some concern about the journal support

[Pavel Roskin]

On Sat, 2008-06-14 at 01:05 +0800, Bean wrote:

> Please see if my last patch works. Basically, it use read_hook as an
> indicator. If read_hook is not null, which means we need to get a
> block location, it use the fs address. If read_hook is null, which
> means we don't care about the location, it use the journal address.
> This should fit the problem.

My testing shows that we still have some problems with journal support.
I could not read /home in grub-fstest, and it didn't work when I
reverted your patch.  It worked with the version immediately before you
added journal support on May 20, but didn't work with the version where
the journal support was added.  After I returned to the current code, I
could read /home again - apparently something changed on the filesystem
in the meantime.

Your patch should fix the issue with hardcoding block locations, if I
understand correctly.

I was asking where journal support would be beneficial in userspace at
all.  And it has to be asked if journal support is useful at the boot
time.

We have some code that is hard to get right and hard to test.  Yet it
will be run every time an unclean ext3 filesystem is found at the boot
time.  What are we gaining?  What is the situation where using the
journal is beneficial?  How likely is it to happen?  Is it possible that
we may be better off not using the journal?  How likely is that?

The standard use of the journal is to make the filesystem consistent
after an unclean shutdown without having to run a time-consuming fsck.
Since grub is not writing anything to the disk, consistency is not
really important.  What's important got grub is reliability and ability
to access all files on the filesystem.

> btw, the reiserfs driver is a little strange, it don't follow the
> normal path of grub_fshelp_read_file, perhaps we just disable the
> journal at the moment.

My impression is that we need to make journal support experimental and
disabled by default for both ext3 and reiserfs.  It should only be
enabled by default if there are a good arguments why it's useful and a
testsuite to prove that it's reliable.

-- 
Regards,
Pavel Roskin

Re: Some concern about the journal support

[Robert Millan]

On Fri, Jun 13, 2008 at 04:14:17PM -0400, Pavel Roskin wrote:
> 
> Your patch should fix the issue with hardcoding block locations, if I
> understand correctly.

Notice that hardcoding block locations only happens when core.img doesn't
fit in the post-mbr area.

Which is a situation we should try to avoid by making core.img small, which
means a smaller ext2.mod also helps ;-)

> I was asking where journal support would be beneficial in userspace at
> all.  And it has to be asked if journal support is useful at the boot
> time.
> 
> We have some code that is hard to get right and hard to test.  Yet it
> will be run every time an unclean ext3 filesystem is found at the boot
> time.  What are we gaining?  What is the situation where using the
> journal is beneficial?  How likely is it to happen?  Is it possible that
> we may be better off not using the journal?  How likely is that?
> 
> The standard use of the journal is to make the filesystem consistent
> after an unclean shutdown without having to run a time-consuming fsck.
> Since grub is not writing anything to the disk, consistency is not
> really important.  What's important got grub is reliability and ability
> to access all files on the filesystem.
> 
> > btw, the reiserfs driver is a little strange, it don't follow the
> > normal path of grub_fshelp_read_file, perhaps we just disable the
> > journal at the moment.
> 
> My impression is that we need to make journal support experimental and
> disabled by default for both ext3 and reiserfs.  It should only be
> enabled by default if there are a good arguments why it's useful and a
> testsuite to prove that it's reliable.

How about a separate module?

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

Re: Some concern about the journal support

[Bean]

On Sat, Jun 14, 2008 at 7:43 PM, Robert Millan <rmh@aybabtu.com> wrote:
> On Fri, Jun 13, 2008 at 04:14:17PM -0400, Pavel Roskin wrote:
>>
>> Your patch should fix the issue with hardcoding block locations, if I
>> understand correctly.
>
> Notice that hardcoding block locations only happens when core.img doesn't
> fit in the post-mbr area.
>
> Which is a situation we should try to avoid by making core.img small, which
> means a smaller ext2.mod also helps ;-)
>
>> I was asking where journal support would be beneficial in userspace at
>> all.  And it has to be asked if journal support is useful at the boot
>> time.
>>
>> We have some code that is hard to get right and hard to test.  Yet it
>> will be run every time an unclean ext3 filesystem is found at the boot
>> time.  What are we gaining?  What is the situation where using the
>> journal is beneficial?  How likely is it to happen?  Is it possible that
>> we may be better off not using the journal?  How likely is that?
>>
>> The standard use of the journal is to make the filesystem consistent
>> after an unclean shutdown without having to run a time-consuming fsck.
>> Since grub is not writing anything to the disk, consistency is not
>> really important.  What's important got grub is reliability and ability
>> to access all files on the filesystem.
>>
>> > btw, the reiserfs driver is a little strange, it don't follow the
>> > normal path of grub_fshelp_read_file, perhaps we just disable the
>> > journal at the moment.
>>
>> My impression is that we need to make journal support experimental and
>> disabled by default for both ext3 and reiserfs.  It should only be
>> enabled by default if there are a good arguments why it's useful and a
>> testsuite to prove that it's reliable.
>
> How about a separate module?

Hi,

It's a good idea, perhaps I can add a journal layer on top of disk to
do the mapping transparently.

-- 
Bean

Re: Some concern about the journal support

[Bean]

[-- Attachment #1: Type: text/plain, Size: 356 bytes --]

>> How about a separate module?
>
> Hi,
>
> It's a good idea, perhaps I can add a journal layer on top of disk to
> do the mapping transparently.
>
> --
> Bean
>

Hi,

This patch revert ext2 and reiserfs driver to pre journal state. The
new journal support would be in a separate module and provide a
transparent mapping layer for the fs driver.

-- 
Bean

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: j1.diff --]
[-- Type: text/x-diff; name=j1.diff, Size: 24182 bytes --]

diff --git a/fs/ext2.c b/fs/ext2.c
index 184b973..8e35f8d 100644
--- a/fs/ext2.c
+++ b/fs/ext2.c
@@ -241,8 +241,6 @@ struct grub_ext2_data
   grub_disk_t disk;
   struct grub_ext2_inode *inode;
   struct grub_fshelp_node diropen;
-  struct grub_fshelp_node logfile;
-  grub_fshelp_journal_t journal;
 };
 
 #ifndef GRUB_UTIL
@@ -257,11 +255,11 @@ inline static grub_err_t
 grub_ext2_blockgroup (struct grub_ext2_data *data, int group, 
 		      struct grub_ext2_block_group *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));
+  return grub_disk_read (data->disk,
+                         ((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);
 }
 
 
@@ -282,9 +280,10 @@ grub_ext2_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
     {
       grub_uint32_t indir[blksz / 4];
 
-      if (grub_fshelp_read (data->disk, data->journal,
-			    grub_le_to_cpu32 (inode->blocks.indir_block),
-			    0, blksz, (char *) indir, log2_blksz))
+      if (grub_disk_read (data->disk,
+			  grub_le_to_cpu32 (inode->blocks.indir_block)
+			  << log2_blksz,
+			  0, blksz, (char *) indir))
 	return grub_errno;
 	  
       blknr = grub_le_to_cpu32 (indir[fileblock - INDIRECT_BLOCKS]);
@@ -297,14 +296,16 @@ grub_ext2_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
 					 + blksz / 4);
       grub_uint32_t indir[blksz / 4];
 
-      if (grub_fshelp_read (data->disk, data->journal,
-			    grub_le_to_cpu32 (inode->blocks.double_indir_block),
-			    0, blksz, (char *) indir, log2_blksz))
+      if (grub_disk_read (data->disk,
+			  grub_le_to_cpu32 (inode->blocks.double_indir_block)
+			  << log2_blksz,
+			  0, blksz, (char *) indir))
 	return grub_errno;
 
-      if (grub_fshelp_read (data->disk, data->journal,
-			    grub_le_to_cpu32 (indir[rblock / perblock]),
-			    0, blksz, (char *) indir, log2_blksz))
+      if (grub_disk_read (data->disk,
+			  grub_le_to_cpu32 (indir[rblock / perblock])
+			  << log2_blksz,
+			  0, blksz, (char *) indir))
 	return grub_errno;
 
       
@@ -318,7 +319,7 @@ grub_ext2_read_block (grub_fshelp_node_t node, grub_disk_addr_t fileblock)
       blknr = -1;
     }
 
-  return grub_fshelp_map_block (data->journal, blknr);
+  return blknr;
 }
 
 /* Read LEN bytes from the file described by DATA starting with byte
@@ -344,6 +345,8 @@ grub_ext2_read_inode (struct grub_ext2_data *data,
 {
   struct grub_ext2_block_group blkgrp;
   struct grub_ext2_sblock *sblock = &data->sblock;
+  int inodes_per_block;
+  unsigned int blkno;
   unsigned int blkoff;
 
   /* It is easier to calculate if the first inode is 0.  */
@@ -355,184 +358,23 @@ grub_ext2_read_inode (struct grub_ext2_data *data,
   if (grub_errno)
     return grub_errno;
 
-  blkoff = ino % grub_le_to_cpu32 (sblock->inodes_per_group);
+  inodes_per_block = EXT2_BLOCK_SIZE (data) / EXT2_INODE_SIZE (data);
+  blkno = (ino % grub_le_to_cpu32 (sblock->inodes_per_group))
+    / inodes_per_block;
+  blkoff = (ino % grub_le_to_cpu32 (sblock->inodes_per_group))
+    % inodes_per_block;
 
   /* Read the inode.  */
-  if (grub_fshelp_read (data->disk, data->journal,
-			grub_le_to_cpu32 (blkgrp.inode_table_id),
-			EXT2_INODE_SIZE (data) * blkoff,
-			sizeof (struct grub_ext2_inode), (char *) inode,
-			LOG2_EXT2_BLOCK_SIZE (data)))
+  if (grub_disk_read (data->disk,
+		      ((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))
     return grub_errno;
   
   return 0;
 }
 
-static void
-grub_ext3_get_journal (struct grub_ext2_data *data)
-{
-  char buf[1 << LOG2_BLOCK_SIZE (data)];
-  struct grub_ext3_journal_sblock *jsb;
-  grub_fshelp_journal_t log;
-  int last_num, num, block, log2bs;
-  grub_uint32_t seq;
-
-  auto void next_block (void);
-  void next_block (void)
-    {
-      block++;
-      if (block >= log->last_block)
-        block = log->first_block;
-    }
-
-  data->journal = 0;
-
-  if (! (data->sblock.feature_compatibility & EXT3_FEATURE_COMPAT_HAS_JOURNAL))
-    return;
-
-  if (! data->sblock.journal_inum)
-    return;
-
-  data->logfile.data = data;
-  data->logfile.ino = data->sblock.journal_inum;
-  data->logfile.inode_read = 1;
-
-  if (grub_ext2_read_inode (data, data->logfile.ino, &data->logfile.inode))
-    return;
-
-  log2bs = LOG2_EXT2_BLOCK_SIZE (data);
-  if (grub_fshelp_read_file (data->disk, &data->logfile, 0,
-                             0, sizeof (struct grub_ext3_journal_sblock),
-                             buf, grub_ext2_read_block,
-                             sizeof (buf), log2bs) !=
-      sizeof (struct grub_ext3_journal_sblock))
-    return;
-
-  jsb = (struct grub_ext3_journal_sblock *) &buf[0];
-  if (grub_be_to_cpu32 (jsb->header.magic) != EXT3_JOURNAL_MAGIC_NUMBER)
-    return;
-
-  /* Empty journal.  */
-  if (! jsb->start)
-    return;
-
-  log = grub_malloc (sizeof (struct grub_fshelp_journal) +
-                     grub_be_to_cpu32 (jsb->maxlen) * sizeof (grub_disk_addr_t));
-  if (! log)
-    return;
-
-  log->type = GRUB_FSHELP_JOURNAL_TYPE_FILE;
-  log->node = &data->logfile;
-  log->get_block = grub_ext2_read_block;
-  log->first_block = grub_be_to_cpu32 (jsb->first);
-  log->last_block = grub_be_to_cpu32 (jsb->maxlen);
-  log->start_block = grub_be_to_cpu32 (jsb->start);
-
-  last_num = num = 0;
-  block = log->start_block;
-  seq = grub_be_to_cpu32 (jsb->sequence);
-
-  while (1)
-    {
-      struct grub_ext3_journal_header *jh;
-
-      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];
-      if (grub_be_to_cpu32 (jh->magic) != EXT3_JOURNAL_MAGIC_NUMBER)
-        break;
-
-      if (grub_be_to_cpu32 (jh->sequence) != seq)
-        break;
-
-      log->mapping[num++] = GRUB_FSHELP_JOURNAL_UNUSED_MAPPING;
-      next_block();
-
-      switch (grub_be_to_cpu32 (jh->block_type))
-        {
-        case EXT3_JOURNAL_DESCRIPTOR_BLOCK:
-          {
-            struct grub_ext3_journal_block_tag *tag;
-            int ofs, flags;
-
-            ofs = sizeof (struct grub_ext3_journal_header);
-
-            do
-              {
-                tag = (struct grub_ext3_journal_block_tag *) &buf[ofs];
-                ofs += sizeof (struct grub_ext3_journal_block_tag);
-
-                if (ofs > (int) sizeof (buf))
-                  break;
-
-                flags = grub_be_to_cpu32 (tag->flags);
-                if (! (flags & EXT3_JOURNAL_FLAG_SAME_UUID))
-                  ofs += 16;
-
-                log->mapping[num++] = grub_be_to_cpu32 (tag->block);
-                next_block();
-              }
-            while (! (flags & EXT3_JOURNAL_FLAG_LAST_TAG));
-
-            continue;
-          }
-
-        case EXT3_JOURNAL_COMMIT_BLOCK:
-          {
-            seq++;
-            last_num = num - 1;
-            continue;
-          }
-
-        case EXT3_JOURNAL_REVOKE_BLOCK:
-          {
-            struct grub_ext3_journal_revoke_header *jrh;
-            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 < cnt; i++)
-              {
-                int j;
-                grub_uint32_t map;
-
-                map = grub_be_to_cpu32 (jrh->data[i]);
-                for (j = 0; j < num; j++)
-                  if (log->mapping[j] == map)
-                    log->mapping[j] = GRUB_FSHELP_JOURNAL_UNUSED_MAPPING;
-              }
-
-            continue;
-          }
-        default:
-          last_num = 0;
-          goto quit;
-        }
-    }
-
-quit:
-  if (! last_num)
-    grub_free (log);
-  else
-    {
-      int size;
-
-      size = sizeof (struct grub_fshelp_journal) +
-            last_num * sizeof (grub_disk_addr_t);
-
-      log->num_mappings = last_num;
-      data->journal = grub_realloc (log, size);
-    }
-}
-
 static struct grub_ext2_data *
 grub_ext2_mount (grub_disk_t disk)
 {
@@ -553,7 +395,6 @@ grub_ext2_mount (grub_disk_t disk)
     goto fail;
   
   data->disk = disk;
-  grub_ext3_get_journal (data);
 
   data->diropen.data = data;
   data->diropen.ino = 2;
@@ -758,11 +599,7 @@ grub_ext2_open (struct grub_file *file, const char *name)
 static grub_err_t
 grub_ext2_close (grub_file_t file)
 {
-  if (file->data)
-    {
-      grub_free (((struct grub_ext2_data *) file->data)->journal);
-      grub_free (file->data);
-    }
+  grub_free (file->data);
 
 #ifndef GRUB_UTIL
   grub_dl_unref (my_mod);
diff --git a/fs/fshelp.c b/fs/fshelp.c
index 2fae065..2bf1939 100644
--- a/fs/fshelp.c
+++ b/fs/fshelp.c
@@ -214,46 +214,6 @@ grub_fshelp_find_file (const char *path, grub_fshelp_node_t rootnode,
   return 0;
 }
 
-
-/* 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
@@ -350,30 +310,3 @@ grub_fshelp_log2blksize (unsigned int blksize, unsigned int *pow)
 
   return GRUB_ERR_NONE;
 }
-
-grub_disk_addr_t
-grub_fshelp_map_block (grub_fshelp_journal_t log, grub_disk_addr_t block)
-{
-  int map_block;
-
-  if ((! log) || (! block))
-    return block;
-
-  for (map_block = log->num_mappings - 1; map_block >= 0; map_block--)
-    {
-      if (log->mapping[map_block] == block)
-        break;
-    }
-
-  if (map_block < 0)
-    return 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);
-}
diff --git a/fs/reiserfs.c b/fs/reiserfs.c
index 9a059e3..9bde6d2 100644
--- a/fs/reiserfs.c
+++ b/fs/reiserfs.c
@@ -234,7 +234,6 @@ struct grub_reiserfs_data
 {
   struct grub_reiserfs_superblock superblock;
   grub_disk_t disk;
-  grub_fshelp_journal_t journal;
 };
 
 /* Internal-only functions. Not to be used outside of this file.  */
@@ -511,8 +510,7 @@ grub_reiserfs_get_item (struct grub_reiserfs_data *data,
   do
     {
       grub_disk_read (data->disk,
-                      grub_fshelp_map_block (data->journal, block_number) *
-                      (block_size >> GRUB_DISK_SECTOR_BITS),
+                      block_number * (block_size >> GRUB_DISK_SECTOR_BITS),
                       (((grub_off_t) block_number * block_size)
                        & (GRUB_DISK_SECTOR_SIZE - 1)),
                       block_size, (char *) block_header);
@@ -662,8 +660,7 @@ grub_reiserfs_read_symlink (grub_fshelp_node_t node)
 
   block_size = grub_le_to_cpu16 (node->data->superblock.block_size);
   len = grub_le_to_cpu16 (found.header.item_size);
-  block = (grub_fshelp_map_block (node->data->journal, found.block_number) *
-           (block_size  >> GRUB_DISK_SECTOR_BITS));
+  block = found.block_number * (block_size  >> GRUB_DISK_SECTOR_BITS);
   offset = grub_le_to_cpu16 (found.header.item_location);
 
   symlink_buffer = grub_malloc (len + 1);
@@ -682,124 +679,6 @@ grub_reiserfs_read_symlink (grub_fshelp_node_t node)
   return 0;
 }
 
-static void
-grub_reiserfs_get_journal (struct grub_reiserfs_data *data)
-{
-  int block_size = grub_le_to_cpu16 (data->superblock.block_size);
-  char buf[block_size];
-  struct grub_reiserfs_journal_header *jh;
-  grub_fshelp_journal_t log;
-  grub_uint32_t seq_id, mount_id;
-  int num_blocks = grub_le_to_cpu32 (data->superblock.journal_original_size);
-  int base_block = grub_le_to_cpu32 (data->superblock.journal_block);
-  int last_num, num, block;
-
-  data->journal = 0;
-
-  if (! data->superblock.journal_block)
-    return;
-
-  if (grub_disk_read (data->disk,
-                      (base_block + num_blocks)
-                      * (block_size >> GRUB_DISK_SECTOR_BITS),
-                      0, sizeof (struct grub_reiserfs_journal_header),
-                      buf))
-    return;
-
-  log = grub_malloc (sizeof (struct grub_fshelp_journal) +
-                     num_blocks * sizeof (grub_disk_addr_t));
-  if (! log)
-    return;
-
-  jh = (struct grub_reiserfs_journal_header *) &buf[0];
-
-  log->type = GRUB_FSHELP_JOURNAL_TYPE_BLOCK;
-  log->blkno = base_block;
-  log->first_block = 0;
-  log->last_block = num_blocks;
-  log->start_block = grub_le_to_cpu32 (jh->unflushed_offset);
-
-  seq_id = grub_le_to_cpu32 (jh->last_flush_uid);
-  mount_id = grub_le_to_cpu32 (jh->mount_id);
-
-  last_num = num = 0;
-  block = log->start_block;
-
-  while (1)
-    {
-      struct grub_reiserfs_description_block *db;
-      struct grub_reiserfs_commit_block *cb;
-      grub_uint32_t i, len, half_len, id, mid;
-
-      if (grub_disk_read (data->disk,
-                          (base_block + block)
-                          * (block_size >> GRUB_DISK_SECTOR_BITS),
-                          0, sizeof (buf), buf))
-        break;
-
-      if (grub_memcmp (&buf[block_size - REISERFS_MAGIC_LEN],
-                       REISERFS_MAGIC_DESC_BLOCK,
-                       sizeof (REISERFS_MAGIC_DESC_BLOCK) - 1))
-        break;
-
-      db = (struct grub_reiserfs_description_block *) &buf[0];
-      id = grub_le_to_cpu32 (db->id);
-      len = grub_le_to_cpu32 (db->len);
-      mid = grub_le_to_cpu32 (db->mount_id);
-      if ((id <= seq_id) && (mid <= mount_id))
-        break;
-
-      log->mapping[num++] = GRUB_FSHELP_JOURNAL_UNUSED_MAPPING;
-      half_len = ((block_size - 24) >> 2);
-      if (half_len > len)
-        half_len = len;
-
-      for (i = 0; i < half_len; i++)
-        log->mapping[num++] = db->real_blocks[i];
-
-      block += grub_le_to_cpu32 (db->len) + 1;
-      if (block >= log->last_block)
-        block -= log->last_block;
-
-      if (grub_disk_read (data->disk,
-                          (base_block + block)
-                          * (block_size >> GRUB_DISK_SECTOR_BITS),
-                          0, sizeof (buf), buf))
-        break;
-
-      cb = (struct grub_reiserfs_commit_block *) &buf[0];
-      if ((grub_le_to_cpu32 (cb->id) != id) ||
-          (grub_le_to_cpu32 (cb->len) != len))
-        break;
-
-      for (i = 0; i < len - half_len; i++)
-        log->mapping[num++] = cb->real_blocks[i];
-
-      last_num = num;
-      log->mapping[num++] = GRUB_FSHELP_JOURNAL_UNUSED_MAPPING;
-
-      block++;
-      if (block >= log->last_block)
-        block -= log->last_block;
-
-      seq_id = id;
-      mount_id = mid;
-    };
-
-  if (! last_num)
-    grub_free (log);
-  else
-    {
-      int size;
-
-      size = sizeof (struct grub_fshelp_journal) +
-            last_num * sizeof (grub_disk_addr_t);
-
-      log->num_mappings = last_num;
-      data->journal = grub_realloc (log, size);
-    }
-}
-
 /* Fill the mounted filesystem structure and return it.  */
 static struct grub_reiserfs_data *
 grub_reiserfs_mount (grub_disk_t disk)
@@ -819,7 +698,6 @@ grub_reiserfs_mount (grub_disk_t disk)
       goto fail;
     }
   data->disk = disk;
-  grub_reiserfs_get_journal (data);
   return data;
 
  fail:
@@ -867,8 +745,7 @@ grub_reiserfs_iterate_dir (grub_fshelp_node_t item,
       struct grub_reiserfs_item_header *item_headers;
       
       grub_disk_read (data->disk,
-                      grub_fshelp_map_block (data->journal, block_number) *
-                      (block_size >> GRUB_DISK_SECTOR_BITS),
+                      block_number * (block_size >> GRUB_DISK_SECTOR_BITS),
                       (((grub_off_t) block_number * block_size)
                        & (GRUB_DISK_SECTOR_SIZE - 1)),
                       block_size, (char *) block_header);
@@ -962,8 +839,7 @@ grub_reiserfs_iterate_dir (grub_fshelp_node_t item,
                         {
                           struct grub_reiserfs_stat_item_v1 entry_v1_stat;
                           grub_disk_read (data->disk,
-                                          grub_fshelp_map_block (data->journal, entry_block_number) *
-                                          (block_size >> GRUB_DISK_SECTOR_BITS),
+                                          entry_block_number * (block_size >> GRUB_DISK_SECTOR_BITS),
                                           grub_le_to_cpu16 (entry_item->header.item_location),
                                           sizeof (entry_v1_stat),
                                           (char *) &entry_v1_stat);
@@ -1005,8 +881,7 @@ grub_reiserfs_iterate_dir (grub_fshelp_node_t item,
                         {
                           struct grub_reiserfs_stat_item_v2 entry_v2_stat;
                           grub_disk_read (data->disk,
-                                          grub_fshelp_map_block (data->journal, entry_block_number) *
-                                          (block_size >> GRUB_DISK_SECTOR_BITS),
+                                          entry_block_number * (block_size >> GRUB_DISK_SECTOR_BITS),
                                           grub_le_to_cpu16 (entry_item->header.item_location),
                                           sizeof (entry_v2_stat),
                                           (char *) &entry_v2_stat);
@@ -1154,8 +1029,7 @@ grub_reiserfs_open (struct grub_file *file, const char *name)
     {
       struct grub_reiserfs_stat_item_v1 entry_v1_stat;
       grub_disk_read (data->disk,
-                      grub_fshelp_map_block (data->journal, block_number) *
-                      (block_size >> GRUB_DISK_SECTOR_BITS),
+                      block_number * (block_size >> GRUB_DISK_SECTOR_BITS),
                       entry_location
                       + (((grub_off_t) block_number * block_size)
                          & (GRUB_DISK_SECTOR_SIZE - 1)),
@@ -1168,8 +1042,7 @@ grub_reiserfs_open (struct grub_file *file, const char *name)
     {
       struct grub_reiserfs_stat_item_v2 entry_v2_stat;
       grub_disk_read (data->disk,
-                      grub_fshelp_map_block (data->journal, block_number) *
-                      (block_size  >> GRUB_DISK_SECTOR_BITS),
+                      block_number * (block_size  >> GRUB_DISK_SECTOR_BITS),
                       entry_location
                       + (((grub_off_t) block_number * block_size)
                          & (GRUB_DISK_SECTOR_SIZE - 1)),
@@ -1237,8 +1110,7 @@ grub_reiserfs_read (grub_file_t file, char *buf, grub_size_t len)
       switch (found.type)
         {
         case GRUB_REISERFS_DIRECT:
-          block = (grub_fshelp_map_block (data->journal, found.block_number) *
-                   (block_size  >> GRUB_DISK_SECTOR_BITS));
+          block = found.block_number * (block_size  >> GRUB_DISK_SECTOR_BITS);
           grub_dprintf ("reiserfs_blocktype", "D: %u\n", (unsigned) block);
           if (initial_position < current_position + item_size)
             {
@@ -1270,8 +1142,7 @@ grub_reiserfs_read (grub_file_t file, char *buf, grub_size_t len)
           if (! indirect_block_ptr)
             goto fail;
           grub_disk_read (found.data->disk,
-                          grub_fshelp_map_block (data->journal, found.block_number) *
-                          (block_size >> GRUB_DISK_SECTOR_BITS),
+                          found.block_number * (block_size >> GRUB_DISK_SECTOR_BITS),
                           grub_le_to_cpu16 (found.header.item_location),
                           item_size, (char *) indirect_block_ptr);
           if (grub_errno)
@@ -1282,9 +1153,8 @@ grub_reiserfs_read (grub_file_t file, char *buf, grub_size_t len)
                  && current_position < final_position;
                indirect_block++)
             {
-              block = (grub_fshelp_map_block (data->journal,
-                                              grub_le_to_cpu32 (indirect_block_ptr[indirect_block])) *
-                       (block_size >> GRUB_DISK_SECTOR_BITS));
+              block = grub_le_to_cpu32 (indirect_block_ptr[indirect_block]) *
+                      (block_size >> GRUB_DISK_SECTOR_BITS);
               grub_dprintf ("reiserfs_blocktype", "I: %u\n", (unsigned) block);
               if (current_position + block_size >= initial_position)
                 {
@@ -1383,7 +1253,6 @@ grub_reiserfs_close (grub_file_t file)
   struct grub_fshelp_node *node = file->data;
   struct grub_reiserfs_data *data = node->data;
 
-  grub_free (data->journal);
   grub_free (data);
   grub_free (node);
 #ifndef GRUB_UTIL
diff --git a/include/grub/fshelp.h b/include/grub/fshelp.h
index 847f78e..698c7aa 100644
--- a/include/grub/fshelp.h
+++ b/include/grub/fshelp.h
@@ -34,34 +34,6 @@ enum grub_fshelp_filetype
     GRUB_FSHELP_SYMLINK
   };
 
-enum grub_fshelp_journal_type
-  {
-    GRUB_FSHELP_JOURNAL_TYPE_BLOCK,
-    GRUB_FSHELP_JOURNAL_TYPE_FILE
-  };
-
-#define GRUB_FSHELP_JOURNAL_UNUSED_MAPPING	(grub_disk_addr_t) -1
-
-struct grub_fshelp_journal
-{
-  int type;
-  union
-    {
-      struct
-        {
-          grub_fshelp_node_t node;
-          grub_disk_addr_t (*get_block) (grub_fshelp_node_t node, grub_disk_addr_t block);
-        };
-       grub_disk_addr_t blkno;
-    };
-  int first_block;
-  int last_block;
-  int start_block;
-  int num_mappings;
-  grub_disk_addr_t mapping[0];
-};
-typedef struct grub_fshelp_journal *grub_fshelp_journal_t;
-
 /* Lookup the node PATH.  The node ROOTNODE describes the root of the
    directory tree.  The node found is returned in FOUNDNODE, which is
    either a ROOTNODE or a new malloc'ed node.  ITERATE_DIR is used to
@@ -84,15 +56,6 @@ 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
@@ -112,7 +75,4 @@ unsigned int
 EXPORT_FUNC(grub_fshelp_log2blksize) (unsigned int blksize,
 				      unsigned int *pow);
 
-grub_disk_addr_t
-EXPORT_FUNC(grub_fshelp_map_block) (grub_fshelp_journal_t log, grub_disk_addr_t block);
-
 #endif /* ! GRUB_FSHELP_HEADER */

Re: Some concern about the journal support

[Robert Millan]

On Sun, Jun 15, 2008 at 01:32:23AM +0800, Bean wrote:
> -/* 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_disk_addr_t
> -grub_fshelp_map_block (grub_fshelp_journal_t log, grub_disk_addr_t block)
> -{

Why are these functions removed?  I suppose they were only useful for journal
handling?  But they predate journal support, which is a bit confusing..

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

Re: Some concern about the journal support

[Bean]

On Sun, Jun 15, 2008 at 2:06 AM, Robert Millan <rmh@aybabtu.com> wrote:
> On Sun, Jun 15, 2008 at 01:32:23AM +0800, Bean wrote:
>> -/* 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_disk_addr_t
>> -grub_fshelp_map_block (grub_fshelp_journal_t log, grub_disk_addr_t block)
>> -{
>
> Why are these functions removed?  I suppose they were only useful for journal
> handling?  But they predate journal support, which is a bit confusing..

Hi,

grub_fshelp_map_block is used to map a fs block to the one in journal.
grub_fshelp_read is added a few days ago to fix a mapping issue. These
two function are only used by journal.

-- 
Bean

Re: Some concern about the journal support

[Robert Millan]

On Sun, Jun 15, 2008 at 02:29:13AM +0800, Bean wrote:
> 
> Hi,
> 
> grub_fshelp_map_block is used to map a fs block to the one in journal.
> grub_fshelp_read is added a few days ago to fix a mapping issue. These
> two function are only used by journal.

Ah, I think I was confusing it with grub_fshelp_read_file()

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

Re: Some concern about the journal support

[Javier Martín]

[-- Attachment #1: Type: text/plain, Size: 721 bytes --]

El dom, 15-06-2008 a las 01:32 +0800, Bean escribió:
> >> How about a separate module?
> > It's a good idea, perhaps I can add a journal layer on top of disk to
> > do the mapping transparently.
> This patch revert ext2 and reiserfs driver to pre journal state. The
> new journal support would be in a separate module and provide a
> transparent mapping layer for the fs driver.

Just my two cents... AFAIK the journal part of ext3 and ext4 in
implemented quite in the same fashion: the "journaled block device" or
JBD acts as a black box to the filesystem, which works with it in a
database transaction-like fashion. I don't know how ReiserFS handles its
journal, but I think the same approach would work.

[-- Attachment #2: Esta parte del mensaje está firmada digitalmente --]
[-- Type: application/pgp-signature, Size: 827 bytes --]

Re: Some concern about the journal support

[Pavel Roskin]

On Sun, 2008-06-15 at 01:32 +0800, Bean wrote:

> This patch revert ext2 and reiserfs driver to pre journal state. The
> new journal support would be in a separate module and provide a
> transparent mapping layer for the fs driver.

Sounds good to me.

-- 
Regards,
Pavel Roskin

Re: Some concern about the journal support

[Bean]

On Mon, Jun 16, 2008 at 9:27 AM, Pavel Roskin <proski@gnu.org> wrote:
> On Sun, 2008-06-15 at 01:32 +0800, Bean wrote:
>
>> This patch revert ext2 and reiserfs driver to pre journal state. The
>> new journal support would be in a separate module and provide a
>> transparent mapping layer for the fs driver.
>
> Sounds good to me.

Committed.

-- 
Bean