All of lore.kernel.org
 help / color / mirror / Atom feed
* [git patch] fuse fixes
@ 2006-04-25  8:35 Miklos Szeredi
  2006-04-25  8:38 ` [PATCH 1/4] Revert "[fuse] fix deadlock between fuse_put_super() and request_end()" Miklos Szeredi
                   ` (4 more replies)
  0 siblings, 5 replies; 13+ messages in thread
From: Miklos Szeredi @ 2006-04-25  8:35 UTC (permalink / raw)
  To: torvalds; +Cc: linux-kernel

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH 1/4] Revert "[fuse] fix deadlock between fuse_put_super() and request_end()"
  2006-04-25  8:35 [git patch] fuse fixes Miklos Szeredi
@ 2006-04-25  8:38 ` Miklos Szeredi
  2006-04-25 21:50   ` Linus Torvalds
  2006-04-25  8:39 ` [PATCH 2/4] [fuse] fix deadlock between fuse_put_super() and request_end() Miklos Szeredi
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 13+ messages in thread
From: Miklos Szeredi @ 2006-04-25  8:38 UTC (permalink / raw)
  To: torvalds; +Cc: linux-kernel

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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/4] [fuse] fix deadlock between fuse_put_super() and request_end()
  2006-04-25  8:35 [git patch] fuse fixes Miklos Szeredi
  2006-04-25  8:38 ` [PATCH 1/4] Revert "[fuse] fix deadlock between fuse_put_super() and request_end()" Miklos Szeredi
@ 2006-04-25  8:39 ` Miklos Szeredi
  2006-04-25  8:39 ` [PATCH 3/4] [fuse] fix race between checking and setting file->private_data Miklos Szeredi
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 13+ messages in thread
From: Miklos Szeredi @ 2006-04-25  8:39 UTC (permalink / raw)
  To: torvalds; +Cc: linux-kernel

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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/4] [fuse] fix race between checking and setting file->private_data
  2006-04-25  8:35 [git patch] fuse fixes Miklos Szeredi
  2006-04-25  8:38 ` [PATCH 1/4] Revert "[fuse] fix deadlock between fuse_put_super() and request_end()" Miklos Szeredi
  2006-04-25  8:39 ` [PATCH 2/4] [fuse] fix deadlock between fuse_put_super() and request_end() Miklos Szeredi
@ 2006-04-25  8:39 ` Miklos Szeredi
  2006-04-25  8:40 ` [PATCH 4/4] [doc] add paragraph about 'fs' subsystem to sysfs.txt Miklos Szeredi
  2006-04-26 20:03 ` [git patch] fuse fixes Jeff Garzik
  4 siblings, 0 replies; 13+ messages in thread
From: Miklos Szeredi @ 2006-04-25  8:39 UTC (permalink / raw)
  To: torvalds; +Cc: linux-kernel

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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 4/4] [doc] add paragraph about 'fs' subsystem to sysfs.txt
  2006-04-25  8:35 [git patch] fuse fixes Miklos Szeredi
                   ` (2 preceding siblings ...)
  2006-04-25  8:39 ` [PATCH 3/4] [fuse] fix race between checking and setting file->private_data Miklos Szeredi
@ 2006-04-25  8:40 ` Miklos Szeredi
  2006-04-25 15:52   ` Randy.Dunlap
  2006-04-26 20:03 ` [git patch] fuse fixes Jeff Garzik
  4 siblings, 1 reply; 13+ messages in thread
From: Miklos Szeredi @ 2006-04-25  8:40 UTC (permalink / raw)
  To: torvalds; +Cc: linux-kernel

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


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 4/4] [doc] add paragraph about 'fs' subsystem to sysfs.txt
  2006-04-25  8:40 ` [PATCH 4/4] [doc] add paragraph about 'fs' subsystem to sysfs.txt Miklos Szeredi
@ 2006-04-25 15:52   ` Randy.Dunlap
  0 siblings, 0 replies; 13+ messages in thread
From: Randy.Dunlap @ 2006-04-25 15:52 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: torvalds, linux-kernel

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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH 1/4] Revert "[fuse] fix deadlock between fuse_put_super() and request_end()"
  2006-04-25  8:38 ` [PATCH 1/4] Revert "[fuse] fix deadlock between fuse_put_super() and request_end()" Miklos Szeredi
@ 2006-04-25 21:50   ` Linus Torvalds
  0 siblings, 0 replies; 13+ messages in thread
From: Linus Torvalds @ 2006-04-25 21:50 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: Linux Kernel Mailing List



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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [git patch] fuse fixes
  2006-04-25  8:35 [git patch] fuse fixes Miklos Szeredi
                   ` (3 preceding siblings ...)
  2006-04-25  8:40 ` [PATCH 4/4] [doc] add paragraph about 'fs' subsystem to sysfs.txt Miklos Szeredi
@ 2006-04-26 20:03 ` Jeff Garzik
  2006-04-27  6:08   ` Miklos Szeredi
  4 siblings, 1 reply; 13+ messages in thread
From: Jeff Garzik @ 2006-04-26 20:03 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: torvalds, linux-kernel

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



^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [git patch] fuse fixes
  2006-04-26 20:03 ` [git patch] fuse fixes Jeff Garzik
@ 2006-04-27  6:08   ` Miklos Szeredi
  2006-04-27 10:33     ` Jeff Garzik
  0 siblings, 1 reply; 13+ messages in thread
From: Miklos Szeredi @ 2006-04-27  6:08 UTC (permalink / raw)
  To: jeff; +Cc: torvalds, linux-kernel

> 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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [git patch] fuse fixes
  2006-04-27  6:08   ` Miklos Szeredi
@ 2006-04-27 10:33     ` Jeff Garzik
  2006-04-27 14:38       ` Miklos Szeredi
  0 siblings, 1 reply; 13+ messages in thread
From: Jeff Garzik @ 2006-04-27 10:33 UTC (permalink / raw)
  To: Miklos Szeredi; +Cc: torvalds, linux-kernel

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




^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [git patch] fuse fixes
  2006-04-27 10:33     ` Jeff Garzik
@ 2006-04-27 14:38       ` Miklos Szeredi
  0 siblings, 0 replies; 13+ messages in thread
From: Miklos Szeredi @ 2006-04-27 14:38 UTC (permalink / raw)
  To: jeff; +Cc: torvalds, linux-kernel

> >> 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

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [git patch] fuse fixes
@ 2006-04-26  9:15 Miklos Szeredi
  0 siblings, 0 replies; 13+ messages in thread
From: Miklos Szeredi @ 2006-04-26  9:15 UTC (permalink / raw)
  To: torvalds; +Cc: linux-kernel

Linus,

I've addressed the comments from the previous submission:

  - Added proper commit message to revert
  - Fixed typo in sysfs.txt

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(), try #2
      [fuse] fix race between checking and setting file->private_data
      [doc] add paragraph about 'fs' subsystem to sysfs.txt


^ permalink raw reply	[flat|nested] 13+ messages in thread

* [git patch] fuse fixes
@ 2006-04-11 19:42 Miklos Szeredi
  0 siblings, 0 replies; 13+ messages in thread
From: Miklos Szeredi @ 2006-04-11 19:42 UTC (permalink / raw)
  To: torvalds; +Cc: linux-kernel

Linus,

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

to receive the following updates:

 fs/fuse/dev.c    |   54 +++++++++++++++++++++++++++++++++++-------------------
 fs/fuse/file.c   |   10 +++++++---
 fs/fuse/fuse_i.h |   15 ++++++---------
 fs/fuse/inode.c  |   27 +++++++++++++++++----------
 4 files changed, 65 insertions(+), 41 deletions(-)

Miklos Szeredi:
      [fuse] fix deadlock between fuse_put_super() and request_end()
      [fuse] Fix accounting the number of waiting requests
      [fuse] Don't init request twice
      [fuse] Direct I/O  should not use fuse_reset_request

diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c
index 6c740f8..cc750c6 100644
--- a/fs/fuse/dev.c
+++ b/fs/fuse/dev.c
@@ -92,48 +92,50 @@ struct fuse_req *fuse_get_req(struct fus
 {
 	struct fuse_req *req;
 	sigset_t oldset;
+	int intr;
 	int err;
 
+	atomic_inc(&fc->num_waiting);
 	block_sigs(&oldset);
-	err = wait_event_interruptible(fc->blocked_waitq, !fc->blocked);
+	intr = wait_event_interruptible(fc->blocked_waitq, !fc->blocked);
 	restore_sigs(&oldset);
-	if (err)
-		return ERR_PTR(-EINTR);
+	err = -EINTR;
+	if (intr)
+		goto out;
 
 	req = fuse_request_alloc();
+	err = -ENOMEM;
 	if (!req)
-		return ERR_PTR(-ENOMEM);
+		goto out;
 
-	atomic_inc(&fc->num_waiting);
-	fuse_request_init(req);
 	req->in.h.uid = current->fsuid;
 	req->in.h.gid = current->fsgid;
 	req->in.h.pid = current->pid;
+	req->waiting = 1;
 	return req;
+
+ out:
+	atomic_dec(&fc->num_waiting);
+	return ERR_PTR(err);
 }
 
 void fuse_put_request(struct fuse_conn *fc, struct fuse_req *req)
 {
 	if (atomic_dec_and_test(&req->count)) {
-		atomic_dec(&fc->num_waiting);
+		if (req->waiting)
+			atomic_dec(&fc->num_waiting);
 		fuse_request_free(req);
 	}
 }
 
-void fuse_release_background(struct fuse_conn *fc, struct fuse_req *req)
+void fuse_remove_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);
+	list_del_init(&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);
 }
 
 /*
@@ -163,17 +165,27 @@ 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);
 	}
 }
 
@@ -277,6 +289,10 @@ static void queue_request(struct fuse_co
 		len_args(req->in.numargs, (struct fuse_arg *) req->in.args);
 	list_add_tail(&req->list, &fc->pending);
 	req->state = FUSE_REQ_PENDING;
+	if (!req->waiting) {
+		req->waiting = 1;
+		atomic_inc(&fc->num_waiting);
+	}
 	wake_up(&fc->waitq);
 	kill_fasync(&fc->fasync, SIGIO, POLL_IN);
 }
diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index e4f041a..fc342cf 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1,6 +1,6 @@
 /*
   FUSE: Filesystem in Userspace
-  Copyright (C) 2001-2005  Miklos Szeredi <miklos@szeredi.hu>
+  Copyright (C) 2001-2006  Miklos Szeredi <miklos@szeredi.hu>
 
   This program can be distributed under the terms of the GNU GPL.
   See the file COPYING.
@@ -565,8 +565,12 @@ static ssize_t fuse_direct_io(struct fil
 		buf += nres;
 		if (nres != nbytes)
 			break;
-		if (count)
-			fuse_reset_request(req);
+		if (count) {
+			fuse_put_request(fc, req);
+			req = fuse_get_req(fc);
+			if (IS_ERR(req))
+				break;
+		}
 	}
 	fuse_put_request(fc, req);
 	if (res > 0) {
diff --git a/fs/fuse/fuse_i.h b/fs/fuse/fuse_i.h
index 19c7185..59661c4 100644
--- a/fs/fuse/fuse_i.h
+++ b/fs/fuse/fuse_i.h
@@ -159,6 +159,9 @@ struct fuse_req {
 	/** Data is being copied to/from the request */
 	unsigned locked:1;
 
+	/** Request is counted as "waiting" */
+	unsigned waiting:1;
+
 	/** State of the request */
 	enum fuse_req_state state;
 
@@ -255,15 +258,9 @@ 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;
@@ -474,11 +471,11 @@ void request_send_noreply(struct fuse_co
 void request_send_background(struct fuse_conn *fc, struct fuse_req *req);
 
 /**
- * Release inodes and file associated with background request
+ * Remove request from the the background list
  */
-void fuse_release_background(struct fuse_conn *fc, struct fuse_req *req);
+void fuse_remove_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 fd34037..43a6fc0 100644
--- a/fs/fuse/inode.c
+++ b/fs/fuse/inode.c
@@ -204,17 +204,26 @@ 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);
@@ -386,7 +395,6 @@ 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);
@@ -541,7 +549,6 @@ 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;

^ permalink raw reply related	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2006-04-27 14:39 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2006-04-25  8:35 [git patch] fuse fixes Miklos Szeredi
2006-04-25  8:38 ` [PATCH 1/4] Revert "[fuse] fix deadlock between fuse_put_super() and request_end()" Miklos Szeredi
2006-04-25 21:50   ` Linus Torvalds
2006-04-25  8:39 ` [PATCH 2/4] [fuse] fix deadlock between fuse_put_super() and request_end() Miklos Szeredi
2006-04-25  8:39 ` [PATCH 3/4] [fuse] fix race between checking and setting file->private_data Miklos Szeredi
2006-04-25  8:40 ` [PATCH 4/4] [doc] add paragraph about 'fs' subsystem to sysfs.txt Miklos Szeredi
2006-04-25 15:52   ` Randy.Dunlap
2006-04-26 20:03 ` [git patch] fuse fixes Jeff Garzik
2006-04-27  6:08   ` Miklos Szeredi
2006-04-27 10:33     ` Jeff Garzik
2006-04-27 14:38       ` Miklos Szeredi
  -- strict thread matches above, loose matches on Subject: below --
2006-04-26  9:15 Miklos Szeredi
2006-04-11 19:42 Miklos Szeredi

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.