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

[Marco Gerards]

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

> I'm not good writer, but my hope is that this description will help for
> better understanding of the patch.  Actually, it has already helped me
> to understand my own patch and to spot some things that better be
> changed in RC2 :-)

Thanks a lot for your text.  It is really clear and useful to me.

There are a few possible problems I noticed:

disk_pager_read_page removes the RC_MAPPING bit from the flags and
notices the disk_cache_wait_remapping that it is safe to continue.
But you broadcast the condition before removing the bit.  I think this
can cause some problems, what do you think?

How about the loop that touches all pages?  I read touch as writing,
does it make pages dirty?  If that happens it is not acceptable.  when
it reads in a page that was evicted it is just an unfortunate waste of
resources but ok for me (but I do not think everyone will agree with
me).  Better fix it in Mach.

When there are no pages free you just throw the entire cache away (if
I understand it correctly).  I do not like this because this can be
one of the main factors that will slow down someone's computer.  Isn't
it possible to ask Mach to evict 10 pages (for example)?  Mach usually
knows better which pages should be evicted.  If that is not possible
you can just evict 10 pages (10 was randomly chosen) and evict the
non-dirty pages first.

There is some possible locking problem:
The call disk_cache_wait_remapping is called with disk_cache_lock
locked.  The condition in disk_pager_read_page is broadcasted while
disk_cache_lock is locked too.

What if you are waiting for the condition?  It can never be send
because when disk_pager_read_page is called ext2fs will deadlock.
Other that that I would not like to lock a loop that depends on an
condition.

Please be really careful with a global lock like disk_cache_lock.  It
cannot make it hard to prevent deadlocks, it can slow ext2fs down
too.  I do not know if there is another way though.

I also hope you will fix the other things you mentioned as possible
problems and not so beautiful things, for example that the pokels do
not call the disk_cache functions when it is shouldn't do that (like
you mentioned).

Have you fixed the bug I reported BTW?

I will put this on savannah as well.

Thanks,
Marco