Kernel Traffic #12 For 1 Apr 1999

The patch looks fine, but I wonder..

I don't think we should move a buffer to the unused list without making damn sure that it doesn't have any IO active on it.

I'll apply it anyway, because it _is_ obviously cleaner (the old code was meant to handle the case where the buffer might still be shared with some MM mapping - something that can't happen any more), and I /think/ it's actually harmless even if we have some old active write to the buffer, but it still makes me worry.

Having thought about it for five minutes I convinced myself that the patch as Andrea had it was potentially horribly deadly, and could result in total disk corruption. Bad.

Just to give you an idea of why the old one was bad:

• buffer with an active writeout is bforgot()'en
• buffer is re-allocated to something else
• the active writeout completes, and marks the buffer uptodate
• the new user gets terminally confused because the contents of the buffer may be crap

It's _extremely_ unlikely, because we put the buffer at the end of the free list and it needs just about pessimal timings to happen anyway, but the unlikely bugs are the worst ones to fix later.

However, it is certainly true that the old bforget() was a piece of crap too. Here's my current one that looks "obviously correct" - can people who saw the performance problems with the original version please test this one out?

Can anybody see any downsides with this patch? Note that the likelihood of the if-statement triggering is pretty low, so I don't think performance should be any different from Andreas patch, but the above looks safe and "feels" right to me. I certainly know that I would have been extremely nervous to release a real kernel with the previous patch posted.

Looking at the ext2 code, that's obviously the case.

However, looking some more, it's also obviously the case that the ext2 code is being really really bad at doing this, and has a ton of assumptions about not only how bforget() used to work, but also about how find_buffer() works.

In particular, as far as I can tell, ext2fs will _busywait_ for the buffer becoming unlocked if it was locked. Uhhuh. Shades of Windows NT.

And I can't even blame anybody: back when that code was written, brelse() used to be rather inefficient and do a "wait_on_buffer()" that caused us to re-schedule if the buffer was locked. Since then, the buffer handling has been cleaned up quite a bit, and now the loop is just a busy loop in kernel space.

But it's certainly an example of a filesystem that is being rather incestuous with the VFS layer, abusing undocumented knowledge of just exactly how brelse() and bforget() used to work (and in this case about exactly what the limitations were).

Anyway, it's probably not a serious problem, because even when it does busy-wait, it eventually does the right thing so it's not actually a correctness bug, and the busy-wait should be so rare as to not really be a performance issue either.

I should probably just remove the re-try logic, and change the busy loop to do a "wait_on_bh()" instead (because we do need to wait on the buffer, otherwise we'd mark the thing free without having finished all the IO on the disk, which has similar race conditions - except now the races are on the disk platter instead of in kernel data structures).

Oh, OK. That makes sense. I missed the reodering side-effect. Thanks ;-)

In FAT fixes (I'll post the them for testing tomorrow or on Wednesday) I actually needed a clear way to keep references to inode that wouldn't affect i_count and related behaviour. I did it - it looks so:

1. Whenever we are scheduling the inode for freeing (free_inodes, invalidate_inodes and two places in iput) we set I_FREEING in ->i_state.
2. I've added struct inode *igrab(struct inode*) that grabs the spinlock, checks for I_FREEING, if it is set - releases the spinlock and returns NULL, otherwise increases i_count, releases the spinlock, does wait_on_inode() and returns the inode.
3. If fs wants to manage private references to struct inode it can use igrab()/iput() to get/release a normal reference. It has to supply ->clear_inode() method and forget all private references to inode when it is called. Some simple internal locking may be needed within the filesystem, indeed. igrab() returning NULL should be processed as the case when the reference was already invalidated by foo_clear_inode() call.

Changes to fs/inode.c are minimal (+10 lines) and do not affect existing filesystems. If fs wants to keep an internal hash indexed *not* by i_ino it can do it easily (e.g. FAT - we can keep i_ino constant over the whole life of in-core inode and forget about bothering with 'busy' directory slots. There go 20-odd races in FAT-derived filesystem + tons of ugly code working around other 20-odd races). In case of FAT all code needed to implement hash indexed by directory entry position + glue for clear interaction with icache took about 40 lines. *And* allowed to remove much more cruft.

There is a small problem with passing everything through Alan: as you know Linus' and Alan's trees are very different with respect to the NFS code because Alan accepted to test out the 8k patches. This means that

a) Not all patches to Linus' tree are applicable to Alan's tree
b) Patches that need to be applied to both trees usually need some massaging when moving from one to the other.

This is why I generally try to pass patches that for Alan directly to him, and only put out fixes against Linus' tree on linux-kernel. Since Alan has been very quick to put out new releases, I think that has been an acceptable practice.

Note: b) is in part due to the extensive NFS client structure changes, and in part due to a few trivial fixes to the sunrpc client. The latter are what affect fs/lockd and fs/nfsd, and are probably the main cause of problems with the knfsd patches (apart from HJ's fix to fs/nfs/dir.c:nfs_rename which is unnecessary on Alan's tree). Most trouble could therefore perhaps be avoided if we start the merge of the sunrpc fixes in question to Linus' tree.

Has anyone else seen Quantum's comments on their "a/v" drives ?

"Disclaimer: The Quantum AV drives have been optimized for use in digital video and audio environments. These optimizations make Quantum's A/V drives unsuitable for use in applications where data integrity is an absolute requirement. For applications where Quantum drives will not be used exclusively for digital AV, please consider Quantum's non-optimized line of hard disk drives".

Gives a new meaning to the word "optimized", eh ? "We made 'em really fast, but oh, sometime they lose bits".

Sigh. We keep getting these kinds of questions long after the fix has been posted N times. I've finally pulled my finger out and gotten around to doing something I've been planning:

There is a new page: http://www.atnf.csiro.au/~rgooch/linux/docs/kernel-newsflash.html

which contains urgent fixes for the latest official kernel. There is also a link to this page from the kernel-list FAQ.

Please, people, if a new kernel falls over in a heap, read this page first. Help conserve electrons.

Linus: feel free to refer to this page in subsequent announcements. It may serve as a pre-emptive reminder. Well, I can hope, anyway...

when i first considered this, i agreed with your reasoning, and thought that it would be a good change. however, after trying it under load, i discovered that leaving b_count as 1 for free buffers actually *helps* performance, and doesn't appear to cause the memory shortage problems you feared. i'm genuinely curious to know, btw, what pathological conditions do you think might cause a catastrophic memory shortage?

setting b_count to 0 for free buffers allows try_to_free_buffers() to free more pages-- this is what you wanted it to do. but it's actually bad behavior in terms of performance, since the pages it frees are quite arbitrary in relation to which buffers are most used. in other words, setting b_count to 0 when freeing buffers exacerbates the fact that the Linux buffer cache has no buffer reclamation policy right now. leaving b_count set to 1 reduces the likelihood that a page will be stolen, thus preserving the contents of the buffer cache.

under load, vmstat shows twice as many "bi" when b_count is set to 0 for free buffers; application throughput drops measurably more as offered load increases than when it is running on a kernel that leaves b_count alone. one can also see the buffer cache size fluctuating significantly when the file working set and the page working set don't all fit in memory. this, i argue, is pathological to performance, since it means pages are flowing in and out of the buffer cache quickly, and are not staying in long enough to be of use.

in the big picture, shrinking either the page cache or the buffer cache will result in lower hit rates and worse (i.e. closer to disk speed) performance. you really want to be careful to throw out the oldest or least used page/buffer, because to do otherwise wastes disk bandwidth, and that hurts *both* file system *and* VM performance.

setting b_count to zero for free buffers might be the right thing to do, but it makes more urgent the need to fix the reclamation problem. successfully stealing pages from the buffer cache right now is very hard on file system performance. perhaps adding some logic to regularly supply the free list with the oldest buffers, and allowing try_to_free_buffers() only to free pages containing buffers on the free list, might be a good solution. perhaps calling try_to_free_buffers() or wakeup_bdflush() should automatically reclaim old buffers, thereby slowing down cache growth and reducing the need to steal pages from it. or what would happen if the buffer cache, and not shrink_mmap(), could choose which page gets freed/stolen?

the best solution is to figure out how to allow page stealing while not disturbing the LRU queues in the buffer cache.

I said they were unlikely..

Basically, the only real case I can imagine where this actually could result in serious problems is:

• write a large file that fills much of your memory because you're not doing anything else.
• remove the file.
• start applications that do _not_ do any writes to the filesystem, but use lots of memory other ways.."

There is a strange routing problem in NORDUnet network, which prevents nic.funet.fi from reaching a part of the world -- I take that to mean that the same part of the world won't reach nic.funet.fi either ...

The list delivery will resume once the routing bug is fixed, as a matter of hours, I hope.

Mind you, I realized what was going on only a couple hours ago, and the situation has been on for 36 hours. (Email queue timeouts threaten at 72 hours, I hope it won't get to that..)