Messages in this thread | | | Date | Fri, 18 Jul 1997 14:29:03 +0100 (BST) | From | Mark Hemment <> | Subject | Re: 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" -----------------------------------------------------
| |