Yes, the add and remove cases do share the same basic loop and thelocking, but the compiler can inline and then CSE some of the end resultanyway. And splitting it up makes the code way easier to follow,and makes it clearer exactly what the semantics are.

In particular, we must make sure that the FASYNC flag in file->f_flagsexactly matches the state of "is this file on any fasync list", sincenot only is that flag visible to user space (F_GETFL), but we also usethat flag to check whether we need to remove any fasync entries on fileclose.

We got that wrong for the case of a mixed use of file locking (whichtries to remove any fasync entries for file leases) and fasync.

Splitting the function up also makes it possible to do some futureoptimizations without making the function even messier. In particular,since the FASYNC flag has to match the state of "is this on a list", wecan do the following future optimizations:

- on remove, we don't even need to get the locks and traverse the list if FASYNC isn't set, since we can know a priori that there is no point (this is effectively the same optimization that we already do in __fput() wrt removing fasync on file close)

- on add, we can use the FASYNC flag to decide whether we are changing an existing entry or need to allocate a new one.

/*- * fasync_helper() is used by almost all character device drivers- * to set up the fasync queue. It returns negative on error, 0 if it did- * no changes and positive if it added/deleted the entry.+ * Remove a fasync entry. If successfully removed, return+ * positive and clear the FASYNC flag. If no entry exists,+ * do nothing and return 0.+ *+ * NOTE! It is very important that the FASYNC flag always+ * match the state "is the filp on a fasync list".+ *+ * We always take the 'filp->f_lock', in since fasync_lock+ * needs to be irq-safe. */-int fasync_helper(int fd, struct file * filp, int on, struct fasync_struct **fapp)+static int fasync_remove_entry(struct file *filp, struct fasync_struct **fapp) { struct fasync_struct *fa, **fp;- struct fasync_struct *new = NULL; int result = 0;

+/*+ * fasync_helper() is used by almost all character device drivers+ * to set up the fasync queue, and for regular files by the file+ * lease code. It returns negative on error, 0 if it did no changes+ * and positive if it added/deleted the entry.+ */+int fasync_helper(int fd, struct file * filp, int on, struct fasync_struct **fapp)+{+ if (!on)+ return fasync_remove_entry(filp, fapp);+ return fasync_add_entry(fd, filp, fapp);+}+ EXPORT_SYMBOL(fasync_helper);