[PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Ankit Jain]

Al, Could this be included in the vfs queue?

This patch adds ioctls to vfs for compatibility with legacy XFS
pre-allocation ioctls (XFS_IOC_*RESVP*). The implementation
effectively invokes sys_fallocate for the new ioctls.
Also handles the compat_ioctl case.
Note: These legacy ioctls are also implemented by OCFS2.

Signed-off-by: Ankit Jain <me@ankitjain.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Tested-by: Christoph Hellwig <hch@lst.de>
---
 fs/compat_ioctl.c      |   29 +++++++++++++++++++++++++++
 fs/ioctl.c             |   35 ++++++++++++++++++++++++++++++++
 fs/open.c              |   51 +++++++++++++++++++++++------------------------
 include/linux/falloc.h |   44 +++++++++++++++++++++++++++++++++++++++++
 include/linux/fs.h     |    6 +++++
 5 files changed, 139 insertions(+), 26 deletions(-)

Index: xfs/fs/compat_ioctl.c
===================================================================
--- xfs.orig/fs/compat_ioctl.c	2009-01-21 21:03:26.217449883 +0100
+++ xfs/fs/compat_ioctl.c	2009-01-27 20:36:59.189424147 +0100
@@ -2765,6 +2765,26 @@ static void compat_ioctl_error(struct fi
 		free_page((unsigned long)path);
 }
 
+#ifdef BROKEN_X86_ALIGNMENT
+/* just account for different alignment */
+static unsigned long copy_to_space_resv(unsigned long arg)
+{
+	struct space_resv_32	__user *p32 = (void __user *)arg;
+	struct space_resv		__user *p = compat_alloc_user_space(sizeof(*p));
+
+	if (copy_in_user(&p->l_type,	&p32->l_type,	sizeof(s16)) ||
+	    copy_in_user(&p->l_whence,	&p32->l_whence, sizeof(s16)) ||
+	    copy_in_user(&p->l_start,	&p32->l_start,	sizeof(s64)) ||
+	    copy_in_user(&p->l_len,	&p32->l_len,	sizeof(s64)) ||
+	    copy_in_user(&p->l_sysid,	&p32->l_sysid,	sizeof(s32)) ||
+	    copy_in_user(&p->l_pid,	&p32->l_pid,	sizeof(u32)) ||
+	    copy_in_user(&p->l_pad,	&p32->l_pad,	4*sizeof(u32)))
+		return -EFAULT;
+
+	return (unsigned long)p;
+}
+#endif
+
 asmlinkage long compat_sys_ioctl(unsigned int fd, unsigned int cmd,
 				unsigned long arg)
 {
@@ -2795,6 +2815,15 @@ asmlinkage long compat_sys_ioctl(unsigne
 	case FIOQSIZE:
 		break;
 
+	case F_IOC_RESVSP_32:
+	case F_IOC_RESVSP64_32:
+#ifdef BROKEN_X86_ALIGNMENT
+		arg = copy_to_space_resv(arg);
+		cmd = _NATIVE_IOC(cmd, struct space_resv);
+#endif
+		error = ioctl_preallocate(filp, arg);
+		goto out_fput;
+
 	case FIBMAP:
 	case FIGETBSZ:
 	case FIONREAD:
Index: xfs/fs/ioctl.c
===================================================================
--- xfs.orig/fs/ioctl.c	2009-01-21 21:03:27.321448363 +0100
+++ xfs/fs/ioctl.c	2009-01-27 20:36:59.189424147 +0100
@@ -15,6 +15,7 @@
 #include <linux/uaccess.h>
 #include <linux/writeback.h>
 #include <linux/buffer_head.h>
+#include <linux/falloc.h>
 
 #include <asm/ioctls.h>
 
@@ -370,6 +371,37 @@ EXPORT_SYMBOL(generic_block_fiemap);
 
 #endif  /*  CONFIG_BLOCK  */
 
+/*
+ * This provides compatibility with legacy XFS pre-allocation ioctls
+ * which predate the fallocate syscall.
+ *
+ * Only the l_start, l_len and l_whence fields of the 'struct space_resv'
+ * are used here, rest are ignored.
+ */
+int ioctl_preallocate(struct file *filp, unsigned long arg)
+{
+	struct inode *inode = filp->f_path.dentry->d_inode;
+	struct space_resv sr;
+
+	if (copy_from_user(&sr, (struct space_resv __user *) arg, sizeof(sr)))
+		return -EFAULT;
+
+	switch (sr.l_whence) {
+	case SEEK_SET:
+		break;
+	case SEEK_CUR:
+		sr.l_start += filp->f_pos;
+		break;
+	case SEEK_END:
+		sr.l_start += i_size_read(inode);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return do_fallocate(filp, FALLOC_FL_KEEP_SIZE, sr.l_start, sr.l_len);
+}
+
 static int file_ioctl(struct file *filp, unsigned int cmd,
 		unsigned long arg)
 {
@@ -385,6 +417,9 @@ static int file_ioctl(struct file *filp,
 		return put_user(inode->i_sb->s_blocksize, p);
 	case FIONREAD:
 		return put_user(i_size_read(inode) - filp->f_pos, p);
+	case F_IOC_RESVSP:
+	case F_IOC_RESVSP64:
+		return ioctl_preallocate(filp, arg);
 	}
 
 	return vfs_ioctl(filp, cmd, arg);
Index: xfs/fs/open.c
===================================================================
--- xfs.orig/fs/open.c	2009-01-21 21:03:27.740294372 +0100
+++ xfs/fs/open.c	2009-01-27 20:41:38.208298998 +0100
@@ -377,63 +377,63 @@ SYSCALL_ALIAS(sys_ftruncate64, SyS_ftrun
 #endif
 #endif /* BITS_PER_LONG == 32 */
 
-SYSCALL_DEFINE(fallocate)(int fd, int mode, loff_t offset, loff_t len)
+
+int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 {
-	struct file *file;
-	struct inode *inode;
-	long ret = -EINVAL;
+	struct inode *inode = file->f_path.dentry->d_inode;
+	long ret;
 
 	if (offset < 0 || len <= 0)
-		goto out;
+		return -EINVAL;
 
 	/* Return error if mode is not supported */
-	ret = -EOPNOTSUPP;
 	if (mode && !(mode & FALLOC_FL_KEEP_SIZE))
-		goto out;
+		return -EOPNOTSUPP;
 
-	ret = -EBADF;
-	file = fget(fd);
-	if (!file)
-		goto out;
 	if (!(file->f_mode & FMODE_WRITE))
-		goto out_fput;
+		return -EBADF;
 	/*
 	 * Revalidate the write permissions, in case security policy has
 	 * changed since the files were opened.
 	 */
 	ret = security_file_permission(file, MAY_WRITE);
 	if (ret)
-		goto out_fput;
+		return ret;
 
-	inode = file->f_path.dentry->d_inode;
-
-	ret = -ESPIPE;
 	if (S_ISFIFO(inode->i_mode))
-		goto out_fput;
+		return -ESPIPE;
 
-	ret = -ENODEV;
 	/*
 	 * Let individual file system decide if it supports preallocation
 	 * for directories or not.
 	 */
 	if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
-		goto out_fput;
+		return -ENODEV;
 
-	ret = -EFBIG;
 	/* Check for wrap through zero too */
 	if (((offset + len) > inode->i_sb->s_maxbytes) || ((offset + len) < 0))
-		goto out_fput;
+		return -EFBIG;
 
-	if (inode->i_op->fallocate)
-		ret = inode->i_op->fallocate(inode, mode, offset, len);
-	else
-		ret = -EOPNOTSUPP;
+	if (!inode->i_op->fallocate)
+		return -EOPNOTSUPP;
 
-out_fput:
-	fput(file);
-out:
-	return ret;
+	return inode->i_op->fallocate(inode, mode, offset, len);
 }
+
+SYSCALL_DEFINE(fallocate)(int fd, int mode, loff_t offset, loff_t len)
+{
+	struct file *file;
+	int error = -EBADF;
+
+	file = fget(fd);
+	if (file) {
+		error = do_fallocate(file, mode, offset, len);
+		fput(file);
+	}
+
+	return error;
+}
+
 #ifdef CONFIG_HAVE_SYSCALL_WRAPPERS
 asmlinkage long SyS_fallocate(long fd, long mode, loff_t offset, loff_t len)
 {
Index: xfs/include/linux/falloc.h
===================================================================
--- xfs.orig/include/linux/falloc.h	2009-01-21 21:03:28.076324621 +0100
+++ xfs/include/linux/falloc.h	2009-01-27 20:36:59.190423995 +0100
@@ -3,4 +3,48 @@
 
 #define FALLOC_FL_KEEP_SIZE	0x01 /* default is extend size */
 
+#ifdef __KERNEL__
+
+/*
+ * Space reservation ioctls and argument structure
+ * are designed to be compatible with the legacy XFS ioctls.
+ */
+struct space_resv {
+	__s16		l_type;
+	__s16		l_whence;
+	__s64		l_start;
+	__s64		l_len;		/* len == 0 means until end of file */
+	__s32		l_sysid;
+	__u32		l_pid;
+	__s32		l_pad[4];	/* reserve area			    */
+};
+
+#define F_IOC_RESVSP		_IOW('X', 40, struct space_resv)
+#define F_IOC_RESVSP64		_IOW('X', 42, struct space_resv)
+
+#if defined(CONFIG_IA64) || defined(CONFIG_X86_64)
+#define BROKEN_X86_ALIGNMENT
+
+#define  _NATIVE_IOC(cmd, type) \
+	  _IOC(_IOC_DIR(cmd), _IOC_TYPE(cmd), _IOC_NR(cmd), sizeof(type))
+
+/* on ia32 l_start is on a 32-bit boundary */
+struct space_resv_32 {
+	__s16		l_type;
+	__s16		l_whence;
+	__s64		l_start	__attribute__((packed));
+			/* len == 0 means until end of file */
+	__s64		l_len __attribute__((packed));
+	__s32		l_sysid;
+	__u32		l_pid;
+	__s32		l_pad[4];	/* reserve area */
+};
+
+#define F_IOC_RESVSP_32		_IOW('X', 40, struct space_resv_32)
+#define F_IOC_RESVSP64_32	_IOW('X', 42, struct space_resv_32)
+
+#endif
+
+#endif /* __KERNEL__ */
+
 #endif /* _FALLOC_H_ */
Index: xfs/include/linux/fs.h
===================================================================
--- xfs.orig/include/linux/fs.h	2009-01-24 17:58:48.960904090 +0100
+++ xfs/include/linux/fs.h	2009-01-27 20:42:01.675299140 +0100
@@ -1694,6 +1694,8 @@ static inline int break_lease(struct ino
 
 extern int do_truncate(struct dentry *, loff_t start, unsigned int time_attrs,
 		       struct file *filp);
+extern int do_fallocate(struct file *file, int mode, loff_t offset,
+			loff_t len);
 extern long do_sys_open(int dfd, const char __user *filename, int flags,
 			int mode);
 extern struct file *filp_open(const char *, int, int);
@@ -1702,6 +1704,10 @@ extern struct file * dentry_open(struct 
 extern int filp_close(struct file *, fl_owner_t id);
 extern char * getname(const char __user *);
 
+/* fs/ioctl.c */
+
+extern int ioctl_preallocate(struct file *filp, unsigned long arg);
+
 /* fs/dcache.c */
 extern void __init vfs_caches_init_early(void);
 extern void __init vfs_caches_init(unsigned long);

[PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Ankit Jain]

Al, Could this be included in the vfs queue?

This patch adds ioctls to vfs for compatibility with legacy XFS
pre-allocation ioctls (XFS_IOC_*RESVP*). The implementation
effectively invokes sys_fallocate for the new ioctls.
Also handles the compat_ioctl case.
Note: These legacy ioctls are also implemented by OCFS2.

Signed-off-by: Ankit Jain <me@ankitjain.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Tested-by: Christoph Hellwig <hch@lst.de>
---
 fs/compat_ioctl.c      |   29 +++++++++++++++++++++++++++
 fs/ioctl.c             |   35 ++++++++++++++++++++++++++++++++
 fs/open.c              |   51 +++++++++++++++++++++++------------------------
 include/linux/falloc.h |   44 +++++++++++++++++++++++++++++++++++++++++
 include/linux/fs.h     |    6 +++++
 5 files changed, 139 insertions(+), 26 deletions(-)

Index: xfs/fs/compat_ioctl.c
===================================================================
--- xfs.orig/fs/compat_ioctl.c	2009-01-21 21:03:26.217449883 +0100
+++ xfs/fs/compat_ioctl.c	2009-01-27 20:36:59.189424147 +0100
@@ -2765,6 +2765,26 @@ static void compat_ioctl_error(struct fi
 		free_page((unsigned long)path);
 }
 
+#ifdef BROKEN_X86_ALIGNMENT
+/* just account for different alignment */
+static unsigned long copy_to_space_resv(unsigned long arg)
+{
+	struct space_resv_32	__user *p32 = (void __user *)arg;
+	struct space_resv		__user *p = compat_alloc_user_space(sizeof(*p));
+
+	if (copy_in_user(&p->l_type,	&p32->l_type,	sizeof(s16)) ||
+	    copy_in_user(&p->l_whence,	&p32->l_whence, sizeof(s16)) ||
+	    copy_in_user(&p->l_start,	&p32->l_start,	sizeof(s64)) ||
+	    copy_in_user(&p->l_len,	&p32->l_len,	sizeof(s64)) ||
+	    copy_in_user(&p->l_sysid,	&p32->l_sysid,	sizeof(s32)) ||
+	    copy_in_user(&p->l_pid,	&p32->l_pid,	sizeof(u32)) ||
+	    copy_in_user(&p->l_pad,	&p32->l_pad,	4*sizeof(u32)))
+		return -EFAULT;
+
+	return (unsigned long)p;
+}
+#endif
+
 asmlinkage long compat_sys_ioctl(unsigned int fd, unsigned int cmd,
 				unsigned long arg)
 {
@@ -2795,6 +2815,15 @@ asmlinkage long compat_sys_ioctl(unsigne
 	case FIOQSIZE:
 		break;
 
+	case F_IOC_RESVSP_32:
+	case F_IOC_RESVSP64_32:
+#ifdef BROKEN_X86_ALIGNMENT
+		arg = copy_to_space_resv(arg);
+		cmd = _NATIVE_IOC(cmd, struct space_resv);
+#endif
+		error = ioctl_preallocate(filp, arg);
+		goto out_fput;
+
 	case FIBMAP:
 	case FIGETBSZ:
 	case FIONREAD:
Index: xfs/fs/ioctl.c
===================================================================
--- xfs.orig/fs/ioctl.c	2009-01-21 21:03:27.321448363 +0100
+++ xfs/fs/ioctl.c	2009-01-27 20:36:59.189424147 +0100
@@ -15,6 +15,7 @@
 #include <linux/uaccess.h>
 #include <linux/writeback.h>
 #include <linux/buffer_head.h>
+#include <linux/falloc.h>
 
 #include <asm/ioctls.h>
 
@@ -370,6 +371,37 @@ EXPORT_SYMBOL(generic_block_fiemap);
 
 #endif  /*  CONFIG_BLOCK  */
 
+/*
+ * This provides compatibility with legacy XFS pre-allocation ioctls
+ * which predate the fallocate syscall.
+ *
+ * Only the l_start, l_len and l_whence fields of the 'struct space_resv'
+ * are used here, rest are ignored.
+ */
+int ioctl_preallocate(struct file *filp, unsigned long arg)
+{
+	struct inode *inode = filp->f_path.dentry->d_inode;
+	struct space_resv sr;
+
+	if (copy_from_user(&sr, (struct space_resv __user *) arg, sizeof(sr)))
+		return -EFAULT;
+
+	switch (sr.l_whence) {
+	case SEEK_SET:
+		break;
+	case SEEK_CUR:
+		sr.l_start += filp->f_pos;
+		break;
+	case SEEK_END:
+		sr.l_start += i_size_read(inode);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return do_fallocate(filp, FALLOC_FL_KEEP_SIZE, sr.l_start, sr.l_len);
+}
+
 static int file_ioctl(struct file *filp, unsigned int cmd,
 		unsigned long arg)
 {
@@ -385,6 +417,9 @@ static int file_ioctl(struct file *filp,
 		return put_user(inode->i_sb->s_blocksize, p);
 	case FIONREAD:
 		return put_user(i_size_read(inode) - filp->f_pos, p);
+	case F_IOC_RESVSP:
+	case F_IOC_RESVSP64:
+		return ioctl_preallocate(filp, arg);
 	}
 
 	return vfs_ioctl(filp, cmd, arg);
Index: xfs/fs/open.c
===================================================================
--- xfs.orig/fs/open.c	2009-01-21 21:03:27.740294372 +0100
+++ xfs/fs/open.c	2009-01-27 20:41:38.208298998 +0100
@@ -377,63 +377,63 @@ SYSCALL_ALIAS(sys_ftruncate64, SyS_ftrun
 #endif
 #endif /* BITS_PER_LONG == 32 */
 
-SYSCALL_DEFINE(fallocate)(int fd, int mode, loff_t offset, loff_t len)
+
+int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 {
-	struct file *file;
-	struct inode *inode;
-	long ret = -EINVAL;
+	struct inode *inode = file->f_path.dentry->d_inode;
+	long ret;
 
 	if (offset < 0 || len <= 0)
-		goto out;
+		return -EINVAL;
 
 	/* Return error if mode is not supported */
-	ret = -EOPNOTSUPP;
 	if (mode && !(mode & FALLOC_FL_KEEP_SIZE))
-		goto out;
+		return -EOPNOTSUPP;
 
-	ret = -EBADF;
-	file = fget(fd);
-	if (!file)
-		goto out;
 	if (!(file->f_mode & FMODE_WRITE))
-		goto out_fput;
+		return -EBADF;
 	/*
 	 * Revalidate the write permissions, in case security policy has
 	 * changed since the files were opened.
 	 */
 	ret = security_file_permission(file, MAY_WRITE);
 	if (ret)
-		goto out_fput;
+		return ret;
 
-	inode = file->f_path.dentry->d_inode;
-
-	ret = -ESPIPE;
 	if (S_ISFIFO(inode->i_mode))
-		goto out_fput;
+		return -ESPIPE;
 
-	ret = -ENODEV;
 	/*
 	 * Let individual file system decide if it supports preallocation
 	 * for directories or not.
 	 */
 	if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
-		goto out_fput;
+		return -ENODEV;
 
-	ret = -EFBIG;
 	/* Check for wrap through zero too */
 	if (((offset + len) > inode->i_sb->s_maxbytes) || ((offset + len) < 0))
-		goto out_fput;
+		return -EFBIG;
 
-	if (inode->i_op->fallocate)
-		ret = inode->i_op->fallocate(inode, mode, offset, len);
-	else
-		ret = -EOPNOTSUPP;
+	if (!inode->i_op->fallocate)
+		return -EOPNOTSUPP;
 
-out_fput:
-	fput(file);
-out:
-	return ret;
+	return inode->i_op->fallocate(inode, mode, offset, len);
 }
+
+SYSCALL_DEFINE(fallocate)(int fd, int mode, loff_t offset, loff_t len)
+{
+	struct file *file;
+	int error = -EBADF;
+
+	file = fget(fd);
+	if (file) {
+		error = do_fallocate(file, mode, offset, len);
+		fput(file);
+	}
+
+	return error;
+}
+
 #ifdef CONFIG_HAVE_SYSCALL_WRAPPERS
 asmlinkage long SyS_fallocate(long fd, long mode, loff_t offset, loff_t len)
 {
Index: xfs/include/linux/falloc.h
===================================================================
--- xfs.orig/include/linux/falloc.h	2009-01-21 21:03:28.076324621 +0100
+++ xfs/include/linux/falloc.h	2009-01-27 20:36:59.190423995 +0100
@@ -3,4 +3,48 @@
 
 #define FALLOC_FL_KEEP_SIZE	0x01 /* default is extend size */
 
+#ifdef __KERNEL__
+
+/*
+ * Space reservation ioctls and argument structure
+ * are designed to be compatible with the legacy XFS ioctls.
+ */
+struct space_resv {
+	__s16		l_type;
+	__s16		l_whence;
+	__s64		l_start;
+	__s64		l_len;		/* len == 0 means until end of file */
+	__s32		l_sysid;
+	__u32		l_pid;
+	__s32		l_pad[4];	/* reserve area			    */
+};
+
+#define F_IOC_RESVSP		_IOW('X', 40, struct space_resv)
+#define F_IOC_RESVSP64		_IOW('X', 42, struct space_resv)
+
+#if defined(CONFIG_IA64) || defined(CONFIG_X86_64)
+#define BROKEN_X86_ALIGNMENT
+
+#define  _NATIVE_IOC(cmd, type) \
+	  _IOC(_IOC_DIR(cmd), _IOC_TYPE(cmd), _IOC_NR(cmd), sizeof(type))
+
+/* on ia32 l_start is on a 32-bit boundary */
+struct space_resv_32 {
+	__s16		l_type;
+	__s16		l_whence;
+	__s64		l_start	__attribute__((packed));
+			/* len == 0 means until end of file */
+	__s64		l_len __attribute__((packed));
+	__s32		l_sysid;
+	__u32		l_pid;
+	__s32		l_pad[4];	/* reserve area */
+};
+
+#define F_IOC_RESVSP_32		_IOW('X', 40, struct space_resv_32)
+#define F_IOC_RESVSP64_32	_IOW('X', 42, struct space_resv_32)
+
+#endif
+
+#endif /* __KERNEL__ */
+
 #endif /* _FALLOC_H_ */
Index: xfs/include/linux/fs.h
===================================================================
--- xfs.orig/include/linux/fs.h	2009-01-24 17:58:48.960904090 +0100
+++ xfs/include/linux/fs.h	2009-01-27 20:42:01.675299140 +0100
@@ -1694,6 +1694,8 @@ static inline int break_lease(struct ino
 
 extern int do_truncate(struct dentry *, loff_t start, unsigned int time_attrs,
 		       struct file *filp);
+extern int do_fallocate(struct file *file, int mode, loff_t offset,
+			loff_t len);
 extern long do_sys_open(int dfd, const char __user *filename, int flags,
 			int mode);
 extern struct file *filp_open(const char *, int, int);
@@ -1702,6 +1704,10 @@ extern struct file * dentry_open(struct 
 extern int filp_close(struct file *, fl_owner_t id);
 extern char * getname(const char __user *);
 
+/* fs/ioctl.c */
+
+extern int ioctl_preallocate(struct file *filp, unsigned long arg);
+
 /* fs/dcache.c */
 extern void __init vfs_caches_init_early(void);
 extern void __init vfs_caches_init(unsigned long);

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

[Ocfs2-devel] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Ankit Jain]

Al, Could this be included in the vfs queue?

This patch adds ioctls to vfs for compatibility with legacy XFS
pre-allocation ioctls (XFS_IOC_*RESVP*). The implementation
effectively invokes sys_fallocate for the new ioctls.
Also handles the compat_ioctl case.
Note: These legacy ioctls are also implemented by OCFS2.

Signed-off-by: Ankit Jain <me@ankitjain.org>
Reviewed-by: Christoph Hellwig <hch@lst.de>
Tested-by: Christoph Hellwig <hch@lst.de>
---
 fs/compat_ioctl.c      |   29 +++++++++++++++++++++++++++
 fs/ioctl.c             |   35 ++++++++++++++++++++++++++++++++
 fs/open.c              |   51 +++++++++++++++++++++++------------------------
 include/linux/falloc.h |   44 +++++++++++++++++++++++++++++++++++++++++
 include/linux/fs.h     |    6 +++++
 5 files changed, 139 insertions(+), 26 deletions(-)

Index: xfs/fs/compat_ioctl.c
===================================================================
--- xfs.orig/fs/compat_ioctl.c	2009-01-21 21:03:26.217449883 +0100
+++ xfs/fs/compat_ioctl.c	2009-01-27 20:36:59.189424147 +0100
@@ -2765,6 +2765,26 @@ static void compat_ioctl_error(struct fi
 		free_page((unsigned long)path);
 }
 
+#ifdef BROKEN_X86_ALIGNMENT
+/* just account for different alignment */
+static unsigned long copy_to_space_resv(unsigned long arg)
+{
+	struct space_resv_32	__user *p32 = (void __user *)arg;
+	struct space_resv		__user *p = compat_alloc_user_space(sizeof(*p));
+
+	if (copy_in_user(&p->l_type,	&p32->l_type,	sizeof(s16)) ||
+	    copy_in_user(&p->l_whence,	&p32->l_whence, sizeof(s16)) ||
+	    copy_in_user(&p->l_start,	&p32->l_start,	sizeof(s64)) ||
+	    copy_in_user(&p->l_len,	&p32->l_len,	sizeof(s64)) ||
+	    copy_in_user(&p->l_sysid,	&p32->l_sysid,	sizeof(s32)) ||
+	    copy_in_user(&p->l_pid,	&p32->l_pid,	sizeof(u32)) ||
+	    copy_in_user(&p->l_pad,	&p32->l_pad,	4*sizeof(u32)))
+		return -EFAULT;
+
+	return (unsigned long)p;
+}
+#endif
+
 asmlinkage long compat_sys_ioctl(unsigned int fd, unsigned int cmd,
 				unsigned long arg)
 {
@@ -2795,6 +2815,15 @@ asmlinkage long compat_sys_ioctl(unsigne
 	case FIOQSIZE:
 		break;
 
+	case F_IOC_RESVSP_32:
+	case F_IOC_RESVSP64_32:
+#ifdef BROKEN_X86_ALIGNMENT
+		arg = copy_to_space_resv(arg);
+		cmd = _NATIVE_IOC(cmd, struct space_resv);
+#endif
+		error = ioctl_preallocate(filp, arg);
+		goto out_fput;
+
 	case FIBMAP:
 	case FIGETBSZ:
 	case FIONREAD:
Index: xfs/fs/ioctl.c
===================================================================
--- xfs.orig/fs/ioctl.c	2009-01-21 21:03:27.321448363 +0100
+++ xfs/fs/ioctl.c	2009-01-27 20:36:59.189424147 +0100
@@ -15,6 +15,7 @@
 #include <linux/uaccess.h>
 #include <linux/writeback.h>
 #include <linux/buffer_head.h>
+#include <linux/falloc.h>
 
 #include <asm/ioctls.h>
 
@@ -370,6 +371,37 @@ EXPORT_SYMBOL(generic_block_fiemap);
 
 #endif  /*  CONFIG_BLOCK  */
 
+/*
+ * This provides compatibility with legacy XFS pre-allocation ioctls
+ * which predate the fallocate syscall.
+ *
+ * Only the l_start, l_len and l_whence fields of the 'struct space_resv'
+ * are used here, rest are ignored.
+ */
+int ioctl_preallocate(struct file *filp, unsigned long arg)
+{
+	struct inode *inode = filp->f_path.dentry->d_inode;
+	struct space_resv sr;
+
+	if (copy_from_user(&sr, (struct space_resv __user *) arg, sizeof(sr)))
+		return -EFAULT;
+
+	switch (sr.l_whence) {
+	case SEEK_SET:
+		break;
+	case SEEK_CUR:
+		sr.l_start += filp->f_pos;
+		break;
+	case SEEK_END:
+		sr.l_start += i_size_read(inode);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return do_fallocate(filp, FALLOC_FL_KEEP_SIZE, sr.l_start, sr.l_len);
+}
+
 static int file_ioctl(struct file *filp, unsigned int cmd,
 		unsigned long arg)
 {
@@ -385,6 +417,9 @@ static int file_ioctl(struct file *filp,
 		return put_user(inode->i_sb->s_blocksize, p);
 	case FIONREAD:
 		return put_user(i_size_read(inode) - filp->f_pos, p);
+	case F_IOC_RESVSP:
+	case F_IOC_RESVSP64:
+		return ioctl_preallocate(filp, arg);
 	}
 
 	return vfs_ioctl(filp, cmd, arg);
Index: xfs/fs/open.c
===================================================================
--- xfs.orig/fs/open.c	2009-01-21 21:03:27.740294372 +0100
+++ xfs/fs/open.c	2009-01-27 20:41:38.208298998 +0100
@@ -377,63 +377,63 @@ SYSCALL_ALIAS(sys_ftruncate64, SyS_ftrun
 #endif
 #endif /* BITS_PER_LONG == 32 */
 
-SYSCALL_DEFINE(fallocate)(int fd, int mode, loff_t offset, loff_t len)
+
+int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 {
-	struct file *file;
-	struct inode *inode;
-	long ret = -EINVAL;
+	struct inode *inode = file->f_path.dentry->d_inode;
+	long ret;
 
 	if (offset < 0 || len <= 0)
-		goto out;
+		return -EINVAL;
 
 	/* Return error if mode is not supported */
-	ret = -EOPNOTSUPP;
 	if (mode && !(mode & FALLOC_FL_KEEP_SIZE))
-		goto out;
+		return -EOPNOTSUPP;
 
-	ret = -EBADF;
-	file = fget(fd);
-	if (!file)
-		goto out;
 	if (!(file->f_mode & FMODE_WRITE))
-		goto out_fput;
+		return -EBADF;
 	/*
 	 * Revalidate the write permissions, in case security policy has
 	 * changed since the files were opened.
 	 */
 	ret = security_file_permission(file, MAY_WRITE);
 	if (ret)
-		goto out_fput;
+		return ret;
 
-	inode = file->f_path.dentry->d_inode;
-
-	ret = -ESPIPE;
 	if (S_ISFIFO(inode->i_mode))
-		goto out_fput;
+		return -ESPIPE;
 
-	ret = -ENODEV;
 	/*
 	 * Let individual file system decide if it supports preallocation
 	 * for directories or not.
 	 */
 	if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
-		goto out_fput;
+		return -ENODEV;
 
-	ret = -EFBIG;
 	/* Check for wrap through zero too */
 	if (((offset + len) > inode->i_sb->s_maxbytes) || ((offset + len) < 0))
-		goto out_fput;
+		return -EFBIG;
 
-	if (inode->i_op->fallocate)
-		ret = inode->i_op->fallocate(inode, mode, offset, len);
-	else
-		ret = -EOPNOTSUPP;
+	if (!inode->i_op->fallocate)
+		return -EOPNOTSUPP;
 
-out_fput:
-	fput(file);
-out:
-	return ret;
+	return inode->i_op->fallocate(inode, mode, offset, len);
 }
+
+SYSCALL_DEFINE(fallocate)(int fd, int mode, loff_t offset, loff_t len)
+{
+	struct file *file;
+	int error = -EBADF;
+
+	file = fget(fd);
+	if (file) {
+		error = do_fallocate(file, mode, offset, len);
+		fput(file);
+	}
+
+	return error;
+}
+
 #ifdef CONFIG_HAVE_SYSCALL_WRAPPERS
 asmlinkage long SyS_fallocate(long fd, long mode, loff_t offset, loff_t len)
 {
Index: xfs/include/linux/falloc.h
===================================================================
--- xfs.orig/include/linux/falloc.h	2009-01-21 21:03:28.076324621 +0100
+++ xfs/include/linux/falloc.h	2009-01-27 20:36:59.190423995 +0100
@@ -3,4 +3,48 @@
 
 #define FALLOC_FL_KEEP_SIZE	0x01 /* default is extend size */
 
+#ifdef __KERNEL__
+
+/*
+ * Space reservation ioctls and argument structure
+ * are designed to be compatible with the legacy XFS ioctls.
+ */
+struct space_resv {
+	__s16		l_type;
+	__s16		l_whence;
+	__s64		l_start;
+	__s64		l_len;		/* len == 0 means until end of file */
+	__s32		l_sysid;
+	__u32		l_pid;
+	__s32		l_pad[4];	/* reserve area			    */
+};
+
+#define F_IOC_RESVSP		_IOW('X', 40, struct space_resv)
+#define F_IOC_RESVSP64		_IOW('X', 42, struct space_resv)
+
+#if defined(CONFIG_IA64) || defined(CONFIG_X86_64)
+#define BROKEN_X86_ALIGNMENT
+
+#define  _NATIVE_IOC(cmd, type) \
+	  _IOC(_IOC_DIR(cmd), _IOC_TYPE(cmd), _IOC_NR(cmd), sizeof(type))
+
+/* on ia32 l_start is on a 32-bit boundary */
+struct space_resv_32 {
+	__s16		l_type;
+	__s16		l_whence;
+	__s64		l_start	__attribute__((packed));
+			/* len == 0 means until end of file */
+	__s64		l_len __attribute__((packed));
+	__s32		l_sysid;
+	__u32		l_pid;
+	__s32		l_pad[4];	/* reserve area */
+};
+
+#define F_IOC_RESVSP_32		_IOW('X', 40, struct space_resv_32)
+#define F_IOC_RESVSP64_32	_IOW('X', 42, struct space_resv_32)
+
+#endif
+
+#endif /* __KERNEL__ */
+
 #endif /* _FALLOC_H_ */
Index: xfs/include/linux/fs.h
===================================================================
--- xfs.orig/include/linux/fs.h	2009-01-24 17:58:48.960904090 +0100
+++ xfs/include/linux/fs.h	2009-01-27 20:42:01.675299140 +0100
@@ -1694,6 +1694,8 @@ static inline int break_lease(struct ino
 
 extern int do_truncate(struct dentry *, loff_t start, unsigned int time_attrs,
 		       struct file *filp);
+extern int do_fallocate(struct file *file, int mode, loff_t offset,
+			loff_t len);
 extern long do_sys_open(int dfd, const char __user *filename, int flags,
 			int mode);
 extern struct file *filp_open(const char *, int, int);
@@ -1702,6 +1704,10 @@ extern struct file * dentry_open(struct 
 extern int filp_close(struct file *, fl_owner_t id);
 extern char * getname(const char __user *);
 
+/* fs/ioctl.c */
+
+extern int ioctl_preallocate(struct file *filp, unsigned long arg);
+
 /* fs/dcache.c */
 extern void __init vfs_caches_init_early(void);
 extern void __init vfs_caches_init(unsigned long);

Re: [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Andrew Morton]

On Thu, 29 Jan 2009 02:29:11 +0530
Ankit Jain <me@ankitjain.org> wrote:

> --- xfs.orig/include/linux/falloc.h	2009-01-21 21:03:28.076324621 +0100
> +++ xfs/include/linux/falloc.h	2009-01-27 20:36:59.190423995 +0100
> @@ -3,4 +3,48 @@
>  
>  #define FALLOC_FL_KEEP_SIZE	0x01 /* default is extend size */
>  
> +#ifdef __KERNEL__
> +
> +/*
> + * Space reservation ioctls and argument structure
> + * are designed to be compatible with the legacy XFS ioctls.
> + */
> +struct space_resv {
> +	__s16		l_type;
> +	__s16		l_whence;
> +	__s64		l_start;
> +	__s64		l_len;		/* len == 0 means until end of file */
> +	__s32		l_sysid;
> +	__u32		l_pid;
> +	__s32		l_pad[4];	/* reserve area			    */
> +};
> +
> +#define F_IOC_RESVSP		_IOW('X', 40, struct space_resv)
> +#define F_IOC_RESVSP64		_IOW('X', 42, struct space_resv)

Should this stuff be inside #ifdef __KERNEL__?  It is shared with userspace.

Are we sure that the aligment of l_start will be reliably the same
across all compilers and versions thereof for all time?

Re: [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Andrew Morton]

On Thu, 29 Jan 2009 02:29:11 +0530
Ankit Jain <me@ankitjain.org> wrote:

> --- xfs.orig/include/linux/falloc.h	2009-01-21 21:03:28.076324621 +0100
> +++ xfs/include/linux/falloc.h	2009-01-27 20:36:59.190423995 +0100
> @@ -3,4 +3,48 @@
>  
>  #define FALLOC_FL_KEEP_SIZE	0x01 /* default is extend size */
>  
> +#ifdef __KERNEL__
> +
> +/*
> + * Space reservation ioctls and argument structure
> + * are designed to be compatible with the legacy XFS ioctls.
> + */
> +struct space_resv {
> +	__s16		l_type;
> +	__s16		l_whence;
> +	__s64		l_start;
> +	__s64		l_len;		/* len == 0 means until end of file */
> +	__s32		l_sysid;
> +	__u32		l_pid;
> +	__s32		l_pad[4];	/* reserve area			    */
> +};
> +
> +#define F_IOC_RESVSP		_IOW('X', 40, struct space_resv)
> +#define F_IOC_RESVSP64		_IOW('X', 42, struct space_resv)

Should this stuff be inside #ifdef __KERNEL__?  It is shared with userspace.

Are we sure that the aligment of l_start will be reliably the same
across all compilers and versions thereof for all time?

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

[Ocfs2-devel] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Andrew Morton]

On Thu, 29 Jan 2009 02:29:11 +0530
Ankit Jain <me@ankitjain.org> wrote:

> --- xfs.orig/include/linux/falloc.h	2009-01-21 21:03:28.076324621 +0100
> +++ xfs/include/linux/falloc.h	2009-01-27 20:36:59.190423995 +0100
> @@ -3,4 +3,48 @@
>  
>  #define FALLOC_FL_KEEP_SIZE	0x01 /* default is extend size */
>  
> +#ifdef __KERNEL__
> +
> +/*
> + * Space reservation ioctls and argument structure
> + * are designed to be compatible with the legacy XFS ioctls.
> + */
> +struct space_resv {
> +	__s16		l_type;
> +	__s16		l_whence;
> +	__s64		l_start;
> +	__s64		l_len;		/* len == 0 means until end of file */
> +	__s32		l_sysid;
> +	__u32		l_pid;
> +	__s32		l_pad[4];	/* reserve area			    */
> +};
> +
> +#define F_IOC_RESVSP		_IOW('X', 40, struct space_resv)
> +#define F_IOC_RESVSP64		_IOW('X', 42, struct space_resv)

Should this stuff be inside #ifdef __KERNEL__?  It is shared with userspace.

Are we sure that the aligment of l_start will be reliably the same
across all compilers and versions thereof for all time?

Re: [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Arnd Bergmann]

On Saturday 31 January 2009, Andrew Morton wrote:
> On Thu, 29 Jan 2009 02:29:11 +0530 Ankit Jain <me@ankitjain.org> wrote:
> > +struct space_resv {
> > +	__s16		l_type;
> > +	__s16		l_whence;
> > +	__s64		l_start;
> > +	__s64		l_len;		/* len == 0 means until end of file */
> > +	__s32		l_sysid;
> > +	__u32		l_pid;
> > +	__s32		l_pad[4];	/* reserve area			    */
> > +};
> > +
> > +#define F_IOC_RESVSP		_IOW('X', 40, struct space_resv)
> > +#define F_IOC_RESVSP64		_IOW('X', 42, struct space_resv)
> 
> Are we sure that the aligment of l_start will be reliably the same
> across all compilers and versions thereof for all time?

On x86, the alignment differs between 32 and 64 bit, otherwise it's ok.
XFS handles the conversion for compat_ioctl in
fs/xfs/linux-2.6/xfs_ioctl32.c. If this becomes a generic file ioctl,
the conversion code should be moved to fs/compat_ioctl.c.

	Arnd <><

Re: [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Arnd Bergmann]

On Saturday 31 January 2009, Andrew Morton wrote:
> On Thu, 29 Jan 2009 02:29:11 +0530 Ankit Jain <me@ankitjain.org> wrote:
> > +struct space_resv {
> > +	__s16		l_type;
> > +	__s16		l_whence;
> > +	__s64		l_start;
> > +	__s64		l_len;		/* len == 0 means until end of file */
> > +	__s32		l_sysid;
> > +	__u32		l_pid;
> > +	__s32		l_pad[4];	/* reserve area			    */
> > +};
> > +
> > +#define F_IOC_RESVSP		_IOW('X', 40, struct space_resv)
> > +#define F_IOC_RESVSP64		_IOW('X', 42, struct space_resv)
> 
> Are we sure that the aligment of l_start will be reliably the same
> across all compilers and versions thereof for all time?

On x86, the alignment differs between 32 and 64 bit, otherwise it's ok.
XFS handles the conversion for compat_ioctl in
fs/xfs/linux-2.6/xfs_ioctl32.c. If this becomes a generic file ioctl,
the conversion code should be moved to fs/compat_ioctl.c.

	Arnd <><

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

[Ocfs2-devel] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Arnd Bergmann]

On Saturday 31 January 2009, Andrew Morton wrote:
> On Thu, 29 Jan 2009 02:29:11 +0530 Ankit Jain <me@ankitjain.org> wrote:
> > +struct space_resv {
> > +	__s16		l_type;
> > +	__s16		l_whence;
> > +	__s64		l_start;
> > +	__s64		l_len;		/* len == 0 means until end of file */
> > +	__s32		l_sysid;
> > +	__u32		l_pid;
> > +	__s32		l_pad[4];	/* reserve area			    */
> > +};
> > +
> > +#define F_IOC_RESVSP		_IOW('X', 40, struct space_resv)
> > +#define F_IOC_RESVSP64		_IOW('X', 42, struct space_resv)
> 
> Are we sure that the aligment of l_start will be reliably the same
> across all compilers and versions thereof for all time?

On x86, the alignment differs between 32 and 64 bit, otherwise it's ok.
XFS handles the conversion for compat_ioctl in
fs/xfs/linux-2.6/xfs_ioctl32.c. If this becomes a generic file ioctl,
the conversion code should be moved to fs/compat_ioctl.c.

	Arnd <><

Re: [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Andrew Morton]

On Sat, 31 Jan 2009 01:38:32 +0100 Arnd Bergmann <arnd@arndb.de> wrote:

> On Saturday 31 January 2009, Andrew Morton wrote:
> > On Thu, 29 Jan 2009 02:29:11 +0530 Ankit Jain <me@ankitjain.org> wrote:
> > > +struct space_resv {
> > > +	__s16		l_type;
> > > +	__s16		l_whence;
> > > +	__s64		l_start;
> > > +	__s64		l_len;		/* len == 0 means until end of file */
> > > +	__s32		l_sysid;
> > > +	__u32		l_pid;
> > > +	__s32		l_pad[4];	/* reserve area			    */
> > > +};
> > > +
> > > +#define F_IOC_RESVSP		_IOW('X', 40, struct space_resv)
> > > +#define F_IOC_RESVSP64		_IOW('X', 42, struct space_resv)
> > 
> > Are we sure that the aligment of l_start will be reliably the same
> > across all compilers and versions thereof for all time?
> 
> On x86, the alignment differs between 32 and 64 bit, otherwise it's ok.

Is this written in a standard somewhere?  Is it guaranteed?

If some (perhaps non-gcc) compiler were to lay this out differently
(perhaps with suitable command-line options) then that's liveable
with - as long as the kernel never changes the layout.  Of course
it would be better to avoid this if poss.

The other potential issue with a structure like this is that there's a
risk that it will lead us to copy four bytes of uninitialised kernel
memory out to userspace.

IOW, it seems a generally bad idea to rely upon compiler-added padding
for this sort of thing.

> XFS handles the conversion for compat_ioctl in
> fs/xfs/linux-2.6/xfs_ioctl32.c. If this becomes a generic file ioctl,
> the conversion code should be moved to fs/compat_ioctl.c.

Re: [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Andrew Morton]

On Sat, 31 Jan 2009 01:38:32 +0100 Arnd Bergmann <arnd@arndb.de> wrote:

> On Saturday 31 January 2009, Andrew Morton wrote:
> > On Thu, 29 Jan 2009 02:29:11 +0530 Ankit Jain <me@ankitjain.org> wrote:
> > > +struct space_resv {
> > > +	__s16		l_type;
> > > +	__s16		l_whence;
> > > +	__s64		l_start;
> > > +	__s64		l_len;		/* len == 0 means until end of file */
> > > +	__s32		l_sysid;
> > > +	__u32		l_pid;
> > > +	__s32		l_pad[4];	/* reserve area			    */
> > > +};
> > > +
> > > +#define F_IOC_RESVSP		_IOW('X', 40, struct space_resv)
> > > +#define F_IOC_RESVSP64		_IOW('X', 42, struct space_resv)
> > 
> > Are we sure that the aligment of l_start will be reliably the same
> > across all compilers and versions thereof for all time?
> 
> On x86, the alignment differs between 32 and 64 bit, otherwise it's ok.

Is this written in a standard somewhere?  Is it guaranteed?

If some (perhaps non-gcc) compiler were to lay this out differently
(perhaps with suitable command-line options) then that's liveable
with - as long as the kernel never changes the layout.  Of course
it would be better to avoid this if poss.

The other potential issue with a structure like this is that there's a
risk that it will lead us to copy four bytes of uninitialised kernel
memory out to userspace.

IOW, it seems a generally bad idea to rely upon compiler-added padding
for this sort of thing.

> XFS handles the conversion for compat_ioctl in
> fs/xfs/linux-2.6/xfs_ioctl32.c. If this becomes a generic file ioctl,
> the conversion code should be moved to fs/compat_ioctl.c.

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

[Ocfs2-devel] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Andrew Morton]

On Sat, 31 Jan 2009 01:38:32 +0100 Arnd Bergmann <arnd@arndb.de> wrote:

> On Saturday 31 January 2009, Andrew Morton wrote:
> > On Thu, 29 Jan 2009 02:29:11 +0530 Ankit Jain <me@ankitjain.org> wrote:
> > > +struct space_resv {
> > > +	__s16		l_type;
> > > +	__s16		l_whence;
> > > +	__s64		l_start;
> > > +	__s64		l_len;		/* len == 0 means until end of file */
> > > +	__s32		l_sysid;
> > > +	__u32		l_pid;
> > > +	__s32		l_pad[4];	/* reserve area			    */
> > > +};
> > > +
> > > +#define F_IOC_RESVSP		_IOW('X', 40, struct space_resv)
> > > +#define F_IOC_RESVSP64		_IOW('X', 42, struct space_resv)
> > 
> > Are we sure that the aligment of l_start will be reliably the same
> > across all compilers and versions thereof for all time?
> 
> On x86, the alignment differs between 32 and 64 bit, otherwise it's ok.

Is this written in a standard somewhere?  Is it guaranteed?

If some (perhaps non-gcc) compiler were to lay this out differently
(perhaps with suitable command-line options) then that's liveable
with - as long as the kernel never changes the layout.  Of course
it would be better to avoid this if poss.

The other potential issue with a structure like this is that there's a
risk that it will lead us to copy four bytes of uninitialised kernel
memory out to userspace.

IOW, it seems a generally bad idea to rely upon compiler-added padding
for this sort of thing.

> XFS handles the conversion for compat_ioctl in
> fs/xfs/linux-2.6/xfs_ioctl32.c. If this becomes a generic file ioctl,
> the conversion code should be moved to fs/compat_ioctl.c.

Re: [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Arnd Bergmann]

On Saturday 31 January 2009, Andrew Morton wrote:
> Is this written in a standard somewhere?  Is it guaranteed?

Alignment is defined in the architecture psABI documents. 
Unfortunately, many of them were written before the 'long long'
type became part of the C standard, so it's not strictly guaranteed.
AFAICT, the alignment of __u64 on x86 is the same as the alignment
of 'double' by convention.

However, the problem is well-understood: x86 is the only one
that has a problem in 32/64 bit compat mode. m68k has similar
issues with 16/32 bit integers, but those don't apply here.

> If some (perhaps non-gcc) compiler were to lay this out differently
> (perhaps with suitable command-line options) then that's liveable
> with - as long as the kernel never changes the layout.  Of course
> it would be better to avoid this if poss.

If a compiler was using irregular structure alignment, all sorts of
library interfaces would break. The kernel ABI is only a small part
of the problem then.

> The other potential issue with a structure like this is that there's a
> risk that it will lead us to copy four bytes of uninitialised kernel
> memory out to userspace.
> 
> IOW, it seems a generally bad idea to rely upon compiler-added padding
> for this sort of thing.

Agreed in general, but the whole point of this particular patch was to
provide compatibility with an interface that has been part of XFS for
many years.
Linux already has a better interface for new users (sys_fallocate), so
changing the patch would not be helpful and not provide any advantage.

There is also no leak of uninitialized data here, because this structure
is only read, never written.

	Arnd <><

Re: [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Arnd Bergmann]

On Saturday 31 January 2009, Andrew Morton wrote:
> Is this written in a standard somewhere?  Is it guaranteed?

Alignment is defined in the architecture psABI documents. 
Unfortunately, many of them were written before the 'long long'
type became part of the C standard, so it's not strictly guaranteed.
AFAICT, the alignment of __u64 on x86 is the same as the alignment
of 'double' by convention.

However, the problem is well-understood: x86 is the only one
that has a problem in 32/64 bit compat mode. m68k has similar
issues with 16/32 bit integers, but those don't apply here.

> If some (perhaps non-gcc) compiler were to lay this out differently
> (perhaps with suitable command-line options) then that's liveable
> with - as long as the kernel never changes the layout.  Of course
> it would be better to avoid this if poss.

If a compiler was using irregular structure alignment, all sorts of
library interfaces would break. The kernel ABI is only a small part
of the problem then.

> The other potential issue with a structure like this is that there's a
> risk that it will lead us to copy four bytes of uninitialised kernel
> memory out to userspace.
> 
> IOW, it seems a generally bad idea to rely upon compiler-added padding
> for this sort of thing.

Agreed in general, but the whole point of this particular patch was to
provide compatibility with an interface that has been part of XFS for
many years.
Linux already has a better interface for new users (sys_fallocate), so
changing the patch would not be helpful and not provide any advantage.

There is also no leak of uninitialized data here, because this structure
is only read, never written.

	Arnd <><
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Arnd Bergmann]

On Saturday 31 January 2009, Andrew Morton wrote:
> Is this written in a standard somewhere?  Is it guaranteed?

Alignment is defined in the architecture psABI documents. 
Unfortunately, many of them were written before the 'long long'
type became part of the C standard, so it's not strictly guaranteed.
AFAICT, the alignment of __u64 on x86 is the same as the alignment
of 'double' by convention.

However, the problem is well-understood: x86 is the only one
that has a problem in 32/64 bit compat mode. m68k has similar
issues with 16/32 bit integers, but those don't apply here.

> If some (perhaps non-gcc) compiler were to lay this out differently
> (perhaps with suitable command-line options) then that's liveable
> with - as long as the kernel never changes the layout.  Of course
> it would be better to avoid this if poss.

If a compiler was using irregular structure alignment, all sorts of
library interfaces would break. The kernel ABI is only a small part
of the problem then.

> The other potential issue with a structure like this is that there's a
> risk that it will lead us to copy four bytes of uninitialised kernel
> memory out to userspace.
> 
> IOW, it seems a generally bad idea to rely upon compiler-added padding
> for this sort of thing.

Agreed in general, but the whole point of this particular patch was to
provide compatibility with an interface that has been part of XFS for
many years.
Linux already has a better interface for new users (sys_fallocate), so
changing the patch would not be helpful and not provide any advantage.

There is also no leak of uninitialized data here, because this structure
is only read, never written.

	Arnd <><

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

[Ocfs2-devel] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Arnd Bergmann]

On Saturday 31 January 2009, Andrew Morton wrote:
> Is this written in a standard somewhere? ?Is it guaranteed?

Alignment is defined in the architecture psABI documents. 
Unfortunately, many of them were written before the 'long long'
type became part of the C standard, so it's not strictly guaranteed.
AFAICT, the alignment of __u64 on x86 is the same as the alignment
of 'double' by convention.

However, the problem is well-understood: x86 is the only one
that has a problem in 32/64 bit compat mode. m68k has similar
issues with 16/32 bit integers, but those don't apply here.

> If some (perhaps non-gcc) compiler were to lay this out differently
> (perhaps with suitable command-line options) then that's liveable
> with - as long as the kernel never changes the layout. ?Of course
> it would be better to avoid this if poss.

If a compiler was using irregular structure alignment, all sorts of
library interfaces would break. The kernel ABI is only a small part
of the problem then.

> The other potential issue with a structure like this is that there's a
> risk that it will lead us to copy four bytes of uninitialised kernel
> memory out to userspace.
> 
> IOW, it seems a generally bad idea to rely upon compiler-added padding
> for this sort of thing.

Agreed in general, but the whole point of this particular patch was to
provide compatibility with an interface that has been part of XFS for
many years.
Linux already has a better interface for new users (sys_fallocate), so
changing the patch would not be helpful and not provide any advantage.

There is also no leak of uninitialized data here, because this structure
is only read, never written.

	Arnd <><

Re: [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Boaz Harrosh]

Arnd Bergmann wrote:
> On Saturday 31 January 2009, Andrew Morton wrote:
>> Is this written in a standard somewhere?  Is it guaranteed?
> 
> Alignment is defined in the architecture psABI documents. 
> Unfortunately, many of them were written before the 'long long'
> type became part of the C standard, so it's not strictly guaranteed.
> AFAICT, the alignment of __u64 on x86 is the same as the alignment
> of 'double' by convention.
> 
> However, the problem is well-understood: x86 is the only one
> that has a problem in 32/64 bit compat mode. m68k has similar
> issues with 16/32 bit integers, but those don't apply here.
> 
>> If some (perhaps non-gcc) compiler were to lay this out differently
>> (perhaps with suitable command-line options) then that's liveable
>> with - as long as the kernel never changes the layout.  Of course
>> it would be better to avoid this if poss.
> 
> If a compiler was using irregular structure alignment, all sorts of
> library interfaces would break. The kernel ABI is only a small part
> of the problem then.
> 
>> The other potential issue with a structure like this is that there's a
>> risk that it will lead us to copy four bytes of uninitialised kernel
>> memory out to userspace.
>>
>> IOW, it seems a generally bad idea to rely upon compiler-added padding
>> for this sort of thing.
> 
> Agreed in general, but the whole point of this particular patch was to
> provide compatibility with an interface that has been part of XFS for
> many years.
> Linux already has a better interface for new users (sys_fallocate), so
> changing the patch would not be helpful and not provide any advantage.
> 
> There is also no leak of uninitialized data here, because this structure
> is only read, never written.
> 
> 	Arnd <><
Arnd Bergmann wrote:
> +struct space_resv {
> +	__s16		l_type;
> +	__s16		l_whence;
> +	__s64		l_start;
> +	__s64		l_len;		/* len == 0 means until end of file */
> +	__s32		l_sysid;
> +	__u32		l_pid;
> +	__s32		l_pad[4];	/* reserve area			    */
> +};

What about telling the compiler exactly what you said above, just
to be sure we all mean the same thing. (And as documentation for new
comers):

+struct space_resv_64 {
+	__s16		l_type;
+	__s16		l_whence;
+	__u32		reserved;
+	__s64		l_start;
+	__s64		l_len;		/* len == 0 means until end of file */
+	__s32		l_sysid;
+	__u32		l_pid;
+	__s32		l_pad[4];	/* reserve area			    */
+} __packed;

And define another one for x86_32

Boaz

Re: [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Boaz Harrosh]

Arnd Bergmann wrote:
> On Saturday 31 January 2009, Andrew Morton wrote:
>> Is this written in a standard somewhere?  Is it guaranteed?
> 
> Alignment is defined in the architecture psABI documents. 
> Unfortunately, many of them were written before the 'long long'
> type became part of the C standard, so it's not strictly guaranteed.
> AFAICT, the alignment of __u64 on x86 is the same as the alignment
> of 'double' by convention.
> 
> However, the problem is well-understood: x86 is the only one
> that has a problem in 32/64 bit compat mode. m68k has similar
> issues with 16/32 bit integers, but those don't apply here.
> 
>> If some (perhaps non-gcc) compiler were to lay this out differently
>> (perhaps with suitable command-line options) then that's liveable
>> with - as long as the kernel never changes the layout.  Of course
>> it would be better to avoid this if poss.
> 
> If a compiler was using irregular structure alignment, all sorts of
> library interfaces would break. The kernel ABI is only a small part
> of the problem then.
> 
>> The other potential issue with a structure like this is that there's a
>> risk that it will lead us to copy four bytes of uninitialised kernel
>> memory out to userspace.
>>
>> IOW, it seems a generally bad idea to rely upon compiler-added padding
>> for this sort of thing.
> 
> Agreed in general, but the whole point of this particular patch was to
> provide compatibility with an interface that has been part of XFS for
> many years.
> Linux already has a better interface for new users (sys_fallocate), so
> changing the patch would not be helpful and not provide any advantage.
> 
> There is also no leak of uninitialized data here, because this structure
> is only read, never written.
> 
> 	Arnd <><
Arnd Bergmann wrote:
> +struct space_resv {
> +	__s16		l_type;
> +	__s16		l_whence;
> +	__s64		l_start;
> +	__s64		l_len;		/* len == 0 means until end of file */
> +	__s32		l_sysid;
> +	__u32		l_pid;
> +	__s32		l_pad[4];	/* reserve area			    */
> +};

What about telling the compiler exactly what you said above, just
to be sure we all mean the same thing. (And as documentation for new
comers):

+struct space_resv_64 {
+	__s16		l_type;
+	__s16		l_whence;
+	__u32		reserved;
+	__s64		l_start;
+	__s64		l_len;		/* len == 0 means until end of file */
+	__s32		l_sysid;
+	__u32		l_pid;
+	__s32		l_pad[4];	/* reserve area			    */
+} __packed;

And define another one for x86_32

Boaz

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

[Ocfs2-devel] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Boaz Harrosh]

Arnd Bergmann wrote:
> On Saturday 31 January 2009, Andrew Morton wrote:
>> Is this written in a standard somewhere?  Is it guaranteed?
> 
> Alignment is defined in the architecture psABI documents. 
> Unfortunately, many of them were written before the 'long long'
> type became part of the C standard, so it's not strictly guaranteed.
> AFAICT, the alignment of __u64 on x86 is the same as the alignment
> of 'double' by convention.
> 
> However, the problem is well-understood: x86 is the only one
> that has a problem in 32/64 bit compat mode. m68k has similar
> issues with 16/32 bit integers, but those don't apply here.
> 
>> If some (perhaps non-gcc) compiler were to lay this out differently
>> (perhaps with suitable command-line options) then that's liveable
>> with - as long as the kernel never changes the layout.  Of course
>> it would be better to avoid this if poss.
> 
> If a compiler was using irregular structure alignment, all sorts of
> library interfaces would break. The kernel ABI is only a small part
> of the problem then.
> 
>> The other potential issue with a structure like this is that there's a
>> risk that it will lead us to copy four bytes of uninitialised kernel
>> memory out to userspace.
>>
>> IOW, it seems a generally bad idea to rely upon compiler-added padding
>> for this sort of thing.
> 
> Agreed in general, but the whole point of this particular patch was to
> provide compatibility with an interface that has been part of XFS for
> many years.
> Linux already has a better interface for new users (sys_fallocate), so
> changing the patch would not be helpful and not provide any advantage.
> 
> There is also no leak of uninitialized data here, because this structure
> is only read, never written.
> 
> 	Arnd <><
Arnd Bergmann wrote:
> +struct space_resv {
> +	__s16		l_type;
> +	__s16		l_whence;
> +	__s64		l_start;
> +	__s64		l_len;		/* len == 0 means until end of file */
> +	__s32		l_sysid;
> +	__u32		l_pid;
> +	__s32		l_pad[4];	/* reserve area			    */
> +};

What about telling the compiler exactly what you said above, just
to be sure we all mean the same thing. (And as documentation for new
comers):

+struct space_resv_64 {
+	__s16		l_type;
+	__s16		l_whence;
+	__u32		reserved;
+	__s64		l_start;
+	__s64		l_len;		/* len == 0 means until end of file */
+	__s32		l_sysid;
+	__u32		l_pid;
+	__s32		l_pad[4];	/* reserve area			    */
+} __packed;

And define another one for x86_32

Boaz

Re: [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Geert Uytterhoeven]

On Sun, 1 Feb 2009, Boaz Harrosh wrote:
> Arnd Bergmann wrote:
> > On Saturday 31 January 2009, Andrew Morton wrote:
> >> Is this written in a standard somewhere?  Is it guaranteed?
> > 
> > Alignment is defined in the architecture psABI documents. 
> > Unfortunately, many of them were written before the 'long long'
> > type became part of the C standard, so it's not strictly guaranteed.
> > AFAICT, the alignment of __u64 on x86 is the same as the alignment
> > of 'double' by convention.
> > 
> > However, the problem is well-understood: x86 is the only one
> > that has a problem in 32/64 bit compat mode. m68k has similar
> > issues with 16/32 bit integers, but those don't apply here.
> > 
> >> If some (perhaps non-gcc) compiler were to lay this out differently
> >> (perhaps with suitable command-line options) then that's liveable
> >> with - as long as the kernel never changes the layout.  Of course
> >> it would be better to avoid this if poss.
> > 
> > If a compiler was using irregular structure alignment, all sorts of
> > library interfaces would break. The kernel ABI is only a small part
> > of the problem then.
> > 
> >> The other potential issue with a structure like this is that there's a
> >> risk that it will lead us to copy four bytes of uninitialised kernel
> >> memory out to userspace.
> >>
> >> IOW, it seems a generally bad idea to rely upon compiler-added padding
> >> for this sort of thing.
> > 
> > Agreed in general, but the whole point of this particular patch was to
> > provide compatibility with an interface that has been part of XFS for
> > many years.
> > Linux already has a better interface for new users (sys_fallocate), so
> > changing the patch would not be helpful and not provide any advantage.
> > 
> > There is also no leak of uninitialized data here, because this structure
> > is only read, never written.
> > 
> > 	Arnd <><
> Arnd Bergmann wrote:
> > +struct space_resv {
> > +	__s16		l_type;
> > +	__s16		l_whence;
> > +	__s64		l_start;
> > +	__s64		l_len;		/* len == 0 means until end of file */
> > +	__s32		l_sysid;
> > +	__u32		l_pid;
> > +	__s32		l_pad[4];	/* reserve area			    */
> > +};
> 
> What about telling the compiler exactly what you said above, just
> to be sure we all mean the same thing. (And as documentation for new
> comers):
> 
> +struct space_resv_64 {
> +	__s16		l_type;
> +	__s16		l_whence;
> +	__u32		reserved;
> +	__s64		l_start;
> +	__s64		l_len;		/* len == 0 means until end of file */
> +	__s32		l_sysid;
> +	__u32		l_pid;
> +	__s32		l_pad[4];	/* reserve area			    */
> +} __packed;

Because the compiler will assume all fields are always unaligned and will use very
suboptimal code to access them?

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Geert Uytterhoeven]

On Sun, 1 Feb 2009, Boaz Harrosh wrote:
> Arnd Bergmann wrote:
> > On Saturday 31 January 2009, Andrew Morton wrote:
> >> Is this written in a standard somewhere?  Is it guaranteed?
> > 
> > Alignment is defined in the architecture psABI documents. 
> > Unfortunately, many of them were written before the 'long long'
> > type became part of the C standard, so it's not strictly guaranteed.
> > AFAICT, the alignment of __u64 on x86 is the same as the alignment
> > of 'double' by convention.
> > 
> > However, the problem is well-understood: x86 is the only one
> > that has a problem in 32/64 bit compat mode. m68k has similar
> > issues with 16/32 bit integers, but those don't apply here.
> > 
> >> If some (perhaps non-gcc) compiler were to lay this out differently
> >> (perhaps with suitable command-line options) then that's liveable
> >> with - as long as the kernel never changes the layout.  Of course
> >> it would be better to avoid this if poss.
> > 
> > If a compiler was using irregular structure alignment, all sorts of
> > library interfaces would break. The kernel ABI is only a small part
> > of the problem then.
> > 
> >> The other potential issue with a structure like this is that there's a
> >> risk that it will lead us to copy four bytes of uninitialised kernel
> >> memory out to userspace.
> >>
> >> IOW, it seems a generally bad idea to rely upon compiler-added padding
> >> for this sort of thing.
> > 
> > Agreed in general, but the whole point of this particular patch was to
> > provide compatibility with an interface that has been part of XFS for
> > many years.
> > Linux already has a better interface for new users (sys_fallocate), so
> > changing the patch would not be helpful and not provide any advantage.
> > 
> > There is also no leak of uninitialized data here, because this structure
> > is only read, never written.
> > 
> > 	Arnd <><
> Arnd Bergmann wrote:
> > +struct space_resv {
> > +	__s16		l_type;
> > +	__s16		l_whence;
> > +	__s64		l_start;
> > +	__s64		l_len;		/* len == 0 means until end of file */
> > +	__s32		l_sysid;
> > +	__u32		l_pid;
> > +	__s32		l_pad[4];	/* reserve area			    */
> > +};
> 
> What about telling the compiler exactly what you said above, just
> to be sure we all mean the same thing. (And as documentation for new
> comers):
> 
> +struct space_resv_64 {
> +	__s16		l_type;
> +	__s16		l_whence;
> +	__u32		reserved;
> +	__s64		l_start;
> +	__s64		l_len;		/* len == 0 means until end of file */
> +	__s32		l_sysid;
> +	__u32		l_pid;
> +	__s32		l_pad[4];	/* reserve area			    */
> +} __packed;

Because the compiler will assume all fields are always unaligned and will use very
suboptimal code to access them?

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

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

[Ocfs2-devel] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Geert Uytterhoeven]

On Sun, 1 Feb 2009, Boaz Harrosh wrote:
> Arnd Bergmann wrote:
> > On Saturday 31 January 2009, Andrew Morton wrote:
> >> Is this written in a standard somewhere?  Is it guaranteed?
> > 
> > Alignment is defined in the architecture psABI documents. 
> > Unfortunately, many of them were written before the 'long long'
> > type became part of the C standard, so it's not strictly guaranteed.
> > AFAICT, the alignment of __u64 on x86 is the same as the alignment
> > of 'double' by convention.
> > 
> > However, the problem is well-understood: x86 is the only one
> > that has a problem in 32/64 bit compat mode. m68k has similar
> > issues with 16/32 bit integers, but those don't apply here.
> > 
> >> If some (perhaps non-gcc) compiler were to lay this out differently
> >> (perhaps with suitable command-line options) then that's liveable
> >> with - as long as the kernel never changes the layout.  Of course
> >> it would be better to avoid this if poss.
> > 
> > If a compiler was using irregular structure alignment, all sorts of
> > library interfaces would break. The kernel ABI is only a small part
> > of the problem then.
> > 
> >> The other potential issue with a structure like this is that there's a
> >> risk that it will lead us to copy four bytes of uninitialised kernel
> >> memory out to userspace.
> >>
> >> IOW, it seems a generally bad idea to rely upon compiler-added padding
> >> for this sort of thing.
> > 
> > Agreed in general, but the whole point of this particular patch was to
> > provide compatibility with an interface that has been part of XFS for
> > many years.
> > Linux already has a better interface for new users (sys_fallocate), so
> > changing the patch would not be helpful and not provide any advantage.
> > 
> > There is also no leak of uninitialized data here, because this structure
> > is only read, never written.
> > 
> > 	Arnd <><
> Arnd Bergmann wrote:
> > +struct space_resv {
> > +	__s16		l_type;
> > +	__s16		l_whence;
> > +	__s64		l_start;
> > +	__s64		l_len;		/* len == 0 means until end of file */
> > +	__s32		l_sysid;
> > +	__u32		l_pid;
> > +	__s32		l_pad[4];	/* reserve area			    */
> > +};
> 
> What about telling the compiler exactly what you said above, just
> to be sure we all mean the same thing. (And as documentation for new
> comers):
> 
> +struct space_resv_64 {
> +	__s16		l_type;
> +	__s16		l_whence;
> +	__u32		reserved;
> +	__s64		l_start;
> +	__s64		l_len;		/* len == 0 means until end of file */
> +	__s32		l_sysid;
> +	__u32		l_pid;
> +	__s32		l_pad[4];	/* reserve area			    */
> +} __packed;

Because the compiler will assume all fields are always unaligned and will use very
suboptimal code to access them?

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Boaz Harrosh]

Geert Uytterhoeven wrote:
> On Sun, 1 Feb 2009, Boaz Harrosh wrote:
>> Arnd Bergmann wrote:
>>> On Saturday 31 January 2009, Andrew Morton wrote:
>>>> Is this written in a standard somewhere?  Is it guaranteed?
>>> Alignment is defined in the architecture psABI documents. 
>>> Unfortunately, many of them were written before the 'long long'
>>> type became part of the C standard, so it's not strictly guaranteed.
>>> AFAICT, the alignment of __u64 on x86 is the same as the alignment
>>> of 'double' by convention.
>>>
>>> However, the problem is well-understood: x86 is the only one
>>> that has a problem in 32/64 bit compat mode. m68k has similar
>>> issues with 16/32 bit integers, but those don't apply here.
>>>
>>>> If some (perhaps non-gcc) compiler were to lay this out differently
>>>> (perhaps with suitable command-line options) then that's liveable
>>>> with - as long as the kernel never changes the layout.  Of course
>>>> it would be better to avoid this if poss.
>>> If a compiler was using irregular structure alignment, all sorts of
>>> library interfaces would break. The kernel ABI is only a small part
>>> of the problem then.
>>>
>>>> The other potential issue with a structure like this is that there's a
>>>> risk that it will lead us to copy four bytes of uninitialised kernel
>>>> memory out to userspace.
>>>>
>>>> IOW, it seems a generally bad idea to rely upon compiler-added padding
>>>> for this sort of thing.
>>> Agreed in general, but the whole point of this particular patch was to
>>> provide compatibility with an interface that has been part of XFS for
>>> many years.
>>> Linux already has a better interface for new users (sys_fallocate), so
>>> changing the patch would not be helpful and not provide any advantage.
>>>
>>> There is also no leak of uninitialized data here, because this structure
>>> is only read, never written.
>>>
>>> 	Arnd <><
>> Arnd Bergmann wrote:
>>> +struct space_resv {
>>> +	__s16		l_type;
>>> +	__s16		l_whence;
>>> +	__s64		l_start;
>>> +	__s64		l_len;		/* len == 0 means until end of file */
>>> +	__s32		l_sysid;
>>> +	__u32		l_pid;
>>> +	__s32		l_pad[4];	/* reserve area			    */
>>> +};
>> What about telling the compiler exactly what you said above, just
>> to be sure we all mean the same thing. (And as documentation for new
>> comers):
>>
>> +struct space_resv_64 {
>> +	__s16		l_type;
>> +	__s16		l_whence;
>> +	__u32		reserved;
>> +	__s64		l_start;
>> +	__s64		l_len;		/* len == 0 means until end of file */
>> +	__s32		l_sysid;
>> +	__u32		l_pid;
>> +	__s32		l_pad[4];	/* reserve area			    */
>> +} __packed;
> 
> Because the compiler will assume all fields are always unaligned and will use very
> suboptimal code to access them?
> 
> Gr{oetje,eeting}s,
> 
> 						Geert
> 

This discussion comes up every once in a while. I'm using an old FC7 compiler
(gcc (GCC) 4.1.2 20070925 (Red Hat 4.1.2-27)) And tests show that when the layout
of a structure is exactly the same the "__packed" on structure declarations does
nothing. It only starts to affect when there are real differences in alignment.
Also tests with gcc 3.4.x showed the same effect.

On previous discussions no one could come forward and say what compiler version
breaks when __packed is applied on structure definition. I'm afraid your statement
above is a myth.

Boaz

Re: [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Boaz Harrosh]

Geert Uytterhoeven wrote:
> On Sun, 1 Feb 2009, Boaz Harrosh wrote:
>> Arnd Bergmann wrote:
>>> On Saturday 31 January 2009, Andrew Morton wrote:
>>>> Is this written in a standard somewhere?  Is it guaranteed?
>>> Alignment is defined in the architecture psABI documents. 
>>> Unfortunately, many of them were written before the 'long long'
>>> type became part of the C standard, so it's not strictly guaranteed.
>>> AFAICT, the alignment of __u64 on x86 is the same as the alignment
>>> of 'double' by convention.
>>>
>>> However, the problem is well-understood: x86 is the only one
>>> that has a problem in 32/64 bit compat mode. m68k has similar
>>> issues with 16/32 bit integers, but those don't apply here.
>>>
>>>> If some (perhaps non-gcc) compiler were to lay this out differently
>>>> (perhaps with suitable command-line options) then that's liveable
>>>> with - as long as the kernel never changes the layout.  Of course
>>>> it would be better to avoid this if poss.
>>> If a compiler was using irregular structure alignment, all sorts of
>>> library interfaces would break. The kernel ABI is only a small part
>>> of the problem then.
>>>
>>>> The other potential issue with a structure like this is that there's a
>>>> risk that it will lead us to copy four bytes of uninitialised kernel
>>>> memory out to userspace.
>>>>
>>>> IOW, it seems a generally bad idea to rely upon compiler-added padding
>>>> for this sort of thing.
>>> Agreed in general, but the whole point of this particular patch was to
>>> provide compatibility with an interface that has been part of XFS for
>>> many years.
>>> Linux already has a better interface for new users (sys_fallocate), so
>>> changing the patch would not be helpful and not provide any advantage.
>>>
>>> There is also no leak of uninitialized data here, because this structure
>>> is only read, never written.
>>>
>>> 	Arnd <><
>> Arnd Bergmann wrote:
>>> +struct space_resv {
>>> +	__s16		l_type;
>>> +	__s16		l_whence;
>>> +	__s64		l_start;
>>> +	__s64		l_len;		/* len == 0 means until end of file */
>>> +	__s32		l_sysid;
>>> +	__u32		l_pid;
>>> +	__s32		l_pad[4];	/* reserve area			    */
>>> +};
>> What about telling the compiler exactly what you said above, just
>> to be sure we all mean the same thing. (And as documentation for new
>> comers):
>>
>> +struct space_resv_64 {
>> +	__s16		l_type;
>> +	__s16		l_whence;
>> +	__u32		reserved;
>> +	__s64		l_start;
>> +	__s64		l_len;		/* len == 0 means until end of file */
>> +	__s32		l_sysid;
>> +	__u32		l_pid;
>> +	__s32		l_pad[4];	/* reserve area			    */
>> +} __packed;
> 
> Because the compiler will assume all fields are always unaligned and will use very
> suboptimal code to access them?
> 
> Gr{oetje,eeting}s,
> 
> 						Geert
> 

This discussion comes up every once in a while. I'm using an old FC7 compiler
(gcc (GCC) 4.1.2 20070925 (Red Hat 4.1.2-27)) And tests show that when the layout
of a structure is exactly the same the "__packed" on structure declarations does
nothing. It only starts to affect when there are real differences in alignment.
Also tests with gcc 3.4.x showed the same effect.

On previous discussions no one could come forward and say what compiler version
breaks when __packed is applied on structure definition. I'm afraid your statement
above is a myth.

Boaz

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

[Ocfs2-devel] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Boaz Harrosh]

Geert Uytterhoeven wrote:
> On Sun, 1 Feb 2009, Boaz Harrosh wrote:
>> Arnd Bergmann wrote:
>>> On Saturday 31 January 2009, Andrew Morton wrote:
>>>> Is this written in a standard somewhere?  Is it guaranteed?
>>> Alignment is defined in the architecture psABI documents. 
>>> Unfortunately, many of them were written before the 'long long'
>>> type became part of the C standard, so it's not strictly guaranteed.
>>> AFAICT, the alignment of __u64 on x86 is the same as the alignment
>>> of 'double' by convention.
>>>
>>> However, the problem is well-understood: x86 is the only one
>>> that has a problem in 32/64 bit compat mode. m68k has similar
>>> issues with 16/32 bit integers, but those don't apply here.
>>>
>>>> If some (perhaps non-gcc) compiler were to lay this out differently
>>>> (perhaps with suitable command-line options) then that's liveable
>>>> with - as long as the kernel never changes the layout.  Of course
>>>> it would be better to avoid this if poss.
>>> If a compiler was using irregular structure alignment, all sorts of
>>> library interfaces would break. The kernel ABI is only a small part
>>> of the problem then.
>>>
>>>> The other potential issue with a structure like this is that there's a
>>>> risk that it will lead us to copy four bytes of uninitialised kernel
>>>> memory out to userspace.
>>>>
>>>> IOW, it seems a generally bad idea to rely upon compiler-added padding
>>>> for this sort of thing.
>>> Agreed in general, but the whole point of this particular patch was to
>>> provide compatibility with an interface that has been part of XFS for
>>> many years.
>>> Linux already has a better interface for new users (sys_fallocate), so
>>> changing the patch would not be helpful and not provide any advantage.
>>>
>>> There is also no leak of uninitialized data here, because this structure
>>> is only read, never written.
>>>
>>> 	Arnd <><
>> Arnd Bergmann wrote:
>>> +struct space_resv {
>>> +	__s16		l_type;
>>> +	__s16		l_whence;
>>> +	__s64		l_start;
>>> +	__s64		l_len;		/* len == 0 means until end of file */
>>> +	__s32		l_sysid;
>>> +	__u32		l_pid;
>>> +	__s32		l_pad[4];	/* reserve area			    */
>>> +};
>> What about telling the compiler exactly what you said above, just
>> to be sure we all mean the same thing. (And as documentation for new
>> comers):
>>
>> +struct space_resv_64 {
>> +	__s16		l_type;
>> +	__s16		l_whence;
>> +	__u32		reserved;
>> +	__s64		l_start;
>> +	__s64		l_len;		/* len == 0 means until end of file */
>> +	__s32		l_sysid;
>> +	__u32		l_pid;
>> +	__s32		l_pad[4];	/* reserve area			    */
>> +} __packed;
> 
> Because the compiler will assume all fields are always unaligned and will use very
> suboptimal code to access them?
> 
> Gr{oetje,eeting}s,
> 
> 						Geert
> 

This discussion comes up every once in a while. I'm using an old FC7 compiler
(gcc (GCC) 4.1.2 20070925 (Red Hat 4.1.2-27)) And tests show that when the layout
of a structure is exactly the same the "__packed" on structure declarations does
nothing. It only starts to affect when there are real differences in alignment.
Also tests with gcc 3.4.x showed the same effect.

On previous discussions no one could come forward and say what compiler version
breaks when __packed is applied on structure definition. I'm afraid your statement
above is a myth.

Boaz

Re: [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Geert Uytterhoeven]

On Sun, 1 Feb 2009, Boaz Harrosh wrote:
> Geert Uytterhoeven wrote:
> > On Sun, 1 Feb 2009, Boaz Harrosh wrote:
> >> Arnd Bergmann wrote:
> >>> +struct space_resv {
> >>> +	__s16		l_type;
> >>> +	__s16		l_whence;
> >>> +	__s64		l_start;
> >>> +	__s64		l_len;		/* len == 0 means until end of file */
> >>> +	__s32		l_sysid;
> >>> +	__u32		l_pid;
> >>> +	__s32		l_pad[4];	/* reserve area			    */
> >>> +};
> >> What about telling the compiler exactly what you said above, just
> >> to be sure we all mean the same thing. (And as documentation for new
> >> comers):
> >>
> >> +struct space_resv_64 {
> >> +	__s16		l_type;
> >> +	__s16		l_whence;
> >> +	__u32		reserved;
> >> +	__s64		l_start;
> >> +	__s64		l_len;		/* len == 0 means until end of file */
> >> +	__s32		l_sysid;
> >> +	__u32		l_pid;
> >> +	__s32		l_pad[4];	/* reserve area			    */
> >> +} __packed;
> > 
> > Because the compiler will assume all fields are always unaligned and will use very
> > suboptimal code to access them?
> 
> This discussion comes up every once in a while. I'm using an old FC7 compiler
> (gcc (GCC) 4.1.2 20070925 (Red Hat 4.1.2-27)) And tests show that when the layout
> of a structure is exactly the same the "__packed" on structure declarations does
> nothing. It only starts to affect when there are real differences in alignment.
> Also tests with gcc 3.4.x showed the same effect.
> 
> On previous discussions no one could come forward and say what compiler version
> breaks when __packed is applied on structure definition. I'm afraid your statement
> above is a myth.

FC7, targeting ia32? Sure, ia32 has no alignment restrictions.
Try e.g. MIPS.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Geert Uytterhoeven]

On Sun, 1 Feb 2009, Boaz Harrosh wrote:
> Geert Uytterhoeven wrote:
> > On Sun, 1 Feb 2009, Boaz Harrosh wrote:
> >> Arnd Bergmann wrote:
> >>> +struct space_resv {
> >>> +	__s16		l_type;
> >>> +	__s16		l_whence;
> >>> +	__s64		l_start;
> >>> +	__s64		l_len;		/* len == 0 means until end of file */
> >>> +	__s32		l_sysid;
> >>> +	__u32		l_pid;
> >>> +	__s32		l_pad[4];	/* reserve area			    */
> >>> +};
> >> What about telling the compiler exactly what you said above, just
> >> to be sure we all mean the same thing. (And as documentation for new
> >> comers):
> >>
> >> +struct space_resv_64 {
> >> +	__s16		l_type;
> >> +	__s16		l_whence;
> >> +	__u32		reserved;
> >> +	__s64		l_start;
> >> +	__s64		l_len;		/* len == 0 means until end of file */
> >> +	__s32		l_sysid;
> >> +	__u32		l_pid;
> >> +	__s32		l_pad[4];	/* reserve area			    */
> >> +} __packed;
> > 
> > Because the compiler will assume all fields are always unaligned and will use very
> > suboptimal code to access them?
> 
> This discussion comes up every once in a while. I'm using an old FC7 compiler
> (gcc (GCC) 4.1.2 20070925 (Red Hat 4.1.2-27)) And tests show that when the layout
> of a structure is exactly the same the "__packed" on structure declarations does
> nothing. It only starts to affect when there are real differences in alignment.
> Also tests with gcc 3.4.x showed the same effect.
> 
> On previous discussions no one could come forward and say what compiler version
> breaks when __packed is applied on structure definition. I'm afraid your statement
> above is a myth.

FC7, targeting ia32? Sure, ia32 has no alignment restrictions.
Try e.g. MIPS.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

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

[Ocfs2-devel] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Geert Uytterhoeven]

On Sun, 1 Feb 2009, Boaz Harrosh wrote:
> Geert Uytterhoeven wrote:
> > On Sun, 1 Feb 2009, Boaz Harrosh wrote:
> >> Arnd Bergmann wrote:
> >>> +struct space_resv {
> >>> +	__s16		l_type;
> >>> +	__s16		l_whence;
> >>> +	__s64		l_start;
> >>> +	__s64		l_len;		/* len == 0 means until end of file */
> >>> +	__s32		l_sysid;
> >>> +	__u32		l_pid;
> >>> +	__s32		l_pad[4];	/* reserve area			    */
> >>> +};
> >> What about telling the compiler exactly what you said above, just
> >> to be sure we all mean the same thing. (And as documentation for new
> >> comers):
> >>
> >> +struct space_resv_64 {
> >> +	__s16		l_type;
> >> +	__s16		l_whence;
> >> +	__u32		reserved;
> >> +	__s64		l_start;
> >> +	__s64		l_len;		/* len == 0 means until end of file */
> >> +	__s32		l_sysid;
> >> +	__u32		l_pid;
> >> +	__s32		l_pad[4];	/* reserve area			    */
> >> +} __packed;
> > 
> > Because the compiler will assume all fields are always unaligned and will use very
> > suboptimal code to access them?
> 
> This discussion comes up every once in a while. I'm using an old FC7 compiler
> (gcc (GCC) 4.1.2 20070925 (Red Hat 4.1.2-27)) And tests show that when the layout
> of a structure is exactly the same the "__packed" on structure declarations does
> nothing. It only starts to affect when there are real differences in alignment.
> Also tests with gcc 3.4.x showed the same effect.
> 
> On previous discussions no one could come forward and say what compiler version
> breaks when __packed is applied on structure definition. I'm afraid your statement
> above is a myth.

FC7, targeting ia32? Sure, ia32 has no alignment restrictions.
Try e.g. MIPS.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Boaz Harrosh]

Geert Uytterhoeven wrote:
> On Sun, 1 Feb 2009, Boaz Harrosh wrote:
>> Geert Uytterhoeven wrote:
>>> On Sun, 1 Feb 2009, Boaz Harrosh wrote:
>>>> Arnd Bergmann wrote:
>>>>> +struct space_resv {
>>>>> +	__s16		l_type;
>>>>> +	__s16		l_whence;
>>>>> +	__s64		l_start;
>>>>> +	__s64		l_len;		/* len == 0 means until end of file */
>>>>> +	__s32		l_sysid;
>>>>> +	__u32		l_pid;
>>>>> +	__s32		l_pad[4];	/* reserve area			    */
>>>>> +};
>>>> What about telling the compiler exactly what you said above, just
>>>> to be sure we all mean the same thing. (And as documentation for new
>>>> comers):
>>>>
>>>> +struct space_resv_64 {
>>>> +	__s16		l_type;
>>>> +	__s16		l_whence;
>>>> +	__u32		reserved;
>>>> +	__s64		l_start;
>>>> +	__s64		l_len;		/* len == 0 means until end of file */
>>>> +	__s32		l_sysid;
>>>> +	__u32		l_pid;
>>>> +	__s32		l_pad[4];	/* reserve area			    */
>>>> +} __packed;
>>> Because the compiler will assume all fields are always unaligned and will use very
>>> suboptimal code to access them?
>> This discussion comes up every once in a while. I'm using an old FC7 compiler
>> (gcc (GCC) 4.1.2 20070925 (Red Hat 4.1.2-27)) And tests show that when the layout
>> of a structure is exactly the same the "__packed" on structure declarations does
>> nothing. It only starts to affect when there are real differences in alignment.
>> Also tests with gcc 3.4.x showed the same effect.
>>
>> On previous discussions no one could come forward and say what compiler version
>> breaks when __packed is applied on structure definition. I'm afraid your statement
>> above is a myth.
> 
> FC7, targeting ia32? Sure, ia32 has no alignment restrictions.
> Try e.g. MIPS.
> 
> Gr{oetje,eeting}s,
> 
> 						Geert
> 

I don't understand

if you have a structure like
struct foo {
	u32 one;
	u32 two;
};
vs
struct foo_packed {
	u32 one;
	u32 two;
} __packed;

Just adding an __attribute__((packed)) to it clearly does not change
the layout of the structure. Are you saying the __attribute__((packed))
is an hint to the compiler that foo_packed might be used unaligned. This
is just brain-dead, because I can use an unaligned pointer to foo just as
I can to foo_packed. Otherwise there is no difference what-so-ever between
the two. I have to see it to believe. It is totally the wrong hint in the
wrong place taking away valuable meaning of saying "please don't use padding
holes in this structure"

Sorry for been so slow, I just don't get it.
Boaz

Re: [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Boaz Harrosh]

Geert Uytterhoeven wrote:
> On Sun, 1 Feb 2009, Boaz Harrosh wrote:
>> Geert Uytterhoeven wrote:
>>> On Sun, 1 Feb 2009, Boaz Harrosh wrote:
>>>> Arnd Bergmann wrote:
>>>>> +struct space_resv {
>>>>> +	__s16		l_type;
>>>>> +	__s16		l_whence;
>>>>> +	__s64		l_start;
>>>>> +	__s64		l_len;		/* len == 0 means until end of file */
>>>>> +	__s32		l_sysid;
>>>>> +	__u32		l_pid;
>>>>> +	__s32		l_pad[4];	/* reserve area			    */
>>>>> +};
>>>> What about telling the compiler exactly what you said above, just
>>>> to be sure we all mean the same thing. (And as documentation for new
>>>> comers):
>>>>
>>>> +struct space_resv_64 {
>>>> +	__s16		l_type;
>>>> +	__s16		l_whence;
>>>> +	__u32		reserved;
>>>> +	__s64		l_start;
>>>> +	__s64		l_len;		/* len == 0 means until end of file */
>>>> +	__s32		l_sysid;
>>>> +	__u32		l_pid;
>>>> +	__s32		l_pad[4];	/* reserve area			    */
>>>> +} __packed;
>>> Because the compiler will assume all fields are always unaligned and will use very
>>> suboptimal code to access them?
>> This discussion comes up every once in a while. I'm using an old FC7 compiler
>> (gcc (GCC) 4.1.2 20070925 (Red Hat 4.1.2-27)) And tests show that when the layout
>> of a structure is exactly the same the "__packed" on structure declarations does
>> nothing. It only starts to affect when there are real differences in alignment.
>> Also tests with gcc 3.4.x showed the same effect.
>>
>> On previous discussions no one could come forward and say what compiler version
>> breaks when __packed is applied on structure definition. I'm afraid your statement
>> above is a myth.
> 
> FC7, targeting ia32? Sure, ia32 has no alignment restrictions.
> Try e.g. MIPS.
> 
> Gr{oetje,eeting}s,
> 
> 						Geert
> 

I don't understand

if you have a structure like
struct foo {
	u32 one;
	u32 two;
};
vs
struct foo_packed {
	u32 one;
	u32 two;
} __packed;

Just adding an __attribute__((packed)) to it clearly does not change
the layout of the structure. Are you saying the __attribute__((packed))
is an hint to the compiler that foo_packed might be used unaligned. This
is just brain-dead, because I can use an unaligned pointer to foo just as
I can to foo_packed. Otherwise there is no difference what-so-ever between
the two. I have to see it to believe. It is totally the wrong hint in the
wrong place taking away valuable meaning of saying "please don't use padding
holes in this structure"

Sorry for been so slow, I just don't get it.
Boaz

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

[Ocfs2-devel] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Boaz Harrosh]

Geert Uytterhoeven wrote:
> On Sun, 1 Feb 2009, Boaz Harrosh wrote:
>> Geert Uytterhoeven wrote:
>>> On Sun, 1 Feb 2009, Boaz Harrosh wrote:
>>>> Arnd Bergmann wrote:
>>>>> +struct space_resv {
>>>>> +	__s16		l_type;
>>>>> +	__s16		l_whence;
>>>>> +	__s64		l_start;
>>>>> +	__s64		l_len;		/* len == 0 means until end of file */
>>>>> +	__s32		l_sysid;
>>>>> +	__u32		l_pid;
>>>>> +	__s32		l_pad[4];	/* reserve area			    */
>>>>> +};
>>>> What about telling the compiler exactly what you said above, just
>>>> to be sure we all mean the same thing. (And as documentation for new
>>>> comers):
>>>>
>>>> +struct space_resv_64 {
>>>> +	__s16		l_type;
>>>> +	__s16		l_whence;
>>>> +	__u32		reserved;
>>>> +	__s64		l_start;
>>>> +	__s64		l_len;		/* len == 0 means until end of file */
>>>> +	__s32		l_sysid;
>>>> +	__u32		l_pid;
>>>> +	__s32		l_pad[4];	/* reserve area			    */
>>>> +} __packed;
>>> Because the compiler will assume all fields are always unaligned and will use very
>>> suboptimal code to access them?
>> This discussion comes up every once in a while. I'm using an old FC7 compiler
>> (gcc (GCC) 4.1.2 20070925 (Red Hat 4.1.2-27)) And tests show that when the layout
>> of a structure is exactly the same the "__packed" on structure declarations does
>> nothing. It only starts to affect when there are real differences in alignment.
>> Also tests with gcc 3.4.x showed the same effect.
>>
>> On previous discussions no one could come forward and say what compiler version
>> breaks when __packed is applied on structure definition. I'm afraid your statement
>> above is a myth.
> 
> FC7, targeting ia32? Sure, ia32 has no alignment restrictions.
> Try e.g. MIPS.
> 
> Gr{oetje,eeting}s,
> 
> 						Geert
> 

I don't understand

if you have a structure like
struct foo {
	u32 one;
	u32 two;
};
vs
struct foo_packed {
	u32 one;
	u32 two;
} __packed;

Just adding an __attribute__((packed)) to it clearly does not change
the layout of the structure. Are you saying the __attribute__((packed))
is an hint to the compiler that foo_packed might be used unaligned. This
is just brain-dead, because I can use an unaligned pointer to foo just as
I can to foo_packed. Otherwise there is no difference what-so-ever between
the two. I have to see it to believe. It is totally the wrong hint in the
wrong place taking away valuable meaning of saying "please don't use padding
holes in this structure"

Sorry for been so slow, I just don't get it.
Boaz

Re: [xfs-masters] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Eric Sandeen]

Boaz Harrosh wrote:

...

> I don't understand
> 
> if you have a structure like
> struct foo {
> 	u32 one;
> 	u32 two;
> };
> vs
> struct foo_packed {
> 	u32 one;
> 	u32 two;
> } __packed;
> 
> Just adding an __attribute__((packed)) to it clearly does not change
> the layout of the structure. Are you saying the __attribute__((packed))
> is an hint to the compiler that foo_packed might be used unaligned. This
> is just brain-dead, because I can use an unaligned pointer to foo just as
> I can to foo_packed. Otherwise there is no difference what-so-ever between
> the two. I have to see it to believe. It is totally the wrong hint in the
> wrong place taking away valuable meaning of saying "please don't use padding
> holes in this structure"
> 
> Sorry for been so slow, I just don't get it.
> Boaz

While I'm no gcc guru, I can confirm that gratuitous use of the packed
attribute is suboptimal; adding "packed" to every ondisk structure made
obdump -d xfs.ko | wc -l explode by about 15,000 lines on ia64.

-Eric

Re: [xfs-masters] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Eric Sandeen]

Boaz Harrosh wrote:

...

> I don't understand
> 
> if you have a structure like
> struct foo {
> 	u32 one;
> 	u32 two;
> };
> vs
> struct foo_packed {
> 	u32 one;
> 	u32 two;
> } __packed;
> 
> Just adding an __attribute__((packed)) to it clearly does not change
> the layout of the structure. Are you saying the __attribute__((packed))
> is an hint to the compiler that foo_packed might be used unaligned. This
> is just brain-dead, because I can use an unaligned pointer to foo just as
> I can to foo_packed. Otherwise there is no difference what-so-ever between
> the two. I have to see it to believe. It is totally the wrong hint in the
> wrong place taking away valuable meaning of saying "please don't use padding
> holes in this structure"
> 
> Sorry for been so slow, I just don't get it.
> Boaz

While I'm no gcc guru, I can confirm that gratuitous use of the packed
attribute is suboptimal; adding "packed" to every ondisk structure made
obdump -d xfs.ko | wc -l explode by about 15,000 lines on ia64.

-Eric

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

[Ocfs2-devel] [xfs-masters] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Eric Sandeen]

Boaz Harrosh wrote:

...

> I don't understand
> 
> if you have a structure like
> struct foo {
> 	u32 one;
> 	u32 two;
> };
> vs
> struct foo_packed {
> 	u32 one;
> 	u32 two;
> } __packed;
> 
> Just adding an __attribute__((packed)) to it clearly does not change
> the layout of the structure. Are you saying the __attribute__((packed))
> is an hint to the compiler that foo_packed might be used unaligned. This
> is just brain-dead, because I can use an unaligned pointer to foo just as
> I can to foo_packed. Otherwise there is no difference what-so-ever between
> the two. I have to see it to believe. It is totally the wrong hint in the
> wrong place taking away valuable meaning of saying "please don't use padding
> holes in this structure"
> 
> Sorry for been so slow, I just don't get it.
> Boaz

While I'm no gcc guru, I can confirm that gratuitous use of the packed
attribute is suboptimal; adding "packed" to every ondisk structure made
obdump -d xfs.ko | wc -l explode by about 15,000 lines on ia64.

-Eric

Re: [xfs-masters] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Boaz Harrosh]

Eric Sandeen wrote:
> Boaz Harrosh wrote:
> 
> ...
> 
>> I don't understand
>>
>> if you have a structure like
>> struct foo {
>> 	u32 one;
>> 	u32 two;
>> };
>> vs
>> struct foo_packed {
>> 	u32 one;
>> 	u32 two;
>> } __packed;
>>
>> Just adding an __attribute__((packed)) to it clearly does not change
>> the layout of the structure. Are you saying the __attribute__((packed))
>> is an hint to the compiler that foo_packed might be used unaligned. This
>> is just brain-dead, because I can use an unaligned pointer to foo just as
>> I can to foo_packed. Otherwise there is no difference what-so-ever between
>> the two. I have to see it to believe. It is totally the wrong hint in the
>> wrong place taking away valuable meaning of saying "please don't use padding
>> holes in this structure"
>>
>> Sorry for been so slow, I just don't get it.
>> Boaz
> 
> While I'm no gcc guru, I can confirm that gratuitous use of the packed
> attribute is suboptimal; adding "packed" to every ondisk structure made
> obdump -d xfs.ko | wc -l explode by about 15,000 lines on ia64.

Yes! but are the structures the same? that is sizeof(foo_packed) == sizeof(foo) ?
If not then clearly above is expected.

In anyway, if __attribute__((packed)) makes some brain-dead gcc do the wrong thing
putting a _Padding member where you expect an alignment hole, and a BUILD_BUG_ON(sizeof() != ())
statement somewhere in code is a must, specifically for the brain-dead.

> 
> -Eric

There are to many places in Kernel where these things are left to chance that give
me an headache, not talking about cross platform mounts.

Boaz

Re: [xfs-masters] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Boaz Harrosh]

Eric Sandeen wrote:
> Boaz Harrosh wrote:
> 
> ...
> 
>> I don't understand
>>
>> if you have a structure like
>> struct foo {
>> 	u32 one;
>> 	u32 two;
>> };
>> vs
>> struct foo_packed {
>> 	u32 one;
>> 	u32 two;
>> } __packed;
>>
>> Just adding an __attribute__((packed)) to it clearly does not change
>> the layout of the structure. Are you saying the __attribute__((packed))
>> is an hint to the compiler that foo_packed might be used unaligned. This
>> is just brain-dead, because I can use an unaligned pointer to foo just as
>> I can to foo_packed. Otherwise there is no difference what-so-ever between
>> the two. I have to see it to believe. It is totally the wrong hint in the
>> wrong place taking away valuable meaning of saying "please don't use padding
>> holes in this structure"
>>
>> Sorry for been so slow, I just don't get it.
>> Boaz
> 
> While I'm no gcc guru, I can confirm that gratuitous use of the packed
> attribute is suboptimal; adding "packed" to every ondisk structure made
> obdump -d xfs.ko | wc -l explode by about 15,000 lines on ia64.

Yes! but are the structures the same? that is sizeof(foo_packed) == sizeof(foo) ?
If not then clearly above is expected.

In anyway, if __attribute__((packed)) makes some brain-dead gcc do the wrong thing
putting a _Padding member where you expect an alignment hole, and a BUILD_BUG_ON(sizeof() != ())
statement somewhere in code is a must, specifically for the brain-dead.

> 
> -Eric

There are to many places in Kernel where these things are left to chance that give
me an headache, not talking about cross platform mounts.

Boaz

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

[Ocfs2-devel] [xfs-masters] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Boaz Harrosh]

Eric Sandeen wrote:
> Boaz Harrosh wrote:
> 
> ...
> 
>> I don't understand
>>
>> if you have a structure like
>> struct foo {
>> 	u32 one;
>> 	u32 two;
>> };
>> vs
>> struct foo_packed {
>> 	u32 one;
>> 	u32 two;
>> } __packed;
>>
>> Just adding an __attribute__((packed)) to it clearly does not change
>> the layout of the structure. Are you saying the __attribute__((packed))
>> is an hint to the compiler that foo_packed might be used unaligned. This
>> is just brain-dead, because I can use an unaligned pointer to foo just as
>> I can to foo_packed. Otherwise there is no difference what-so-ever between
>> the two. I have to see it to believe. It is totally the wrong hint in the
>> wrong place taking away valuable meaning of saying "please don't use padding
>> holes in this structure"
>>
>> Sorry for been so slow, I just don't get it.
>> Boaz
> 
> While I'm no gcc guru, I can confirm that gratuitous use of the packed
> attribute is suboptimal; adding "packed" to every ondisk structure made
> obdump -d xfs.ko | wc -l explode by about 15,000 lines on ia64.

Yes! but are the structures the same? that is sizeof(foo_packed) == sizeof(foo) ?
If not then clearly above is expected.

In anyway, if __attribute__((packed)) makes some brain-dead gcc do the wrong thing
putting a _Padding member where you expect an alignment hole, and a BUILD_BUG_ON(sizeof() != ())
statement somewhere in code is a must, specifically for the brain-dead.

> 
> -Eric

There are to many places in Kernel where these things are left to chance that give
me an headache, not talking about cross platform mounts.

Boaz

Re: [xfs-masters] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Eric Sandeen]

Boaz Harrosh wrote:
> Eric Sandeen wrote:
>> Boaz Harrosh wrote:
>>
>> ...
>>
>>> I don't understand
>>>
>>> if you have a structure like
>>> struct foo {
>>> 	u32 one;
>>> 	u32 two;
>>> };
>>> vs
>>> struct foo_packed {
>>> 	u32 one;
>>> 	u32 two;
>>> } __packed;
>>>
>>> Just adding an __attribute__((packed)) to it clearly does not change
>>> the layout of the structure. Are you saying the __attribute__((packed))
>>> is an hint to the compiler that foo_packed might be used unaligned. This
>>> is just brain-dead, because I can use an unaligned pointer to foo just as
>>> I can to foo_packed. Otherwise there is no difference what-so-ever between
>>> the two. I have to see it to believe. It is totally the wrong hint in the
>>> wrong place taking away valuable meaning of saying "please don't use padding
>>> holes in this structure"
>>>
>>> Sorry for been so slow, I just don't get it.
>>> Boaz
>> While I'm no gcc guru, I can confirm that gratuitous use of the packed
>> attribute is suboptimal; adding "packed" to every ondisk structure made
>> obdump -d xfs.ko | wc -l explode by about 15,000 lines on ia64.
> 
> Yes! but are the structures the same? that is sizeof(foo_packed) == sizeof(foo) ?
> If not then clearly above is expected.

Yes, they are the same.  They're disk structure definitions after all;
ia64 doesn't *need* the packing, but adding the packed attribute changes
the code that gcc generates.

See also, perhaps,
http://digitalvampire.org/blog/index.php/2006/07/31/why-you-shouldnt-use-__attribute__packed/

For an interface like this maybe it's fine, but sprnkling it around like
pixie dust may not be a good plan.  :)

-Eric

Re: [xfs-masters] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Eric Sandeen]

Boaz Harrosh wrote:
> Eric Sandeen wrote:
>> Boaz Harrosh wrote:
>>
>> ...
>>
>>> I don't understand
>>>
>>> if you have a structure like
>>> struct foo {
>>> 	u32 one;
>>> 	u32 two;
>>> };
>>> vs
>>> struct foo_packed {
>>> 	u32 one;
>>> 	u32 two;
>>> } __packed;
>>>
>>> Just adding an __attribute__((packed)) to it clearly does not change
>>> the layout of the structure. Are you saying the __attribute__((packed))
>>> is an hint to the compiler that foo_packed might be used unaligned. This
>>> is just brain-dead, because I can use an unaligned pointer to foo just as
>>> I can to foo_packed. Otherwise there is no difference what-so-ever between
>>> the two. I have to see it to believe. It is totally the wrong hint in the
>>> wrong place taking away valuable meaning of saying "please don't use padding
>>> holes in this structure"
>>>
>>> Sorry for been so slow, I just don't get it.
>>> Boaz
>> While I'm no gcc guru, I can confirm that gratuitous use of the packed
>> attribute is suboptimal; adding "packed" to every ondisk structure made
>> obdump -d xfs.ko | wc -l explode by about 15,000 lines on ia64.
> 
> Yes! but are the structures the same? that is sizeof(foo_packed) == sizeof(foo) ?
> If not then clearly above is expected.

Yes, they are the same.  They're disk structure definitions after all;
ia64 doesn't *need* the packing, but adding the packed attribute changes
the code that gcc generates.

See also, perhaps,
http://digitalvampire.org/blog/index.php/2006/07/31/why-you-shouldnt-use-__attribute__packed/

For an interface like this maybe it's fine, but sprnkling it around like
pixie dust may not be a good plan.  :)

-Eric

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

[Ocfs2-devel] [xfs-masters] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Eric Sandeen]

Boaz Harrosh wrote:
> Eric Sandeen wrote:
>> Boaz Harrosh wrote:
>>
>> ...
>>
>>> I don't understand
>>>
>>> if you have a structure like
>>> struct foo {
>>> 	u32 one;
>>> 	u32 two;
>>> };
>>> vs
>>> struct foo_packed {
>>> 	u32 one;
>>> 	u32 two;
>>> } __packed;
>>>
>>> Just adding an __attribute__((packed)) to it clearly does not change
>>> the layout of the structure. Are you saying the __attribute__((packed))
>>> is an hint to the compiler that foo_packed might be used unaligned. This
>>> is just brain-dead, because I can use an unaligned pointer to foo just as
>>> I can to foo_packed. Otherwise there is no difference what-so-ever between
>>> the two. I have to see it to believe. It is totally the wrong hint in the
>>> wrong place taking away valuable meaning of saying "please don't use padding
>>> holes in this structure"
>>>
>>> Sorry for been so slow, I just don't get it.
>>> Boaz
>> While I'm no gcc guru, I can confirm that gratuitous use of the packed
>> attribute is suboptimal; adding "packed" to every ondisk structure made
>> obdump -d xfs.ko | wc -l explode by about 15,000 lines on ia64.
> 
> Yes! but are the structures the same? that is sizeof(foo_packed) == sizeof(foo) ?
> If not then clearly above is expected.

Yes, they are the same.  They're disk structure definitions after all;
ia64 doesn't *need* the packing, but adding the packed attribute changes
the code that gcc generates.

See also, perhaps,
http://digitalvampire.org/blog/index.php/2006/07/31/why-you-shouldnt-use-__attribute__packed/

For an interface like this maybe it's fine, but sprnkling it around like
pixie dust may not be a good plan.  :)

-Eric

Re: [xfs-masters] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Christoph Hellwig]

The structures have been defined exactly like that in XFS (and ocfs2)
before, and there are similar cases in other ioctls handlers.

If anyone feels like changing this in some way feel free to wade through
the endless discussions about the pros and cons for it, but I think
doing it in context of this patch is not helpful.

Re: [xfs-masters] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Christoph Hellwig]

The structures have been defined exactly like that in XFS (and ocfs2)
before, and there are similar cases in other ioctls handlers.

If anyone feels like changing this in some way feel free to wade through
the endless discussions about the pros and cons for it, but I think
doing it in context of this patch is not helpful.

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

[Ocfs2-devel] [xfs-masters] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Christoph Hellwig]

The structures have been defined exactly like that in XFS (and ocfs2)
before, and there are similar cases in other ioctls handlers.

If anyone feels like changing this in some way feel free to wade through
the endless discussions about the pros and cons for it, but I think
doing it in context of this patch is not helpful.

Re: [xfs-masters] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Boaz Harrosh]

Christoph Hellwig wrote:
> The structures have been defined exactly like that in XFS (and ocfs2)
> before, and there are similar cases in other ioctls handlers.
> 
> If anyone feels like changing this in some way feel free to wade through
> the endless discussions about the pros and cons for it, but I think
> doing it in context of this patch is not helpful.

OK so ia64 gcc is broken in regard to __attribute__((packed(1))),
and it should not be used.
But clearly the programmer, Like in this patch exactly, should spell
out the hole created by padding and spell that hole out and call it
__Padding. The porgrammer thought about it, identified there is an hole,
please don't drop this information on the floor. Put it in the code so I 
don't have to break my head on it.

> Arnd Bergmann wrote:
>> > +struct space_resv {
>> > +	__s16		l_type;
>> > +	__s16		l_whence;
>> > +	__s64		l_start;
>> > +	__s64		l_len;		/* len == 0 means until end of file */
>> > +	__s32		l_sysid;
>> > +	__u32		l_pid;
>> > +	__s32		l_pad[4];	/* reserve area			    */
>> > +};
> 
> What about telling the compiler exactly what you said above, just
> to be sure we all mean the same thing. (And as documentation for new
> comers):
> 
> +struct space_resv_64 {
> +	__s16		l_type;
> +	__s16		l_whence;
> +	__u32		reserved;
change that to
	__u32		__padding_hole;
> +	__s64		l_start;
> +	__s64		l_len;		/* len == 0 means until end of file */
> +	__s32		l_sysid;
> +	__u32		l_pid;
> +	__s32		l_pad[4];	/* reserve area			    */
> +} __packed;
Drop the evil __packed but show me the padding

Thanks
Boaz

Re: [xfs-masters] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Boaz Harrosh]

Christoph Hellwig wrote:
> The structures have been defined exactly like that in XFS (and ocfs2)
> before, and there are similar cases in other ioctls handlers.
> 
> If anyone feels like changing this in some way feel free to wade through
> the endless discussions about the pros and cons for it, but I think
> doing it in context of this patch is not helpful.

OK so ia64 gcc is broken in regard to __attribute__((packed(1))),
and it should not be used.
But clearly the programmer, Like in this patch exactly, should spell
out the hole created by padding and spell that hole out and call it
__Padding. The porgrammer thought about it, identified there is an hole,
please don't drop this information on the floor. Put it in the code so I 
don't have to break my head on it.

> Arnd Bergmann wrote:
>> > +struct space_resv {
>> > +	__s16		l_type;
>> > +	__s16		l_whence;
>> > +	__s64		l_start;
>> > +	__s64		l_len;		/* len == 0 means until end of file */
>> > +	__s32		l_sysid;
>> > +	__u32		l_pid;
>> > +	__s32		l_pad[4];	/* reserve area			    */
>> > +};
> 
> What about telling the compiler exactly what you said above, just
> to be sure we all mean the same thing. (And as documentation for new
> comers):
> 
> +struct space_resv_64 {
> +	__s16		l_type;
> +	__s16		l_whence;
> +	__u32		reserved;
change that to
	__u32		__padding_hole;
> +	__s64		l_start;
> +	__s64		l_len;		/* len == 0 means until end of file */
> +	__s32		l_sysid;
> +	__u32		l_pid;
> +	__s32		l_pad[4];	/* reserve area			    */
> +} __packed;
Drop the evil __packed but show me the padding

Thanks
Boaz

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

[Ocfs2-devel] [xfs-masters] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Boaz Harrosh]

Christoph Hellwig wrote:
> The structures have been defined exactly like that in XFS (and ocfs2)
> before, and there are similar cases in other ioctls handlers.
> 
> If anyone feels like changing this in some way feel free to wade through
> the endless discussions about the pros and cons for it, but I think
> doing it in context of this patch is not helpful.

OK so ia64 gcc is broken in regard to __attribute__((packed(1))),
and it should not be used.
But clearly the programmer, Like in this patch exactly, should spell
out the hole created by padding and spell that hole out and call it
__Padding. The porgrammer thought about it, identified there is an hole,
please don't drop this information on the floor. Put it in the code so I 
don't have to break my head on it.

> Arnd Bergmann wrote:
>> > +struct space_resv {
>> > +	__s16		l_type;
>> > +	__s16		l_whence;
>> > +	__s64		l_start;
>> > +	__s64		l_len;		/* len == 0 means until end of file */
>> > +	__s32		l_sysid;
>> > +	__u32		l_pid;
>> > +	__s32		l_pad[4];	/* reserve area			    */
>> > +};
> 
> What about telling the compiler exactly what you said above, just
> to be sure we all mean the same thing. (And as documentation for new
> comers):
> 
> +struct space_resv_64 {
> +	__s16		l_type;
> +	__s16		l_whence;
> +	__u32		reserved;
change that to
	__u32		__padding_hole;
> +	__s64		l_start;
> +	__s64		l_len;		/* len == 0 means until end of file */
> +	__s32		l_sysid;
> +	__u32		l_pid;
> +	__s32		l_pad[4];	/* reserve area			    */
> +} __packed;
Drop the evil __packed but show me the padding

Thanks
Boaz

Re: [xfs-masters] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Arnd Bergmann]

On Sunday 01 February 2009, Boaz Harrosh wrote:
> Christoph Hellwig wrote:
> > The structures have been defined exactly like that in XFS (and ocfs2)
> > before, and there are similar cases in other ioctls handlers.
> > 
> > If anyone feels like changing this in some way feel free to wade through
> > the endless discussions about the pros and cons for it, but I think
> > doing it in context of this patch is not helpful.
> 
> OK so ia64 gcc is broken in regard to __attribute__((packed(1))),
> and it should not be used.

No, the compiler is correct, it has to generate more complex code
if it cannot assume that data is naturally aligned and the architecture
does not support unaligned loads. If you don't understand this, please
at least read the list archives about the last five times this came up
before claiming that the compiler is broken.

> But clearly the programmer, Like in this patch exactly, should spell
> out the hole created by padding and spell that hole out and call it
> __Padding. The porgrammer thought about it, identified there is an hole,
> please don't drop this information on the floor. Put it in the code so I 
> don't have to break my head on it.

Again, you are missing the point. The patch has to leave out the padding
and whatnot because the idea of the patch is to provide a
backwards-compatible API for programs written against the legacy XFS
API. The original definition of the interface was flawed, but unless you
can travel back in time to complain about this in the initial submission
of the XFS file system, you should not blame the entirely correct patch now.

	Arnd <><

Re: [xfs-masters] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Arnd Bergmann]

On Sunday 01 February 2009, Boaz Harrosh wrote:
> Christoph Hellwig wrote:
> > The structures have been defined exactly like that in XFS (and ocfs2)
> > before, and there are similar cases in other ioctls handlers.
> > 
> > If anyone feels like changing this in some way feel free to wade through
> > the endless discussions about the pros and cons for it, but I think
> > doing it in context of this patch is not helpful.
> 
> OK so ia64 gcc is broken in regard to __attribute__((packed(1))),
> and it should not be used.

No, the compiler is correct, it has to generate more complex code
if it cannot assume that data is naturally aligned and the architecture
does not support unaligned loads. If you don't understand this, please
at least read the list archives about the last five times this came up
before claiming that the compiler is broken.

> But clearly the programmer, Like in this patch exactly, should spell
> out the hole created by padding and spell that hole out and call it
> __Padding. The porgrammer thought about it, identified there is an hole,
> please don't drop this information on the floor. Put it in the code so I 
> don't have to break my head on it.

Again, you are missing the point. The patch has to leave out the padding
and whatnot because the idea of the patch is to provide a
backwards-compatible API for programs written against the legacy XFS
API. The original definition of the interface was flawed, but unless you
can travel back in time to complain about this in the initial submission
of the XFS file system, you should not blame the entirely correct patch now.

	Arnd <><

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

[Ocfs2-devel] [xfs-masters] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Arnd Bergmann]

On Sunday 01 February 2009, Boaz Harrosh wrote:
> Christoph Hellwig wrote:
> > The structures have been defined exactly like that in XFS (and ocfs2)
> > before, and there are similar cases in other ioctls handlers.
> > 
> > If anyone feels like changing this in some way feel free to wade through
> > the endless discussions about the pros and cons for it, but I think
> > doing it in context of this patch is not helpful.
> 
> OK so ia64 gcc is broken in regard to __attribute__((packed(1))),
> and it should not be used.

No, the compiler is correct, it has to generate more complex code
if it cannot assume that data is naturally aligned and the architecture
does not support unaligned loads. If you don't understand this, please
at least read the list archives about the last five times this came up
before claiming that the compiler is broken.

> But clearly the programmer, Like in this patch exactly, should spell
> out the hole created by padding and spell that hole out and call it
> __Padding. The porgrammer thought about it, identified there is an hole,
> please don't drop this information on the floor. Put it in the code so I 
> don't have to break my head on it.

Again, you are missing the point. The patch has to leave out the padding
and whatnot because the idea of the patch is to provide a
backwards-compatible API for programs written against the legacy XFS
API. The original definition of the interface was flawed, but unless you
can travel back in time to complain about this in the initial submission
of the XFS file system, you should not blame the entirely correct patch now.

	Arnd <><

Re: [xfs-masters] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Boaz Harrosh]

Arnd Bergmann wrote:
> On Sunday 01 February 2009, Boaz Harrosh wrote:
>> Christoph Hellwig wrote:
>>> The structures have been defined exactly like that in XFS (and ocfs2)
>>> before, and there are similar cases in other ioctls handlers.
>>>
>>> If anyone feels like changing this in some way feel free to wade through
>>> the endless discussions about the pros and cons for it, but I think
>>> doing it in context of this patch is not helpful.
>> OK so ia64 gcc is broken in regard to __attribute__((packed(1))),
>> and it should not be used.
> 
> No, the compiler is correct, it has to generate more complex code
> if it cannot assume that data is naturally aligned and the architecture
> does not support unaligned loads. If you don't understand this, please
> at least read the list archives about the last five times this came up
> before claiming that the compiler is broken.
> 

Wrong!! Sorry, you guys don't listen.
I'm talking of the case where the structures are EXACTLY the same anyway
you look at them. sizeof(foo) == sizeof(foo_packed) and 
offsetof(foo_memmber) == offsetof(foo_packed_member) for every member of
the structure. foo && foo_packed are declared exactly the same but with
__attribute__((packed(1))) applied to the later.

THEN in ia64 case the compiler is brain dead, because it relates
"unalignment" to packed(1) which are two different things.

>> But clearly the programmer, Like in this patch exactly, should spell
>> out the hole created by padding and spell that hole out and call it
>> __Padding. The porgrammer thought about it, identified there is an hole,
>> please don't drop this information on the floor. Put it in the code so I 
>> don't have to break my head on it.
> 
> Again, you are missing the point. The patch has to leave out the padding
> and whatnot because the idea of the patch is to provide a
> backwards-compatible API for programs written against the legacy XFS
> API. The original definition of the interface was flawed, but unless you
> can travel back in time to complain about this in the initial submission
> of the XFS file system, you should not blame the entirely correct patch now.
> 

Lets make a small test is: sizeof(orig_struct) == sizeof(my_struct)?
offsetof(orig_any_memmber) == offsetof(my_any_memmber)?

Yes because the padding is there if you shove it under the rug or expose
it.

What I'm saying is don't repeat the passed mistakes, spell out the exact
structure layout. So it will compile the same for any <= 64bit machine.
If the way a structure was defined in the passed, produced two or more
memory representations, spell these out with different structure
definitions and choose the one you need at compile time.

Otherwise you have fixed nothing. You are just repeating the same
passed mistake. And down the future some programmer will wish he add
a time machine to go back and fix what we've done today. But bingo, he
did and told me about it, and now it is time to fix that.

> 	Arnd <><

Don't let the compiler have the driver's sit. You do the driving, because
he does not know where you want to go.

Boaz

Re: [xfs-masters] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Boaz Harrosh]

Arnd Bergmann wrote:
> On Sunday 01 February 2009, Boaz Harrosh wrote:
>> Christoph Hellwig wrote:
>>> The structures have been defined exactly like that in XFS (and ocfs2)
>>> before, and there are similar cases in other ioctls handlers.
>>>
>>> If anyone feels like changing this in some way feel free to wade through
>>> the endless discussions about the pros and cons for it, but I think
>>> doing it in context of this patch is not helpful.
>> OK so ia64 gcc is broken in regard to __attribute__((packed(1))),
>> and it should not be used.
> 
> No, the compiler is correct, it has to generate more complex code
> if it cannot assume that data is naturally aligned and the architecture
> does not support unaligned loads. If you don't understand this, please
> at least read the list archives about the last five times this came up
> before claiming that the compiler is broken.
> 

Wrong!! Sorry, you guys don't listen.
I'm talking of the case where the structures are EXACTLY the same anyway
you look at them. sizeof(foo) == sizeof(foo_packed) and 
offsetof(foo_memmber) == offsetof(foo_packed_member) for every member of
the structure. foo && foo_packed are declared exactly the same but with
__attribute__((packed(1))) applied to the later.

THEN in ia64 case the compiler is brain dead, because it relates
"unalignment" to packed(1) which are two different things.

>> But clearly the programmer, Like in this patch exactly, should spell
>> out the hole created by padding and spell that hole out and call it
>> __Padding. The porgrammer thought about it, identified there is an hole,
>> please don't drop this information on the floor. Put it in the code so I 
>> don't have to break my head on it.
> 
> Again, you are missing the point. The patch has to leave out the padding
> and whatnot because the idea of the patch is to provide a
> backwards-compatible API for programs written against the legacy XFS
> API. The original definition of the interface was flawed, but unless you
> can travel back in time to complain about this in the initial submission
> of the XFS file system, you should not blame the entirely correct patch now.
> 

Lets make a small test is: sizeof(orig_struct) == sizeof(my_struct)?
offsetof(orig_any_memmber) == offsetof(my_any_memmber)?

Yes because the padding is there if you shove it under the rug or expose
it.

What I'm saying is don't repeat the passed mistakes, spell out the exact
structure layout. So it will compile the same for any <= 64bit machine.
If the way a structure was defined in the passed, produced two or more
memory representations, spell these out with different structure
definitions and choose the one you need at compile time.

Otherwise you have fixed nothing. You are just repeating the same
passed mistake. And down the future some programmer will wish he add
a time machine to go back and fix what we've done today. But bingo, he
did and told me about it, and now it is time to fix that.

> 	Arnd <><

Don't let the compiler have the driver's sit. You do the driving, because
he does not know where you want to go.

Boaz

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

[Ocfs2-devel] [xfs-masters] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Boaz Harrosh]

Arnd Bergmann wrote:
> On Sunday 01 February 2009, Boaz Harrosh wrote:
>> Christoph Hellwig wrote:
>>> The structures have been defined exactly like that in XFS (and ocfs2)
>>> before, and there are similar cases in other ioctls handlers.
>>>
>>> If anyone feels like changing this in some way feel free to wade through
>>> the endless discussions about the pros and cons for it, but I think
>>> doing it in context of this patch is not helpful.
>> OK so ia64 gcc is broken in regard to __attribute__((packed(1))),
>> and it should not be used.
> 
> No, the compiler is correct, it has to generate more complex code
> if it cannot assume that data is naturally aligned and the architecture
> does not support unaligned loads. If you don't understand this, please
> at least read the list archives about the last five times this came up
> before claiming that the compiler is broken.
> 

Wrong!! Sorry, you guys don't listen.
I'm talking of the case where the structures are EXACTLY the same anyway
you look at them. sizeof(foo) == sizeof(foo_packed) and 
offsetof(foo_memmber) == offsetof(foo_packed_member) for every member of
the structure. foo && foo_packed are declared exactly the same but with
__attribute__((packed(1))) applied to the later.

THEN in ia64 case the compiler is brain dead, because it relates
"unalignment" to packed(1) which are two different things.

>> But clearly the programmer, Like in this patch exactly, should spell
>> out the hole created by padding and spell that hole out and call it
>> __Padding. The porgrammer thought about it, identified there is an hole,
>> please don't drop this information on the floor. Put it in the code so I 
>> don't have to break my head on it.
> 
> Again, you are missing the point. The patch has to leave out the padding
> and whatnot because the idea of the patch is to provide a
> backwards-compatible API for programs written against the legacy XFS
> API. The original definition of the interface was flawed, but unless you
> can travel back in time to complain about this in the initial submission
> of the XFS file system, you should not blame the entirely correct patch now.
> 

Lets make a small test is: sizeof(orig_struct) == sizeof(my_struct)?
offsetof(orig_any_memmber) == offsetof(my_any_memmber)?

Yes because the padding is there if you shove it under the rug or expose
it.

What I'm saying is don't repeat the passed mistakes, spell out the exact
structure layout. So it will compile the same for any <= 64bit machine.
If the way a structure was defined in the passed, produced two or more
memory representations, spell these out with different structure
definitions and choose the one you need@compile time.

Otherwise you have fixed nothing. You are just repeating the same
passed mistake. And down the future some programmer will wish he add
a time machine to go back and fix what we've done today. But bingo, he
did and told me about it, and now it is time to fix that.

> 	Arnd <><

Don't let the compiler have the driver's sit. You do the driving, because
he does not know where you want to go.

Boaz

Re: [xfs-masters] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Geert Uytterhoeven]

On Mon, 2 Feb 2009, Boaz Harrosh wrote:
> Arnd Bergmann wrote:
> > On Sunday 01 February 2009, Boaz Harrosh wrote:
> >> Christoph Hellwig wrote:
> >>> The structures have been defined exactly like that in XFS (and ocfs2)
> >>> before, and there are similar cases in other ioctls handlers.
> >>>
> >>> If anyone feels like changing this in some way feel free to wade through
> >>> the endless discussions about the pros and cons for it, but I think
> >>> doing it in context of this patch is not helpful.
> >> OK so ia64 gcc is broken in regard to __attribute__((packed(1))),
> >> and it should not be used.
> > 
> > No, the compiler is correct, it has to generate more complex code
> > if it cannot assume that data is naturally aligned and the architecture
> > does not support unaligned loads. If you don't understand this, please
> > at least read the list archives about the last five times this came up
> > before claiming that the compiler is broken.
> > 
> 
> Wrong!! Sorry, you guys don't listen.
> I'm talking of the case where the structures are EXACTLY the same anyway
> you look at them. sizeof(foo) == sizeof(foo_packed) and 
> offsetof(foo_memmber) == offsetof(foo_packed_member) for every member of
> the structure. foo && foo_packed are declared exactly the same but with
> __attribute__((packed(1))) applied to the later.
> 
> THEN in ia64 case the compiler is brain dead, because it relates
> "unalignment" to packed(1) which are two different things.

The natural alignment of a structure is max(alignment(member)), for all
members. With __attribute__((packed)), the natural alignment of the structure
is 1, so the compiler cannot assume anything.

While the ints in the structure may still be at offsets 0, 4, 8, and so on,
this doesn't say anything about their actual memory addresses, as the struct
base address itself may be unaligned.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: [xfs-masters] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Geert Uytterhoeven]

On Mon, 2 Feb 2009, Boaz Harrosh wrote:
> Arnd Bergmann wrote:
> > On Sunday 01 February 2009, Boaz Harrosh wrote:
> >> Christoph Hellwig wrote:
> >>> The structures have been defined exactly like that in XFS (and ocfs2)
> >>> before, and there are similar cases in other ioctls handlers.
> >>>
> >>> If anyone feels like changing this in some way feel free to wade through
> >>> the endless discussions about the pros and cons for it, but I think
> >>> doing it in context of this patch is not helpful.
> >> OK so ia64 gcc is broken in regard to __attribute__((packed(1))),
> >> and it should not be used.
> > 
> > No, the compiler is correct, it has to generate more complex code
> > if it cannot assume that data is naturally aligned and the architecture
> > does not support unaligned loads. If you don't understand this, please
> > at least read the list archives about the last five times this came up
> > before claiming that the compiler is broken.
> > 
> 
> Wrong!! Sorry, you guys don't listen.
> I'm talking of the case where the structures are EXACTLY the same anyway
> you look at them. sizeof(foo) == sizeof(foo_packed) and 
> offsetof(foo_memmber) == offsetof(foo_packed_member) for every member of
> the structure. foo && foo_packed are declared exactly the same but with
> __attribute__((packed(1))) applied to the later.
> 
> THEN in ia64 case the compiler is brain dead, because it relates
> "unalignment" to packed(1) which are two different things.

The natural alignment of a structure is max(alignment(member)), for all
members. With __attribute__((packed)), the natural alignment of the structure
is 1, so the compiler cannot assume anything.

While the ints in the structure may still be at offsets 0, 4, 8, and so on,
this doesn't say anything about their actual memory addresses, as the struct
base address itself may be unaligned.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

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

[Ocfs2-devel] [xfs-masters] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Geert Uytterhoeven]

On Mon, 2 Feb 2009, Boaz Harrosh wrote:
> Arnd Bergmann wrote:
> > On Sunday 01 February 2009, Boaz Harrosh wrote:
> >> Christoph Hellwig wrote:
> >>> The structures have been defined exactly like that in XFS (and ocfs2)
> >>> before, and there are similar cases in other ioctls handlers.
> >>>
> >>> If anyone feels like changing this in some way feel free to wade through
> >>> the endless discussions about the pros and cons for it, but I think
> >>> doing it in context of this patch is not helpful.
> >> OK so ia64 gcc is broken in regard to __attribute__((packed(1))),
> >> and it should not be used.
> > 
> > No, the compiler is correct, it has to generate more complex code
> > if it cannot assume that data is naturally aligned and the architecture
> > does not support unaligned loads. If you don't understand this, please
> > at least read the list archives about the last five times this came up
> > before claiming that the compiler is broken.
> > 
> 
> Wrong!! Sorry, you guys don't listen.
> I'm talking of the case where the structures are EXACTLY the same anyway
> you look at them. sizeof(foo) == sizeof(foo_packed) and 
> offsetof(foo_memmber) == offsetof(foo_packed_member) for every member of
> the structure. foo && foo_packed are declared exactly the same but with
> __attribute__((packed(1))) applied to the later.
> 
> THEN in ia64 case the compiler is brain dead, because it relates
> "unalignment" to packed(1) which are two different things.

The natural alignment of a structure is max(alignment(member)), for all
members. With __attribute__((packed)), the natural alignment of the structure
is 1, so the compiler cannot assume anything.

While the ints in the structure may still be at offsets 0, 4, 8, and so on,
this doesn't say anything about their actual memory addresses, as the struct
base address itself may be unaligned.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert at linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

Re: [xfs-masters] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Boaz Harrosh]

Geert Uytterhoeven wrote:
> On Mon, 2 Feb 2009, Boaz Harrosh wrote:
>> Arnd Bergmann wrote:
>>> No, the compiler is correct, it has to generate more complex code
>>> if it cannot assume that data is naturally aligned and the architecture
>>> does not support unaligned loads. If you don't understand this, please
>>> at least read the list archives about the last five times this came up
>>> before claiming that the compiler is broken.
>>>
>> Wrong!! Sorry, you guys don't listen.
>> I'm talking of the case where the structures are EXACTLY the same anyway
>> you look at them. sizeof(foo) == sizeof(foo_packed) and 
>> offsetof(foo_memmber) == offsetof(foo_packed_member) for every member of
>> the structure. foo && foo_packed are declared exactly the same but with
>> __attribute__((packed(1))) applied to the later.
>>
>> THEN in ia64 case the compiler is brain dead, because it relates
>> "unalignment" to packed(1) which are two different things.
> 
> The natural alignment of a structure is max(alignment(member)), for all
> members. With __attribute__((packed)), the natural alignment of the structure
> is 1, so the compiler cannot assume anything.
> 

No the natural alignment is what it is, after the application of
__attribute__((packed(1))). In a well defined structure that is a no-opt.
But yes in ai64 the gcc programmers got lazy and did not make that analysis
after laying out the structure.

> While the ints in the structure may still be at offsets 0, 4, 8, and so on,
> this doesn't say anything about their actual memory addresses, as the struct
> base address itself may be unaligned.
> 

The base address can be unaligned even if the structure is aligned. In that
case you need the __atrubute__((aligned)) thingy. It is true that if the sizeof(foo_packed)
is though unaligned, the compiler will have to assume unalignment in array operations.
but if the sizeof(foo_packed) is naturally aligned at the output then the compiler
has all the needed information to know that even if I declared __packed, it calculated
and knows that it is well aligned at the end.

> Gr{oetje,eeting}s,
> 
> 						Geert
> 

Please note that I gave up on the compiler and understand that the use of __packed
is dangerous in some cases, sigh. My standing point is to make sure there are no guesses
left, and a BUILD_BUG_ON to make sure of that.

Boaz

Re: [xfs-masters] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Boaz Harrosh]

Geert Uytterhoeven wrote:
> On Mon, 2 Feb 2009, Boaz Harrosh wrote:
>> Arnd Bergmann wrote:
>>> No, the compiler is correct, it has to generate more complex code
>>> if it cannot assume that data is naturally aligned and the architecture
>>> does not support unaligned loads. If you don't understand this, please
>>> at least read the list archives about the last five times this came up
>>> before claiming that the compiler is broken.
>>>
>> Wrong!! Sorry, you guys don't listen.
>> I'm talking of the case where the structures are EXACTLY the same anyway
>> you look at them. sizeof(foo) == sizeof(foo_packed) and 
>> offsetof(foo_memmber) == offsetof(foo_packed_member) for every member of
>> the structure. foo && foo_packed are declared exactly the same but with
>> __attribute__((packed(1))) applied to the later.
>>
>> THEN in ia64 case the compiler is brain dead, because it relates
>> "unalignment" to packed(1) which are two different things.
> 
> The natural alignment of a structure is max(alignment(member)), for all
> members. With __attribute__((packed)), the natural alignment of the structure
> is 1, so the compiler cannot assume anything.
> 

No the natural alignment is what it is, after the application of
__attribute__((packed(1))). In a well defined structure that is a no-opt.
But yes in ai64 the gcc programmers got lazy and did not make that analysis
after laying out the structure.

> While the ints in the structure may still be at offsets 0, 4, 8, and so on,
> this doesn't say anything about their actual memory addresses, as the struct
> base address itself may be unaligned.
> 

The base address can be unaligned even if the structure is aligned. In that
case you need the __atrubute__((aligned)) thingy. It is true that if the sizeof(foo_packed)
is though unaligned, the compiler will have to assume unalignment in array operations.
but if the sizeof(foo_packed) is naturally aligned at the output then the compiler
has all the needed information to know that even if I declared __packed, it calculated
and knows that it is well aligned at the end.

> Gr{oetje,eeting}s,
> 
> 						Geert
> 

Please note that I gave up on the compiler and understand that the use of __packed
is dangerous in some cases, sigh. My standing point is to make sure there are no guesses
left, and a BUILD_BUG_ON to make sure of that.

Boaz

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

[Ocfs2-devel] [xfs-masters] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Boaz Harrosh]

Geert Uytterhoeven wrote:
> On Mon, 2 Feb 2009, Boaz Harrosh wrote:
>> Arnd Bergmann wrote:
>>> No, the compiler is correct, it has to generate more complex code
>>> if it cannot assume that data is naturally aligned and the architecture
>>> does not support unaligned loads. If you don't understand this, please
>>> at least read the list archives about the last five times this came up
>>> before claiming that the compiler is broken.
>>>
>> Wrong!! Sorry, you guys don't listen.
>> I'm talking of the case where the structures are EXACTLY the same anyway
>> you look at them. sizeof(foo) == sizeof(foo_packed) and 
>> offsetof(foo_memmber) == offsetof(foo_packed_member) for every member of
>> the structure. foo && foo_packed are declared exactly the same but with
>> __attribute__((packed(1))) applied to the later.
>>
>> THEN in ia64 case the compiler is brain dead, because it relates
>> "unalignment" to packed(1) which are two different things.
> 
> The natural alignment of a structure is max(alignment(member)), for all
> members. With __attribute__((packed)), the natural alignment of the structure
> is 1, so the compiler cannot assume anything.
> 

No the natural alignment is what it is, after the application of
__attribute__((packed(1))). In a well defined structure that is a no-opt.
But yes in ai64 the gcc programmers got lazy and did not make that analysis
after laying out the structure.

> While the ints in the structure may still be at offsets 0, 4, 8, and so on,
> this doesn't say anything about their actual memory addresses, as the struct
> base address itself may be unaligned.
> 

The base address can be unaligned even if the structure is aligned. In that
case you need the __atrubute__((aligned)) thingy. It is true that if the sizeof(foo_packed)
is though unaligned, the compiler will have to assume unalignment in array operations.
but if the sizeof(foo_packed) is naturally aligned at the output then the compiler
has all the needed information to know that even if I declared __packed, it calculated
and knows that it is well aligned at the end.

> Gr{oetje,eeting}s,
> 
> 						Geert
> 

Please note that I gave up on the compiler and understand that the use of __packed
is dangerous in some cases, sigh. My standing point is to make sure there are no guesses
left, and a BUILD_BUG_ON to make sure of that.

Boaz

Re: [xfs-masters] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Jamie Lokier]

Boaz Harrosh wrote:
> No the natural alignment is what it is, after the application of
> __attribute__((packed(1))). In a well defined structure that is a no-opt.
> But yes in ai64 the gcc programmers got lazy and did not make that analysis
> after laying out the structure.

No.  That's what you *want* packed to mean, but it doesn't mean that.

__attribute__ packed on a struct definition means to pack the
structure _and_ set its assumed alignment to 1.

This is what the packed attribute historically means, and it cannot be
changed without breaking existing code.

If you want to remove padding from a structure, but still keep its
natural alignment, you do it with two attributes together:
__attribute__((packed, aligned(4))).  You have to choose the alignment
you want in that case.

GCC manual:
     When used on a struct, or struct member, the `aligned' attribute
     can only increase the alignment; in order to decrease it, the
     `packed' attribute must be specified as well.  When used as part
     of a typedef, the `aligned' attribute can both increase and
     decrease alignment, and specifying the `packed' attribute will
     generate a warning.

It's a counterintuitive, because you must use
__attribute__((aligned(1))) when declaring a variable to reduce its
alignment, but you must use __attribute__((packed)) when declaring a
struct type.  Doing it at the end of a struct typedef is a weird mix
of semantics, so don't do that.

By the way, this discussion is why the "-Wpacked" and "-Wpadding"
options are available.

> The base address can be unaligned even if the structure is aligned. In that
> case you need the __atrubute__((aligned)) thingy.

No, because __attribute__((packed)) on a struct doesn't mean what you
want it to mean.  Use __attribute((packed,aligned(4))) if that's what
you want.

> Please note that I gave up on the compiler and understand that the
> use of __packed is dangerous in some cases, sigh. My standing point
> is to make sure there are no guesses left, and a BUILD_BUG_ON to
> make sure of that.

In this code, it's not a bug because it must be backward compatible
with existing binary code.  "Fixing" the padding breaks
compatibility, which is pointless for this patch.

-- Jamie

Re: [xfs-masters] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Jamie Lokier]

Boaz Harrosh wrote:
> No the natural alignment is what it is, after the application of
> __attribute__((packed(1))). In a well defined structure that is a no-opt.
> But yes in ai64 the gcc programmers got lazy and did not make that analysis
> after laying out the structure.

No.  That's what you *want* packed to mean, but it doesn't mean that.

__attribute__ packed on a struct definition means to pack the
structure _and_ set its assumed alignment to 1.

This is what the packed attribute historically means, and it cannot be
changed without breaking existing code.

If you want to remove padding from a structure, but still keep its
natural alignment, you do it with two attributes together:
__attribute__((packed, aligned(4))).  You have to choose the alignment
you want in that case.

GCC manual:
     When used on a struct, or struct member, the `aligned' attribute
     can only increase the alignment; in order to decrease it, the
     `packed' attribute must be specified as well.  When used as part
     of a typedef, the `aligned' attribute can both increase and
     decrease alignment, and specifying the `packed' attribute will
     generate a warning.

It's a counterintuitive, because you must use
__attribute__((aligned(1))) when declaring a variable to reduce its
alignment, but you must use __attribute__((packed)) when declaring a
struct type.  Doing it at the end of a struct typedef is a weird mix
of semantics, so don't do that.

By the way, this discussion is why the "-Wpacked" and "-Wpadding"
options are available.

> The base address can be unaligned even if the structure is aligned. In that
> case you need the __atrubute__((aligned)) thingy.

No, because __attribute__((packed)) on a struct doesn't mean what you
want it to mean.  Use __attribute((packed,aligned(4))) if that's what
you want.

> Please note that I gave up on the compiler and understand that the
> use of __packed is dangerous in some cases, sigh. My standing point
> is to make sure there are no guesses left, and a BUILD_BUG_ON to
> make sure of that.

In this code, it's not a bug because it must be backward compatible
with existing binary code.  "Fixing" the padding breaks
compatibility, which is pointless for this patch.

-- Jamie

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

[Ocfs2-devel] [xfs-masters] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Jamie Lokier]

Boaz Harrosh wrote:
> No the natural alignment is what it is, after the application of
> __attribute__((packed(1))). In a well defined structure that is a no-opt.
> But yes in ai64 the gcc programmers got lazy and did not make that analysis
> after laying out the structure.

No.  That's what you *want* packed to mean, but it doesn't mean that.

__attribute__ packed on a struct definition means to pack the
structure _and_ set its assumed alignment to 1.

This is what the packed attribute historically means, and it cannot be
changed without breaking existing code.

If you want to remove padding from a structure, but still keep its
natural alignment, you do it with two attributes together:
__attribute__((packed, aligned(4))).  You have to choose the alignment
you want in that case.

GCC manual:
     When used on a struct, or struct member, the `aligned' attribute
     can only increase the alignment; in order to decrease it, the
     `packed' attribute must be specified as well.  When used as part
     of a typedef, the `aligned' attribute can both increase and
     decrease alignment, and specifying the `packed' attribute will
     generate a warning.

It's a counterintuitive, because you must use
__attribute__((aligned(1))) when declaring a variable to reduce its
alignment, but you must use __attribute__((packed)) when declaring a
struct type.  Doing it at the end of a struct typedef is a weird mix
of semantics, so don't do that.

By the way, this discussion is why the "-Wpacked" and "-Wpadding"
options are available.

> The base address can be unaligned even if the structure is aligned. In that
> case you need the __atrubute__((aligned)) thingy.

No, because __attribute__((packed)) on a struct doesn't mean what you
want it to mean.  Use __attribute((packed,aligned(4))) if that's what
you want.

> Please note that I gave up on the compiler and understand that the
> use of __packed is dangerous in some cases, sigh. My standing point
> is to make sure there are no guesses left, and a BUILD_BUG_ON to
> make sure of that.

In this code, it's not a bug because it must be backward compatible
with existing binary code.  "Fixing" the padding breaks
compatibility, which is pointless for this patch.

-- Jamie

Re: [xfs-masters] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Boaz Harrosh]

Jamie Lokier wrote:
> Boaz Harrosh wrote:
>> No the natural alignment is what it is, after the application of
>> __attribute__((packed(1))). In a well defined structure that is a no-opt.
>> But yes in ai64 the gcc programmers got lazy and did not make that analysis
>> after laying out the structure.
> 
> No.  That's what you *want* packed to mean, but it doesn't mean that.
> 
> __attribute__ packed on a struct definition means to pack the
> structure _and_ set its assumed alignment to 1.
> 
> This is what the packed attribute historically means, and it cannot be
> changed without breaking existing code.
> 
> If you want to remove padding from a structure, but still keep its
> natural alignment, you do it with two attributes together:
> __attribute__((packed, aligned(4))).  You have to choose the alignment
> you want in that case.
> 
> GCC manual:
>      When used on a struct, or struct member, the `aligned' attribute
>      can only increase the alignment; in order to decrease it, the
>      `packed' attribute must be specified as well.  When used as part
>      of a typedef, the `aligned' attribute can both increase and
>      decrease alignment, and specifying the `packed' attribute will
>      generate a warning.
> 
> It's a counterintuitive, because you must use
> __attribute__((aligned(1))) when declaring a variable to reduce its
> alignment, but you must use __attribute__((packed)) when declaring a
> struct type.  Doing it at the end of a struct typedef is a weird mix
> of semantics, so don't do that.
> 
> By the way, this discussion is why the "-Wpacked" and "-Wpadding"
> options are available.
> 
>> The base address can be unaligned even if the structure is aligned. In that
>> case you need the __atrubute__((aligned)) thingy.
> 
> No, because __attribute__((packed)) on a struct doesn't mean what you
> want it to mean.  Use __attribute((packed,aligned(4))) if that's what
> you want.
> 

OK Thanks I'll use that then. 
(And BUILD_BUG_ON to make sure)

>> Please note that I gave up on the compiler and understand that the
>> use of __packed is dangerous in some cases, sigh. My standing point
>> is to make sure there are no guesses left, and a BUILD_BUG_ON to
>> make sure of that.
> 
> In this code, it's not a bug because it must be backward compatible
> with existing binary code.  "Fixing" the padding breaks
> compatibility, which is pointless for this patch.
> 

That's what I don't like. In compatibility problems situation, we actually
have two kind of structures. I rather that they are spelled out explicitly
and chosen from at the appropriate places. But I'll give up it is probably
just me. (Though proof that it was not me who raised the subject)

I would at least expect a big fat comment explaining what happened to the
structure on what known ARCHs, and how it is expected to look in memory.
And a BUILD_BUG_ON to make sure of that.

> -- Jamie

Thanks, the __attribute((packed,aligned(__WORD_SIZE))) is new for me

Boaz

Re: [xfs-masters] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Boaz Harrosh]

Jamie Lokier wrote:
> Boaz Harrosh wrote:
>> No the natural alignment is what it is, after the application of
>> __attribute__((packed(1))). In a well defined structure that is a no-opt.
>> But yes in ai64 the gcc programmers got lazy and did not make that analysis
>> after laying out the structure.
> 
> No.  That's what you *want* packed to mean, but it doesn't mean that.
> 
> __attribute__ packed on a struct definition means to pack the
> structure _and_ set its assumed alignment to 1.
> 
> This is what the packed attribute historically means, and it cannot be
> changed without breaking existing code.
> 
> If you want to remove padding from a structure, but still keep its
> natural alignment, you do it with two attributes together:
> __attribute__((packed, aligned(4))).  You have to choose the alignment
> you want in that case.
> 
> GCC manual:
>      When used on a struct, or struct member, the `aligned' attribute
>      can only increase the alignment; in order to decrease it, the
>      `packed' attribute must be specified as well.  When used as part
>      of a typedef, the `aligned' attribute can both increase and
>      decrease alignment, and specifying the `packed' attribute will
>      generate a warning.
> 
> It's a counterintuitive, because you must use
> __attribute__((aligned(1))) when declaring a variable to reduce its
> alignment, but you must use __attribute__((packed)) when declaring a
> struct type.  Doing it at the end of a struct typedef is a weird mix
> of semantics, so don't do that.
> 
> By the way, this discussion is why the "-Wpacked" and "-Wpadding"
> options are available.
> 
>> The base address can be unaligned even if the structure is aligned. In that
>> case you need the __atrubute__((aligned)) thingy.
> 
> No, because __attribute__((packed)) on a struct doesn't mean what you
> want it to mean.  Use __attribute((packed,aligned(4))) if that's what
> you want.
> 

OK Thanks I'll use that then. 
(And BUILD_BUG_ON to make sure)

>> Please note that I gave up on the compiler and understand that the
>> use of __packed is dangerous in some cases, sigh. My standing point
>> is to make sure there are no guesses left, and a BUILD_BUG_ON to
>> make sure of that.
> 
> In this code, it's not a bug because it must be backward compatible
> with existing binary code.  "Fixing" the padding breaks
> compatibility, which is pointless for this patch.
> 

That's what I don't like. In compatibility problems situation, we actually
have two kind of structures. I rather that they are spelled out explicitly
and chosen from at the appropriate places. But I'll give up it is probably
just me. (Though proof that it was not me who raised the subject)

I would at least expect a big fat comment explaining what happened to the
structure on what known ARCHs, and how it is expected to look in memory.
And a BUILD_BUG_ON to make sure of that.

> -- Jamie

Thanks, the __attribute((packed,aligned(__WORD_SIZE))) is new for me

Boaz

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

[Ocfs2-devel] [xfs-masters] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Boaz Harrosh]

Jamie Lokier wrote:
> Boaz Harrosh wrote:
>> No the natural alignment is what it is, after the application of
>> __attribute__((packed(1))). In a well defined structure that is a no-opt.
>> But yes in ai64 the gcc programmers got lazy and did not make that analysis
>> after laying out the structure.
> 
> No.  That's what you *want* packed to mean, but it doesn't mean that.
> 
> __attribute__ packed on a struct definition means to pack the
> structure _and_ set its assumed alignment to 1.
> 
> This is what the packed attribute historically means, and it cannot be
> changed without breaking existing code.
> 
> If you want to remove padding from a structure, but still keep its
> natural alignment, you do it with two attributes together:
> __attribute__((packed, aligned(4))).  You have to choose the alignment
> you want in that case.
> 
> GCC manual:
>      When used on a struct, or struct member, the `aligned' attribute
>      can only increase the alignment; in order to decrease it, the
>      `packed' attribute must be specified as well.  When used as part
>      of a typedef, the `aligned' attribute can both increase and
>      decrease alignment, and specifying the `packed' attribute will
>      generate a warning.
> 
> It's a counterintuitive, because you must use
> __attribute__((aligned(1))) when declaring a variable to reduce its
> alignment, but you must use __attribute__((packed)) when declaring a
> struct type.  Doing it at the end of a struct typedef is a weird mix
> of semantics, so don't do that.
> 
> By the way, this discussion is why the "-Wpacked" and "-Wpadding"
> options are available.
> 
>> The base address can be unaligned even if the structure is aligned. In that
>> case you need the __atrubute__((aligned)) thingy.
> 
> No, because __attribute__((packed)) on a struct doesn't mean what you
> want it to mean.  Use __attribute((packed,aligned(4))) if that's what
> you want.
> 

OK Thanks I'll use that then. 
(And BUILD_BUG_ON to make sure)

>> Please note that I gave up on the compiler and understand that the
>> use of __packed is dangerous in some cases, sigh. My standing point
>> is to make sure there are no guesses left, and a BUILD_BUG_ON to
>> make sure of that.
> 
> In this code, it's not a bug because it must be backward compatible
> with existing binary code.  "Fixing" the padding breaks
> compatibility, which is pointless for this patch.
> 

That's what I don't like. In compatibility problems situation, we actually
have two kind of structures. I rather that they are spelled out explicitly
and chosen from at the appropriate places. But I'll give up it is probably
just me. (Though proof that it was not me who raised the subject)

I would at least expect a big fat comment explaining what happened to the
structure on what known ARCHs, and how it is expected to look in memory.
And a BUILD_BUG_ON to make sure of that.

> -- Jamie

Thanks, the __attribute((packed,aligned(__WORD_SIZE))) is new for me

Boaz

Re: [xfs-masters] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Jamie Lokier]

Boaz Harrosh wrote:
> I would at least expect a big fat comment explaining what happened to the
> structure on what known ARCHs, and how it is expected to look in memory.
> And a BUILD_BUG_ON to make sure of that.

You may have a point.

Struct layout on some architectures changes between compiler default
ABIs in these implicit-padding cases.  Kernel binary compatibility
will be affected.  It is a good reason why we have explicit padding to
natural alignment normally.

>From http://wiki.debian.org/ArmEabiPort

    "With the new ABI, default structure packing changes, as do some
    default data sizes and alignment (which also have a knock-on effect on
    structure packing). In particular the minimum size and alignment of a
    structure was 4 bytes. Under the EABI there is no minimum and the
    alignment is determined by the types of the components it
    contains. This will break programs that know too much about the way
    structures are packed and can break code that writes binary files by
    dumping and reading structures."

    "One of the key differences between the traditional GNU/Linux ABI
    and the EABI is that 64-bit types (like long long) are aligned
    differently. In the traditional ABI, these types had 4-byte alignment;
    in the EABI they have 8-byte alignment. As a result, if you use the
    same structure definitions (in a header file) and include it in code
    used in both the kernel and in application code, you may find that the
    structure size and alignment differ."

-- Jamie

Re: [xfs-masters] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Jamie Lokier]

Boaz Harrosh wrote:
> I would at least expect a big fat comment explaining what happened to the
> structure on what known ARCHs, and how it is expected to look in memory.
> And a BUILD_BUG_ON to make sure of that.

You may have a point.

Struct layout on some architectures changes between compiler default
ABIs in these implicit-padding cases.  Kernel binary compatibility
will be affected.  It is a good reason why we have explicit padding to
natural alignment normally.

>From http://wiki.debian.org/ArmEabiPort

    "With the new ABI, default structure packing changes, as do some
    default data sizes and alignment (which also have a knock-on effect on
    structure packing). In particular the minimum size and alignment of a
    structure was 4 bytes. Under the EABI there is no minimum and the
    alignment is determined by the types of the components it
    contains. This will break programs that know too much about the way
    structures are packed and can break code that writes binary files by
    dumping and reading structures."

    "One of the key differences between the traditional GNU/Linux ABI
    and the EABI is that 64-bit types (like long long) are aligned
    differently. In the traditional ABI, these types had 4-byte alignment;
    in the EABI they have 8-byte alignment. As a result, if you use the
    same structure definitions (in a header file) and include it in code
    used in both the kernel and in application code, you may find that the
    structure size and alignment differ."

-- Jamie

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

[Ocfs2-devel] [xfs-masters] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Jamie Lokier]

Boaz Harrosh wrote:
> I would at least expect a big fat comment explaining what happened to the
> structure on what known ARCHs, and how it is expected to look in memory.
> And a BUILD_BUG_ON to make sure of that.

You may have a point.

Struct layout on some architectures changes between compiler default
ABIs in these implicit-padding cases.  Kernel binary compatibility
will be affected.  It is a good reason why we have explicit padding to
natural alignment normally.

From http://wiki.debian.org/ArmEabiPort

    "With the new ABI, default structure packing changes, as do some
    default data sizes and alignment (which also have a knock-on effect on
    structure packing). In particular the minimum size and alignment of a
    structure was 4 bytes. Under the EABI there is no minimum and the
    alignment is determined by the types of the components it
    contains. This will break programs that know too much about the way
    structures are packed and can break code that writes binary files by
    dumping and reading structures."

    "One of the key differences between the traditional GNU/Linux ABI
    and the EABI is that 64-bit types (like long long) are aligned
    differently. In the traditional ABI, these types had 4-byte alignment;
    in the EABI they have 8-byte alignment. As a result, if you use the
    same structure definitions (in a header file) and include it in code
    used in both the kernel and in application code, you may find that the
    structure size and alignment differ."

-- Jamie

Re: [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Christoph Hellwig]

On Thu, Jan 29, 2009 at 02:29:11AM +0530, Ankit Jain wrote:
> Al, Could this be included in the vfs queue?
> 
> This patch adds ioctls to vfs for compatibility with legacy XFS
> pre-allocation ioctls (XFS_IOC_*RESVP*). The implementation
> effectively invokes sys_fallocate for the new ioctls.
> Also handles the compat_ioctl case.
> Note: These legacy ioctls are also implemented by OCFS2.

Still not in, but I remember Al pinged me because it broke on 64bit
architectures due issues in the compat_ioctl support.

Below is a version with that fixed and the ioctl renamed to FS_IOC_*
instead of F_IOC_* to match other generic filesystem ioctls.

--

Subject: fs: Add new pre-allocation ioctls to vfs for compatibility  with legacy xfs ioctls
From: Ankit Jain <me@ankitjain.org>

This patch adds ioctls to vfs for compatibility with legacy XFS
pre-allocation ioctls (XFS_IOC_*RESVP*). The implementation
effectively invokes sys_fallocate for the new ioctls.
Also handles the compat_ioctl case.
Note: These legacy ioctls are also implemented by OCFS2.

Signed-off-by: Ankit Jain <me@ankitjain.org>
Signed-off-by: Christoph Hellwig <hch@lst.de>


Index: linux-2.6/fs/compat_ioctl.c
===================================================================
--- linux-2.6.orig/fs/compat_ioctl.c	2009-06-18 13:23:44.842814582 +0200
+++ linux-2.6/fs/compat_ioctl.c	2009-06-19 10:45:22.061840838 +0200
@@ -31,6 +31,7 @@
 #include <linux/skbuff.h>
 #include <linux/netlink.h>
 #include <linux/vt.h>
+#include <linux/falloc.h>
 #include <linux/fs.h>
 #include <linux/file.h>
 #include <linux/ppp_defs.h>
@@ -1820,6 +1821,41 @@ lp_timeout_trans(unsigned int fd, unsign
 	return sys_ioctl(fd, cmd, (unsigned long)tn);
 }
 
+/* on ia32 l_start is on a 32-bit boundary */
+#if defined(CONFIG_IA64) || defined(CONFIG_X86_64)
+struct space_resv_32 {
+	__s16		l_type;
+	__s16		l_whence;
+	__s64		l_start	__attribute__((packed));
+			/* len == 0 means until end of file */
+	__s64		l_len __attribute__((packed));
+	__s32		l_sysid;
+	__u32		l_pid;
+	__s32		l_pad[4];	/* reserve area */
+};
+
+#define FS_IOC_RESVSP_32		_IOW ('X', 40, struct space_resv_32)
+#define FS_IOC_RESVSP64_32	_IOW ('X', 42, struct space_resv_32)
+
+/* just account for different alignment */
+static int compat_ioctl_preallocate(struct file *file, unsigned long arg)
+{
+	struct space_resv_32	__user *p32 = (void __user *)arg;
+	struct space_resv	__user *p = compat_alloc_user_space(sizeof(*p));
+
+	if (copy_in_user(&p->l_type,	&p32->l_type,	sizeof(s16)) ||
+	    copy_in_user(&p->l_whence,	&p32->l_whence, sizeof(s16)) ||
+	    copy_in_user(&p->l_start,	&p32->l_start,	sizeof(s64)) ||
+	    copy_in_user(&p->l_len,	&p32->l_len,	sizeof(s64)) ||
+	    copy_in_user(&p->l_sysid,	&p32->l_sysid,	sizeof(s32)) ||
+	    copy_in_user(&p->l_pid,	&p32->l_pid,	sizeof(u32)) ||
+	    copy_in_user(&p->l_pad,	&p32->l_pad,	4*sizeof(u32)))
+		return -EFAULT;
+
+	return ioctl_preallocate(file, p);
+}
+#endif
+
 
 typedef int (*ioctl_trans_handler_t)(unsigned int, unsigned int,
 					unsigned long, struct file *);
@@ -2808,6 +2844,18 @@ asmlinkage long compat_sys_ioctl(unsigne
 	case FIOQSIZE:
 		break;
 
+#if defined(CONFIG_IA64) || defined(CONFIG_X86_64)
+	case FS_IOC_RESVSP_32:
+	case FS_IOC_RESVSP64_32:
+		error = compat_ioctl_preallocate(filp, arg);
+		goto out_fput;
+#else
+	case FS_IOC_RESVSP:
+	case FS_IOC_RESVSP64:
+		error = ioctl_preallocate(filp, (void __user *)arg);
+		goto out_fput;
+#endif
+
 	case FIBMAP:
 	case FIGETBSZ:
 	case FIONREAD:
Index: linux-2.6/fs/ioctl.c
===================================================================
--- linux-2.6.orig/fs/ioctl.c	2009-06-18 13:23:44.864813328 +0200
+++ linux-2.6/fs/ioctl.c	2009-06-19 10:45:22.065891285 +0200
@@ -15,6 +15,7 @@
 #include <linux/uaccess.h>
 #include <linux/writeback.h>
 #include <linux/buffer_head.h>
+#include <linux/falloc.h>
 
 #include <asm/ioctls.h>
 
@@ -403,6 +404,37 @@ EXPORT_SYMBOL(generic_block_fiemap);
 
 #endif  /*  CONFIG_BLOCK  */
 
+/*
+ * This provides compatibility with legacy XFS pre-allocation ioctls
+ * which predate the fallocate syscall.
+ *
+ * Only the l_start, l_len and l_whence fields of the 'struct space_resv'
+ * are used here, rest are ignored.
+ */
+int ioctl_preallocate(struct file *filp, void __user *argp)
+{
+	struct inode *inode = filp->f_path.dentry->d_inode;
+	struct space_resv sr;
+
+	if (copy_from_user(&sr, argp, sizeof(sr)))
+		return -EFAULT;
+
+	switch (sr.l_whence) {
+	case SEEK_SET:
+		break;
+	case SEEK_CUR:
+		sr.l_start += filp->f_pos;
+		break;
+	case SEEK_END:
+		sr.l_start += i_size_read(inode);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return do_fallocate(filp, FALLOC_FL_KEEP_SIZE, sr.l_start, sr.l_len);
+}
+
 static int file_ioctl(struct file *filp, unsigned int cmd,
 		unsigned long arg)
 {
@@ -414,6 +446,9 @@ static int file_ioctl(struct file *filp,
 		return ioctl_fibmap(filp, p);
 	case FIONREAD:
 		return put_user(i_size_read(inode) - filp->f_pos, p);
+	case FS_IOC_RESVSP:
+	case FS_IOC_RESVSP64:
+		return ioctl_preallocate(filp, p);
 	}
 
 	return vfs_ioctl(filp, cmd, arg);
Index: linux-2.6/fs/open.c
===================================================================
--- linux-2.6.orig/fs/open.c	2009-06-15 06:52:46.423947558 +0200
+++ linux-2.6/fs/open.c	2009-06-19 10:45:22.071891142 +0200
@@ -378,63 +378,63 @@ SYSCALL_ALIAS(sys_ftruncate64, SyS_ftrun
 #endif
 #endif /* BITS_PER_LONG == 32 */
 
-SYSCALL_DEFINE(fallocate)(int fd, int mode, loff_t offset, loff_t len)
+
+int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 {
-	struct file *file;
-	struct inode *inode;
-	long ret = -EINVAL;
+	struct inode *inode = file->f_path.dentry->d_inode;
+	long ret;
 
 	if (offset < 0 || len <= 0)
-		goto out;
+		return -EINVAL;
 
 	/* Return error if mode is not supported */
-	ret = -EOPNOTSUPP;
 	if (mode && !(mode & FALLOC_FL_KEEP_SIZE))
-		goto out;
+		return -EOPNOTSUPP;
 
-	ret = -EBADF;
-	file = fget(fd);
-	if (!file)
-		goto out;
 	if (!(file->f_mode & FMODE_WRITE))
-		goto out_fput;
+		return -EBADF;
 	/*
 	 * Revalidate the write permissions, in case security policy has
 	 * changed since the files were opened.
 	 */
 	ret = security_file_permission(file, MAY_WRITE);
 	if (ret)
-		goto out_fput;
+		return ret;
 
-	inode = file->f_path.dentry->d_inode;
-
-	ret = -ESPIPE;
 	if (S_ISFIFO(inode->i_mode))
-		goto out_fput;
+		return -ESPIPE;
 
-	ret = -ENODEV;
 	/*
 	 * Let individual file system decide if it supports preallocation
 	 * for directories or not.
 	 */
 	if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
-		goto out_fput;
+		return -ENODEV;
 
-	ret = -EFBIG;
 	/* Check for wrap through zero too */
 	if (((offset + len) > inode->i_sb->s_maxbytes) || ((offset + len) < 0))
-		goto out_fput;
+		return -EFBIG;
 
-	if (inode->i_op->fallocate)
-		ret = inode->i_op->fallocate(inode, mode, offset, len);
-	else
-		ret = -EOPNOTSUPP;
+	if (!inode->i_op->fallocate)
+		return -EOPNOTSUPP;
 
-out_fput:
-	fput(file);
-out:
-	return ret;
+	return inode->i_op->fallocate(inode, mode, offset, len);
 }
+
+SYSCALL_DEFINE(fallocate)(int fd, int mode, loff_t offset, loff_t len)
+{
+	struct file *file;
+	int error = -EBADF;
+
+	file = fget(fd);
+	if (file) {
+		error = do_fallocate(file, mode, offset, len);
+		fput(file);
+	}
+
+	return error;
+}
+
 #ifdef CONFIG_HAVE_SYSCALL_WRAPPERS
 asmlinkage long SyS_fallocate(long fd, long mode, loff_t offset, loff_t len)
 {
Index: linux-2.6/include/linux/falloc.h
===================================================================
--- linux-2.6.orig/include/linux/falloc.h	2009-06-15 06:52:46.427939617 +0200
+++ linux-2.6/include/linux/falloc.h	2009-06-19 10:45:22.077973761 +0200
@@ -3,4 +3,25 @@
 
 #define FALLOC_FL_KEEP_SIZE	0x01 /* default is extend size */
 
+#ifdef __KERNEL__
+
+/*
+ * Space reservation ioctls and argument structure
+ * are designed to be compatible with the legacy XFS ioctls.
+ */
+struct space_resv {
+	__s16		l_type;
+	__s16		l_whence;
+	__s64		l_start;
+	__s64		l_len;		/* len == 0 means until end of file */
+	__s32		l_sysid;
+	__u32		l_pid;
+	__s32		l_pad[4];	/* reserved area */
+};
+
+#define FS_IOC_RESVSP		_IOW('X', 40, struct space_resv)
+#define FS_IOC_RESVSP64		_IOW('X', 42, struct space_resv)
+
+#endif /* __KERNEL__ */
+
 #endif /* _FALLOC_H_ */
Index: linux-2.6/include/linux/fs.h
===================================================================
--- linux-2.6.orig/include/linux/fs.h	2009-06-17 11:49:42.162814358 +0200
+++ linux-2.6/include/linux/fs.h	2009-06-19 10:45:22.081962259 +0200
@@ -1905,6 +1905,8 @@ static inline int break_lease(struct ino
 
 extern int do_truncate(struct dentry *, loff_t start, unsigned int time_attrs,
 		       struct file *filp);
+extern int do_fallocate(struct file *file, int mode, loff_t offset,
+			loff_t len);
 extern long do_sys_open(int dfd, const char __user *filename, int flags,
 			int mode);
 extern struct file *filp_open(const char *, int, int);
@@ -1913,6 +1915,10 @@ extern struct file * dentry_open(struct 
 extern int filp_close(struct file *, fl_owner_t id);
 extern char * getname(const char __user *);
 
+/* fs/ioctl.c */
+
+extern int ioctl_preallocate(struct file *filp, void __user *argp);
+
 /* fs/dcache.c */
 extern void __init vfs_caches_init_early(void);
 extern void __init vfs_caches_init(unsigned long);

Re: [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Christoph Hellwig]

On Thu, Jan 29, 2009 at 02:29:11AM +0530, Ankit Jain wrote:
> Al, Could this be included in the vfs queue?
> 
> This patch adds ioctls to vfs for compatibility with legacy XFS
> pre-allocation ioctls (XFS_IOC_*RESVP*). The implementation
> effectively invokes sys_fallocate for the new ioctls.
> Also handles the compat_ioctl case.
> Note: These legacy ioctls are also implemented by OCFS2.

Still not in, but I remember Al pinged me because it broke on 64bit
architectures due issues in the compat_ioctl support.

Below is a version with that fixed and the ioctl renamed to FS_IOC_*
instead of F_IOC_* to match other generic filesystem ioctls.

--

Subject: fs: Add new pre-allocation ioctls to vfs for compatibility  with legacy xfs ioctls
From: Ankit Jain <me@ankitjain.org>

This patch adds ioctls to vfs for compatibility with legacy XFS
pre-allocation ioctls (XFS_IOC_*RESVP*). The implementation
effectively invokes sys_fallocate for the new ioctls.
Also handles the compat_ioctl case.
Note: These legacy ioctls are also implemented by OCFS2.

Signed-off-by: Ankit Jain <me@ankitjain.org>
Signed-off-by: Christoph Hellwig <hch@lst.de>


Index: linux-2.6/fs/compat_ioctl.c
===================================================================
--- linux-2.6.orig/fs/compat_ioctl.c	2009-06-18 13:23:44.842814582 +0200
+++ linux-2.6/fs/compat_ioctl.c	2009-06-19 10:45:22.061840838 +0200
@@ -31,6 +31,7 @@
 #include <linux/skbuff.h>
 #include <linux/netlink.h>
 #include <linux/vt.h>
+#include <linux/falloc.h>
 #include <linux/fs.h>
 #include <linux/file.h>
 #include <linux/ppp_defs.h>
@@ -1820,6 +1821,41 @@ lp_timeout_trans(unsigned int fd, unsign
 	return sys_ioctl(fd, cmd, (unsigned long)tn);
 }
 
+/* on ia32 l_start is on a 32-bit boundary */
+#if defined(CONFIG_IA64) || defined(CONFIG_X86_64)
+struct space_resv_32 {
+	__s16		l_type;
+	__s16		l_whence;
+	__s64		l_start	__attribute__((packed));
+			/* len == 0 means until end of file */
+	__s64		l_len __attribute__((packed));
+	__s32		l_sysid;
+	__u32		l_pid;
+	__s32		l_pad[4];	/* reserve area */
+};
+
+#define FS_IOC_RESVSP_32		_IOW ('X', 40, struct space_resv_32)
+#define FS_IOC_RESVSP64_32	_IOW ('X', 42, struct space_resv_32)
+
+/* just account for different alignment */
+static int compat_ioctl_preallocate(struct file *file, unsigned long arg)
+{
+	struct space_resv_32	__user *p32 = (void __user *)arg;
+	struct space_resv	__user *p = compat_alloc_user_space(sizeof(*p));
+
+	if (copy_in_user(&p->l_type,	&p32->l_type,	sizeof(s16)) ||
+	    copy_in_user(&p->l_whence,	&p32->l_whence, sizeof(s16)) ||
+	    copy_in_user(&p->l_start,	&p32->l_start,	sizeof(s64)) ||
+	    copy_in_user(&p->l_len,	&p32->l_len,	sizeof(s64)) ||
+	    copy_in_user(&p->l_sysid,	&p32->l_sysid,	sizeof(s32)) ||
+	    copy_in_user(&p->l_pid,	&p32->l_pid,	sizeof(u32)) ||
+	    copy_in_user(&p->l_pad,	&p32->l_pad,	4*sizeof(u32)))
+		return -EFAULT;
+
+	return ioctl_preallocate(file, p);
+}
+#endif
+
 
 typedef int (*ioctl_trans_handler_t)(unsigned int, unsigned int,
 					unsigned long, struct file *);
@@ -2808,6 +2844,18 @@ asmlinkage long compat_sys_ioctl(unsigne
 	case FIOQSIZE:
 		break;
 
+#if defined(CONFIG_IA64) || defined(CONFIG_X86_64)
+	case FS_IOC_RESVSP_32:
+	case FS_IOC_RESVSP64_32:
+		error = compat_ioctl_preallocate(filp, arg);
+		goto out_fput;
+#else
+	case FS_IOC_RESVSP:
+	case FS_IOC_RESVSP64:
+		error = ioctl_preallocate(filp, (void __user *)arg);
+		goto out_fput;
+#endif
+
 	case FIBMAP:
 	case FIGETBSZ:
 	case FIONREAD:
Index: linux-2.6/fs/ioctl.c
===================================================================
--- linux-2.6.orig/fs/ioctl.c	2009-06-18 13:23:44.864813328 +0200
+++ linux-2.6/fs/ioctl.c	2009-06-19 10:45:22.065891285 +0200
@@ -15,6 +15,7 @@
 #include <linux/uaccess.h>
 #include <linux/writeback.h>
 #include <linux/buffer_head.h>
+#include <linux/falloc.h>
 
 #include <asm/ioctls.h>
 
@@ -403,6 +404,37 @@ EXPORT_SYMBOL(generic_block_fiemap);
 
 #endif  /*  CONFIG_BLOCK  */
 
+/*
+ * This provides compatibility with legacy XFS pre-allocation ioctls
+ * which predate the fallocate syscall.
+ *
+ * Only the l_start, l_len and l_whence fields of the 'struct space_resv'
+ * are used here, rest are ignored.
+ */
+int ioctl_preallocate(struct file *filp, void __user *argp)
+{
+	struct inode *inode = filp->f_path.dentry->d_inode;
+	struct space_resv sr;
+
+	if (copy_from_user(&sr, argp, sizeof(sr)))
+		return -EFAULT;
+
+	switch (sr.l_whence) {
+	case SEEK_SET:
+		break;
+	case SEEK_CUR:
+		sr.l_start += filp->f_pos;
+		break;
+	case SEEK_END:
+		sr.l_start += i_size_read(inode);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return do_fallocate(filp, FALLOC_FL_KEEP_SIZE, sr.l_start, sr.l_len);
+}
+
 static int file_ioctl(struct file *filp, unsigned int cmd,
 		unsigned long arg)
 {
@@ -414,6 +446,9 @@ static int file_ioctl(struct file *filp,
 		return ioctl_fibmap(filp, p);
 	case FIONREAD:
 		return put_user(i_size_read(inode) - filp->f_pos, p);
+	case FS_IOC_RESVSP:
+	case FS_IOC_RESVSP64:
+		return ioctl_preallocate(filp, p);
 	}
 
 	return vfs_ioctl(filp, cmd, arg);
Index: linux-2.6/fs/open.c
===================================================================
--- linux-2.6.orig/fs/open.c	2009-06-15 06:52:46.423947558 +0200
+++ linux-2.6/fs/open.c	2009-06-19 10:45:22.071891142 +0200
@@ -378,63 +378,63 @@ SYSCALL_ALIAS(sys_ftruncate64, SyS_ftrun
 #endif
 #endif /* BITS_PER_LONG == 32 */
 
-SYSCALL_DEFINE(fallocate)(int fd, int mode, loff_t offset, loff_t len)
+
+int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 {
-	struct file *file;
-	struct inode *inode;
-	long ret = -EINVAL;
+	struct inode *inode = file->f_path.dentry->d_inode;
+	long ret;
 
 	if (offset < 0 || len <= 0)
-		goto out;
+		return -EINVAL;
 
 	/* Return error if mode is not supported */
-	ret = -EOPNOTSUPP;
 	if (mode && !(mode & FALLOC_FL_KEEP_SIZE))
-		goto out;
+		return -EOPNOTSUPP;
 
-	ret = -EBADF;
-	file = fget(fd);
-	if (!file)
-		goto out;
 	if (!(file->f_mode & FMODE_WRITE))
-		goto out_fput;
+		return -EBADF;
 	/*
 	 * Revalidate the write permissions, in case security policy has
 	 * changed since the files were opened.
 	 */
 	ret = security_file_permission(file, MAY_WRITE);
 	if (ret)
-		goto out_fput;
+		return ret;
 
-	inode = file->f_path.dentry->d_inode;
-
-	ret = -ESPIPE;
 	if (S_ISFIFO(inode->i_mode))
-		goto out_fput;
+		return -ESPIPE;
 
-	ret = -ENODEV;
 	/*
 	 * Let individual file system decide if it supports preallocation
 	 * for directories or not.
 	 */
 	if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
-		goto out_fput;
+		return -ENODEV;
 
-	ret = -EFBIG;
 	/* Check for wrap through zero too */
 	if (((offset + len) > inode->i_sb->s_maxbytes) || ((offset + len) < 0))
-		goto out_fput;
+		return -EFBIG;
 
-	if (inode->i_op->fallocate)
-		ret = inode->i_op->fallocate(inode, mode, offset, len);
-	else
-		ret = -EOPNOTSUPP;
+	if (!inode->i_op->fallocate)
+		return -EOPNOTSUPP;
 
-out_fput:
-	fput(file);
-out:
-	return ret;
+	return inode->i_op->fallocate(inode, mode, offset, len);
 }
+
+SYSCALL_DEFINE(fallocate)(int fd, int mode, loff_t offset, loff_t len)
+{
+	struct file *file;
+	int error = -EBADF;
+
+	file = fget(fd);
+	if (file) {
+		error = do_fallocate(file, mode, offset, len);
+		fput(file);
+	}
+
+	return error;
+}
+
 #ifdef CONFIG_HAVE_SYSCALL_WRAPPERS
 asmlinkage long SyS_fallocate(long fd, long mode, loff_t offset, loff_t len)
 {
Index: linux-2.6/include/linux/falloc.h
===================================================================
--- linux-2.6.orig/include/linux/falloc.h	2009-06-15 06:52:46.427939617 +0200
+++ linux-2.6/include/linux/falloc.h	2009-06-19 10:45:22.077973761 +0200
@@ -3,4 +3,25 @@
 
 #define FALLOC_FL_KEEP_SIZE	0x01 /* default is extend size */
 
+#ifdef __KERNEL__
+
+/*
+ * Space reservation ioctls and argument structure
+ * are designed to be compatible with the legacy XFS ioctls.
+ */
+struct space_resv {
+	__s16		l_type;
+	__s16		l_whence;
+	__s64		l_start;
+	__s64		l_len;		/* len == 0 means until end of file */
+	__s32		l_sysid;
+	__u32		l_pid;
+	__s32		l_pad[4];	/* reserved area */
+};
+
+#define FS_IOC_RESVSP		_IOW('X', 40, struct space_resv)
+#define FS_IOC_RESVSP64		_IOW('X', 42, struct space_resv)
+
+#endif /* __KERNEL__ */
+
 #endif /* _FALLOC_H_ */
Index: linux-2.6/include/linux/fs.h
===================================================================
--- linux-2.6.orig/include/linux/fs.h	2009-06-17 11:49:42.162814358 +0200
+++ linux-2.6/include/linux/fs.h	2009-06-19 10:45:22.081962259 +0200
@@ -1905,6 +1905,8 @@ static inline int break_lease(struct ino
 
 extern int do_truncate(struct dentry *, loff_t start, unsigned int time_attrs,
 		       struct file *filp);
+extern int do_fallocate(struct file *file, int mode, loff_t offset,
+			loff_t len);
 extern long do_sys_open(int dfd, const char __user *filename, int flags,
 			int mode);
 extern struct file *filp_open(const char *, int, int);
@@ -1913,6 +1915,10 @@ extern struct file * dentry_open(struct 
 extern int filp_close(struct file *, fl_owner_t id);
 extern char * getname(const char __user *);
 
+/* fs/ioctl.c */
+
+extern int ioctl_preallocate(struct file *filp, void __user *argp);
+
 /* fs/dcache.c */
 extern void __init vfs_caches_init_early(void);
 extern void __init vfs_caches_init(unsigned long);

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

[Ocfs2-devel] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Christoph Hellwig]

On Thu, Jan 29, 2009 at 02:29:11AM +0530, Ankit Jain wrote:
> Al, Could this be included in the vfs queue?
> 
> This patch adds ioctls to vfs for compatibility with legacy XFS
> pre-allocation ioctls (XFS_IOC_*RESVP*). The implementation
> effectively invokes sys_fallocate for the new ioctls.
> Also handles the compat_ioctl case.
> Note: These legacy ioctls are also implemented by OCFS2.

Still not in, but I remember Al pinged me because it broke on 64bit
architectures due issues in the compat_ioctl support.

Below is a version with that fixed and the ioctl renamed to FS_IOC_*
instead of F_IOC_* to match other generic filesystem ioctls.

--

Subject: fs: Add new pre-allocation ioctls to vfs for compatibility  with legacy xfs ioctls
From: Ankit Jain <me@ankitjain.org>

This patch adds ioctls to vfs for compatibility with legacy XFS
pre-allocation ioctls (XFS_IOC_*RESVP*). The implementation
effectively invokes sys_fallocate for the new ioctls.
Also handles the compat_ioctl case.
Note: These legacy ioctls are also implemented by OCFS2.

Signed-off-by: Ankit Jain <me@ankitjain.org>
Signed-off-by: Christoph Hellwig <hch@lst.de>


Index: linux-2.6/fs/compat_ioctl.c
===================================================================
--- linux-2.6.orig/fs/compat_ioctl.c	2009-06-18 13:23:44.842814582 +0200
+++ linux-2.6/fs/compat_ioctl.c	2009-06-19 10:45:22.061840838 +0200
@@ -31,6 +31,7 @@
 #include <linux/skbuff.h>
 #include <linux/netlink.h>
 #include <linux/vt.h>
+#include <linux/falloc.h>
 #include <linux/fs.h>
 #include <linux/file.h>
 #include <linux/ppp_defs.h>
@@ -1820,6 +1821,41 @@ lp_timeout_trans(unsigned int fd, unsign
 	return sys_ioctl(fd, cmd, (unsigned long)tn);
 }
 
+/* on ia32 l_start is on a 32-bit boundary */
+#if defined(CONFIG_IA64) || defined(CONFIG_X86_64)
+struct space_resv_32 {
+	__s16		l_type;
+	__s16		l_whence;
+	__s64		l_start	__attribute__((packed));
+			/* len == 0 means until end of file */
+	__s64		l_len __attribute__((packed));
+	__s32		l_sysid;
+	__u32		l_pid;
+	__s32		l_pad[4];	/* reserve area */
+};
+
+#define FS_IOC_RESVSP_32		_IOW ('X', 40, struct space_resv_32)
+#define FS_IOC_RESVSP64_32	_IOW ('X', 42, struct space_resv_32)
+
+/* just account for different alignment */
+static int compat_ioctl_preallocate(struct file *file, unsigned long arg)
+{
+	struct space_resv_32	__user *p32 = (void __user *)arg;
+	struct space_resv	__user *p = compat_alloc_user_space(sizeof(*p));
+
+	if (copy_in_user(&p->l_type,	&p32->l_type,	sizeof(s16)) ||
+	    copy_in_user(&p->l_whence,	&p32->l_whence, sizeof(s16)) ||
+	    copy_in_user(&p->l_start,	&p32->l_start,	sizeof(s64)) ||
+	    copy_in_user(&p->l_len,	&p32->l_len,	sizeof(s64)) ||
+	    copy_in_user(&p->l_sysid,	&p32->l_sysid,	sizeof(s32)) ||
+	    copy_in_user(&p->l_pid,	&p32->l_pid,	sizeof(u32)) ||
+	    copy_in_user(&p->l_pad,	&p32->l_pad,	4*sizeof(u32)))
+		return -EFAULT;
+
+	return ioctl_preallocate(file, p);
+}
+#endif
+
 
 typedef int (*ioctl_trans_handler_t)(unsigned int, unsigned int,
 					unsigned long, struct file *);
@@ -2808,6 +2844,18 @@ asmlinkage long compat_sys_ioctl(unsigne
 	case FIOQSIZE:
 		break;
 
+#if defined(CONFIG_IA64) || defined(CONFIG_X86_64)
+	case FS_IOC_RESVSP_32:
+	case FS_IOC_RESVSP64_32:
+		error = compat_ioctl_preallocate(filp, arg);
+		goto out_fput;
+#else
+	case FS_IOC_RESVSP:
+	case FS_IOC_RESVSP64:
+		error = ioctl_preallocate(filp, (void __user *)arg);
+		goto out_fput;
+#endif
+
 	case FIBMAP:
 	case FIGETBSZ:
 	case FIONREAD:
Index: linux-2.6/fs/ioctl.c
===================================================================
--- linux-2.6.orig/fs/ioctl.c	2009-06-18 13:23:44.864813328 +0200
+++ linux-2.6/fs/ioctl.c	2009-06-19 10:45:22.065891285 +0200
@@ -15,6 +15,7 @@
 #include <linux/uaccess.h>
 #include <linux/writeback.h>
 #include <linux/buffer_head.h>
+#include <linux/falloc.h>
 
 #include <asm/ioctls.h>
 
@@ -403,6 +404,37 @@ EXPORT_SYMBOL(generic_block_fiemap);
 
 #endif  /*  CONFIG_BLOCK  */
 
+/*
+ * This provides compatibility with legacy XFS pre-allocation ioctls
+ * which predate the fallocate syscall.
+ *
+ * Only the l_start, l_len and l_whence fields of the 'struct space_resv'
+ * are used here, rest are ignored.
+ */
+int ioctl_preallocate(struct file *filp, void __user *argp)
+{
+	struct inode *inode = filp->f_path.dentry->d_inode;
+	struct space_resv sr;
+
+	if (copy_from_user(&sr, argp, sizeof(sr)))
+		return -EFAULT;
+
+	switch (sr.l_whence) {
+	case SEEK_SET:
+		break;
+	case SEEK_CUR:
+		sr.l_start += filp->f_pos;
+		break;
+	case SEEK_END:
+		sr.l_start += i_size_read(inode);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return do_fallocate(filp, FALLOC_FL_KEEP_SIZE, sr.l_start, sr.l_len);
+}
+
 static int file_ioctl(struct file *filp, unsigned int cmd,
 		unsigned long arg)
 {
@@ -414,6 +446,9 @@ static int file_ioctl(struct file *filp,
 		return ioctl_fibmap(filp, p);
 	case FIONREAD:
 		return put_user(i_size_read(inode) - filp->f_pos, p);
+	case FS_IOC_RESVSP:
+	case FS_IOC_RESVSP64:
+		return ioctl_preallocate(filp, p);
 	}
 
 	return vfs_ioctl(filp, cmd, arg);
Index: linux-2.6/fs/open.c
===================================================================
--- linux-2.6.orig/fs/open.c	2009-06-15 06:52:46.423947558 +0200
+++ linux-2.6/fs/open.c	2009-06-19 10:45:22.071891142 +0200
@@ -378,63 +378,63 @@ SYSCALL_ALIAS(sys_ftruncate64, SyS_ftrun
 #endif
 #endif /* BITS_PER_LONG == 32 */
 
-SYSCALL_DEFINE(fallocate)(int fd, int mode, loff_t offset, loff_t len)
+
+int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 {
-	struct file *file;
-	struct inode *inode;
-	long ret = -EINVAL;
+	struct inode *inode = file->f_path.dentry->d_inode;
+	long ret;
 
 	if (offset < 0 || len <= 0)
-		goto out;
+		return -EINVAL;
 
 	/* Return error if mode is not supported */
-	ret = -EOPNOTSUPP;
 	if (mode && !(mode & FALLOC_FL_KEEP_SIZE))
-		goto out;
+		return -EOPNOTSUPP;
 
-	ret = -EBADF;
-	file = fget(fd);
-	if (!file)
-		goto out;
 	if (!(file->f_mode & FMODE_WRITE))
-		goto out_fput;
+		return -EBADF;
 	/*
 	 * Revalidate the write permissions, in case security policy has
 	 * changed since the files were opened.
 	 */
 	ret = security_file_permission(file, MAY_WRITE);
 	if (ret)
-		goto out_fput;
+		return ret;
 
-	inode = file->f_path.dentry->d_inode;
-
-	ret = -ESPIPE;
 	if (S_ISFIFO(inode->i_mode))
-		goto out_fput;
+		return -ESPIPE;
 
-	ret = -ENODEV;
 	/*
 	 * Let individual file system decide if it supports preallocation
 	 * for directories or not.
 	 */
 	if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
-		goto out_fput;
+		return -ENODEV;
 
-	ret = -EFBIG;
 	/* Check for wrap through zero too */
 	if (((offset + len) > inode->i_sb->s_maxbytes) || ((offset + len) < 0))
-		goto out_fput;
+		return -EFBIG;
 
-	if (inode->i_op->fallocate)
-		ret = inode->i_op->fallocate(inode, mode, offset, len);
-	else
-		ret = -EOPNOTSUPP;
+	if (!inode->i_op->fallocate)
+		return -EOPNOTSUPP;
 
-out_fput:
-	fput(file);
-out:
-	return ret;
+	return inode->i_op->fallocate(inode, mode, offset, len);
 }
+
+SYSCALL_DEFINE(fallocate)(int fd, int mode, loff_t offset, loff_t len)
+{
+	struct file *file;
+	int error = -EBADF;
+
+	file = fget(fd);
+	if (file) {
+		error = do_fallocate(file, mode, offset, len);
+		fput(file);
+	}
+
+	return error;
+}
+
 #ifdef CONFIG_HAVE_SYSCALL_WRAPPERS
 asmlinkage long SyS_fallocate(long fd, long mode, loff_t offset, loff_t len)
 {
Index: linux-2.6/include/linux/falloc.h
===================================================================
--- linux-2.6.orig/include/linux/falloc.h	2009-06-15 06:52:46.427939617 +0200
+++ linux-2.6/include/linux/falloc.h	2009-06-19 10:45:22.077973761 +0200
@@ -3,4 +3,25 @@
 
 #define FALLOC_FL_KEEP_SIZE	0x01 /* default is extend size */
 
+#ifdef __KERNEL__
+
+/*
+ * Space reservation ioctls and argument structure
+ * are designed to be compatible with the legacy XFS ioctls.
+ */
+struct space_resv {
+	__s16		l_type;
+	__s16		l_whence;
+	__s64		l_start;
+	__s64		l_len;		/* len == 0 means until end of file */
+	__s32		l_sysid;
+	__u32		l_pid;
+	__s32		l_pad[4];	/* reserved area */
+};
+
+#define FS_IOC_RESVSP		_IOW('X', 40, struct space_resv)
+#define FS_IOC_RESVSP64		_IOW('X', 42, struct space_resv)
+
+#endif /* __KERNEL__ */
+
 #endif /* _FALLOC_H_ */
Index: linux-2.6/include/linux/fs.h
===================================================================
--- linux-2.6.orig/include/linux/fs.h	2009-06-17 11:49:42.162814358 +0200
+++ linux-2.6/include/linux/fs.h	2009-06-19 10:45:22.081962259 +0200
@@ -1905,6 +1905,8 @@ static inline int break_lease(struct ino
 
 extern int do_truncate(struct dentry *, loff_t start, unsigned int time_attrs,
 		       struct file *filp);
+extern int do_fallocate(struct file *file, int mode, loff_t offset,
+			loff_t len);
 extern long do_sys_open(int dfd, const char __user *filename, int flags,
 			int mode);
 extern struct file *filp_open(const char *, int, int);
@@ -1913,6 +1915,10 @@ extern struct file * dentry_open(struct 
 extern int filp_close(struct file *, fl_owner_t id);
 extern char * getname(const char __user *);
 
+/* fs/ioctl.c */
+
+extern int ioctl_preallocate(struct file *filp, void __user *argp);
+
 /* fs/dcache.c */
 extern void __init vfs_caches_init_early(void);
 extern void __init vfs_caches_init(unsigned long);

Re: [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Arnd Bergmann]

On Friday 19 June 2009 06:28:07 pm Christoph Hellwig wrote:

> +/* on ia32 l_start is on a 32-bit boundary */
> +#if defined(CONFIG_IA64) || defined(CONFIG_X86_64)
> +struct space_resv_32 {
> +	__s16		l_type;
> +	__s16		l_whence;
> +	__s64		l_start	__attribute__((packed));
> +			/* len == 0 means until end of file */
> +	__s64		l_len __attribute__((packed));
> +	__s32		l_sysid;
> +	__u32		l_pid;
> +	__s32		l_pad[4];	/* reserve area */
> +};
> +
> +#define FS_IOC_RESVSP_32		_IOW ('X', 40, struct space_resv_32)
> +#define FS_IOC_RESVSP64_32	_IOW ('X', 42, struct space_resv_32)

I'd just define this using compat_s64 instead of __s64 __packed so we can
use the same code on all architectures, even at the small cost of extra
text size on non-x86 architectures.

> +/* just account for different alignment */
> +static int compat_ioctl_preallocate(struct file *file, unsigned long arg)
> +{
> +	struct space_resv_32	__user *p32 = (void __user *)arg;
> +	struct space_resv	__user *p = compat_alloc_user_space(sizeof(*p));
> +
> +	if (copy_in_user(&p->l_type,	&p32->l_type,	sizeof(s16)) ||
> +	    copy_in_user(&p->l_whence,	&p32->l_whence, sizeof(s16)) ||
> +	    copy_in_user(&p->l_start,	&p32->l_start,	sizeof(s64)) ||
> +	    copy_in_user(&p->l_len,	&p32->l_len,	sizeof(s64)) ||
> +	    copy_in_user(&p->l_sysid,	&p32->l_sysid,	sizeof(s32)) ||
> +	    copy_in_user(&p->l_pid,	&p32->l_pid,	sizeof(u32)) ||
> +	    copy_in_user(&p->l_pad,	&p32->l_pad,	4*sizeof(u32)))
> +		return -EFAULT;
> +
> +	return ioctl_preallocate(file, p);
> +}
> +#endif
> +

Here, you can call do_fallocate directly in the same way that ioctl_preallocate
does, replacing the copy_in_user calls with __get_user().

	Arnd <><

Re: [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Arnd Bergmann]

On Friday 19 June 2009 06:28:07 pm Christoph Hellwig wrote:

> +/* on ia32 l_start is on a 32-bit boundary */
> +#if defined(CONFIG_IA64) || defined(CONFIG_X86_64)
> +struct space_resv_32 {
> +	__s16		l_type;
> +	__s16		l_whence;
> +	__s64		l_start	__attribute__((packed));
> +			/* len == 0 means until end of file */
> +	__s64		l_len __attribute__((packed));
> +	__s32		l_sysid;
> +	__u32		l_pid;
> +	__s32		l_pad[4];	/* reserve area */
> +};
> +
> +#define FS_IOC_RESVSP_32		_IOW ('X', 40, struct space_resv_32)
> +#define FS_IOC_RESVSP64_32	_IOW ('X', 42, struct space_resv_32)

I'd just define this using compat_s64 instead of __s64 __packed so we can
use the same code on all architectures, even at the small cost of extra
text size on non-x86 architectures.

> +/* just account for different alignment */
> +static int compat_ioctl_preallocate(struct file *file, unsigned long arg)
> +{
> +	struct space_resv_32	__user *p32 = (void __user *)arg;
> +	struct space_resv	__user *p = compat_alloc_user_space(sizeof(*p));
> +
> +	if (copy_in_user(&p->l_type,	&p32->l_type,	sizeof(s16)) ||
> +	    copy_in_user(&p->l_whence,	&p32->l_whence, sizeof(s16)) ||
> +	    copy_in_user(&p->l_start,	&p32->l_start,	sizeof(s64)) ||
> +	    copy_in_user(&p->l_len,	&p32->l_len,	sizeof(s64)) ||
> +	    copy_in_user(&p->l_sysid,	&p32->l_sysid,	sizeof(s32)) ||
> +	    copy_in_user(&p->l_pid,	&p32->l_pid,	sizeof(u32)) ||
> +	    copy_in_user(&p->l_pad,	&p32->l_pad,	4*sizeof(u32)))
> +		return -EFAULT;
> +
> +	return ioctl_preallocate(file, p);
> +}
> +#endif
> +

Here, you can call do_fallocate directly in the same way that ioctl_preallocate
does, replacing the copy_in_user calls with __get_user().

	Arnd <><

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

Re: [xfs-masters] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Christoph Hellwig]

On Sat, Jun 20, 2009 at 08:13:59AM +0000, Arnd Bergmann wrote:
> I'd just define this using compat_s64 instead of __s64 __packed so we can
> use the same code on all architectures, even at the small cost of extra
> text size on non-x86 architectures.

I'm not a big fan of imposing the translation overhead to all
architectures, but doing it allows using the good old HANDLE_IOCTL
and futher simplifying things.



Index: linux-2.6/fs/compat_ioctl.c
===================================================================
--- linux-2.6.orig/fs/compat_ioctl.c	2009-06-20 20:19:47.035930201 +0200
+++ linux-2.6/fs/compat_ioctl.c	2009-06-20 20:20:43.821833210 +0200
@@ -31,6 +31,7 @@
 #include <linux/skbuff.h>
 #include <linux/netlink.h>
 #include <linux/vt.h>
+#include <linux/falloc.h>
 #include <linux/fs.h>
 #include <linux/file.h>
 #include <linux/ppp_defs.h>
@@ -1820,6 +1821,38 @@ lp_timeout_trans(unsigned int fd, unsign
 	return sys_ioctl(fd, cmd, (unsigned long)tn);
 }
 
+/* on ia32 l_start is on a 32-bit boundary */
+struct space_resv_32 {
+	__s16		l_type;
+	__s16		l_whence;
+	compat_s64	l_start;
+	compat_s64	l_len;		/* len == 0 means until end of file */
+	__s32		l_sysid;
+	__u32		l_pid;
+	__s32		l_pad[4];	/* reserve area */
+};
+
+#define FS_IOC_RESVSP_32		_IOW ('X', 40, struct space_resv_32)
+#define FS_IOC_RESVSP64_32	_IOW ('X', 42, struct space_resv_32)
+
+/* just account for different alignment */
+static int compat_ioctl_resvsp(unsigned fd, unsigned cmd, unsigned long arg)
+{
+	struct space_resv_32	__user *p32 = (void __user *)arg;
+	struct space_resv	__user *p = compat_alloc_user_space(sizeof(*p));
+
+	if (copy_in_user(&p->l_type,	&p32->l_type,	sizeof(s16)) ||
+	    copy_in_user(&p->l_whence,	&p32->l_whence, sizeof(s16)) ||
+	    copy_in_user(&p->l_start,	&p32->l_start,	sizeof(s64)) ||
+	    copy_in_user(&p->l_len,	&p32->l_len,	sizeof(s64)) ||
+	    copy_in_user(&p->l_sysid,	&p32->l_sysid,	sizeof(s32)) ||
+	    copy_in_user(&p->l_pid,	&p32->l_pid,	sizeof(u32)) ||
+	    copy_in_user(&p->l_pad,	&p32->l_pad,	4*sizeof(u32)))
+		return -EFAULT;
+
+	return sys_ioctl(fd, FS_IOC_RESVSP, p);
+}
+
 
 typedef int (*ioctl_trans_handler_t)(unsigned int, unsigned int,
 					unsigned long, struct file *);
@@ -1915,6 +1948,8 @@ COMPATIBLE_IOCTL(FIGETBSZ)
 /* 'X' - originally XFS but some now in the VFS */
 COMPATIBLE_IOCTL(FIFREEZE)
 COMPATIBLE_IOCTL(FITHAW)
+HANDLE_IOCTL(FS_IOC_RESVSP_32, compat_ioctl_resvsp)
+HANDLE_IOCTL(FS_IOC_RESVSP64_32, compat_ioctl_resvsp)
 /* RAID */
 COMPATIBLE_IOCTL(RAID_VERSION)
 COMPATIBLE_IOCTL(GET_ARRAY_INFO)
Index: linux-2.6/fs/ioctl.c
===================================================================
--- linux-2.6.orig/fs/ioctl.c	2009-06-20 20:19:47.040930349 +0200
+++ linux-2.6/fs/ioctl.c	2009-06-20 20:20:43.821833210 +0200
@@ -15,6 +15,7 @@
 #include <linux/uaccess.h>
 #include <linux/writeback.h>
 #include <linux/buffer_head.h>
+#include <linux/falloc.h>
 
 #include <asm/ioctls.h>
 
@@ -403,6 +404,37 @@ EXPORT_SYMBOL(generic_block_fiemap);
 
 #endif  /*  CONFIG_BLOCK  */
 
+/*
+ * This provides compatibility with legacy XFS pre-allocation ioctls
+ * which predate the fallocate syscall.
+ *
+ * Only the l_start, l_len and l_whence fields of the 'struct space_resv'
+ * are used here, rest are ignored.
+ */
+static int ioctl_resvsp(struct file *filp, void __user *argp)
+{
+	struct inode *inode = filp->f_path.dentry->d_inode;
+	struct space_resv sr;
+
+	if (copy_from_user(&sr, argp, sizeof(sr)))
+		return -EFAULT;
+
+	switch (sr.l_whence) {
+	case SEEK_SET:
+		break;
+	case SEEK_CUR:
+		sr.l_start += filp->f_pos;
+		break;
+	case SEEK_END:
+		sr.l_start += i_size_read(inode);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return do_fallocate(filp, FALLOC_FL_KEEP_SIZE, sr.l_start, sr.l_len);
+}
+
 static int file_ioctl(struct file *filp, unsigned int cmd,
 		unsigned long arg)
 {
@@ -414,6 +446,9 @@ static int file_ioctl(struct file *filp,
 		return ioctl_fibmap(filp, p);
 	case FIONREAD:
 		return put_user(i_size_read(inode) - filp->f_pos, p);
+	case FS_IOC_RESVSP:
+	case FS_IOC_RESVSP64:
+		return ioctl_resvsp(filp, p);
 	}
 
 	return vfs_ioctl(filp, cmd, arg);
Index: linux-2.6/fs/open.c
===================================================================
--- linux-2.6.orig/fs/open.c	2009-06-20 20:19:47.054929896 +0200
+++ linux-2.6/fs/open.c	2009-06-20 20:20:43.822836760 +0200
@@ -378,63 +378,63 @@ SYSCALL_ALIAS(sys_ftruncate64, SyS_ftrun
 #endif
 #endif /* BITS_PER_LONG == 32 */
 
-SYSCALL_DEFINE(fallocate)(int fd, int mode, loff_t offset, loff_t len)
+
+int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 {
-	struct file *file;
-	struct inode *inode;
-	long ret = -EINVAL;
+	struct inode *inode = file->f_path.dentry->d_inode;
+	long ret;
 
 	if (offset < 0 || len <= 0)
-		goto out;
+		return -EINVAL;
 
 	/* Return error if mode is not supported */
-	ret = -EOPNOTSUPP;
 	if (mode && !(mode & FALLOC_FL_KEEP_SIZE))
-		goto out;
+		return -EOPNOTSUPP;
 
-	ret = -EBADF;
-	file = fget(fd);
-	if (!file)
-		goto out;
 	if (!(file->f_mode & FMODE_WRITE))
-		goto out_fput;
+		return -EBADF;
 	/*
 	 * Revalidate the write permissions, in case security policy has
 	 * changed since the files were opened.
 	 */
 	ret = security_file_permission(file, MAY_WRITE);
 	if (ret)
-		goto out_fput;
+		return ret;
 
-	inode = file->f_path.dentry->d_inode;
-
-	ret = -ESPIPE;
 	if (S_ISFIFO(inode->i_mode))
-		goto out_fput;
+		return -ESPIPE;
 
-	ret = -ENODEV;
 	/*
 	 * Let individual file system decide if it supports preallocation
 	 * for directories or not.
 	 */
 	if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
-		goto out_fput;
+		return -ENODEV;
 
-	ret = -EFBIG;
 	/* Check for wrap through zero too */
 	if (((offset + len) > inode->i_sb->s_maxbytes) || ((offset + len) < 0))
-		goto out_fput;
+		return -EFBIG;
 
-	if (inode->i_op->fallocate)
-		ret = inode->i_op->fallocate(inode, mode, offset, len);
-	else
-		ret = -EOPNOTSUPP;
+	if (!inode->i_op->fallocate)
+		return -EOPNOTSUPP;
 
-out_fput:
-	fput(file);
-out:
-	return ret;
+	return inode->i_op->fallocate(inode, mode, offset, len);
 }
+
+SYSCALL_DEFINE(fallocate)(int fd, int mode, loff_t offset, loff_t len)
+{
+	struct file *file;
+	int error = -EBADF;
+
+	file = fget(fd);
+	if (file) {
+		error = do_fallocate(file, mode, offset, len);
+		fput(file);
+	}
+
+	return error;
+}
+
 #ifdef CONFIG_HAVE_SYSCALL_WRAPPERS
 asmlinkage long SyS_fallocate(long fd, long mode, loff_t offset, loff_t len)
 {
Index: linux-2.6/include/linux/falloc.h
===================================================================
--- linux-2.6.orig/include/linux/falloc.h	2009-06-20 20:19:47.059930323 +0200
+++ linux-2.6/include/linux/falloc.h	2009-06-20 20:20:43.822836760 +0200
@@ -3,4 +3,25 @@
 
 #define FALLOC_FL_KEEP_SIZE	0x01 /* default is extend size */
 
+#ifdef __KERNEL__
+
+/*
+ * Space reservation ioctls and argument structure
+ * are designed to be compatible with the legacy XFS ioctls.
+ */
+struct space_resv {
+	__s16		l_type;
+	__s16		l_whence;
+	__s64		l_start;
+	__s64		l_len;		/* len == 0 means until end of file */
+	__s32		l_sysid;
+	__u32		l_pid;
+	__s32		l_pad[4];	/* reserved area */
+};
+
+#define FS_IOC_RESVSP		_IOW('X', 40, struct space_resv)
+#define FS_IOC_RESVSP64		_IOW('X', 42, struct space_resv)
+
+#endif /* __KERNEL__ */
+
 #endif /* _FALLOC_H_ */
Index: linux-2.6/include/linux/fs.h
===================================================================
--- linux-2.6.orig/include/linux/fs.h	2009-06-20 20:19:47.065929969 +0200
+++ linux-2.6/include/linux/fs.h	2009-06-20 20:20:43.823832557 +0200
@@ -1905,6 +1905,8 @@ static inline int break_lease(struct ino
 
 extern int do_truncate(struct dentry *, loff_t start, unsigned int time_attrs,
 		       struct file *filp);
+extern int do_fallocate(struct file *file, int mode, loff_t offset,
+			loff_t len);
 extern long do_sys_open(int dfd, const char __user *filename, int flags,
 			int mode);
 extern struct file *filp_open(const char *, int, int);

Re: [xfs-masters] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Christoph Hellwig]

On Sat, Jun 20, 2009 at 08:13:59AM +0000, Arnd Bergmann wrote:
> I'd just define this using compat_s64 instead of __s64 __packed so we can
> use the same code on all architectures, even at the small cost of extra
> text size on non-x86 architectures.

I'm not a big fan of imposing the translation overhead to all
architectures, but doing it allows using the good old HANDLE_IOCTL
and futher simplifying things.



Index: linux-2.6/fs/compat_ioctl.c
===================================================================
--- linux-2.6.orig/fs/compat_ioctl.c	2009-06-20 20:19:47.035930201 +0200
+++ linux-2.6/fs/compat_ioctl.c	2009-06-20 20:20:43.821833210 +0200
@@ -31,6 +31,7 @@
 #include <linux/skbuff.h>
 #include <linux/netlink.h>
 #include <linux/vt.h>
+#include <linux/falloc.h>
 #include <linux/fs.h>
 #include <linux/file.h>
 #include <linux/ppp_defs.h>
@@ -1820,6 +1821,38 @@ lp_timeout_trans(unsigned int fd, unsign
 	return sys_ioctl(fd, cmd, (unsigned long)tn);
 }
 
+/* on ia32 l_start is on a 32-bit boundary */
+struct space_resv_32 {
+	__s16		l_type;
+	__s16		l_whence;
+	compat_s64	l_start;
+	compat_s64	l_len;		/* len == 0 means until end of file */
+	__s32		l_sysid;
+	__u32		l_pid;
+	__s32		l_pad[4];	/* reserve area */
+};
+
+#define FS_IOC_RESVSP_32		_IOW ('X', 40, struct space_resv_32)
+#define FS_IOC_RESVSP64_32	_IOW ('X', 42, struct space_resv_32)
+
+/* just account for different alignment */
+static int compat_ioctl_resvsp(unsigned fd, unsigned cmd, unsigned long arg)
+{
+	struct space_resv_32	__user *p32 = (void __user *)arg;
+	struct space_resv	__user *p = compat_alloc_user_space(sizeof(*p));
+
+	if (copy_in_user(&p->l_type,	&p32->l_type,	sizeof(s16)) ||
+	    copy_in_user(&p->l_whence,	&p32->l_whence, sizeof(s16)) ||
+	    copy_in_user(&p->l_start,	&p32->l_start,	sizeof(s64)) ||
+	    copy_in_user(&p->l_len,	&p32->l_len,	sizeof(s64)) ||
+	    copy_in_user(&p->l_sysid,	&p32->l_sysid,	sizeof(s32)) ||
+	    copy_in_user(&p->l_pid,	&p32->l_pid,	sizeof(u32)) ||
+	    copy_in_user(&p->l_pad,	&p32->l_pad,	4*sizeof(u32)))
+		return -EFAULT;
+
+	return sys_ioctl(fd, FS_IOC_RESVSP, p);
+}
+
 
 typedef int (*ioctl_trans_handler_t)(unsigned int, unsigned int,
 					unsigned long, struct file *);
@@ -1915,6 +1948,8 @@ COMPATIBLE_IOCTL(FIGETBSZ)
 /* 'X' - originally XFS but some now in the VFS */
 COMPATIBLE_IOCTL(FIFREEZE)
 COMPATIBLE_IOCTL(FITHAW)
+HANDLE_IOCTL(FS_IOC_RESVSP_32, compat_ioctl_resvsp)
+HANDLE_IOCTL(FS_IOC_RESVSP64_32, compat_ioctl_resvsp)
 /* RAID */
 COMPATIBLE_IOCTL(RAID_VERSION)
 COMPATIBLE_IOCTL(GET_ARRAY_INFO)
Index: linux-2.6/fs/ioctl.c
===================================================================
--- linux-2.6.orig/fs/ioctl.c	2009-06-20 20:19:47.040930349 +0200
+++ linux-2.6/fs/ioctl.c	2009-06-20 20:20:43.821833210 +0200
@@ -15,6 +15,7 @@
 #include <linux/uaccess.h>
 #include <linux/writeback.h>
 #include <linux/buffer_head.h>
+#include <linux/falloc.h>
 
 #include <asm/ioctls.h>
 
@@ -403,6 +404,37 @@ EXPORT_SYMBOL(generic_block_fiemap);
 
 #endif  /*  CONFIG_BLOCK  */
 
+/*
+ * This provides compatibility with legacy XFS pre-allocation ioctls
+ * which predate the fallocate syscall.
+ *
+ * Only the l_start, l_len and l_whence fields of the 'struct space_resv'
+ * are used here, rest are ignored.
+ */
+static int ioctl_resvsp(struct file *filp, void __user *argp)
+{
+	struct inode *inode = filp->f_path.dentry->d_inode;
+	struct space_resv sr;
+
+	if (copy_from_user(&sr, argp, sizeof(sr)))
+		return -EFAULT;
+
+	switch (sr.l_whence) {
+	case SEEK_SET:
+		break;
+	case SEEK_CUR:
+		sr.l_start += filp->f_pos;
+		break;
+	case SEEK_END:
+		sr.l_start += i_size_read(inode);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return do_fallocate(filp, FALLOC_FL_KEEP_SIZE, sr.l_start, sr.l_len);
+}
+
 static int file_ioctl(struct file *filp, unsigned int cmd,
 		unsigned long arg)
 {
@@ -414,6 +446,9 @@ static int file_ioctl(struct file *filp,
 		return ioctl_fibmap(filp, p);
 	case FIONREAD:
 		return put_user(i_size_read(inode) - filp->f_pos, p);
+	case FS_IOC_RESVSP:
+	case FS_IOC_RESVSP64:
+		return ioctl_resvsp(filp, p);
 	}
 
 	return vfs_ioctl(filp, cmd, arg);
Index: linux-2.6/fs/open.c
===================================================================
--- linux-2.6.orig/fs/open.c	2009-06-20 20:19:47.054929896 +0200
+++ linux-2.6/fs/open.c	2009-06-20 20:20:43.822836760 +0200
@@ -378,63 +378,63 @@ SYSCALL_ALIAS(sys_ftruncate64, SyS_ftrun
 #endif
 #endif /* BITS_PER_LONG == 32 */
 
-SYSCALL_DEFINE(fallocate)(int fd, int mode, loff_t offset, loff_t len)
+
+int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 {
-	struct file *file;
-	struct inode *inode;
-	long ret = -EINVAL;
+	struct inode *inode = file->f_path.dentry->d_inode;
+	long ret;
 
 	if (offset < 0 || len <= 0)
-		goto out;
+		return -EINVAL;
 
 	/* Return error if mode is not supported */
-	ret = -EOPNOTSUPP;
 	if (mode && !(mode & FALLOC_FL_KEEP_SIZE))
-		goto out;
+		return -EOPNOTSUPP;
 
-	ret = -EBADF;
-	file = fget(fd);
-	if (!file)
-		goto out;
 	if (!(file->f_mode & FMODE_WRITE))
-		goto out_fput;
+		return -EBADF;
 	/*
 	 * Revalidate the write permissions, in case security policy has
 	 * changed since the files were opened.
 	 */
 	ret = security_file_permission(file, MAY_WRITE);
 	if (ret)
-		goto out_fput;
+		return ret;
 
-	inode = file->f_path.dentry->d_inode;
-
-	ret = -ESPIPE;
 	if (S_ISFIFO(inode->i_mode))
-		goto out_fput;
+		return -ESPIPE;
 
-	ret = -ENODEV;
 	/*
 	 * Let individual file system decide if it supports preallocation
 	 * for directories or not.
 	 */
 	if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
-		goto out_fput;
+		return -ENODEV;
 
-	ret = -EFBIG;
 	/* Check for wrap through zero too */
 	if (((offset + len) > inode->i_sb->s_maxbytes) || ((offset + len) < 0))
-		goto out_fput;
+		return -EFBIG;
 
-	if (inode->i_op->fallocate)
-		ret = inode->i_op->fallocate(inode, mode, offset, len);
-	else
-		ret = -EOPNOTSUPP;
+	if (!inode->i_op->fallocate)
+		return -EOPNOTSUPP;
 
-out_fput:
-	fput(file);
-out:
-	return ret;
+	return inode->i_op->fallocate(inode, mode, offset, len);
 }
+
+SYSCALL_DEFINE(fallocate)(int fd, int mode, loff_t offset, loff_t len)
+{
+	struct file *file;
+	int error = -EBADF;
+
+	file = fget(fd);
+	if (file) {
+		error = do_fallocate(file, mode, offset, len);
+		fput(file);
+	}
+
+	return error;
+}
+
 #ifdef CONFIG_HAVE_SYSCALL_WRAPPERS
 asmlinkage long SyS_fallocate(long fd, long mode, loff_t offset, loff_t len)
 {
Index: linux-2.6/include/linux/falloc.h
===================================================================
--- linux-2.6.orig/include/linux/falloc.h	2009-06-20 20:19:47.059930323 +0200
+++ linux-2.6/include/linux/falloc.h	2009-06-20 20:20:43.822836760 +0200
@@ -3,4 +3,25 @@
 
 #define FALLOC_FL_KEEP_SIZE	0x01 /* default is extend size */
 
+#ifdef __KERNEL__
+
+/*
+ * Space reservation ioctls and argument structure
+ * are designed to be compatible with the legacy XFS ioctls.
+ */
+struct space_resv {
+	__s16		l_type;
+	__s16		l_whence;
+	__s64		l_start;
+	__s64		l_len;		/* len == 0 means until end of file */
+	__s32		l_sysid;
+	__u32		l_pid;
+	__s32		l_pad[4];	/* reserved area */
+};
+
+#define FS_IOC_RESVSP		_IOW('X', 40, struct space_resv)
+#define FS_IOC_RESVSP64		_IOW('X', 42, struct space_resv)
+
+#endif /* __KERNEL__ */
+
 #endif /* _FALLOC_H_ */
Index: linux-2.6/include/linux/fs.h
===================================================================
--- linux-2.6.orig/include/linux/fs.h	2009-06-20 20:19:47.065929969 +0200
+++ linux-2.6/include/linux/fs.h	2009-06-20 20:20:43.823832557 +0200
@@ -1905,6 +1905,8 @@ static inline int break_lease(struct ino
 
 extern int do_truncate(struct dentry *, loff_t start, unsigned int time_attrs,
 		       struct file *filp);
+extern int do_fallocate(struct file *file, int mode, loff_t offset,
+			loff_t len);
 extern long do_sys_open(int dfd, const char __user *filename, int flags,
 			int mode);
 extern struct file *filp_open(const char *, int, int);

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

[Ocfs2-devel] [xfs-masters] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Christoph Hellwig]

On Sat, Jun 20, 2009 at 08:13:59AM +0000, Arnd Bergmann wrote:
> I'd just define this using compat_s64 instead of __s64 __packed so we can
> use the same code on all architectures, even at the small cost of extra
> text size on non-x86 architectures.

I'm not a big fan of imposing the translation overhead to all
architectures, but doing it allows using the good old HANDLE_IOCTL
and futher simplifying things.



Index: linux-2.6/fs/compat_ioctl.c
===================================================================
--- linux-2.6.orig/fs/compat_ioctl.c	2009-06-20 20:19:47.035930201 +0200
+++ linux-2.6/fs/compat_ioctl.c	2009-06-20 20:20:43.821833210 +0200
@@ -31,6 +31,7 @@
 #include <linux/skbuff.h>
 #include <linux/netlink.h>
 #include <linux/vt.h>
+#include <linux/falloc.h>
 #include <linux/fs.h>
 #include <linux/file.h>
 #include <linux/ppp_defs.h>
@@ -1820,6 +1821,38 @@ lp_timeout_trans(unsigned int fd, unsign
 	return sys_ioctl(fd, cmd, (unsigned long)tn);
 }
 
+/* on ia32 l_start is on a 32-bit boundary */
+struct space_resv_32 {
+	__s16		l_type;
+	__s16		l_whence;
+	compat_s64	l_start;
+	compat_s64	l_len;		/* len == 0 means until end of file */
+	__s32		l_sysid;
+	__u32		l_pid;
+	__s32		l_pad[4];	/* reserve area */
+};
+
+#define FS_IOC_RESVSP_32		_IOW ('X', 40, struct space_resv_32)
+#define FS_IOC_RESVSP64_32	_IOW ('X', 42, struct space_resv_32)
+
+/* just account for different alignment */
+static int compat_ioctl_resvsp(unsigned fd, unsigned cmd, unsigned long arg)
+{
+	struct space_resv_32	__user *p32 = (void __user *)arg;
+	struct space_resv	__user *p = compat_alloc_user_space(sizeof(*p));
+
+	if (copy_in_user(&p->l_type,	&p32->l_type,	sizeof(s16)) ||
+	    copy_in_user(&p->l_whence,	&p32->l_whence, sizeof(s16)) ||
+	    copy_in_user(&p->l_start,	&p32->l_start,	sizeof(s64)) ||
+	    copy_in_user(&p->l_len,	&p32->l_len,	sizeof(s64)) ||
+	    copy_in_user(&p->l_sysid,	&p32->l_sysid,	sizeof(s32)) ||
+	    copy_in_user(&p->l_pid,	&p32->l_pid,	sizeof(u32)) ||
+	    copy_in_user(&p->l_pad,	&p32->l_pad,	4*sizeof(u32)))
+		return -EFAULT;
+
+	return sys_ioctl(fd, FS_IOC_RESVSP, p);
+}
+
 
 typedef int (*ioctl_trans_handler_t)(unsigned int, unsigned int,
 					unsigned long, struct file *);
@@ -1915,6 +1948,8 @@ COMPATIBLE_IOCTL(FIGETBSZ)
 /* 'X' - originally XFS but some now in the VFS */
 COMPATIBLE_IOCTL(FIFREEZE)
 COMPATIBLE_IOCTL(FITHAW)
+HANDLE_IOCTL(FS_IOC_RESVSP_32, compat_ioctl_resvsp)
+HANDLE_IOCTL(FS_IOC_RESVSP64_32, compat_ioctl_resvsp)
 /* RAID */
 COMPATIBLE_IOCTL(RAID_VERSION)
 COMPATIBLE_IOCTL(GET_ARRAY_INFO)
Index: linux-2.6/fs/ioctl.c
===================================================================
--- linux-2.6.orig/fs/ioctl.c	2009-06-20 20:19:47.040930349 +0200
+++ linux-2.6/fs/ioctl.c	2009-06-20 20:20:43.821833210 +0200
@@ -15,6 +15,7 @@
 #include <linux/uaccess.h>
 #include <linux/writeback.h>
 #include <linux/buffer_head.h>
+#include <linux/falloc.h>
 
 #include <asm/ioctls.h>
 
@@ -403,6 +404,37 @@ EXPORT_SYMBOL(generic_block_fiemap);
 
 #endif  /*  CONFIG_BLOCK  */
 
+/*
+ * This provides compatibility with legacy XFS pre-allocation ioctls
+ * which predate the fallocate syscall.
+ *
+ * Only the l_start, l_len and l_whence fields of the 'struct space_resv'
+ * are used here, rest are ignored.
+ */
+static int ioctl_resvsp(struct file *filp, void __user *argp)
+{
+	struct inode *inode = filp->f_path.dentry->d_inode;
+	struct space_resv sr;
+
+	if (copy_from_user(&sr, argp, sizeof(sr)))
+		return -EFAULT;
+
+	switch (sr.l_whence) {
+	case SEEK_SET:
+		break;
+	case SEEK_CUR:
+		sr.l_start += filp->f_pos;
+		break;
+	case SEEK_END:
+		sr.l_start += i_size_read(inode);
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return do_fallocate(filp, FALLOC_FL_KEEP_SIZE, sr.l_start, sr.l_len);
+}
+
 static int file_ioctl(struct file *filp, unsigned int cmd,
 		unsigned long arg)
 {
@@ -414,6 +446,9 @@ static int file_ioctl(struct file *filp,
 		return ioctl_fibmap(filp, p);
 	case FIONREAD:
 		return put_user(i_size_read(inode) - filp->f_pos, p);
+	case FS_IOC_RESVSP:
+	case FS_IOC_RESVSP64:
+		return ioctl_resvsp(filp, p);
 	}
 
 	return vfs_ioctl(filp, cmd, arg);
Index: linux-2.6/fs/open.c
===================================================================
--- linux-2.6.orig/fs/open.c	2009-06-20 20:19:47.054929896 +0200
+++ linux-2.6/fs/open.c	2009-06-20 20:20:43.822836760 +0200
@@ -378,63 +378,63 @@ SYSCALL_ALIAS(sys_ftruncate64, SyS_ftrun
 #endif
 #endif /* BITS_PER_LONG == 32 */
 
-SYSCALL_DEFINE(fallocate)(int fd, int mode, loff_t offset, loff_t len)
+
+int do_fallocate(struct file *file, int mode, loff_t offset, loff_t len)
 {
-	struct file *file;
-	struct inode *inode;
-	long ret = -EINVAL;
+	struct inode *inode = file->f_path.dentry->d_inode;
+	long ret;
 
 	if (offset < 0 || len <= 0)
-		goto out;
+		return -EINVAL;
 
 	/* Return error if mode is not supported */
-	ret = -EOPNOTSUPP;
 	if (mode && !(mode & FALLOC_FL_KEEP_SIZE))
-		goto out;
+		return -EOPNOTSUPP;
 
-	ret = -EBADF;
-	file = fget(fd);
-	if (!file)
-		goto out;
 	if (!(file->f_mode & FMODE_WRITE))
-		goto out_fput;
+		return -EBADF;
 	/*
 	 * Revalidate the write permissions, in case security policy has
 	 * changed since the files were opened.
 	 */
 	ret = security_file_permission(file, MAY_WRITE);
 	if (ret)
-		goto out_fput;
+		return ret;
 
-	inode = file->f_path.dentry->d_inode;
-
-	ret = -ESPIPE;
 	if (S_ISFIFO(inode->i_mode))
-		goto out_fput;
+		return -ESPIPE;
 
-	ret = -ENODEV;
 	/*
 	 * Let individual file system decide if it supports preallocation
 	 * for directories or not.
 	 */
 	if (!S_ISREG(inode->i_mode) && !S_ISDIR(inode->i_mode))
-		goto out_fput;
+		return -ENODEV;
 
-	ret = -EFBIG;
 	/* Check for wrap through zero too */
 	if (((offset + len) > inode->i_sb->s_maxbytes) || ((offset + len) < 0))
-		goto out_fput;
+		return -EFBIG;
 
-	if (inode->i_op->fallocate)
-		ret = inode->i_op->fallocate(inode, mode, offset, len);
-	else
-		ret = -EOPNOTSUPP;
+	if (!inode->i_op->fallocate)
+		return -EOPNOTSUPP;
 
-out_fput:
-	fput(file);
-out:
-	return ret;
+	return inode->i_op->fallocate(inode, mode, offset, len);
 }
+
+SYSCALL_DEFINE(fallocate)(int fd, int mode, loff_t offset, loff_t len)
+{
+	struct file *file;
+	int error = -EBADF;
+
+	file = fget(fd);
+	if (file) {
+		error = do_fallocate(file, mode, offset, len);
+		fput(file);
+	}
+
+	return error;
+}
+
 #ifdef CONFIG_HAVE_SYSCALL_WRAPPERS
 asmlinkage long SyS_fallocate(long fd, long mode, loff_t offset, loff_t len)
 {
Index: linux-2.6/include/linux/falloc.h
===================================================================
--- linux-2.6.orig/include/linux/falloc.h	2009-06-20 20:19:47.059930323 +0200
+++ linux-2.6/include/linux/falloc.h	2009-06-20 20:20:43.822836760 +0200
@@ -3,4 +3,25 @@
 
 #define FALLOC_FL_KEEP_SIZE	0x01 /* default is extend size */
 
+#ifdef __KERNEL__
+
+/*
+ * Space reservation ioctls and argument structure
+ * are designed to be compatible with the legacy XFS ioctls.
+ */
+struct space_resv {
+	__s16		l_type;
+	__s16		l_whence;
+	__s64		l_start;
+	__s64		l_len;		/* len == 0 means until end of file */
+	__s32		l_sysid;
+	__u32		l_pid;
+	__s32		l_pad[4];	/* reserved area */
+};
+
+#define FS_IOC_RESVSP		_IOW('X', 40, struct space_resv)
+#define FS_IOC_RESVSP64		_IOW('X', 42, struct space_resv)
+
+#endif /* __KERNEL__ */
+
 #endif /* _FALLOC_H_ */
Index: linux-2.6/include/linux/fs.h
===================================================================
--- linux-2.6.orig/include/linux/fs.h	2009-06-20 20:19:47.065929969 +0200
+++ linux-2.6/include/linux/fs.h	2009-06-20 20:20:43.823832557 +0200
@@ -1905,6 +1905,8 @@ static inline int break_lease(struct ino
 
 extern int do_truncate(struct dentry *, loff_t start, unsigned int time_attrs,
 		       struct file *filp);
+extern int do_fallocate(struct file *file, int mode, loff_t offset,
+			loff_t len);
 extern long do_sys_open(int dfd, const char __user *filename, int flags,
 			int mode);
 extern struct file *filp_open(const char *, int, int);

[Ocfs2-devel] [PATCH] fs: Add new pre-allocation ioctls to vfs for compatibility with legacy xfs ioctls

[Arnd Bergmann]

On Friday 19 June 2009 06:28:07 pm Christoph Hellwig wrote:

> +/* on ia32 l_start is on a 32-bit boundary */
> +#if defined(CONFIG_IA64) || defined(CONFIG_X86_64)
> +struct space_resv_32 {
> +	__s16		l_type;
> +	__s16		l_whence;
> +	__s64		l_start	__attribute__((packed));
> +			/* len == 0 means until end of file */
> +	__s64		l_len __attribute__((packed));
> +	__s32		l_sysid;
> +	__u32		l_pid;
> +	__s32		l_pad[4];	/* reserve area */
> +};
> +
> +#define FS_IOC_RESVSP_32		_IOW ('X', 40, struct space_resv_32)
> +#define FS_IOC_RESVSP64_32	_IOW ('X', 42, struct space_resv_32)

I'd just define this using compat_s64 instead of __s64 __packed so we can
use the same code on all architectures, even at the small cost of extra
text size on non-x86 architectures.

> +/* just account for different alignment */
> +static int compat_ioctl_preallocate(struct file *file, unsigned long arg)
> +{
> +	struct space_resv_32	__user *p32 = (void __user *)arg;
> +	struct space_resv	__user *p = compat_alloc_user_space(sizeof(*p));
> +
> +	if (copy_in_user(&p->l_type,	&p32->l_type,	sizeof(s16)) ||
> +	    copy_in_user(&p->l_whence,	&p32->l_whence, sizeof(s16)) ||
> +	    copy_in_user(&p->l_start,	&p32->l_start,	sizeof(s64)) ||
> +	    copy_in_user(&p->l_len,	&p32->l_len,	sizeof(s64)) ||
> +	    copy_in_user(&p->l_sysid,	&p32->l_sysid,	sizeof(s32)) ||
> +	    copy_in_user(&p->l_pid,	&p32->l_pid,	sizeof(u32)) ||
> +	    copy_in_user(&p->l_pad,	&p32->l_pad,	4*sizeof(u32)))
> +		return -EFAULT;
> +
> +	return ioctl_preallocate(file, p);
> +}
> +#endif
> +

Here, you can call do_fallocate directly in the same way that ioctl_preallocate
does, replacing the copy_in_user calls with __get_user().

	Arnd <><