lkml.org 
[lkml]   [1997]   [Jul]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: buffer/swapping in pre-patch-2.0.31-2 + the 17th July patch
Hi,

A few comments.

On Thu, 17 Jul 1997, Dr. Werner Fink wrote:
> some improvements in comparison to the last one:
>
> * Limiting the start age of pages recently swapped in on the
> physical mem size of the task. Limits are 25% for rss and 12.5%
> for shm_rss.

This does have a firm foundation - when a paged is paged in from swap it
should be 'locked' into core for a minimum amount of time to justify the
cycles spent reading it in. The check of the RSS against core size is
good.
However, I feel doubling the value given to the age of such pages is a
little agressive. A simple increment should be enough to give these
pages a slight advantage, while avoiding the 'locking-down' of too many
pages.

Note, I currently running a patched 2.1.43 system with PAGE_AGE_VALUE (in
include/linux/pagemap.h) set to "PAGE_INTIAL_AGE + 3". This "seems" to
make the system more responsive - although that might be because I expect
it to (really need a dobule-blind test:)
I have mentioned changing the value of PAGE_AGE_VALUE to you before...

> * Add a new version of the fs/inode.c race patch/debugging code
> of Bill Hawes.

There are two races in an unpatched fs/inode.c;
1) When destroying an inode's identity (clear_inode()) via
get_empty_inode()), the destroyer may block while another
context calls iget() on the same inode.
The original code increments i_count, but this does not
avoid the race.
A simple fix to this is to remove the inode from both the
hash and free lists before calling truncate_inode_pages()
and dp_op->drop(). Nothing more fancy is needed.
(The patch tries to change too much - lets keep changes small
at this stage).

2) The second race is very small, and not dangerious.
Take an inode (call it 'x') which is dirty and has a reference
count of 1.
The last reference to this inode is removed with a call to
iput(). As it is the last ref, and the inode is dirty,
write_inode() is called which locks the inode while writing
it to store. The inode still has an i_count of 1.
Suppose the write blocks (say, because there are no free
request structures left).
While blocked, a sync_inodes() occurs. As inode 'x' is
locked, it blocks at wait_on_inode().
Meanwhile, our write_inode() wakes up and i_count for 'x'
is decremented - which gives a value of 0.
Now, before the sync-ing context gets onto proc, another
context calls get_empty_inode(). Inode 'x' has a zero
i_count, and is not locked or dirty, so it is a prime
candidate for reaping. clear_inode() destroys its identity,
and returns it to the caller.

Unfortunately, there is still a __wait_on_inode() in progess
for 'x' which is still waiting on the run-queue.

For this race to be dangerious depends on the file system
dependent code. I guess it isn't. But it is never a good idea
to block for a resource if you do not hold a reference to it.
I tend to use hard and soft counts to avoid such races, but
simply incrementing i_count on the entry to __wait_on_inode()
and dec-ing it on exit closes the doors. (This is what
__wait_on_buffer() does!).


I haven't looked through the buffer changes in detail, but did notice one
which might show up 'old' bugs;
Previously, refill_freelist() would only call wakeup_bdflush() if an
atomic allocation failed. This failure would be rare - or at least I
would hope so! Your patch (correctly) increases the chance of calling
wakeup_bdflush().
If wakeup_bdflush() is not called, there is no other way for
refill_freelist() to block. Hence, getblk() would not block (perhaps
rarely).
Now, everthing should be coded to expect getblk() not to be 'atomic'
(with regard to buffer management), but there is the possibility something
has been missed.
The odds are this isn't going to be a problem, but I thought I'd warn
you incase...

Regards,

markhe

-----------------------------------------------------
Mark Hemment UNIX/C Software Engineer (contractor)
"Success has many fathers. Failure is a b**tard"
-----------------------------------------------------




\
 
 \ /
  Last update: 2005-03-22 13:39    [W:0.144 / U:0.136 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site