[PATCH][LIBFS] Move transaction file ops into libfs

[PATCH][LIBFS] Move transaction file ops into libfs

[James Morris]

This patch moves some duplicated transaction-based IO file ops into libfs,
which will also then be useful for future filesystems.  I've tested
SELinux and nfsd, and both seem ok.  Thanks to Al Viro for suggesting the
correct abstraction for transaction_write().

Please review.


 fs/libfs.c                   |  102 +++++++++++++++++++++++++++++++++++++++++++
 fs/nfsd/nfsctl.c             |   98 +++++++----------------------------------
 include/linux/fs.h           |   16 ++++++
 security/selinux/selinuxfs.c |   96 ++++++----------------------------------
 4 files changed, 151 insertions(+), 161 deletions(-)

Signed-off-by: James Morris <jmorris@redhat.com>

diff -urN -X dontdiff linux-2.6.8-rc4.o/fs/libfs.c linux-2.6.8-rc4.w2/fs/libfs.c
--- linux-2.6.8-rc4.o/fs/libfs.c	2004-08-10 01:20:14.000000000 -0400
+++ linux-2.6.8-rc4.w2/fs/libfs.c	2004-08-13 04:27:07.583622512 -0400
@@ -456,6 +456,104 @@
 	return count;
 }
 
+/*
+ * transaction based IO methods.
+ * The file expects a single write which triggers the transaction, and then
+ * possibly a read which collects the result - which is stored in a
+ * file-local buffer.
+ */
+ssize_t transaction_write(struct file *file, const char __user *buf, size_t size)
+{
+	ssize_t ret = 0;
+	struct transaction_argresp *ar = file->private_data;
+	
+	/* only one write allowed per open */
+	if (ar) {
+		ret = -EINVAL;
+		goto out;
+	}
+	
+	if (size > PAGE_SIZE - sizeof(*ar) - 1) {
+		ret = -EFBIG;
+		goto out;
+	}
+	
+	ar = (struct transaction_argresp *)get_zeroed_page(GFP_KERNEL);
+	if (!ar) {
+		ret = -ENOMEM;
+		goto out;
+	}
+	ar->size = 0;
+	
+	down(&file->f_dentry->d_inode->i_sem);
+	if (file->private_data)
+		ret = -EINVAL;
+	else
+		file->private_data = ar;
+	up(&file->f_dentry->d_inode->i_sem);
+	
+	if (ret) {
+		free_page((unsigned long)ar);
+		goto out;
+	}
+	
+	if (copy_from_user(ar->data, buf, size))
+		ret = -EFAULT;
+out:
+	return ret;
+}
+
+ssize_t transaction_read(struct file *file, char __user *buf, size_t size, loff_t *pos)
+{
+	struct transaction_argresp *ar;
+	struct inode *inode = file->f_dentry->d_inode;
+	ssize_t rv = 0;
+	
+	BUG_ON(inode->i_fop->write == NULL);
+
+	if (file->private_data == NULL)
+		rv = inode->i_fop->write(file, buf, 0, pos);
+	if (rv < 0)
+		goto out;
+	else
+		rv = 0;
+
+	ar = file->private_data;
+	if (!ar)
+		goto out;
+
+	if (*pos >= ar->size)
+		goto out;
+
+	if (*pos + size > ar->size)
+		size = ar->size - *pos;
+
+	if (copy_to_user(buf, ar->data + *pos, size)) {
+		rv = -EFAULT;
+		goto out;
+	}
+
+	*pos += size;
+	rv = size;
+out:
+	return rv;
+}
+
+int transaction_open(struct inode *inode, struct file *file)
+{
+	file->private_data = NULL;
+	return 0;
+}
+
+int transaction_release(struct inode *inode, struct file *file)
+{
+	char *page = file->private_data;
+
+	file->private_data = NULL;
+	free_page((unsigned long)page);
+	return 0;
+}
+
 EXPORT_SYMBOL(dcache_dir_close);
 EXPORT_SYMBOL(dcache_dir_lseek);
 EXPORT_SYMBOL(dcache_dir_open);
@@ -479,3 +577,7 @@
 EXPORT_SYMBOL(simple_sync_file);
 EXPORT_SYMBOL(simple_unlink);
 EXPORT_SYMBOL(simple_read_from_buffer);
+EXPORT_SYMBOL(transaction_write);
+EXPORT_SYMBOL(transaction_read);
+EXPORT_SYMBOL(transaction_open);
+EXPORT_SYMBOL(transaction_release);
diff -urN -X dontdiff linux-2.6.8-rc4.o/fs/nfsd/nfsctl.c linux-2.6.8-rc4.w2/fs/nfsd/nfsctl.c
--- linux-2.6.8-rc4.o/fs/nfsd/nfsctl.c	2004-08-10 01:20:14.000000000 -0400
+++ linux-2.6.8-rc4.w2/fs/nfsd/nfsctl.c	2004-08-13 04:34:09.796436408 -0400
@@ -80,101 +80,37 @@
 	[NFSD_Leasetime] = write_leasetime,
 };
 
-/* an argresp is stored in an allocated page and holds the 
- * size of the argument or response, along with its content
- */
-struct argresp {
-	ssize_t size;
-	char data[0];
-};
-
-/*
- * transaction based IO methods.
- * The file expects a single write which triggers the transaction, and then
- * possibly a read which collects the result - which is stored in a 
- * file-local buffer.
- */
-static ssize_t TA_write(struct file *file, const char __user *buf, size_t size, loff_t *pos)
+static ssize_t nfsctl_transaction_write(struct file *file, const char __user *buf, size_t size, loff_t *pos)
 {
 	ino_t ino =  file->f_dentry->d_inode->i_ino;
-	struct argresp *ar;
+	struct transaction_argresp *ar;
 	ssize_t rv = 0;
 
-	if (ino >= sizeof(write_op)/sizeof(write_op[0]) || !write_op[ino])
-		return -EINVAL;
-	if (file->private_data) 
-		return -EINVAL; /* only one write allowed per open */
-	if (size > PAGE_SIZE - sizeof(struct argresp))
-		return -EFBIG;
-
-	ar = kmalloc(PAGE_SIZE, GFP_KERNEL);
-	if (!ar)
-		return -ENOMEM;
-	ar->size = 0;
-	down(&file->f_dentry->d_inode->i_sem);
-	if (file->private_data)
+	if (ino >= sizeof(write_op)/sizeof(write_op[0]) || !write_op[ino]) {
 		rv = -EINVAL;
-	else
-		file->private_data = ar;
-	up(&file->f_dentry->d_inode->i_sem);
-	if (rv) {
-		kfree(ar);
-		return rv;
+		goto out;
 	}
-	if (copy_from_user(ar->data, buf, size))
-		return -EFAULT;
-	
+
+	rv = transaction_write(file, buf, size);
+	if (rv)
+		goto out;
+
+	ar = file->private_data;
+
 	rv =  write_op[ino](file, ar->data, size);
 	if (rv>0) {
 		ar->size = rv;
 		rv = size;
 	}
+out:
 	return rv;
 }
 
-
-static ssize_t TA_read(struct file *file, char __user *buf, size_t size, loff_t *pos)
-{
-	struct argresp *ar;
-	ssize_t rv = 0;
-	
-	if (file->private_data == NULL)
-		rv = TA_write(file, buf, 0, pos);
-	if (rv < 0)
-		return rv;
-
-	ar = file->private_data;
-	if (!ar)
-		return 0;
-	if (*pos >= ar->size)
-		return 0;
-	if (*pos + size > ar->size)
-		size = ar->size - *pos;
-	if (copy_to_user(buf, ar->data + *pos, size))
-		return -EFAULT;
-	*pos += size;
-	return size;
-}
-
-static int TA_open(struct inode *inode, struct file *file)
-{
-	file->private_data = NULL;
-	return 0;
-}
-
-static int TA_release(struct inode *inode, struct file *file)
-{
-	void *p = file->private_data;
-	file->private_data = NULL;
-	kfree(p);
-	return 0;
-}
-
 static struct file_operations transaction_ops = {
-	.write		= TA_write,
-	.read		= TA_read,
-	.open		= TA_open,
-	.release	= TA_release,
+	.write		= nfsctl_transaction_write,
+	.read		= transaction_read,
+	.open		= transaction_open,
+	.release	= transaction_release,
 };
 
 extern struct seq_operations nfs_exports_op;
@@ -366,7 +302,7 @@
 	if (len)
 		return len;
 	
-	mesg = buf; len = PAGE_SIZE-sizeof(struct argresp);
+	mesg = buf; len = PAGE_SIZE-sizeof(struct transaction_argresp);
 	qword_addhex(&mesg, &len, (char*)&fh.fh_base, fh.fh_size);
 	mesg[-1] = '\n';
 	return mesg - buf;	
diff -urN -X dontdiff linux-2.6.8-rc4.o/include/linux/fs.h linux-2.6.8-rc4.w2/include/linux/fs.h
--- linux-2.6.8-rc4.o/include/linux/fs.h	2004-08-10 01:20:15.000000000 -0400
+++ linux-2.6.8-rc4.w2/include/linux/fs.h	2004-08-13 04:25:30.754342800 -0400
@@ -1553,6 +1553,22 @@
 /* kernel/fork.c */
 extern int unshare_files(void);
 
+/* Transaction based IO helpers */
+
+/*
+ * An argresp is stored in an allocated page and holds the 
+ * size of the argument or response, along with its content
+ */
+struct transaction_argresp {
+	ssize_t size;
+	char data[0];
+};
+
+ssize_t transaction_write(struct file *file, const char __user *buf, size_t size);
+ssize_t transaction_read(struct file *file, char __user *buf, size_t size, loff_t *pos);
+int transaction_open(struct inode *inode, struct file *file);
+int transaction_release(struct inode *inode, struct file *file);
+
 #ifdef CONFIG_SECURITY
 static inline char *alloc_secdata(void)
 {
diff -urN -X dontdiff linux-2.6.8-rc4.o/security/selinux/selinuxfs.c linux-2.6.8-rc4.w2/security/selinux/selinuxfs.c
--- linux-2.6.8-rc4.o/security/selinux/selinuxfs.c	2004-06-16 01:20:26.000000000 -0400
+++ linux-2.6.8-rc4.w2/security/selinux/selinuxfs.c	2004-08-13 04:33:10.895390728 -0400
@@ -390,103 +390,39 @@
 	[SEL_USER] = sel_write_user,
 };
 
-/* an argresp is stored in an allocated page and holds the
- * size of the argument or response, along with its content
- */
-struct argresp {
-	ssize_t size;
-	char data[0];
-};
+#define PAYLOAD_SIZE (PAGE_SIZE - sizeof(struct transaction_argresp))
 
-#define PAYLOAD_SIZE (PAGE_SIZE - sizeof(struct argresp))
-
-/*
- * transaction based IO methods.
- * The file expects a single write which triggers the transaction, and then
- * possibly a read which collects the result - which is stored in a
- * file-local buffer.
- */
-static ssize_t TA_write(struct file *file, const char __user *buf, size_t size, loff_t *pos)
+static ssize_t selinux_transaction_write(struct file *file, const char __user *buf, size_t size, loff_t *pos)
 {
 	ino_t ino =  file->f_dentry->d_inode->i_ino;
-	struct argresp *ar;
+	struct transaction_argresp *ar;
 	ssize_t rv = 0;
 
-	if (ino >= sizeof(write_op)/sizeof(write_op[0]) || !write_op[ino])
-		return -EINVAL;
-	if (file->private_data)
-		return -EINVAL; /* only one write allowed per open */
-	if (size > PAYLOAD_SIZE - 1) /* allow one byte for null terminator */
-		return -EFBIG;
-
-	ar = kmalloc(PAGE_SIZE, GFP_KERNEL);
-	if (!ar)
-		return -ENOMEM;
-	memset(ar, 0, PAGE_SIZE); /* clear buffer, particularly last byte */
-	ar->size = 0;
-	down(&file->f_dentry->d_inode->i_sem);
-	if (file->private_data)
+	if (ino >= sizeof(write_op)/sizeof(write_op[0]) || !write_op[ino]) {
 		rv = -EINVAL;
-	else
-		file->private_data = ar;
-	up(&file->f_dentry->d_inode->i_sem);
-	if (rv) {
-		kfree(ar);
-		return rv;
+		goto out;
 	}
-	if (copy_from_user(ar->data, buf, size))
-		return -EFAULT;
+
+	rv = transaction_write(file, buf, size);
+	if (rv)
+		goto out;
+
+	ar = file->private_data;
 
 	rv =  write_op[ino](file, ar->data, size);
 	if (rv>0) {
 		ar->size = rv;
 		rv = size;
 	}
+out:
 	return rv;
 }
 
-static ssize_t TA_read(struct file *file, char __user *buf, size_t size, loff_t *pos)
-{
-	struct argresp *ar;
-	ssize_t rv = 0;
-
-	if (file->private_data == NULL)
-		rv = TA_write(file, buf, 0, pos);
-	if (rv < 0)
-		return rv;
-
-	ar = file->private_data;
-	if (!ar)
-		return 0;
-	if (*pos >= ar->size)
-		return 0;
-	if (*pos + size > ar->size)
-		size = ar->size - *pos;
-	if (copy_to_user(buf, ar->data + *pos, size))
-		return -EFAULT;
-	*pos += size;
-	return size;
-}
-
-static int TA_open(struct inode *inode, struct file *file)
-{
-	file->private_data = NULL;
-	return 0;
-}
-
-static int TA_release(struct inode *inode, struct file *file)
-{
-	void *p = file->private_data;
-	file->private_data = NULL;
-	kfree(p);
-	return 0;
-}
-
 static struct file_operations transaction_ops = {
-	.write		= TA_write,
-	.read		= TA_read,
-	.open		= TA_open,
-	.release	= TA_release,
+	.write		= selinux_transaction_write,
+	.read		= transaction_read,
+	.open		= transaction_open,
+	.release	= transaction_release,
 };
 
 /*

[PATCH][LIBFS] Move transaction file ops into libfs + cleanup (update)

[James Morris]

Below is an updated version of the patch, which moves duplicated
transaction-based file operation code into libfs.  Since the last post,
the patch has been through a couple of iterations with Al, who suggested a
number of cleanups including locking and interface simplification.

For filesystem writers, the interface is now much simpler.  The
simple_transaction_get() helper should be part of the file op write
method.  This safely obtains the transaction request data during write(),
allocates a page for it and stores it there.  The data is returned to the
caller for potential further processing, which then makes it available for
the next read() call via simple_transaction_set().  See the selinuxfs and
nfsctl code for examples of use.

Please review and apply.

Signed-off-by: James Morris <jmorris@redhat.com>

 fs/libfs.c                   |   55 ++++++++++++++++++++++
 fs/nfsd/nfsctl.c             |   96 +++++-----------------------------------
 include/linux/fs.h           |   26 ++++++++++
 security/selinux/selinuxfs.c |  103 ++++++-------------------------------------
 4 files changed, 110 insertions(+), 170 deletions(-)


diff -urN -X dontdiff linux-2.6.8.1.o/fs/libfs.c linux-2.6.8.1.w/fs/libfs.c
--- linux-2.6.8.1.o/fs/libfs.c	2004-08-14 10:25:42.000000000 -0400
+++ linux-2.6.8.1.w/fs/libfs.c	2004-08-14 12:57:55.654875688 -0400
@@ -456,6 +456,58 @@
 	return count;
 }
 
+/*
+ * Transaction based IO.
+ * The file expects a single write which triggers the transaction, and then
+ * possibly a read which collects the result - which is stored in a
+ * file-local buffer.
+ */
+char *simple_transaction_get(struct file *file, const char __user *buf, size_t size)
+{
+	struct simple_transaction_argresp *ar;
+	static spinlock_t simple_transaction_lock = SPIN_LOCK_UNLOCKED;
+
+	if (size > SIMPLE_TRANSACTION_LIMIT - 1)
+		return ERR_PTR(-EFBIG);
+
+	ar = (struct simple_transaction_argresp *)get_zeroed_page(GFP_KERNEL);
+	if (!ar)
+		return ERR_PTR(-ENOMEM);
+
+	spin_lock(&simple_transaction_lock);
+	
+	/* only one write allowed per open */
+	if (file->private_data) {
+		spin_unlock(&simple_transaction_lock);
+		free_page((unsigned long)ar);
+		return ERR_PTR(-EBUSY);
+	}
+
+	file->private_data = ar;
+	
+	spin_unlock(&simple_transaction_lock);
+	
+	if (copy_from_user(ar->data, buf, size))
+		return ERR_PTR(-EFAULT);
+
+	return ar->data;
+}
+
+ssize_t simple_transaction_read(struct file *file, char __user *buf, size_t size, loff_t *pos)
+{
+	struct simple_transaction_argresp *ar = file->private_data;
+	
+	if (!ar)
+		return 0;
+	return simple_read_from_buffer(buf, size, pos, ar->data, ar->size);
+}
+
+int simple_transaction_release(struct inode *inode, struct file *file)
+{
+	free_page((unsigned long)file->private_data);
+	return 0;
+}
+
 EXPORT_SYMBOL(dcache_dir_close);
 EXPORT_SYMBOL(dcache_dir_lseek);
 EXPORT_SYMBOL(dcache_dir_open);
@@ -479,3 +531,6 @@
 EXPORT_SYMBOL(simple_sync_file);
 EXPORT_SYMBOL(simple_unlink);
 EXPORT_SYMBOL(simple_read_from_buffer);
+EXPORT_SYMBOL(simple_transaction_get);
+EXPORT_SYMBOL(simple_transaction_read);
+EXPORT_SYMBOL(simple_transaction_release);
diff -urN -X dontdiff linux-2.6.8.1.o/fs/nfsd/nfsctl.c linux-2.6.8.1.w/fs/nfsd/nfsctl.c
--- linux-2.6.8.1.o/fs/nfsd/nfsctl.c	2004-08-14 10:25:42.000000000 -0400
+++ linux-2.6.8.1.w/fs/nfsd/nfsctl.c	2004-08-14 12:58:33.246160944 -0400
@@ -80,101 +80,31 @@
 	[NFSD_Leasetime] = write_leasetime,
 };
 
-/* an argresp is stored in an allocated page and holds the 
- * size of the argument or response, along with its content
- */
-struct argresp {
-	ssize_t size;
-	char data[0];
-};
-
-/*
- * transaction based IO methods.
- * The file expects a single write which triggers the transaction, and then
- * possibly a read which collects the result - which is stored in a 
- * file-local buffer.
- */
-static ssize_t TA_write(struct file *file, const char __user *buf, size_t size, loff_t *pos)
+static ssize_t nfsctl_transaction_write(struct file *file, const char __user *buf, size_t size, loff_t *pos)
 {
 	ino_t ino =  file->f_dentry->d_inode->i_ino;
-	struct argresp *ar;
-	ssize_t rv = 0;
+	char *data;
+	ssize_t rv;
 
 	if (ino >= sizeof(write_op)/sizeof(write_op[0]) || !write_op[ino])
 		return -EINVAL;
-	if (file->private_data) 
-		return -EINVAL; /* only one write allowed per open */
-	if (size > PAGE_SIZE - sizeof(struct argresp))
-		return -EFBIG;
 
-	ar = kmalloc(PAGE_SIZE, GFP_KERNEL);
-	if (!ar)
-		return -ENOMEM;
-	ar->size = 0;
-	down(&file->f_dentry->d_inode->i_sem);
-	if (file->private_data)
-		rv = -EINVAL;
-	else
-		file->private_data = ar;
-	up(&file->f_dentry->d_inode->i_sem);
-	if (rv) {
-		kfree(ar);
-		return rv;
-	}
-	if (copy_from_user(ar->data, buf, size))
-		return -EFAULT;
-	
-	rv =  write_op[ino](file, ar->data, size);
+	data = simple_transaction_get(file, buf, size);
+	if (IS_ERR(data))
+		return PTR_ERR(data);
+
+	rv =  write_op[ino](file, data, size);
 	if (rv>0) {
-		ar->size = rv;
+		simple_transaction_set(file, rv);
 		rv = size;
 	}
 	return rv;
 }
 
-
-static ssize_t TA_read(struct file *file, char __user *buf, size_t size, loff_t *pos)
-{
-	struct argresp *ar;
-	ssize_t rv = 0;
-	
-	if (file->private_data == NULL)
-		rv = TA_write(file, buf, 0, pos);
-	if (rv < 0)
-		return rv;
-
-	ar = file->private_data;
-	if (!ar)
-		return 0;
-	if (*pos >= ar->size)
-		return 0;
-	if (*pos + size > ar->size)
-		size = ar->size - *pos;
-	if (copy_to_user(buf, ar->data + *pos, size))
-		return -EFAULT;
-	*pos += size;
-	return size;
-}
-
-static int TA_open(struct inode *inode, struct file *file)
-{
-	file->private_data = NULL;
-	return 0;
-}
-
-static int TA_release(struct inode *inode, struct file *file)
-{
-	void *p = file->private_data;
-	file->private_data = NULL;
-	kfree(p);
-	return 0;
-}
-
 static struct file_operations transaction_ops = {
-	.write		= TA_write,
-	.read		= TA_read,
-	.open		= TA_open,
-	.release	= TA_release,
+	.write		= nfsctl_transaction_write,
+	.read		= simple_transaction_read,
+	.release	= simple_transaction_release,
 };
 
 extern struct seq_operations nfs_exports_op;
@@ -366,7 +296,7 @@
 	if (len)
 		return len;
 	
-	mesg = buf; len = PAGE_SIZE-sizeof(struct argresp);
+	mesg = buf; len = SIMPLE_TRANSACTION_LIMIT;
 	qword_addhex(&mesg, &len, (char*)&fh.fh_base, fh.fh_size);
 	mesg[-1] = '\n';
 	return mesg - buf;	
diff -urN -X dontdiff linux-2.6.8.1.o/include/linux/fs.h linux-2.6.8.1.w/include/linux/fs.h
--- linux-2.6.8.1.o/include/linux/fs.h	2004-08-14 10:25:44.000000000 -0400
+++ linux-2.6.8.1.w/include/linux/fs.h	2004-08-14 12:57:56.175796496 -0400
@@ -1550,6 +1550,32 @@
 /* kernel/fork.c */
 extern int unshare_files(void);
 
+/* Transaction based IO helpers */
+
+/*
+ * An argresp is stored in an allocated page and holds the 
+ * size of the argument or response, along with its content
+ */
+struct simple_transaction_argresp {
+	ssize_t size;
+	char data[0];
+};
+
+#define SIMPLE_TRANSACTION_LIMIT (PAGE_SIZE - sizeof(struct simple_transaction_argresp))
+
+char *simple_transaction_get(struct file *file, const char __user *buf, size_t size);
+ssize_t simple_transaction_read(struct file *file, char __user *buf, size_t size, loff_t *pos);
+int simple_transaction_release(struct inode *inode, struct file *file);
+
+static inline void simple_transaction_set(struct file *file, size_t n)
+{
+	struct simple_transaction_argresp *ar = file->private_data;
+	
+	BUG_ON(n > SIMPLE_TRANSACTION_LIMIT);
+	mb();
+	ar->size = n;
+}
+
 #ifdef CONFIG_SECURITY
 static inline char *alloc_secdata(void)
 {
diff -urN -X dontdiff linux-2.6.8.1.o/security/selinux/selinuxfs.c linux-2.6.8.1.w/security/selinux/selinuxfs.c
--- linux-2.6.8.1.o/security/selinux/selinuxfs.c	2004-06-16 01:20:26.000000000 -0400
+++ linux-2.6.8.1.w/security/selinux/selinuxfs.c	2004-08-14 12:58:44.667424648 -0400
@@ -390,103 +390,31 @@
 	[SEL_USER] = sel_write_user,
 };
 
-/* an argresp is stored in an allocated page and holds the
- * size of the argument or response, along with its content
- */
-struct argresp {
-	ssize_t size;
-	char data[0];
-};
-
-#define PAYLOAD_SIZE (PAGE_SIZE - sizeof(struct argresp))
-
-/*
- * transaction based IO methods.
- * The file expects a single write which triggers the transaction, and then
- * possibly a read which collects the result - which is stored in a
- * file-local buffer.
- */
-static ssize_t TA_write(struct file *file, const char __user *buf, size_t size, loff_t *pos)
+static ssize_t selinux_transaction_write(struct file *file, const char __user *buf, size_t size, loff_t *pos)
 {
 	ino_t ino =  file->f_dentry->d_inode->i_ino;
-	struct argresp *ar;
-	ssize_t rv = 0;
+	char *data;
+	ssize_t rv;
 
 	if (ino >= sizeof(write_op)/sizeof(write_op[0]) || !write_op[ino])
 		return -EINVAL;
-	if (file->private_data)
-		return -EINVAL; /* only one write allowed per open */
-	if (size > PAYLOAD_SIZE - 1) /* allow one byte for null terminator */
-		return -EFBIG;
 
-	ar = kmalloc(PAGE_SIZE, GFP_KERNEL);
-	if (!ar)
-		return -ENOMEM;
-	memset(ar, 0, PAGE_SIZE); /* clear buffer, particularly last byte */
-	ar->size = 0;
-	down(&file->f_dentry->d_inode->i_sem);
-	if (file->private_data)
-		rv = -EINVAL;
-	else
-		file->private_data = ar;
-	up(&file->f_dentry->d_inode->i_sem);
-	if (rv) {
-		kfree(ar);
-		return rv;
-	}
-	if (copy_from_user(ar->data, buf, size))
-		return -EFAULT;
+	data = simple_transaction_get(file, buf, size);
+	if (IS_ERR(data))
+		return PTR_ERR(data);
 
-	rv =  write_op[ino](file, ar->data, size);
+	rv =  write_op[ino](file, data, size);
 	if (rv>0) {
-		ar->size = rv;
+		simple_transaction_set(file, rv);
 		rv = size;
 	}
 	return rv;
 }
 
-static ssize_t TA_read(struct file *file, char __user *buf, size_t size, loff_t *pos)
-{
-	struct argresp *ar;
-	ssize_t rv = 0;
-
-	if (file->private_data == NULL)
-		rv = TA_write(file, buf, 0, pos);
-	if (rv < 0)
-		return rv;
-
-	ar = file->private_data;
-	if (!ar)
-		return 0;
-	if (*pos >= ar->size)
-		return 0;
-	if (*pos + size > ar->size)
-		size = ar->size - *pos;
-	if (copy_to_user(buf, ar->data + *pos, size))
-		return -EFAULT;
-	*pos += size;
-	return size;
-}
-
-static int TA_open(struct inode *inode, struct file *file)
-{
-	file->private_data = NULL;
-	return 0;
-}
-
-static int TA_release(struct inode *inode, struct file *file)
-{
-	void *p = file->private_data;
-	file->private_data = NULL;
-	kfree(p);
-	return 0;
-}
-
 static struct file_operations transaction_ops = {
-	.write		= TA_write,
-	.read		= TA_read,
-	.open		= TA_open,
-	.release	= TA_release,
+	.write		= selinux_transaction_write,
+	.read		= simple_transaction_read,
+	.release	= simple_transaction_release,
 };
 
 /*
@@ -534,7 +462,8 @@
 	if (length < 0)
 		goto out2;
 
-	length = scnprintf(buf, PAYLOAD_SIZE, "%x %x %x %x %u",
+	length = scnprintf(buf, SIMPLE_TRANSACTION_LIMIT,
+			  "%x %x %x %x %u",
 			  avd.allowed, avd.decided,
 			  avd.auditallow, avd.auditdeny,
 			  avd.seqno);
@@ -588,7 +517,7 @@
 	if (length < 0)
 		goto out2;
 
-	if (len > PAYLOAD_SIZE) {
+	if (len > SIMPLE_TRANSACTION_LIMIT) {
 		printk(KERN_ERR "%s:  context size (%u) exceeds payload "
 		       "max\n", __FUNCTION__, len);
 		length = -ERANGE;
@@ -649,7 +578,7 @@
 	if (length < 0)
 		goto out2;
 
-	if (len > PAYLOAD_SIZE) {
+	if (len > SIMPLE_TRANSACTION_LIMIT) {
 		length = -ERANGE;
 		goto out3;
 	}
@@ -709,7 +638,7 @@
 			length = rc;
 			goto out3;
 		}
-		if ((length + len) >= PAYLOAD_SIZE) {
+		if ((length + len) >= SIMPLE_TRANSACTION_LIMIT) {
 			kfree(newcon);
 			length = -ERANGE;
 			goto out3;

Re: [PATCH][LIBFS] Move transaction file ops into libfs + cleanup (update)

[Francois Romieu]

James Morris <jmorris@redhat.com> :
[...]
> diff -urN -X dontdiff linux-2.6.8.1.o/include/linux/fs.h linux-2.6.8.1.w/include/linux/fs.h
> --- linux-2.6.8.1.o/include/linux/fs.h	2004-08-14 10:25:44.000000000 -0400
> +++ linux-2.6.8.1.w/include/linux/fs.h	2004-08-14 12:57:56.175796496 -0400
> @@ -1550,6 +1550,32 @@
[...]
> +static inline void simple_transaction_set(struct file *file, size_t n)
> +{
> +	struct simple_transaction_argresp *ar = file->private_data;
> +	
> +	BUG_ON(n > SIMPLE_TRANSACTION_LIMIT);
> +	mb();
> +	ar->size = n;
> +}

Could you add the justification for the 'mb' (or the expected effect on the
api) as a comment ?

--
Ueimor

Re: [PATCH][LIBFS] Move transaction file ops into libfs + cleanup (update)

[James Morris]

On Sat, 14 Aug 2004, Francois Romieu wrote:

> > +static inline void simple_transaction_set(struct file *file, size_t n)
> > +{
> > +	struct simple_transaction_argresp *ar = file->private_data;
> > +	
> > +	BUG_ON(n > SIMPLE_TRANSACTION_LIMIT);
> > +	mb();
> > +	ar->size = n;
> > +}
> 
> Could you add the justification for the 'mb' (or the expected effect on the
> api) as a comment ?

This ensures that ar->size will really remain zero until ar->data is ready
for reading.

- James
-- 
James Morris
<jmorris@redhat.com>

Re: [PATCH][LIBFS] Move transaction file ops into libfs + cleanup (update)

[Chris Wright]

Hi James,

* James Morris (jmorris@redhat.com) wrote:
> Below is an updated version of the patch, which moves duplicated
> transaction-based file operation code into libfs.  Since the last post,
> the patch has been through a couple of iterations with Al, who suggested a
> number of cleanups including locking and interface simplification.

Looks nice. I didn't realize you were working on this consolidation too.
I cooked up a similar patch.  In this case the user loads its inode
specific write_ops on open, then just uses the generic helpers.  I also
fully serialized all write/read transactions per inode.  It's lightly
tested.  If there's anything you like in there, feel free to use it.

thanks,
-chris

===== include/linux/fs.h 1.344 vs edited =====
--- 1.344/include/linux/fs.h	2004-08-09 12:05:17 -07:00
+++ edited/include/linux/fs.h	2004-08-14 11:25:29 -07:00
@@ -1107,6 +1107,15 @@
 	struct list_head fs_supers;
 };
 
+/* Transaction file */
+typedef ssize_t (*write_op_t) (struct file *, char *, size_t);
+struct TA_file {
+	size_t ops_size;
+	write_op_t *write_ops;
+	size_t ta_size;
+	char *ta_data;
+};
+
 struct super_block *get_sb_bdev(struct file_system_type *fs_type,
 	int flags, const char *dev_name, void *data,
 	int (*fill_super)(struct super_block *, void *, int));
@@ -1531,6 +1540,11 @@
 extern void simple_release_fs(struct vfsmount **mount, int *count);
 
 extern ssize_t simple_read_from_buffer(void __user *, size_t, loff_t *, const void *, size_t);
+
+extern int TA_open(struct file *, write_op_t *, size_t);
+extern ssize_t TA_write(struct file *, const char __user *, size_t, loff_t *);
+extern ssize_t TA_read(struct file *, char __user *, size_t, loff_t *);
+extern int TA_release(struct inode *, struct file *);
 
 extern int inode_change_ok(struct inode *, struct iattr *);
 extern int __must_check inode_setattr(struct inode *, struct iattr *);
===== fs/libfs.c 1.33 vs edited =====
--- 1.33/fs/libfs.c	2004-08-05 23:10:54 -07:00
+++ edited/fs/libfs.c	2004-08-14 12:07:53 -07:00
@@ -456,6 +456,93 @@
 	return count;
 }
 
+int TA_open(struct file *file, write_op_t *ops, size_t ops_size)
+{
+	struct TA_file *ta;
+	ta = kmalloc(sizeof(*ta), GFP_KERNEL);
+	if (ta) {
+		ta->ops_size = ops_size;
+		ta->write_ops = ops;
+		ta->ta_size = 0;
+		ta->ta_data = NULL;
+		file->private_data = ta;
+		return 0;
+	}
+	return -ENOMEM;
+}
+
+/*
+ * transaction based IO methods.
+ * The file expects a single write which triggers the transaction, and then
+ * possibly a read which collects the result - which is stored in a 
+ * file-local buffer.
+ */
+ssize_t TA_write(struct file *file, const char __user *buf, size_t size, loff_t *pos)
+{
+	ino_t ino =  file->f_dentry->d_inode->i_ino;
+	struct TA_file *ta = file->private_data;
+	ssize_t rv = 0;
+	char *data;
+
+	if (ino >= ta->ops_size || !ta->write_ops[ino])
+		return -EINVAL;
+	if (size > PAGE_SIZE)
+		return -EFBIG;
+
+	data = (char *)get_zeroed_page(GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+	down(&file->f_dentry->d_inode->i_sem);
+	if (!ta->ta_data)
+		ta->ta_data = data;
+	else
+		rv = -EINVAL;
+	if (rv) {
+		free_page((unsigned long)data);
+		goto out;
+	}
+	rv = -EFAULT;
+	if (copy_from_user(ta->ta_data, buf, size))
+		goto out;
+	
+	rv =  ta->write_ops[ino](file, ta->ta_data, size);
+	if (rv>0) {
+		ta->ta_size = rv;
+		rv = size;
+	}
+out:
+	up(&file->f_dentry->d_inode->i_sem);
+	return rv;
+}
+
+
+ssize_t TA_read(struct file *file, char __user *buf, size_t size, loff_t *pos)
+{
+	struct TA_file *ta;
+	ssize_t rv = 0;
+	
+	down(&file->f_dentry->d_inode->i_sem);
+	ta = file->private_data;
+	if (ta)
+		rv = simple_read_from_buffer(buf, size, pos, ta->ta_data,
+					     ta->ta_size);
+	up(&file->f_dentry->d_inode->i_sem);
+	return rv;
+}
+
+int TA_release(struct inode *inode, struct file *file)
+{
+	struct TA_file *ta;
+	char *data = NULL;
+	ta = file->private_data;
+	file->private_data = NULL;
+	if (ta)
+		data = ta->ta_data;
+	kfree(ta);
+	free_page((unsigned long)data);
+	return 0;
+}
+
 EXPORT_SYMBOL(dcache_dir_close);
 EXPORT_SYMBOL(dcache_dir_lseek);
 EXPORT_SYMBOL(dcache_dir_open);
@@ -479,3 +566,7 @@
 EXPORT_SYMBOL(simple_sync_file);
 EXPORT_SYMBOL(simple_unlink);
 EXPORT_SYMBOL(simple_read_from_buffer);
+EXPORT_SYMBOL(TA_open);
+EXPORT_SYMBOL(TA_write);
+EXPORT_SYMBOL(TA_read);
+EXPORT_SYMBOL(TA_release);
===== fs/nfsd/nfsctl.c 1.43 vs edited =====
--- 1.43/fs/nfsd/nfsctl.c	2004-06-30 08:29:47 -07:00
+++ edited/fs/nfsd/nfsctl.c	2004-08-14 11:25:31 -07:00
@@ -80,100 +80,15 @@
 	[NFSD_Leasetime] = write_leasetime,
 };
 
-/* an argresp is stored in an allocated page and holds the 
- * size of the argument or response, along with its content
- */
-struct argresp {
-	ssize_t size;
-	char data[0];
-};
-
-/*
- * transaction based IO methods.
- * The file expects a single write which triggers the transaction, and then
- * possibly a read which collects the result - which is stored in a 
- * file-local buffer.
- */
-static ssize_t TA_write(struct file *file, const char __user *buf, size_t size, loff_t *pos)
-{
-	ino_t ino =  file->f_dentry->d_inode->i_ino;
-	struct argresp *ar;
-	ssize_t rv = 0;
-
-	if (ino >= sizeof(write_op)/sizeof(write_op[0]) || !write_op[ino])
-		return -EINVAL;
-	if (file->private_data) 
-		return -EINVAL; /* only one write allowed per open */
-	if (size > PAGE_SIZE - sizeof(struct argresp))
-		return -EFBIG;
-
-	ar = kmalloc(PAGE_SIZE, GFP_KERNEL);
-	if (!ar)
-		return -ENOMEM;
-	ar->size = 0;
-	down(&file->f_dentry->d_inode->i_sem);
-	if (file->private_data)
-		rv = -EINVAL;
-	else
-		file->private_data = ar;
-	up(&file->f_dentry->d_inode->i_sem);
-	if (rv) {
-		kfree(ar);
-		return rv;
-	}
-	if (copy_from_user(ar->data, buf, size))
-		return -EFAULT;
-	
-	rv =  write_op[ino](file, ar->data, size);
-	if (rv>0) {
-		ar->size = rv;
-		rv = size;
-	}
-	return rv;
-}
-
-
-static ssize_t TA_read(struct file *file, char __user *buf, size_t size, loff_t *pos)
-{
-	struct argresp *ar;
-	ssize_t rv = 0;
-	
-	if (file->private_data == NULL)
-		rv = TA_write(file, buf, 0, pos);
-	if (rv < 0)
-		return rv;
-
-	ar = file->private_data;
-	if (!ar)
-		return 0;
-	if (*pos >= ar->size)
-		return 0;
-	if (*pos + size > ar->size)
-		size = ar->size - *pos;
-	if (copy_to_user(buf, ar->data + *pos, size))
-		return -EFAULT;
-	*pos += size;
-	return size;
-}
-
-static int TA_open(struct inode *inode, struct file *file)
-{
-	file->private_data = NULL;
-	return 0;
-}
-
-static int TA_release(struct inode *inode, struct file *file)
+static int nfsd_TA_open(struct inode *inode, struct file *file)
 {
-	void *p = file->private_data;
-	file->private_data = NULL;
-	kfree(p);
-	return 0;
+	return TA_open(file, write_op, sizeof(write_op)/sizeof(write_op[0]));
 }
 
 static struct file_operations transaction_ops = {
 	.write		= TA_write,
 	.read		= TA_read,
-	.open		= TA_open,
+	.open		= nfsd_TA_open,
 	.release	= TA_release,
 };
 
@@ -366,7 +281,7 @@
 	if (len)
 		return len;
 	
-	mesg = buf; len = PAGE_SIZE-sizeof(struct argresp);
+	mesg = buf; len = PAGE_SIZE;
 	qword_addhex(&mesg, &len, (char*)&fh.fh_base, fh.fh_size);
 	mesg[-1] = '\n';
 	return mesg - buf;	
===== security/selinux/selinuxfs.c 1.12 vs edited =====
--- 1.12/security/selinux/selinuxfs.c	2004-06-03 18:47:11 -07:00
+++ edited/security/selinux/selinuxfs.c	2004-08-14 11:25:30 -07:00
@@ -390,102 +390,15 @@
 	[SEL_USER] = sel_write_user,
 };
 
-/* an argresp is stored in an allocated page and holds the
- * size of the argument or response, along with its content
- */
-struct argresp {
-	ssize_t size;
-	char data[0];
-};
-
-#define PAYLOAD_SIZE (PAGE_SIZE - sizeof(struct argresp))
-
-/*
- * transaction based IO methods.
- * The file expects a single write which triggers the transaction, and then
- * possibly a read which collects the result - which is stored in a
- * file-local buffer.
- */
-static ssize_t TA_write(struct file *file, const char __user *buf, size_t size, loff_t *pos)
-{
-	ino_t ino =  file->f_dentry->d_inode->i_ino;
-	struct argresp *ar;
-	ssize_t rv = 0;
-
-	if (ino >= sizeof(write_op)/sizeof(write_op[0]) || !write_op[ino])
-		return -EINVAL;
-	if (file->private_data)
-		return -EINVAL; /* only one write allowed per open */
-	if (size > PAYLOAD_SIZE - 1) /* allow one byte for null terminator */
-		return -EFBIG;
-
-	ar = kmalloc(PAGE_SIZE, GFP_KERNEL);
-	if (!ar)
-		return -ENOMEM;
-	memset(ar, 0, PAGE_SIZE); /* clear buffer, particularly last byte */
-	ar->size = 0;
-	down(&file->f_dentry->d_inode->i_sem);
-	if (file->private_data)
-		rv = -EINVAL;
-	else
-		file->private_data = ar;
-	up(&file->f_dentry->d_inode->i_sem);
-	if (rv) {
-		kfree(ar);
-		return rv;
-	}
-	if (copy_from_user(ar->data, buf, size))
-		return -EFAULT;
-
-	rv =  write_op[ino](file, ar->data, size);
-	if (rv>0) {
-		ar->size = rv;
-		rv = size;
-	}
-	return rv;
-}
-
-static ssize_t TA_read(struct file *file, char __user *buf, size_t size, loff_t *pos)
-{
-	struct argresp *ar;
-	ssize_t rv = 0;
-
-	if (file->private_data == NULL)
-		rv = TA_write(file, buf, 0, pos);
-	if (rv < 0)
-		return rv;
-
-	ar = file->private_data;
-	if (!ar)
-		return 0;
-	if (*pos >= ar->size)
-		return 0;
-	if (*pos + size > ar->size)
-		size = ar->size - *pos;
-	if (copy_to_user(buf, ar->data + *pos, size))
-		return -EFAULT;
-	*pos += size;
-	return size;
-}
-
-static int TA_open(struct inode *inode, struct file *file)
-{
-	file->private_data = NULL;
-	return 0;
-}
-
-static int TA_release(struct inode *inode, struct file *file)
+static int sel_TA_open(struct inode *inode, struct file *file)
 {
-	void *p = file->private_data;
-	file->private_data = NULL;
-	kfree(p);
-	return 0;
+	return TA_open(file, write_op, sizeof(write_op)/sizeof(write_op[0]));
 }
 
 static struct file_operations transaction_ops = {
 	.write		= TA_write,
 	.read		= TA_read,
-	.open		= TA_open,
+	.open		= sel_TA_open,
 	.release	= TA_release,
 };
 
@@ -534,7 +447,7 @@
 	if (length < 0)
 		goto out2;
 
-	length = scnprintf(buf, PAYLOAD_SIZE, "%x %x %x %x %u",
+	length = scnprintf(buf, PAGE_SIZE, "%x %x %x %x %u",
 			  avd.allowed, avd.decided,
 			  avd.auditallow, avd.auditdeny,
 			  avd.seqno);
@@ -588,7 +501,7 @@
 	if (length < 0)
 		goto out2;
 
-	if (len > PAYLOAD_SIZE) {
+	if (len > PAGE_SIZE) {
 		printk(KERN_ERR "%s:  context size (%u) exceeds payload "
 		       "max\n", __FUNCTION__, len);
 		length = -ERANGE;
@@ -649,7 +562,7 @@
 	if (length < 0)
 		goto out2;
 
-	if (len > PAYLOAD_SIZE) {
+	if (len > PAGE_SIZE) {
 		length = -ERANGE;
 		goto out3;
 	}
@@ -709,7 +622,7 @@
 			length = rc;
 			goto out3;
 		}
-		if ((length + len) >= PAYLOAD_SIZE) {
+		if ((length + len) >= PAGE_SIZE) {
 			kfree(newcon);
 			length = -ERANGE;
 			goto out3;

Re: [PATCH][LIBFS] Move transaction file ops into libfs + cleanup (update)

[viro]

On Sat, Aug 14, 2004 at 12:55:01PM -0700, Chris Wright wrote:
> Looks nice. I didn't realize you were working on this consolidation too.
> I cooked up a similar patch.  In this case the user loads its inode
> specific write_ops on open, then just uses the generic helpers.  I also
> fully serialized all write/read transactions per inode.  It's lightly
> tested.  If there's anything you like in there, feel free to use it.

This is *wrong*.

First of all, it ties you to ->i_ino values.  Which is OK on a specific
fs, but not in a generic helper functions.

What's more, there is no point in any extra structures here - you are
getting a file-specific method anyway, so you make it ->write() (which
is where behaviour differs) instead of ->open().  Which kills the
need of callbacks.

As a general rule, it's better to provide several helpers and let the
users of interface call them rather than trying to fit everything into
callbacks, flags, etc.

Consider for instance a driver that wants one such request/reply file.
With your scheme it will have to declare two functions - foo_write_op()
and foo_open(), the latter being a boilerplate _and_ declare (for fsck
knows what reason) a single-element array so that foo_open() could pass
array - file->f_dentry->d_inode->i_ino, only to compensate for use of
->i_ino in your helper.

Lots of glue for no good reason _and_ a new function type to deal with.
As opposed to one function (foo_write()) that is a normal instance of
->write() and is actually smaller than your foo_write_op() + foo_open().
No arrays, no magic, no boilerplate code...

Re: [PATCH][LIBFS] Move transaction file ops into libfs + cleanup (update)

[Chris Wright]

* viro@parcelfarce.linux.theplanet.co.uk (viro@parcelfarce.linux.theplanet.co.uk) wrote:
> On Sat, Aug 14, 2004 at 12:55:01PM -0700, Chris Wright wrote:
> > Looks nice. I didn't realize you were working on this consolidation too.
> > I cooked up a similar patch.  In this case the user loads its inode
> > specific write_ops on open, then just uses the generic helpers.  I also
> > fully serialized all write/read transactions per inode.  It's lightly
> > tested.  If there's anything you like in there, feel free to use it.
> 
> This is *wrong*.

Thanks for peeking at it.

> First of all, it ties you to ->i_ino values.  Which is OK on a specific
> fs, but not in a generic helper functions.

Good point.

> What's more, there is no point in any extra structures here - you are
> getting a file-specific method anyway, so you make it ->write() (which
> is where behaviour differs) instead of ->open().  Which kills the
> need of callbacks.
> 
> As a general rule, it's better to provide several helpers and let the
> users of interface call them rather than trying to fit everything into
> callbacks, flags, etc.

Yes, I took too simplistic a view on moving the common code over to
generic.

> Consider for instance a driver that wants one such request/reply file.
> With your scheme it will have to declare two functions - foo_write_op()
> and foo_open(), the latter being a boilerplate _and_ declare (for fsck
> knows what reason) a single-element array so that foo_open() could pass
> array - file->f_dentry->d_inode->i_ino, only to compensate for use of
> ->i_ino in your helper.
> 
> Lots of glue for no good reason _and_ a new function type to deal with.
> As opposed to one function (foo_write()) that is a normal instance of
> ->write() and is actually smaller than your foo_write_op() + foo_open().
> No arrays, no magic, no boilerplate code...

Agreed.  Thanks for the feedback.
-chris

Re: [PATCH][LIBFS] Move transaction file ops into libfs + cleanup (update)

[Andrew Morton]

James Morris <jmorris@redhat.com> wrote:
>
> On Sat, 14 Aug 2004, Francois Romieu wrote:
> 
> > > +static inline void simple_transaction_set(struct file *file, size_t n)
> > > +{
> > > +	struct simple_transaction_argresp *ar = file->private_data;
> > > +	
> > > +	BUG_ON(n > SIMPLE_TRANSACTION_LIMIT);
> > > +	mb();
> > > +	ar->size = n;
> > > +}
> > 
> > Could you add the justification for the 'mb' (or the expected effect on the
> > api) as a comment ?
> 
> This ensures that ar->size will really remain zero until ar->data is ready
> for reading.

I updated the patch to include this important detail in a comment. 
Shouldn't it be an smb_mb()?

Re: [PATCH][LIBFS] Move transaction file ops into libfs + cleanup (update)

[James Morris]

On Sun, 15 Aug 2004, Andrew Morton wrote:

> I updated the patch to include this important detail in a comment. 
> Shouldn't it be an smb_mb()?

Yes, this is SMP specific.  i.e. thread A is writing to ar->data and 
thread B wants to know if ar->data is non-zero to read it.


- James
-- 
James Morris
<jmorris@redhat.com>

Re: [PATCH][LIBFS] Move transaction file ops into libfs + cleanup (update)

[James Morris]

On Sun, 15 Aug 2004, James Morris wrote:

> Yes, this is SMP specific.  i.e. thread A is writing to ar->data and 
> thread B wants to know if ar->data is non-zero to read it.
                                ^^^^  should be 'size'


- James
-- 
James Morris
<jmorris@redhat.com>