lkml.org 
[lkml]   [1999]   [Apr]   [14]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectBugs 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]
\
 
 \ /
  Last update: 2005-03-22 13:51    [W:0.155 / U:0.004 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site