* [PATCH 1/5] xfs: remove the unused XFS_ATTR_NONBLOCK flag
2012-12-08 12:08 [PATCH 0/5] refactor the preallocation and hole punching code Christoph Hellwig
@ 2012-12-08 12:08 ` Christoph Hellwig
2012-12-08 12:08 ` [PATCH 2/5] xfs: always take the iolock around xfs_setattr_size Christoph Hellwig
` (3 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2012-12-08 12:08 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-kill-XFS_ATTR_NONBLOCK --]
[-- Type: text/plain, Size: 1514 bytes --]
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/xfs/xfs_ioctl.c | 3 ---
fs/xfs/xfs_vnodeops.h | 1 -
2 files changed, 4 deletions(-)
Index: xfs/fs/xfs/xfs_ioctl.c
===================================================================
--- xfs.orig/fs/xfs/xfs_ioctl.c 2012-12-06 16:04:50.908778675 +0100
+++ xfs/fs/xfs/xfs_ioctl.c 2012-12-06 16:05:13.272111942 +0100
@@ -633,9 +633,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_vnodeops.h
===================================================================
--- xfs.orig/fs/xfs/xfs_vnodeops.h 2012-12-06 16:04:50.908778675 +0100
+++ xfs/fs/xfs/xfs_vnodeops.h 2012-12-06 16:05:18.452111928 +0100
@@ -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 *vap, int flags);
#define XFS_ATTR_DMI 0x01 /* invocation from a DMI function */
-#define XFS_ATTR_NONBLOCK 0x02 /* return EAGAIN if operation 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] 12+ messages in thread* [PATCH 2/5] xfs: always take the iolock around xfs_setattr_size
2012-12-08 12:08 [PATCH 0/5] refactor the preallocation and hole punching code Christoph Hellwig
2012-12-08 12:08 ` [PATCH 1/5] xfs: remove the unused XFS_ATTR_NONBLOCK flag Christoph Hellwig
@ 2012-12-08 12:08 ` Christoph Hellwig
2012-12-08 12:08 ` [PATCH 3/5] xfs: always hold the iolock when calling xfs_change_file_space Christoph Hellwig
` (2 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2012-12-08 12:08 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-cleanup-xfs_setattr_size-locking --]
[-- Type: text/plain, Size: 3910 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_file.c | 2 +-
fs/xfs/xfs_iops.c | 28 +++++++++++++++-------------
fs/xfs/xfs_vnodeops.c | 3 +--
fs/xfs/xfs_vnodeops.h | 2 +-
4 files changed, 18 insertions(+), 17 deletions(-)
Index: xfs/fs/xfs/xfs_file.c
===================================================================
--- xfs.orig/fs/xfs/xfs_file.c 2012-11-30 18:39:45.396020469 -0800
+++ xfs/fs/xfs/xfs_file.c 2012-11-30 18:39:56.240020746 -0800
@@ -853,7 +853,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 2012-11-30 18:39:45.396020469 -0800
+++ xfs/fs/xfs/xfs_iops.c 2012-11-30 18:39:56.240020746 -0800
@@ -689,8 +689,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);
@@ -718,11 +717,6 @@ xfs_setattr_size(
ATTR_MTIME_SET|ATTR_KILL_SUID|ATTR_KILL_SGID|
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;
@@ -736,7 +730,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);
}
@@ -893,12 +886,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_vnodeops.c
===================================================================
--- xfs.orig/fs/xfs/xfs_vnodeops.c 2012-11-30 18:39:45.396020469 -0800
+++ xfs/fs/xfs/xfs_vnodeops.c 2012-11-30 18:39:56.244020743 -0800
@@ -2291,8 +2291,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_vnodeops.h
===================================================================
--- xfs.orig/fs/xfs/xfs_vnodeops.h 2012-11-30 18:39:53.784020685 -0800
+++ xfs/fs/xfs/xfs_vnodeops.h 2012-11-30 18:39:56.244020743 -0800
@@ -13,7 +13,7 @@ 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 *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 */
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 12+ messages in thread* [PATCH 3/5] xfs: always hold the iolock when calling xfs_change_file_space
2012-12-08 12:08 [PATCH 0/5] refactor the preallocation and hole punching code Christoph Hellwig
2012-12-08 12:08 ` [PATCH 1/5] xfs: remove the unused XFS_ATTR_NONBLOCK flag Christoph Hellwig
2012-12-08 12:08 ` [PATCH 2/5] xfs: always take the iolock around xfs_setattr_size Christoph Hellwig
@ 2012-12-08 12:08 ` Christoph Hellwig
2012-12-08 12:08 ` [PATCH 4/5] xfs: simplify the fallocate path Christoph Hellwig
2012-12-08 12:08 ` [PATCH 5/5] xfs: fold xfs_change_file_space into xfs_ioc_space Christoph Hellwig
4 siblings, 0 replies; 12+ 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] 12+ messages in thread* [PATCH 4/5] xfs: simplify the fallocate path
2012-12-08 12:08 [PATCH 0/5] refactor the preallocation and hole punching code Christoph Hellwig
` (2 preceding siblings ...)
2012-12-08 12:08 ` [PATCH 3/5] xfs: always hold the iolock when calling xfs_change_file_space Christoph Hellwig
@ 2012-12-08 12:08 ` Christoph Hellwig
2012-12-10 2:09 ` Dave Chinner
2012-12-08 12:08 ` [PATCH 5/5] xfs: fold xfs_change_file_space into xfs_ioc_space Christoph Hellwig
4 siblings, 1 reply; 12+ messages in thread
From: Christoph Hellwig @ 2012-12-08 12:08 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-simplify-fallocate --]
[-- Type: text/plain, Size: 5998 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_file.c | 81 ++++++++++++++++++++++++++++++++------------------
fs/xfs/xfs_vnodeops.c | 39 ++----------------------
fs/xfs/xfs_vnodeops.h | 3 +
3 files changed, 60 insertions(+), 63 deletions(-)
Index: xfs/fs/xfs/xfs_file.c
===================================================================
--- xfs.orig/fs/xfs/xfs_file.c 2012-11-30 16:52:18.396009912 -0800
+++ xfs/fs/xfs/xfs_file.c 2012-11-30 18:33:53.496011458 -0800
@@ -806,44 +806,69 @@ 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->f_path.dentry->d_inode;
- 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->f_path.dentry->d_inode;
+ struct xfs_inode *ip = XFS_I(inode);
+ struct xfs_trans *tp;
+ bool setprealloc = false;
+ 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;
+ setprealloc = true;
}
- if (file->f_flags & O_DSYNC)
- attr_flags |= XFS_ATTR_SYNC;
- error = -xfs_change_file_space(ip, cmd, &bf, 0, attr_flags);
+ tp = xfs_trans_alloc(ip->i_mount, XFS_TRANS_WRITEID);
+ error = xfs_trans_reserve(tp, 0, XFS_WRITEID_LOG_RES(ip->i_mount),
+ 0, 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;
+
+ 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;
@@ -853,12 +878,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_vnodeops.c
===================================================================
--- xfs.orig/fs/xfs/xfs_vnodeops.c 2012-11-30 16:52:18.396009912 -0800
+++ xfs/fs/xfs/xfs_vnodeops.c 2012-11-30 16:54:23.704013121 -0800
@@ -1627,28 +1627,9 @@ xfs_set_dmattrs(
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)
@@ -1895,21 +1876,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_vnodeops.h
===================================================================
--- xfs.orig/fs/xfs/xfs_vnodeops.h 2012-11-30 16:52:18.400009913 -0800
+++ xfs/fs/xfs/xfs_vnodeops.h 2012-11-30 16:54:23.704013121 -0800
@@ -36,6 +36,9 @@ int xfs_symlink(struct xfs_inode *dp, st
int xfs_set_dmattrs(struct xfs_inode *ip, u_int evmask, u_int16_t state);
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_rename(struct xfs_inode *src_dp, struct xfs_name *src_name,
struct xfs_inode *src_ip, struct xfs_inode *target_dp,
struct xfs_name *target_name, struct xfs_inode *target_ip);
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 12+ messages in thread* Re: [PATCH 4/5] xfs: simplify the fallocate path
2012-12-08 12:08 ` [PATCH 4/5] xfs: simplify the fallocate path Christoph Hellwig
@ 2012-12-10 2:09 ` Dave Chinner
2012-12-10 10:52 ` Christoph Hellwig
0 siblings, 1 reply; 12+ messages in thread
From: Dave Chinner @ 2012-12-10 2:09 UTC (permalink / raw)
To: Christoph Hellwig; +Cc: xfs
On Sat, Dec 08, 2012 at 07:08:16AM -0500, 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.
.....
> goto out_unlock;
> + setprealloc = true;
You don't use this flag anywhere ;)
> }
>
> - if (file->f_flags & O_DSYNC)
> - attr_flags |= XFS_ATTR_SYNC;
>
> - error = -xfs_change_file_space(ip, cmd, &bf, 0, attr_flags);
> + tp = xfs_trans_alloc(ip->i_mount, XFS_TRANS_WRITEID);
> + error = xfs_trans_reserve(tp, 0, XFS_WRITEID_LOG_RES(ip->i_mount),
> + 0, 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;
> +
> + 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);
While I like most of this series, I don't really like the
duplication of this piece of code. It seems to me that a simple
helper like:
int
xfs_inode_set_prealloc(
struct xfs_inode *ip,
bool set_prealloc,
bool clear_prealloc,
bool clear_sguid,
bool sync)
might be better, and call it from both the ioctl and fallocate
code...
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] 12+ messages in thread* Re: [PATCH 4/5] xfs: simplify the fallocate path
2012-12-10 2:09 ` Dave Chinner
@ 2012-12-10 10:52 ` Christoph Hellwig
0 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2012-12-10 10:52 UTC (permalink / raw)
To: Dave Chinner; +Cc: Christoph Hellwig, xfs
On Mon, Dec 10, 2012 at 01:09:56PM +1100, Dave Chinner wrote:
> You don't use this flag anywhere ;)
Fixed.
> While I like most of this series, I don't really like the
> duplication of this piece of code. It seems to me that a simple
> helper like:
I had that in a previous version, but it seemed uglier than just
opencoding it. I'll give it another try, maybe I can come up
with something nicer now.
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 5/5] xfs: fold xfs_change_file_space into xfs_ioc_space
2012-12-08 12:08 [PATCH 0/5] refactor the preallocation and hole punching code Christoph Hellwig
` (3 preceding siblings ...)
2012-12-08 12:08 ` [PATCH 4/5] xfs: simplify the fallocate path Christoph Hellwig
@ 2012-12-08 12:08 ` Christoph Hellwig
4 siblings, 0 replies; 12+ messages in thread
From: Christoph Hellwig @ 2012-12-08 12:08 UTC (permalink / raw)
To: xfs
[-- Attachment #1: xfs-kill-xfs_change_file_space-2 --]
[-- Type: text/plain, Size: 10539 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_ioctl.c | 130 +++++++++++++++++++++++++++++++++---
fs/xfs/xfs_vnodeops.c | 180 --------------------------------------------------
fs/xfs/xfs_vnodeops.h | 7 -
3 files changed, 125 insertions(+), 192 deletions(-)
Index: xfs/fs/xfs/xfs_ioctl.c
===================================================================
--- xfs.orig/fs/xfs/xfs_ioctl.c 2012-11-30 18:39:57.876020790 -0800
+++ xfs/fs/xfs/xfs_ioctl.c 2012-11-30 18:43:52.728026802 -0800
@@ -613,7 +613,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;
/*
@@ -633,17 +637,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, 0, XFS_WRITEID_LOG_RES(mp), 0, 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_vnodeops.c
===================================================================
--- xfs.orig/fs/xfs/xfs_vnodeops.c 2012-11-30 18:42:17.796024371 -0800
+++ xfs/fs/xfs/xfs_vnodeops.c 2012-11-30 18:43:52.728026802 -0800
@@ -2054,7 +2054,7 @@ xfs_free_file_space(
}
-STATIC int
+int
xfs_zero_file_space(
struct xfs_inode *ip,
xfs_off_t offset,
@@ -2113,181 +2113,3 @@ out:
return error;
}
-
-/*
- * 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);
-
- if ((error = xfs_trans_reserve(tp, 0, XFS_WRITEID_LOG_RES(mp),
- 0, 0, 0))) {
- /* ASSERT(0); */
- 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);
-}
Index: xfs/fs/xfs/xfs_vnodeops.h
===================================================================
--- xfs.orig/fs/xfs/xfs_vnodeops.h 2012-11-30 18:42:17.796024371 -0800
+++ xfs/fs/xfs/xfs_vnodeops.h 2012-11-30 18:43:52.732026802 -0800
@@ -14,9 +14,7 @@ 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_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 */
int xfs_readlink(struct xfs_inode *ip, char *link);
int xfs_release(struct xfs_inode *ip);
@@ -34,11 +32,10 @@ int xfs_readdir(struct xfs_inode *dp, vo
int xfs_symlink(struct xfs_inode *dp, struct xfs_name *link_name,
const char *target_path, umode_t mode, struct xfs_inode **ipp);
int xfs_set_dmattrs(struct xfs_inode *ip, u_int evmask, u_int16_t state);
-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);
int xfs_rename(struct xfs_inode *src_dp, struct xfs_name *src_name,
struct xfs_inode *src_ip, struct xfs_inode *target_dp,
struct xfs_name *target_name, struct xfs_inode *target_ip);
_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs
^ permalink raw reply [flat|nested] 12+ messages in thread