Re: [Fwd: [patch #2508] ext2fs support for large store (> 1.5G)]

[Marco Gerards]

ogi@fmi.uni-sofia.bg writes:

>> I have some comments so far, I will continue to read the patch and
>> test it later this weekend.
>
> Thank you for reviewing the code :-)
>
> Would you add next comments to Savannah, so that we have all comments
> about the patch in one place?  I've already added your mail as comment
> about the patch.  My reply[1] is there too.
>
> [1] http://savannah.gnu.org/patch/index.php?func=detailitem&item_id=2508

Fine for me.  I have added a reply there.  I will paste it here so
others can follow it as well.  And it would be nice if you replied to
this mail when you explain how things work (if you want to do that).

This is copied form what I posted on savannah, so the formatting
(especially that of the sourcecode) is really bad:

--------------------------

Thanks for your fast response.

Because you have tested it only on a 3GB partition I can do just the same.

About the ChangeLog patch, I think you can better leave it out. (I've
included things like that, but now I notice a lot of noise can be
annoying while reading patches)

Can you please describe how your patches work? I am familiar with how
Neal proposed to fix it. So please describe the theory behind your
patch and which function is doing that what you describe.

Because this patch it quite big and many people who don't have the
time want to discuss it such explanation is important for acceptation
of the patch. And such explanation will help me a lot while reading
the patch.

Something I have noticed is that there is a disk_cache_blocks. I do
not understand why you have that. You can easily map in every page and
let GNUMach decide to evict the page (like Neal proposed). But before
I can say more about it I can better wait for your reply.

The same is true for pokels. They are there to keep track of all th
metadata in memory so you can write it easely to disk when syncing. I
assume you have datastructures for your cache that hold this
information as well, do you still need the pokel stuff? Well, perhaps
I am misunderstanding something about pokels or your patch (because I
do not fully understand it yet, give me a bit more time for that :)).

I think we can better discuss all this on the mailinglist. (because
not everyone has a look at savannah and the mail notification to
bug-hurd is broken)

Some things I have noticed so far:

      /* XXX: Touch all pages.  It seems that sometimes GNU Mach
         "forgets" to notify us about evicted pages.  */
      for (vm_offset_t i = 0; i < disk_cache_size; i += vm_page_size)
        *(volatile char *)(disk_cache + i);
Why do you need that? What happens when it isn't used?

And doesn't this mean al pages get dirty??

In disk_cache_map:

  bptr = ihash_find (disk_cache_bptr, block);
  if (bptr)
      ...
      return bptr;
    }

Personally I would use "return 0" here.

In the same function I see this code:

      if (pending_end >= 0)
        pager_return_some (diskfs_disk_pager,
                           pending_begin * vm_page_size,
                           (pending_end - pending_begin) * vm_page_size,
                           1);
      else
        printf ("ext2fs: disk cache is starving\n");

      /* Give it some time.  This should happen rarely.  */
      sleep (1);

I'm not that happy with this.  Can you explain when it happens (I
guess when you run out of cache?) and how sleeping fixes it.

I see some function calls that is enabled in the debugging
code. Please make sure the code works the same when debugging is of
like it works when it is on. (So, have you tested with debugging off?)

I hope you can explain the theory and reasoning about this all, but in
the meanwhile I will continue to read the patch.



Thanks,
Marco