fuse: close file synchronously

There are two types of I/O activity that can be "in progress" at the time of fuse_release() execution: asynchronous read-ahead and write-back. The patch ensures that they are completed before fuse_release_common sends FUSE_RELEASE to userspace.

So far as fuse_release() waits for end of async I/O, its callbacks (fuse_readpages_end and fuse_writepage_finish) calling fuse_file_put cannot be the last holders of fuse file anymore. To emphasize the fact, the patch replaces fuse_file_put with __fuse_file_put there.

Signed-off-by Maxim Patlasov <[email protected]>

---
 fs/fuse/file.c |   55 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 52 insertions(+), 3 deletions(-)

fs/fuse/file.c +0 -0

index 4f23134..aed9be2 100644

fs/fuse/file.c +13 -0

 	fuse_file_put(ff, ff->fc->destroy_req != NULL);
 }
 

@@ -271,6 +282,31 @@ static int fuse_open(struct inode *inode, struct file *file)
 
 static int fuse_release(struct inode *inode, struct file *file)
 {
+	struct fuse_file *ff = file->private_data;
+
+	if (ff->fc->close_wait) {
+		struct fuse_inode *fi = get_fuse_inode(inode);
+
+		/*
+		 * Must remove file from write list. Otherwise it is possible
+		 * this file will get more writeback from another files
+		 * rerouted via write_files.
+		 */
+		spin_lock(&ff->fc->lock);
+		list_del_init(&ff->write_entry);
+		spin_unlock(&ff->fc->lock);

fs/fuse/file.c +18 -3

+	}
+
 	fuse_release_common(file, FUSE_RELEASE);
 
 	/* return value is ignored by VFS */

@@ -610,8 +646,17 @@ static void fuse_readpages_end(struct fuse_conn *fc, struct fuse_req *req)
 		unlock_page(page);
 		page_cache_release(page);
 	}
-	if (req->ff)
-		fuse_file_put(req->ff, false);
+	if (req->ff) {
+		if (fc->close_wait) {
+			struct fuse_inode *fi = get_fuse_inode(req->inode);
+
+			spin_lock(&fc->lock);
+			__fuse_file_put(req->ff);
+			wake_up(&fi->page_waitq);
+			spin_unlock(&fc->lock);
+		} else
+			fuse_file_put(req->ff, false);
+	}
 }
 
 struct fuse_fill_data {

@@ -637,6 +682,7 @@ static void fuse_send_readpages(struct fuse_fill_data *data)
 	if (fc->async_read) {
 		req->ff = fuse_file_get(ff);
 		req->end = fuse_readpages_end;
+		req->inode = data->inode;
 		fuse_request_send_background(fc, req);
 	} else {
 		fuse_request_send(fc, req);

@@ -1178,7 +1224,8 @@ static ssize_t fuse_direct_write(struct file *file, const char __user *buf,
 static void fuse_writepage_free(struct fuse_conn *fc, struct fuse_req *req)
 {
 	__free_page(req->pages[0]);
-	fuse_file_put(req->ff, false);
+	if (!fc->close_wait)
+		fuse_file_put(req->ff, false);
 }
 
 static void fuse_writepage_finish(struct fuse_conn *fc, struct fuse_req *req)

@@ -1191,6 +1238,8 @@ static void fuse_writepage_finish(struct fuse_conn *fc, struct fuse_req *req)
 	dec_bdi_stat(bdi, BDI_WRITEBACK);
 	dec_zone_page_state(req->pages[0], NR_WRITEBACK_TEMP);
 	bdi_writeout_inc(bdi);
+	if (fc->close_wait)
+		__fuse_file_put(req->ff);
 	wake_up(&fi->page_waitq);
 }
 

Quoted 20 lines

There are two types of I/O activity that can be "in progress" at the time of fuse_release() execution: asynchronous read-ahead and write-back. The patch ensures that they are completed before fuse_release_common sends FUSE_RELEASE to userspace.

So far as fuse_release() waits for end of async I/O, its callbacks (fuse_readpages_end and fuse_writepage_finish) calling fuse_file_put cannot be the last holders of fuse file anymore. To emphasize the fact, the patch replaces fuse_file_put with __fuse_file_put there.

Signed-off-by Maxim Patlasov <[email protected]>

---
  fs/fuse/file.c |   55 ++++++++++++++++++++++++++++++++++++++++++++++++++++---
  1 files changed, 52 insertions(+), 3 deletions(-)

fs/fuse/file.c +0 -0

index 4f23134..aed9be2 100644

fs/fuse/file.c static void fuse_file_put(struct fuse_file *ff, bool sync) · line 145

Quoted 9 lines

fs/fuse/file.c +6 -0

@@ -137,6 +137,12 @@ static void fuse_file_put(struct fuse_file *ff, bool sync)
  	}
  }
 
+static void __fuse_file_put(struct fuse_file *ff)
+{
+	if (atomic_dec_and_test(&ff->count))
+		BUG();
+}
+

I think a comment in or before this function to explain the reasoning for the BUG would be helpful.

fs/fuse/file.c void fuse_release_common(struct file *file, int opcode) · line 274

Quoted 13 lines

fs/fuse/file.c +5 -0

  int fuse_do_open(struct fuse_conn *fc, u64 nodeid, struct file *file,
  		 bool isdir)
  {

@@ -260,7 +266,12 @@ void fuse_release_common(struct file *file, int opcode)
  	 * Make the release synchronous if this is a fuseblk mount,
  	 * synchronous RELEASE is allowed (and desirable) in this case
  	 * because the server can be trusted not to screw up.
+	 *
+	 * We might wait for them (asynchronous READ or WRITE requests), so:
  	 */
+	if (ff->fc->close_wait)
+		BUG_ON(atomic_read(&ff->count) != 1);
+

It might be cleaner to pull the new part of the comment and the BUG_ON() check to before the existing comment and fuse_file_put (e.g., create a new comment).

Quoted 20 lines

fs/fuse/file.c +13 -0

  	fuse_file_put(ff, ff->fc->destroy_req != NULL);
  }
 

@@ -271,6 +282,31 @@ static int fuse_open(struct inode *inode, struct file *file)
 
  static int fuse_release(struct inode *inode, struct file *file)
  {
+	struct fuse_file *ff = file->private_data;
+
+	if (ff->fc->close_wait) {
+		struct fuse_inode *fi = get_fuse_inode(inode);
+
+		/*
+		 * Must remove file from write list. Otherwise it is possible
+		 * this file will get more writeback from another files
+		 * rerouted via write_files.
+		 */
+		spin_lock(&ff->fc->lock);
+		list_del_init(&ff->write_entry);
+		spin_unlock(&ff->fc->lock);

fs/fuse/file.c static int fuse_open(struct inode *inode, struct file *file) · line 307

Quoted 10 lines

fs/fuse/file.c +10 -0

+
+		wait_event(fi->page_waitq, atomic_read(&ff->count) == 1);
+
+		/*
+		 * Wait for threads just released ff to leave their critical
+		 * sections. Taking spinlock is the first thing
+		 * fuse_release_common does, so that this is unnecessary, but
+		 * it is still good to emphasize right here, that we need this.
+		 */
+		spin_unlock_wait(&ff->fc->lock);

I'm all for clarity, but if the wait is unnecessary, perhaps just leave the comment..? Just my .02.

Aside from the few nits here, the set looks pretty good to me.

Hi,

There is a long-standing demand for syncronous behaviour of fuse_release:

http://sourceforge.net/mailarchive/message.php?msg_id=19343889
http://sourceforge.net/mailarchive/message.php?msg_id=29814693

A few months ago Avati and me explained why such a feature would be useful:

http://sourceforge.net/mailarchive/message.php?msg_id=29889055
http://sourceforge.net/mailarchive/message.php?msg_id=29867423

In short, the problem is that fuse_release (that's called on last user close(2)) sends FUSE_RELEASE to userspace and returns without waiting for ACK from userspace. Consequently, there is a gap when user regards the file released while userspace fuse is still working on it. An attempt to access the file from another node leads to complicated synchronization problems because the first node still "holds" the file.

The patch-set resolves the problem by making fuse_release synchronous:
wait for ACK from userspace for FUSE_RELEASE if the feature is ON.

To keep single-threaded userspace implementations happy the patch-set ensures that by the time fuse_release_common calls fuse_file_put, no more in-flight I/O exists. Asynchronous fuse callbacks (like fuse_readpages_end) cannot trigger FUSE_RELEASE anymore. Hence, we'll never block in contexts other than close().

Thanks,
Maxim

---

Maxim Patlasov (5):
fuse: add close_wait flag to fuse_conn
fuse: cosmetic rework of fuse_send_readpages
fuse: wait for end of IO on release
fuse: enable close_wait feature
fuse: fix synchronous case of fuse_file_put()

  fs/fuse/file.c            |   82 ++++++++++++++++++++++++++++++++++++++-------
  fs/fuse/fuse_i.h          |    3 ++
  fs/fuse/inode.c           |    5 ++-
  include/uapi/linux/fuse.h |    7 +++-
  4 files changed, 82 insertions(+), 15 deletions(-)

Hi,

There is a long-standing demand for syncronous behaviour of fuse_release:

http://sourceforge.net/mailarchive/message.php?msg_id=19343889
http://sourceforge.net/mailarchive/message.php?msg_id=29814693

A few months ago Avati and me explained why such a feature would be useful:

http://sourceforge.net/mailarchive/message.php?msg_id=29889055
http://sourceforge.net/mailarchive/message.php?msg_id=29867423

In short, the problem is that fuse_release (that's called on last user close(2)) sends FUSE_RELEASE to userspace and returns without waiting for ACK from userspace. Consequently, there is a gap when user regards the file released while userspace fuse is still working on it. An attempt to access the file from another node leads to complicated synchronization problems because the first node still "holds" the file.

The patch-set resolves the problem by making fuse_release synchronous:
wait for ACK from userspace for FUSE_RELEASE if the feature is ON.

To keep single-threaded userspace implementations happy the patch-set ensures that by the time fuse_release_common calls fuse_file_put, no more in-flight I/O exists. Asynchronous fuse callbacks (like fuse_readpages_end) cannot trigger FUSE_RELEASE anymore. Hence, we'll never block in contexts other than close().

For example doing a readlink() on a magic symlink under /proc shouldn't result in a synchronous call to a fuse filesystem. Making fput() synchronous may actually end up doing that (even if it's not very likely).

Quoted 4 lines · Miklos Szeredi

For example doing a readlink() on a magic symlink under /proc shouldn't result in a synchronous call to a fuse filesystem. Making fput() synchronous may actually end up doing that (even if it's not very likely).

Thinking about this a bit more. As it is it sounds wrong to rely on a synchronous release, when in fact release is just not synchronous, as indicated by the above example. Maybe it's the proc code that's buggy and shouldn't do get_file/fput because everyone is assuming release being synchronous with close(). Don't know.

Quoted 11 lines · Miklos Szeredi

Quoted 4 lines · Miklos Szeredi

For example doing a readlink() on a magic symlink under /proc shouldn't result in a synchronous call to a fuse filesystem. Making fput() synchronous may actually end up doing that (even if it's not very likely).

Thinking about this a bit more. As it is it sounds wrong to rely on a synchronous release, when in fact release is just not synchronous, as indicated by the above example. Maybe it's the proc code that's buggy and shouldn't do get_file/fput because everyone is assuming release being synchronous with close(). Don't know.

What the hell? ->release() is not and has never been synchronous with close(). There is any number of places where the final fput() might be called and no, this readlink example is irrelevant - things like munmap()/dup2()/close of a socket discarding a datagram with the last reference to struct file in it, et sodding cetera.

Hell, another thread might be in the middle of read(2) at the moment when you call close(). Result: the final fput() will be done when we are about to return from that read(2).

Let's approach it from the other direction: what if you give back the write lease on the first flush? It will probably work fine for 99% of cases, since no other writes are going to happen after the first flush. For the remaining cases you'll just have to reacquire the lease when a write happens after the flush. I guess performance-wise that will not be an issue, but I may be wrong.

Quoted 6 lines · Miklos Szeredi

Let's approach it from the other direction: what if you give back the write lease on the first flush? It will probably work fine for 99% of cases, since no other writes are going to happen after the first flush. For the remaining cases you'll just have to reacquire the lease when a write happens after the flush. I guess performance-wise that will not be an issue, but I may be wrong.

What about this?

Quoted 19 lines · Maxim Patlasov

Hi,

There is a long-standing demand for syncronous behaviour of fuse_release:

http://sourceforge.net/mailarchive/message.php?msg_id=19343889
http://sourceforge.net/mailarchive/message.php?msg_id=29814693

A few months ago Avati and me explained why such a feature would be useful:

http://sourceforge.net/mailarchive/message.php?msg_id=29889055
http://sourceforge.net/mailarchive/message.php?msg_id=29867423

In short, the problem is that fuse_release (that's called on last user close(2)) sends FUSE_RELEASE to userspace and returns without waiting for ACK from userspace. Consequently, there is a gap when user regards the file released while userspace fuse is still working on it. An attempt to access the file from another node leads to complicated synchronization problems because the first node still "holds" the file.

Quoted 8 lines

The patch-set resolves the problem by making fuse_release synchronous:
wait for ACK from userspace for FUSE_RELEASE if the feature is ON.

To keep single-threaded userspace implementations happy the patch-set ensures that by the time fuse_release_common calls fuse_file_put, no more in-flight I/O exists. Asynchronous fuse callbacks (like fuse_readpages_end) cannot trigger FUSE_RELEASE anymore. Hence, we'll never block in contexts other than close().

There are a few fput() calls outside sys_close(), all of these can trigger FUSE_RELEASE. Most of those are OK, but for some I'm reluctant to enable synchronous release.

For example doing a readlink() on a magic symlink under /proc shouldn't result in a synchronous call to a fuse filesystem. Making fput() synchronous may actually end up doing that (even if it's not very likely).

At least for the unprivileged fuse daemon case it shouldn't be done. If the fuse daemon can be "trusted" then enabling synchronous release should be okay, that's why it's enabled for fuseblk.

But maybe I'm just too paranoid...