[PATCH] vfs: does call expand_files when needed

[PATCH] vfs: does call expand_files when needed

[Liu Aleaxander]

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);
+		if (error < 0)
+			goto out;
+		
+		/*
+		 * If we needed to expand the fs array we
+		 * might have blocked - try again.
+		 */
+		if (error)
+			goto repeat;
+	}
+		
 	if (start <= files->next_fd)
 		files->next_fd = fd + 1;

-- 
1.6.2.5

--
regards
Liu Aleaxander

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

[Américo Wang]

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.


> +               if (error < 0)
> +                       goto out;
> +
> +               /*
> +                * If we needed to expand the fs array we
> +                * might have blocked - try again.
> +                */
> +               if (error)
> +                       goto repeat;
> +       }
> +
>        if (start <= files->next_fd)
>                files->next_fd = fd + 1;
>
> --
> 1.6.2.5
>
> --
> regards
> Liu Aleaxander
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>
--
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

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

[Liu Aleaxander]

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?

>
> > +               if (error < 0)
> > +                       goto out;
> > +
> > +               /*
> > +                * If we needed to expand the fs array we
> > +                * might have blocked - try again.
> > +                */
> > +               if (error)
> > +                       goto repeat;
> > +       }
> > +
> >        if (start <= files->next_fd)
> >                files->next_fd = fd + 1;
> >
> > --
> > 1.6.2.5
> >
> > --
> > regards
> > Liu Aleaxander
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/
> >



--
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

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

[Américo Wang]

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, it's trivial, not so much an
improvement, IMO.
--
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

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

[Liu Aleaxander]

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

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

[Américo Wang]

On Wed, Nov 18, 2009 at 4:54 PM, Liu Aleaxander <aleaxander@gmail.com> wrote:

<snip>

>
>>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.


I am not rejecting it, I said this is trivial, so accepting it or droping
it both are OK for me.

I don't think the orignal code is ugly, '< fdt->max_fds' is not checked
for expand_files(), but for find_next_zero_bit().

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

[Liu Aleaxander]

On Wed, Nov 18, 2009 at 5:22 PM, Américo Wang <xiyou.wangcong@gmail.com> wrote:
> On Wed, Nov 18, 2009 at 4:54 PM, Liu Aleaxander <aleaxander@gmail.com> wrote:
>
> <snip>
>
>>
>>>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.
>
>
> I am not rejecting it, I said this is trivial, so accepting it or droping
> it both are OK for me.
>
> I don't think the orignal code is ugly,
I didn't say the old code is ugly either. ;)

>'< fdt->max_fds' is not checked for expand_files(), but for find_next_zero_bit().
According to the old code, it's true, but it can also be applied to
expand_files checking. And just like what I said, we did rarely need
expand the file table as usual; even though we need, one-time expand
will be enough for a long while as it doubles the original size. (Am I
right?).



-- 
regards
Liu Aleaxander