Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH] device arguments from lookup)

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH] device arguments from lookup)

[Andries.Brouwer]

>> Opening device files often has interesting side effects.

> Too bad. They can be triggered by similar races between attacker
> changing the type of object (file<->symlink) and backup.

Yes. This is a well-known security problem.
Doing
	stat("file", &s);
	if (action desired) {
		action("file");
	}
is no good because there is a race.
But doing
	fd = open("file", flags);
	fstat(fd, &s);
	if (action desired) {
		f_action(fd);
	}
is no good either because the open() has unknown side effects.
It helps to add flags like O_NONBLOCK and perhaps O_NOCTTY,
but that is not quite good enough.

One would like to have a version of the open() call that was
guaranteed free of side effects, and gave a fd only -
perhaps for stat(), perhaps for ioctl().
This guarantee could perhaps be obtained by omitting the
	f->f_op->open(inode,f);
call in dentry_open() when the open call is
	open("file", O_FDONLY);
Of course it may be that we afterwards decide that fd must
be used, and then it needs upgrading:
	fd = f_open(fd, O_RDWR);

Andries

[Such a construction allows various cleanups.
But no doubt it has problems that I have not yet thought of.]

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH] device arguments from lookup)

[Alexander Viro]

On Sat, 19 May 2001 Andries.Brouwer@cwi.nl wrote:

> One would like to have a version of the open() call that was
> guaranteed free of side effects, and gave a fd only -
> perhaps for stat(), perhaps for ioctl().
> This guarantee could perhaps be obtained by omitting the
> 	f->f_op->open(inode,f);
> call in dentry_open() when the open call is
> 	open("file", O_FDONLY);
> Of course it may be that we afterwards decide that fd must
> be used, and then it needs upgrading:
> 	fd = f_open(fd, O_RDWR);

clone(), walk(), clunk(), stat() and open() ;-) Basically, we can add
unopened descriptors. I.e. no IO until you open it (turning the thing into
opened one), but we can do lookups (move to child), we can clone and
kill them and we can stat them.

It makes tree traversals much easier, but AFAIK nobody had exported that
API directly to userland. Might be a good idea, but it's completely
non-portable...

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH] device arguments from lookup)

[Matthew Wilcox]

On Sat, May 19, 2001 at 12:51:07PM -0400, Alexander Viro wrote:
> clone(), walk(), clunk(), stat() and open() ;-) Basically, we can add
> unopened descriptors. I.e. no IO until you open it (turning the thing into
> opened one), but we can do lookups (move to child), we can clone and
> kill them and we can stat them.

Those who would like a more detailed explanation can find one at
http://plan9.bell-labs.com/sys/man/5/INDEX.html

-- 
Revolutions do not require corporate support.

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH] device arguments from lookup)

[Alexander Viro]

On Sat, 19 May 2001, Matthew Wilcox wrote:

> On Sat, May 19, 2001 at 12:51:07PM -0400, Alexander Viro wrote:
> > clone(), walk(), clunk(), stat() and open() ;-) Basically, we can add
> > unopened descriptors. I.e. no IO until you open it (turning the thing into
> > opened one), but we can do lookups (move to child), we can clone and
> > kill them and we can stat them.
> 
> Those who would like a more detailed explanation can find one at
> http://plan9.bell-labs.com/sys/man/5/INDEX.html

Umm... Yes, it's an allusion to 9P, but no, I'm not serious about exporting
that to userland.

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH] device arguments from lookup)

[Matthew Kirkwood]

On Sat, 19 May 2001 Andries.Brouwer@cwi.nl wrote:

> One would like to have a version of the open() call that was
> guaranteed free of side effects, and gave a fd only -
> perhaps for stat(), perhaps for ioctl().

I did this a while ago, after some discussion.  The
implementation may suck, but I think it's a useful
facility.

http://web.gnu.walfield.org/mail-archive/linux-fsdevel/2000-March/0230.html

Matthew.

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH] device arguments from lookup)

[Andries.Brouwer]

Alexander Viro writes:

> Folks, before you get all excited about cramming side effects
> into open(2), consider ...

I agree completely.

> A lot of stuff relies on the fact that close(open(foo, O_RDONLY))
> is a no-op. Breaking that assumption is a Bad Thing(tm).

Also here I would like to agree. Unfortunately this is false.
Opening device files often has interesting side effects.

Andries

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH] device arguments from lookup)

[Alexander Viro]

On Sat, 19 May 2001 Andries.Brouwer@cwi.nl wrote:

> > A lot of stuff relies on the fact that close(open(foo, O_RDONLY))
> > is a no-op. Breaking that assumption is a Bad Thing(tm).
> 
> Also here I would like to agree. Unfortunately this is false.
> Opening device files often has interesting side effects.

Too bad. They can be triggered by similar races between attacker
changing the type of object (file<->symlink) and backup.

[RFD w/info-PATCH] device arguments from lookup, partion code in userspace

[Ben LaHaise]

Hey folks,

The work-in-progress patch for-demonstration-purposes-only below consists
of 3 major components, and is meant to start discussion about the future
direction of device naming and its interaction block layer.  The main
motivations here are the wasting of minor numbers for partitions, and the
duplication of code between user and kernel space in areas such as
partition detection, uuid location, lvm setup, mount by label, journal
replay, and so on...

1. Generic lookup method and argument parsiing (fs/lookupargs.c)

	This code implements a lookup function which is for demonstration
	purposes used in fs/block_dev.c.  The general idea is to pass
	additional parameters to device drivers on open via a comma
	seperated list of options following the device's name.  Sample
	uses:

		/dev/sda/raw		-> open sda in raw mode.
		/dev/sda/limit=102400	-> open sda with a limit of 100K
		/dev/sda/offset=1024,limit=2048
			-> open a device that gives a view of sda at an
			   offset of 1KB to 2KB

	The arguments are defined in a table (fs/block_dev.c:660), which
	defines the name and type of argument to parse.  This table is
	used at lookup time to determine if an option name is valid
	(resulting in a postive dentry) or invalid.  Potential uses for
	this are numerous: opening a control channel to a device,
	specifying a graphics mode for a framebuffer on open, replacing
	ioctls, .... lots of options.  Please seperate comments on this
	portion from the other parts of the patch.

2. Restricted block device (drivers/block/blkrestrict.c)

	This is a quick-n-dirty implementation of a simple md-like block
	device that adds an offset to sector requests and limits the
	maximum offset on the device.  The idea here is to replace the
	special case minor numbers used for the partitioning code with
	a generic runtime allocated translation node.  The idea will work
	best once its data can be stored in a kdev_t structure.  The API
	for use is simple:

		kdev_t restrict_create_dev(kdev_t dev,
				unsigned long long offset,
				unsigned long long limit)

	The associated cleanup of the startup code is not addressed here.
	Comments on this part (I know the implementation is ugly, talk
	about the ideas please)?

3. Userspace partition code proposal

	Given the above two bits, here's a brief explaination of a
	proposal to move management of the partitioning scheme into
	userspace, along with portions of raid startup, lvm, uuid and
	mount by label code needed for mounting the root filesystem.

	Consider that the device node currently known as /dev/hda5 can
	also be viewed as /dev/hda at offset 512000 with a limit of 10GB.
	With the extensions in fs/block_dev.c, you could replace /dev/hda5
	with /dev/hda/offset=512000,limit=10240000000.  Now, by putting
	the partition parsing code into a libpart and binding mount to a
	libpart, the root filesystem mounting code can be run out of an
	initrd image.  The use of mount gives us the ability to mount
	filesystems by UUID, by label or other exotic schemes without
	having to add any additional code to the kernel.

I'm going to stop writing this now.  I need sleep...

Folks, please let me know your opinions on the ideas presented herein, and
do attempt to keep the bits of code that are useful.  Cheers,

		-ben

[23:34:07] <viro> bcrl: you are sick.
[23:41:13] <viro> bcrl: you _are_ sick.
[23:43:24] <viro> bcrl: you are _fscking_ sick.

here starts v2.4.5-pre3_bdev_naming-A0.diff
diff -urN kernels/2.4/v2.4.5-pre3/Makefile bdev_naming/Makefile
--- kernels/2.4/v2.4.5-pre3/Makefile	Thu May 17 18:09:42 2001
+++ bdev_naming/Makefile	Sat May 19 01:33:39 2001
@@ -1,7 +1,7 @@
 VERSION = 2
 PATCHLEVEL = 4
 SUBLEVEL = 5
-EXTRAVERSION =-pre3
+EXTRAVERSION =-pre3-sick-test

 KERNELRELEASE=$(VERSION).$(PATCHLEVEL).$(SUBLEVEL)$(EXTRAVERSION)

diff -urN kernels/2.4/v2.4.5-pre3/arch/i386/boot/install.sh bdev_naming/arch/i386/boot/install.sh
--- kernels/2.4/v2.4.5-pre3/arch/i386/boot/install.sh	Tue Jan  3 06:57:26 1995
+++ bdev_naming/arch/i386/boot/install.sh	Fri May 18 20:24:36 2001
@@ -21,6 +21,7 @@

 # User may have a custom install script

+if [ -x ~/bin/installkernel ]; then exec ~/bin/installkernel "$@"; fi
 if [ -x /sbin/installkernel ]; then exec /sbin/installkernel "$@"; fi

 # Default install - same as make zlilo
diff -urN kernels/2.4/v2.4.5-pre3/drivers/block/Makefile bdev_naming/drivers/block/Makefile
--- kernels/2.4/v2.4.5-pre3/drivers/block/Makefile	Fri Dec 29 17:07:21 2000
+++ bdev_naming/drivers/block/Makefile	Sat May 19 00:29:08 2001
@@ -12,7 +12,7 @@

 export-objs	:= ll_rw_blk.o blkpg.o loop.o DAC960.o

-obj-y	:= ll_rw_blk.o blkpg.o genhd.o elevator.o
+obj-y	:= ll_rw_blk.o blkpg.o genhd.o elevator.o blkrestrict.o

 obj-$(CONFIG_MAC_FLOPPY)	+= swim3.o
 obj-$(CONFIG_BLK_DEV_FD)	+= floppy.o
diff -urN kernels/2.4/v2.4.5-pre3/drivers/block/blkrestrict.c bdev_naming/drivers/block/blkrestrict.c
--- kernels/2.4/v2.4.5-pre3/drivers/block/blkrestrict.c	Wed Dec 31 19:00:00 1969
+++ bdev_naming/drivers/block/blkrestrict.c	Sat May 19 01:17:36 2001
@@ -0,0 +1,105 @@
+/* driver/block/blkrestrict.c - written by Benjamin LaHaise
+ *	Block device limit enforcer.  Designed to implement partition
+ *	tables under control of other code.
+ *
+ *	Copyright 2001 Red Hat, Inc.
+ *
+ *	This program is free software; you can redistribute it and/or
+ *	modify it under the terms of the GNU General Public License as
+ *	published by the Free Software Foundation; either version 2 of
+ *	the License, or (at your option) any later version.
+ *
+ *	This program is distributed in the hope that it will be useful,
+ *	but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *	MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *	GNU General Public License for more details.
+ *
+ *	You should have received a copy of the GNU General Public License
+ *	along with this program; if not, write to the Free Software
+ *	Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+#include <linux/fs.h>
+#include <linux/blkdev.h>
+#include <linux/init.h>
+
+static char major_name[] = "restrict";
+static unsigned int major_nr;
+static unsigned int minor_nr;	/* next free minor number */
+
+static struct restrict_info {
+	unsigned long	offset;
+	unsigned long	limit;
+	kdev_t		dev;
+} restrict_info[256];	/* FIXME: stupid */
+
+static int restrict_blk_size[256];	/* grr */
+
+kdev_t restrict_create_dev(kdev_t dev, unsigned long long offset, unsigned long long limit)
+{
+	unsigned int minor = minor_nr++;	/* FIXME: overflow/smp/fish */
+	struct restrict_info *info = &restrict_info[minor];
+
+	info->offset = offset / 512;
+	info->limit = limit / 512;
+	info->dev = dev;
+
+	restrict_blk_size[minor] = info->limit - info->offset;
+
+	printk("restrict_create_dev: (0x%02x, 0x%02x) offset=0x%lx limit=0x%lx on (0x%04x)\n", major_nr, minor, info->offset, info->limit, info->dev);	/* FIXME: duh */
+
+	return MKDEV(major_nr, minor);
+}
+
+static int restrict_open(struct inode *inode, struct file *file)
+{
+	return 0;
+}
+
+static int restrict_release(struct inode *inode, struct file *file)
+{
+	return 0;
+}
+
+static int restrict_make_req(request_queue_t *q, int rw, struct buffer_head *bh)
+{
+	struct restrict_info *info = &restrict_info[MINOR(bh->b_rdev)];
+	unsigned long new_sector = bh->b_rsector + info->offset;
+
+	if (new_sector >= info->limit || new_sector < bh->b_rsector) {
+		printk("restrict_make_req: 0x%lx beyond limit on 0x%x (0x%lx,0x%lx)\n", bh->b_rsector, bh->b_rdev, info->offset, info->limit);
+		buffer_IO_error(bh);
+		return 0;
+	}
+
+	bh->b_rdev = info->dev;
+	bh->b_rsector += info->offset;
+
+	return 1;
+}
+
+static struct block_device_operations restrict_bdops = {
+	open:		restrict_open,
+	release:	restrict_release,
+};
+
+static int __init blkrestrict_init(void)
+{
+	major_nr = register_blkdev(0, major_name, &restrict_bdops);
+	if (major_nr < 0)
+		return major_nr;
+
+	printk("blkrestrict_init: got major %u\n", major_nr);
+
+	blk_queue_make_request(BLK_DEFAULT_QUEUE(major_nr), restrict_make_req);
+	blk_size[major_nr] = restrict_blk_size;
+
+	return 0;
+}
+
+static void __exit blkrestrict_exit(void)
+{
+	unregister_blkdev(major_nr, major_name);
+}
+
+module_init(blkrestrict_init);
+module_exit(blkrestrict_exit);
diff -urN kernels/2.4/v2.4.5-pre3/fs/Makefile bdev_naming/fs/Makefile
--- kernels/2.4/v2.4.5-pre3/fs/Makefile	Thu Apr  5 11:53:44 2001
+++ bdev_naming/fs/Makefile	Fri May 18 18:49:49 2001
@@ -12,7 +12,7 @@

 obj-y :=	open.o read_write.o devices.o file_table.o buffer.o \
 		super.o  block_dev.o stat.o exec.o pipe.o namei.o fcntl.o \
-		ioctl.o readdir.o select.o fifo.o locks.o \
+		ioctl.o readdir.o select.o fifo.o locks.o lookupargs.o \
 		dcache.o inode.o attr.o bad_inode.o file.o iobuf.o dnotify.o \
 		filesystems.o

diff -urN kernels/2.4/v2.4.5-pre3/fs/block_dev.c bdev_naming/fs/block_dev.c
--- kernels/2.4/v2.4.5-pre3/fs/block_dev.c	Thu May 17 18:09:42 2001
+++ bdev_naming/fs/block_dev.c	Sat May 19 01:31:51 2001
@@ -14,9 +14,12 @@
 #include <linux/major.h>
 #include <linux/devfs_fs_kernel.h>
 #include <linux/smp_lock.h>
+#include <linux/lookupargs.h>

 #include <asm/uaccess.h>

+extern kdev_t restrict_create_dev(kdev_t dev, unsigned long long offset, unsigned long long limit);
+
 extern int *blk_size[];
 extern int *blksize_size[];

@@ -648,10 +651,52 @@
 	return ret;
 }

+struct blkdev_param {
+	unsigned long long	offset,
+				limit;
+	int			raw;
+};
+
+arg_format_t blkdev_arg_fmt[] = {
+	{ "offset",	Arg_ull,	offsetof(struct blkdev_param, offset) },
+	{ "limit",	Arg_ull,	offsetof(struct blkdev_param, limit) },
+	{ "raw",	Arg_bool,	offsetof(struct blkdev_param, raw) },
+	{ NULL }
+};
+
+static struct dentry *blkdev_lookup(struct inode *inode, struct dentry *dentry)
+{
+	return generic_parse_lookup(inode, dentry, blkdev_arg_fmt);
+}
+
 int blkdev_open(struct inode * inode, struct file * filp)
 {
-	int ret = -ENXIO;
+	int ret;
 	struct block_device *bdev = inode->i_bdev;
+	struct dentry *dentry = filp->f_dentry;
+	struct blkdev_param param = { 0ULL, ~0ULL, 0 };
+
+	if (dentry && dentry->d_parent &&
+	    dentry->d_inode == dentry->d_parent->d_inode) {
+		printk("blkdev_open: args='%*s'\n", dentry->d_name.len, dentry->d_name.name);
+		ret = generic_parse_args(&dentry->d_name, blkdev_arg_fmt, &param);
+		if (ret)
+			return ret;
+		printk("blkdev_open: offset=0x%Lx limit=0x%Lx raw=%d",
+			param.offset, param.limit, param.raw);
+
+		if (param.offset || ~param.limit) {
+			struct inode *old_inode = inode;
+			inode = get_empty_inode();
+			inode->i_rdev = restrict_create_dev(old_inode->i_rdev, param.offset, param.limit);
+			bdev = inode->i_bdev = bdget(inode->i_rdev);
+			filp->f_dentry = d_alloc_root(inode);
+			/* FIXME: error handling, dangling dentry/inode */
+		}
+	}
+
+	ret = -ENXIO;
+
 	down(&bdev->bd_sem);
 	lock_kernel();
 	if (!bdev->bd_op)
@@ -721,6 +766,10 @@
 	write:		block_write,
 	fsync:		block_fsync,
 	ioctl:		blkdev_ioctl,
+};
+
+struct inode_operations def_blk_iops = {
+	lookup:		blkdev_lookup,
 };

 const char * bdevname(kdev_t dev)
diff -urN kernels/2.4/v2.4.5-pre3/fs/devices.c bdev_naming/fs/devices.c
--- kernels/2.4/v2.4.5-pre3/fs/devices.c	Sun Oct  1 23:35:16 2000
+++ bdev_naming/fs/devices.c	Fri May 18 18:41:00 2001
@@ -205,6 +205,7 @@
 		inode->i_rdev = to_kdev_t(rdev);
 	} else if (S_ISBLK(mode)) {
 		inode->i_fop = &def_blk_fops;
+		inode->i_op = &def_blk_iops;
 		inode->i_rdev = to_kdev_t(rdev);
 		inode->i_bdev = bdget(rdev);
 	} else if (S_ISFIFO(mode))
diff -urN kernels/2.4/v2.4.5-pre3/fs/lookupargs.c bdev_naming/fs/lookupargs.c
--- kernels/2.4/v2.4.5-pre3/fs/lookupargs.c	Wed Dec 31 19:00:00 1969
+++ bdev_naming/fs/lookupargs.c	Sat May 19 00:26:31 2001
@@ -0,0 +1,156 @@
+/* fs/lookupargs.c - written by Benjamin LaHaise
+ *	Support for comma seperated argument lists via a lookup method.
+ *	Useful for device drivers and other filesystem entities.
+ *
+ *	Copyright 2001 Red Hat, Inc.
+ *
+ *	This program is free software; you can redistribute it and/or
+ *	modify it under the terms of the GNU General Public License as
+ *	published by the Free Software Foundation; either version 2 of
+ *	the License, or (at your option) any later version.
+ *
+ *	This program is distributed in the hope that it will be useful,
+ *	but WITHOUT ANY WARRANTY; without even the implied warranty of
+ *	MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ *	GNU General Public License for more details.
+ *
+ *	You should have received a copy of the GNU General Public License
+ *	along with this program; if not, write to the Free Software
+ *	Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA
+ */
+#include <linux/fs.h>
+#include <linux/lookupargs.h>
+
+/* Returns the format if arg is in the list of options */
+static const struct parsed_arg_format *find_arg_fmt(
+	const struct parsed_arg *arg, arg_format_t *fmt)
+{
+	if (fmt)
+	for (; fmt->name; fmt++) {
+		const char *opt = fmt->name;
+
+		if (!memcmp(arg->arg_start, opt, arg->arg_len) &&
+		    strlen(opt) == arg->arg_len) {
+			return fmt;
+		}
+	}
+
+	return NULL;
+}
+
+/* TODO: fix it to actually validate the argument */
+int generic_check_arg(const struct parsed_arg *arg, arg_format_t *fmt)
+{
+	return !find_arg_fmt(arg, fmt);
+}
+
+static int parse_arg(const struct qstr *qstr, int offset, struct parsed_arg *arg)
+{
+	const char *str = qstr->name + offset;
+	int left = qstr->len - offset;
+
+	arg->arg_start = NULL;
+	arg->arg_len = 0;
+	arg->option_start = NULL;
+	arg->option_len = 0;
+
+	if (offset < 0)
+		return -1;
+
+	if (left <= 0)
+		return -1;
+
+	/* First off, scan for the argument name -> ends at end of string,
+	 * an equals sign or comma.
+	 */
+	arg->arg_start = str;
+	for (; left > 0 && (*str != '=') && (*str != ',');
+	     left--,str++)
+		;
+
+	arg->arg_len = str - arg->arg_start;
+
+	/* This argument ends if therer's nothing left or we've hit a comma. */
+	if (left <= 0)
+		goto out;
+
+	left--;
+	if (*str++ == ',')
+		goto out;
+
+	/* Second part: scan the option looking for the end: ends at
+	 * end of string or a comma.
+	 */
+	arg->option_start = str;
+	for (; left > 0 && (*str != ',');
+	     left--,str++) {
+		/* Eat the escaped character */
+		if (*str == '\\' && left > 1)
+			left--, str++;
+	}
+
+	arg->option_len = str - arg->arg_start;
+
+out:
+	return str - (const char *)qstr->name;
+}
+
+/* TODO: FIXME: proper range checking!!! */
+static int fill_arg_data(const struct parsed_arg *arg, arg_format_t *fmt, char *data)
+{
+	char *end;
+
+	data += fmt->offset;
+
+	switch (fmt->type) {
+	case Arg_bool:
+		*(int *)data = 1;
+		return 0;
+	case Arg_ull:
+		if (!arg->option_start || !arg->option_len)
+			break;
+		*(unsigned long long *)data = simple_strtoull(arg->option_start, &end, 10);
+		return 0;
+	}
+	return -EINVAL;
+}
+
+int generic_parse_args(const struct qstr *str, arg_format_t *fmt_list, void *data)
+{
+	int ret = 0;
+	for_each_parsed_arg(str) {
+		arg_format_t *fmt = find_arg_fmt(&arg, fmt_list);
+		ret = -EINVAL;
+		if (!fmt)
+			break;
+		ret = fill_arg_data(&arg, fmt, (char *)data);
+		if (ret)
+			break;
+	}
+	return ret;
+}
+
+struct dentry *generic_parse_lookup(
+	struct inode *inode,
+	struct dentry *dentry,
+	arg_format_t *fmt_list)
+{
+	/* Application compatibility: report -ENOTDIR on "." and ".." */
+	if (dentry->d_name.name[0] == '.' &&
+	    ((dentry->d_name.len == 1) ||
+	     (dentry->d_name.name[1] == '.' && dentry->d_name.len == 2)))
+		return ERR_PTR(-ENOTDIR);
+
+	/* Make sure all the arguments are okay */
+	{ for_each_parsed_arg(&dentry->d_name) {
+		arg_format_t *fmt = find_arg_fmt(&arg, fmt_list);
+		if (!fmt || generic_check_arg(&arg, fmt)) {
+			inode = NULL;
+			break;
+		}
+	}}
+
+	d_add(dentry, inode);
+	return NULL;
+}
+
diff -urN kernels/2.4/v2.4.5-pre3/fs/namei.c bdev_naming/fs/namei.c
--- kernels/2.4/v2.4.5-pre3/fs/namei.c	Thu May  3 11:22:16 2001
+++ bdev_naming/fs/namei.c	Fri May 18 22:38:50 2001
@@ -470,7 +470,8 @@
 		 * to be able to know about the current root directory and
 		 * parent relationships.
 		 */
-		if (this.name[0] == '.') switch (this.len) {
+		if (this.name[0] == '.' && S_ISDIR(nd->dentry->d_inode->i_mode))
+			switch (this.len) {
 			default:
 				break;
 			case 2:
@@ -538,7 +539,8 @@
 last_component:
 		if (lookup_flags & LOOKUP_PARENT)
 			goto lookup_parent;
-		if (this.name[0] == '.') switch (this.len) {
+		if (this.name[0] == '.' && S_ISDIR(nd->dentry->d_inode->i_mode))
+			switch (this.len) {
 			default:
 				break;
 			case 2:
@@ -593,7 +595,7 @@
 lookup_parent:
 		nd->last = this;
 		nd->last_type = LAST_NORM;
-		if (this.name[0] != '.')
+		if (this.name[0] != '.' || !S_ISDIR(nd->dentry->d_inode->i_mode))
 			goto return_base;
 		if (this.len == 1)
 			nd->last_type = LAST_DOT;
diff -urN kernels/2.4/v2.4.5-pre3/include/linux/fs.h bdev_naming/include/linux/fs.h
--- kernels/2.4/v2.4.5-pre3/include/linux/fs.h	Thu May 17 18:09:42 2001
+++ bdev_naming/include/linux/fs.h	Fri May 18 20:10:50 2001
@@ -984,6 +984,7 @@
 extern void bdput(struct block_device *);
 extern int blkdev_open(struct inode *, struct file *);
 extern struct file_operations def_blk_fops;
+extern struct inode_operations def_blk_iops;
 extern struct file_operations def_fifo_fops;
 extern int ioctl_by_bdev(struct block_device *, unsigned, unsigned long);
 extern int blkdev_get(struct block_device *, mode_t, unsigned, int);
diff -urN kernels/2.4/v2.4.5-pre3/include/linux/lookupargs.h bdev_naming/include/linux/lookupargs.h
--- kernels/2.4/v2.4.5-pre3/include/linux/lookupargs.h	Wed Dec 31 19:00:00 1969
+++ bdev_naming/include/linux/lookupargs.h	Fri May 18 23:06:56 2001
@@ -0,0 +1,48 @@
+/* include/linux/lookupargs.h
+ */
+struct parsed_arg {
+	const char	*arg_start;
+	const char	*option_start;
+	int		arg_len;
+	int		option_len;
+};
+
+enum parsed_arg_type {
+	Arg_bool,	/* really an int */
+	Arg_ull,
+#if 0
+	//Arg_str,	/* really a char */
+	Arg_c,
+	Arg_uc,
+	Arg_s,
+	Arg_us,
+	Arg_i,
+	Arg_ui,
+	Arg_l,
+	Arg_ul,
+	Arg_ll,
+	Arg_u32,
+	Arg_u64,
+#endif
+};
+
+typedef const struct parsed_arg_format {
+	const char		*name;
+	enum parsed_arg_type	type;
+	size_t			offset;
+} arg_format_t;
+
+#define for_each_parsed_arg(str)\
+	struct parsed_arg arg;	\
+	int __offset = 0;		\
+	while ((__offset = parse_arg((str), __offset, &arg)) > 0)
+
+struct dentry;
+struct inode;
+struct qstr;
+
+extern int generic_parse_args(
+	const struct qstr *str, arg_format_t *fmt, void *data);
+extern struct dentry *generic_parse_lookup(
+	struct inode *inode, struct dentry *dentry, arg_format_t *fmt);
+
diff -urN kernels/2.4/v2.4.5-pre3/include/linux/raid/md_k.h bdev_naming/include/linux/raid/md_k.h
--- kernels/2.4/v2.4.5-pre3/include/linux/raid/md_k.h	Thu May 17 18:09:42 2001
+++ bdev_naming/include/linux/raid/md_k.h	Sat May 19 01:13:18 2001
@@ -36,6 +36,7 @@
 		case RAID5:		return 5;
 	}
 	panic("pers_to_level()");
+	return 0;
 }

 extern inline int level_to_pers (int level)

Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH] device arguments from lookup)

[Alexander Viro]

	Folks, before you get all excited about cramming side effects into
open(2), consider the following case:

1) opening "/dev/zero/start_nuclear_war" has a certain side effect.

2) Local user does the following:
	ln -sf /dev/zero/start_nuclear_war bar
	while true; do
		mkdir foo
		rmdir foo
		ln -sf bar foo
		rm foo
	done

3) Comes the night and root runs (from crontab) updatedb(8). Said beast
includes find(1). With sufficiently bad timing find _will_ be tricked
into attempt to open foo. It will honestly lstat() it, all right. But
there's no way to make sure that subsequent open() on the found directory
will get the same object.

4) Side effect happens...

Similar scenarios can be found for other programs run by/as root, but I
think that the point is obvious - side effects on open() are not a good
idea. Yes, we can play with checking for O_DIRECTORY, yodda, yodda, but
I wouldn't bet a dime on security of a system with such side effects.
A lot of stuff relies on the fact that close(open(foo, O_RDONLY)) is a
no-op. Breaking that assumption is a Bad Thing(tm).

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Abramo Bagnara]

Alexander Viro wrote:
> 
>         Folks, before you get all excited about cramming side effects into
> open(2), consider the following case:
> 
> 1) opening "/dev/zero/start_nuclear_war" has a certain side effect.
> 
> 2) Local user does the following:
>         ln -sf /dev/zero/start_nuclear_war bar
>         while true; do
>                 mkdir foo
>                 rmdir foo
>                 ln -sf bar foo
>                 rm foo
>         done
> 
> 3) Comes the night and root runs (from crontab) updatedb(8). Said beast
> includes find(1). With sufficiently bad timing find _will_ be tricked
> into attempt to open foo. It will honestly lstat() it, all right. But
> there's no way to make sure that subsequent open() on the found directory
> will get the same object.
> 
> 4) Side effect happens...
> 
> Similar scenarios can be found for other programs run by/as root, but I
> think that the point is obvious - side effects on open() are not a good
> idea. Yes, we can play with checking for O_DIRECTORY, yodda, yodda, but
> I wouldn't bet a dime on security of a system with such side effects.
> A lot of stuff relies on the fact that close(open(foo, O_RDONLY)) is a
> no-op. Breaking that assumption is a Bad Thing(tm).

Can't this easily avoided if the needed action is not

< /dev/zero/start_nuclear_war 
or
> /dev/zero/start_nuclear_war

but

echo "I'm evil" > /dev/zero/start_nuclear_war

?

-- 
Abramo Bagnara                       mailto:abramo@alsa-project.org

Opera Unica                          Phone: +39.546.656023
Via Emilia Interna, 140
48014 Castel Bolognese (RA) - Italy

ALSA project               http://www.alsa-project.org
It sounds good!

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Alexander Viro]

On Sat, 19 May 2001, Abramo Bagnara wrote:

> Can't this easily avoided if the needed action is not
> 
> < /dev/zero/start_nuclear_war 
> or
> > /dev/zero/start_nuclear_war
> 
> but
> 
> echo "I'm evil" > /dev/zero/start_nuclear_war

Sure. And that's the right thing to do (not the implied action, that is -
_that_ would be too messy).

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Willem Konynenberg]

Abramo Bagnara wrote:
> Alexander Viro wrote:
> >         Folks, before you get all excited about cramming side effects into
> > open(2), consider the following case:
> > 
> > 1) opening "/dev/zero/start_nuclear_war" has a certain side effect.
[...]
> Can't this easily avoided if the needed action is not
> 
> < /dev/zero/start_nuclear_war 
> or
> > /dev/zero/start_nuclear_war
> 
> but
> 
> echo "I'm evil" > /dev/zero/start_nuclear_war
> 
> ?

Yes, and that is exactly the difference between having a side effect
on the open(2), versus having the effect as a result of a write(2).

Unfortunately, there are already some cases where an open
on a device can have unexpected results.  If you don't want
to get blocked waiting for the carrier-detect signal from the
modem when opening a tty device, you had better specify the
O_NONBLOCK option on the open.  If you don't want this flag
to be active during the actual I/O operations, then you would
have to do an fcntl to clear the O_NONBLOCK again after the open.

So I guess things have already been a bit messy in this
area for many years, even before linux even existed, and
in some cases you can't really do anything about it because
the behaviour is mandated by the applicable standards, like
POSIX, SUS, or whatever.
(The blocking of the open on a tty device is explicitly
 documented in my copy of the X/Open specification.)

Fortunately, blocking the nightly backup program by making it
accidentally open a tty is not quite as catastrophic as having
it start a nuclear war, or format the disks, or something,
just because a user was playing games with symlinks.

-- 
     Willem Konynenberg <wfk@xos.nl>
I am not able rightly to apprehend the kind of confusion of ideas
that could provoke such a question  --  Charles Babbage

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Pavel Machek]

Hi!

> Yes, and that is exactly the difference between having a side effect
> on the open(2), versus having the effect as a result of a write(2).
> 
> Unfortunately, there are already some cases where an open
> on a device can have unexpected results.  If you don't want
> to get blocked waiting for the carrier-detect signal from the
> modem when opening a tty device, you had better specify the
> O_NONBLOCK option on the open.  If you don't want this flag
> to be active during the actual I/O operations, then you would
> have to do an fcntl to clear the O_NONBLOCK again after the open.
> 
> So I guess things have already been a bit messy in this
> area for many years, even before linux even existed, and
> in some cases you can't really do anything about it because
> the behaviour is mandated by the applicable standards, like
> POSIX, SUS, or whatever.
> (The blocking of the open on a tty device is explicitly
>  documented in my copy of the X/Open specification.)
> 
> Fortunately, blocking the nightly backup program by making it
> accidentally open a tty is not quite as catastrophic as having
> it start a nuclear war, or format the disks, or something,
> just because a user was playing games with symlinks.

Maybe not *as* catastrophic, but security hole, anyway. User should
not be able to block system backups.

Small demonstration for bugtraq, anyone?
								Pavel
-- 
I'm pavel@ucw.cz. "In my country we have almost anarchy and I don't care."
Panos Katsaloulis describing me w.r.t. patents at discuss@linmodems.org

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Pavel Machek]

Hi!

> So I guess things have already been a bit messy in this
> area for many years, even before linux even existed, and
> in some cases you can't really do anything about it because
> the behaviour is mandated by the applicable standards, like
> POSIX, SUS, or whatever.
> (The blocking of the open on a tty device is explicitly
>  documented in my copy of the X/Open specification.)

If X/Open documents security hole, then, I guess, X/Open will have to
be changed.
								Pavel
-- 
I'm pavel@ucw.cz. "In my country we have almost anarchy and I don't care."
Panos Katsaloulis describing me w.r.t. patents at discuss@linmodems.org

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH] device arguments from lookup)

[Linus Torvalds]

On Sat, 19 May 2001, Alexander Viro wrote:
>
> 	Folks, before you get all excited about cramming side effects into
> open(2), consider the following case:

Your argument is stupid, imnsho.

Side-effects are perfectly fine if they are _local_ to the file
descriptor. Your example is contrieved and idiotic.

Filename extensions would not replace ioctl's. But they are wonderful ways
to avoid unnecessary binary name-spaces, like the ones we have with
"callout" TTY names, and the one that the fb people had.

For example, do a "ls -l /dev/fd0*", and ponder. Also, realize that we
have these hard-coded names in _addition_ to the magic ioctl to set even
more parameters. These are all stupid and bad, and it would have been a
_lot_ cleaner to be able to do

	open("/dev/fd0/H1440", O_RDWR)..

or

	open("/dev/fd0/HD,18,85", O_RDWD)

to open special non-standard high-density modes.

We already did this, in a very limited and stupid way, by encoding the
minor number and generating a standard naming scheme. We can do the same
thing in a _much_ more generic way by just realizing that we wanted the
open to be name-based in the first place.

These are _not_ side effects. They are very much naming conventions. If I
want to open a the floppy in one of the special extended modes, it makes a
LOT more sense to just open it with the naming, than to open a "generic"
floppy device only to them use a magic and very unreadable ioctl to set
the mode of the device.

In short, I don't buy your arguments for one single second.

		Linus

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH] device arguments from lookup)

[Alexander Viro]

On Sat, 19 May 2001, Linus Torvalds wrote:

> 
> On Sat, 19 May 2001, Alexander Viro wrote:
> >
> > 	Folks, before you get all excited about cramming side effects into
> > open(2), consider the following case:
> 
> Your argument is stupid, imnsho.
> 
> Side-effects are perfectly fine if they are _local_ to the file
> descriptor. Your example is contrieved and idiotic.

Linus, would you _look_ at the uses of open() proposed upthread?

Would you like to argue that close(open("/bin/ls,-l,/etc/passwd", O_RDONLY));
as equivalent of spawn(3) is _not_ contrieved and idiotic?

Would you like to argue that close(open("/dev/md0/..add-...=/foo/bar",O_RDONLY))
as a way to add stripes is not contrieved and idiotic?
 
> These are _not_ side effects. They are very much naming conventions. If I

I would say that both examples above (both really proposed) _are_ side
effects by any definition.

> want to open a the floppy in one of the special extended modes, it makes a
> LOT more sense to just open it with the naming, than to open a "generic"
> floppy device only to them use a magic and very unreadable ioctl to set
> the mode of the device.

Who argues for ioctls? I'm perfectly OK with the stuff that affects future
IO on the descriptor you've opened. That's what open() is for, after all.
However, IMNSHO examples of abusing open() (see above, grep your mailbox if
you think that I'm making it up) posted to that thread _are_ side effects
- ugly as hell, contrieved and bound to be source of exploits.

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Jeff Garzik]

Are we talking about device arguments just for chrdevs and blkdevs? 
(ie. drivers)  or for regular files too?

Speaking about drivers specifically, a controlling miscdev, one per
device or one per group of devices depending on your needs, is a much
more clean solution for passing ioctl-type data.  You are free to come
up with whatever method of communication with the driver is most
efficient for your needs -- without perverting open(2).

Notice also a "metadata miscdev" solves the problem of passing options
on open -- just pass those options to the miscdev before you open it...

metadata miscdevs are a clean solution to what procfs hacks and ioctls
are trying to accomplish.

	Jeff


-- 
Jeff Garzik      | "Do you have to make light of everything?!"
Building 1024    | "I'm extremely serious about nailing your
MandrakeSoft     |  step-daughter, but other than that, yes."

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Jeff Garzik]

Jeff Garzik wrote:
> Notice also a "metadata miscdev" solves the problem of passing options
> on open -- just pass those options to the miscdev before you open it...

to be more clear, "it" == the data device, not the metadata miscdev

-- 
Jeff Garzik      | "Do you have to make light of everything?!"
Building 1024    | "I'm extremely serious about nailing your
MandrakeSoft     |  step-daughter, but other than that, yes."

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Alexander Viro]

On Sat, 19 May 2001, Jeff Garzik wrote:

> Are we talking about device arguments just for chrdevs and blkdevs? 
> (ie. drivers)  or for regular files too?

Let's distinguish between per-fd effects (that's what name in open(name, flags)
is for - you are asking for descriptor and telling what behaviour do you
want for IO on it) and system-wide side effects.

IMO encoding the former into name is perfectly fine, and no write on
another file can be sanely used for that purpose. For the latter, though,
we need to write commands into files and here your miscdevices (or procfs
files, or /dev/foo/ctl - whatever) is needed.

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Oliver Xymoron]

On Sat, 19 May 2001, Alexander Viro wrote:

> Let's distinguish between per-fd effects (that's what name in
> open(name, flags) is for - you are asking for descriptor and telling
> what behaviour do you want for IO on it) and system-wide side effects.
>
> IMO encoding the former into name is perfectly fine, and no write on
> another file can be sanely used for that purpose. For the latter, though,
> we need to write commands into files and here your miscdevices (or procfs
> files, or /dev/foo/ctl - whatever) is needed.

I'm a little skeptical about the necessity of these per-fd effects in the
first place - after all, Plan 9 does without them.  There's only one
floppy drive, yes? No concurrent users of serial ports? The counter that
comes to mind is sound devices supporting multiple opens, but I think
esound and friends are a better solution to that problem.

What I'd like to see:

- An interface for registering an array of related devices (almost always
two: raw and ctl) and their legacy device numbers with a single userspace
callout that does whatever /dev/ creation needs to be done. Thus, naming
and permissions live in user space. No "device node is also a directory"
weirdness which is overkill in the vast majority of cases. No kernel names
or permissions leaking into userspace.

- An unregister_devices that does the same, giving userspace a
chance to persist permissions, etc.

- A userspace program that keeps a mapping of kernel names to /dev/ names,
permissions, etc.

- An autofs hook that does the reverse mapping for running with modules
(possibly calling modprobe directly)

Possible future extension:

- Allow exporting proc as a large collection of devices. Manage /proc in
userspace on a tmpfs.

--
 "Love the dolphins," she advised him. "Write by W.A.S.T.E.."

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[David Lang]

what makes you think it's safe to say there's only one floppy drive?

David Lang

On Mon, 21 May 2001, Oliver Xymoron wrote:

> On Sat, 19 May 2001, Alexander Viro wrote:
>
> > Let's distinguish between per-fd effects (that's what name in
> > open(name, flags) is for - you are asking for descriptor and telling
> > what behaviour do you want for IO on it) and system-wide side effects.
> >
> > IMO encoding the former into name is perfectly fine, and no write on
> > another file can be sanely used for that purpose. For the latter, though,
> > we need to write commands into files and here your miscdevices (or procfs
> > files, or /dev/foo/ctl - whatever) is needed.
>
> I'm a little skeptical about the necessity of these per-fd effects in the
> first place - after all, Plan 9 does without them.  There's only one
> floppy drive, yes? No concurrent users of serial ports? The counter that
> comes to mind is sound devices supporting multiple opens, but I think
> esound and friends are a better solution to that problem.
>
> What I'd like to see:
>
> - An interface for registering an array of related devices (almost always
> two: raw and ctl) and their legacy device numbers with a single userspace
> callout that does whatever /dev/ creation needs to be done. Thus, naming
> and permissions live in user space. No "device node is also a directory"
> weirdness which is overkill in the vast majority of cases. No kernel names
> or permissions leaking into userspace.
>
> - An unregister_devices that does the same, giving userspace a
> chance to persist permissions, etc.
>
> - A userspace program that keeps a mapping of kernel names to /dev/ names,
> permissions, etc.
>
> - An autofs hook that does the reverse mapping for running with modules
> (possibly calling modprobe directly)
>
> Possible future extension:
>
> - Allow exporting proc as a large collection of devices. Manage /proc in
> userspace on a tmpfs.
>
> --
>  "Love the dolphins," she advised him. "Write by W.A.S.T.E.."
>
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Oliver Xymoron]

On Mon, 21 May 2001, David Lang wrote:

> what makes you think it's safe to say there's only one floppy drive?

Read as: it doesn't make sense to have per-fd state on a single floppy
device given that there's only one actual hardware instance associated
with it and multiple openers don't make sense. Opening a floppy at
different densities with magic filenames was an example Linus used earlier
in the thread. Surely there can be more than one drive and more than one
serial port.

> On Mon, 21 May 2001, Oliver Xymoron wrote:
>
> > On Sat, 19 May 2001, Alexander Viro wrote:
> >
> > > Let's distinguish between per-fd effects (that's what name in
> > > open(name, flags) is for - you are asking for descriptor and telling
> > > what behaviour do you want for IO on it) and system-wide side effects.
> > >
> > > IMO encoding the former into name is perfectly fine, and no write on
> > > another file can be sanely used for that purpose. For the latter, though,
> > > we need to write commands into files and here your miscdevices (or procfs
> > > files, or /dev/foo/ctl - whatever) is needed.
> >
> > I'm a little skeptical about the necessity of these per-fd effects in the
> > first place - after all, Plan 9 does without them.  There's only one
> > floppy drive, yes? No concurrent users of serial ports? The counter that
> > comes to mind is sound devices supporting multiple opens, but I think
> > esound and friends are a better solution to that problem.

--
 "Love the dolphins," she advised him. "Write by W.A.S.T.E.."

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Daniel Phillips]

On Monday 21 May 2001 19:16, Oliver Xymoron wrote:
> What I'd like to see:
>
> - An interface for registering an array of related devices (almost
> always two: raw and ctl) and their legacy device numbers with a
> single userspace callout that does whatever /dev/ creation needs to
> be done. Thus, naming and permissions live in user space. No "device
> node is also a directory" weirdness...

Could you be specific about what is weird about it?

> ...which is overkill in the vast majority of cases.

--
Daniel

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Oliver Xymoron]

On Mon, 21 May 2001, Daniel Phillips wrote:

> On Monday 21 May 2001 19:16, Oliver Xymoron wrote:
> > What I'd like to see:
> >
> > - An interface for registering an array of related devices (almost
> > always two: raw and ctl) and their legacy device numbers with a
> > single userspace callout that does whatever /dev/ creation needs to
> > be done. Thus, naming and permissions live in user space. No "device
> > node is also a directory" weirdness...
>
> Could you be specific about what is weird about it?

*boggle*

Without precedent in any other UNIX? Or other operating systems, for that
matter? Can you honestly say it doesn't strike you as weird? It's beating
the least surprise rule with a big stick, fercryinoutloud.

Ok, so technically UNIX directories were once just files. But it's been a
long time since people thought exposing that implementation detail was a
good idea, and anyway, it's the opposite situation (and no longer true on
modern fses).

I don't think it's likely to be even workable. Just consider the directory
entry for a moment - is it going to be marked d or [cb]? If it doesn't
have the directory bit set, Midnight commander won't let me look at it,
and I wouldn't blame cd or ls for complaining. If it does have the 'd' bit
set, I wouldn't blame cp, tar, find, or a million other programs if they
did the wrong thing. They've had 30 years to expect that files aren't
directories. They're going to act weird.

Linus has been kicking this idea around for a couple years now and it's
still a cute solution looking for a problem. It just doesn't belong in
UNIX.

More importantly, there's no call for the weirdness. Look, we've already
got to have a userspace callout for new devices so that we can do config,
firmware downloading, automounting, etc. There's no reason we can't stick
the rest of the dynamic /dev/ magic in userspace with the same mechanism.

--
 "Love the dolphins," she advised him. "Write by W.A.S.T.E.."

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Daniel Phillips]

On Tuesday 22 May 2001 17:24, Oliver Xymoron wrote:
> On Mon, 21 May 2001, Daniel Phillips wrote:
> > On Monday 21 May 2001 19:16, Oliver Xymoron wrote:
> > > What I'd like to see:
> > >
> > > - An interface for registering an array of related devices
> > > (almost always two: raw and ctl) and their legacy device numbers
> > > with a single userspace callout that does whatever /dev/ creation
> > > needs to be done. Thus, naming and permissions live in user
> > > space. No "device node is also a directory" weirdness...
> >
> > Could you be specific about what is weird about it?
>
> *boggle*
>
>[general sense of unease]
>
> I don't think it's likely to be even workable. Just consider the
> directory entry for a moment - is it going to be marked d or [cb]?

It's going to be marked 'd', it's a directory, not a file.

> If it doesn't have the directory bit set, Midnight commander won't
> let me look at it, and I wouldn't blame cd or ls for complaining. If it
> does have the 'd' bit set, I wouldn't blame cp, tar, find, or a
> million other programs if they did the wrong thing. They've had 30
> years to expect that files aren't directories. They're going to act
> weird.

No problem, it's a directory.

> Linus has been kicking this idea around for a couple years now and
> it's still a cute solution looking for a problem. It just doesn't
> belong in UNIX.

Hmm, ok, do we still have any *technical* reasons?

--
Daniel

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Oliver Xymoron]

On Tue, 22 May 2001, Daniel Phillips wrote:

> > I don't think it's likely to be even workable. Just consider the
> > directory entry for a moment - is it going to be marked d or [cb]?
>
> It's going to be marked 'd', it's a directory, not a file.

Are we talking about the same proposal?  The one where I can open /dev/dsp
and /dev/dsp/ctl? But I can still do 'cat /dev/hda > /dev/dsp'?

It's still a file. If it's not a file anymore, it ain't UNIX.

> > If it doesn't have the directory bit set, Midnight commander won't
> > let me look at it, and I wouldn't blame cd or ls for complaining. If it
> > does have the 'd' bit set, I wouldn't blame cp, tar, find, or a
> > million other programs if they did the wrong thing. They've had 30
> > years to expect that files aren't directories. They're going to act
> > weird.
>
> No problem, it's a directory.
>
> > Linus has been kicking this idea around for a couple years now and
> > it's still a cute solution looking for a problem. It just doesn't
> > belong in UNIX.
>
> Hmm, ok, do we still have any *technical* reasons?

If you define *technical* to not include design, sure. Oh, did I
mention unnecessary, solvable in userspace?

--
 "Love the dolphins," she advised him. "Write by W.A.S.T.E.."

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Daniel Phillips]

On Tuesday 22 May 2001 19:49, Oliver Xymoron wrote:
> On Tue, 22 May 2001, Daniel Phillips wrote:
> > > I don't think it's likely to be even workable. Just consider the
> > > directory entry for a moment - is it going to be marked d or
> > > [cb]?
> >
> > It's going to be marked 'd', it's a directory, not a file.
>
> Are we talking about the same proposal?  The one where I can open
> /dev/dsp and /dev/dsp/ctl? But I can still do 'cat /dev/hda >
> /dev/dsp'?

We already support read/write on directories in the VFS, that's not a
problem.

> It's still a file. If it's not a file anymore, it ain't UNIX.

It's a file with the directory bit set, I believe that's UNIX.

> > > If it doesn't have the directory bit set, Midnight commander
> > > won't let me look at it, and I wouldn't blame cd or ls for
> > > complaining. If it does have the 'd' bit set, I wouldn't blame
> > > cp, tar, find, or a million other programs if they did the wrong
> > > thing. They've had 30 years to expect that files aren't
> > > directories. They're going to act weird.
> >
> > No problem, it's a directory.
> >
> > > Linus has been kicking this idea around for a couple years now
> > > and it's still a cute solution looking for a problem. It just
> > > doesn't belong in UNIX.
> >
> > Hmm, ok, do we still have any *technical* reasons?
>
> If you define *technical* to not include design, sure.

Sorry, I don't see what you mean, do you mean the design is
difficult?

> Oh, did I mention unnecessary, solvable in userspace?

That's exactly the point: the generic filesystem allows all the
funny-shaped stuff to be dealt with in user space.  The
filesystem itself is lovely and clean.

BTW, I didn't realize I was reinventing Linus's wheel, this just
seemed very obvious and natural to me.  So I had to believe
there's a technical obstacle somewhere.

Has anyone written code to demonstrate the idea?

--
Daniel

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Edgar Toernig]

Daniel Phillips wrote:
> 
> On Tuesday 22 May 2001 17:24, Oliver Xymoron wrote:
> > On Mon, 21 May 2001, Daniel Phillips wrote:
> > > On Monday 21 May 2001 19:16, Oliver Xymoron wrote:
> > > > What I'd like to see:
> > > >
> > > > - An interface for registering an array of related devices
> > > > (almost always two: raw and ctl) and their legacy device numbers
> > > > with a single userspace callout that does whatever /dev/ creation
> > > > needs to be done. Thus, naming and permissions live in user
> > > > space. No "device node is also a directory" weirdness...
> > >
> > > Could you be specific about what is weird about it?
> >
> > *boggle*
> >
> >[general sense of unease]

I fully agree with Oliver.  It's an abomination.

> > I don't think it's likely to be even workable. Just consider the
> > directory entry for a moment - is it going to be marked d or [cb]?
> 
> It's going to be marked 'd', it's a directory, not a file.

Aha.  So you lose the S_ISCHR/BLK attribute.

> > If it doesn't have the directory bit set, Midnight commander won't
> > let me look at it, and I wouldn't blame cd or ls for complaining. If it
> > does have the 'd' bit set, I wouldn't blame cp, tar, find, or a
> > million other programs if they did the wrong thing. They've had 30
> > years to expect that files aren't directories. They're going to act
> > weird.
> 
> No problem, it's a directory.

Directories are not allowed to be read from/written to.  The VFS may
support it, but it's not (current) UNIX.

> > Linus has been kicking this idea around for a couple years now and
> > it's still a cute solution looking for a problem. It just doesn't
> > belong in UNIX.
> 
> Hmm, ok, do we still have any *technical* reasons?

So with your definition, I have a fs-object that is marked as a directory
but opening it opens a device.  Pretty nice.  How I'm supposed to list
it's contents?  open+readdir?  But the open has nasty side effects.
So you have a directory that you are not allowed to list (because of the
possible side effects) but is allowed to be read from/written to maybe
even issue ioctls to?.  And you call that sane???

IMO the whole idea of arguments following the device name is junk (incl
a "/ctrl").

Just think about the implications of the original "/dev/ttyS0/19200"
suggestion.  It sounds nice and tempting.  But which programs will
benefit.  Which gets confused.  What will be cleaned up.  After some
thoughts you'll find out that it's useless ;-)

And with special "ctrl" devices (ie /dev/ttyS0 and /dev/ttyS0ctrl):
This _may_ work for some kind of devices.  But serial ports are one
example where it simply will _not_.  It requires that you know the
name of the device.  For ttys this is often not the case.  Even if
you manage to get some name for stdin for example - now I should
simply attach a "ctrl" to that name to get a control channel???
At least dangerous.  If I'm lucky I only get an EPERM...

Ciao, ET.

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Alexander Viro]

On Wed, 23 May 2001, Edgar Toernig wrote:

> And with special "ctrl" devices (ie /dev/ttyS0 and /dev/ttyS0ctrl):
> This _may_ work for some kind of devices.  But serial ports are one
> example where it simply will _not_.  It requires that you know the

That's quite funny, you know...

------------------------------------------------------------------------
From: Dennis Ritchie (dmr@bell-labs.com)
Subject: Re: Plan 9 (was Re: Rubouts)
Newsgroups: alt.folklore.computers
Date: 1998/10/12
   
Neil Franklin wrote:
>
> No ioctl()s?
>
> Something like:    echo "38400,8,n,1" > /ioctrl/ttyS0    ?
>
> Now that would be cool.
>
Exactly like that, though it would be /dev/eia80ctl .
No ioctl().

> Is there anyone who has an URL about Plan 9. Code download?
>

 http://plan9.bell-labs.com/plan9


        Dennis
------------------------------------------------------------------------

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Daniel Phillips]

On Wednesday 23 May 2001 06:19, Edgar Toernig wrote:
> IMO the whole idea of arguments following the device name is junk
> (incl a "/ctrl").

You know I didn't suggest that, right?  I find it pretty strange too, but
I'm listening to hear the technical arguments.

> Just think about the implications of the original "/dev/ttyS0/19200"
> suggestion.  It sounds nice and tempting.  But which programs will
> benefit.  Which gets confused.  What will be cleaned up.  After some
> thoughts you'll find out that it's useless ;-)

You know I didn't suggest that either, right?  But I'm with you, I don't
like it at'all, not least because we might change baud rate on the fly.

> And with special "ctrl" devices (ie /dev/ttyS0 and /dev/ttyS0ctrl):
> This _may_ work for some kind of devices.  But serial ports are one
> example where it simply will _not_.  It requires that you know the
> name of the device.  For ttys this is often not the case.
> Even if you manage to get some name for stdin for example - now I 
> should simply attach a "ctrl" to that name to get a control channel???
> At least dangerous.  If I'm lucky I only get an EPERM...

Again, I'll provide a sympathetic ear, but it wasn't my suggestion.

> Ciao, ET.

And you were referring to who?

--
Daniel

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Daniel Phillips]

On Wednesday 23 May 2001 06:19, Edgar Toernig wrote:
> Daniel Phillips wrote:
> > On Tuesday 22 May 2001 17:24, Oliver Xymoron wrote:
> > > On Mon, 21 May 2001, Daniel Phillips wrote:
> > > > On Monday 21 May 2001 19:16, Oliver Xymoron wrote:
> > > > > What I'd like to see:
> > > > >
> > > > > - An interface for registering an array of related devices
> > > > > (almost always two: raw and ctl) and their legacy device
> > > > > numbers with a single userspace callout that does whatever
> > > > > /dev/ creation needs to be done. Thus, naming and permissions
> > > > > live in user space. No "device node is also a directory"
> > > > > weirdness...
> > > >
> > > > Could you be specific about what is weird about it?
> > >
> > > *boggle*
> > >
> > >[general sense of unease]
>
> I fully agree with Oliver.  It's an abomination.

We are, or at least, I am, investigating this question purely on
technical grounds - name calling is a noop.  I'd be happy to find a
real reason why this is a bad idea but so far none has been
presented.

Don't get me wrong, the fact that people I respect have reservations
about the idea does mean something to me, but this still needs to be
investigated properly.  Now on to the technical content...

> > > I don't think it's likely to be even workable. Just consider the
> > > directory entry for a moment - is it going to be marked d or
> > > [cb]?
> >
> > It's going to be marked 'd', it's a directory, not a file.
>
> Aha.  So you lose the S_ISCHR/BLK attribute.

Readdir fills in a directory type, so ls sees it as a directory and does
the right thing.  On the other hand, we know we're on a device 
filesystem so we will next open the name as a regular file, and find
ISCHR or ISBLK: good.

The rule for this filesystem is: if you open with O_DIRECTORY then
directory operations are permitted, nothing else.  If you open without
O_DIRECTORY then directory operations are forbidden (as
usual) and normal device semantics apply.

If there is weirdness anywhere, it's right here with this rule.  The
question is: what if anything breaks?

> > > If it doesn't have the directory bit set, Midnight commander
> > > won't let me look at it, and I wouldn't blame cd or ls for
> > > complaining. If it does have the 'd' bit set, I wouldn't blame
> > > cp, tar, find, or a million other programs if they did the wrong
> > > thing. They've had 30 years to expect that files aren't
> > > directories. They're going to act weird.
> >
> > No problem, it's a directory.
>
> Directories are not allowed to be read from/written to.  The VFS may
> support it, but it's not (current) UNIX.

Here, we obey this rule: if you open it with O_DIRECTORY then you
can't read from or write to it.

> > > Linus has been kicking this idea around for a couple years now
> > > and it's still a cute solution looking for a problem. It just
> > > doesn't belong in UNIX.
> >
> > Hmm, ok, do we still have any *technical* reasons?
>
> So with your definition, I have a fs-object that is marked as a
> directory but opening it opens a device.  Pretty nice..

No, you have to open it without O_DIRECTORY to get your device
fd handle.

> How I'm supposed to list it's contents?  open+readdir?

Nothing breaks here, ls works as it always did.

This is what ls does:

open("foobar", O_RDONLY|O_NONBLOCK|O_LARGEFILE|O_DIRECTORY) = 3
fstat(3, {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
fcntl64(0x3, 0x2, 0x1, 0x2)             = -1 ENOSYS (Function not implemented)
fcntl(3, F_SETFD, FD_CLOEXEC)           = 0
brk(0x805b000)                          = 0x805b000
getdents64(0x3, 0x8058270, 0x1000, 0x26) = -1 ENOSYS (Function not implemented)
getdents(3, /* 2 entries */, 2980)      = 28
getdents(3, /* 0 entries */, 2980)      = 0
close(3)                                = 0

Note that ls doesn't do anything as inconvenient as opening 
foobar as a normal file first, expecting that operation to fail.

> But the open has nasty side effects.
> So you have a directory that you are not allowed
> to list (because of the possible side effects) but is allowed to be
> read from/written to maybe even issue ioctls to?. 

No, you would get side effects only if you open as a regular file.
I'd agree that that sucks, but that's not what we're trying to fix
just now.

> And you call that sane???

I would hope it seems saner now, after the clarification.
Please, if you know something that actually breaks, tell me.

--
Daniel

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Oliver Xymoron]

On Wed, 23 May 2001, Daniel Phillips wrote:

> > > > *boggle*
> > > >
> > > >[general sense of unease]
> >
> > I fully agree with Oliver.  It's an abomination.
>
> We are, or at least, I am, investigating this question purely on
> technical grounds - name calling is a noop.  I'd be happy to find a
> real reason why this is a bad idea but so far none has been
> presented.

I will agree that the thing can be done in principle. You're not going to
find anyone who's going to argue that part. All other things being equal,
I actually think it's a neat idea.

The part that is a problem is people, namely people who write programs.
They've had decades to expect that directories are not also files, and if
they happen to do things like check whether a file is not a directory
before opening it, it's _our fault_ if they get confused.

Consider the recent subtle change to fork() that was reversed because it
uncovered an unforseen bug in bash. The proposed change is not at all
subtle, is entirely without precedent, and is likely to break much.

--
 "Love the dolphins," she advised him. "Write by W.A.S.T.E.."

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Edgar Toernig]

Daniel Phillips wrote:
> On Wednesday 23 May 2001 06:19, Edgar Toernig wrote:
> > Daniel Phillips wrote:
> > > On Tuesday 22 May 2001 17:24, Oliver Xymoron wrote:
> > > > On Mon, 21 May 2001, Daniel Phillips wrote:
> > > > > On Monday 21 May 2001 19:16, Oliver Xymoron wrote:
> > > > > > What I'd like to see:
> > > > > >
> > > > > > - An interface for registering an array of related devices
> > > > > > (almost always two: raw and ctl) and their legacy device
> > > > > > numbers with a single userspace callout that does whatever
> > > > > > /dev/ creation needs to be done. Thus, naming and permissions
> > > > > > live in user space. No "device node is also a directory"
> > > > > > weirdness...
> > > > >
> > > > > Could you be specific about what is weird about it?
> > > >
> > > > *boggle*
> > > >
> > > >[general sense of unease]
> >
> > I fully agree with Oliver.  It's an abomination.
> 
> We are, or at least, I am, investigating this question purely on
> technical grounds - name calling is a noop.

Right.  But sometimes new ideas raise these kind of feelings ;)

> > > It's going to be marked 'd', it's a directory, not a file.
> >
> > Aha.  So you lose the S_ISCHR/BLK attribute.
> 
> Readdir fills in a directory type, so ls sees it as a directory and does
> the right thing.  On the other hand, we know we're on a device
> filesystem so we will next open the name as a regular file, and find
> ISCHR or ISBLK: good.

??? The kernel may know it, but the app?  Or do you really want to
give different stat data on stat(2) and fstat(2)?  These flags are
currently used by archive/backup prgs.  It's a hint that these files
are not regular files and shouldn't be opened for reading.
Having a 'd' would mean that they would really try to enter the
directory and save it's contents.  Don't know what happens in this
case to your "special" files ;-)

> The rule for this filesystem is: if you open with O_DIRECTORY then
> directory operations are permitted, nothing else.  If you open without
> O_DIRECTORY then directory operations are forbidden (as
> usual) and normal device semantics apply.

As usual?  I think you've just changed the rules for O_DIRECTORY.  Up
to now it's only a flag that tells open it should fail if the name
does not refer to a directory.  Nothing else.  It was introduced to
remove a race condition in user space applications.  Especially it
is optional - everything works the same whether you give the flag
or not (except the race avoidance of course).  And there are a lot
of programs that do not use O_DIRECTORY (it's a Linux private flag,
not even mentioned in POSIX).  Every program that does:

	fd = open(foo, O_RDONLY);
	fchdir(fd);
	x = opendir(".")

will break.  And that is POSIX conform.  And I know that there are
programs that use this when recursively scanning directories (avoids
name mangling and repeated name lookups of the directory on later
stat calls).

> > Directories are not allowed to be read from/written to.  The VFS may
> > support it, but it's not (current) UNIX.
> 
> Here, we obey this rule: if you open it with O_DIRECTORY then you
> can't read from or write to it.

IMHO you've just invented opendir(2).

> Nothing breaks here, ls works as it always did.
> 
> This is what ls does:
> 
> open("foobar", O_RDONLY|O_NONBLOCK|O_LARGEFILE|O_DIRECTORY) = 3
> fstat(3, {st_mode=S_IFDIR|0755, st_size=4096, ...}) = 0
> fcntl64(0x3, 0x2, 0x1, 0x2)             = -1 ENOSYS (Function not implemented)
> fcntl(3, F_SETFD, FD_CLOEXEC)           = 0
> brk(0x805b000)                          = 0x805b000
> getdents64(0x3, 0x8058270, 0x1000, 0x26) = -1 ENOSYS (Function not implemented)
> getdents(3, /* 2 entries */, 2980)      = 28
> getdents(3, /* 0 entries */, 2980)      = 0
> close(3)                                = 0
> 
> Note that ls doesn't do anything as inconvenient as opening
> foobar as a normal file first, expecting that operation to fail.

Well, your ls does not work "as it always did".  Here's an strace of
my libc5 system ls:

open(".", O_RDONLY)                     = 3
fcntl(3, F_SETFD, FD_CLOEXEC)           = 0
getdents(3, /* 64 entries */, 4096)     = 1216
getdents(3, /* 9 entries */, 4096)      = 168
getdents(3, /* 0 entries */, 4096)      = 0
close(3)                                = 0

And my find(1) does:

open(".", O_RDONLY)                     = 3
[scan all dirs]
fchdir(3)                               = 0

to return to its initial dir.  Will break too.

> No, you would get side effects only if you open as a regular file.

IMHO your assumption that opening a dir _requires_ O_DIRECTORY is
wrong.  You've put in a new semantic that has not been there and
that will break programs and POSIX conformance.

> Please, if you know something that actually breaks, tell me.

Yeah, see above ;)

Ciao, ET.

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Marko Kreen]

On Thu, May 24, 2001 at 02:23:27AM +0200, Edgar Toernig wrote:
> Daniel Phillips wrote:
> > > > It's going to be marked 'd', it's a directory, not a file.
> > >
> > > Aha.  So you lose the S_ISCHR/BLK attribute.
> > 
> > Readdir fills in a directory type, so ls sees it as a directory and does
> > the right thing.  On the other hand, we know we're on a device
> > filesystem so we will next open the name as a regular file, and find
> > ISCHR or ISBLK: good.
> 
> ??? The kernel may know it, but the app?  Or do you really want to
> give different stat data on stat(2) and fstat(2)?  These flags are
> currently used by archive/backup prgs.  It's a hint that these files
> are not regular files and shouldn't be opened for reading.
> Having a 'd' would mean that they would really try to enter the
> directory and save it's contents.  Don't know what happens in this
> case to your "special" files ;-)

IMHO the CHR/BLK is not needed.  Think of /proc.  In the future,
the backup tools will be told to ignore /dev, that's all.

-- 
marko

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Oliver Xymoron]

On Thu, 24 May 2001, Marko Kreen wrote:

> On Thu, May 24, 2001 at 02:23:27AM +0200, Edgar Toernig wrote:
> > Daniel Phillips wrote:
> > > > > It's going to be marked 'd', it's a directory, not a file.
> > > >
> > > > Aha.  So you lose the S_ISCHR/BLK attribute.
> > >
> > > Readdir fills in a directory type, so ls sees it as a directory and does
> > > the right thing.  On the other hand, we know we're on a device
> > > filesystem so we will next open the name as a regular file, and find
> > > ISCHR or ISBLK: good.
> >
> > ??? The kernel may know it, but the app?  Or do you really want to
> > give different stat data on stat(2) and fstat(2)?  These flags are
> > currently used by archive/backup prgs.  It's a hint that these files
> > are not regular files and shouldn't be opened for reading.
> > Having a 'd' would mean that they would really try to enter the
> > directory and save it's contents.  Don't know what happens in this
> > case to your "special" files ;-)
>
> IMHO the CHR/BLK is not needed.  Think of /proc.  In the future,
> the backup tools will be told to ignore /dev, that's all.

The /dev dir should not be special. At least not to the kernel. I have
device files in places other than /dev, and you probably do too (hint:
anonymous FTP).

--
 "Love the dolphins," she advised him. "Write by W.A.S.T.E.."

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Daniel Phillips]

On Thursday 24 May 2001 02:23, Edgar Toernig wrote:
> Daniel Phillips wrote:
> > > > It's going to be marked 'd', it's a directory, not a file.
> > >
> > > Aha.  So you lose the S_ISCHR/BLK attribute.
> >
> > Readdir fills in a directory type, so ls sees it as a directory and
> > does the right thing.  On the other hand, we know we're on a device
> > filesystem so we will next open the name as a regular file, and
> > find ISCHR or ISBLK: good.
>
> ??? The kernel may know it, but the app?  Or do you really want to
> give different stat data on stat(2) and fstat(2)?  These flags are
> currently used by archive/backup prgs.  It's a hint that these files
> are not regular files and shouldn't be opened for reading.
> Having a 'd' would mean that they would really try to enter the
> directory and save it's contents.  Don't know what happens in this
> case to your "special" files ;-)

I guess that's much like the question 'what happens in proc?'.

Recursively entering the device directory is ok as long as everything
inside it is ok.  I tried zipping /proc/bus -r and what I got is what I'd
expect if I'd cat'ed every non-directory entry.  This is what I
expected.  Maybe it's not right - zipping /proc/kcore is kind of
interesting.  Regardless, we are no worse than proc here.  In fact,
since we don't anticipate putting an elephant like kcore in as a
device property, we're a little nicer to get along with.

Correct me if I'm wrong, but what we learn from the proc example
is that tarring your whole source tree starting at / is not something
you want to do.  Just extend that idea to /dev - however, if you do
it, it will produce pretty reasonable results.

What *won't* happen is, you won't get side effects from opening
your serial ports (you'd have to open them without O_DIRECTORY
to get that) so that seems like a little step forward.

I'm still thinking about some of your other comments.

--
Daniel

--
Daniel

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Edgar Toernig]

Daniel Phillips wrote:
> 
> > > Readdir fills in a directory type, so ls sees it as a directory and
> > > does the right thing.  On the other hand, we know we're on a device
> > > filesystem so we will next open the name as a regular file, and
> > > find ISCHR or ISBLK: good.
> >
> > ??? The kernel may know it, but the app?  Or do you really want to
> > give different stat data on stat(2) and fstat(2)?  These flags are
> > currently used by archive/backup prgs.  It's a hint that these files
> > are not regular files and shouldn't be opened for reading.
> > Having a 'd' would mean that they would really try to enter the
> > directory and save it's contents.  Don't know what happens in this
> > case to your "special" files ;-)
> 
> I guess that's much like the question 'what happens in proc?'.

And that's already bad enough.  Most of the "files" in proc should
be fifos!  And using proc as an excuse to introduce another set of
magic dirs?  No, thanks.

> Correct me if I'm wrong, but what we learn from the proc example
> is that tarring your whole source tree starting at / is not something
> you want to do.

IMHO it would be better to fix proc instead of adding more magic.  At
the moment you have to exclude /proc.  You want to add /dev.  And next?
Exclude all $HOME/dev (in case process name spaces get added)?  Or make
fifos magic too and add all of them to the exclude list?  But there's
no central place for fifos.  So lets add more magic :-(

> What *won't* happen is, you won't get side effects from opening
> your serial ports (you'd have to open them without O_DIRECTORY
> to get that) so that seems like a little step forward.

As already said: depending on O_DIRECTORY breaks POSIX compliance
and that alone should kill this idea...

Over and out, ET.

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Alexander Viro]

On Thu, 24 May 2001, Edgar Toernig wrote:

> > What *won't* happen is, you won't get side effects from opening
> > your serial ports (you'd have to open them without O_DIRECTORY
> > to get that) so that seems like a little step forward.
> 
> As already said: depending on O_DIRECTORY breaks POSIX compliance
> and that alone should kill this idea...

What really kills that idea is the fact that you can trick applications
into opening your serial ports _without_ O_DIRECTORY.

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Daniel Phillips]

On Thursday 24 May 2001 23:26, Alexander Viro wrote:
> On Thu, 24 May 2001, Edgar Toernig wrote:
> > > What *won't* happen is, you won't get side effects from opening
> > > your serial ports (you'd have to open them without O_DIRECTORY
> > > to get that) so that seems like a little step forward.
> >
> > As already said: depending on O_DIRECTORY breaks POSIX compliance
> > and that alone should kill this idea...
>
> What really kills that idea is the fact that you can trick
> applications into opening your serial ports _without_ O_DIRECTORY.

Err, I thought we already had that problem, but worse: an ordinary
ls -l will do it.  This way, we harmlessly list the device's properties 
instead.

--
Daniel

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Daniel Phillips]

On Thursday 24 May 2001 22:59, Edgar Toernig wrote:
> Daniel Phillips wrote:
> > > > Readdir fills in a directory type, so ls sees it as a directory
> > > > and does the right thing.  On the other hand, we know we're on
> > > > a device filesystem so we will next open the name as a regular
> > > > file, and find ISCHR or ISBLK: good.
> > >
> > > ??? The kernel may know it, but the app?  Or do you really want
> > > to give different stat data on stat(2) and fstat(2)?  These flags
> > > are currently used by archive/backup prgs.  It's a hint that
> > > these files are not regular files and shouldn't be opened for
> > > reading. Having a 'd' would mean that they would really try to
> > > enter the directory and save it's contents.  Don't know what
> > > happens in this case to your "special" files ;-)
> >
> > I guess that's much like the question 'what happens in proc?'.
>
> And that's already bad enough.  Most of the "files" in proc should
> be fifos!  And using proc as an excuse to introduce another set of
> magic dirs?  No, thanks.

Wait a second, I thought proc was here to stay.  Wait another
second, device nodes are already magic.  Magic is magic, just
choose your color ;-)

This set of magic dirs is supposed to clean things up, not mess things 
up.  We already saw how the side-effects-on-open problem in ls -l goes 
away.  There's a much bigger problem I'd love to deal with: the 'no 
heirarchy can please everybody' problem.  In database terms, aheirarchy 
is an insufficiently general model for real-world problems, in other 
words, they never worked.  Tables work.  That's where I'm trying to go 
with this, so please bear with me.  This is not just a solution in 
search of a problem.

> > Correct me if I'm wrong, but what we learn from the proc example
> > is that tarring your whole source tree starting at / is not
> > something you want to do.
>
> IMHO it would be better to fix proc instead of adding more magic.  At
> the moment you have to exclude /proc.  You want to add /dev.

Well, actually no, ls -R, tar, zip, etc, work pretty well with the 
scheme I've described.

> And
> next? Exclude all $HOME/dev (in case process name spaces get added)? 
> Or make fifos magic too and add all of them to the exclude list?  But
> there's no central place for fifos.  So lets add more magic :-(

No, no, no, agreed and sometimes magic is good.  It's not deep magic.  
The only new thing here is the interpretation of the O_DIRECTORY flag, 
or rather, the lack of it.

> > What *won't* happen is, you won't get side effects from opening
> > your serial ports (you'd have to open them without O_DIRECTORY
> > to get that) so that seems like a little step forward.
>
> As already said: depending on O_DIRECTORY breaks POSIX compliance
> and that alone should kill this idea...

Thanks, two good points:
  - libc5 will get confused when doing ls in /magicdev
  - POSIX specifically forbids this

I'll put this away until I've specifically dug into both of them.  OK, 
over and out, thanks for your commentary.

/me peruses man pages

Oops, oh wait, there's already another open point: your breakage 
examples both rely on opening ".".  You're right, "." should always be 
a directory and I believe that's enforced by the VFS.  So we don't have 
an example of breakage yet.

--
Daniel

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Edgar Toernig]

Daniel Phillips wrote:
> 
> Oops, oh wait, there's already another open point: your breakage
> examples both rely on opening ".".  You're right, "." should always be
> a directory and I believe that's enforced by the VFS.  So we don't have
> an example of breakage yet.

That's just because I did a simple "ls".  But it doesn't make a
difference.  The magicdevs _are_ directories and

	chdir("magicdev");
	open(".", O_RDONLY);

shouldn't open the device.

Ciao, ET.

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Daniel Phillips]

On Saturday 26 May 2001 05:07, Edgar Toernig wrote:
> Daniel Phillips wrote:
> > Oops, oh wait, there's already another open point: your breakage
> > examples both rely on opening ".".  You're right, "." should always
> > be a directory and I believe that's enforced by the VFS.  So we
> > don't have an example of breakage yet.
>
> That's just because I did a simple "ls".  But it doesn't make a
> difference.  The magicdevs _are_ directories and
>
> 	chdir("magicdev");
> 	open(".", O_RDONLY);
>
> shouldn't open the device.

It won't, the open for "." is handled in the VFS, not the filesystem - 
it will open the directory.  (Without needing to be told it's a 
directory via O_DIRECTORY.)  If you do open("magicdev") you'll get the 
device, because that's handled by magicdevfs.

I'm not claiming there isn't breakage somewhere, just that we didn't 
find it on this attempt.

--
Daniel

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Edgar Toernig]

Daniel Phillips wrote:
> 
> It won't, the open for "." is handled in the VFS, not the filesystem -
> it will open the directory.  (Without needing to be told it's a
> directory via O_DIRECTORY.)  If you do open("magicdev") you'll get the
> device, because that's handled by magicdevfs.

You really mean that "magicdev" is a directory and:

	open("magicdev/.", O_RDONLY);
	open("magicdev", O_RDONLY);

would both succeed but open different objects?

> I'm not claiming there isn't breakage somewhere,

you break UNIX fundamentals.  But I'm quite relieved now because I'm
pretty sure that something like that will never go into the kernel.

Ciao, ET.

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Ben LaHaise]

On Sun, 27 May 2001, Edgar Toernig wrote:

> You really mean that "magicdev" is a directory and:
>
> 	open("magicdev/.", O_RDONLY);

At least for the patch I posted, that would return -ENOTDIR, and exactly
for the reason that not doing so would break find.  I've been convinced
that we really need to be careful which, if any, options are permitted in
this fashion.

		-ben

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Daniel Phillips]

On Sunday 27 May 2001 15:32, Edgar Toernig wrote:
> Daniel Phillips wrote:
> > It won't, the open for "." is handled in the VFS, not the
> > filesystem - it will open the directory.  (Without needing to be
> > told it's a directory via O_DIRECTORY.)  If you do open("magicdev")
> > you'll get the device, because that's handled by magicdevfs.
>
> You really mean that "magicdev" is a directory and:
>
> 	open("magicdev/.", O_RDONLY);
> 	open("magicdev", O_RDONLY);
>
> would both succeed but open different objects?

Yes, and:

        open("magicdev/.", O_RDONLY | O_DIRECTORY);
        open("magicdev", O_RDONLY | O_DIRECTORY);

will both succeed and open the same object.

> > I'm not claiming there isn't breakage somewhere,
>
> you break UNIX fundamentals.  But I'm quite relieved now because I'm
> pretty sure that something like that will never go into the kernel.

OK, I'll take that as "I couldn't find a piece of code that breaks, so 
it's on to the legal issues".

SUS doesn't seem to have a lot to say about this.  The nearest thing to 
a ruling I found was "The special filename dot refers to the directory 
specified by its predecessor".  Which is not the same thing as:

   open("foo", O_RDONLY) == open ("foo/.", O_RDONLY)

I don't know about POSIX (I don't have it: a pox on standards 
organizations that don't make their standards freely available) but SUS 
doesn't seem to forbid this.

--
Daniel

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Marko Kreen]

On Sun, May 27, 2001 at 10:45:17PM +0200, Daniel Phillips wrote:
> On Sunday 27 May 2001 15:32, Edgar Toernig wrote:
> > Daniel Phillips wrote:
> > > I'm not claiming there isn't breakage somewhere,
> >
> > you break UNIX fundamentals.  But I'm quite relieved now because I'm
> > pretty sure that something like that will never go into the kernel.
> 
> OK, I'll take that as "I couldn't find a piece of code that breaks, so 
> it's on to the legal issues".
> 
> SUS doesn't seem to have a lot to say about this.  The nearest thing to 
> a ruling I found was "The special filename dot refers to the directory 
> specified by its predecessor".  Which is not the same thing as:
> 
>    open("foo", O_RDONLY) == open ("foo/.", O_RDONLY)
> 
> I don't know about POSIX (I don't have it: a pox on standards 
> organizations that don't make their standards freely available) but SUS 
> doesn't seem to forbid this.

My question is: Is it needed?  You are advocating quite
non-obvious behaviour on a UNIX-like fs.  Cant the end result
achieved in more obvious manner?

I see at most 3 types of magic files:

1) regular file - nothing special.  Whether it has CHR/BLK set
   or not is irrelevant.

2) file with subdevs.  As 1) but you can acces dev/something
   for subdev 'something'.  Permissions should be probably taken
   from 'dev'.  Ofcourse you cant do 'ls' on the thing.

3) magicdev as directory.  Act as ordinary directory.  Only
   reason is to group devices.

And all those should be manageable by devfsd, so you can tell
devfsd to take subdev and create it as file somewhere else.
So 2) and 3) are more like 'defaults'.

So: is there additional type required with non-obvious file/dir
behaviour mix?

-- 
marko

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Horst von Brand]

Daniel Phillips <phillips@bonn-fries.net> said:
> On Sunday 27 May 2001 15:32, Edgar Toernig wrote:

[...]

> > you break UNIX fundamentals.  But I'm quite relieved now because I'm
> > pretty sure that something like that will never go into the kernel.

> OK, I'll take that as "I couldn't find a piece of code that breaks, so 
> it's on to the legal issues".

It boggles my (perhaps underdeveloped) mind to have things that are files
_and_ directories at the same time. The last time this was discussed was
for handling forks (a la Mac et al) in files, and it was shot down.

> SUS doesn't seem to have a lot to say about this.  The nearest thing to 
> a ruling I found was "The special filename dot refers to the directory 
> specified by its predecessor".  Which is not the same thing as:
> 
>    open("foo", O_RDONLY) == open ("foo/.", O_RDONLY)

It says "foo" and "foo/." are the same _directory_, where "foo" is a
directory as otherwise "foo/<something>" makes no sense, AFAICS. Is there
any mention on a _file_ "bar" and going "bar/" or "bar/<something>"?
-- 
Horst von Brand                             vonbrand@sleipnir.valparaiso.cl
Casilla 9G, Vin~a del Mar, Chile                               +56 32 672616

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Daniel Phillips]

On Monday 28 May 2001 03:26, Horst von Brand wrote:
> Daniel Phillips <phillips@bonn-fries.net> said:
> > On Sunday 27 May 2001 15:32, Edgar Toernig wrote:
>
> [...]
>
> > > you break UNIX fundamentals.  But I'm quite relieved now because
> > > I'm pretty sure that something like that will never go into the
> > > kernel.
> >
> > OK, I'll take that as "I couldn't find a piece of code that breaks,
> > so it's on to the legal issues".
>
> It boggles my (perhaps underdeveloped) mind to have things that are
> files _and_ directories at the same time.

They are not, the device file and the directory are different objects 
that have the same name.  In C, "foo" and "struct foo" can appear in 
the same scope but they are different objects.  This must have seemed 
to be a strange idea at first.  Here we have "foo" (a device) and 
"directory foo" (the device's properties).

When I first saw Linus mention the idea I did a double-take, I thought 
it was a strange idea and my first reaction was, it would break all 
kinds of things.  But when I started examining cases I was unable to 
find any real problems.  When I asked code examples of breakage none of 
the supplied examples survived scrutiny.  Then, when I looked through 
SUS I didn't find any prohibition.

> The last time this was
> discussed was for handling forks (a la Mac et al) in files, and it
> was shot down.

Do you have the subject line?  It might save us some time ;-)

I seem to recall that the fork idea died because it was thought to 
require changes to userspace programs such as tar and find.  The 
magicdev idea doesn't require such changes, none that I've seen so far.

> > SUS doesn't seem to have a lot to say about this.  The nearest
> > thing to a ruling I found was "The special filename dot refers to
> > the directory specified by its predecessor".  Which is not the same
> > thing as:
> >
> >    open("foo", O_RDONLY) == open ("foo/.", O_RDONLY)
>
> It says "foo" and "foo/." are the same _directory_, where "foo" is a
> directory as otherwise "foo/<something>" makes no sense, AFAICS. Is
> there any mention on a _file_ "bar" and going "bar/" or
> "bar/<something>"?

In SUS I didn't find anything, one way or the other.  I don't know 
about POSIX.

--
Daniel

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Horst von Brand]

Daniel Phillips <phillips@bonn-fries.net> said:
> On Monday 28 May 2001 03:26, Horst von Brand wrote:
> > Daniel Phillips <phillips@bonn-fries.net> said:
> > > On Sunday 27 May 2001 15:32, Edgar Toernig wrote:
> >
> > [...]
> >
> > > > you break UNIX fundamentals.  But I'm quite relieved now because
> > > > I'm pretty sure that something like that will never go into the
> > > > kernel.
> > >
> > > OK, I'll take that as "I couldn't find a piece of code that breaks,
> > > so it's on to the legal issues".
> >
> > It boggles my (perhaps underdeveloped) mind to have things that are
> > files _and_ directories at the same time.

> They are not, the device file and the directory are different objects 
> that have the same name.  In C, "foo" and "struct foo" can appear in 
> the same scope but they are different objects.  This must have seemed 
> to be a strange idea at first.  Here we have "foo" (a device) and 
> "directory foo" (the device's properties).

They have the exact same name, how is anybody going to distinguish them?

> When I first saw Linus mention the idea I did a double-take, I thought 
> it was a strange idea and my first reaction was, it would break all 
> kinds of things.  But when I started examining cases I was unable to 
> find any real problems.  When I asked code examples of breakage none of 
> the supplied examples survived scrutiny.  Then, when I looked through 
> SUS I didn't find any prohibition.

I isn't allowed either...

> > The last time this was
> > discussed was for handling forks (a la Mac et al) in files, and it
> > was shot down.
> 
> Do you have the subject line?  It might save us some time ;-)

Nope, sorry.

> I seem to recall that the fork idea died because it was thought to 
> require changes to userspace programs such as tar and find.  The 
> magicdev idea doesn't require such changes, none that I've seen so far.

tar(1) of /dev should blow up in exactly the same way, AFAICS...

Everybody just knows a device is a device, a file is a file, and a
directory is a directory. Standards notwithstanding, this is how things
work, and have worked for a _long_ time; with absolutely no warning that
the assumption might become wrong sometime or be wrong on some strange
beast (you didn't find anything in your search). I'd suspect nobody
bothered to cast this in stone because nobody even considered such a
twisted possibility.

Take it up with somebody on the standards commitees, they (should) have
looked long and hard at the nooks and cranies in the standard, and so are
in a better position to comment than we here are.
-- 
Dr. Horst H. von Brand                       mailto:vonbrand@inf.utfsm.cl
Departamento de Informatica                     Fono: +56 32 654431
Universidad Tecnica Federico Santa Maria              +56 32 654239
Casilla 110-V, Valparaiso, Chile                Fax:  +56 32 797513

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Edgar Toernig]

nitpicking: a system call without side effects would be pretty useless.

Alexander Viro wrote:
> A lot of stuff relies on the fact that close(open(foo, O_RDONLY)) is a
> no-op. Breaking that assumption is a Bad Thing(tm).

That assumption is totally bogus.  Even for regular files you have side
effects (atime); for anything else they're unpredictable.

Ciao, ET.

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Alexander Viro]

On Sun, 20 May 2001, Edgar Toernig wrote:

> That assumption is totally bogus.  Even for regular files you have side
> effects (atime); for anything else they're unpredictable.

That means only one thing: safe backups are possible only in single-user
mode. For values of safe being "not triggering these side effects on
arbitrary files outside of the area you are trying to backup". You can't
pin an object down until you open it. You can check that it's the same
object you think it is, but that will require fstat(). I.e. opening the
thing.

If all effects of open() either disappear on close() or are something you
don't care about - fine. Otherwise you have a problem. On any UNIX.

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Linus Torvalds]

On Sat, 19 May 2001, Alexander Viro wrote:
> 
> On Sun, 20 May 2001, Edgar Toernig wrote:
> 
> > That assumption is totally bogus.  Even for regular files you have side
> > effects (atime); for anything else they're unpredictable.
> 
> That means only one thing: safe backups are possible only in single-user
> mode.

There are some strong arguments that we should have filesystem
"backdoors" for maintenance purposes, including backup. 

You can, of course, so parts of this on a LVM level, and doing backups
with "disk snapshots" may be a valid approach. However, even that is
debatable: there is very little that says that the disk image has to be
up-to-date at any particular point in time, so even with a disk snapshot
capability (which is not necessarily reasonable under all circumstances)
there are arguments for maintenance interfaces.

Thinks like "lazy fsck" (ie fsck while already running the filesystem) and
defragmentation simply is not feasible on a LVM level.

		Linus

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Jeff Garzik]

[-- Attachment #1: Type: text/plain, Size: 1992 bytes --]

Linus Torvalds wrote:
> There are some strong arguments that we should have filesystem
> "backdoors" for maintenance purposes, including backup.

I think I agree with something Al said over IRC, that fs-level snapshots
are preferred over block level snapshots.

fs-level snapshots should become easy if you have a generic transaction
layer.  The OS spits out file ops, which get processed into a set of fs
transactions.  (remember that fs-level stuff like "change this block
bitmap" is also a transaction, just like the more generic "update this
inode's mtime")

Also, I think there should be generic block allocation strategies that
fs's can use.  Implementing fs-specific strategies such as ext2's
readahead or XFS's delayed allocation is not a solution, IMHO, but
working towards solving the real problem.
</ramble>


> You can, of course, so parts of this on a LVM level, and doing backups
> with "disk snapshots" may be a valid approach. However, even that is
> debatable: there is very little that says that the disk image has to be
> up-to-date at any particular point in time, so even with a disk snapshot
> capability (which is not necessarily reasonable under all circumstances)
> there are arguments for maintenance interfaces.

I've been hacking on the attached, a snapshot block device driver, which
doesn't require LVM at all.  (warning: compiled and updated per outside
review, but very alpha...  do not apply)

The point of the driver is to provide a sync point at snapshot time, at
which all metadata and data is flushed to the block device.

My question... is there a fundamental flaw in this plan?  Ideally when
userspace says "start snapshot", the fsync_dev occurs [a
simplification].  At that point, userspace can safely run dump or tar or
whatever on the virtual snapshot device.

-- 
Jeff Garzik      | "Do you have to make light of everything?!"
Building 1024    | "I'm extremely serious about nailing your
MandrakeSoft     |  step-daughter, but other than that, yes."

[-- Attachment #2: snap.patch --]
[-- Type: text/plain, Size: 30762 bytes --]

Index: linux_2_4/drivers/block/Config.in
diff -u linux_2_4/drivers/block/Config.in:1.1.1.44 linux_2_4/drivers/block/Config.in:1.1.1.44.4.1
--- linux_2_4/drivers/block/Config.in:1.1.1.44	Tue May 15 04:43:24 2001
+++ linux_2_4/drivers/block/Config.in	Wed May 16 15:44:59 2001
@@ -46,4 +46,6 @@
 fi
 dep_bool '  Initial RAM disk (initrd) support' CONFIG_BLK_DEV_INITRD $CONFIG_BLK_DEV_RAM
 
+tristate 'Snapshot device support' CONFIG_BLK_DEV_SNAP
+
 endmenu
Index: linux_2_4/drivers/block/Makefile
diff -u linux_2_4/drivers/block/Makefile:1.1.1.46 linux_2_4/drivers/block/Makefile:1.1.1.46.4.1
--- linux_2_4/drivers/block/Makefile:1.1.1.46	Tue May 15 04:43:24 2001
+++ linux_2_4/drivers/block/Makefile	Wed May 16 15:44:59 2001
@@ -31,6 +31,7 @@
 obj-$(CONFIG_BLK_DEV_DAC960)	+= DAC960.o
 
 obj-$(CONFIG_BLK_DEV_NBD)	+= nbd.o
+obj-$(CONFIG_BLK_DEV_SNAP)	+= snap.o
 
 subdir-$(CONFIG_PARIDE) += paride
 
Index: linux_2_4/drivers/block/snap.c
diff -u /dev/null linux_2_4/drivers/block/snap.c:1.1.6.10
--- /dev/null	Sat May 19 17:36:30 2001
+++ linux_2_4/drivers/block/snap.c	Thu May 17 11:48:54 2001
@@ -0,0 +1,1055 @@
+/*
+   Copyright 2001 Jeff Garzik <jgarzik@mandrakesoft.com>
+   Copyright (C) 2000 Jens Axboe <axboe@suse.de>
+  
+   May be copied or modified under the terms of the GNU General Public
+   License.  See linux/COPYING for more information.
+  
+   Several ideas and some code taken from Jens Axboe's pktcdvd.c 0.0.2j.
+  
+   To-Do list:
+   * Write support.  It's easy, and might be useful in isolated circumstances.
+   * Convert MAX_SNAPDEVS to a module parameter.
+   * Wrap use of "%" operator, to prepare for 64-bit-sized blockdevs on 
+     32-bit processors
+  
+ */
+
+#define VERSION_CODE	"v0.5.0-take6  17 May 2001  Jeff Garzik <jgarzik@mandrakesoft.com>"
+#define MODNAME		"snap"
+#define PFX		MODNAME ": "
+#define MAX_SNAPDEVS	16 
+
+#include <linux/module.h>
+#include <linux/kernel.h>
+#include <linux/slab.h>
+#include <linux/errno.h>
+#include <linux/spinlock.h>
+#include <linux/interrupt.h>
+#include <linux/file.h>
+#include <linux/blk.h>
+#include <linux/blkpg.h>
+#include <linux/init.h>
+#include <linux/snap.h>
+#include <asm/uaccess.h>
+
+static int *snap_sizes;
+static int *snap_blksize;
+static int *snap_readahead;
+static struct snap_device *snap_devs;
+static int snap_major = -1;
+static spinlock_t snap_lock = SPIN_LOCK_UNLOCKED;
+
+
+/*
+ * a bit of a kludge, but we want to be able to pass source, log,
+ * or snap dev and get the right one.
+ */
+static struct snap_device *snap_find_dev(kdev_t dev)
+{
+	int i, j;
+	struct snap_device *sd;
+
+	spin_lock(&snap_lock);
+
+	for (i = 0; i < MAX_SNAPDEVS; i++) {
+		sd = &snap_devs[i];
+		if ((sd->src.dev == dev) || (sd->snap_dev == dev))
+			goto out;
+		for (j = 0; j < sd->n_logs; j++)
+			if (sd->logs[j].dev == dev)
+				goto out;
+	}
+	sd = NULL;
+
+out:
+	spin_unlock(&snap_lock);
+	return sd;
+}
+
+static request_queue_t *snap_get_queue(kdev_t dev)
+{
+	struct snap_device *sd = snap_find_dev(dev);
+
+	if (!sd)
+		return NULL;
+	return &sd->q;
+}
+
+/* run through the block bitmap on each log device,
+ * checking the most recently appended log first,
+ * to see if the requested block has been remapped and stored
+ * on a log device.
+ * If block was remapped, return log device index
+ */
+static int snap_find_blk(struct snap_device *sd, unsigned long blocknr)
+{
+	unsigned int i;
+
+	for (i = sd->active_logs - 1; i >= 0; i--) {
+		unsigned long bitmap_blk = blocknr / sd->bits_per_block;
+		unsigned int bitmap_bits = blocknr % sd->bits_per_block;
+		struct buffer_head *bit_bh = bread(sd->src.dev, bitmap_blk, sd->blksz);
+		u8 *buf;
+		unsigned int in_log;
+		if (!bit_bh) {
+			printk(KERN_ERR "%s: cannot read log %u bitmap_blk %lu\n",
+			       sd->name, i, bitmap_blk);
+			return -2;
+		}
+		lock_buffer(bit_bh);
+		buf = bit_bh->b_data;
+		in_log = (buf[bitmap_bits / sizeof(u8)] & (1 << (bitmap_bits % sizeof(u8))));
+		unlock_buffer(bit_bh);
+		brelse(bit_bh);
+		if (in_log)
+			return i;
+	}
+	
+	return -1;
+}
+
+/* we have never stored a block located at src_bh->b_rsector before.
+ * allocate space on a log device, and store it.
+ */
+static int snapshot_blk(struct snap_device *sd, request_queue_t *q,
+		        struct buffer_head *src_bh)
+{
+	unsigned int log;
+	struct buffer_head old_bh_d;
+	struct buffer_head *old_bh = &old_bh_d;
+	struct buffer_head *bit_bh, *map_bh, *data_bh;
+	unsigned long bitmap_blk, map_blk, data_blk;
+	unsigned int bitmap_bits, map_ofs;
+	unsigned long blocknr = src_bh->b_rsector;
+	unsigned long *map;
+	u8 *buf;
+
+	/* get index of last active log */
+	if (sd->active_logs == 0) {
+		log = 0;
+		sd->active_logs++;
+	} else {
+		log = sd->active_logs - 1;
+	}
+	
+	/* if no free blocks in current log, move on to the next log. */
+	if (sd->logs[log].free == 0) {
+	
+		/* if no more logs, end snapshot */
+		if (sd->active_logs == sd->n_logs) {
+			request_queue_t *sq = sd->src.q;
+			spin_lock_irq(&io_request_lock);
+
+			sq->make_request_fn = sd->src.make_request_fn;
+			sd->src.make_request_fn = NULL;
+			clear_bit(SNAP_ACTIVE, &sd->flags);
+
+			spin_unlock_irq(&io_request_lock);
+			printk(KERN_WARNING "%s: device full, ending snapshot\n", sd->name);
+			return q->make_request_fn(q, WRITE, src_bh);
+		}
+
+		sd->active_logs++;
+		log++;
+	}
+
+	/* read old data from source device */
+	/* call ll_rw_blk directly on a custom bh to avoid some conflicts */
+	init_buffer(old_bh, NULL, NULL);
+	atomic_set(&old_bh->b_count, 1);
+	old_bh->b_rdev = sd->src.dev;
+	old_bh->b_rsector = blocknr;
+	old_bh->b_state = 1 << BH_Mapped;
+	old_bh->b_size = sd->blksz;
+	old_bh->b_data = kmalloc(sd->blksz, GFP_KERNEL);
+	if (!old_bh->b_data) {
+		printk(KERN_ERR "%s: memory alloc fail on srcdev blk %lu\n",
+		       sd->name, blocknr);
+		goto end_io;
+	}
+	ll_rw_block(READ, 1, &old_bh);
+	wait_on_buffer(old_bh);
+	lock_buffer(old_bh);
+
+	/* read bitmap block from log device */
+	bitmap_blk = blocknr / sd->bits_per_block;
+	bit_bh = bread(sd->logs[log].dev, bitmap_blk, sd->blksz);
+	if (!bit_bh) {
+		brelse(old_bh);
+		printk(KERN_ERR "%s: cannot read log %u bitmap_blk %lu\n",
+		       sd->name, log, bitmap_blk);
+		goto end_io;
+	}
+	lock_buffer(bit_bh);
+
+	/* read map block from log device */
+	map_blk = sd->map_base + (blocknr / sd->maps_per_block);
+	map_bh = bread(sd->logs[log].dev, map_blk, sd->blksz);
+	if (!map_bh) {
+		brelse(old_bh);
+		unlock_buffer(bit_bh);
+		brelse(bit_bh);
+		printk(KERN_ERR "%s: cannot read log %u map_blk %lu\n",
+		       sd->name, log, map_blk);
+		goto end_io;
+	}
+	lock_buffer(map_bh);
+
+	/* getblk data block from log device */
+	data_blk = sd->data_base + (sd->logs[log].data_blocks - sd->logs[log].free);
+	data_bh = getblk(sd->logs[log].dev, data_blk, sd->blksz);
+	if (!data_bh) {
+		brelse(old_bh);
+		unlock_buffer(bit_bh);
+		brelse(bit_bh);
+		unlock_buffer(map_bh);
+		brelse(map_bh);
+		printk(KERN_ERR "%s: cannot getblk log %u data_blk %lu\n",
+		       sd->name, log, data_blk);
+		goto end_io;
+	}
+	lock_buffer(data_bh);
+
+	/* write snapshot of source block to log device */
+	memcpy(data_bh->b_data, old_bh->b_data, sd->blksz);
+	mark_buffer_dirty(data_bh);
+	mark_buffer_uptodate(data_bh, 1);
+	unlock_buffer(data_bh);
+	brelse(data_bh);
+
+	unlock_buffer(old_bh);
+	brelse(old_bh);
+	kfree(old_bh->b_data);
+	
+	/* update block map on logdev to point to snapshot'd block */
+	map_ofs = blocknr % sd->maps_per_block;
+	map = (unsigned long *) map_bh->b_data;
+	map[map_ofs] = data_blk;
+	mark_buffer_dirty(map_bh);
+	mark_buffer_uptodate(map_bh, 1);
+	unlock_buffer(map_bh);
+	brelse(map_bh);
+	
+	/* update bitmap on logdev to indicate remapped block
+	 * is stored on this log device
+	 */
+	bitmap_bits = blocknr % sd->bits_per_block;
+	buf = bit_bh->b_data;
+	buf[bitmap_bits / sizeof(u8)] |= (1 << (bitmap_bits % sizeof(u8)));
+	mark_buffer_dirty(bit_bh);
+	mark_buffer_uptodate(bit_bh, 1);
+	unlock_buffer(bit_bh);
+	brelse(bit_bh);
+	
+	/* update free count */
+	sd->logs[log].free--;
+	
+	/* finally, pass the write down to the source device */
+	return sd->src.make_request_fn(q, WRITE, src_bh);
+	
+end_io:
+	buffer_IO_error(src_bh);
+	return 0;
+}
+
+/*
+ * our replacement for the source device's make_request_fn
+ */
+static int src_make_request(request_queue_t *q, int rw, struct buffer_head *bh)
+{
+	struct snap_device *sd;
+	int log;
+
+	sd = snap_find_dev(bh->b_rdev);
+	
+	/*
+	 * various sanity checks
+	 */
+
+	if (sd == NULL) {
+		printk(KERN_ERR PFX "src request routed to us by unknown snap device\n");
+		goto end_io;
+	}
+	
+	if (bh->b_size != sd->blksz) {
+		printk(KERN_ERR "%s: wrong bh size\n", sd->name);
+		goto end_io;
+	}
+
+	/* read is easy - simply pass it on through to
+	 * underlying block device
+	 */
+	if (rw == READ || rw == READA)
+		return sd->src.make_request_fn(q, rw, bh);
+
+	/* sanity check */	
+	if (rw != WRITE) {
+		printk(KERN_ERR "%s: unknown rw mode %d\n", sd->name, rw);
+		goto end_io;
+	}
+
+	/* if block was already remapped, let the write proceed
+	 * as-is.  No more work needs to be done on our part.
+	 */
+	log = snap_find_blk(sd, bh->b_rsector);
+	if (log < -1)
+		goto end_io;
+	if (log >= 0)
+		return sd->src.make_request_fn(q, rw, bh);
+
+	/* block was not already remapped, write to a log device */
+	return snapshot_blk(sd, q, bh);
+	
+end_io:
+	buffer_IO_error(bh);
+	return 0;
+}
+
+/*
+ * turns snapshotting on or off, AFTER the device has been set up
+ */
+static int snap_mode(struct snap_device *sd, unsigned int starting)
+{
+	request_queue_t *sq;
+	
+	if (!sd->src.q)
+		return -EINVAL;
+	if (starting && test_bit(SNAP_ACTIVE, &sd->flags))
+		return -EINVAL;
+	if (!starting && !test_bit(SNAP_ACTIVE, &sd->flags))
+		return -EINVAL;
+
+	sd->src.q = sq = blk_get_queue(sd->src.dev);
+	
+	/* flush outstanding I/Os for the source device.
+	 * Note that this is a race -IF- we were attempting
+	 * to ensure no I/Os occur between call to fsync_dev()
+	 * and when the swap make_request functions, below.
+	 * However, we don't care about this race since this
+	 * is just a sync point, so life is good.
+	 */
+	fsync_dev(sd->src.dev);
+
+	/* swap srcdev make_request_fn with our own
+	 */
+	spin_lock_irq(&io_request_lock);
+
+	if (starting) {
+		set_bit(SNAP_ACTIVE, &sd->flags);
+		sd->src.make_request_fn = sq->make_request_fn;
+		sq->make_request_fn = src_make_request;
+	} else {
+		sq->make_request_fn = sd->src.make_request_fn;
+		sd->src.make_request_fn = NULL;
+		clear_bit(SNAP_ACTIVE, &sd->flags);
+	}
+
+	spin_unlock_irq(&io_request_lock);
+
+	printk(KERN_INFO "%s: %sing snapshot of %s\n",
+	       sd->name, starting ? "start" : "end", bdevname(sd->src.dev));
+
+	return 0;
+}
+
+/*
+ * a read has occured on the snapshot blkdev, and we know
+ * the block has been remapped, and we know it is on log
+ * device 'log'.  Remap the read into a read of the log device.
+ */
+static int remap_to_logdev(struct snap_device *sd, unsigned int log,
+			  request_queue_t *q, int rw, struct buffer_head *bh)
+{
+	unsigned long map_blk;
+	unsigned int map_ofs;
+	struct buffer_head *map_bh;
+	unsigned long *map;
+
+	/*
+	 * calc the position in the vector of unsigned long
+	 * block numbers where remapped block number is stored
+	 */
+	map_blk = sd->map_base + (bh->b_rsector / sd->maps_per_block);
+	map_ofs = bh->b_rsector % sd->maps_per_block;
+
+	/* read the block from the vector */
+	map_bh = bread(sd->logs[log].dev, map_blk, sd->blksz);
+	if (!map_bh) {
+		printk(KERN_ERR "%s: unable to read log %u block %lu\n",
+		       sd->name, log, map_blk);
+		goto end_io;
+	}
+	lock_buffer(map_bh);
+
+	/* read remapped block number from vector */
+	map = (unsigned long *) map_bh->b_data;
+	map_blk = map[map_ofs];
+	unlock_buffer(map_bh);
+	brelse(map_bh);
+
+	/* do the remap */
+	bh->b_rdev = sd->logs[log].dev;
+	bh->b_rsector = map_blk;
+	return 1;
+
+end_io:
+	buffer_IO_error(bh);
+	return 0;
+}
+
+/*
+ * the make_request_fn for our virtual snapshot blkdev.
+ * All reads are remapped to the source device or a log device,
+ * after some I/O occurs to device where to remap the read.
+ * Writes are not currently supported.
+ */
+static int snap_make_request(request_queue_t *q, int rw, struct buffer_head *bh)
+{
+	struct snap_device *sd;
+	int log;
+
+	if (MINOR(bh->b_rdev) >= MAX_SNAPDEVS) {
+		printk(KERN_ERR PFX "%s out of range\n", kdevname(bh->b_rdev));
+		goto end_io;
+	}
+
+	sd = &snap_devs[MINOR(bh->b_rdev)];
+
+	/*
+	 * various sanity checks
+	 */
+
+	if (!sd->src.dev) {
+		printk(KERN_ERR "%s: request received for non-active sd\n", sd->name);
+		goto end_io;
+	}
+
+	if (rw != READ && rw != READA) {
+		printk(KERN_ERR "%s: non-READ[A] request\n", sd->name);
+		goto end_io;
+	}
+
+	if (bh->b_size != sd->blksz) {
+		printk(KERN_ERR "%s: wrong bh size\n", sd->name);
+		goto end_io;
+	}
+
+	log = snap_find_blk(sd, bh->b_rsector);
+	if (log < -1)
+		goto end_io;
+	if (log >= 0)
+		return remap_to_logdev(sd, log, q, rw, bh);
+	
+	/*
+	 * ok, the block was not remapped onto a log device,
+	 * so remap the request to request from the original
+	 * source device.  Since block numbers are the same
+	 * on the virtual snapdev and the srcdev, we need
+	 * only to change the device to which the read request
+	 * is targetted.
+	 */
+	bh->b_rdev = sd->src.dev;
+	return 1;
+
+end_io:
+	buffer_IO_error(bh);
+	return 0;
+}
+
+/*
+ * initialize the request queue for the snapshot blkdev
+ */
+static void snap_init_queue(struct snap_device *sd)
+{
+	request_queue_t *q = &sd->q;
+
+	blk_queue_make_request(q, snap_make_request);
+	blk_queue_headactive(q, 0);
+}
+
+/**********************************************************************
+ *
+ * Block device ioctl operation and associated functions.
+ *
+ */
+ 
+/* initialize a log device.  all we need to do is zero the bitmap
+ * that appears at the beginning of the disk
+ */
+static void snap_mkfs (struct snap_device *sd, unsigned int logidx,
+		       unsigned int bitmap_blocks)
+{
+	unsigned int i, blksz = sd->blksz;
+	
+	for (i = 0; i < bitmap_blocks; i++) {
+		struct buffer_head *bh = getblk(sd->logs[i].dev, i, blksz);
+		if (bh) {
+			lock_buffer(bh);
+			memset(bh->b_data, 0, blksz);
+			mark_buffer_dirty(bh);
+			mark_buffer_uptodate(bh, 1);
+			unlock_buffer(bh);
+			brelse(bh);
+		} else {
+			printk(KERN_ERR "%s: cannot get blk %u\n", sd->name, i);
+		}
+	}
+}
+
+/*
+ * the source and log devices have been opened and sanity-checked
+ * at this point.  initialize 'sd' based on the information provided
+ */
+static int snap_new_dev(struct snap_device *sd,
+			kdev_t src_dev, struct block_device *src_bdev,
+			unsigned int n_log_fds, struct snap_dev_init_info *sdii)
+{
+	unsigned int i;
+	unsigned long map_blocks, bitmap_blocks;
+	int ret;
+	void *log_buf;
+
+	MOD_INC_USE_COUNT;
+
+	log_buf = kmalloc(sizeof(struct snap_log) * n_log_fds, GFP_KERNEL);
+	if (!log_buf) {
+		ret = -ENOMEM;
+		goto out_bufs;
+	}
+
+	spin_lock(&snap_lock);
+
+	memset(sd, 0, sizeof(struct snap_device));
+
+	sd->src.dev = src_dev;
+	sd->src.q = blk_get_queue (sd->src.dev);
+	sd->blksz = blksize_size[MAJOR(src_dev)][MINOR(src_dev)];
+	sd->src.bdev = src_bdev;
+	sd->src.blocks = blk_size[MAJOR(src_dev)][MINOR(src_dev)];
+	
+	sd->n_logs = n_log_fds;
+	sd->logs = log_buf;
+	memset(sd->logs, 0, sizeof(struct snap_log) * n_log_fds);
+
+	sd->bits_per_block = sd->blksz * 8;
+	sd->maps_per_block = sd->blksz / (sizeof(unsigned long) * 2);
+
+	bitmap_blocks = sd->src.blocks / sd->bits_per_block;
+	if (sd->src.blocks % sd->bits_per_block)
+		bitmap_blocks++;
+
+	map_blocks = sd->src.blocks / sd->maps_per_block;
+	if (sd->src.blocks % sd->maps_per_block)
+		map_blocks++;
+	
+	sd->map_base = bitmap_blocks;
+	sd->data_base = bitmap_blocks + map_blocks;
+
+	for (i = 0; i < n_log_fds; i++) {
+		kdev_t logdev = sdii[i].inode->i_rdev;
+		sd->logs[i].dev = logdev;
+		sd->logs[i].size = blk_size[MAJOR(logdev)][MINOR(logdev)];
+
+		if (sd->logs[i].size < (sd->data_base * 2)) {
+			ret = -EINVAL;
+			goto out_free;
+		}
+
+		sd->logs[i].data_blocks =
+		sd->logs[i].free =
+			sd->logs[i].size - bitmap_blocks - map_blocks;
+		
+		set_blocksize(logdev, sd->blksz);
+	}
+
+	sd->snap_dev = MKDEV(snap_major, i);
+	sprintf(sd->name, "snap%d", i);
+	atomic_set(&sd->refcnt, 0);
+
+	spin_unlock(&snap_lock);
+
+	snap_init_queue(sd);
+
+	for (i = 0; i < n_log_fds; i++)
+		snap_mkfs (sd, i, bitmap_blocks);
+
+	DPRINTK(PFX "dev %s sucessfully registered\n", sd->name);
+	return 0;
+
+out_free:
+	spin_unlock(&snap_lock);
+	kfree(sd->logs);
+out_bufs:
+	MOD_DEC_USE_COUNT;
+	return ret;
+}
+
+/* clean up in the event of snap_setup_dev error exit */
+static void snap_setup_dev_cleanup (unsigned int n_log_fds, struct file *src,
+				    struct snap_dev_init_info *sdii)
+{
+	unsigned int i;
+
+	if (sdii) {
+		for (i = 0; i < n_log_fds; i++)
+			if (sdii[i].file) {
+				blkdev_put(sdii[i].inode->i_bdev, BDEV_FILE);
+				fput(sdii[i].file);
+			}
+		kfree(sdii);
+	}
+
+	if (src) {
+		blkdev_put(src->f_dentry->d_inode->i_bdev, BDEV_FILE);
+		fput(src);
+	}
+}
+
+/*
+ * called from ioctl.  verifies the passed file descriptors to
+ * be open and valid block devices, then called snap_new_dev
+ * to actually initialize the device
+ */
+static int snap_setup_dev(struct snap_device *sd, struct snap_setup *setup)
+{
+	struct snap_dev_init_info *sdii = NULL;
+	struct inode *inode_src;
+	struct file *src;
+	int ret;
+	unsigned int i, n_log_fds = setup->n_log_fds;
+
+	if ((src = fget(setup->src_fd)) == NULL) {
+		printk(KERN_ERR "%s: bad file descriptor %d passed\n", sd->name, setup->src_fd);
+		return -EBADF;
+	}
+	if ((inode_src = src->f_dentry->d_inode) == NULL) {
+		printk(PFX "huh? file descriptor %d contains no inode?\n", setup->src_fd);
+		fput(src);
+		return -EINVAL;
+	}
+	if (!S_ISBLK(inode_src->i_mode)) {
+		printk(PFX "device is not a block device (duh)\n");
+		fput(src);
+		return -ENOTBLK;
+	}
+	if (snap_find_dev(inode_src->i_rdev)) {
+		printk(PFX "source device associated with another snapshot dev\n");
+		fput(src);
+		return -EBUSY;
+	}
+	ret = blkdev_get(inode_src->i_bdev, src->f_mode, src->f_flags, BDEV_FILE);
+	if (ret) {
+		fput(src);
+		return ret;
+	}
+
+	sdii = kmalloc(sizeof(*sdii) * n_log_fds, GFP_KERNEL);
+	if (!sdii) {
+		ret = -ENOMEM;
+		goto out;
+	}
+	memset(sdii, 0, sizeof(*sdii) * n_log_fds);
+
+	for (i = 0; i < n_log_fds; i++) {
+		struct file *logf;
+		struct inode *logi;
+
+		logf = sdii[i].file = fget(setup->log_fd[i]);
+		if (!logf) {
+			printk(PFX "bad file descriptor %d passed\n",
+			       setup->log_fd[i]);
+			ret = -EBADF;
+			goto out;
+		}
+		logi = sdii[i].inode = sdii[i].file->f_dentry->d_inode;
+		if (!logi) {
+			printk(PFX "huh? file descriptor %d contains no inode?\n",
+			       setup->log_fd[i]);
+			ret = -EINVAL;
+			goto out;
+		}
+		if (!S_ISBLK(logi->i_mode)) {
+			printk(PFX "device is not a block device\n");
+			ret = -ENOTBLK;
+			goto out;
+		}
+		if (snap_find_dev(logi->i_rdev)) {
+			printk(PFX "log device %d associated with another snapshot dev\n", i);
+			ret = -EBUSY;
+			goto out;
+		}
+		if (IS_RDONLY(logi)) {
+			printk(PFX "Can't write to read-only dev\n");
+			ret = -EROFS;
+			goto out;
+		}
+		ret = blkdev_get(logi->i_bdev, logf->f_mode,
+				 logf->f_flags, BDEV_FILE);
+		if (ret)
+			goto out;
+	}
+	
+	if ((ret = snap_new_dev(sd, inode_src->i_rdev, inode_src->i_bdev,
+				n_log_fds, sdii))) {
+		printk(PFX "all booked up\n");
+		goto out;
+	}
+	
+	sd->src.dentry = dget(src->f_dentry);
+	for (i = 0; i < n_log_fds; i++)
+		sd->logs[i].dentry = dget(sdii[i].file->f_dentry);
+	atomic_inc(&sd->refcnt);
+
+	ret = 0;
+out:
+	snap_setup_dev_cleanup(n_log_fds, src, sdii);
+	return ret;
+}
+
+/*
+ * called from ioctl.  tears down the currently
+ * setup snapshot device, and closes all open resources
+ */
+static int snap_remove_dev(struct snap_device *sd)
+{
+	unsigned int i;
+
+	/* terminate any on-going snapshot, if any */
+	snap_mode(sd, 0);
+
+	blkdev_put(sd->src.dentry->d_inode->i_bdev, BDEV_FILE);
+	dput(sd->src.dentry);
+
+	for (i = 0; i < sd->n_logs; i++) {
+		blkdev_put(sd->logs[i].dentry->d_inode->i_bdev, BDEV_FILE);
+		dput(sd->logs[i].dentry);
+		invalidate_buffers(sd->logs[i].dev);
+	}
+	kfree(sd->logs);
+
+	invalidate_buffers(sd->snap_dev);
+
+	for (i = 0; i < sd->n_logs; i++)
+		blk_cleanup_queue(blk_get_queue(sd->logs[i].dev));
+	blk_cleanup_queue(blk_get_queue(sd->src.dev));
+
+	DPRINTK(PFX "dev %s unregistered\n", sd->name);
+
+	memset(sd, 0, sizeof(struct snap_device));
+
+	MOD_DEC_USE_COUNT;
+	return 0;
+}
+
+static int snap_ioctl(struct inode *inode, struct file *file,
+		     unsigned int cmd, unsigned long arg)
+{
+	struct snap_device *sd;
+	int err;
+
+	if (MINOR(inode->i_rdev) >= MAX_SNAPDEVS)
+		return -EINVAL;
+
+	sd = &snap_devs[MINOR(inode->i_rdev)];
+
+	if ((cmd != SNAP_SETUP_DEV) && !sd->src.dev) {
+		DPRINTK(PFX "dev not setup\n");
+		return -ENXIO;
+	}
+
+	switch (cmd) {
+	case SNAP_GET_STATS:
+		if (copy_to_user(&arg, &sd->stats, sizeof(struct snap_stats)))
+			return -EFAULT;
+
+	case SNAP_SETUP_DEV: {
+		struct snap_setup se, *sp;
+		unsigned int alloc_size;
+		
+		if (!capable(CAP_SYS_ADMIN))
+			return -EPERM;
+		if (sd->src.dev) {
+			printk(PFX "dev already setup\n");
+			return -EBUSY;
+		}
+		if (copy_from_user(&se, &arg, sizeof(se)))
+			return -EFAULT;
+		if (se.n_log_fds == 0 || se.n_log_fds > MAX_LOGDEVS)
+			return -EINVAL;
+		alloc_size = sizeof(se) + (sizeof(int) * se.n_log_fds);
+		sp = kmalloc(alloc_size, GFP_KERNEL);
+		if (!sp)
+			return -ENOMEM;
+		if (copy_from_user(sp, &arg, alloc_size)) {
+			kfree(sp);
+			return -EFAULT;
+		}
+		err = snap_setup_dev(sd, sp);
+		kfree(sp);
+		return err;
+	}
+
+	case SNAP_TEARDOWN_DEV:
+		if (!capable(CAP_SYS_ADMIN))
+			return -EPERM;
+		fsync_dev(sd->src.dev); /* HACK! minimize, but not close, race */
+		if (atomic_read(&sd->refcnt) != 1)
+			return -EBUSY;
+		return snap_remove_dev(sd);
+
+	case SNAP_START:
+		return snap_mode(sd, 1);
+	case SNAP_END:
+		return snap_mode(sd, 0);
+		
+	case BLKGETSIZE:
+		return put_user(blk_size[snap_major][MINOR(inode->i_rdev)] << 1, (long *)arg);
+
+	case BLKROSET:
+	case BLKROGET:
+	case BLKSSZGET:
+	case BLKRASET:
+	case BLKRAGET:
+	case BLKFLSBUF:
+		return blk_ioctl(inode->i_rdev, cmd, arg);
+
+	/*
+	 * forward all other ioctls to src blkdev
+	 */
+	default:
+		return ioctl_by_bdev(sd->src.bdev, cmd, arg);
+	}
+
+	return 0;
+}
+
+/**********************************************************************
+ *
+ * Block device operations (open/close/check_media_change),
+ * and associated functions.
+ *
+ */
+ 
+static inline void snap_mark_readonly(struct snap_device *sd, int on)
+{
+	if (on)
+		set_bit(SNAP_READONLY, &sd->flags);
+	else
+		clear_bit(SNAP_READONLY, &sd->flags);
+}
+
+static int snap_open_dev(struct snap_device *sd, int write)
+{
+	unsigned long dev_size;
+
+	if (!sd->src.dev)
+		return 0;
+
+	dev_size = blk_size[MAJOR(sd->src.dev)][MINOR(sd->src.dev)];
+	snap_sizes[MINOR(sd->snap_dev)] = dev_size;
+
+	if (write) {
+#if 0 /* no write support */
+		if ((ret = snap_open_write(sd)))
+			return ret;
+		snap_mark_readonly(sd, 0);
+#else
+		return -EINVAL;
+#endif
+	} else {
+		snap_mark_readonly(sd, 1);
+	}
+
+	if (write)
+		printk(PFX "%lukB available on disc\n", dev_size);
+
+	return 0;
+}
+
+static int snap_open(struct inode *inode, struct file *file)
+{
+	struct snap_device *sd = NULL;
+	int ret;
+
+	VPRINTK(PFX "entering open\n");
+
+	/* remove when write mode is supported */
+	if (file->f_mode & FMODE_WRITE) {
+		printk(PFX "write mode not supported\n");
+		return -EINVAL;
+	}
+
+	MOD_INC_USE_COUNT;
+
+	if (MINOR(inode->i_rdev) >= MAX_SNAPDEVS) {
+		printk(PFX "max %d snapdevs supported\n", MAX_SNAPDEVS);
+		ret = -ENODEV;
+		goto out;
+	}
+
+	/*
+	 * either device is not configured, or pktsetup is old and doesn't
+	 * use O_CREAT to create device
+	 */
+	sd = &snap_devs[MINOR(inode->i_rdev)];
+	if (!sd->src.dev && !(file->f_flags & O_CREAT)) {
+		VPRINTK(PFX "not configured and O_CREAT not set\n");
+		ret = -ENXIO;
+		goto out;
+	}
+
+	atomic_inc(&sd->refcnt);
+	if ((atomic_read(&sd->refcnt) > 1) && (file->f_mode & FMODE_WRITE)) {
+		VPRINTK(PFX "busy open for write\n");
+		ret = -EBUSY;
+		goto out_dec;
+	}
+
+	ret = snap_open_dev(sd, file->f_mode & FMODE_WRITE);
+	if (ret)
+		goto out_dec;
+
+	/*
+	 * needed here as well, since ext2 (among others) may change
+	 * the blocksize at mount time
+	 */
+	set_blocksize(sd->snap_dev, sd->blksz);
+	return 0;
+
+out_dec:
+	atomic_dec(&sd->refcnt);
+out:
+	VPRINTK(PFX "failed open (%d)\n", ret);
+	MOD_DEC_USE_COUNT;
+	return ret;
+}
+
+static void snap_release_dev(struct snap_device *sd)
+{
+	fsync_dev(sd->snap_dev);
+	invalidate_buffers(sd->snap_dev);
+
+	atomic_dec(&sd->refcnt);
+}
+
+static int snap_close(struct inode *inode, struct file *file)
+{
+	struct snap_device *sd = &snap_devs[MINOR(inode->i_rdev)];
+
+	if (sd->src.dev)
+		snap_release_dev(sd);
+
+	MOD_DEC_USE_COUNT;
+	return 0;
+}
+
+/* FIXME: -really- handle media change, by disabling active snapshot
+ * if source media changes, or handle fatal errors if target media
+ * changes
+ */
+static int snap_media_change(kdev_t dev)
+{
+	struct snap_device *sd = &snap_devs[MINOR(dev)];
+	return sd->src.bdev->bd_op->check_media_change(dev);
+}
+
+static struct block_device_operations snap_ops = {
+	open:			snap_open,
+	release:		snap_close,
+	ioctl:			snap_ioctl,
+	check_media_change:	snap_media_change,
+};
+
+/**********************************************************************
+ *
+ * Module initializations and cleanup
+ *
+ */
+ 
+static inline void snap_cleanup (void)
+{
+	if (snap_devs)
+		kfree(snap_devs);
+	if (snap_sizes)
+		kfree(snap_sizes);
+	if (snap_blksize)
+		kfree(snap_blksize);
+	if (snap_readahead)
+		kfree(snap_readahead);
+
+	snap_devs = NULL;
+	snap_sizes = NULL;
+	snap_blksize = NULL;
+	snap_readahead = NULL;
+	blk_size[snap_major] = NULL;
+	blksize_size[snap_major] = NULL;
+	max_readahead[snap_major] = NULL;
+	blk_dev[snap_major].queue = NULL;
+}
+
+static int __init snap_init(void)
+{
+	snap_major = devfs_register_blkdev(0, MODNAME, &snap_ops);
+	if (snap_major <= 0) { /* zero is invalid value b/c we need a major */
+		printk("unable to register snap device\n");
+		return -EIO;
+	}
+	devfs_register(NULL, MODNAME, 0, DEVFS_FL_DEFAULT, snap_major,
+		       S_IFBLK | S_IRUSR | S_IWUSR, &snap_ops, NULL);
+
+	snap_sizes = kmalloc(MAX_SNAPDEVS * sizeof(int), GFP_KERNEL);
+	if (snap_sizes == NULL)
+		goto err;
+
+	snap_blksize = kmalloc(MAX_SNAPDEVS * sizeof(int), GFP_KERNEL);
+	if (snap_blksize == NULL)
+		goto err;
+
+	snap_readahead = kmalloc(MAX_SNAPDEVS * sizeof(int), GFP_KERNEL);
+	if (snap_readahead == NULL)
+		goto err;
+
+	snap_devs = kmalloc(MAX_SNAPDEVS * sizeof(struct snap_device), GFP_KERNEL);
+	if (snap_devs == NULL)
+		goto err;
+
+	memset(snap_devs, 0, MAX_SNAPDEVS * sizeof(struct snap_device));
+	memset(snap_sizes, 0, MAX_SNAPDEVS * sizeof(int));
+	memset(snap_blksize, 0, MAX_SNAPDEVS * sizeof(int));
+	memset(snap_readahead, 0, MAX_SNAPDEVS * sizeof(int));
+
+	blk_size[snap_major] = snap_sizes;
+	blksize_size[snap_major] = snap_blksize;
+	max_readahead[snap_major] = snap_readahead;
+	read_ahead[snap_major] = 128;
+
+	blk_dev[snap_major].queue = snap_get_queue;
+
+	DPRINTK(PFX "%s\n", VERSION_CODE);
+	return 0;
+
+err:
+	printk(PFX "out of memory\n");
+	devfs_unregister(devfs_find_handle(NULL, MODNAME, 0, 0,
+		 	 DEVFS_SPECIAL_BLK, 0));
+	devfs_unregister_blkdev(snap_major, MODNAME);
+	snap_cleanup ();
+	return -ENOMEM;
+}
+
+static void __exit snap_exit(void)
+{
+	devfs_unregister(devfs_find_handle(NULL, MODNAME, 0, 0,
+		 	 DEVFS_SPECIAL_BLK, 0));
+	devfs_unregister_blkdev(snap_major, MODNAME);
+
+	snap_cleanup ();
+	snap_major = 0;
+}
+
+MODULE_DESCRIPTION("Snapshot block device");
+MODULE_AUTHOR("Jeff Garzik <jgarzik@mandrakesoft.com>");
+
+module_init(snap_init);
+module_exit(snap_exit);
Index: linux_2_4/include/linux/snap.h
diff -u /dev/null linux_2_4/include/linux/snap.h:1.1.6.5
--- /dev/null	Sat May 19 17:36:31 2001
+++ linux_2_4/include/linux/snap.h	Thu May 17 11:09:38 2001
@@ -0,0 +1,126 @@
+/*
+ * Copyright 2001 Jeff Garzik <jgarzik@mandrakesoft.com>
+ * Copyright (C) 2000 Jens Axboe <axboe@suse.de>
+ *
+ * May be copied or modified under the terms of the GNU General Public
+ * License.  See linux/COPYING for more information.
+ *
+ */
+#ifndef __LINUX_SNAP_H
+#define __LINUX_SNAP_H
+
+/*
+ * 1 for normal debug messages, 2 is very verbose. 0 to turn it off.
+ */
+#define SNAP_DEBUG		1
+
+/*
+ * No user-servicable parts beyond this point ->
+ */
+
+#if SNAP_DEBUG
+#define DPRINTK(fmt, args...) printk(KERN_NOTICE fmt, ##args)
+#else
+#define DPRINTK(fmt, args...)
+#endif
+
+#if SNAP_DEBUG > 1
+#define VPRINTK(fmt, args...) printk(KERN_NOTICE fmt, ##args)
+#else
+#define VPRINTK(fmt, args...)
+#endif
+
+/* bh list unique identifier */
+#define SNAP_BUF_LIST		0x93
+
+/*
+ * flags
+ */
+#define SNAP_READONLY		1	/* read only dev */
+#define SNAP_ACTIVE		2	/* snapshot dev is active */
+
+/*
+ * Very unused stats for now
+ */
+struct snap_stats
+{
+	unsigned long		bh_s;
+	unsigned long		bh_e;
+	unsigned long		bh_cache_hits;
+	unsigned long		page_cache_hits;
+	unsigned long		secs_w;
+	unsigned long		secs_r;
+};
+
+#define MAX_LOGDEVS		512 /* arbitrary limit */
+
+struct snap_setup
+{
+	int			src_fd;
+	unsigned int		n_log_fds;
+	int			log_fd[0];
+};
+
+/*
+ * packet ioctls
+ */
+#define SNAP_IOCTL_MAGIC	('N')
+#define SNAP_GET_STATS		_IOR(SNAP_IOCTL_MAGIC, 0xE0, struct snap_stats)
+#define SNAP_SETUP_DEV		_IOW(SNAP_IOCTL_MAGIC, 0xE1, struct snap_setup)
+#define SNAP_TEARDOWN_DEV	_IOW(SNAP_IOCTL_MAGIC, 0xE2, unsigned int)
+#define SNAP_START		_IO(SNAP_IOCTL_MAGIC, 0xE3)
+#define SNAP_END		_IO(SNAP_IOCTL_MAGIC, 0xE4)
+
+#ifdef __KERNEL__
+#include <linux/blkdev.h>
+
+struct snap_dev_init_info
+{
+	struct inode *inode;
+	struct file *file;
+};
+
+struct snap_src
+{
+	request_queue_t			*q;
+	kdev_t				dev;
+
+	make_request_fn			*make_request_fn;
+
+	struct block_device		*bdev;
+	struct dentry			*dentry;
+	unsigned long			blocks;
+};
+
+struct snap_log
+{
+	kdev_t				dev;
+	struct dentry			*dentry;
+	unsigned long			size;
+	unsigned long			free;
+	unsigned long			data_blocks;
+};
+
+struct snap_device
+{
+	struct snap_src			src;
+	struct snap_log			*logs;
+	unsigned int			n_logs;
+	unsigned int			active_logs;
+	
+	request_queue_t			q;
+	atomic_t			refcnt;
+	kdev_t				snap_dev;
+	unsigned long			flags;
+	char				name[20];
+	unsigned int			bits_per_block;
+	unsigned int			maps_per_block;
+	unsigned long			map_base;
+	unsigned long			data_base;
+	unsigned int			blksz;
+	struct snap_stats		stats;
+};
+
+#endif /* __KERNEL__ */
+
+#endif /* __LINUX_SNAP_H */

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Jeff Garzik]

Here's a dumb question, and I apologize if I am questioning computer
science dogma...

Why are LVM and EVMS(competing LVM project) needed at all?

Surely the same can be accomplished with
* md
* snapshot blkdev (attached in previous e-mail)
* giving partitions and blkdevs the ability to grow and shrink
* giving filesystems the ability to grow and shrink

On-line optimization (defrag, etc) shouldn't be hard once you have the
ability to move blocks and files around, which would come with the
ability to grow and shrink blkdevs and fs's.

-- 
Jeff Garzik      | "Do you have to make light of everything?!"
Building 1024    | "I'm extremely serious about nailing your
MandrakeSoft     |  step-daughter, but other than that, yes."

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Andrew Clausen]

Jeff Garzik wrote:
> 
> Here's a dumb question, and I apologize if I am questioning computer
> science dogma...
> 
> Why are LVM and EVMS(competing LVM project) needed at all?

EVMS and LVM aren't really competing projects, BTW.  EVMS is
"competing" more with MD.  EVMS will probably use LVM.  (I have
been "out of it" for a month, damned uni assignments...!)
 
> Surely the same can be accomplished with
> * md
> * snapshot blkdev (attached in previous e-mail)
> * giving partitions and blkdevs the ability to grow and shrink
> * giving filesystems the ability to grow and shrink

This last one has little to do with LVM/EVMS.  (it's largely
the same for partitions)  The only difference is you don't need
to handle the resize-the-start case (see below)

> On-line optimization (defrag, etc) shouldn't be hard once you have the
> ability to move blocks and files around, which would come with the
> ability to grow and shrink blkdevs and fs's.

(1) traditional partition implementations tend to have bad
implementations (small static limits on # partitions, etc.)
In other words, partition tables weren't designed for lots
of partitions, which is useful.  (For example, when you expand
a logical volume, you don't need partitions to be "next to each
other"... but the cost is you need to create another partition.
Existing partition table formats tend to starve you)

(2) layering MD on top of partitions means it's impossible to
get redundancy (across disks) on partition table metadata.  So,
if you lose your partition table on one disk, that makes that
whole disk useless.

(3) probably not a good reason: the tools to manage LVM
are more convienient than maintaining partition tables +
MD.

Andrew Clausen

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Oliver Xymoron]

On Sat, 19 May 2001, Jeff Garzik wrote:

> Why are LVM and EVMS(competing LVM project) needed at all?
>
> Surely the same can be accomplished with
> * md
> * snapshot blkdev (attached in previous e-mail)
> * giving partitions and blkdevs the ability to grow and shrink
> * giving filesystems the ability to grow and shrink

You can migrate data off disks while the filesystems on top of them are
live. Add disk b, migrate a->b, remove disk a. Perhaps this is intrinsic
in the above somehow but I don't see it.

--
 "Love the dolphins," she advised him. "Write by W.A.S.T.E.."

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Andreas Dilger]

Jeff writes:
> Here's a dumb question, and I apologize if I am questioning computer
> science dogma...
> 
> Why are LVM and EVMS(competing LVM project) needed at all?
> 
> Surely the same can be accomplished with
> * md
> * snapshot blkdev (attached in previous e-mail)
> * giving partitions and blkdevs the ability to grow and shrink
> * giving filesystems the ability to grow and shrink
> 
> On-line optimization (defrag, etc) shouldn't be hard once you have the
> ability to move blocks and files around, which would come with the
> ability to grow and shrink blkdevs and fs's.

You're missing virtual->physical block mapping allowing you to move parts
of the device around, freedom from the need for contiguous disk space.

In the end, what you've described above is pretty much what LVM does (and
EVMS does better).  Having the various components inside a single layer
like EVMS gives you a lot move flexibility, IMHO.  You also don't have
the issue of wasted minor numbers for unused partitions, or too few minor
numbers in other cases.

For example, with MD RAID you still need devices of equal size to create
a RAID 1 mirror, or part of one device is wasted.  With EVMS you can (in
the future, or right now with AIX/HPUX LVM) do the RAID 1 mirroring on a
per-logical-extent basis and you get your physical extents from any device.
Because your virtual->physical mapping is already abstract, it also allows
you to add mirroring to any existing LVM device without interruption.

Cheers, Andreas

PS - I used to think shrinking a filesystem online was useful, but there
     are a huge amount of problems with this and very few real-life
     benefits, as long as you can at least do offline shrinking.  With
     proper LVM usage, the need to shrink a filesystem never really
     happens in practise, unlike the partition case where you always
     have to guess in advance how big a filesystem needs to be, and then
     add 10% for a safety margin.  With LVM you just create the minimal
     sized device you need now, and freely grow it in the future.
-- 
Andreas Dilger  \ "If a man ate a pound of pasta and a pound of antipasto,
                 \  would they cancel out, leaving him still hungry?"
http://www-mddsp.enel.ucalgary.ca/People/adilger/               -- Dogbert

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Malcolm Beattie]

[cc list reduced]

Andreas Dilger writes:
> PS - I used to think shrinking a filesystem online was useful, but there
>      are a huge amount of problems with this and very few real-life
>      benefits, as long as you can at least do offline shrinking.  With
>      proper LVM usage, the need to shrink a filesystem never really
>      happens in practise, unlike the partition case where you always
>      have to guess in advance how big a filesystem needs to be, and then
>      add 10% for a safety margin.  With LVM you just create the minimal
>      sized device you need now, and freely grow it in the future.

In an attempt to nudge you back towards your previous opinion: consider
a system-wide spool or tmp filesystem. It would be nice to be able to
add in a few extra volumes for a busy period but then shrink it down
again when usage returns to normal. In the absence of the ability to
shrink a live filesystem, storage management becomes a much harder job.
You can't throw in a spare volume or two where it's needed without
careful thought because you'll be ratchetting up the space on that one
filesystem without being able to change your mind and reduce it again
later. You'll end up with stingy storage admins who refuse to give you
a bunch of extra filesystem space for a while because they can't get it
back again afterwards.

--Malcolm

-- 
Malcolm Beattie <mbeattie@sable.ox.ac.uk>
Unix Systems Programmer
Oxford University Computing Services

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Andreas Dilger]

Malcolm Beattie writes:
> Andreas Dilger writes:
> > PS - I used to think shrinking a filesystem online was useful, but there
> >      are a huge amount of problems with this and very few real-life
> >      benefits, as long as you can at least do offline shrinking.  With
> >      proper LVM usage, the need to shrink a filesystem never really
> >      happens in practise, unlike the partition case where you always
> >      have to guess in advance how big a filesystem needs to be, and then
> >      add 10% for a safety margin.  With LVM you just create the minimal
> >      sized device you need now, and freely grow it in the future.
> 
> In an attempt to nudge you back towards your previous opinion: consider
> a system-wide spool or tmp filesystem. It would be nice to be able to
> add in a few extra volumes for a busy period but then shrink it down
> again when usage returns to normal. In the absence of the ability to
> shrink a live filesystem, storage management becomes a much harder job.
> You can't throw in a spare volume or two where it's needed without
> careful thought because you'll be ratchetting up the space on that one
> filesystem without being able to change your mind and reduce it again
> later. You'll end up with stingy storage admins who refuse to give you
> a bunch of extra filesystem space for a while because they can't get it
> back again afterwards.

I suppose it depends a bit on how your system is administered.  On LVM
systems, I tend to allocate new volumes for special situations like this.
When the special need is gone, you simply remove the whole thing.  Yes,
this is a bit of a hack for not having online shrinking, but I have not
really had a _big_ need to do that.

The only time I've really needed online shrinking is when someone
screwed up and made / or /var way too huge for some (bad) reason and
you can't unmount it conveniently.  Under AIX, you can't shrink JFS
even unmounted so it meant backup/restore.  Even so, having empty
space in a filesystem is not a reason to panic, while having no free
space in a filesystem _is_ a reason to panic, hence online growing
of ext2.

Cheers, Andreas
-- 
Andreas Dilger  \ "If a man ate a pound of pasta and a pound of antipasto,
                 \  would they cancel out, leaving him still hungry?"
http://www-mddsp.enel.ucalgary.ca/People/adilger/               -- Dogbert

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Andreas Dilger]

Linus writes:
> There are some strong arguments that we should have filesystem
> "backdoors" for maintenance purposes, including backup. 
> 
> You can, of course, so parts of this on a LVM level, and doing backups
> with "disk snapshots" may be a valid approach. However, even that is
> debatable: there is very little that says that the disk image has to be
> up-to-date at any particular point in time, so even with a disk snapshot
> capability (which is not necessarily reasonable under all circumstances)
> there are arguments for maintenance interfaces.

Actually, the LVM snapshot interface has (optional) hooks into the filesystem
to ensure that it is consistent at the time the snapshot is created.  For
most filesystems, it will call fsync_dev(dev) so that all buffers are written
to disk.  However, for journalled filesystems, LVM needs to write out the
journal and mark the filesystem clean because the snapshot is a read-only
block device.  In this case it calls fsync_dev_lockfs(dev) which will call
the write_super_lockfs() method for the filesystem (if it exists) which
tells the filesystem to flush the journal, block transactions, and mark the
filesystem clean until the unlockfs() method is called.

Reiserfs and XFS both use this to make consistent snapshots of the live
filesystem.  Unfortunately, XFS checks filesystem UUIDs at mount time,
which means you can't mount two copies of the same filesystem (even read-only).

> Things like "lazy fsck" (ie fsck while already running the filesystem) and
> defragmentation simply is not feasible on a LVM level.

Yes, with consistent LVM snapshots you can do fsck on the read-only copy.
In 99.9*% cases you will not detect any errors and you can continue.  If
you _do_ detect an error you probably want to stop everything and fix it
(fsck repairing an in-use filesystem is too twisted and dangerous, IMHO,
and a huge amount of effort for an extremely rare situation).

Cheers, Andreas
-- 
Andreas Dilger  \ "If a man ate a pound of pasta and a pound of antipasto,
                 \  would they cancel out, leaving him still hungry?"
http://www-mddsp.enel.ucalgary.ca/People/adilger/               -- Dogbert

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Linus Torvalds]

On Tue, 22 May 2001, Andreas Dilger wrote:
> 
> Actually, the LVM snapshot interface has (optional) hooks into the filesystem
> to ensure that it is consistent at the time the snapshot is created.

Note that this is still fundamentally a broken interface: the filesystem
may not _have_ a block device underneath it, yet you might very well like
to do defragmentation and backup none-the-less.

Also, lvm snapshots are fundamentally limited to read-only data, which
means that the LVM interfaces cannot be used for defragmentation and lazy
fsck etc anyway. You _have_ to do those at a filesystem level.

disk snapshots are useful, but they are not the answer.

		Linus

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Peter J. Braam]

On Tue, 22 May 2001, Linus Torvalds wrote:
>


> On Tue, 22 May 2001, Andreas Dilger wrote:  Actually, the LVM snapshot
> interface has (optional) hooks into the filesystem to ensure that it
> is consistent at the time the snapshot is created.

But I think that LVM is implemented "the wrong way around".

File system journal recovery can corrupt a snapshot, because it copies
data that needs to be preserved in a snapshot. During journal replay such
data may be copied again, but the source can have new data already.

Most LVM snapshot systems write the new data in the separate volume and
don't copy the old data that eliminates this problem (and also eliminates
the copy of data but introduces data copy when a snapshot is removed).

- Peter -

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Andreas Dilger]

Peter Braam writes:
> On Tue, 22 May 2001, Andreas Dilger wrote:
> > Actually, the LVM snapshot
> > interface has (optional) hooks into the filesystem to ensure that it
> > is consistent at the time the snapshot is created.
> 
> File system journal recovery can corrupt a snapshot, because it copies
> data that needs to be preserved in a snapshot. During journal replay such
> data may be copied again, but the source can have new data already.

The way it is implemented in reiserfs is to wait for existing transactions
to complete, entirely flush the journal and block all new transactions from
starting.  Stephen implemented a journal flush API to do this for ext3, but
the hooks to call it from LVM are not in place yet.  This way the journal is
totally empty at the time the snapshot is done, so the read-only copy does
not need to do journal recovery, so no problems can arise.

Cheers, Andreas
-- 
Andreas Dilger  \ "If a man ate a pound of pasta and a pound of antipasto,
                 \  would they cancel out, leaving him still hungry?"
http://www-mddsp.enel.ucalgary.ca/People/adilger/               -- Dogbert

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Peter J. Braam]

Andreas,


I think that the issue is something different.  Suppose the snapshot has
been created. I know that this can be done safely with the API's you
allude to. Life goes on and the journal FS keeps changing the file system
and if the system doesn't crash, everything is fine: blocks get copied
correctly from the primary volume to the snapshot volume.

Now consider a crash -- not during snapshot creation, but way after that
when "life is going on".  Suppose there is a two block transaction that
has made it to the journal and after writing one block to the fs location
the system crashes.  The journal replay will try to write that block
again.

But during recovery, LVM cannot possibly know if the whole process of
copying out the data from the current to the snapshot area completed
during the previous run. Yes, LVM updates the redirection table first and
then copies, but, still, you don't know _where exactly_ the writes stopped
happening and in particular you don't know if the block was copied already
or not.

So during replay it is quite possible that LVM corrupts the snapshot.

It's better to keep the snapshot in the old volume and write the new data
to a separate area (that's what most commercial systems do I think).  It
avoid redirections and copying upon write.  When you delete the snapshot
you have to copy, but you can do that as a low priority process.
Finally, as you pointed out a full volume is handled better too in that
way, since you don't terminate the snapshot but you tell the current
volume that it is full.

Hmm, I was expecting a storm of email explaining what I have
misunderstood, but it has in fact been rather quiet...

- Peter -






On Tue, 22 May 2001, Andreas Dilger wrote:

> Peter Braam writes:
> > On Tue, 22 May 2001, Andreas Dilger wrote:
> > > Actually, the LVM snapshot
> > > interface has (optional) hooks into the filesystem to ensure that it
> > > is consistent at the time the snapshot is created.
> >
> > File system journal recovery can corrupt a snapshot, because it copies
> > data that needs to be preserved in a snapshot. During journal replay such
> > data may be copied again, but the source can have new data already.
>
> The way it is implemented in reiserfs is to wait for existing transactions
> to complete, entirely flush the journal and block all new transactions from
> starting.  Stephen implemented a journal flush API to do this for ext3, but
> the hooks to call it from LVM are not in place yet.  This way the journal is
> totally empty at the time the snapshot is done, so the read-only copy does
> not need to do journal recovery, so no problems can arise.
>
> Cheers, Andreas
>

-- 

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Stephen C. Tweedie]

Hi,

On Tue, May 22, 2001 at 02:59:32PM -0600, Peter J. Braam wrote:
 
> But during recovery, LVM cannot possibly know if the whole process of
> copying out the data from the current to the snapshot area completed
> during the previous run. Yes, LVM updates the redirection table first and
> then copies, but, still, you don't know _where exactly_ the writes stopped
> happening and in particular you don't know if the block was copied already
> or not.

LVM updates the snapshot redirection without knowing that the new
redirection location has been written?  So if I write to a LVM
snapshot and take a crash, I might not actually get either the old or
the new data, but in fact some previous random contents of a new
block?  Eek.  Journaling will not like that.  Databases won't like
that.  Anything that relies on fsync to ensure some write ordering on
disk will be potentially upset by that.

> It's better to keep the snapshot in the old volume and write the new data
> to a separate area (that's what most commercial systems do I think).

No.  The commercial systems write snapshots to a new area, usually.
There are two very good reason for that --- when you come to delete a
snapshot, there's no IO involved; and you avoid fragmenting the
original root volume.  

In systems I'm familiar with, the copy-out is always done in the same
direction with the snapshot getting the new block.  This even happens
if the snapshot is writable: regardless of whether it is the snapshot
or the root being written, the copy-out always results in the snapshot
getting moved, not the root.

Cheers, 
 Stephen

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Daniel Phillips]

On Tuesday 22 May 2001 22:10, Andreas Dilger wrote:
> Peter Braam writes:
> > File system journal recovery can corrupt a snapshot, because it
> > copies data that needs to be preserved in a snapshot. During
> > journal replay such data may be copied again, but the source can
> > have new data already.
>
> The way it is implemented in reiserfs is to wait for existing
> transactions to complete, entirely flush the journal and block all
> new transactions from starting.  Stephen implemented a journal flush
> API to do this for ext3, but the hooks to call it from LVM are not in
> place yet.  This way the journal is totally empty at the time the
> snapshot is done, so the read-only copy does not need to do journal
> recovery, so no problems can arise.

I suppose I'm just reiterating the obvious, but we should eventually
have a generic filesystem transaction API at the VFS level, once we
have enough data points to know what the One True API should be.

--
Daniel

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Hans Reiser]

Daniel Phillips wrote:
> 
> On Tuesday 22 May 2001 22:10, Andreas Dilger wrote:
> > Peter Braam writes:
> > > File system journal recovery can corrupt a snapshot, because it
> > > copies data that needs to be preserved in a snapshot. During
> > > journal replay such data may be copied again, but the source can
> > > have new data already.
> >
> > The way it is implemented in reiserfs is to wait for existing
> > transactions to complete, entirely flush the journal and block all
> > new transactions from starting.  Stephen implemented a journal flush
> > API to do this for ext3, but the hooks to call it from LVM are not in
> > place yet.  This way the journal is totally empty at the time the
> > snapshot is done, so the read-only copy does not need to do journal
> > recovery, so no problems can arise.
> 
> I suppose I'm just reiterating the obvious, but we should eventually
> have a generic filesystem transaction API at the VFS level, once we
> have enough data points to know what the One True API should be.
> 
> --
> Daniel
> -
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/


Daniel, implementing transactions is not a trivial thing as you probably know. 
It requires that you resolve such issues as, what happens if the user forgets to
close the transaction, issues of lock/transaction duration, of transaction
batching, of levels of isolation, of concurrent transactions modifying global fs
metadata and some but not all of those concurrent transactions receiving a
rollback, and of permissions relating to keeping transactions open.  I would
encourage you to participate in the reiser4 design discussion we will be having
over the next 6 months, and give us your opinions.  Josh will be leading that
design effort for the ReiserFS team.

Hans

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Daniel Phillips]

On Friday 25 May 2001 00:00, Hans Reiser wrote:
> Daniel Phillips wrote:
> > I suppose I'm just reiterating the obvious, but we should
> > eventually have a generic filesystem transaction API at the VFS
> > level, once we have enough data points to know what the One True
> > API should be.
>
> Daniel, implementing transactions is not a trivial thing as you
> probably know. It requires that you resolve such issues as, what
> happens if the user forgets to close the transaction, issues of
> lock/transaction duration, of transaction batching, of levels of
> isolation, of concurrent transactions modifying global fs metadata
> and some but not all of those concurrent transactions receiving a
> rollback, and of permissions relating to keeping transactions open. 
> I would encourage you to participate in the reiser4 design discussion
> we will be having over the next 6 months, and give us your opinions. 
> Josh will be leading that design effort for the ReiserFS team.

Graciously accepted.  Coming up with something sensible in a mere 6 
months would be a minor miracle. ;-)

- what happens if the user forgets to close the transaction?

   I plan to set a checkpoint there (because the transaction got
   too big) and log the fact that it's open.

- issues of lock/transaction duration

   Once again relying on checkpoints, when the transaction gets
   uncomfortably big for cache, set a checkpoint.  I haven't thought
   about locks

- transaction batching

   1) Explicit transaction batch close 2) Cache gets past a certain     
   fullness.  In both cases, no new transactions are allowed to start
   and as soon as all current ones are closed we close the batch.

- of levels of isolation
- concurrent transactions modifying global fs metadata
   and some but not all of those concurrent transactions receiving a
   rollback

   First I was going to write 'huh?' here, then I realized you're       
   talking about real database ops, not just filesystem ops.  I had
   in mind something more modest: transactions are 'mv', 'read/write'
   (if the 'atomic read/write' is set), other filesystem operations I've
   forgotten, and anything the user puts between open_xact and          
   close_xact.  You are raising the ante a little ;-)

   In my case (Tux2) I could do an efficient rollback to the beginning
  of the batch (phase), then I would have had to have kept an           
   in-memory log of the transactions for selective replay.  With a      
   journal log you can obviously do the same thing, but perhaps more
   efficiently if your journal design supports undo/redo.

   The above is a pure flight of fancy, we won't be seeing anything
   so fancy as an API across filesystems.

- permissions relating to keeping transactions open. 
   We can see this one in the light of a simple filesystem              
   transaction: what happens if we are in the middle of a mv and        
   someone changes the permissions?  Go with the starting or
   ending permissions?

Well, the database side of this is really interesting, but to get 
something generic across filesystems, the scope pretty well has to be 
limited to journal-type transactions, don't you think?

--
Daniel

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH]device arguments from lookup)

[Stephen C. Tweedie]

Hi,

On Tue, May 22, 2001 at 01:16:42PM -0600, Peter J. Braam wrote:
 
> File system journal recovery can corrupt a snapshot, because it copies
> data that needs to be preserved in a snapshot.

Journal recovery may move data from the journal to other locations on
the device, yes, but that doesn't change the logical contents of the
filesystem.  I don't see how that results in "corruption": the
snapshot is (or at least, ought to be!) fully independent of the
original version of the data, so such recovery should only be taking
the snapshot from one consistent state to a different but equivalent
state.

> During journal replay such
> data may be copied again, but the source can have new data already.

Only if you are recovering a live volume, surely?  And that is
*guaranteed* to cause problems.  

--Stephen

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH] device arguments from lookup)

[Pavel Machek]

Hi!

> A lot of stuff relies on the fact that close(open(foo, O_RDONLY)) is a
> no-op. Breaking that assumption is a Bad Thing(tm).

Then we have a problem. Just opening /dev/ttyS0 currently *has* side
effects (it is visible on modem lines from serial port; it can block
you forever). 

If this assumption is somewhere, we should fix that place... Or fix
serial ports.
								Pavel
-- 
I'm pavel@ucw.cz. "In my country we have almost anarchy and I don't care."
Panos Katsaloulis describing me w.r.t. patents at discuss@linmodems.org

Re: Why side-effects on open(2) are evil. (was Re: [RFD w/info-PATCH] device arguments from lookup)

[Alexander Viro]

On Sun, 20 May 2001, Pavel Machek wrote:

> Hi!
> 
> > A lot of stuff relies on the fact that close(open(foo, O_RDONLY)) is a
> > no-op. Breaking that assumption is a Bad Thing(tm).
> 
> Then we have a problem. Just opening /dev/ttyS0 currently *has* side
> effects (it is visible on modem lines from serial port; it can block
> you forever). 
> 
> If this assumption is somewhere, we should fix that place... Or fix
> serial ports.

There is no way to fix it. If process A has ability to create and remove
files in directory foo, then process B has no way to know what file it
will actually open upon the attempt to open file in foo.

	Example: you want to open /home/luser/barf and /home in on root
filesystem (too many systems have such setup, and braindead as it is
it _is_ valid). Luser creates a link to his tty (currently owned by
luser, so no bullshit about "let's restrict link(2) to the case when
target is owned by caller", please). After that he renames that link
to barf.

	If you've just decided to open it and rename() comes when you
enter open(3) (in libc, still in userland), you _will_ end up opening
luser's tty.

	OTOH, behaviour of serial ports is required by standards.

All we can do is to open it in non-blocking mode and then checking whether
we've got what we wanted. You _must_ call fstat(2) after opening a file
that could be replaced under you. If you are not doing that (and open
file in directory controled by somebody else) - you have an exploitable
race. However, fstat() is too late to avoid side-effects of open() itself.

For serial ports O_NDELAY is enough to avoid that side effect. For something
where it's not enough - well, too bad. Don't do it.