Patchwork ext4: limit group search loop for non-extent files

login
register
mail settings
Submitter Eric Sandeen
Date May 3, 2013, 8:50 p.m.
Message ID <5184230E.6020500@redhat.com>
Download mbox | patch
Permalink /patch/241399/
State Accepted
Headers show

Comments

Eric Sandeen - May 3, 2013, 8:50 p.m.
From: Lachlan McIlroy <lmcilroy@redhat.com>

In the case where we are allocating for a non-extent file,
we must limit the groups we allocate from to those below
2^32 blocks, and ext4_mb_regular_allocator() attempts to
do this initially by putting a cap on ngroups for the
subsequent search loop.

However, the initial target group comes in from the 
allocation context (ac), and it may already be beyond
the artificially limited ngroups.  In this case,
the limit

	if (group == ngroups)
		group = 0;

at the top of the loop is never true, and the loop will
run away.

Catch this case inside the loop and reset the search to
start at group 0.

[sandeen@redhat.com: add commit msg & comments]

Signed-off-by: Eric Sandeen <sandeen@redhat.com>
---

p.s. I suppose this limit could go after the assignment
from ac->ac_g_ex.fe_group, depending on your sense
of style...


--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lachlan McIlroy - May 6, 2013, 12:26 a.m.
----- Original Message -----
> From: Lachlan McIlroy <lmcilroy@redhat.com>
> 
> In the case where we are allocating for a non-extent file,
> we must limit the groups we allocate from to those below
> 2^32 blocks, and ext4_mb_regular_allocator() attempts to
> do this initially by putting a cap on ngroups for the
> subsequent search loop.
> 
> However, the initial target group comes in from the
> allocation context (ac), and it may already be beyond
> the artificially limited ngroups.  In this case,
> the limit
> 
> 	if (group == ngroups)
> 		group = 0;
> 
> at the top of the loop is never true, and the loop will
> run away.
> 
> Catch this case inside the loop and reset the search to
> start at group 0.
> 
> [sandeen@redhat.com: add commit msg & comments]
> 
> Signed-off-by: Eric Sandeen <sandeen@redhat.com>
> ---
> 
> p.s. I suppose this limit could go after the assignment
> from ac->ac_g_ex.fe_group, depending on your sense
> of style...
> 
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index ee6614b..f54baee 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -1994,7 +1994,11 @@ repeat:
>  		group = ac->ac_g_ex.fe_group;
>  
>  		for (i = 0; i < ngroups; group++, i++) {
> -			if (group == ngroups)
> +			/*
> +			 * Artificially restricted ngroups for non-extent
> +			 * files makes group > ngroups possible on first loop.
> +			 */
> +			if (group >= ngroups)
>  				group = 0;
>  
>  			/* This now checks without needing the buddy page */
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Thanks for following through with this Eric.  Looks good to me.
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Theodore Ts'o - May 6, 2013, 3:08 a.m.
On Fri, May 03, 2013 at 03:50:22PM -0500, Eric Sandeen wrote:
> From: Lachlan McIlroy <lmcilroy@redhat.com>

Note: if it's from Lachlan, it needs his Signed-off-by.  Lachlan, can
you send an Ack saying it's OK for me to add a:

Signed-off-by: Lachlan McIlroy <lmcilroy@redhat.com>

to the commit description?

Thanks,

						- Ted
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Lachlan McIlroy - May 6, 2013, 4:56 a.m.
Ack.  Yes Ted that's fine with me to add my Signed-off-by.  Thanks.

----- Original Message -----
> On Fri, May 03, 2013 at 03:50:22PM -0500, Eric Sandeen wrote:
> > From: Lachlan McIlroy <lmcilroy@redhat.com>
> 
> Note: if it's from Lachlan, it needs his Signed-off-by.  Lachlan, can
> you send an Ack saying it's OK for me to add a:
> 
> Signed-off-by: Lachlan McIlroy <lmcilroy@redhat.com>
> 
> to the commit description?
> 
> Thanks,
> 
> 						- Ted
> 
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Theodore Ts'o - May 6, 2013, 1:48 p.m.
On Mon, May 06, 2013 at 12:56:02AM -0400, Lachlan McIlroy wrote:
> Ack.  Yes Ted that's fine with me to add my Signed-off-by.  Thanks.

Thanks!

I've also tagged this with a cc:stable@vger.kernel.org.

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

Patch

diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
index ee6614b..f54baee 100644
--- a/fs/ext4/mballoc.c
+++ b/fs/ext4/mballoc.c
@@ -1994,7 +1994,11 @@  repeat:
 		group = ac->ac_g_ex.fe_group;
 
 		for (i = 0; i < ngroups; group++, i++) {
-			if (group == ngroups)
+			/*
+			 * Artificially restricted ngroups for non-extent
+			 * files makes group > ngroups possible on first loop.
+			 */
+			if (group >= ngroups)
 				group = 0;
 
 			/* This now checks without needing the buddy page */