Re: [PATCH] vfs: does call expand_files when needed

[Liu Aleaxander <aleaxander@gmail.com>]

On Wed, Nov 18, 2009 at 4:35 PM, Américo Wang <xiyou.wangcong@gmail.com> wrote:
> On Wed, Nov 18, 2009 at 3:41 PM, Liu Aleaxander <aleaxander@gmail.com> wrote:
>> On Wed, Nov 18, 2009 at 3:17 PM, Américo Wang <xiyou.wangcong@gmail.com> wrote:
>>>
>>> On Wed, Nov 18, 2009 at 1:54 PM, Liu Aleaxander <aleaxander@gmail.com> wrote:
>>> > From: Liu Aleaxander <Aleaxander@gmail.com>
>>> > Date: Wed, 18 Nov 2009 10:59:09 +0800
>>> > Subject: [PATCH] vfs: does call expand_files when needed
>>> >
>>> > I don't think we should call expand_files every time we open a
>>> > file for a new unused fd, so does the expand when necessary.
>>> >
>>> > Signed-off-by: Liu Aleaxander <Aleaxander@gmail.com>
>>> > ---
>>> >  fs/file.c |   27 ++++++++++++++-------------
>>> >  1 files changed, 14 insertions(+), 13 deletions(-)
>>> >
>>> > diff --git a/fs/file.c b/fs/file.c
>>> > index 87e1290..3f3d0fc 100644
>>> > --- a/fs/file.c
>>> > +++ b/fs/file.c
>>> > @@ -452,22 +452,22 @@ repeat:
>>> >        if (fd < files->next_fd)
>>> >                fd = files->next_fd;
>>> >
>>> > -       if (fd < fdt->max_fds)
>>> > +       if (likely(fd < fdt->max_fds)) {
>>> >                fd = find_next_zero_bit(fdt->open_fds->fds_bits,
>>> >                                           fdt->max_fds, fd);
>>> > -
>>> > -       error = expand_files(files, fd);
>>> > -       if (error < 0)
>>> > -               goto out;
>>> > -
>>> > -       /*
>>> > -        * If we needed to expand the fs array we
>>> > -        * might have blocked - try again.
>>> > -        */
>>> > -       if (error)
>>> > -               goto repeat;
>>> > -
>>> > +       } else {
>>> > +               error = expand_files(files, fd);
>>>
>>>
>>> In expand_files(), it has the check for
>>> ' < fdt->max_fds', so this change is not necessary.
>>
>> Yeah, indeed. But why we should go into an another function to do a
>> _double_ check especially we mostly don't need to do that?
>
> You only optimized one call path,
Yes, and that's the intent of this patch.

>it's trivial, not so much an improvement, IMO.
So, shouldn't we do the optimize when there is a way to do that?

While, I don't think so. And BTW, it's not just a problem of
optimization, but also make it be more sense: JUST call expand when
need. I don't know why you are rejecting about this, especially it did
optimized one call path(as you said), and it doesn't make the code
uglier than before but making it be more sense, and, in fact, a kind
of more readable.

-- 
regards
Liu Aleaxander
--
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