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

[Ognyan Kulev]

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

You're right, but this has no practical sequences, because 
condition_wait will continue after unlocking of disk_cache_lock in 
disk_pager_read_page.  See below.  Anyway, I'll change this (swap 
broadcast and clearing the flag) in RC2.

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

Here "touch" is used as "peek".  Il'l change that too.

And, really, Mach should be fixed.  But I don't have time resource to do 
this, and Mach code is famous for being unpleasent to read.

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

In the text, there is some rationale behind flushing all pages with 
reference count zero.  Dirty pages are not flushed, because they have 
refcount > 0.  (Dirty pages are always in some pokel or are synced 
before deref.)

There is another way for fine-grained flushing of pages. 
disk_cache_hint cycles through whole cache and is used for finding 
unreferenced block which is not in core (i.e. evicted).  So next 5-10 
unreferenced blocks after disk_cache_hint can be flushed 
(pager_return_some) and another try can be made.  Synchronization 
between pager_notify_pageout and disk_cache_map is not clear to me right 
now, though.  I'll try to implement it for RC2.

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

This is by design.  condition_wait unlocks the lock and waits.  When 
condition is broadcasted, it locks the lock again, potentially blocking 
the thread.  In our case, this will make disk_cache_wait_remapping to 
wait until disk_pager_read_page unlocks disk_cache_lock.

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

Yes, but in our case I see no way to avoid it too.

> Have you fixed the bug I reported BTW?

No, unfortunately I still haven't found time for debugging :-(  But this 
is something that must be fixed before RC2.

Regards,
ogi