On Wed, Oct 27, 2010 at 11:20 AM, Arnd Bergmann <arnd@xxxxxxxx> wrote:
>
> Ok, then let's use your tree with proper changelogs. That should be the
> easiest way to make sure that the code you test is the one that gets in.
Half of them are yours, and the one that is mine needs the comment
fixup. Can you organize it all for me to pull?
Here's my patch again, with the comment move and a suggested commit
message (and my sign-off and Bruce's tested-by: the only thing that
changed in the patch was literally some comment placement so the
tested-by should still be perfectly valid).
Feel free to edit the message/patch to your hearts content.
Thanks,
Linus
From: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
Subject: fasync: re-organize fasync entry insertion to allow it under a spinlock
You currently cannot use "fasync_helper()" in an atomic environment to
insert a new fasync entry, because it will need to allocate the new
"struct fasync_struct".
Yet fcntl_setlease() wants to call this under lock_flocks(), which is in
the process of being converted from the BKL to a spinlock.
In order to fix this, this abstracts out the actual fasync list
insertion and the fasync allocations into functions of their own, and
teaches fs/locks.c to pre-allocate the fasync_struct entry. That way
the actual list insertion can happen while holding the required
spinlock.
Signed-off-by: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
Tested-by: Bruce Fields <bfields@xxxxxxxxxxxx>
---
fs/fcntl.c | 62 ++++++++++++++++++++++++++++++++++++++++-----------
fs/locks.c | 17 +++++++++++++-
include/linux/fs.h | 5 ++++
3 files changed, 69 insertions(+), 15 deletions(-)
diff --git a/fs/fcntl.c b/fs/fcntl.c
index f8cc34f..ecc8b39 100644
--- a/fs/fcntl.c
+++ b/fs/fcntl.c
@@ -640,7 +640,7 @@ static void fasync_free_rcu(struct rcu_head *head)
* match the state "is the filp on a fasync list".
*
*/
-static int fasync_remove_entry(struct file *filp, struct fasync_struct **fapp)
+int fasync_remove_entry(struct file *filp, struct fasync_struct **fapp)
{
struct fasync_struct *fa, **fp;
int result = 0;
@@ -666,21 +666,31 @@ static int fasync_remove_entry(struct file *filp, struct fasync_struct **fapp)
return result;
}
+struct fasync_struct *fasync_alloc(void)
+{
+ return kmem_cache_alloc(fasync_cache, GFP_KERNEL);
+}
+
/*
- * Add a fasync entry. Return negative on error, positive if
- * added, and zero if did nothing but change an existing one.
+ * NOTE! This can be used only for unused fasync entries:
+ * entries that actually got inserted on the fasync list
+ * need to be released by rcu - see fasync_remove_entry.
+ */
+void fasync_free(struct fasync_struct *new)
+{
+ kmem_cache_free(fasync_cache, new);
+}
+
+/*
+ * Insert a new entry into the fasync list. Return the pointer to the
+ * old one if we didn't use the new one.
*
* NOTE! It is very important that the FASYNC flag always
* match the state "is the filp on a fasync list".
*/
-static int fasync_add_entry(int fd, struct file *filp, struct fasync_struct **fapp)
+struct fasync_struct *fasync_insert_entry(int fd, struct file *filp, struct fasync_struct **fapp, struct fasync_struct *new)
{
- struct fasync_struct *new, *fa, **fp;
- int result = 0;
-
- new = kmem_cache_alloc(fasync_cache, GFP_KERNEL);
- if (!new)
- return -ENOMEM;
+ struct fasync_struct *fa, **fp;
spin_lock(&filp->f_lock);
spin_lock(&fasync_lock);
@@ -691,8 +701,6 @@ static int fasync_add_entry(int fd, struct file *filp, struct fasync_struct **fa
spin_lock_irq(&fa->fa_lock);
fa->fa_fd = fd;
spin_unlock_irq(&fa->fa_lock);
-
- kmem_cache_free(fasync_cache, new);
goto out;
}
@@ -702,13 +710,39 @@ static int fasync_add_entry(int fd, struct file *filp, struct fasync_struct **fa
new->fa_fd = fd;
new->fa_next = *fapp;
rcu_assign_pointer(*fapp, new);
- result = 1;
filp->f_flags |= FASYNC;
out:
spin_unlock(&fasync_lock);
spin_unlock(&filp->f_lock);
- return result;
+ return fa;
+}
+
+/*
+ * Add a fasync entry. Return negative on error, positive if
+ * added, and zero if did nothing but change an existing one.
+ */
+static int fasync_add_entry(int fd, struct file *filp, struct fasync_struct **fapp)
+{
+ struct fasync_struct *new;
+
+ new = fasync_alloc();
+ if (!new)
+ return -ENOMEM;
+
+ /*
+ * fasync_insert_entry() returns the old (update) entry if
+ * it existed.
+ *
+ * So free the (unused) new entry and return 0 to let the
+ * caller know that we didn't add any new fasync entries.
+ */
+ if (fasync_insert_entry(fd, filp, fapp, new)) {
+ fasync_free(new);
+ return 0;
+ }
+
+ return 1;
}
/*
diff --git a/fs/locks.c b/fs/locks.c
index 4de3a26..9ff3f66 100644
--- a/fs/locks.c
+++ b/fs/locks.c
@@ -1515,6 +1515,7 @@ EXPORT_SYMBOL_GPL(vfs_setlease);
int fcntl_setlease(unsigned int fd, struct file *filp, long arg)
{
struct file_lock fl, *flp = &fl;
+ struct fasync_struct *new;
struct inode *inode = filp->f_path.dentry->d_inode;
int error;
@@ -1523,13 +1524,25 @@ int fcntl_setlease(unsigned int fd, struct file *filp, long arg)
if (error)
return error;
+ new = fasync_alloc();
+ if (!new)
+ return -ENOMEM;
+
lock_flocks();
error = __vfs_setlease(filp, arg, &flp);
if (error || arg == F_UNLCK)
goto out_unlock;
- error = fasync_helper(fd, filp, 1, &flp->fl_fasync);
+ /*
+ * fasync_insert_entry() returns the old entry if any.
+ * If there was no old entry, then it used 'new' and
+ * inserted it into the fasync list. Clear new so that
+ * we don't release it here.
+ */
+ if (!fasync_insert_entry(fd, filp, &flp->fl_fasync, new))
+ new = NULL;
+
if (error < 0) {
/* remove lease just inserted by setlease */
flp->fl_type = F_UNLCK | F_INPROGRESS;
@@ -1541,6 +1554,8 @@ int fcntl_setlease(unsigned int fd, struct file *filp, long arg)
error = __f_setown(filp, task_pid(current), PIDTYPE_PID, 0);
out_unlock:
unlock_flocks();
+ if (new)
+ fasync_free(new);
return error;
}
diff --git a/include/linux/fs.h b/include/linux/fs.h
index 240eb1d..d487772 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1310,6 +1310,11 @@ struct fasync_struct {
/* SMP safe fasync helpers: */
extern int fasync_helper(int, struct file *, int, struct fasync_struct **);
+extern struct fasync_struct *fasync_insert_entry(int, struct file *, struct fasync_struct **, struct fasync_struct *);
+extern int fasync_remove_entry(struct file *, struct fasync_struct **);
+extern struct fasync_struct *fasync_alloc(void);
+extern void fasync_free(struct fasync_struct *);
+
/* can be called from interrupts */
extern void kill_fasync(struct fasync_struct **, int, int);
