From: Jan Kara The patch fixes locking of i_flags. It removes S_QUOTA flag from i_flags because it was almost unused and updating it on some places correctly (under i_sem) would be tricky. Note that accessing of S_NOQUOTA flag is serialized by dqptr_sem and so we can reliably tested without i_sem. Signed-off-by: Andrew Morton --- 25-akpm/fs/dquot.c | 52 +++++++++++++++++++++------------------ 25-akpm/fs/inode.c | 12 ++++----- 25-akpm/include/linux/fs.h | 16 +++++------- 25-akpm/include/linux/quotaops.h | 19 ++++++++++++-- 4 files changed, 57 insertions(+), 42 deletions(-) diff -puN fs/dquot.c~quota-iflags-locking-fix fs/dquot.c --- 25/fs/dquot.c~quota-iflags-locking-fix 2004-07-05 16:07:02.507789504 -0700 +++ 25-akpm/fs/dquot.c 2004-07-05 16:07:02.518787832 -0700 @@ -96,9 +96,11 @@ * * Any operation working on dquots via inode pointers must hold dqptr_sem. If * operation is just reading pointers from inode (or not using them at all) the - * read lock is enough. If pointers are altered function must hold write lock. - * If operation is holding reference to dquot in other way (e.g. quotactl ops) - * it must be guarded by dqonoff_sem. + * read lock is enough. If pointers are altered function must hold write lock + * (these locking rules also apply for S_NOQUOTA flag in the inode - note that + * for altering the flag i_sem is also needed). If operation is holding + * reference to dquot in other way (e.g. quotactl ops) it must be guarded by + * dqonoff_sem. * This locking assures that: * a) update/access to dquot pointers in inode is serialized * b) everyone is guarded against invalidate_dquots() @@ -112,7 +114,7 @@ * operations on dquots don't hold dq_lock as they copy data under dq_data_lock * spinlock to internal buffers before writing. * - * Lock ordering (including some related VFS locks) is the following: + * Lock ordering (including related VFS locks) is following: * i_sem > dqonoff_sem > iprune_sem > journal_lock > dqptr_sem > * > dquot->dq_lock > dqio_sem * i_sem on quota files is special (it's below dqio_sem) @@ -686,16 +688,8 @@ static inline int dqput_blocks(struct dq int remove_inode_dquot_ref(struct inode *inode, int type, struct list_head *tofree_head) { struct dquot *dquot = inode->i_dquot[type]; - int cnt; inode->i_dquot[type] = NODQUOT; - /* any other quota in use? */ - for (cnt = 0; cnt < MAXQUOTAS; cnt++) { - if (inode->i_dquot[cnt] != NODQUOT) - goto put_it; - } - inode->i_flags &= ~S_QUOTA; -put_it: if (dquot != NODQUOT) { if (dqput_blocks(dquot)) { #ifdef __DQUOT_PARANOIA @@ -956,8 +950,6 @@ int dquot_initialize(struct inode *inode break; } inode->i_dquot[cnt] = dqget(inode->i_sb, id, cnt); - if (inode->i_dquot[cnt]) - inode->i_flags |= S_QUOTA; } } out_err: @@ -974,7 +966,6 @@ int dquot_drop(struct inode *inode) int cnt; down_write(&sb_dqopt(inode->i_sb)->dqptr_sem); - inode->i_flags &= ~S_QUOTA; for (cnt = 0; cnt < MAXQUOTAS; cnt++) { if (inode->i_dquot[cnt] != NODQUOT) { dqput(inode->i_dquot[cnt]); @@ -1367,7 +1358,7 @@ static int vfs_quota_on_file(struct file struct quota_info *dqopt = sb_dqopt(sb); struct dquot *to_drop[MAXQUOTAS]; int error, cnt; - unsigned int oldflags; + unsigned int oldflags = -1; if (!fmt) return -ESRCH; @@ -1379,22 +1370,26 @@ static int vfs_quota_on_file(struct file if (!S_ISREG(inode->i_mode)) goto out_fmt; + down(&inode->i_sem); down(&dqopt->dqonoff_sem); if (sb_has_quota_enabled(sb, type)) { + up(&inode->i_sem); error = -EBUSY; goto out_lock; } - oldflags = inode->i_flags; - dqopt->files[type] = f; - error = -EINVAL; - if (!fmt->qf_ops->check_quota_file(sb, type)) - goto out_file_init; /* We don't want quota and atime on quota files (deadlocks possible) * We also need to set GFP mask differently because we cannot recurse * into filesystem when allocating page for quota inode */ down_write(&dqopt->dqptr_sem); + oldflags = inode->i_flags & (S_NOATIME | S_NOQUOTA); inode->i_flags |= S_NOQUOTA | S_NOATIME; + up_write(&dqopt->dqptr_sem); + up(&inode->i_sem); + dqopt->files[type] = f; + error = -EINVAL; + if (!fmt->qf_ops->check_quota_file(sb, type)) + goto out_file_init; /* * We write to quota files deep within filesystem code. We don't want * the VFS to reenter filesystem code when it tries to allocate a @@ -1404,11 +1399,11 @@ static int vfs_quota_on_file(struct file mapping_set_gfp_mask(inode->i_mapping, mapping_gfp_mask(inode->i_mapping) & ~__GFP_FS); + down_write(&dqopt->dqptr_sem); for (cnt = 0; cnt < MAXQUOTAS; cnt++) { to_drop[cnt] = inode->i_dquot[cnt]; inode->i_dquot[cnt] = NODQUOT; } - inode->i_flags &= ~S_QUOTA; up_write(&dqopt->dqptr_sem); /* We must put dquots outside of dqptr_sem because we may need to * start transaction for dquot_release() */ @@ -1434,11 +1429,20 @@ static int vfs_quota_on_file(struct file return 0; out_file_init: - inode->i_flags = oldflags; dqopt->files[type] = NULL; out_lock: - up_write(&dqopt->dqptr_sem); up(&dqopt->dqonoff_sem); + if (oldflags != -1) { + down(&inode->i_sem); + down_write(&dqopt->dqptr_sem); + /* Reset the NOATIME flag back. I know it could change in the + * mean time but playing with NOATIME flags on a quota file is + * never a good idea */ + inode->i_flags &= ~(S_NOATIME | S_NOQUOTA); + inode->i_flags |= oldflags; + up_write(&dqopt->dqptr_sem); + up(&inode->i_sem); + } out_fmt: put_quota_format(fmt); diff -puN fs/inode.c~quota-iflags-locking-fix fs/inode.c --- 25/fs/inode.c~quota-iflags-locking-fix 2004-07-05 16:07:02.509789200 -0700 +++ 25-akpm/fs/inode.c 2004-07-05 16:07:02.519787680 -0700 @@ -1235,26 +1235,26 @@ void remove_dquot_ref(struct super_block if (!sb->dq_op) return; /* nothing to do */ spin_lock(&inode_lock); /* This lock is for inodes code */ - /* We don't have to lock against quota code - test IS_QUOTAINIT is just for speedup... */ - + + /* We hold dqptr_sem so we are safe against the quota code */ list_for_each(act_head, &inode_in_use) { inode = list_entry(act_head, struct inode, i_list); - if (inode->i_sb == sb && IS_QUOTAINIT(inode)) + if (inode->i_sb == sb && !IS_NOQUOTA(inode)) remove_inode_dquot_ref(inode, type, tofree_head); } list_for_each(act_head, &inode_unused) { inode = list_entry(act_head, struct inode, i_list); - if (inode->i_sb == sb && IS_QUOTAINIT(inode)) + if (inode->i_sb == sb && !IS_NOQUOTA(inode)) remove_inode_dquot_ref(inode, type, tofree_head); } list_for_each(act_head, &sb->s_dirty) { inode = list_entry(act_head, struct inode, i_list); - if (IS_QUOTAINIT(inode)) + if (!IS_NOQUOTA(inode)) remove_inode_dquot_ref(inode, type, tofree_head); } list_for_each(act_head, &sb->s_io) { inode = list_entry(act_head, struct inode, i_list); - if (IS_QUOTAINIT(inode)) + if (!IS_NOQUOTA(inode)) remove_inode_dquot_ref(inode, type, tofree_head); } spin_unlock(&inode_lock); diff -puN include/linux/fs.h~quota-iflags-locking-fix include/linux/fs.h --- 25/include/linux/fs.h~quota-iflags-locking-fix 2004-07-05 16:07:02.510789048 -0700 +++ 25-akpm/include/linux/fs.h 2004-07-05 16:07:02.521787376 -0700 @@ -133,14 +133,13 @@ extern int leases_enable, dir_notify_ena #define S_SYNC 1 /* Writes are synced at once */ #define S_NOATIME 2 /* Do not update access times */ -#define S_QUOTA 4 /* Quota initialized for file */ -#define S_APPEND 8 /* Append-only file */ -#define S_IMMUTABLE 16 /* Immutable file */ -#define S_DEAD 32 /* removed, but still open directory */ -#define S_NOQUOTA 64 /* Inode is not counted to quota */ -#define S_DIRSYNC 128 /* Directory modifications are synchronous */ -#define S_NOCMTIME 256 /* Do not update file c/mtime */ -#define S_SWAPFILE 512 /* Do not truncate: swapon got its bmaps */ +#define S_APPEND 4 /* Append-only file */ +#define S_IMMUTABLE 8 /* Immutable file */ +#define S_DEAD 16 /* removed, but still open directory */ +#define S_NOQUOTA 32 /* Inode is not counted to quota */ +#define S_DIRSYNC 64 /* Directory modifications are synchronous */ +#define S_NOCMTIME 128 /* Do not update file c/mtime */ +#define S_SWAPFILE 256 /* Do not truncate: swapon got its bmaps */ /* * Note that nosuid etc flags are inode-specific: setting some file-system @@ -164,7 +163,6 @@ extern int leases_enable, dir_notify_ena ((inode)->i_flags & (S_SYNC|S_DIRSYNC))) #define IS_MANDLOCK(inode) __IS_FLG(inode, MS_MANDLOCK) -#define IS_QUOTAINIT(inode) ((inode)->i_flags & S_QUOTA) #define IS_NOQUOTA(inode) ((inode)->i_flags & S_NOQUOTA) #define IS_APPEND(inode) ((inode)->i_flags & S_APPEND) #define IS_IMMUTABLE(inode) ((inode)->i_flags & S_IMMUTABLE) diff -puN include/linux/quotaops.h~quota-iflags-locking-fix include/linux/quotaops.h --- 25/include/linux/quotaops.h~quota-iflags-locking-fix 2004-07-05 16:07:02.512788744 -0700 +++ 25-akpm/include/linux/quotaops.h 2004-07-05 16:07:02.521787376 -0700 @@ -69,9 +69,22 @@ static __inline__ void DQUOT_INIT(struct /* The same as with DQUOT_INIT */ static __inline__ void DQUOT_DROP(struct inode *inode) { - if (IS_QUOTAINIT(inode)) { - BUG_ON(!inode->i_sb); - inode->i_sb->dq_op->drop(inode); /* Ops must be set when there's any quota... */ + /* Here we can get arbitrary inode from clear_inode() so we have + * to be careful. OTOH we don't need locking as quota operations + * are allowed to change only at mount time */ + if (!IS_NOQUOTA(inode) && inode->i_sb && inode->i_sb->dq_op + && inode->i_sb->dq_op->drop) { + int cnt; + /* Test before calling to rule out calls from proc and such + * where we are not allowed to block. Note that this is + * actually reliable test even without the lock - the caller + * must assure that nobody can come after the DQUOT_DROP and + * add quota pointers back anyway */ + for (cnt = 0; cnt < MAXQUOTAS; cnt++) + if (inode->i_dquot[cnt] != NODQUOT) + break; + if (cnt < MAXQUOTAS) + inode->i_sb->dq_op->drop(inode); } } _