| Message ID | 1446781368-13766-1-git-send-email-ebiggers3@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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
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);
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(-)