* [PATCH 1/5] xfs: always take the iolock around xfs_setattr_size
2013-10-12 7:55 [PATCH 0/5] refactor the preallocation and hole punching code Christoph Hellwig
@ 2013-10-12 7:55 ` Christoph Hellwig
2013-10-14 4:50 ` Dave Chinner
2013-10-14 14:09 ` [PATCH 1/5 v3] " Christoph Hellwig
2013-10-12 7:55 ` [PATCH 2/5] xfs: remove the unused XFS_ATTR_NONBLOCK flag Christoph Hellwig
` (4 subsequent siblings)
5 siblings, 2 replies; 24+ messages in thread
From: Christoph Hellwig @ 2013-10-12 7:55 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-cleanup-xfs_setattr_size-locking --]
[-- Type: text/plain, Size: 3946 bytes --]
There is no reason to conditionally take the iolock inside xfs_setattr_size
when we can let the caller handle it unconditionally, which just incrases
the lock hold time for the case where it was previously taken internally
by a few instructions.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_bmap_util.c | 3 +--
fs/xfs/xfs_file.c | 2 +-
fs/xfs/xfs_iops.c | 29 ++++++++++++++++-------------
fs/xfs/xfs_iops.h | 2 +-
4 files changed, 19 insertions(+), 17 deletions(-)
Index: xfs/fs/xfs/xfs_file.c
===================================================================
--- xfs.orig/fs/xfs/xfs_file.c 2013-10-01 21:20:47.564230097 +0200
+++ xfs/fs/xfs/xfs_file.c 2013-10-01 21:20:54.312230257 +0200
@@ -852,7 +852,7 @@ xfs_file_fallocate(
iattr.ia_valid = ATTR_SIZE;
iattr.ia_size = new_size;
- error = -xfs_setattr_size(ip, &iattr, XFS_ATTR_NOLOCK);
+ error = -xfs_setattr_size(ip, &iattr);
}
out_unlock:
Index: xfs/fs/xfs/xfs_iops.c
===================================================================
--- xfs.orig/fs/xfs/xfs_iops.c 2013-10-01 21:20:47.564230097 +0200
+++ xfs/fs/xfs/xfs_iops.c 2013-10-01 21:20:54.316230257 +0200
@@ -709,8 +709,7 @@ out_dqrele:
int
xfs_setattr_size(
struct xfs_inode *ip,
- struct iattr *iattr,
- int flags)
+ struct iattr *iattr)
{
struct xfs_mount *mp = ip->i_mount;
struct inode *inode = VFS_I(ip);
@@ -733,15 +732,11 @@ xfs_setattr_size(
if (error)
return XFS_ERROR(error);
+ ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
ASSERT(S_ISREG(ip->i_d.di_mode));
ASSERT((mask & (ATTR_UID|ATTR_GID|ATTR_ATIME|ATTR_ATIME_SET|
ATTR_MTIME_SET|ATTR_KILL_PRIV|ATTR_TIMES_SET)) == 0);
- if (!(flags & XFS_ATTR_NOLOCK)) {
- lock_flags |= XFS_IOLOCK_EXCL;
- xfs_ilock(ip, lock_flags);
- }
-
oldsize = inode->i_size;
newsize = iattr->ia_size;
@@ -755,7 +750,6 @@ xfs_setattr_size(
/*
* Use the regular setattr path to update the timestamps.
*/
- xfs_iunlock(ip, lock_flags);
iattr->ia_valid &= ~ATTR_SIZE;
return xfs_setattr_nonsize(ip, iattr, 0);
}
@@ -916,12 +910,21 @@ out_trans_cancel:
STATIC int
xfs_vn_setattr(
- struct dentry *dentry,
- struct iattr *iattr)
+ struct dentry *dentry,
+ struct iattr *iattr)
{
- if (iattr->ia_valid & ATTR_SIZE)
- return -xfs_setattr_size(XFS_I(dentry->d_inode), iattr, 0);
- return -xfs_setattr_nonsize(XFS_I(dentry->d_inode), iattr, 0);
+ struct xfs_inode *ip = XFS_I(dentry->d_inode);
+ int error;
+
+ if (iattr->ia_valid & ATTR_SIZE) {
+ xfs_ilock(ip, XFS_IOLOCK_EXCL);
+ error = xfs_setattr_size(ip, iattr);
+ xfs_iunlock(ip, XFS_IOLOCK_EXCL);
+ } else {
+ error = xfs_setattr_nonsize(ip, iattr, 0);
+ }
+
+ return -error;
}
STATIC int
Index: xfs/fs/xfs/xfs_bmap_util.c
===================================================================
--- xfs.orig/fs/xfs/xfs_bmap_util.c 2013-10-01 21:20:47.564230097 +0200
+++ xfs/fs/xfs/xfs_bmap_util.c 2013-10-01 21:20:54.316230257 +0200
@@ -1622,8 +1622,7 @@ xfs_change_file_space(
iattr.ia_valid = ATTR_SIZE;
iattr.ia_size = startoffset;
- error = xfs_setattr_size(ip, &iattr,
- attr_flags | XFS_ATTR_NOLOCK);
+ error = xfs_setattr_size(ip, &iattr);
xfs_iunlock(ip, XFS_IOLOCK_EXCL);
if (error)
Index: xfs/fs/xfs/xfs_iops.h
===================================================================
--- xfs.orig/fs/xfs/xfs_iops.h 2013-10-01 21:20:47.680230100 +0200
+++ xfs/fs/xfs/xfs_iops.h 2013-10-01 21:20:54.316230257 +0200
@@ -38,6 +38,6 @@ extern void xfs_setup_inode(struct xfs_i
extern int xfs_setattr_nonsize(struct xfs_inode *ip, struct iattr *vap,
int flags);
-extern int xfs_setattr_size(struct xfs_inode *ip, struct iattr *vap, int flags);
+extern int xfs_setattr_size(struct xfs_inode *ip, struct iattr *vap);
#endif /* __XFS_IOPS_H__ */
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 1/5] xfs: always take the iolock around xfs_setattr_size
2013-10-12 7:55 ` [PATCH 1/5] xfs: always take the iolock around xfs_setattr_size Christoph Hellwig
@ 2013-10-14 4:50 ` Dave Chinner
2013-10-14 7:29 ` Christoph Hellwig
2013-10-14 14:09 ` [PATCH 1/5 v3] " Christoph Hellwig
1 sibling, 1 reply; 24+ messages in thread
From: Dave Chinner @ 2013-10-14 4:50 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Sat, Oct 12, 2013 at 12:55:04AM -0700, Christoph Hellwig wrote:
> There is no reason to conditionally take the iolock inside xfs_setattr_size
> when we can let the caller handle it unconditionally, which just incrases
> the lock hold time for the case where it was previously taken internally
> by a few instructions.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
>
> ---
> fs/xfs/xfs_bmap_util.c | 3 +--
> fs/xfs/xfs_file.c | 2 +-
> fs/xfs/xfs_iops.c | 29 ++++++++++++++++-------------
> fs/xfs/xfs_iops.h | 2 +-
> 4 files changed, 19 insertions(+), 17 deletions(-)
>
> Index: xfs/fs/xfs/xfs_file.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_file.c 2013-10-01 21:20:47.564230097 +0200
> +++ xfs/fs/xfs/xfs_file.c 2013-10-01 21:20:54.312230257 +0200
> @@ -852,7 +852,7 @@ xfs_file_fallocate(
>
> iattr.ia_valid = ATTR_SIZE;
> iattr.ia_size = new_size;
> - error = -xfs_setattr_size(ip, &iattr, XFS_ATTR_NOLOCK);
> + error = -xfs_setattr_size(ip, &iattr);
> }
>
> out_unlock:
> Index: xfs/fs/xfs/xfs_iops.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_iops.c 2013-10-01 21:20:47.564230097 +0200
> +++ xfs/fs/xfs/xfs_iops.c 2013-10-01 21:20:54.316230257 +0200
> @@ -709,8 +709,7 @@ out_dqrele:
> int
> xfs_setattr_size(
> struct xfs_inode *ip,
> - struct iattr *iattr,
> - int flags)
> + struct iattr *iattr)
> {
> struct xfs_mount *mp = ip->i_mount;
> struct inode *inode = VFS_I(ip);
> @@ -733,15 +732,11 @@ xfs_setattr_size(
> if (error)
> return XFS_ERROR(error);
>
> + ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
> ASSERT(S_ISREG(ip->i_d.di_mode));
> ASSERT((mask & (ATTR_UID|ATTR_GID|ATTR_ATIME|ATTR_ATIME_SET|
> ATTR_MTIME_SET|ATTR_KILL_PRIV|ATTR_TIMES_SET)) == 0);
>
> - if (!(flags & XFS_ATTR_NOLOCK)) {
> - lock_flags |= XFS_IOLOCK_EXCL;
> - xfs_ilock(ip, lock_flags);
> - }
> -
I think there should be a bit more cleanup of this lock_flags
variable. There's quite a few error paths that now "goto out_unlock"
without actually holding a lock. i.e. they could just become a
straight "return error", and lock_flags can probably go away
completely...
Otherwise it look s good to me.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 1/5] xfs: always take the iolock around xfs_setattr_size
2013-10-14 4:50 ` Dave Chinner
@ 2013-10-14 7:29 ` Christoph Hellwig
0 siblings, 0 replies; 24+ messages in thread
From: Christoph Hellwig @ 2013-10-14 7:29 UTC (permalink / raw)
To: Dave Chinner; +Cc: Christoph Hellwig, xfs
On Mon, Oct 14, 2013 at 03:50:07PM +1100, Dave Chinner wrote:
> I think there should be a bit more cleanup of this lock_flags
> variable. There's quite a few error paths that now "goto out_unlock"
> without actually holding a lock. i.e. they could just become a
> straight "return error", and lock_flags can probably go away
> completely...
I'll look into that.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 1/5 v3] xfs: always take the iolock around xfs_setattr_size
2013-10-12 7:55 ` [PATCH 1/5] xfs: always take the iolock around xfs_setattr_size Christoph Hellwig
2013-10-14 4:50 ` Dave Chinner
@ 2013-10-14 14:09 ` Christoph Hellwig
2013-10-14 20:28 ` Dave Chinner
1 sibling, 1 reply; 24+ messages in thread
From: Christoph Hellwig @ 2013-10-14 14:09 UTC (permalink / raw)
To: xfs
There is no reason to conditionally take the iolock inside xfs_setattr_size
when we can let the caller handle it unconditionally, which just incrases
the lock hold time for the case where it was previously taken internally
by a few instructions.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_bmap_util.c | 3 +--
fs/xfs/xfs_file.c | 2 +-
fs/xfs/xfs_iops.c | 39 +++++++++++++++++++++------------------
fs/xfs/xfs_iops.h | 2 +-
4 files changed, 24 insertions(+), 22 deletions(-)
Index: xfs/fs/xfs/xfs_file.c
===================================================================
--- xfs.orig/fs/xfs/xfs_file.c 2013-10-11 20:05:38.272512388 +0200
+++ xfs/fs/xfs/xfs_file.c 2013-10-14 16:07:38.316153285 +0200
@@ -852,7 +852,7 @@ xfs_file_fallocate(
iattr.ia_valid = ATTR_SIZE;
iattr.ia_size = new_size;
- error = -xfs_setattr_size(ip, &iattr, XFS_ATTR_NOLOCK);
+ error = -xfs_setattr_size(ip, &iattr);
}
out_unlock:
Index: xfs/fs/xfs/xfs_iops.c
===================================================================
--- xfs.orig/fs/xfs/xfs_iops.c 2013-10-11 20:05:38.272512388 +0200
+++ xfs/fs/xfs/xfs_iops.c 2013-10-14 15:28:38.900097685 +0200
@@ -709,8 +709,7 @@ out_dqrele:
int
xfs_setattr_size(
struct xfs_inode *ip,
- struct iattr *iattr,
- int flags)
+ struct iattr *iattr)
{
struct xfs_mount *mp = ip->i_mount;
struct inode *inode = VFS_I(ip);
@@ -733,15 +732,11 @@ xfs_setattr_size(
if (error)
return XFS_ERROR(error);
+ ASSERT(xfs_isilocked(ip, XFS_IOLOCK_EXCL));
ASSERT(S_ISREG(ip->i_d.di_mode));
ASSERT((mask & (ATTR_UID|ATTR_GID|ATTR_ATIME|ATTR_ATIME_SET|
ATTR_MTIME_SET|ATTR_KILL_PRIV|ATTR_TIMES_SET)) == 0);
- if (!(flags & XFS_ATTR_NOLOCK)) {
- lock_flags |= XFS_IOLOCK_EXCL;
- xfs_ilock(ip, lock_flags);
- }
-
oldsize = inode->i_size;
newsize = iattr->ia_size;
@@ -750,12 +745,11 @@ xfs_setattr_size(
*/
if (newsize == 0 && oldsize == 0 && ip->i_d.di_nextents == 0) {
if (!(mask & (ATTR_CTIME|ATTR_MTIME)))
- goto out_unlock;
+ return 0;
/*
* Use the regular setattr path to update the timestamps.
*/
- xfs_iunlock(ip, lock_flags);
iattr->ia_valid &= ~ATTR_SIZE;
return xfs_setattr_nonsize(ip, iattr, 0);
}
@@ -765,7 +759,7 @@ xfs_setattr_size(
*/
error = xfs_qm_dqattach(ip, 0);
if (error)
- goto out_unlock;
+ return error;
/*
* Now we can make the changes. Before we join the inode to the
@@ -783,7 +777,7 @@ xfs_setattr_size(
*/
error = xfs_zero_eof(ip, newsize, oldsize);
if (error)
- goto out_unlock;
+ return error;
}
/*
@@ -802,7 +796,7 @@ xfs_setattr_size(
error = -filemap_write_and_wait_range(VFS_I(ip)->i_mapping,
ip->i_d.di_size, newsize);
if (error)
- goto out_unlock;
+ return error;
}
/*
@@ -812,7 +806,7 @@ xfs_setattr_size(
error = -block_truncate_page(inode->i_mapping, newsize, xfs_get_blocks);
if (error)
- goto out_unlock;
+ return error;
tp = xfs_trans_alloc(mp, XFS_TRANS_SETATTR_SIZE);
error = xfs_trans_reserve(tp, &M_RES(mp)->tr_itruncate, 0, 0);
@@ -916,12 +910,21 @@ out_trans_cancel:
STATIC int
xfs_vn_setattr(
- struct dentry *dentry,
- struct iattr *iattr)
+ struct dentry *dentry,
+ struct iattr *iattr)
{
- if (iattr->ia_valid & ATTR_SIZE)
- return -xfs_setattr_size(XFS_I(dentry->d_inode), iattr, 0);
- return -xfs_setattr_nonsize(XFS_I(dentry->d_inode), iattr, 0);
+ struct xfs_inode *ip = XFS_I(dentry->d_inode);
+ int error;
+
+ if (iattr->ia_valid & ATTR_SIZE) {
+ xfs_ilock(ip, XFS_IOLOCK_EXCL);
+ error = xfs_setattr_size(ip, iattr);
+ xfs_iunlock(ip, XFS_IOLOCK_EXCL);
+ } else {
+ error = xfs_setattr_nonsize(ip, iattr, 0);
+ }
+
+ return -error;
}
STATIC int
Index: xfs/fs/xfs/xfs_bmap_util.c
===================================================================
--- xfs.orig/fs/xfs/xfs_bmap_util.c 2013-10-11 20:05:38.272512388 +0200
+++ xfs/fs/xfs/xfs_bmap_util.c 2013-10-14 16:07:38.316153285 +0200
@@ -1622,8 +1622,7 @@ xfs_change_file_space(
iattr.ia_valid = ATTR_SIZE;
iattr.ia_size = startoffset;
- error = xfs_setattr_size(ip, &iattr,
- attr_flags | XFS_ATTR_NOLOCK);
+ error = xfs_setattr_size(ip, &iattr);
xfs_iunlock(ip, XFS_IOLOCK_EXCL);
if (error)
Index: xfs/fs/xfs/xfs_iops.h
===================================================================
--- xfs.orig/fs/xfs/xfs_iops.h 2013-10-11 20:05:38.272512388 +0200
+++ xfs/fs/xfs/xfs_iops.h 2013-10-14 16:07:38.440153288 +0200
@@ -38,6 +38,6 @@ extern void xfs_setup_inode(struct xfs_i
extern int xfs_setattr_nonsize(struct xfs_inode *ip, struct iattr *vap,
int flags);
-extern int xfs_setattr_size(struct xfs_inode *ip, struct iattr *vap, int flags);
+extern int xfs_setattr_size(struct xfs_inode *ip, struct iattr *vap);
#endif /* __XFS_IOPS_H__ */
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 1/5 v3] xfs: always take the iolock around xfs_setattr_size
2013-10-14 14:09 ` [PATCH 1/5 v3] " Christoph Hellwig
@ 2013-10-14 20:28 ` Dave Chinner
0 siblings, 0 replies; 24+ messages in thread
From: Dave Chinner @ 2013-10-14 20:28 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Mon, Oct 14, 2013 at 07:09:35AM -0700, Christoph Hellwig wrote:
> There is no reason to conditionally take the iolock inside xfs_setattr_size
> when we can let the caller handle it unconditionally, which just incrases
> the lock hold time for the case where it was previously taken internally
> by a few instructions.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
...
> @@ -750,12 +745,11 @@ xfs_setattr_size(
> */
> if (newsize == 0 && oldsize == 0 && ip->i_d.di_nextents == 0) {
> if (!(mask & (ATTR_CTIME|ATTR_MTIME)))
> - goto out_unlock;
> + return 0;
That looks better :)
Reviewed-by: Dave Chinner <dchinner@redhat.com>
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 2/5] xfs: remove the unused XFS_ATTR_NONBLOCK flag
2013-10-12 7:55 [PATCH 0/5] refactor the preallocation and hole punching code Christoph Hellwig
2013-10-12 7:55 ` [PATCH 1/5] xfs: always take the iolock around xfs_setattr_size Christoph Hellwig
@ 2013-10-12 7:55 ` Christoph Hellwig
2013-10-14 4:51 ` Dave Chinner
2013-10-12 7:55 ` [PATCH 3/5] xfs: always hold the iolock when calling xfs_change_file_space Christoph Hellwig
` (3 subsequent siblings)
5 siblings, 1 reply; 24+ messages in thread
From: Christoph Hellwig @ 2013-10-12 7:55 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-kill-XFS_ATTR_NONBLOCK --]
[-- Type: text/plain, Size: 1397 bytes --]
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_ioctl.c | 3 ---
fs/xfs/xfs_iops.h | 1 -
2 files changed, 4 deletions(-)
Index: xfs/fs/xfs/xfs_ioctl.c
===================================================================
--- xfs.orig/fs/xfs/xfs_ioctl.c 2013-09-12 18:43:35.480015650 +0200
+++ xfs/fs/xfs/xfs_ioctl.c 2013-10-01 20:16:53.240138969 +0200
@@ -661,9 +661,6 @@ xfs_ioc_space(
if (!S_ISREG(inode->i_mode))
return -XFS_ERROR(EINVAL);
- if (filp->f_flags & (O_NDELAY|O_NONBLOCK))
- attr_flags |= XFS_ATTR_NONBLOCK;
-
if (filp->f_flags & O_DSYNC)
attr_flags |= XFS_ATTR_SYNC;
Index: xfs/fs/xfs/xfs_iops.h
===================================================================
--- xfs.orig/fs/xfs/xfs_iops.h 2013-08-13 15:21:06.716303663 +0200
+++ xfs/fs/xfs/xfs_iops.h 2013-10-01 20:17:18.440139568 +0200
@@ -31,7 +31,6 @@ extern void xfs_setup_inode(struct xfs_i
* Internal setattr interfaces.
*/
#define XFS_ATTR_DMI 0x01 /* invocation from a DMI function */
-#define XFS_ATTR_NONBLOCK 0x02 /* return EAGAIN if op would block */
#define XFS_ATTR_NOLOCK 0x04 /* Don't grab any conflicting locks */
#define XFS_ATTR_NOACL 0x08 /* Don't call xfs_acl_chmod */
#define XFS_ATTR_SYNC 0x10 /* synchronous operation required */
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 2/5] xfs: remove the unused XFS_ATTR_NONBLOCK flag
2013-10-12 7:55 ` [PATCH 2/5] xfs: remove the unused XFS_ATTR_NONBLOCK flag Christoph Hellwig
@ 2013-10-14 4:51 ` Dave Chinner
2013-10-17 20:01 ` Ben Myers
0 siblings, 1 reply; 24+ messages in thread
From: Dave Chinner @ 2013-10-14 4:51 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Sat, Oct 12, 2013 at 12:55:05AM -0700, Christoph Hellwig wrote:
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Still unused after all this time, so removing it is fine by me.
Reviewed-by: Dave Chinner <dchinner@redhat.com>
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/5] xfs: remove the unused XFS_ATTR_NONBLOCK flag
2013-10-14 4:51 ` Dave Chinner
@ 2013-10-17 20:01 ` Ben Myers
2013-10-17 20:03 ` Christoph Hellwig
0 siblings, 1 reply; 24+ messages in thread
From: Ben Myers @ 2013-10-17 20:01 UTC (permalink / raw)
To: Dave Chinner, Christoph Hellwig; +Cc: xfs
Gents,
On Mon, Oct 14, 2013 at 03:51:52PM +1100, Dave Chinner wrote:
> On Sat, Oct 12, 2013 at 12:55:05AM -0700, Christoph Hellwig wrote:
> > Signed-off-by: Christoph Hellwig <hch@lst.de>
>
> Still unused after all this time, so removing it is fine by me.
>
> Reviewed-by: Dave Chinner <dchinner@redhat.com>
An xfs ioctl user who requests nonblocking behavior will no longer get it.
This seems to constitute API breakage. How can we verify that this is unused
since anyone can open with O_NONBLOCK?
Regards,
Ben
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/5] xfs: remove the unused XFS_ATTR_NONBLOCK flag
2013-10-17 20:01 ` Ben Myers
@ 2013-10-17 20:03 ` Christoph Hellwig
2013-10-17 21:17 ` Ben Myers
0 siblings, 1 reply; 24+ messages in thread
From: Christoph Hellwig @ 2013-10-17 20:03 UTC (permalink / raw)
To: Ben Myers; +Cc: Christoph Hellwig, xfs
On Thu, Oct 17, 2013 at 03:01:09PM -0500, Ben Myers wrote:
> An xfs ioctl user who requests nonblocking behavior will no longer get it.
> This seems to constitute API breakage. How can we verify that this is unused
> since anyone can open with O_NONBLOCK?
The flag isn't checked anywhere, which means it doesn't have any effect,
and doesn't as far as I can look back. I also don't know how the
prealloc ioctls could behave non-blocking in any sane way.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 2/5] xfs: remove the unused XFS_ATTR_NONBLOCK flag
2013-10-17 20:03 ` Christoph Hellwig
@ 2013-10-17 21:17 ` Ben Myers
0 siblings, 0 replies; 24+ messages in thread
From: Ben Myers @ 2013-10-17 21:17 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
Hey Christoph,
On Thu, Oct 17, 2013 at 01:03:15PM -0700, Christoph Hellwig wrote:
> On Thu, Oct 17, 2013 at 03:01:09PM -0500, Ben Myers wrote:
> > An xfs ioctl user who requests nonblocking behavior will no longer get it.
> > This seems to constitute API breakage. How can we verify that this is unused
> > since anyone can open with O_NONBLOCK?
>
> The flag isn't checked anywhere, which means it doesn't have any effect,
> and doesn't as far as I can look back.
Gah. I was about to say that you just removed the last usage of it in patch 1!
But that patch is about NOLOCK, not NONBLOCK. I should get my eyes checked. ;)
Anyway, XFS_ATTR_NONBLOCK is used by dmapi so that nfs threads don't block for
offline files. Folks who care about that sort of thing will have to add it
back in. That's fine.
FWIW, you can see that here:
http://oss.sgi.com/cgi-bin/gitweb.cgi?p=xfs/xfs.git;a=shortlog;h=refs/heads/v3.0-xfs_dmapi
> I also don't know how the
> prealloc ioctls could behave non-blocking in any sane way.
Right. Nfsd doesn't use any of those ioctls, so I don't really see the point
either.
Looks like my concern was unfounded.
Thanks,
Ben
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 3/5] xfs: always hold the iolock when calling xfs_change_file_space
2013-10-12 7:55 [PATCH 0/5] refactor the preallocation and hole punching code Christoph Hellwig
2013-10-12 7:55 ` [PATCH 1/5] xfs: always take the iolock around xfs_setattr_size Christoph Hellwig
2013-10-12 7:55 ` [PATCH 2/5] xfs: remove the unused XFS_ATTR_NONBLOCK flag Christoph Hellwig
@ 2013-10-12 7:55 ` Christoph Hellwig
2013-10-14 4:55 ` Dave Chinner
2013-10-12 7:55 ` [PATCH 4/5] xfs: simplify the fallocate path Christoph Hellwig
` (2 subsequent siblings)
5 siblings, 1 reply; 24+ messages in thread
From: Christoph Hellwig @ 2013-10-12 7:55 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-kill-XFS_ATTR_NOLOCK --]
[-- Type: text/plain, Size: 8022 bytes --]
Currently fallocate always holds the iolock when calling into
xfs_change_file_space, while the ioctl path lets some of the lower level
functions take it, but leave it out in others.
This patch makes sure the ioctl path also always holds the iolock and
thus introduces consistent locking for the preallocation operations while
simplifying the code and allowing to kill the now unused XFS_ATTR_NOLOCK
flag.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_bmap_util.c | 71 ++++++++++++++++---------------------------------
fs/xfs/xfs_file.c | 2 -
fs/xfs/xfs_ioctl.c | 2 +
fs/xfs/xfs_iops.h | 1
4 files changed, 27 insertions(+), 49 deletions(-)
Index: xfs/fs/xfs/xfs_file.c
===================================================================
--- xfs.orig/fs/xfs/xfs_file.c 2013-10-01 20:19:12.000000000 +0200
+++ xfs/fs/xfs/xfs_file.c 2013-10-01 20:21:00.720144850 +0200
@@ -816,7 +816,7 @@ xfs_file_fallocate(
xfs_flock64_t bf;
xfs_inode_t *ip = XFS_I(inode);
int cmd = XFS_IOC_RESVSP;
- int attr_flags = XFS_ATTR_NOLOCK;
+ int attr_flags = 0;
if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE))
return -EOPNOTSUPP;
Index: xfs/fs/xfs/xfs_ioctl.c
===================================================================
--- xfs.orig/fs/xfs/xfs_ioctl.c 2013-10-01 20:16:53.000000000 +0200
+++ xfs/fs/xfs/xfs_ioctl.c 2013-10-01 20:21:00.724144850 +0200
@@ -670,7 +670,9 @@ xfs_ioc_space(
error = mnt_want_write_file(filp);
if (error)
return error;
+ xfs_ilock(ip, XFS_IOLOCK_EXCL);
error = xfs_change_file_space(ip, cmd, bf, filp->f_pos, attr_flags);
+ xfs_iunlock(ip, XFS_IOLOCK_EXCL);
mnt_drop_write_file(filp);
return -error;
}
Index: xfs/fs/xfs/xfs_bmap_util.c
===================================================================
--- xfs.orig/fs/xfs/xfs_bmap_util.c 2013-10-01 20:19:12.404142276 +0200
+++ xfs/fs/xfs/xfs_bmap_util.c 2013-10-01 20:22:25.328146861 +0200
@@ -989,8 +989,7 @@ xfs_alloc_file_space(
xfs_inode_t *ip,
xfs_off_t offset,
xfs_off_t len,
- int alloc_type,
- int attr_flags)
+ int alloc_type)
{
xfs_mount_t *mp = ip->i_mount;
xfs_off_t count;
@@ -1248,8 +1247,7 @@ STATIC int
xfs_free_file_space(
xfs_inode_t *ip,
xfs_off_t offset,
- xfs_off_t len,
- int attr_flags)
+ xfs_off_t len)
{
int committed;
int done;
@@ -1267,7 +1265,6 @@ xfs_free_file_space(
int rt;
xfs_fileoff_t startoffset_fsb;
xfs_trans_t *tp;
- int need_iolock = 1;
mp = ip->i_mount;
@@ -1284,20 +1281,15 @@ xfs_free_file_space(
startoffset_fsb = XFS_B_TO_FSB(mp, offset);
endoffset_fsb = XFS_B_TO_FSBT(mp, offset + len);
- if (attr_flags & XFS_ATTR_NOLOCK)
- need_iolock = 0;
- if (need_iolock) {
- xfs_ilock(ip, XFS_IOLOCK_EXCL);
- /* wait for the completion of any pending DIOs */
- inode_dio_wait(VFS_I(ip));
- }
+ /* wait for the completion of any pending DIOs */
+ inode_dio_wait(VFS_I(ip));
rounding = max_t(xfs_off_t, 1 << mp->m_sb.sb_blocklog, PAGE_CACHE_SIZE);
ioffset = offset & ~(rounding - 1);
error = -filemap_write_and_wait_range(VFS_I(ip)->i_mapping,
ioffset, -1);
if (error)
- goto out_unlock_iolock;
+ goto out;
truncate_pagecache_range(VFS_I(ip), ioffset, -1);
/*
@@ -1311,7 +1303,7 @@ xfs_free_file_space(
error = xfs_bmapi_read(ip, startoffset_fsb, 1,
&imap, &nimap, 0);
if (error)
- goto out_unlock_iolock;
+ goto out;
ASSERT(nimap == 0 || nimap == 1);
if (nimap && imap.br_startblock != HOLESTARTBLOCK) {
xfs_daddr_t block;
@@ -1326,7 +1318,7 @@ xfs_free_file_space(
error = xfs_bmapi_read(ip, endoffset_fsb - 1, 1,
&imap, &nimap, 0);
if (error)
- goto out_unlock_iolock;
+ goto out;
ASSERT(nimap == 0 || nimap == 1);
if (nimap && imap.br_startblock != HOLESTARTBLOCK) {
ASSERT(imap.br_startblock != DELAYSTARTBLOCK);
@@ -1412,18 +1404,15 @@ xfs_free_file_space(
xfs_iunlock(ip, XFS_ILOCK_EXCL);
}
- out_unlock_iolock:
- if (need_iolock)
- xfs_iunlock(ip, XFS_IOLOCK_EXCL);
+ out:
return error;
error0:
xfs_bmap_cancel(&free_list);
error1:
xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES | XFS_TRANS_ABORT);
- xfs_iunlock(ip, need_iolock ? (XFS_ILOCK_EXCL | XFS_IOLOCK_EXCL) :
- XFS_ILOCK_EXCL);
- return error;
+ xfs_iunlock(ip, XFS_ILOCK_EXCL);
+ goto out;
}
@@ -1431,8 +1420,7 @@ STATIC int
xfs_zero_file_space(
struct xfs_inode *ip,
xfs_off_t offset,
- xfs_off_t len,
- int attr_flags)
+ xfs_off_t len)
{
struct xfs_mount *mp = ip->i_mount;
uint granularity;
@@ -1453,9 +1441,6 @@ xfs_zero_file_space(
ASSERT(start_boundary >= offset);
ASSERT(end_boundary <= offset + len);
- if (!(attr_flags & XFS_ATTR_NOLOCK))
- xfs_ilock(ip, XFS_IOLOCK_EXCL);
-
if (start_boundary < end_boundary - 1) {
/* punch out the page cache over the conversion range */
truncate_pagecache_range(VFS_I(ip), start_boundary,
@@ -1463,16 +1448,16 @@ xfs_zero_file_space(
/* convert the blocks */
error = xfs_alloc_file_space(ip, start_boundary,
end_boundary - start_boundary - 1,
- XFS_BMAPI_PREALLOC | XFS_BMAPI_CONVERT,
- attr_flags);
+ XFS_BMAPI_PREALLOC | XFS_BMAPI_CONVERT);
if (error)
- goto out_unlock;
+ goto out;
/* We've handled the interior of the range, now for the edges */
- if (start_boundary != offset)
+ if (start_boundary != offset) {
error = xfs_iozero(ip, offset, start_boundary - offset);
- if (error)
- goto out_unlock;
+ if (error)
+ goto out;
+ }
if (end_boundary != offset + len)
error = xfs_iozero(ip, end_boundary,
@@ -1486,9 +1471,7 @@ xfs_zero_file_space(
error = xfs_iozero(ip, offset, len);
}
-out_unlock:
- if (!(attr_flags & XFS_ATTR_NOLOCK))
- xfs_iunlock(ip, XFS_IOLOCK_EXCL);
+out:
return error;
}
@@ -1571,8 +1554,7 @@ xfs_change_file_space(
setprealloc = clrprealloc = 0;
switch (cmd) {
case XFS_IOC_ZERO_RANGE:
- error = xfs_zero_file_space(ip, startoffset, bf->l_len,
- attr_flags);
+ error = xfs_zero_file_space(ip, startoffset, bf->l_len);
if (error)
return error;
setprealloc = 1;
@@ -1581,7 +1563,7 @@ xfs_change_file_space(
case XFS_IOC_RESVSP:
case XFS_IOC_RESVSP64:
error = xfs_alloc_file_space(ip, startoffset, bf->l_len,
- XFS_BMAPI_PREALLOC, attr_flags);
+ XFS_BMAPI_PREALLOC);
if (error)
return error;
setprealloc = 1;
@@ -1589,8 +1571,8 @@ xfs_change_file_space(
case XFS_IOC_UNRESVSP:
case XFS_IOC_UNRESVSP64:
- if ((error = xfs_free_file_space(ip, startoffset, bf->l_len,
- attr_flags)))
+ error = xfs_free_file_space(ip, startoffset, bf->l_len);
+ if (error)
return error;
break;
@@ -1608,22 +1590,17 @@ xfs_change_file_space(
* truncate, direct IO) from racing against the transient
* allocated but not written state we can have here.
*/
- xfs_ilock(ip, XFS_IOLOCK_EXCL);
if (startoffset > fsize) {
error = xfs_alloc_file_space(ip, fsize,
- startoffset - fsize, 0,
- attr_flags | XFS_ATTR_NOLOCK);
- if (error) {
- xfs_iunlock(ip, XFS_IOLOCK_EXCL);
+ startoffset - fsize, 0);
+ if (error)
break;
- }
}
iattr.ia_valid = ATTR_SIZE;
iattr.ia_size = startoffset;
error = xfs_setattr_size(ip, &iattr);
- xfs_iunlock(ip, XFS_IOLOCK_EXCL);
if (error)
return error;
Index: xfs/fs/xfs/xfs_iops.h
===================================================================
--- xfs.orig/fs/xfs/xfs_iops.h 2013-10-01 20:19:51.160143197 +0200
+++ xfs/fs/xfs/xfs_iops.h 2013-10-01 20:21:19.416145295 +0200
@@ -31,7 +31,6 @@ extern void xfs_setup_inode(struct xfs_i
* Internal setattr interfaces.
*/
#define XFS_ATTR_DMI 0x01 /* invocation from a DMI function */
-#define XFS_ATTR_NOLOCK 0x04 /* Don't grab any conflicting locks */
#define XFS_ATTR_NOACL 0x08 /* Don't call xfs_acl_chmod */
#define XFS_ATTR_SYNC 0x10 /* synchronous operation required */
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 3/5] xfs: always hold the iolock when calling xfs_change_file_space
2013-10-12 7:55 ` [PATCH 3/5] xfs: always hold the iolock when calling xfs_change_file_space Christoph Hellwig
@ 2013-10-14 4:55 ` Dave Chinner
0 siblings, 0 replies; 24+ messages in thread
From: Dave Chinner @ 2013-10-14 4:55 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Sat, Oct 12, 2013 at 12:55:06AM -0700, Christoph Hellwig wrote:
> Currently fallocate always holds the iolock when calling into
> xfs_change_file_space, while the ioctl path lets some of the lower level
> functions take it, but leave it out in others.
>
> This patch makes sure the ioctl path also always holds the iolock and
> thus introduces consistent locking for the preallocation operations while
> simplifying the code and allowing to kill the now unused XFS_ATTR_NOLOCK
> flag.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Looks good to me.
Reviewed-by: Dave Chinner <dchinner@redhat.com>
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 4/5] xfs: simplify the fallocate path
2013-10-12 7:55 [PATCH 0/5] refactor the preallocation and hole punching code Christoph Hellwig
` (2 preceding siblings ...)
2013-10-12 7:55 ` [PATCH 3/5] xfs: always hold the iolock when calling xfs_change_file_space Christoph Hellwig
@ 2013-10-12 7:55 ` Christoph Hellwig
2013-10-14 5:04 ` Dave Chinner
2013-10-12 7:55 ` [PATCH 5/5] xfs: fold xfs_change_file_space into xfs_ioc_space Christoph Hellwig
2013-10-21 21:58 ` [PATCH 0/5] refactor the preallocation and hole punching code Ben Myers
5 siblings, 1 reply; 24+ messages in thread
From: Christoph Hellwig @ 2013-10-12 7:55 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-simplify-fallocate --]
[-- Type: text/plain, Size: 5785 bytes --]
Call xfs_alloc_file_space or xfs_free_file_space directly from
xfs_file_fallocate instead of going through xfs_change_file_space.
This simplified the code by removing the unessecary marshalling of the
arguments into an xfs_flock64_t structure and allows removing checks that
are already done in the VFS code.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_bmap_util.c | 39 ++-----------------------
fs/xfs/xfs_bmap_util.h | 4 ++
fs/xfs/xfs_file.c | 76 ++++++++++++++++++++++++++++++-------------------
3 files changed, 56 insertions(+), 63 deletions(-)
Index: xfs/fs/xfs/xfs_file.c
===================================================================
--- xfs.orig/fs/xfs/xfs_file.c 2013-10-01 21:06:23.000000000 +0200
+++ xfs/fs/xfs/xfs_file.c 2013-10-01 21:12:47.552218689 +0200
@@ -805,44 +805,64 @@ out:
STATIC long
xfs_file_fallocate(
- struct file *file,
- int mode,
- loff_t offset,
- loff_t len)
+ struct file *file,
+ int mode,
+ loff_t offset,
+ loff_t len)
{
- struct inode *inode = file_inode(file);
- long error;
- loff_t new_size = 0;
- xfs_flock64_t bf;
- xfs_inode_t *ip = XFS_I(inode);
- int cmd = XFS_IOC_RESVSP;
- int attr_flags = 0;
+ struct inode *inode = file_inode(file);
+ struct xfs_inode *ip = XFS_I(inode);
+ struct xfs_trans *tp;
+ long error;
+ loff_t new_size = 0;
+ if (!S_ISREG(inode->i_mode))
+ return -EINVAL;
if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE))
return -EOPNOTSUPP;
- bf.l_whence = 0;
- bf.l_start = offset;
- bf.l_len = len;
-
xfs_ilock(ip, XFS_IOLOCK_EXCL);
+ if (mode & FALLOC_FL_PUNCH_HOLE) {
+ error = xfs_free_file_space(ip, offset, len);
+ if (error)
+ goto out_unlock;
+ } else {
+ if (!(mode & FALLOC_FL_KEEP_SIZE) &&
+ offset + len > i_size_read(inode)) {
+ new_size = offset + len;
+ error = -inode_newsize_ok(inode, new_size);
+ if (error)
+ goto out_unlock;
+ }
- if (mode & FALLOC_FL_PUNCH_HOLE)
- cmd = XFS_IOC_UNRESVSP;
-
- /* check the new inode size is valid before allocating */
- if (!(mode & FALLOC_FL_KEEP_SIZE) &&
- offset + len > i_size_read(inode)) {
- new_size = offset + len;
- error = inode_newsize_ok(inode, new_size);
+ error = xfs_alloc_file_space(ip, offset, len,
+ XFS_BMAPI_PREALLOC);
if (error)
goto out_unlock;
}
- if (file->f_flags & O_DSYNC)
- attr_flags |= XFS_ATTR_SYNC;
+ tp = xfs_trans_alloc(ip->i_mount, XFS_TRANS_WRITEID);
+ error = xfs_trans_reserve(tp, &M_RES(ip->i_mount)->tr_writeid, 0, 0);
+ if (error) {
+ xfs_trans_cancel(tp, 0);
+ goto out_unlock;
+ }
+
+ xfs_ilock(ip, XFS_ILOCK_EXCL);
+ xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
+ ip->i_d.di_mode &= ~S_ISUID;
+ if (ip->i_d.di_mode & S_IXGRP)
+ ip->i_d.di_mode &= ~S_ISGID;
+
+ if (!(mode & FALLOC_FL_PUNCH_HOLE))
+ ip->i_d.di_flags |= XFS_DIFLAG_PREALLOC;
- error = -xfs_change_file_space(ip, cmd, &bf, 0, attr_flags);
+ xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
+ xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
+
+ if (file->f_flags & O_DSYNC)
+ xfs_trans_set_sync(tp);
+ error = xfs_trans_commit(tp, 0);
if (error)
goto out_unlock;
@@ -852,12 +872,12 @@ xfs_file_fallocate(
iattr.ia_valid = ATTR_SIZE;
iattr.ia_size = new_size;
- error = -xfs_setattr_size(ip, &iattr);
+ error = xfs_setattr_size(ip, &iattr);
}
out_unlock:
xfs_iunlock(ip, XFS_IOLOCK_EXCL);
- return error;
+ return -error;
}
Index: xfs/fs/xfs/xfs_bmap_util.c
===================================================================
--- xfs.orig/fs/xfs/xfs_bmap_util.c 2013-10-01 21:06:23.000000000 +0200
+++ xfs/fs/xfs/xfs_bmap_util.c 2013-10-01 21:08:25.264212455 +0200
@@ -965,28 +965,9 @@ xfs_free_eofblocks(
return error;
}
-/*
- * xfs_alloc_file_space()
- * This routine allocates disk space for the given file.
- *
- * If alloc_type == 0, this request is for an ALLOCSP type
- * request which will change the file size. In this case, no
- * DMAPI event will be generated by the call. A TRUNCATE event
- * will be generated later by xfs_setattr.
- *
- * If alloc_type != 0, this request is for a RESVSP type
- * request, and a DMAPI DM_EVENT_WRITE will be generated if the
- * lower block boundary byte address is less than the file's
- * length.
- *
- * RETURNS:
- * 0 on success
- * errno on error
- *
- */
-STATIC int
+int
xfs_alloc_file_space(
- xfs_inode_t *ip,
+ struct xfs_inode *ip,
xfs_off_t offset,
xfs_off_t len,
int alloc_type)
@@ -1231,21 +1212,9 @@ xfs_zero_remaining_bytes(
return error;
}
-/*
- * xfs_free_file_space()
- * This routine frees disk space for the given file.
- *
- * This routine is only called by xfs_change_file_space
- * for an UNRESVSP type call.
- *
- * RETURNS:
- * 0 on success
- * errno on error
- *
- */
-STATIC int
+int
xfs_free_file_space(
- xfs_inode_t *ip,
+ struct xfs_inode *ip,
xfs_off_t offset,
xfs_off_t len)
{
Index: xfs/fs/xfs/xfs_bmap_util.h
===================================================================
--- xfs.orig/fs/xfs/xfs_bmap_util.h 2013-08-13 15:21:06.000000000 +0200
+++ xfs/fs/xfs/xfs_bmap_util.h 2013-10-01 21:09:13.964213613 +0200
@@ -96,6 +96,10 @@ int xfs_bmap_last_extent(struct xfs_tran
int xfs_change_file_space(struct xfs_inode *ip, int cmd,
xfs_flock64_t *bf, xfs_off_t offset,
int attr_flags);
+int xfs_alloc_file_space(struct xfs_inode *ip, xfs_off_t offset,
+ xfs_off_t len, int alloc_type);
+int xfs_free_file_space(struct xfs_inode *ip, xfs_off_t offset,
+ xfs_off_t len);
/* EOF block manipulation functions */
bool xfs_can_free_eofblocks(struct xfs_inode *ip, bool force);
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 4/5] xfs: simplify the fallocate path
2013-10-12 7:55 ` [PATCH 4/5] xfs: simplify the fallocate path Christoph Hellwig
@ 2013-10-14 5:04 ` Dave Chinner
2013-10-14 7:30 ` Christoph Hellwig
0 siblings, 1 reply; 24+ messages in thread
From: Dave Chinner @ 2013-10-14 5:04 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Sat, Oct 12, 2013 at 12:55:07AM -0700, Christoph Hellwig wrote:
> Call xfs_alloc_file_space or xfs_free_file_space directly from
> xfs_file_fallocate instead of going through xfs_change_file_space.
>
> This simplified the code by removing the unessecary marshalling of the
> arguments into an xfs_flock64_t structure and allows removing checks that
> are already done in the VFS code.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
>
> ---
> fs/xfs/xfs_bmap_util.c | 39 ++-----------------------
> fs/xfs/xfs_bmap_util.h | 4 ++
> fs/xfs/xfs_file.c | 76 ++++++++++++++++++++++++++++++-------------------
> 3 files changed, 56 insertions(+), 63 deletions(-)
>
> Index: xfs/fs/xfs/xfs_file.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_file.c 2013-10-01 21:06:23.000000000 +0200
> +++ xfs/fs/xfs/xfs_file.c 2013-10-01 21:12:47.552218689 +0200
> @@ -805,44 +805,64 @@ out:
>
> STATIC long
> xfs_file_fallocate(
.....
> + tp = xfs_trans_alloc(ip->i_mount, XFS_TRANS_WRITEID);
> + error = xfs_trans_reserve(tp, &M_RES(ip->i_mount)->tr_writeid, 0, 0);
> + if (error) {
> + xfs_trans_cancel(tp, 0);
> + goto out_unlock;
> + }
> +
> + xfs_ilock(ip, XFS_ILOCK_EXCL);
> + xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
> + ip->i_d.di_mode &= ~S_ISUID;
> + if (ip->i_d.di_mode & S_IXGRP)
> + ip->i_d.di_mode &= ~S_ISGID;
> +
> + if (!(mode & FALLOC_FL_PUNCH_HOLE))
> + ip->i_d.di_flags |= XFS_DIFLAG_PREALLOC;
>
> - error = -xfs_change_file_space(ip, cmd, &bf, 0, attr_flags);
> + xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
> + xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> +
> + if (file->f_flags & O_DSYNC)
> + xfs_trans_set_sync(tp);
> + error = xfs_trans_commit(tp, 0);
> if (error)
> goto out_unlock;
Seems a bit clunky to do all this work when we've got to repeat most
of it when when we call xfs_setattr_size() if the size has changed.
Any thoughts on how we might reduce to a single transaction?
Otherwise it looks ok.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 4/5] xfs: simplify the fallocate path
2013-10-14 5:04 ` Dave Chinner
@ 2013-10-14 7:30 ` Christoph Hellwig
2013-10-14 20:03 ` Dave Chinner
0 siblings, 1 reply; 24+ messages in thread
From: Christoph Hellwig @ 2013-10-14 7:30 UTC (permalink / raw)
To: Dave Chinner; +Cc: Christoph Hellwig, xfs
On Mon, Oct 14, 2013 at 04:04:24PM +1100, Dave Chinner wrote:
> Seems a bit clunky to do all this work when we've got to repeat most
> of it when when we call xfs_setattr_size() if the size has changed.
> Any thoughts on how we might reduce to a single transaction?
I tried that, but the helper becomes a complete mess of flag values for the
possible inode modifications. You also pointed this out the last time
around. If anyone can come up with a saner helper than I tired feel
free to send a patch on top.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 4/5] xfs: simplify the fallocate path
2013-10-14 7:30 ` Christoph Hellwig
@ 2013-10-14 20:03 ` Dave Chinner
0 siblings, 0 replies; 24+ messages in thread
From: Dave Chinner @ 2013-10-14 20:03 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Mon, Oct 14, 2013 at 12:30:47AM -0700, Christoph Hellwig wrote:
> On Mon, Oct 14, 2013 at 04:04:24PM +1100, Dave Chinner wrote:
> > Seems a bit clunky to do all this work when we've got to repeat most
> > of it when when we call xfs_setattr_size() if the size has changed.
> > Any thoughts on how we might reduce to a single transaction?
>
> I tried that, but the helper becomes a complete mess of flag values for the
> possible inode modifications. You also pointed this out the last time
> around. If anyone can come up with a saner helper than I tired feel
> free to send a patch on top.
OK, no worries. Consider it:
Reviewed-by: Dave Chinner <dchinner@redhat.com>
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 5/5] xfs: fold xfs_change_file_space into xfs_ioc_space
2013-10-12 7:55 [PATCH 0/5] refactor the preallocation and hole punching code Christoph Hellwig
` (3 preceding siblings ...)
2013-10-12 7:55 ` [PATCH 4/5] xfs: simplify the fallocate path Christoph Hellwig
@ 2013-10-12 7:55 ` Christoph Hellwig
2013-10-14 5:08 ` Dave Chinner
2013-10-21 21:58 ` [PATCH 0/5] refactor the preallocation and hole punching code Ben Myers
5 siblings, 1 reply; 24+ messages in thread
From: Christoph Hellwig @ 2013-10-12 7:55 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-kill-xfs_change_file_space-2 --]
[-- Type: text/plain, Size: 10731 bytes --]
Now that only one caller of xfs_change_file_space is left it can be merged
into said caller.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_bmap_util.c | 178 -------------------------------------------------
fs/xfs/xfs_bmap_util.h | 5 -
fs/xfs/xfs_ioctl.c | 130 +++++++++++++++++++++++++++++++++--
fs/xfs/xfs_iops.h | 4 -
4 files changed, 126 insertions(+), 191 deletions(-)
Index: xfs/fs/xfs/xfs_ioctl.c
===================================================================
--- xfs.orig/fs/xfs/xfs_ioctl.c 2013-10-01 21:06:23.000000000 +0200
+++ xfs/fs/xfs/xfs_ioctl.c 2013-10-01 21:14:36.688221283 +0200
@@ -641,7 +641,11 @@ xfs_ioc_space(
unsigned int cmd,
xfs_flock64_t *bf)
{
- int attr_flags = 0;
+ struct xfs_mount *mp = ip->i_mount;
+ struct xfs_trans *tp;
+ struct iattr iattr;
+ bool setprealloc = false;
+ bool clrprealloc = false;
int error;
/*
@@ -661,17 +665,127 @@ xfs_ioc_space(
if (!S_ISREG(inode->i_mode))
return -XFS_ERROR(EINVAL);
- if (filp->f_flags & O_DSYNC)
- attr_flags |= XFS_ATTR_SYNC;
-
- if (ioflags & IO_INVIS)
- attr_flags |= XFS_ATTR_DMI;
-
error = mnt_want_write_file(filp);
if (error)
return error;
+
xfs_ilock(ip, XFS_IOLOCK_EXCL);
- error = xfs_change_file_space(ip, cmd, bf, filp->f_pos, attr_flags);
+
+ switch (bf->l_whence) {
+ case 0: /*SEEK_SET*/
+ break;
+ case 1: /*SEEK_CUR*/
+ bf->l_start += filp->f_pos;
+ break;
+ case 2: /*SEEK_END*/
+ bf->l_start += XFS_ISIZE(ip);
+ break;
+ default:
+ error = XFS_ERROR(EINVAL);
+ goto out_unlock;
+ }
+
+ /*
+ * length of <= 0 for resv/unresv/zero is invalid. length for
+ * alloc/free is ignored completely and we have no idea what userspace
+ * might have set it to, so set it to zero to allow range
+ * checks to pass.
+ */
+ switch (cmd) {
+ case XFS_IOC_ZERO_RANGE:
+ case XFS_IOC_RESVSP:
+ case XFS_IOC_RESVSP64:
+ case XFS_IOC_UNRESVSP:
+ case XFS_IOC_UNRESVSP64:
+ if (bf->l_len <= 0) {
+ error = XFS_ERROR(EINVAL);
+ goto out_unlock;
+ }
+ break;
+ default:
+ bf->l_len = 0;
+ break;
+ }
+
+ if (bf->l_start < 0 ||
+ bf->l_start > mp->m_super->s_maxbytes ||
+ bf->l_start + bf->l_len < 0 ||
+ bf->l_start + bf->l_len >= mp->m_super->s_maxbytes) {
+ error = XFS_ERROR(EINVAL);
+ goto out_unlock;
+ }
+
+ switch (cmd) {
+ case XFS_IOC_ZERO_RANGE:
+ error = xfs_zero_file_space(ip, bf->l_start, bf->l_len);
+ if (!error)
+ setprealloc = true;
+ break;
+ case XFS_IOC_RESVSP:
+ case XFS_IOC_RESVSP64:
+ error = xfs_alloc_file_space(ip, bf->l_start, bf->l_len,
+ XFS_BMAPI_PREALLOC);
+ if (!error)
+ setprealloc = true;
+ break;
+ case XFS_IOC_UNRESVSP:
+ case XFS_IOC_UNRESVSP64:
+ error = xfs_free_file_space(ip, bf->l_start, bf->l_len);
+ break;
+ case XFS_IOC_ALLOCSP:
+ case XFS_IOC_ALLOCSP64:
+ case XFS_IOC_FREESP:
+ case XFS_IOC_FREESP64:
+ if (bf->l_start > XFS_ISIZE(ip)) {
+ error = xfs_alloc_file_space(ip, XFS_ISIZE(ip),
+ bf->l_start - XFS_ISIZE(ip), 0);
+ if (error)
+ goto out_unlock;
+ }
+
+ iattr.ia_valid = ATTR_SIZE;
+ iattr.ia_size = bf->l_start;
+
+ error = xfs_setattr_size(ip, &iattr);
+ if (!error)
+ clrprealloc = true;
+ break;
+ default:
+ ASSERT(0);
+ error = XFS_ERROR(EINVAL);
+ }
+
+ if (error)
+ goto out_unlock;
+
+ tp = xfs_trans_alloc(mp, XFS_TRANS_WRITEID);
+ error = xfs_trans_reserve(tp, &M_RES(mp)->tr_writeid, 0, 0);
+ if (error) {
+ xfs_trans_cancel(tp, 0);
+ goto out_unlock;
+ }
+
+ xfs_ilock(ip, XFS_ILOCK_EXCL);
+ xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
+
+ if (!(ioflags & IO_INVIS)) {
+ ip->i_d.di_mode &= ~S_ISUID;
+ if (ip->i_d.di_mode & S_IXGRP)
+ ip->i_d.di_mode &= ~S_ISGID;
+ xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
+ }
+
+ if (setprealloc)
+ ip->i_d.di_flags |= XFS_DIFLAG_PREALLOC;
+ else if (clrprealloc)
+ ip->i_d.di_flags &= ~XFS_DIFLAG_PREALLOC;
+
+ xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
+ if (filp->f_flags & O_DSYNC)
+ xfs_trans_set_sync(tp);
+ error = xfs_trans_commit(tp, 0);
+
+out_unlock:
xfs_iunlock(ip, XFS_IOLOCK_EXCL);
mnt_drop_write_file(filp);
return -error;
Index: xfs/fs/xfs/xfs_bmap_util.c
===================================================================
--- xfs.orig/fs/xfs/xfs_bmap_util.c 2013-10-01 21:08:25.264212455 +0200
+++ xfs/fs/xfs/xfs_bmap_util.c 2013-10-01 21:15:28.508222514 +0200
@@ -1385,7 +1385,7 @@ xfs_free_file_space(
}
-STATIC int
+int
xfs_zero_file_space(
struct xfs_inode *ip,
xfs_off_t offset,
@@ -1446,182 +1446,6 @@ out:
}
/*
- * xfs_change_file_space()
- * This routine allocates or frees disk space for the given file.
- * The user specified parameters are checked for alignment and size
- * limitations.
- *
- * RETURNS:
- * 0 on success
- * errno on error
- *
- */
-int
-xfs_change_file_space(
- xfs_inode_t *ip,
- int cmd,
- xfs_flock64_t *bf,
- xfs_off_t offset,
- int attr_flags)
-{
- xfs_mount_t *mp = ip->i_mount;
- int clrprealloc;
- int error;
- xfs_fsize_t fsize;
- int setprealloc;
- xfs_off_t startoffset;
- xfs_trans_t *tp;
- struct iattr iattr;
-
- if (!S_ISREG(ip->i_d.di_mode))
- return XFS_ERROR(EINVAL);
-
- switch (bf->l_whence) {
- case 0: /*SEEK_SET*/
- break;
- case 1: /*SEEK_CUR*/
- bf->l_start += offset;
- break;
- case 2: /*SEEK_END*/
- bf->l_start += XFS_ISIZE(ip);
- break;
- default:
- return XFS_ERROR(EINVAL);
- }
-
- /*
- * length of <= 0 for resv/unresv/zero is invalid. length for
- * alloc/free is ignored completely and we have no idea what userspace
- * might have set it to, so set it to zero to allow range
- * checks to pass.
- */
- switch (cmd) {
- case XFS_IOC_ZERO_RANGE:
- case XFS_IOC_RESVSP:
- case XFS_IOC_RESVSP64:
- case XFS_IOC_UNRESVSP:
- case XFS_IOC_UNRESVSP64:
- if (bf->l_len <= 0)
- return XFS_ERROR(EINVAL);
- break;
- default:
- bf->l_len = 0;
- break;
- }
-
- if (bf->l_start < 0 ||
- bf->l_start > mp->m_super->s_maxbytes ||
- bf->l_start + bf->l_len < 0 ||
- bf->l_start + bf->l_len >= mp->m_super->s_maxbytes)
- return XFS_ERROR(EINVAL);
-
- bf->l_whence = 0;
-
- startoffset = bf->l_start;
- fsize = XFS_ISIZE(ip);
-
- setprealloc = clrprealloc = 0;
- switch (cmd) {
- case XFS_IOC_ZERO_RANGE:
- error = xfs_zero_file_space(ip, startoffset, bf->l_len);
- if (error)
- return error;
- setprealloc = 1;
- break;
-
- case XFS_IOC_RESVSP:
- case XFS_IOC_RESVSP64:
- error = xfs_alloc_file_space(ip, startoffset, bf->l_len,
- XFS_BMAPI_PREALLOC);
- if (error)
- return error;
- setprealloc = 1;
- break;
-
- case XFS_IOC_UNRESVSP:
- case XFS_IOC_UNRESVSP64:
- error = xfs_free_file_space(ip, startoffset, bf->l_len);
- if (error)
- return error;
- break;
-
- case XFS_IOC_ALLOCSP:
- case XFS_IOC_ALLOCSP64:
- case XFS_IOC_FREESP:
- case XFS_IOC_FREESP64:
- /*
- * These operations actually do IO when extending the file, but
- * the allocation is done seperately to the zeroing that is
- * done. This set of operations need to be serialised against
- * other IO operations, such as truncate and buffered IO. We
- * need to take the IOLOCK here to serialise the allocation and
- * zeroing IO to prevent other IOLOCK holders (e.g. getbmap,
- * truncate, direct IO) from racing against the transient
- * allocated but not written state we can have here.
- */
- if (startoffset > fsize) {
- error = xfs_alloc_file_space(ip, fsize,
- startoffset - fsize, 0);
- if (error)
- break;
- }
-
- iattr.ia_valid = ATTR_SIZE;
- iattr.ia_size = startoffset;
-
- error = xfs_setattr_size(ip, &iattr);
-
- if (error)
- return error;
-
- clrprealloc = 1;
- break;
-
- default:
- ASSERT(0);
- return XFS_ERROR(EINVAL);
- }
-
- /*
- * update the inode timestamp, mode, and prealloc flag bits
- */
- tp = xfs_trans_alloc(mp, XFS_TRANS_WRITEID);
- error = xfs_trans_reserve(tp, &M_RES(mp)->tr_writeid, 0, 0);
- if (error) {
- xfs_trans_cancel(tp, 0);
- return error;
- }
-
- xfs_ilock(ip, XFS_ILOCK_EXCL);
- xfs_trans_ijoin(tp, ip, XFS_ILOCK_EXCL);
-
- if ((attr_flags & XFS_ATTR_DMI) == 0) {
- ip->i_d.di_mode &= ~S_ISUID;
-
- /*
- * Note that we don't have to worry about mandatory
- * file locking being disabled here because we only
- * clear the S_ISGID bit if the Group execute bit is
- * on, but if it was on then mandatory locking wouldn't
- * have been enabled.
- */
- if (ip->i_d.di_mode & S_IXGRP)
- ip->i_d.di_mode &= ~S_ISGID;
-
- xfs_trans_ichgtime(tp, ip, XFS_ICHGTIME_MOD | XFS_ICHGTIME_CHG);
- }
- if (setprealloc)
- ip->i_d.di_flags |= XFS_DIFLAG_PREALLOC;
- else if (clrprealloc)
- ip->i_d.di_flags &= ~XFS_DIFLAG_PREALLOC;
-
- xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
- if (attr_flags & XFS_ATTR_SYNC)
- xfs_trans_set_sync(tp);
- return xfs_trans_commit(tp, 0);
-}
-
-/*
* We need to check that the format of the data fork in the temporary inode is
* valid for the target inode before doing the swap. This is not a problem with
* attr1 because of the fixed fork offset, but attr2 has a dynamically sized
Index: xfs/fs/xfs/xfs_bmap_util.h
===================================================================
--- xfs.orig/fs/xfs/xfs_bmap_util.h 2013-10-01 21:09:13.964213613 +0200
+++ xfs/fs/xfs/xfs_bmap_util.h 2013-10-01 21:15:54.148223124 +0200
@@ -93,13 +93,12 @@ int xfs_bmap_last_extent(struct xfs_tran
int *is_empty);
/* preallocation and hole punch interface */
-int xfs_change_file_space(struct xfs_inode *ip, int cmd,
- xfs_flock64_t *bf, xfs_off_t offset,
- int attr_flags);
int xfs_alloc_file_space(struct xfs_inode *ip, xfs_off_t offset,
xfs_off_t len, int alloc_type);
int xfs_free_file_space(struct xfs_inode *ip, xfs_off_t offset,
xfs_off_t len);
+int xfs_zero_file_space(struct xfs_inode *ip, xfs_off_t offset,
+ xfs_off_t len);
/* EOF block manipulation functions */
bool xfs_can_free_eofblocks(struct xfs_inode *ip, bool force);
Index: xfs/fs/xfs/xfs_iops.h
===================================================================
--- xfs.orig/fs/xfs/xfs_iops.h 2013-10-01 21:06:23.584209563 +0200
+++ xfs/fs/xfs/xfs_iops.h 2013-10-01 21:16:31.752224017 +0200
@@ -30,9 +30,7 @@ extern void xfs_setup_inode(struct xfs_i
/*
* Internal setattr interfaces.
*/
-#define XFS_ATTR_DMI 0x01 /* invocation from a DMI function */
-#define XFS_ATTR_NOACL 0x08 /* Don't call xfs_acl_chmod */
-#define XFS_ATTR_SYNC 0x10 /* synchronous operation required */
+#define XFS_ATTR_NOACL 0x01 /* Don't call xfs_acl_chmod */
extern int xfs_setattr_nonsize(struct xfs_inode *ip, struct iattr *vap,
int flags);
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 24+ messages in thread* Re: [PATCH 5/5] xfs: fold xfs_change_file_space into xfs_ioc_space
2013-10-12 7:55 ` [PATCH 5/5] xfs: fold xfs_change_file_space into xfs_ioc_space Christoph Hellwig
@ 2013-10-14 5:08 ` Dave Chinner
2013-10-15 15:31 ` Christoph Hellwig
0 siblings, 1 reply; 24+ messages in thread
From: Dave Chinner @ 2013-10-14 5:08 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Sat, Oct 12, 2013 at 12:55:08AM -0700, Christoph Hellwig wrote:
> Now that only one caller of xfs_change_file_space is left it can be merged
> into said caller.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Looks good.
Reviewed-by: Dave Chinner <dchinner@redhat.com>
One question, though:
> + case XFS_IOC_ALLOCSP:
> + case XFS_IOC_ALLOCSP64:
> + case XFS_IOC_FREESP:
> + case XFS_IOC_FREESP64:
Should we, at this point, mark these ioctls as deprecated and
schedule then for removal given that we've recommended against using
them for the past 10 years and we have fallocate() now?
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 5/5] xfs: fold xfs_change_file_space into xfs_ioc_space
2013-10-14 5:08 ` Dave Chinner
@ 2013-10-15 15:31 ` Christoph Hellwig
2013-10-15 21:47 ` Dave Chinner
0 siblings, 1 reply; 24+ messages in thread
From: Christoph Hellwig @ 2013-10-15 15:31 UTC (permalink / raw)
To: Dave Chinner; +Cc: Christoph Hellwig, xfs
On Mon, Oct 14, 2013 at 04:08:07PM +1100, Dave Chinner wrote:
> One question, though:
>
> > + case XFS_IOC_ALLOCSP:
> > + case XFS_IOC_ALLOCSP64:
> > + case XFS_IOC_FREESP:
> > + case XFS_IOC_FREESP64:
>
> Should we, at this point, mark these ioctls as deprecated and
> schedule then for removal given that we've recommended against using
> them for the past 10 years and we have fallocate() now?
I don't see any reason to remove them given that it's only about 15
extra lines of code. But if you care enough to get rid of them we
probably need multiple years of actuall warnings emmited when used
before actually removing them. I would be very surprised if there
aren't same users that wouldn't argue very vocally against their
removal.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 5/5] xfs: fold xfs_change_file_space into xfs_ioc_space
2013-10-15 15:31 ` Christoph Hellwig
@ 2013-10-15 21:47 ` Dave Chinner
2013-10-16 7:03 ` Christoph Hellwig
0 siblings, 1 reply; 24+ messages in thread
From: Dave Chinner @ 2013-10-15 21:47 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Tue, Oct 15, 2013 at 08:31:43AM -0700, Christoph Hellwig wrote:
> On Mon, Oct 14, 2013 at 04:08:07PM +1100, Dave Chinner wrote:
> > One question, though:
> >
> > > + case XFS_IOC_ALLOCSP:
> > > + case XFS_IOC_ALLOCSP64:
> > > + case XFS_IOC_FREESP:
> > > + case XFS_IOC_FREESP64:
> >
> > Should we, at this point, mark these ioctls as deprecated and
> > schedule then for removal given that we've recommended against using
> > them for the past 10 years and we have fallocate() now?
>
> I don't see any reason to remove them given that it's only about 15
> extra lines of code. But if you care enough to get rid of them we
> probably need multiple years of actuall warnings emmited when used
> before actually removing them. I would be very surprised if there
> aren't same users that wouldn't argue very vocally against their
> removal.
I'll put a significant quantity of beer on the table if anyone other
than xfstests is actually using these ioctls. In all my years of
working with XFS, I've never seen a single user of them, even on
Irix.
The one person I know who was considering using XFS_IOC_ALLOCSP
convinced me (quite easily) to implement XFS_IOC_ZERO_RANGE for them
because writing all those zeros to re-initialise pre-allocated VM
images was going to be prohibitively expensive...
Anyway, it was just a thought.
Cheers,
Dave.
--
Dave Chinner
david@fromorbit.com
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 5/5] xfs: fold xfs_change_file_space into xfs_ioc_space
2013-10-15 21:47 ` Dave Chinner
@ 2013-10-16 7:03 ` Christoph Hellwig
0 siblings, 0 replies; 24+ messages in thread
From: Christoph Hellwig @ 2013-10-16 7:03 UTC (permalink / raw)
To: Dave Chinner; +Cc: Christoph Hellwig, xfs
On Wed, Oct 16, 2013 at 08:47:33AM +1100, Dave Chinner wrote:
> I'll put a significant quantity of beer on the table if anyone other
> than xfstests is actually using these ioctls. In all my years of
> working with XFS, I've never seen a single user of them, even on
> Irix.
>
> The one person I know who was considering using XFS_IOC_ALLOCSP
> convinced me (quite easily) to implement XFS_IOC_ZERO_RANGE for them
> because writing all those zeros to re-initialise pre-allocated VM
> images was going to be prohibitively expensive...
>
> Anyway, it was just a thought.
Send a patch to warn on their usage, and you might be the lucky owner of
a beer or two a few years down the road :)
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 24+ messages in thread
* Re: [PATCH 0/5] refactor the preallocation and hole punching code
2013-10-12 7:55 [PATCH 0/5] refactor the preallocation and hole punching code Christoph Hellwig
` (4 preceding siblings ...)
2013-10-12 7:55 ` [PATCH 5/5] xfs: fold xfs_change_file_space into xfs_ioc_space Christoph Hellwig
@ 2013-10-21 21:58 ` Ben Myers
5 siblings, 0 replies; 24+ messages in thread
From: Ben Myers @ 2013-10-21 21:58 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
Hi Christoph,
On Sat, Oct 12, 2013 at 12:55:03AM -0700, Christoph Hellwig wrote:
> This series massages the preallocation and hole punching code into something
> sane. The big catchall xfs_change_file_space function is gone, fallocate
> and the ioctl entry point now directly call the low-level functions, and
> fallocate can avoid packing its arguments into the xfs_flock form and
> duplicating error checking already done in the VFS. In addition we also
> use a common locking patter now, that is preallocations using the ioctl
> path now also take iolock, just like fallocate and all other ioctl cases
> already did beforehand.
>
>
> Changes since version 1:
> - remove the unused setprealloc variable in xfs_file_fallocate
> - rebased
Applied this series.
Thanks,
Ben
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 24+ messages in thread
* [PATCH 3/5] xfs: always hold the iolock when calling xfs_change_file_space
2012-12-08 12:08 Christoph Hellwig
@ 2012-12-08 12:08 ` Christoph Hellwig
0 siblings, 0 replies; 24+ messages in thread
From: Christoph Hellwig @ 2012-12-08 12:08 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-kill-XFS_ATTR_NOLOCK --]
[-- Type: text/plain, Size: 8108 bytes --]
Currently fallocate always holds the iolock when calling into
xfs_change_file_space, while the ioctl path lets some of the lower level
functions take it, but leave it out in others.
This patch makes sure the ioctl path also always holds the iolock and
thus introduces consistent locking for the preallocation operations while
simplifying the code and allowing to kill the now unused XFS_ATTR_NOLOCK
flag.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_file.c | 2 -
fs/xfs/xfs_ioctl.c | 2 +
fs/xfs/xfs_vnodeops.c | 71 ++++++++++++++++----------------------------------
fs/xfs/xfs_vnodeops.h | 1
4 files changed, 27 insertions(+), 49 deletions(-)
Index: xfs/fs/xfs/xfs_file.c
===================================================================
--- xfs.orig/fs/xfs/xfs_file.c 2012-11-30 18:38:47.404018984 -0800
+++ xfs/fs/xfs/xfs_file.c 2012-11-30 18:39:27.892020019 -0800
@@ -817,7 +817,7 @@ xfs_file_fallocate(
xfs_flock64_t bf;
xfs_inode_t *ip = XFS_I(inode);
int cmd = XFS_IOC_RESVSP;
- int attr_flags = XFS_ATTR_NOLOCK;
+ int attr_flags = 0;
if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE))
return -EOPNOTSUPP;
Index: xfs/fs/xfs/xfs_ioctl.c
===================================================================
--- xfs.orig/fs/xfs/xfs_ioctl.c 2012-11-30 18:38:51.724019096 -0800
+++ xfs/fs/xfs/xfs_ioctl.c 2012-11-30 18:39:27.892020019 -0800
@@ -642,7 +642,9 @@ xfs_ioc_space(
error = mnt_want_write_file(filp);
if (error)
return error;
+ xfs_ilock(ip, XFS_IOLOCK_EXCL);
error = xfs_change_file_space(ip, cmd, bf, filp->f_pos, attr_flags);
+ xfs_iunlock(ip, XFS_IOLOCK_EXCL);
mnt_drop_write_file(filp);
return -error;
}
Index: xfs/fs/xfs/xfs_vnodeops.c
===================================================================
--- xfs.orig/fs/xfs/xfs_vnodeops.c 2012-11-30 18:38:47.408018982 -0800
+++ xfs/fs/xfs/xfs_vnodeops.c 2012-11-30 18:39:27.896020019 -0800
@@ -1651,8 +1651,7 @@ xfs_alloc_file_space(
xfs_inode_t *ip,
xfs_off_t offset,
xfs_off_t len,
- int alloc_type,
- int attr_flags)
+ int alloc_type)
{
xfs_mount_t *mp = ip->i_mount;
xfs_off_t count;
@@ -1912,8 +1911,7 @@ STATIC int
xfs_free_file_space(
xfs_inode_t *ip,
xfs_off_t offset,
- xfs_off_t len,
- int attr_flags)
+ xfs_off_t len)
{
int committed;
int done;
@@ -1931,7 +1929,6 @@ xfs_free_file_space(
int rt;
xfs_fileoff_t startoffset_fsb;
xfs_trans_t *tp;
- int need_iolock = 1;
mp = ip->i_mount;
@@ -1948,20 +1945,15 @@ xfs_free_file_space(
startoffset_fsb = XFS_B_TO_FSB(mp, offset);
endoffset_fsb = XFS_B_TO_FSBT(mp, offset + len);
- if (attr_flags & XFS_ATTR_NOLOCK)
- need_iolock = 0;
- if (need_iolock) {
- xfs_ilock(ip, XFS_IOLOCK_EXCL);
- /* wait for the completion of any pending DIOs */
- inode_dio_wait(VFS_I(ip));
- }
+ /* wait for the completion of any pending DIOs */
+ inode_dio_wait(VFS_I(ip));
rounding = max_t(uint, 1 << mp->m_sb.sb_blocklog, PAGE_CACHE_SIZE);
ioffset = offset & ~(rounding - 1);
error = -filemap_write_and_wait_range(VFS_I(ip)->i_mapping,
ioffset, -1);
if (error)
- goto out_unlock_iolock;
+ goto out;
truncate_pagecache_range(VFS_I(ip), ioffset, -1);
/*
@@ -1975,7 +1967,7 @@ xfs_free_file_space(
error = xfs_bmapi_read(ip, startoffset_fsb, 1,
&imap, &nimap, 0);
if (error)
- goto out_unlock_iolock;
+ goto out;
ASSERT(nimap == 0 || nimap == 1);
if (nimap && imap.br_startblock != HOLESTARTBLOCK) {
xfs_daddr_t block;
@@ -1990,7 +1982,7 @@ xfs_free_file_space(
error = xfs_bmapi_read(ip, endoffset_fsb - 1, 1,
&imap, &nimap, 0);
if (error)
- goto out_unlock_iolock;
+ goto out;
ASSERT(nimap == 0 || nimap == 1);
if (nimap && imap.br_startblock != HOLESTARTBLOCK) {
ASSERT(imap.br_startblock != DELAYSTARTBLOCK);
@@ -2081,18 +2073,15 @@ xfs_free_file_space(
xfs_iunlock(ip, XFS_ILOCK_EXCL);
}
- out_unlock_iolock:
- if (need_iolock)
- xfs_iunlock(ip, XFS_IOLOCK_EXCL);
+ out:
return error;
error0:
xfs_bmap_cancel(&free_list);
error1:
xfs_trans_cancel(tp, XFS_TRANS_RELEASE_LOG_RES | XFS_TRANS_ABORT);
- xfs_iunlock(ip, need_iolock ? (XFS_ILOCK_EXCL | XFS_IOLOCK_EXCL) :
- XFS_ILOCK_EXCL);
- return error;
+ xfs_iunlock(ip, XFS_ILOCK_EXCL);
+ goto out;
}
@@ -2100,8 +2089,7 @@ STATIC int
xfs_zero_file_space(
struct xfs_inode *ip,
xfs_off_t offset,
- xfs_off_t len,
- int attr_flags)
+ xfs_off_t len)
{
struct xfs_mount *mp = ip->i_mount;
uint granularity;
@@ -2122,9 +2110,6 @@ xfs_zero_file_space(
ASSERT(start_boundary >= offset);
ASSERT(end_boundary <= offset + len);
- if (!(attr_flags & XFS_ATTR_NOLOCK))
- xfs_ilock(ip, XFS_IOLOCK_EXCL);
-
if (start_boundary < end_boundary - 1) {
/* punch out the page cache over the conversion range */
truncate_pagecache_range(VFS_I(ip), start_boundary,
@@ -2132,16 +2117,16 @@ xfs_zero_file_space(
/* convert the blocks */
error = xfs_alloc_file_space(ip, start_boundary,
end_boundary - start_boundary - 1,
- XFS_BMAPI_PREALLOC | XFS_BMAPI_CONVERT,
- attr_flags);
+ XFS_BMAPI_PREALLOC | XFS_BMAPI_CONVERT);
if (error)
- goto out_unlock;
+ goto out;
/* We've handled the interior of the range, now for the edges */
- if (start_boundary != offset)
+ if (start_boundary != offset) {
error = xfs_iozero(ip, offset, start_boundary - offset);
- if (error)
- goto out_unlock;
+ if (error)
+ goto out;
+ }
if (end_boundary != offset + len)
error = xfs_iozero(ip, end_boundary,
@@ -2155,9 +2140,7 @@ xfs_zero_file_space(
error = xfs_iozero(ip, offset, len);
}
-out_unlock:
- if (!(attr_flags & XFS_ATTR_NOLOCK))
- xfs_iunlock(ip, XFS_IOLOCK_EXCL);
+out:
return error;
}
@@ -2240,8 +2223,7 @@ xfs_change_file_space(
setprealloc = clrprealloc = 0;
switch (cmd) {
case XFS_IOC_ZERO_RANGE:
- error = xfs_zero_file_space(ip, startoffset, bf->l_len,
- attr_flags);
+ error = xfs_zero_file_space(ip, startoffset, bf->l_len);
if (error)
return error;
setprealloc = 1;
@@ -2250,7 +2232,7 @@ xfs_change_file_space(
case XFS_IOC_RESVSP:
case XFS_IOC_RESVSP64:
error = xfs_alloc_file_space(ip, startoffset, bf->l_len,
- XFS_BMAPI_PREALLOC, attr_flags);
+ XFS_BMAPI_PREALLOC);
if (error)
return error;
setprealloc = 1;
@@ -2258,8 +2240,8 @@ xfs_change_file_space(
case XFS_IOC_UNRESVSP:
case XFS_IOC_UNRESVSP64:
- if ((error = xfs_free_file_space(ip, startoffset, bf->l_len,
- attr_flags)))
+ error = xfs_free_file_space(ip, startoffset, bf->l_len);
+ if (error)
return error;
break;
@@ -2277,22 +2259,17 @@ xfs_change_file_space(
* truncate, direct IO) from racing against the transient
* allocated but not written state we can have here.
*/
- xfs_ilock(ip, XFS_IOLOCK_EXCL);
if (startoffset > fsize) {
error = xfs_alloc_file_space(ip, fsize,
- startoffset - fsize, 0,
- attr_flags | XFS_ATTR_NOLOCK);
- if (error) {
- xfs_iunlock(ip, XFS_IOLOCK_EXCL);
+ startoffset - fsize, 0);
+ if (error)
break;
- }
}
iattr.ia_valid = ATTR_SIZE;
iattr.ia_size = startoffset;
error = xfs_setattr_size(ip, &iattr);
- xfs_iunlock(ip, XFS_IOLOCK_EXCL);
if (error)
return error;
Index: xfs/fs/xfs/xfs_vnodeops.h
===================================================================
--- xfs.orig/fs/xfs/xfs_vnodeops.h 2012-11-30 18:38:51.724019096 -0800
+++ xfs/fs/xfs/xfs_vnodeops.h 2012-11-30 18:39:27.896020019 -0800
@@ -15,7 +15,6 @@ struct xfs_inode;
int xfs_setattr_nonsize(struct xfs_inode *ip, struct iattr *vap, int flags);
int xfs_setattr_size(struct xfs_inode *ip, struct iattr *iattr);
#define XFS_ATTR_DMI 0x01 /* invocation from a DMI function */
-#define XFS_ATTR_NOLOCK 0x04 /* Don't grab any conflicting locks */
#define XFS_ATTR_NOACL 0x08 /* Don't call xfs_acl_chmod */
#define XFS_ATTR_SYNC 0x10 /* synchronous operation required */
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 24+ messages in thread