]> git.hungrycats.org Git - linux/commitdiff
[PATCH] quota: inode->i_flags locking fixes
authorJan Kara <jack@ucw.cz>
Sun, 11 Jul 2004 02:28:28 +0000 (19:28 -0700)
committerLinus Torvalds <torvalds@ppc970.osdl.org>
Sun, 11 Jul 2004 02:28:28 +0000 (19:28 -0700)
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 <akpm@osdl.org>
Signed-off-by: Linus Torvalds <torvalds@osdl.org>
fs/dquot.c
fs/inode.c
include/linux/fs.h
include/linux/quotaops.h

index 748beac66e02c241107f13b26a2a418ee26839de..2c2bdb506c3f9233ac4dc5c194b165385d72689f 100644 (file)
  *
  * 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()
  * 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 dquot *dquot)
 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, int type)
                                        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 *f, int type, int format_id)
        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 *f, int type, int format_id)
        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 *f, int type, int format_id)
        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 *f, int type, int format_id)
        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);
 
index 5cd72ced8c9e5e16dee11bd4a50c9535747894f6..ec0945963c930aa30abbb6e51019abe47bafe5a0 100644 (file)
@@ -1235,26 +1235,26 @@ void remove_dquot_ref(struct super_block *sb, int type, struct list_head *tofree
        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);
index 92e5252649981234084bc49570e032772e8c3df0..de1cd53ca7d3eb2c7a03c7d4a7265604da30b93c 100644 (file)
@@ -133,14 +133,13 @@ extern int leases_enable, dir_notify_enable, lease_break_time;
 
 #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_enable, lease_break_time;
                                        ((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)
index 9db474dcafd585edf0d92147a5b70ea2a4eaa191..acc926c64e78941f0dbe74807d287140f128a577 100644 (file)
@@ -69,9 +69,22 @@ static __inline__ void DQUOT_INIT(struct inode *inode)
 /* 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);
        }
 }