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

[Marco Gerards]

Ognyan Kulev <ogi@fmi.uni-sofia.bg> writes:

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

No, it is not fixed.  It is just that the kernel determines the amount
of mapped blocks (all individual mappings) and when which mapping
should be removed.

But like I said I could've misunderstood the code.  I will just read
the text you are writing.

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

Great!  I am looking forwards to the full 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.

Does this problem cause any problems for currently use ext2fs?

>> Personally I would use "return 0" here.
>
> "if (bptr)" means that bptr != 0, so the code is correct.

It is correct.  This is a bit more about likes and dislikes so just
ignore me (unless the gcs or Marcus tell you otherwise ;)).

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

Ok, I understand.  Thinking of this (the amount of threads and the
amount of blocks that will be referenced by just one thread) it will
be very unlikely that this will ever happen.

Because this rarely happens it would be wise to test this (perhaps by
setting DISK_CACHE_BLOCKS to a lower value?).  Did this ever happen
while you were testing it?

I am thinking if it is possible to prevent this from happening
completely.  I will come back to this later.

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

Ah, great!

BTW, did you find this bug I mentioned?  I must admit that I did not
have the time to look into it yet. :(

Thanks,
Marco