[git patch] fuse fixes

[git patch] fuse fixes

[Miklos Szeredi]

Linus,

Please pull from 'for-linus' branch of
master.kernel.org:/pub/scm/linux/kernel/git/mszeredi/fuse.git

to receive the following updates:

 Documentation/filesystems/sysfs.txt |    5 ++++
 fs/fuse/dev.c                       |   35 ++++++++++++++++++-------------
 fs/fuse/fuse_i.h                    |   12 ++++++++--
 fs/fuse/inode.c                     |   40 ++++++++++++++++--------------------
 4 files changed, 52 insertions(+), 40 deletions(-)

Miklos Szeredi:
      Revert "[fuse] fix deadlock between fuse_put_super() and request_end()"
      [fuse] fix deadlock between fuse_put_super() and request_end()
      [fuse] fix race between checking and setting file->private_data
      [doc] add paragraph about 'fs' subsystem to sysfs.txt

I'll also be replying to this mail with the actual patches.

Thanks,
Miklos

[PATCH 1/4] Revert "[fuse] fix deadlock between fuse_put_super() and request_end()"

[Miklos Szeredi]

This reverts 73ce8355c243a434524a34c05cc417dd0467996e commit.

---

 fs/fuse/dev.c    |   28 ++++++++++++----------------
 fs/fuse/fuse_i.h |   12 +++++++++---
 fs/fuse/inode.c  |   27 ++++++++++-----------------
 3 files changed, 31 insertions(+), 36 deletions(-)

2dc5efa3d0dd4f57db589c61ed9ed80fec9ac45b
diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index cc750c6..4967bd4 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -128,14 +128,20 @@ void fuse_put_request(struct fuse_conn *
 	}
 }
 
-void fuse_remove_background(struct fuse_conn *fc, struct fuse_req *req)
+void fuse_release_background(struct fuse_conn *fc, struct fuse_req *req)
 {
-	list_del_init(&req->bg_entry);
+	iput(req->inode);
+	iput(req->inode2);
+	if (req->file)
+		fput(req->file);
+	spin_lock(&fc->lock);
+	list_del(&req->bg_entry);
 	if (fc->num_background == FUSE_MAX_BACKGROUND) {
 		fc->blocked = 0;
 		wake_up_all(&fc->blocked_waitq);
 	}
 	fc->num_background--;
+	spin_unlock(&fc->lock);
 }
 
 /*
@@ -165,27 +171,17 @@ static void request_end(struct fuse_conn
 		wake_up(&req->waitq);
 		fuse_put_request(fc, req);
 	} else {
-		struct inode *inode = req->inode;
-		struct inode *inode2 = req->inode2;
-		struct file *file = req->file;
 		void (*end) (struct fuse_conn *, struct fuse_req *) = req->end;
 		req->end = NULL;
-		req->inode = NULL;
-		req->inode2 = NULL;
-		req->file = NULL;
-		if (!list_empty(&req->bg_entry))
-			fuse_remove_background(fc, req);
 		spin_unlock(&fc->lock);
-
+		down_read(&fc->sbput_sem);
+		if (fc->mounted)
+			fuse_release_background(fc, req);
+		up_read(&fc->sbput_sem);
 		if (end)
 			end(fc, req);
 		else
 			fuse_put_request(fc, req);
-
-		if (file)
-			fput(file);
-		iput(inode);
-		iput(inode2);
 	}
 }
 
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 59661c4..0474202 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -258,9 +258,15 @@ struct fuse_conn {
 	/** waitq for blocked connection */
 	wait_queue_head_t blocked_waitq;
 
+	/** RW semaphore for exclusion with fuse_put_super() */
+	struct rw_semaphore sbput_sem;
+
 	/** The next unique request id */
 	u64 reqctr;
 
+	/** Mount is active */
+	unsigned mounted;
+
 	/** Connection established, cleared on umount, connection
 	    abort and device release */
 	unsigned connected;
@@ -471,11 +477,11 @@ void request_send_noreply(struct fuse_co
 void request_send_background(struct fuse_conn *fc, struct fuse_req *req);
 
 /**
- * Remove request from the the background list
+ * Release inodes and file associated with background request
  */
-void fuse_remove_background(struct fuse_conn *fc, struct fuse_req *req);
+void fuse_release_background(struct fuse_conn *fc, struct fuse_req *req);
 
-/** Abort all requests */
+/* Abort all requests */
 void fuse_abort_conn(struct fuse_conn *fc);
 
 /**
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index 43a6fc0..fd34037 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -204,26 +204,17 @@ static void fuse_put_super(struct super_
 {
 	struct fuse_conn *fc = get_fuse_conn_super(sb);
 
+	down_write(&fc->sbput_sem);
+	while (!list_empty(&fc->background))
+		fuse_release_background(fc,
+					list_entry(fc->background.next,
+						   struct fuse_req, bg_entry));
+
 	spin_lock(&fc->lock);
+	fc->mounted = 0;
 	fc->connected = 0;
-	while (!list_empty(&fc->background)) {
-		struct fuse_req *req = list_entry(fc->background.next,
-						  struct fuse_req, bg_entry);
-		struct inode *inode = req->inode;
-		struct inode *inode2 = req->inode2;
-
-		/* File would hold a reference to vfsmount */
-		BUG_ON(req->file);
-		req->inode = NULL;
-		req->inode2 = NULL;
-		fuse_remove_background(fc, req);
-
-		spin_unlock(&fc->lock);
-		iput(inode);
-		iput(inode2);
-		spin_lock(&fc->lock);
-	}
 	spin_unlock(&fc->lock);
+	up_write(&fc->sbput_sem);
 	/* Flush all readers on this fs */
 	kill_fasync(&fc->fasync, SIGIO, POLL_IN);
 	wake_up_all(&fc->waitq);
@@ -395,6 +386,7 @@ static struct fuse_conn *new_conn(void)
 		INIT_LIST_HEAD(&fc->processing);
 		INIT_LIST_HEAD(&fc->io);
 		INIT_LIST_HEAD(&fc->background);
+		init_rwsem(&fc->sbput_sem);
 		kobj_set_kset_s(fc, connections_subsys);
 		kobject_init(&fc->kobj);
 		atomic_set(&fc->num_waiting, 0);
@@ -549,6 +541,7 @@ static int fuse_fill_super(struct super_
 		goto err_free_req;
 
 	sb->s_root = root_dentry;
+	fc->mounted = 1;
 	fc->connected = 1;
 	kobject_get(&fc->kobj);
 	file->private_data = fc;
-- 
1.2.4

Re: [PATCH 1/4] Revert "[fuse] fix deadlock between fuse_put_super() and request_end()"

[Linus Torvalds]

On Tue, 25 Apr 2006, Miklos Szeredi wrote:
>
> This reverts 73ce8355c243a434524a34c05cc417dd0467996e commit.

Btw, I _really_ hate commit messages that just say "this reverts that".

Please please _please_ say why it gets reverted, even if it's just a short 
explanation of what was wrong, and what the right thing is.

And if you have an old version of git that doesn't even start up an editor 
to allow you to talk about why the revert happens (yeah, that was a 
mistake), please do upgrade.

Trust me, you'll find a lot of the other improvements to your liking too.

		Linus

[PATCH 2/4] [fuse] fix deadlock between fuse_put_super() and request_end()

[Miklos Szeredi]

A deadlock was possible, when the last reference to the superblock was
held due to a background request containing a file reference.

Releasing the file would release the vfsmount which in turn would
release the superblock.  Since sbput_sem is held during the fput() and
fuse_put_super() tries to acquire this same semaphore, a deadlock
results.

The solution is to move the fput() outside the region protected by
sbput_sem.

Signed-off-by: Miklos Szeredi <miklos@szeredi.hu>

---

 fs/fuse/dev.c |   13 +++++++++++--
 1 files changed, 11 insertions(+), 2 deletions(-)

cdea55d00688ed9a494345b131521a2cae36d42a
diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 4967bd4..104a62d 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -128,12 +128,16 @@ void fuse_put_request(struct fuse_conn *
 	}
 }
 
+/*
+ * Called with sbput_sem held for read (request_end) or write
+ * (fuse_put_super).  By the time fuse_put_super() is finished, all
+ * inodes belonging to background requests must be released, so the
+ * iputs have to be done within the locked region.
+ */
 void fuse_release_background(struct fuse_conn *fc, struct fuse_req *req)
 {
 	iput(req->inode);
 	iput(req->inode2);
-	if (req->file)
-		fput(req->file);
 	spin_lock(&fc->lock);
 	list_del(&req->bg_entry);
 	if (fc->num_background == FUSE_MAX_BACKGROUND) {
@@ -178,6 +182,11 @@ static void request_end(struct fuse_conn
 		if (fc->mounted)
 			fuse_release_background(fc, req);
 		up_read(&fc->sbput_sem);
+
+		/* fput must go outside sbput_sem, otherwise it can deadlock */
+		if (req->file)
+			fput(req->file);
+
 		if (end)
 			end(fc, req);
 		else
-- 
1.2.4

[PATCH 3/4] [fuse] fix race between checking and setting file->private_data

[Miklos Szeredi]

BKL does not protect against races if the task may sleep between
checking and setting a value.  So move checking of file->private_data
near to setting it in fuse_fill_super().

Found by Al Viro.

Signed-off-by: Miklos Szeredi <miklos@szeredi.hu>

---

 fs/fuse/inode.c |   13 ++++++++-----
 1 files changed, 8 insertions(+), 5 deletions(-)

2e6c033a9b3a0e8b191b8d916364562a442c3955
diff --git a/fs/fuse/inode.c b/fs/fuse/inode.c
index fd34037..7627022 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -500,11 +500,6 @@ static int fuse_fill_super(struct super_
 	if (file->f_op != &fuse_dev_operations)
 		return -EINVAL;
 
-	/* Setting file->private_data can't race with other mount()
-	   instances, since BKL is held for ->get_sb() */
-	if (file->private_data)
-		return -EINVAL;
-
 	fc = new_conn();
 	if (!fc)
 		return -ENOMEM;
@@ -540,6 +535,12 @@ static int fuse_fill_super(struct super_
 	if (err)
 		goto err_free_req;
 
+	/* Setting file->private_data can't race with other mount()
+	   instances, since BKL is held for ->get_sb() */
+	err = -EINVAL;
+	if (file->private_data)
+		goto err_kobject_del;
+
 	sb->s_root = root_dentry;
 	fc->mounted = 1;
 	fc->connected = 1;
@@ -556,6 +557,8 @@ static int fuse_fill_super(struct super_
 
 	return 0;
 
+ err_kobject_del:
+	kobject_del(&fc->kobj);
  err_free_req:
 	fuse_request_free(init_req);
  err_put_root:
-- 
1.2.4

[PATCH 4/4] [doc] add paragraph about 'fs' subsystem to sysfs.txt

[Miklos Szeredi]

Signed-off-by: Miklos Szeredi <miklos@szeredi.hu>

---

 Documentation/filesystems/sysfs.txt |    5 +++++
 1 files changed, 5 insertions(+), 0 deletions(-)

0a2864e3522f3cd5f4447a6e0dd360fb9f9b2a7d
diff --git a/Documentation/filesystems/sysfs.txt b/Documentation/filesystems/sysfs.txt
index c8bce82..1b3a952 100644
--- a/Documentation/filesystems/sysfs.txt
+++ b/Documentation/filesystems/sysfs.txt
@@ -246,6 +246,7 @@ class/
 devices/
 firmware/
 net/
+fs/
 
 devices/ contains a filesystem representation of the device tree. It maps
 directly to the internal kernel device tree, which is a hierarchy of
@@ -264,6 +265,10 @@ drivers/ contains a directory for each d
 for devices on that particular bus (this assumes that drivers do not
 span multiple bus types).
 
+fs/ contains a directory for some filesystems.  Currently each
+filesystem wanting to export attributes must create it's own hierarchy
+below fs/ (see ./fuse.txt for an example).
+
 
 More information can driver-model specific features can be found in
 Documentation/driver-model/. 
-- 
1.2.4

Re: [PATCH 4/4] [doc] add paragraph about 'fs' subsystem to sysfs.txt

[Randy.Dunlap]

On Tue, 25 Apr 2006 10:40:43 +0200 Miklos Szeredi wrote:

> diff --git a/Documentation/filesystems/sysfs.txt b/Documentation/filesystems/sysfs.txt
> index c8bce82..1b3a952 100644
> --- a/Documentation/filesystems/sysfs.txt
> +++ b/Documentation/filesystems/sysfs.txt
> @@ -246,6 +246,7 @@ class/
>  devices/
>  firmware/
>  net/
> +fs/
>  
>  devices/ contains a filesystem representation of the device tree. It maps
>  directly to the internal kernel device tree, which is a hierarchy of
> @@ -264,6 +265,10 @@ drivers/ contains a directory for each d
>  for devices on that particular bus (this assumes that drivers do not
>  span multiple bus types).
>  
> +fs/ contains a directory for some filesystems.  Currently each
> +filesystem wanting to export attributes must create it's own hierarchy
> +below fs/ (see ./fuse.txt for an example).

s/it's/its/
"it's" == "it is", which wouldn't be appropriate here.

---
~Randy

Re: [git patch] fuse fixes

[Jeff Garzik]

Miklos Szeredi wrote:
> Linus,
> 
> Please pull from 'for-linus' branch of
> master.kernel.org:/pub/scm/linux/kernel/git/mszeredi/fuse.git

Reading the code...
> struct fuse_req *fuse_request_alloc(void)
> {
>         struct fuse_req *req = kmem_cache_alloc(fuse_req_cachep, SLAB_KERNEL);
>         if (req)
>                 fuse_request_init(req);
>         return req;
> }

This function is called from everywhere, and so, it looks like it should 
use SLAB_NOFS rather than SLAB_KERNEL.  I would audit every GFP_KERNEL 
and SLAB_KERNEL usage, and consider replacing with SLAB_NOFS or GFP_NOFS.

	Jeff

Re: [git patch] fuse fixes

[Miklos Szeredi]

> This function is called from everywhere, and so, it looks like it should 
> use SLAB_NOFS rather than SLAB_KERNEL.  I would audit every GFP_KERNEL 
> and SLAB_KERNEL usage, and consider replacing with SLAB_NOFS or GFP_NOFS.

GFP_NOFS doesn't make much sense, since mm never calls back into FUSE
anyway: FUSE writes through the page-cache, and hence never dirties
any pages.

I'll add a comment to fuse_request_alloc().

Thanks,
Miklos

Re: [git patch] fuse fixes

[Jeff Garzik]

Miklos Szeredi wrote:
>> This function is called from everywhere, and so, it looks like it should 
>> use SLAB_NOFS rather than SLAB_KERNEL.  I would audit every GFP_KERNEL 
>> and SLAB_KERNEL usage, and consider replacing with SLAB_NOFS or GFP_NOFS.
> 
> GFP_NOFS doesn't make much sense, since mm never calls back into FUSE
> anyway: FUSE writes through the page-cache, and hence never dirties
> any pages.
> 
> I'll add a comment to fuse_request_alloc().

If you're using loop, particularly something insane like swapping over 
loop, "the path" will certainly want to know that its passing through 
the VFS layer, regardless of specific page cache behavior, AFAICS.

	Jeff

Re: [git patch] fuse fixes

[Miklos Szeredi]

> >> This function is called from everywhere, and so, it looks like it should 
> >> use SLAB_NOFS rather than SLAB_KERNEL.  I would audit every GFP_KERNEL 
> >> and SLAB_KERNEL usage, and consider replacing with SLAB_NOFS or GFP_NOFS.
> > 
> > GFP_NOFS doesn't make much sense, since mm never calls back into FUSE
> > anyway: FUSE writes through the page-cache, and hence never dirties
> > any pages.
> > 
> > I'll add a comment to fuse_request_alloc().
> 
> If you're using loop, particularly something insane like swapping over 
> loop, "the path" will certainly want to know that its passing through 
> the VFS layer, regardless of specific page cache behavior, AFAICS.

IIRC "loop" decouples the base filesystem from any user of the loop
device by a kernel thread and mediating buffers.

Miklos