coda: kill file_count abuse

->release is the proper way to detect the last close of a file, file_count should never be used in filesystems.

Index: linux-2.6/fs/coda/dir.c
===================================================================
@@ -86,7 +86,6 @@ const struct file_operations coda_dir_op
 	.read		= generic_read_dir,
 	.readdir	= coda_readdir,
 	.open		= coda_open,
-	.flush		= coda_flush,
 	.release	= coda_release,
 	.fsync		= coda_fsync,
 };

Index: linux-2.6/include/linux/coda_linux.h
===================================================================
@@ -36,7 +36,6 @@ extern const struct file_operations coda
 
 /* operations shared over more than one file */
 int coda_open(struct inode *i, struct file *f);
-int coda_flush(struct file *f, fl_owner_t id);
 int coda_release(struct inode *i, struct file *f);
 int coda_permission(struct inode *inode, int mask, struct nameidata *nd);
 int coda_revalidate_inode(struct dentry *);

Has been tried, the problem with that once ->release is called it is too late to pass the the error back to close(2).

Jan

I think you'll find the problem is that fput() throws away the error from ->release, not that it's too late....

Cheers,

Dave.

Dave Chinner
Principal Engineer
SGI Australian Software Group

Just where would that return value go?

BTW, the reason why checks for struct file refcount blow is not far from that:

task A: write()
task B (sharing descriptor table with A): close()
task C (with another reference to struct file in question): close() task A: return from write()

Now, the final fput() here happens in write(). In particular, no call of close(2) sees refcount equal to 1.

To the context that dropped the last reference. It can't be reported to anything else....

I'd much prefer to get an error on a file to any process that was using the file than have a potential indication of data or filesystem corruption silently dropped in the bit bucket.....

Sure, but you've given no reason why the write() is unable to or should not return the error.

Cheers,

Dave.

Dave Chinner
Principal Engineer
SGI Australian Software Group

Oh, for fsck sake...

Send a datagram with SCM_RIGHTS in it. Have all other references to these files closed. Now have close(2) kill the socket that might eventually be used to receive the datagram. Now, all these files are closed. Tell me, which error value would you report to that caller of close() and how would it guess which files had those been?

Consider munmap(). It can release the last reference to any number of files (mmap them in different places, then munmap the entire area). Which error would you report?

Consider exit(), for crying out loud. You have a file opened. You fork a child. You close the file in parent. Child goes away. Who do you report that stuff?

Here is a patch which removes the file_count test from coda_flush. This avoids the race condition described by Al Viro where we may never see the file_count drop to 0 in ->flush, in which case userspace is never notified that a writeback should occur.

---

Going from here I can fix the problem in several ways in userspace. The current implementation would end up making useless snapshots as a result of every close, and still leave us with lost updates in the the write-after-close case, but that isn't any worse than what it is now.

Seeing every fops->flush does allow us to perform preliminary checks so we can return errors for common issues and we can flag the file so that the final release upcall (where we can no longer return errors) will start the writeback.

Really the problem for me is that Coda's semantics (write on last close) are not compatible with UNIX semantics, where writes may occur even after the application has closed all fd's. This is quite similar to having an open fd to a removed file, UNIX obviously doesn't return an error or block the unlink if there is an active reference it just detaches the inode from the namespace.

I will try to find a clean way to block the close syscall until fput drops the last reference. However I realize that such an implementation would not be acceptable for other file systems, and there are some interesting unresolved details such as 'which close is going to be the last close'.

index 7594962..0a300dd 100644
@@ -169,15 +169,10 @@ int coda_flush(struct file *coda_file, fl_owner_t id)
 	unsigned short coda_flags = coda_flags_to_cflags(flags);
 	struct coda_file_info *cfi;
 	struct inode *coda_inode;
-	int err = 0, fcnt;
+	int err = 0;
 
 	lock_kernel();
 
-	/* last close semantics */
-	fcnt = file_count(coda_file);
-	if (fcnt > 1)
-		goto out;
-
 	/* No need to make an upcall when we have not made any modifications
 	 * to the file */
 	if ((coda_file->f_flags & O_ACCMODE) == O_RDONLY)

@@ -246,7 +241,7 @@ int coda_release(struct inode *coda_inode, struct file *coda_file)
 	coda_file->private_data = NULL;
 
 	unlock_kernel();
-	return err;
+	return 0; /* no point in returning err, VFS ignores returned value */
 }
 
 int coda_fsync(struct file *coda_file, struct dentry *coda_dentry, int datasync)

Simply impossible.

	fd = open(...);
	fd2 = dup(fd);
	....
	close(fd2);  <-- deadlock
	close(fd);

Or

	fd = open(...);
	p = mmap(..., fd, ...);
	close(fd);  <-- deadlock
	...
	munmap(p, ...);

The problem is that you are mixing two different operations:
* closing descriptor: descriptor doesn't refer to this opened file
anymore. Can be done by close(), can be done by dup2(), can be done by fcntl() analog of dup2(), can be done by execve(), can be done by exit(). And can be done by death-by-signal.
* closing opened file: no references (including descriptors) exist
anymore, no IO of any kind will be done on that IO channel. Can happen when descriptor gets closed, can happen when mapping gets destroyed (munmap, etc.), can happen when SCM_RIGHTS datagram gets destroyed (including the garbage collection), can happen when any syscall on that file finishes after another thread has closed descriptor passed to that syscall, etc.

You _can't_ expect the errors from the latter to be reliably
passed to some process; for one thing, many files can get closed by single system call (e.g. closing an AF_UNIX socket with pending SCM_RIGHTS datagrams will flush those datagrams, dropping references to files we tried to pass via them).

Why does CODA need special warranties in that area, anyway?
Related question: does fsync() force the writeback?

As usual you are correct.

Originally Coda only used the CODA_CLOSE upcall which was called from fops->release (fput). The problem was that we had various errors that never managed to make it back to the user. So I split up the operation into the part that does the write back and is the source of most errors. (CODA_STORE, called from fops->flush) and the part that drops the last reference (CODA_RELEASE, called from fops->release).

I've actually only used the _STORE/_RELEASE upcalls in a development version, so the old code is not only still around, it is the typically used variant.

I'll submit a patch that removes those upcalls, they will never work the way I hoped.

I don't think it really needs special warranties. The intent is to write back data only when the last reference has been released. Ideally we would like to return errors that occur during this writeback to the application. Clearly the latter isn't possible.

I think it should, but at the moment it does not. My guess on why the implementation isn't there is that it mesh well with the last-writer close model. The cache manager actually doesn't trigger any write back operations until the owrite counter drops to 0 and this happens only after all open for write descriptors for a file have been released. As a result it was a bit of a hack to get the _STORE/_RELEASE variant to work because it couldn't rely on the counter dropping to 0, which is one of the reasons why I never released a Coda client that actually uses those new upcalls.

We do have an upcall that is sent when fsync is called and could use that. For fsync we could ignore that owrite counter. I am not sure if the in-kernel implementation is correct, I would expect that it should be an atomic operation wrt. other writers to be able to guarantee that the resulting file actually contains what we expect but right now it looks like it does grab the inode mutex when it syncs the file to disk but releases the lock before it sends an upcall to the cache manager.

Jan

Yup, all good cases where we can't report an error.

OTOH, consider a single thread doing an open(), write(), close(). That's a case where we could report an error, and it's far, far more common than any of the scenarios you list above.

Effectively, that is my point - it's the fput() calling context that knows whether it can return a meaningful error or not. And ->release being non-void implies that you can return meaningful errors to the caller.

So either fput() needs to propagate errors or ->release() should be void, right?

Cheers,

Dave.

Dave Chinner
Principal Engineer
SGI Australian Software Group

Right, there are actually quite a few places where different drivers and file systems are returning non-zero errors from ->release. I just built a kernel with the prototype changed to void to see how many places are affected, 118 files changed, 211 insertions(+), 391 deletions(-)

That's with my default config on i386, so there are probably quite a few more. The change also break the ABI quite badly (several EXPORT_SYMBOL functions are affected) There were a few places where the error handling path seems to lead to a possible struct file or private_data memory leak.

I see.

So if I want last-writer semantics, to let the last close trigger writeback (upcall to userspace) combined with the ability to return an error from the writeback back to close(2). To return an error I have to use fops->flush, but as you've shown, I cannot reliably test if this close dropped the last reference. In fact in your example there was write activity even after the last close.

I guess I should find a way to delay the close (or at least the userspace notification/return from syscall) until there are no active writes.

Jan

=====
Some of the possibly suspect error handling cases I found while building a kernel with void release() in fops, I haven't really looked at these too deeply (i.e. only looked at the deallocation, not the allocations so these may very well be harmless),

drivers/char/ipmi/ipmi_devintf.c: ipmi_release
returns error when ipmi_destroy_user fails, does not call ipmi_fasync and seems to leak memory referenced by file->private_data

drivers/scsi/sg.c: sg_release

    may return ENXIO. leaks file->private_data when sfp->parentdp is NULL.

fs/autofs4/root.c: autofs4_dir_close
returns EBUSY or ENOENT. In the case it returns busy it isn't releasing a struct file.

fs/bad_inode.c: bad_file_release
returns EIO. Since the return code is ignored either way, there is no need to even implement this function.

fs/dlm/user.c: device_close
may return ENOENT, doesn't free memory referenced by file->private_data if it fails this way.

fs/ecryptfs/file.c: ecryptfs_release
returns error when it fails to close the lower file, it also seems to leak memory in this case.