On Thu, Nov 5, 2015 at 7:42 PM, Eric Biggers <ebiggers3@xxxxxxxxx> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
