vfs: clear remainder of 'full_fds_bits' in dup_fd()
diff mbox

Message ID 1446781368-13766-1-git-send-email-ebiggers3@gmail.com
State New
Headers show

Commit Message

Eric Biggers Nov. 6, 2015, 3:42 a.m. UTC
This fixes a bug from commit f3f86e33dc3d ("vfs: Fix pathological
performance case for __alloc_fd()").

Signed-off-by: Eric Biggers <ebiggers3@gmail.com>
---
 fs/file.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

Comments

Linus Torvalds Nov. 6, 2015, 4:15 a.m. UTC | #1
On Thu, Nov 5, 2015 at 7:42 PM, Eric Biggers <ebiggers3@gmail.com> wrote:
> This fixes a bug from commit f3f86e33dc3d ("vfs: Fix pathological
> performance case for __alloc_fd()").

Gaah. Good catch. The first version of that patch allocated the
full_fds_bits array separately and always cleared it (using kzalloc),
but then people hated on that..  So I did the "obvious" thing to just
allocate it as part of the same allocation as open_fds. And didn't
think about that thay does to the dup_fd() code.

That said, the more I look at your patch, the more I hate it. Not
because your patch is wrong, but because dup_fd() is such a nasty
horrid mess, and your patch looks bad just because that function is
bad.

Just look at what "copy_fdtable()" does: it's the exact same bitmap
copy, but it actually makes more sense there. Well, at least it's more
compact without the very odd "let's drop the spinlock in the middle
and clear the end later. That optimization just doesn't seem to be
worth it. Especially since we don't do it for the expend_fdtable()
case.

So what I would do is to just extract out the bitmap copying from
"copy_fdtable()" (call it "copy_fd_bitmaps()"?) and then use that for
both copy_fdtable() and for dup_fd(). And then I guess you could leave
the

  memset(new_fds, 0, size);

outside the spinlock still, but at least have the bitmap copying in
one sane place rather than spread out oddly like that.

Would you mind doing that version of the patch instead? I can do it
too, but I'd rather give authorship to you, since you found the stupid
issue, which is actually the much bigger deal.

                   Linus
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Biggers Nov. 6, 2015, 4:33 a.m. UTC | #2
On Thu, Nov 05, 2015 at 08:15:49PM -0800, Linus Torvalds wrote:
> Would you mind doing that version of the patch instead? I can do it
> too, but I'd rather give authorship to you, since you found the stupid
> issue, which is actually the much bigger deal.

Sure, I'll try it that way and see what it looks like.

Eric
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch
diff mbox

diff --git a/fs/file.c b/fs/file.c
index 6f6eb2b..36e5103 100644
--- a/fs/file.c
+++ b/fs/file.c
@@ -276,7 +276,7 @@  struct files_struct *dup_fd(struct files_struct *oldf, int *errorp)
 {
 	struct files_struct *newf;
 	struct file **old_fds, **new_fds;
-	int open_files, size, i;
+	int open_files, i;
 	struct fdtable *old_fdt, *new_fdt;
 
 	*errorp = -ENOMEM;
@@ -357,18 +357,16 @@  struct files_struct *dup_fd(struct files_struct *oldf, int *errorp)
 	}
 	spin_unlock(&oldf->file_lock);
 
-	/* compute the remainder to be cleared */
-	size = (new_fdt->max_fds - open_files) * sizeof(struct file *);
-
-	/* This is long word aligned thus could use a optimized version */
-	memset(new_fds, 0, size);
-
+	/* clear the remainder if needed */
 	if (new_fdt->max_fds > open_files) {
-		int left = (new_fdt->max_fds - open_files) / 8;
+		int left = new_fdt->max_fds - open_files;
 		int start = open_files / BITS_PER_LONG;
 
-		memset(&new_fdt->open_fds[start], 0, left);
-		memset(&new_fdt->close_on_exec[start], 0, left);
+		memset(new_fds, 0, left * sizeof(struct file *));
+		memset(&new_fdt->open_fds[start], 0, left / 8);
+		memset(&new_fdt->close_on_exec[start], 0, left / 8);
+		memset(&new_fdt->full_fds_bits[BITBIT_NR(open_files)], 0,
+		       BITBIT_SIZE(new_fdt->max_fds) - BITBIT_SIZE(open_files));
 	}
 
 	rcu_assign_pointer(newf->fdt, new_fdt);