Re: ext2 FS corruption with 2.5.59.

[Andrew Morton <akpm@digeo.com>]

Oleg Drokin <green@namesys.com> wrote:
>
> Looking through 2.5.54, I found that changeset named "[PATCH] quota locking update" forwarded by you
> is the cause for the problem.

OK, it's writepage-versus-writepage and/or writepage-versus-truncate. 
There's no locking between the updaters and i_blocks goes negative.
This is good ;)  We have done well.

I went for the new frlocks which I dragged into -mm to fix up the xtime_lock
problems.  It's not 100% appropriate, but it'll do just fine.

Stephen, we can use that frlock in the inode to protect access to the 64-bit
i_size on 32-bit platforms as well.  However in the case of i_size, the
updaters do not need to take the frlock's spinlock, because writers are
serialised inode->i_sem.

So could you please take a look at adding new writer-side primitives to
frlocks to cover this requirement?  See i_size_read/i_size_write from the -aa
kernel:

+static inline void i_size_write(struct inode * inode, loff_t i_size)
+{
+#if BITS_PER_LONG==32 && defined(CONFIG_SMP)
+#ifdef __ARCH_HAS_GET_SET_64BIT
+	set_64bit((unsigned long long *) &inode->i_size, (unsigned long long) i_size);
+#else
+	inode->i_size_version2++;
+	wmb();
+	inode->i_size = i_size;
+	wmb();
+	inode->i_size_version1++;
+	wmb(); /* make it visible ASAP */
+#endif
+#elif BITS_PER_LONG==64 || !defined(CONFIG_SMP)
+	inode->i_size = i_size;
+#endif
+}


Something like this might suffice:

diff -puN include/linux/frlock.h~frlock_write_begin include/linux/frlock.h
--- 25/include/linux/frlock.h~frlock_write_begin	2003-01-25 18:59:27.000000000 -0800
+++ 25-akpm/include/linux/frlock.h	2003-01-25 19:01:27.000000000 -0800
@@ -80,6 +80,22 @@ static inline unsigned fr_read_end(frloc
 	return rw->pre_sequence;
 }
 
+static inline unsigned fr_write_begin(frlock_t *rw)
+{
+	unsigned ret = rw->pre_sequence++;
+	wmb();
+	return ret;
+}	
+
+static inline unsigned fr_write_end(frlock_t *rw) 
+{
+	unsgned ret;
+	wmb();
+	ret = ++(rw->post_sequence);
+	wmb();
+	return ret;
+}
+
 /*
  * Possible sw/hw IRQ protected versions of the interfaces.
  */

Thanks.




Since Jan removed the lock_kernel()s in inode_add_bytes() and
inode_sub_bytes(), these functions have been racy.

One problematic workload has been discovered in which concurrent writepage
and truncate on SMP quickly causes i_blocks to go negative.  writepage() does
not take i_sem, and it seems that for ext2, there are no other locks in
force when inode_add_bytes() is called.

Putting the BKL back in there is not acceptable.  To fix this race I have
used the new frlocks.

This is actually fairly pointless, because these fields are write-mostly and
read-rarely.  But we need a spinlock anyway because of the concurrent
modifiers problem.

And we need this frlock in the inode to solve the problem of the nonatomicity
of i_size.  (But some aditional frlock work is needed first: in the case of
i_size we do not need to take the spinlock because of i_sem protection).

The splitting of the used disk space into i_blocks and i_bytes is silly - we
should nuke all that and just have a bare loff_t i_usedbytes.   Later.


 inode.c    |    1 +
 stat.c     |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 linux/fs.h |   52 +++++++++++++++++-----------------------------------
 ksyms.c    |    4 ++++
 4 files changed, 68 insertions(+), 35 deletions(-)

diff -puN include/linux/fs.h~inode-accounting-race-fix include/linux/fs.h
--- linux-hype/include/linux/fs.h~inode-accounting-race-fix	2003-01-25 18:36:25.000000000 -0800
+++ linux-hype-akpm/include/linux/fs.h	2003-01-25 18:36:25.000000000 -0800
@@ -18,6 +18,7 @@
 #include <linux/stat.h>
 #include <linux/cache.h>
 #include <linux/radix-tree.h>
+#include <linux/frlock.h>
 #include <asm/atomic.h>
 
 struct iovec;
@@ -371,9 +372,19 @@ struct inode {
 	struct timespec		i_ctime;
 	unsigned int		i_blkbits;
 	unsigned long		i_blksize;
-	unsigned long		i_blocks;
 	unsigned long		i_version;
+
+	/*
+	 * i_bytes and i_blocks represent the amount of allocated disk space:
+	 * (i_blocks * 512 + i_bytes) (FIXME: just use an loff_t here).  They
+	 * are protected by i_frlock.
+	 */
+	unsigned long		i_blocks;
 	unsigned short          i_bytes;
+
+	/* Protects i_blocks, i_bytes and, later, i_size */
+	frlock_t		i_frlock;
+
 	struct semaphore	i_sem;
 	struct inode_operations	*i_op;
 	struct file_operations	*i_fop;	/* former ->i_op->default_file_ops */
@@ -400,7 +411,7 @@ struct inode {
 	void			*i_security;
 	__u32			i_generation;
 	union {
-		void				*generic_ip;
+		void		*generic_ip;
 	} u;
 };
 
@@ -412,39 +423,6 @@ struct fown_struct {
 	void *security;
 };
 
-static inline void inode_add_bytes(struct inode *inode, loff_t bytes)
-{
-	inode->i_blocks += bytes >> 9;
-	bytes &= 511;
-	inode->i_bytes += bytes;
-	if (inode->i_bytes >= 512) {
-		inode->i_blocks++;
-		inode->i_bytes -= 512;
-	}
-}
-
-static inline void inode_sub_bytes(struct inode *inode, loff_t bytes)
-{
-	inode->i_blocks -= bytes >> 9;
-	bytes &= 511;
-	if (inode->i_bytes < bytes) {
-		inode->i_blocks--;
-		inode->i_bytes += 512;
-	}
-	inode->i_bytes -= bytes;
-}
-
-static inline loff_t inode_get_bytes(struct inode *inode)
-{
-	return (((loff_t)inode->i_blocks) << 9) + inode->i_bytes;
-}
-
-static inline void inode_set_bytes(struct inode *inode, loff_t bytes)
-{
-	inode->i_blocks = bytes >> 9;
-	inode->i_bytes = bytes & 511;
-}
-
 /*
  * Track a single file's readahead state
  */
@@ -1277,6 +1255,10 @@ extern int page_symlink(struct inode *in
 extern struct inode_operations page_symlink_inode_operations;
 extern void generic_fillattr(struct inode *, struct kstat *);
 extern int vfs_getattr(struct vfsmount *, struct dentry *, struct kstat *);
+void inode_add_bytes(struct inode *inode, loff_t bytes);
+void inode_sub_bytes(struct inode *inode, loff_t bytes);
+loff_t inode_get_bytes(struct inode *inode);
+void inode_set_bytes(struct inode *inode, loff_t bytes);
 
 extern int vfs_readdir(struct file *, filldir_t, void *);
 
diff -puN fs/stat.c~inode-accounting-race-fix fs/stat.c
--- linux-hype/fs/stat.c~inode-accounting-race-fix	2003-01-25 18:36:25.000000000 -0800
+++ linux-hype-akpm/fs/stat.c	2003-01-25 18:36:25.000000000 -0800
@@ -316,3 +316,49 @@ asmlinkage long sys_fstat64(unsigned lon
 }
 
 #endif /* LFS-64 */
+
+void inode_add_bytes(struct inode *inode, loff_t bytes)
+{
+	fr_write_lock(&inode->i_frlock);
+	inode->i_blocks += bytes >> 9;
+	bytes &= 511;
+	inode->i_bytes += bytes;
+	if (inode->i_bytes >= 512) {
+		inode->i_blocks++;
+		inode->i_bytes -= 512;
+	}
+	fr_write_unlock(&inode->i_frlock);
+}
+
+void inode_sub_bytes(struct inode *inode, loff_t bytes)
+{
+	fr_write_lock(&inode->i_frlock);
+	inode->i_blocks -= bytes >> 9;
+	bytes &= 511;
+	if (inode->i_bytes < bytes) {
+		inode->i_blocks--;
+		inode->i_bytes += 512;
+	}
+	inode->i_bytes -= bytes;
+	fr_write_unlock(&inode->i_frlock);
+}
+
+loff_t inode_get_bytes(struct inode *inode)
+{
+	unsigned seq;
+	loff_t ret;
+
+	do {
+		seq = fr_read_begin(&inode->i_frlock);
+		ret = (((loff_t)inode->i_blocks) << 9) + inode->i_bytes;
+	} while (seq != fr_read_end(&inode->i_frlock));
+	return ret;
+}
+
+void inode_set_bytes(struct inode *inode, loff_t bytes)
+{
+	fr_write_lock(&inode->i_frlock);
+	inode->i_blocks = bytes >> 9;
+	inode->i_bytes = bytes & 511;
+	fr_write_unlock(&inode->i_frlock);
+}
diff -puN kernel/ksyms.c~inode-accounting-race-fix kernel/ksyms.c
--- linux-hype/kernel/ksyms.c~inode-accounting-race-fix	2003-01-25 18:36:25.000000000 -0800
+++ linux-hype-akpm/kernel/ksyms.c	2003-01-25 18:36:25.000000000 -0800
@@ -272,6 +272,10 @@ EXPORT_SYMBOL(vfs_fstat);
 EXPORT_SYMBOL(vfs_stat);
 EXPORT_SYMBOL(vfs_lstat);
 EXPORT_SYMBOL(vfs_getattr);
+EXPORT_SYMBOL(inode_add_bytes);
+EXPORT_SYMBOL(inode_sub_bytes);
+EXPORT_SYMBOL(inode_get_bytes);
+EXPORT_SYMBOL(inode_set_bytes);
 EXPORT_SYMBOL(lock_rename);
 EXPORT_SYMBOL(unlock_rename);
 EXPORT_SYMBOL(generic_read_dir);
diff -puN fs/inode.c~inode-accounting-race-fix fs/inode.c
--- linux-hype/fs/inode.c~inode-accounting-race-fix	2003-01-25 18:36:25.000000000 -0800
+++ linux-hype-akpm/fs/inode.c	2003-01-25 18:36:25.000000000 -0800
@@ -176,6 +176,7 @@ void inode_init_once(struct inode *inode
 	spin_lock_init(&inode->i_data.private_lock);
 	INIT_LIST_HEAD(&inode->i_data.i_mmap);
 	INIT_LIST_HEAD(&inode->i_data.i_mmap_shared);
+	frlock_init(&inode->i_frlock);
 }
 
 static void init_once(void * foo, kmem_cache_t * cachep, unsigned long flags)

_