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

[Ognyan Kulev]

Marco Gerards wrote:
> 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.

Do you mean to map (all metadata at fixed location) and (all indirect 
blocks), and to not care about reusing pages for different blocks?

> 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'm writing description of the patch, but I'm still at introduction yet. 
 Anyway, you can see part of the style of the description in 
http://debian.fmi.uni-sofia.bg/~ogi/hurd/ext3fs/ext2fs_20040211.txt . 
(This link is temporary.)  I hope to finish it in couple of days.  And 
again: currently there isn't much information this text.

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

It's just that: GNU Mach is instructed to notify us about evicted pages, 
but it doesn't.  This is not rare case.  The code above is work-around. 
 It first touches all pages and then synces them all.  Fortunately this 
seems to always work.

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

With small cache, after some time, yes.  Some pages are evicted, but 
Mach doesn't tell us.  And the count of these pages increases slowly.

> In disk_cache_map:
>
>   bptr = ihash_find (disk_cache_bptr, block);
>   if (bptr)
>       ...
>       return bptr;
>     }
>
> Personally I would use "return 0" here.

"if (bptr)" means that bptr != 0, so the code is correct.

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

The "starving" happens when all blocks are in use.  That is, each block 
has reference count > 0 and it can't be replaced.  So the best we can 
hope of is to wait for some other thread to release block.

The non-starving case is when there are some blocks with reference count 
== 0, but none of them is evicted.  So the "for" is to sync and flush 
all these potential candidates for replacement.  Although 
pager_return_some (..., 1) (sync+flush with waiting) is used, sleep (1) 
is executed again.  In theory, this is not needed.  I'll remove it (for 
the non-starving case) in RC2 and test 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 don't remember if I try it with debugging off.  So this is one more 
note before releasing RC2 :-)

Regards,
ogi