On Thu, Mar 24, 2011 at 01:18:06PM -0400, brian.foster@xxxxxxx wrote: > > -----Original Message----- > > From: xfs-bounces@xxxxxxxxxxx [mailto:xfs-bounces@xxxxxxxxxxx] On > > Behalf Of Dave Chinner > > Sent: Wednesday, March 23, 2011 2:14 AM > > To: xfs@xxxxxxxxxxx > > Cc: aelder@xxxxxxx > > Subject: [PATCH 1/6] xfs: preallocation transactions do not need to be > > synchronous > > > diff --git a/fs/xfs/linux-2.6/xfs_file.c b/fs/xfs/linux-2.6/xfs_file.c index > > ae59865..baa2cb3 100644 > > --- a/fs/xfs/linux-2.6/xfs_file.c > > +++ b/fs/xfs/linux-2.6/xfs_file.c > > @@ -896,6 +896,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; > > > > if (mode & ~(FALLOC_FL_KEEP_SIZE | > > FALLOC_FL_PUNCH_HOLE | > > @@ -922,6 +923,9 @@ xfs_file_fallocate( > > goto out_unlock; > > } > > > > + if (file->f_flags & O_DSYNC) > > + attr_flags |= XFS_ATTR_SYNC; > > + > > error = -xfs_change_file_space(ip, cmd, &bf, 0, XFS_ATTR_NOLOCK); > > if (error) > > goto out_unlock; > > Where are you passing attr_flags? Argh. Good catch. That's the problem with having two interfaces that do the same thing - you're not sure which one is being tested by the test suite in any given test. Fixed patch below. > I was looking at this because I noticed a nice performance > improvement in some Samba tests with the xfs_trans_set_sync() call > removed, but I have a follow up question... I'd like to back port > this patch to 2.6.34. The majority of this patch applies > (manually), but the segment above is problematic in that I have an > xfs_vn_fallocate() inode operations handler (xfs_iops.c) without > the file pointer rather than the above file operations handler. Yup, .fallocate got moved from an inode operation to a file operation a while back in: 2fe17c1 fallocate should be a file operation > Without really knowing any of this code, I assume I can IS_SYNC() > the inode to check for the sync mount situation? Assuming that is > correct, is there a straightforward way to cover the open(..., > O_SYNC) situation? I suppose I'm open to hacking the VFS to one > off this call if I have to, but would rather avoid the ugliness. > Any comments are appreciated, thanks. Porting the above commit first is probably the only way to get this. Cheers, Dave. -- Dave Chinner david@xxxxxxxxxxxxx xfs: preallocation transactions do not need to be synchronous From: Dave Chinner <dchinner@xxxxxxxxxx> Preallocation and hole punch transactions are currently synchronous and this is causing performance problems in some cases. The transactions don't need to be synchronous as we don't need to guarantee the preallocation is persistent on disk until a fdatasync, fsync, sync operation occurs. If the file is opened O_SYNC or O_DATASYNC, only then should the transaction be issued synchronously. Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx> Reviewed-by: Christoph Hellwig <hch@xxxxxx> --- fs/xfs/linux-2.6/xfs_file.c | 6 +++++- fs/xfs/linux-2.6/xfs_ioctl.c | 4 ++++ fs/xfs/xfs_vnodeops.c | 3 ++- fs/xfs/xfs_vnodeops.h | 1 + 4 files changed, 12 insertions(+), 2 deletions(-) diff --git a/fs/xfs/linux-2.6/xfs_file.c b/fs/xfs/linux-2.6/xfs_file.c index ae59865..0c255b1 100644 --- a/fs/xfs/linux-2.6/xfs_file.c +++ b/fs/xfs/linux-2.6/xfs_file.c @@ -896,6 +896,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; if (mode & ~(FALLOC_FL_KEEP_SIZE | FALLOC_FL_PUNCH_HOLE | @@ -922,7 +923,10 @@ xfs_file_fallocate( goto out_unlock; } - error = -xfs_change_file_space(ip, cmd, &bf, 0, XFS_ATTR_NOLOCK); + if (file->f_flags & O_DSYNC) + attr_flags |= XFS_ATTR_SYNC; + + error = -xfs_change_file_space(ip, cmd, &bf, 0, attr_flags); if (error) goto out_unlock; diff --git a/fs/xfs/linux-2.6/xfs_ioctl.c b/fs/xfs/linux-2.6/xfs_ioctl.c index 0ca0e3c..acca2c5 100644 --- a/fs/xfs/linux-2.6/xfs_ioctl.c +++ b/fs/xfs/linux-2.6/xfs_ioctl.c @@ -624,6 +624,10 @@ xfs_ioc_space( if (filp->f_flags & (O_NDELAY|O_NONBLOCK)) attr_flags |= XFS_ATTR_NONBLOCK; + + if (filp->f_flags & O_DSYNC) + attr_flags |= XFS_ATTR_SYNC; + if (ioflags & IO_INVIS) attr_flags |= XFS_ATTR_DMI; diff --git a/fs/xfs/xfs_vnodeops.c b/fs/xfs/xfs_vnodeops.c index 37d8146..c48b421 100644 --- a/fs/xfs/xfs_vnodeops.c +++ b/fs/xfs/xfs_vnodeops.c @@ -2831,7 +2831,8 @@ xfs_change_file_space( ip->i_d.di_flags &= ~XFS_DIFLAG_PREALLOC; xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE); - xfs_trans_set_sync(tp); + if (attr_flags & XFS_ATTR_SYNC) + xfs_trans_set_sync(tp); error = xfs_trans_commit(tp, 0); diff --git a/fs/xfs/xfs_vnodeops.h b/fs/xfs/xfs_vnodeops.h index f6702927..3bcd233 100644 --- a/fs/xfs/xfs_vnodeops.h +++ b/fs/xfs/xfs_vnodeops.h @@ -18,6 +18,7 @@ int xfs_setattr(struct xfs_inode *ip, struct iattr *vap, int flags); #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 */ int xfs_readlink(struct xfs_inode *ip, char *link); int xfs_release(struct xfs_inode *ip); _______________________________________________ xfs mailing list xfs@xxxxxxxxxxx http://oss.sgi.com/mailman/listinfo/xfs
