xfs: error sign fixes for 3.15-rc6
cover xfs: error sign fixes for 3.15-rc6
Hi folks,
I've been working through the mess that is the
error sign impedance mismatch between the core XFS code and the rest of the kernel. I'm about half way through the codebase, and I've found a bunch of incorrect error signs throughout the VFS interface layers.
These were effectively all found by inspection, and is further evidence that we need to convert all of XFS to negative errors as quickly as possible. It is clear that we are missing these on review, and changes made by developers adding functionality to the VFS and hence just touching the outer layers of XFS are simply not aware of these issues in the first place.
This patchset is based on v3.15-rc5 because some of the problems that need to be fixed have merged into mainline via the ext4 tree (COLLAPSE_RANGE changes). Hence I'll need to update the master branch of the XFS tree to v3.15-rc5 before I merge these and push them to Linus. I'm considering them as important fixes as things like xfs_dir_fsync() and xfs_commit_metadata() returning incorrect errnos mean data integrity operation failures may not be detected correctly by applications.....
Comments, thoughts, testing all welcome!
Cheers,
Dave.
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 1/9 xfs: xfs_dir_fsync() returns positive errno
From: Dave Chinner <[email protected]>
And it should be negative.
--- fs/xfs/xfs_file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
fs/xfs/xfs_file.c +1 -1
@@ -155,7 +155,7 @@ xfs_dir_fsync(if (!lsn)return 0 ; -return _xfs_log_force_lsn (mp, lsn,XFS_LOG_SYNC ,NULL ); +return -_xfs_log_force_lsn (mp, lsn,XFS_LOG_SYNC ,NULL ); }STATIC int
Signature · 6 lines
1.9.0 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 1/9] xfs: xfs_dir_fsync() returns positive errno
Quoted 15 lines · Dave Chinner
From: Dave Chinner <[email protected]>
And it should be negative.
Signed-off-by Dave Chinner <[email protected]>--- fs/xfs/xfs_file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)fs/xfs/xfs_file.c +0
-0
fs/xfs/xfs_file.c xfs_dir_fsync( · line 161
Quoted 8 lines
fs/xfs/xfs_file.c +1
-1@@ -155,7 +155,7 @@ xfs_dir_fsync(if (!lsn)return 0 ; -return _xfs_log_force_lsn (mp, lsn,XFS_LOG_SYNC ,NULL ); +return -_xfs_log_force_lsn (mp, lsn,XFS_LOG_SYNC ,NULL ); }STATIC int
Cheers,
-Jeff
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 2/9 xfs: fix incorrect error sign in xfs_file_aio_read
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
--- fs/xfs/xfs_file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
fs/xfs/xfs_file.c +1 -1
@@ -295,7 +295,7 @@ xfs_file_aio_read(xfs_rw_ilock (ip,XFS_IOLOCK_EXCL );if (inode->i_mapping->nrpages) { - ret = -filemap_write_and_wait_range ( + ret =filemap_write_and_wait_range (VFS_I (ip)->i_mapping, pos,-1 );if (ret) {
Signature · 6 lines
1.9.0 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 2/9] xfs: fix incorrect error sign in xfs_file_aio_read
Quoted 13 lines · Dave Chinner
From: Dave Chinner <[email protected]>
Signed-off-by Dave Chinner <[email protected]>--- fs/xfs/xfs_file.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)fs/xfs/xfs_file.c +0
-0
fs/xfs/xfs_file.c xfs_file_aio_read( · line 301
Quoted 8 lines
fs/xfs/xfs_file.c +1
-1@@ -295,7 +295,7 @@ xfs_file_aio_read(xfs_rw_ilock (ip,XFS_IOLOCK_EXCL );if (inode->i_mapping->nrpages) { - ret = -filemap_write_and_wait_range ( + ret =filemap_write_and_wait_range (VFS_I (ip)->i_mapping, pos,-1 );if (ret) {
Cheers,
-Jeff
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 3/9 xfs: xfs_commit_metadata returns wrong errno
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
Invert it.
--- fs/xfs/xfs_export.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
fs/xfs/xfs_export.c +1 -1
@@ -237,7 +237,7 @@ xfs_fs_nfs_commit_metadata(if (!lsn)return 0 ; -return _xfs_log_force_lsn (mp, lsn,XFS_LOG_SYNC ,NULL ); +return -_xfs_log_force_lsn (mp, lsn,XFS_LOG_SYNC ,NULL ); }const struct export_operations xfs_export_operations = {
Signature · 6 lines
1.9.0 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 3/9] xfs: xfs_commit_metadata returns wrong errno
Quoted 15 lines · Dave Chinner
From: Dave Chinner <[email protected]>
Invert it.
Signed-off-by Dave Chinner <[email protected]>--- fs/xfs/xfs_export.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)fs/xfs/xfs_export.c +0
-0
fs/xfs/xfs_export.c xfs_fs_nfs_commit_metadata( · line 243
Quoted 8 lines
fs/xfs/xfs_export.c +1
-1@@ -237,7 +237,7 @@ xfs_fs_nfs_commit_metadata(if (!lsn)return 0 ; -return _xfs_log_force_lsn (mp, lsn,XFS_LOG_SYNC ,NULL ); +return -_xfs_log_force_lsn (mp, lsn,XFS_LOG_SYNC ,NULL ); }const struct export_operations xfs_export_operations = {
Cheers,
-Jeff
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 4/9 xfs: correct error sign on COLLAPSE_RANGE errors
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
--- fs/xfs/xfs_file.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
fs/xfs/xfs_file.c +2 -2
@@ -837,7 +837,7 @@ xfs_file_fallocate(unsigned blksize_mask = (1 << inode->i_blkbits) -1 ;if (offset & blksize_mask || len & blksize_mask) { - error = -EINVAL ; + error =EINVAL ; goto out_unlock; }
@@ -846,7 +846,7 @@ xfs_file_fallocate(* in which case it is effectively a truncate operation */ if (offset + len >=i_size_read (inode)) { - error = -EINVAL ; + error =EINVAL ; goto out_unlock; }
Signature · 6 lines
1.9.0 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 4/9] xfs: correct error sign on COLLAPSE_RANGE errors
Quoted 22 lines · Dave Chinner
From: Dave Chinner <[email protected]>
Signed-off-by Dave Chinner <[email protected]>--- fs/xfs/xfs_file.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)fs/xfs/xfs_file.c +1
-1@@ -837,7 +837,7 @@ xfs_file_fallocate(unsigned blksize_mask = (1 << inode->i_blkbits) -1 ;if (offset & blksize_mask || len & blksize_mask) { - error = -EINVAL ; + error =EINVAL ; goto out_unlock; }
fs/xfs/xfs_file.c xfs_file_fallocate( · line 852
Quoted 8 lines
fs/xfs/xfs_file.c +1
-1@@ -846,7 +846,7 @@ xfs_file_fallocate(* in which case it is effectively a truncate operation */ if (offset + len >=i_size_read (inode)) { - error = -EINVAL ; + error =EINVAL ; goto out_unlock; }
Cheers,
-Jeff
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 5/9 xfs: fix wrong errno from xfs_initxattrs
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
--- fs/xfs/xfs_iops.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
fs/xfs/xfs_iops.c +4 -4
@@ -72,8 +72,8 @@ xfs_initxattrs(int error =0 ;for (xattr = xattr_array; xattr->name !=NULL ; xattr++) { - error =xfs_attr_set (ip, xattr->name, xattr->value, - xattr->value_len,ATTR_SECURE ); + error = -xfs_attr_set (ip, xattr->name, xattr->value, + xattr->value_len,ATTR_SECURE );if (error <0 )break ; }
@@ -93,8 +93,8 @@ xfs_init_security(struct inode *dir, const struct qstr *qstr) { - return security_inode_init_security(inode, dir, qstr, - &xfs_initxattrs, NULL); + return -security_inode_init_security(inode, dir, qstr, + &xfs_initxattrs, NULL); } static void
Signature · 6 lines
1.9.0 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 5/9] xfs: fix wrong errno from xfs_initxattrs
Quoted 24 lines · Dave Chinner
From: Dave Chinner <[email protected]>
Signed-off-by Dave Chinner <[email protected]>--- fs/xfs/xfs_iops.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)fs/xfs/xfs_iops.c +2
-2@@ -72,8 +72,8 @@ xfs_initxattrs(int error =0 ;for (xattr = xattr_array; xattr->name !=NULL ; xattr++) { - error =xfs_attr_set (ip, xattr->name, xattr->value, - xattr->value_len,ATTR_SECURE ); + error = -xfs_attr_set (ip, xattr->name, xattr->value, + xattr->value_len,ATTR_SECURE );if (error <0 )break ; }
fs/xfs/xfs_iops.c xfs_init_security( · line 100
Quoted 10 lines
fs/xfs/xfs_iops.c +2
-2@@ -93,8 +93,8 @@ xfs_init_security(struct inode *dir, const struct qstr *qstr) { - return security_inode_init_security(inode, dir, qstr, - &xfs_initxattrs, NULL); + return -security_inode_init_security(inode, dir, qstr, + &xfs_initxattrs, NULL); } static void
Cheers,
-Jeff
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 6/9 xfs: fix wrong err sign on xfs_set_acl()
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
--- fs/xfs/xfs_iops.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
fs/xfs/xfs_iops.c +2 -2
@@ -173,12 +173,12 @@ xfs_generic_create(#ifdef CONFIG_XFS_POSIX_ACL if (default_acl) { - error =xfs_set_acl (inode, default_acl,ACL_TYPE_DEFAULT ); + error = -xfs_set_acl (inode, default_acl,ACL_TYPE_DEFAULT );if (error) goto out_cleanup_inode; }if (acl) { - error =xfs_set_acl (inode, acl,ACL_TYPE_ACCESS ); + error = -xfs_set_acl (inode, acl,ACL_TYPE_ACCESS );if (error) goto out_cleanup_inode; }
Signature · 6 lines
1.9.0 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 6/9] xfs: fix wrong err sign on xfs_set_acl()
Quoted 17 lines · Dave Chinner
From: Dave Chinner <[email protected]>
Signed-off-by Dave Chinner <[email protected]>--- fs/xfs/xfs_iops.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)fs/xfs/xfs_iops.c +0
-1@@ -173,12 +173,12 @@ xfs_generic_create(#ifdef CONFIG_XFS_POSIX_ACL if (default_acl) { - error =xfs_set_acl (inode, default_acl,ACL_TYPE_DEFAULT );
fs/xfs/xfs_iops.c xfs_generic_create( · line 184
Quoted 10 lines
fs/xfs/xfs_iops.c +2
-1+ error = -xfs_set_acl (inode, default_acl,ACL_TYPE_DEFAULT );if (error) goto out_cleanup_inode; }if (acl) { - error =xfs_set_acl (inode, acl,ACL_TYPE_ACCESS ); + error = -xfs_set_acl (inode, acl,ACL_TYPE_ACCESS );if (error) goto out_cleanup_inode; }
Cheers,
-Jeff
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 7/9 xfs: negate mount workqueue init error value
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
--- fs/xfs/xfs_super.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
fs/xfs/xfs_super.c +1 -1
@@ -1433,7 +1433,7 @@ xfs_fs_fill_super(if (error) goto out_free_fsname; - error =xfs_init_mount_workqueues (mp); + error = -xfs_init_mount_workqueues (mp);if (error) goto out_close_devices;
Signature · 6 lines
1.9.0 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 7/9] xfs: negate mount workqueue init error value
Quoted 13 lines · Dave Chinner
From: Dave Chinner <[email protected]>
Signed-off-by Dave Chinner <[email protected]>--- fs/xfs/xfs_super.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)fs/xfs/xfs_super.c +0
-0
fs/xfs/xfs_super.c xfs_fs_fill_super( · line 1439
Quoted 8 lines
fs/xfs/xfs_super.c +1
-1@@ -1433,7 +1433,7 @@ xfs_fs_fill_super(if (error) goto out_free_fsname; - error =xfs_init_mount_workqueues (mp); + error = -xfs_init_mount_workqueues (mp);if (error) goto out_close_devices;
Cheers,
-Jeff
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 8/9 xfs: negate xfs_icsb_init_counters error value
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
--- fs/xfs/xfs_super.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
fs/xfs/xfs_super.c +1 -1
@@ -1437,7 +1437,7 @@ xfs_fs_fill_super(if (error) goto out_close_devices; - error =xfs_icsb_init_counters (mp); + error = -xfs_icsb_init_counters (mp);if (error) goto out_destroy_workqueues;
Signature · 6 lines
1.9.0 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 8/9] xfs: negate xfs_icsb_init_counters error value
Quoted 13 lines · Dave Chinner
From: Dave Chinner <[email protected]>
Signed-off-by Dave Chinner <[email protected]>--- fs/xfs/xfs_super.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)fs/xfs/xfs_super.c +0
-0
fs/xfs/xfs_super.c xfs_fs_fill_super( · line 1443
Quoted 8 lines
fs/xfs/xfs_super.c +1
-1@@ -1437,7 +1437,7 @@ xfs_fs_fill_super(if (error) goto out_close_devices; - error =xfs_icsb_init_counters (mp); + error = -xfs_icsb_init_counters (mp);if (error) goto out_destroy_workqueues;
Cheers,
-Jeff
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
PATCH 9/9 xfs: list_lru_init returns a negative error
In reply to Dave ChinnerFrom: Dave Chinner <[email protected]>
And we don't invert it properly when initialising the dquot lru list.
--- fs/xfs/xfs_qm.c | 26 ++++++++++++++------------ 1 file changed, 14 insertions(+), 12 deletions(-)
fs/xfs/xfs_qm.c +14 -12
@@ -843,22 +843,17 @@ xfs_qm_init_quotainfo( qinf = mp->m_quotainfo =kmem_zalloc (sizeof (xfs_quotainfo_t),KM_SLEEP ); -if ((error =list_lru_init (&qinf->qi_lru))) { -kmem_free (qinf); - mp->m_quotainfo =NULL ; -return error; - } + error = -list_lru_init (&qinf->qi_lru); +if (error) + goto out_free_qinf;/* * See if quotainodes are setup, and if not, allocate them, * and change the superblock accordingly. */ -if ((error =xfs_qm_init_quotainos (mp))) { -list_lru_destroy (&qinf->qi_lru); -kmem_free (qinf); - mp->m_quotainfo =NULL ; -return error; - } + error =xfs_qm_init_quotainos (mp); +if (error) + goto out_free_lru;INIT_RADIX_TREE (&qinf->qi_uquota_tree,GFP_NOFS );INIT_RADIX_TREE (&qinf->qi_gquota_tree,GFP_NOFS );
@@ -918,7 +913,7 @@ xfs_qm_init_quotainfo( qinf->qi_isoftlimit =be64_to_cpu (ddqp->d_ino_softlimit); qinf->qi_rtbhardlimit =be64_to_cpu (ddqp->d_rtb_hardlimit); qinf->qi_rtbsoftlimit =be64_to_cpu (ddqp->d_rtb_softlimit); - +xfs_qm_dqdestroy (dqp); }else { qinf->qi_btimelimit =XFS_QM_BTIMELIMIT ;
@@ -935,6 +930,13 @@ xfs_qm_init_quotainfo( qinf->qi_shrinker.flags =SHRINKER_NUMA_AWARE ;register_shrinker (&qinf->qi_shrinker);return 0 ; + +out_free_lru: +list_lru_destroy (&qinf->qi_lru); +out_free_qinf: +kmem_free (qinf); + mp->m_quotainfo =NULL ; +return error; }
Signature · 6 lines
1.9.0 _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs
Re: [PATCH 9/9] xfs: list_lru_init returns a negative error
Quoted 6 lines · Dave Chinner
From: Dave Chinner <[email protected]>
And we don't invert it properly when initialising the dquot lru list.
Signed-off-by Dave Chinner <[email protected]>
Cheers,
-Jeff
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Hi Dave,
Quoted 11 lines · Dave Chinner
Hi folks,
I've been working through the mess that is the
error sign impedance mismatch between the core XFS code and the rest of the kernel. I'm about half way through the codebase, and I've found a bunch of incorrect error signs throughout the VFS interface layers.These were effectively all found by inspection, and is further evidence that we need to convert all of XFS to negative errors as quickly as possible.
Would you like to find a volunteer to deal with that? I have a colleague is trying to study XFS these days, looks this is a good task to him.
BTW, before doing negative errors converting, should we have libxfs infrastructure support at first?
Cheers,
-Jeff
_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs
Quoted 11 lines · Jeff Liu
Hi Dave,
Quoted 8 lines · Dave Chinner
Hi folks,
I've been working through the mess that is the
error sign impedance mismatch between the core XFS code and the rest of the kernel. I'm about half way through the codebase, and I've found a bunch of incorrect error signs throughout the VFS interface layers.
Quoted 6 lines
Quoted 3 lines
These were effectively all found by inspection, and is further evidence that we need to convert all of XFS to negative errors as quickly as possible.
Would you like to find a volunteer to deal with that? I have a colleague is trying to study XFS these days, looks this is a good task to him.
Thanks for the offer, but I've already done half the conversion of the codebase. Hence I may as well finish it. Indeed, The sooner I've done it, the sooner we can get it into the dev tree for testing....
Quoted 2 lines
BTW, before doing negative errors converting, should we have libxfs infrastructure support at first?
Makes no difference - the libxfs structural changes are just moving files about and futzing with makefiles. It'll only take me an hour to redo those patches from scratch, so it doesn't matter what state the code base is in to start with. I'd prefer that the kernel libxfs uses negative errors from the start, though ;)
Cheers,
Dave.
Signature · 7 lines
Dave Chinner [email protected] _______________________________________________ xfs mailing list [email protected] http://oss.sgi.com/mailman/listinfo/xfs