[PATCH] ext4: add a new spinlock i_raw_lock to protect the ext4's raw inode

[PATCH] ext4: add a new spinlock i_raw_lock to protect the ext4's raw inode

[Theodore Ts'o]

To avoid potential data races, use a spinlock which protects the raw
(on-disk) inode.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 fs/ext4/ext4.h  | 2 ++
 fs/ext4/inode.c | 5 ++++-
 fs/ext4/super.c | 1 +
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 86c2cda..1d08a1b 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -875,6 +875,8 @@ struct ext4_inode_info {
 	struct inode vfs_inode;
 	struct jbd2_inode *jinode;
 
+	spinlock_t i_raw_lock;	/* protects updates to the raw inode */
+
 	/*
 	 * File creation time. Its function is same as that of
 	 * struct timespec i_{a,c,m}time in the generic inode.
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 2eb5fad..1ee8ff6 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -4311,7 +4311,9 @@ static int ext4_do_update_inode(handle_t *handle,
 	uid_t i_uid;
 	gid_t i_gid;
 
-	/* For fields not not tracking in the in-memory inode,
+	spin_lock(&ei->i_raw_lock);
+
+	/* For fields not tracked in the in-memory inode,
 	 * initialise them to zero for new inodes. */
 	if (ext4_test_inode_state(inode, EXT4_STATE_NEW))
 		memset(raw_inode, 0, EXT4_SB(inode->i_sb)->s_inode_size);
@@ -4419,6 +4421,7 @@ static int ext4_do_update_inode(handle_t *handle,
 
 	ext4_update_inode_fsync_trans(handle, inode, need_datasync);
 out_brelse:
+	spin_unlock(&ei->i_raw_lock);
 	brelse(bh);
 	ext4_std_error(inode->i_sb, err);
 	return err;
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index c4895c1..1f8cb18 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -879,6 +879,7 @@ static struct inode *ext4_alloc_inode(struct super_block *sb)
 		return NULL;
 
 	ei->vfs_inode.i_version = 1;
+	spin_lock_init(&ei->i_raw_lock);
 	INIT_LIST_HEAD(&ei->i_prealloc_list);
 	spin_lock_init(&ei->i_prealloc_lock);
 	ext4_es_init_tree(&ei->i_es_tree);
-- 
1.9.0

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 1/3] generic/004: fix filtering of expected error message

[Theodore Ts'o]

The failure message goes to stderr, so we need to redirect stderr to
stdout before running sed.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
Cc: Christoph Hellwig <hch@infradead.org>
---
 tests/generic/004     | 2 +-
 tests/generic/004.out | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/generic/004 b/tests/generic/004
index 406c29e..23ca349 100755
--- a/tests/generic/004
+++ b/tests/generic/004
@@ -58,7 +58,7 @@ $XFS_IO_PROG -T \
 rm ${testfile}
 
 # test creating a r/o tmpfile.  Should fail
-$XFS_IO_PROG -Tr ${TEST_DIR} -c "close" | _filter_test_dir
+$XFS_IO_PROG -Tr ${TEST_DIR} -c "close" 2>&1 | _filter_test_dir
 
 # success, all done
 status=0
diff --git a/tests/generic/004.out b/tests/generic/004.out
index b85c11c..527b2c2 100644
--- a/tests/generic/004.out
+++ b/tests/generic/004.out
@@ -3,4 +3,4 @@ wrote 4096/4096 bytes at offset 0
 XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
 read 4096/4096 bytes at offset 0
 XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
-/mnt/test: Invalid argument
+TEST_DIR: Invalid argument
-- 
1.9.0

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 1/3] generic/004: fix filtering of expected error message

[Eric Sandeen]

On 4/12/14, 5:55 PM, Theodore Ts'o wrote:
> The failure message goes to stderr, so we need to redirect stderr to
> stdout before running sed.

Heh, whoops.

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> Cc: Christoph Hellwig <hch@infradead.org>
> ---
>  tests/generic/004     | 2 +-
>  tests/generic/004.out | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/generic/004 b/tests/generic/004
> index 406c29e..23ca349 100755
> --- a/tests/generic/004
> +++ b/tests/generic/004
> @@ -58,7 +58,7 @@ $XFS_IO_PROG -T \
>  rm ${testfile}
>  
>  # test creating a r/o tmpfile.  Should fail
> -$XFS_IO_PROG -Tr ${TEST_DIR} -c "close" | _filter_test_dir
> +$XFS_IO_PROG -Tr ${TEST_DIR} -c "close" 2>&1 | _filter_test_dir
>  
>  # success, all done
>  status=0
> diff --git a/tests/generic/004.out b/tests/generic/004.out
> index b85c11c..527b2c2 100644
> --- a/tests/generic/004.out
> +++ b/tests/generic/004.out
> @@ -3,4 +3,4 @@ wrote 4096/4096 bytes at offset 0
>  XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  read 4096/4096 bytes at offset 0
>  XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> -/mnt/test: Invalid argument
> +TEST_DIR: Invalid argument
> 

Re: [PATCH 1/3] generic/004: fix filtering of expected error message

[Eric Sandeen]

On 4/12/14, 5:55 PM, Theodore Ts'o wrote:
> The failure message goes to stderr, so we need to redirect stderr to
> stdout before running sed.

Heh, whoops.

Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> Cc: Christoph Hellwig <hch@infradead.org>
> ---
>  tests/generic/004     | 2 +-
>  tests/generic/004.out | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/generic/004 b/tests/generic/004
> index 406c29e..23ca349 100755
> --- a/tests/generic/004
> +++ b/tests/generic/004
> @@ -58,7 +58,7 @@ $XFS_IO_PROG -T \
>  rm ${testfile}
>  
>  # test creating a r/o tmpfile.  Should fail
> -$XFS_IO_PROG -Tr ${TEST_DIR} -c "close" | _filter_test_dir
> +$XFS_IO_PROG -Tr ${TEST_DIR} -c "close" 2>&1 | _filter_test_dir
>  
>  # success, all done
>  status=0
> diff --git a/tests/generic/004.out b/tests/generic/004.out
> index b85c11c..527b2c2 100644
> --- a/tests/generic/004.out
> +++ b/tests/generic/004.out
> @@ -3,4 +3,4 @@ wrote 4096/4096 bytes at offset 0
>  XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
>  read 4096/4096 bytes at offset 0
>  XXX Bytes, X ops; XX:XX:XX.X (XXX YYY/sec and XXX ops/sec)
> -/mnt/test: Invalid argument
> +TEST_DIR: Invalid argument
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 2/3] generic/022: update expected output after the test was renamed

After generic/022 was renamed by commit 21723cdb, the wrong test
number was put into generic/022.out.  Fix this.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
Cc: Lukas Czerner <lczerner@redhat.com>
---
 tests/generic/022.out | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/generic/022.out b/tests/generic/022.out
index 96c2d33..fbffa59 100644
--- a/tests/generic/022.out
+++ b/tests/generic/022.out
@@ -1,4 +1,4 @@
-QA output created by 009
+QA output created by 022
 	1. into a hole
 f4f35d60b3cc18aaa6d8d92f0cd3708a
 	2. into allocated space
-- 
1.9.0

[PATCH 2/3] generic/022: update expected output after the test was renamed

[Theodore Ts'o]

After generic/022 was renamed by commit 21723cdb, the wrong test
number was put into generic/022.out.  Fix this.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
Cc: Lukas Czerner <lczerner@redhat.com>
---
 tests/generic/022.out | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/generic/022.out b/tests/generic/022.out
index 96c2d33..fbffa59 100644
--- a/tests/generic/022.out
+++ b/tests/generic/022.out
@@ -1,4 +1,4 @@
-QA output created by 009
+QA output created by 022
 	1. into a hole
 f4f35d60b3cc18aaa6d8d92f0cd3708a
 	2. into allocated space
-- 
1.9.0

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 2/3] generic/022: update expected output after the test was renamed

[Eric Sandeen]

On 4/12/14, 5:55 PM, Theodore Ts'o wrote:
> After generic/022 was renamed by commit 21723cdb, the wrong test
> number was put into generic/022.out.  Fix this.
> 
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> Cc: Lukas Czerner <lczerner@redhat.com>

Lukas sent this a couple days ago as well.

In any case, again, Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  tests/generic/022.out | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/generic/022.out b/tests/generic/022.out
> index 96c2d33..fbffa59 100644
> --- a/tests/generic/022.out
> +++ b/tests/generic/022.out
> @@ -1,4 +1,4 @@
> -QA output created by 009
> +QA output created by 022
>  	1. into a hole
>  f4f35d60b3cc18aaa6d8d92f0cd3708a
>  	2. into allocated space
> 

Re: [PATCH 2/3] generic/022: update expected output after the test was renamed

[Eric Sandeen]

On 4/12/14, 5:55 PM, Theodore Ts'o wrote:
> After generic/022 was renamed by commit 21723cdb, the wrong test
> number was put into generic/022.out.  Fix this.
> 
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> Cc: Lukas Czerner <lczerner@redhat.com>

Lukas sent this a couple days ago as well.

In any case, again, Reviewed-by: Eric Sandeen <sandeen@redhat.com>

> ---
>  tests/generic/022.out | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tests/generic/022.out b/tests/generic/022.out
> index 96c2d33..fbffa59 100644
> --- a/tests/generic/022.out
> +++ b/tests/generic/022.out
> @@ -1,4 +1,4 @@
> -QA output created by 009
> +QA output created by 022
>  	1. into a hole
>  f4f35d60b3cc18aaa6d8d92f0cd3708a
>  	2. into allocated space
> 

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH 3/3] generic/237: fix filtering for expected failure message

[Theodore Ts'o]

Newer kernels return EACCES instead of EPERM when modifying an acl
fails.  Update the filtering to handle this.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 tests/generic/237     | 2 +-
 tests/generic/237.out | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/generic/237 b/tests/generic/237
index 6a61ec8..b068150 100755
--- a/tests/generic/237
+++ b/tests/generic/237
@@ -68,7 +68,7 @@ touch file1
 chown $acl1.$acl1 file1
 
 echo "Expect to FAIL"
-$runas -u $acl2 -g $acl2 -- `which setfacl` -m u::rwx file1 2>&1 | sed 's/^setfacl: \/.*file1: Operation not permitted$/setfacl: file1: Operation not permitted/'
+$runas -u $acl2 -g $acl2 -- `which setfacl` -m u::rwx file1 2>&1 | sed -e 's/Permission denied/Operation not permitted/' -e 's/file1: //' -e 's,[/a-zA-Z]*setfacl: ,setfacl: ,'
 
 echo "Test over."
 # success, all done
diff --git a/tests/generic/237.out b/tests/generic/237.out
index 09d98c6..4edafa6 100644
--- a/tests/generic/237.out
+++ b/tests/generic/237.out
@@ -1,4 +1,4 @@
 QA output created by 237
 Expect to FAIL
-setfacl: file1: Operation not permitted
+setfacl: Operation not permitted
 Test over.
-- 
1.9.0

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/3] generic/237: fix filtering for expected failure message

[Christoph Hellwig]

On Sat, Apr 12, 2014 at 06:55:17PM -0400, Theodore Ts'o wrote:
> Newer kernels return EACCES instead of EPERM when modifying an acl
> fails.  Update the filtering to handle this.
> 
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>


I've not seen this yet.  What exact configuration do you use to get the
error?

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/3] generic/237: fix filtering for expected failure message

[Theodore Ts'o]

On Sun, Apr 13, 2014 at 01:17:38AM -0700, Christoph Hellwig wrote:
> On Sat, Apr 12, 2014 at 06:55:17PM -0400, Theodore Ts'o wrote:
> > Newer kernels return EACCES instead of EPERM when modifying an acl
> > fails.  Update the filtering to handle this.
> > 
> > Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> 
> I've not seen this yet.  What exact configuration do you use to get the
> error?

I was using a 32-bit kernel, using a recent Linus upstream.  The bug
was showing up for both ext4 and xfs, and xfstests-bld is using a very
recent version of the acl package?

If you're not seeing it, perhaps the most likely cause would be the
fact that I was using an i386 kernel, and/or the version of the acl
userspace package found in kvm-xfstests?

If you want a repro, you could grab:

	git://git.kernel.org/pub/scm/fs/ext2/xfstests-bld.git

and then building the per the README.

If you don't have a Debian testing system conviently to hand, you
could pull down:

https://www.kernel.org/pub/linux/kernel/people/tytso/kvm-xfstests/root_fs.img.i386

... and then update the image to have the xfstests upstream version of
/root/xfstests/tests/generic/237*.

Cheers,

						- Ted

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/3] generic/237: fix filtering for expected failure message

[Dave Chinner]

On Sat, Apr 12, 2014 at 06:55:17PM -0400, Theodore Ts'o wrote:
> Newer kernels return EACCES instead of EPERM when modifying an acl
> fails.  Update the filtering to handle this.
> 
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
> ---
>  tests/generic/237     | 2 +-
>  tests/generic/237.out | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/generic/237 b/tests/generic/237
> index 6a61ec8..b068150 100755
> --- a/tests/generic/237
> +++ b/tests/generic/237
> @@ -68,7 +68,7 @@ touch file1
>  chown $acl1.$acl1 file1
>  
>  echo "Expect to FAIL"
> -$runas -u $acl2 -g $acl2 -- `which setfacl` -m u::rwx file1 2>&1 | sed 's/^setfacl: \/.*file1: Operation not permitted$/setfacl: file1: Operation not permitted/'
> +$runas -u $acl2 -g $acl2 -- `which setfacl` -m u::rwx file1 2>&1 | sed -e 's/Permission denied/Operation not permitted/' -e 's/file1: //' -e 's,[/a-zA-Z]*setfacl: ,setfacl: ,'

This is getting unweildy. Can you factor this into a local
_filter_setfacl_error function of some kind? It would also help if
you put a comment iterating all the different output formats it's
now trying to filter out, too.

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH 3/3] generic/237: fix filtering for expected failure message

[Theodore Ts'o]

After looking at this more closely, this patch isn't needed.  It was a
configuration screw up on my end.

					- Ted

Re: [PATCH 3/3] generic/237: fix filtering for expected failure message

[Theodore Ts'o]

After looking at this more closely, this patch isn't needed.  It was a
configuration screw up on my end.

					- Ted

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

Re: [PATCH] ext4: add a new spinlock i_raw_lock to protect the ext4's raw inode

[Theodore Ts'o]

On Sat, Apr 12, 2014 at 06:55:14PM -0400, Theodore Ts'o wrote:
> To avoid potential data races, use a spinlock which protects the raw
> (on-disk) inode.
> 
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>

Sigh, sorry, left over patch in the /tmp/p directory.  Please ignore....

      	     	       	     	    - Ted

Re: [PATCH] ext4: add a new spinlock i_raw_lock to protect the ext4's raw inode

[Theodore Ts'o]

On Sat, Apr 12, 2014 at 06:55:14PM -0400, Theodore Ts'o wrote:
> To avoid potential data races, use a spinlock which protects the raw
> (on-disk) inode.
> 
> Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>

Sigh, sorry, left over patch in the /tmp/p directory.  Please ignore....

      	     	       	     	    - Ted

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs

[PATCH] ext4: add a new spinlock i_raw_lock to protect the ext4's raw inode

[Theodore Ts'o]

To avoid potential data races, use a spinlock which protects the raw
(on-disk) inode.

Signed-off-by: "Theodore Ts'o" <tytso@mit.edu>
---
 fs/ext4/ext4.h  | 2 ++
 fs/ext4/inode.c | 5 ++++-
 fs/ext4/super.c | 1 +
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
index 86c2cda..1d08a1b 100644
--- a/fs/ext4/ext4.h
+++ b/fs/ext4/ext4.h
@@ -875,6 +875,8 @@ struct ext4_inode_info {
 	struct inode vfs_inode;
 	struct jbd2_inode *jinode;
 
+	spinlock_t i_raw_lock;	/* protects updates to the raw inode */
+
 	/*
 	 * File creation time. Its function is same as that of
 	 * struct timespec i_{a,c,m}time in the generic inode.
diff --git a/fs/ext4/inode.c b/fs/ext4/inode.c
index 2eb5fad..1ee8ff6 100644
--- a/fs/ext4/inode.c
+++ b/fs/ext4/inode.c
@@ -4311,7 +4311,9 @@ static int ext4_do_update_inode(handle_t *handle,
 	uid_t i_uid;
 	gid_t i_gid;
 
-	/* For fields not not tracking in the in-memory inode,
+	spin_lock(&ei->i_raw_lock);
+
+	/* For fields not tracked in the in-memory inode,
 	 * initialise them to zero for new inodes. */
 	if (ext4_test_inode_state(inode, EXT4_STATE_NEW))
 		memset(raw_inode, 0, EXT4_SB(inode->i_sb)->s_inode_size);
@@ -4419,6 +4421,7 @@ static int ext4_do_update_inode(handle_t *handle,
 
 	ext4_update_inode_fsync_trans(handle, inode, need_datasync);
 out_brelse:
+	spin_unlock(&ei->i_raw_lock);
 	brelse(bh);
 	ext4_std_error(inode->i_sb, err);
 	return err;
diff --git a/fs/ext4/super.c b/fs/ext4/super.c
index c4895c1..1f8cb18 100644
--- a/fs/ext4/super.c
+++ b/fs/ext4/super.c
@@ -879,6 +879,7 @@ static struct inode *ext4_alloc_inode(struct super_block *sb)
 		return NULL;
 
 	ei->vfs_inode.i_version = 1;
+	spin_lock_init(&ei->i_raw_lock);
 	INIT_LIST_HEAD(&ei->i_prealloc_list);
 	spin_lock_init(&ei->i_prealloc_lock);
 	ext4_es_init_tree(&ei->i_es_tree);
-- 
1.9.0