Messages in this thread | | | Date | Wed, 14 Apr 1999 01:04:55 +0200 | From | Jan Kara <> | Subject | Bugs in quota code (long) (new) |
| |
Hello.
It seems like my mail didn't get into the list so I'm resending an updated version:
So finally I fixed reported race in quotas and I've probably found some others. So here is the list of them:
1) dquot_transfer() gathers all quotas of inode. When it gathers a quota it performs a check and then goes for another quota. But dqget() might block and so the check might be invalid now (some other operation changed the quota in the meantime). Also when quota_off is run while we are sleeping the quota might be discarded without writing to disk (see latter bugs). I already reported this bug. Fix - lock got quotas...
2) When DQUOT_TRANSFER() is called. It first checked and moved the quotas, then performed the notify_change() and when it failed it tried to put quotas back. But this can fail - either we turned quotas off or the quotas changed while we were sleeping and so transfer will refuse to return the quotas. This was also in my previous mail ;-). Fix - move the notify_change() into the dquot_transfer(). This has a bit unpleasant consequence - dquot_transfer() now can't return NO_QUOTA / QUOTA_OK but it has to return error codes so it is a bit inconsistent. But on the other size every call to dquot_transfer() is done via DQUOT_TRANSFER() do it doesn't impact any current code...
3) When dqget() is waiting for dquot the dquot might become invalidated. It is a bit unpleasant for dqget() to return unhashed and invalidated entries (also some functions doesn't count with it and to they will happily dereference NULL). So the dqget() checks dquot after wait_on_dquot() and if it is invalidated it will return NODQUOT (as if the dquot was invalidated before the dqget() was called).
4) Functions like dquot_alloc_block/inode also perform the check and when all checks are OK it starts to update the quotas. Because update does lock_dquot() it can block. Now the quotas may change and also for example dquot_transfer() can be run in the meantime and so we will update other quotas than we should. We (as in dquot transfer) get all quotas and lock them so nothing can happen between check and update. Also we have to remove the locks from dquot_incr_block/inode of course...
5) When quota_off is run, it will drop all pointers to quotas from inodes. Then invalidate_dquots() goes through list and invalidates every quota it sees (of course only of specified typed on right device ;-)). But this way we can loose some data - someone (eq. get_quota()) does dqget() - use count is now 2. Now the function blocks (copy_to_ user has to swap?). The quota_off is run. All pointers from inodes are dropped but not all dquots are written - dqput writes only dquots where use_count == 0 and our dquot has use count == 1. Then we invalidate all quotas (we ignore if the dquot is dirty). Then the function awakes and does dqput(). Now the use_count becomes 0 but quota is invalidated and so it is not written... Fix - when invalidate_dquots() sees dirty dquot it writes it first and then it invalidates it...
6) get_quota() doesn't lock the quota and so it might return some bad data when copy_to_user blocks (eq. quota_off is run). Fix - lock it.
So these were the bugs (OK, probably some of these aren't bugs and I'm missing something...) I found and made a patch for. The patch is attached to this mail. It is against 2.2.1. For me the patched quotas work but I'd be glad if someone could try it also in the other environment..
Here are also some other bugs / suggestions:
7) We already use slab cache to allocate dquots but we *never* free them. We are just putting them on the free_list. Maybe we can free some dquots / provide some function like prune_qcache...
8) In case that there is not enough memory and grow_dquots() will not be able to allocate any other quotas it will be called again and again. But we have nonempty free list so in fact we have free quotas...
9) get_empty_dquot() tests whether got dquot isn't locked or dirty. But locked dquot can't get into free list (dqput always waits for it). Dirty dquot can be in the free list - when some quota is invalidated and someone other is performing some action on it (after my patches this should never happen because every change is done with locked dquot but..). But in that case writing isn't the right solution ;-)... ------------------------------------ The new things: ad 8) Changed grow_dquots() a bit so it will now return number of successfully allocated structures. If it succeeds, everything restarts. Otherwise we search the free list. ad 9) Removed the write from get_empty_dquot()...
Patch also does: Fixes the second pass in sync_dquots() and invalidate_dquots() - on second pass we didn't initialize our pointer so in fact we never did more than one pass.
Added test to sync_dquots for invalidated dquot.
Added correct initialization of wait_queue to clear_dquot.
Added test to dquot_initialize() for dqget() returning NODQUOT.
dqput() now clears DQ_LOCK (after complaining) and DQ_MOD flags when putting on free list.
Bye, Honza
PS: gzipped diff against 2.2.1 attached...
[unhandled content-type:application/x-gunzip]
| |