Patchwork [01/31] tune2fs: Don't convert block # to cluster # when clearing uninit_bg

login
register
mail settings
Submitter Darrick J. Wong
Date Oct. 1, 2013, 1:26 a.m.
Message ID <20131001012649.28415.17225.stgit@birch.djwong.org>
Download mbox | patch
Permalink /patch/279281/
State Accepted
Headers show

Comments

Darrick J. Wong - Oct. 1, 2013, 1:26 a.m.
When we're constructing the initial block bitmap as part of removing the
gdt_csum (i.e. uninit_bg) feature, we mustn't convert the block numbers to
cluster numbers because ext2fs_mark_block_bitmap2() does this for us.

Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
---
 misc/tune2fs.c |   13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)



--
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
Lukas Czerner - Oct. 3, 2013, 4:53 p.m.
On Mon, 30 Sep 2013, Darrick J. Wong wrote:

> Date: Mon, 30 Sep 2013 18:26:49 -0700
> From: Darrick J. Wong <darrick.wong@oracle.com>
> To: tytso@mit.edu, darrick.wong@oracle.com
> Cc: linux-ext4@vger.kernel.org
> Subject: [PATCH 01/31] tune2fs: Don't convert block # to cluster # when
>     clearing uninit_bg
> 
> When we're constructing the initial block bitmap as part of removing the
> gdt_csum (i.e. uninit_bg) feature, we mustn't convert the block numbers to
> cluster numbers because ext2fs_mark_block_bitmap2() does this for us.

I wonder if it's possible to use the old-style bitmap interface (the
one which does not understand 64-bit bitmaps). If so, then you also
need to fix ext2fs_mark_generic_bmap() (and others) so that we
convert blocks to clusters if needed.

Thanks!
-Lukas

> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
>  misc/tune2fs.c |   13 +++++--------
>  1 file changed, 5 insertions(+), 8 deletions(-)
> 
> 
> diff --git a/misc/tune2fs.c b/misc/tune2fs.c
> index ddf3259..52247e0 100644
> --- a/misc/tune2fs.c
> +++ b/misc/tune2fs.c
> @@ -820,20 +820,17 @@ static void disable_uninit_bg(ext2_filsys fs, __u32 csum_feature_flag)
>  	/* The bbitmap is zeroed; we must mark group metadata blocks in use */
>  	for (i = 0; i < fs->group_desc_count; i++) {
>  		b = ext2fs_block_bitmap_loc(fs, i);
> -		ext2fs_mark_block_bitmap2(fs->block_map, EXT2FS_B2C(fs, b));
> +		ext2fs_mark_block_bitmap2(fs->block_map, b);
>  		b = ext2fs_inode_bitmap_loc(fs, i);
> -		ext2fs_mark_block_bitmap2(fs->block_map, EXT2FS_B2C(fs, b));
> +		ext2fs_mark_block_bitmap2(fs->block_map, b);
>  
>  		retval = ext2fs_super_and_bgd_loc2(fs, i, &b, &c, &d, NULL);
>  		if (retval == 0 && b)
> -			ext2fs_mark_block_bitmap2(fs->block_map,
> -						  EXT2FS_B2C(fs, b));
> +			ext2fs_mark_block_bitmap2(fs->block_map, b);
>  		if (retval == 0 && c)
> -			ext2fs_mark_block_bitmap2(fs->block_map,
> -						  EXT2FS_B2C(fs, c));
> +			ext2fs_mark_block_bitmap2(fs->block_map, c);
>  		if (retval == 0 && d)
> -			ext2fs_mark_block_bitmap2(fs->block_map,
> -						  EXT2FS_B2C(fs, d));
> +			ext2fs_mark_block_bitmap2(fs->block_map, d);
>  		if (retval) {
>  			com_err("disable_uninit_bg", retval,
>  				"while initializing block bitmaps");
> 
> --
> 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
> 
--
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
Darrick J. Wong - Oct. 3, 2013, 7:04 p.m.
On Thu, Oct 03, 2013 at 06:53:38PM +0200, Lukáš Czerner wrote:
> On Mon, 30 Sep 2013, Darrick J. Wong wrote:
> 
> > Date: Mon, 30 Sep 2013 18:26:49 -0700
> > From: Darrick J. Wong <darrick.wong@oracle.com>
> > To: tytso@mit.edu, darrick.wong@oracle.com
> > Cc: linux-ext4@vger.kernel.org
> > Subject: [PATCH 01/31] tune2fs: Don't convert block # to cluster # when
> >     clearing uninit_bg
> > 
> > When we're constructing the initial block bitmap as part of removing the
> > gdt_csum (i.e. uninit_bg) feature, we mustn't convert the block numbers to
> > cluster numbers because ext2fs_mark_block_bitmap2() does this for us.
> 
> I wonder if it's possible to use the old-style bitmap interface (the
> one which does not understand 64-bit bitmaps). If so, then you also
> need to fix ext2fs_mark_generic_bmap() (and others) so that we
> convert blocks to clusters if needed.

It's possible to do this (bad thing) by not specifying EXT2_FLAG_64BITS when
calling ext2fs_openfs(); see patch 26 for a quick fix.

(I'm imagining that most bigalloc users probably want 64bit anyway?)

(No idea, bigalloc isn't (currently) my use case.)

--D
> 
> Thanks!
> -Lukas
> 
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> >  misc/tune2fs.c |   13 +++++--------
> >  1 file changed, 5 insertions(+), 8 deletions(-)
> > 
> > 
> > diff --git a/misc/tune2fs.c b/misc/tune2fs.c
> > index ddf3259..52247e0 100644
> > --- a/misc/tune2fs.c
> > +++ b/misc/tune2fs.c
> > @@ -820,20 +820,17 @@ static void disable_uninit_bg(ext2_filsys fs, __u32 csum_feature_flag)
> >  	/* The bbitmap is zeroed; we must mark group metadata blocks in use */
> >  	for (i = 0; i < fs->group_desc_count; i++) {
> >  		b = ext2fs_block_bitmap_loc(fs, i);
> > -		ext2fs_mark_block_bitmap2(fs->block_map, EXT2FS_B2C(fs, b));
> > +		ext2fs_mark_block_bitmap2(fs->block_map, b);
> >  		b = ext2fs_inode_bitmap_loc(fs, i);
> > -		ext2fs_mark_block_bitmap2(fs->block_map, EXT2FS_B2C(fs, b));
> > +		ext2fs_mark_block_bitmap2(fs->block_map, b);
> >  
> >  		retval = ext2fs_super_and_bgd_loc2(fs, i, &b, &c, &d, NULL);
> >  		if (retval == 0 && b)
> > -			ext2fs_mark_block_bitmap2(fs->block_map,
> > -						  EXT2FS_B2C(fs, b));
> > +			ext2fs_mark_block_bitmap2(fs->block_map, b);
> >  		if (retval == 0 && c)
> > -			ext2fs_mark_block_bitmap2(fs->block_map,
> > -						  EXT2FS_B2C(fs, c));
> > +			ext2fs_mark_block_bitmap2(fs->block_map, c);
> >  		if (retval == 0 && d)
> > -			ext2fs_mark_block_bitmap2(fs->block_map,
> > -						  EXT2FS_B2C(fs, d));
> > +			ext2fs_mark_block_bitmap2(fs->block_map, d);
> >  		if (retval) {
> >  			com_err("disable_uninit_bg", retval,
> >  				"while initializing block bitmaps");
> > 
> > --
> > 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
> > 
--
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
Lukas Czerner - Oct. 7, 2013, 12:49 p.m.
On Thu, 3 Oct 2013, Darrick J. Wong wrote:

> Date: Thu, 3 Oct 2013 12:04:44 -0700
> From: Darrick J. Wong <darrick.wong@oracle.com>
> To: Lukáš Czerner <lczerner@redhat.com>
> Cc: tytso@mit.edu, linux-ext4@vger.kernel.org
> Subject: Re: [PATCH 01/31] tune2fs: Don't convert block # to cluster # when
>     clearing uninit_bg
> 
> On Thu, Oct 03, 2013 at 06:53:38PM +0200, Lukáš Czerner wrote:
> > On Mon, 30 Sep 2013, Darrick J. Wong wrote:
> > 
> > > Date: Mon, 30 Sep 2013 18:26:49 -0700
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > To: tytso@mit.edu, darrick.wong@oracle.com
> > > Cc: linux-ext4@vger.kernel.org
> > > Subject: [PATCH 01/31] tune2fs: Don't convert block # to cluster # when
> > >     clearing uninit_bg
> > > 
> > > When we're constructing the initial block bitmap as part of removing the
> > > gdt_csum (i.e. uninit_bg) feature, we mustn't convert the block numbers to
> > > cluster numbers because ext2fs_mark_block_bitmap2() does this for us.
> > 
> > I wonder if it's possible to use the old-style bitmap interface (the
> > one which does not understand 64-bit bitmaps). If so, then you also
> > need to fix ext2fs_mark_generic_bmap() (and others) so that we
> > convert blocks to clusters if needed.
> 
> It's possible to do this (bad thing) by not specifying EXT2_FLAG_64BITS when
> calling ext2fs_openfs(); see patch 26 for a quick fix.
> 
> (I'm imagining that most bigalloc users probably want 64bit anyway?)
> 
> (No idea, bigalloc isn't (currently) my use case.)

Fair enough. The patch no. 26 seems to do the trick.

Thanks!
-Lukas

> 
> --D
> > 
> > Thanks!
> > -Lukas
> > 
> > > 
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > ---
> > >  misc/tune2fs.c |   13 +++++--------
> > >  1 file changed, 5 insertions(+), 8 deletions(-)
> > > 
> > > 
> > > diff --git a/misc/tune2fs.c b/misc/tune2fs.c
> > > index ddf3259..52247e0 100644
> > > --- a/misc/tune2fs.c
> > > +++ b/misc/tune2fs.c
> > > @@ -820,20 +820,17 @@ static void disable_uninit_bg(ext2_filsys fs, __u32 csum_feature_flag)
> > >  	/* The bbitmap is zeroed; we must mark group metadata blocks in use */
> > >  	for (i = 0; i < fs->group_desc_count; i++) {
> > >  		b = ext2fs_block_bitmap_loc(fs, i);
> > > -		ext2fs_mark_block_bitmap2(fs->block_map, EXT2FS_B2C(fs, b));
> > > +		ext2fs_mark_block_bitmap2(fs->block_map, b);
> > >  		b = ext2fs_inode_bitmap_loc(fs, i);
> > > -		ext2fs_mark_block_bitmap2(fs->block_map, EXT2FS_B2C(fs, b));
> > > +		ext2fs_mark_block_bitmap2(fs->block_map, b);
> > >  
> > >  		retval = ext2fs_super_and_bgd_loc2(fs, i, &b, &c, &d, NULL);
> > >  		if (retval == 0 && b)
> > > -			ext2fs_mark_block_bitmap2(fs->block_map,
> > > -						  EXT2FS_B2C(fs, b));
> > > +			ext2fs_mark_block_bitmap2(fs->block_map, b);
> > >  		if (retval == 0 && c)
> > > -			ext2fs_mark_block_bitmap2(fs->block_map,
> > > -						  EXT2FS_B2C(fs, c));
> > > +			ext2fs_mark_block_bitmap2(fs->block_map, c);
> > >  		if (retval == 0 && d)
> > > -			ext2fs_mark_block_bitmap2(fs->block_map,
> > > -						  EXT2FS_B2C(fs, d));
> > > +			ext2fs_mark_block_bitmap2(fs->block_map, d);
> > >  		if (retval) {
> > >  			com_err("disable_uninit_bg", retval,
> > >  				"while initializing block bitmaps");
> > > 
> > > --
> > > 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
> > > 
> --
> 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 - Oct. 7, 2013, 1:03 p.m.
On Thu, Oct 03, 2013 at 12:04:44PM -0700, Darrick J. Wong wrote:
> On Thu, Oct 03, 2013 at 06:53:38PM +0200, Lukáš Czerner wrote:
> > On Mon, 30 Sep 2013, Darrick J. Wong wrote:
> > 
> > > Date: Mon, 30 Sep 2013 18:26:49 -0700
> > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > To: tytso@mit.edu, darrick.wong@oracle.com
> > > Cc: linux-ext4@vger.kernel.org
> > > Subject: [PATCH 01/31] tune2fs: Don't convert block # to cluster # when
> > >     clearing uninit_bg
> > > 
> > > When we're constructing the initial block bitmap as part of removing the
> > > gdt_csum (i.e. uninit_bg) feature, we mustn't convert the block numbers to
> > > cluster numbers because ext2fs_mark_block_bitmap2() does this for us.
> > 
> > I wonder if it's possible to use the old-style bitmap interface (the
> > one which does not understand 64-bit bitmaps). If so, then you also
> > need to fix ext2fs_mark_generic_bmap() (and others) so that we
> > convert blocks to clusters if needed.
> 
> It's possible to do this (bad thing) by not specifying EXT2_FLAG_64BITS when
> calling ext2fs_openfs(); see patch 26 for a quick fix.

Bigalloc requires the new-style bitmaps, so patch 26 is the right fix
for this.

						- 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
Darrick J. Wong - Oct. 9, 2013, 10:10 p.m.
On Mon, Oct 07, 2013 at 09:03:28AM -0400, Theodore Ts'o wrote:
> On Thu, Oct 03, 2013 at 12:04:44PM -0700, Darrick J. Wong wrote:
> > On Thu, Oct 03, 2013 at 06:53:38PM +0200, Lukáš Czerner wrote:
> > > On Mon, 30 Sep 2013, Darrick J. Wong wrote:
> > > 
> > > > Date: Mon, 30 Sep 2013 18:26:49 -0700
> > > > From: Darrick J. Wong <darrick.wong@oracle.com>
> > > > To: tytso@mit.edu, darrick.wong@oracle.com
> > > > Cc: linux-ext4@vger.kernel.org
> > > > Subject: [PATCH 01/31] tune2fs: Don't convert block # to cluster # when
> > > >     clearing uninit_bg
> > > > 
> > > > When we're constructing the initial block bitmap as part of removing the
> > > > gdt_csum (i.e. uninit_bg) feature, we mustn't convert the block numbers to
> > > > cluster numbers because ext2fs_mark_block_bitmap2() does this for us.
> > > 
> > > I wonder if it's possible to use the old-style bitmap interface (the
> > > one which does not understand 64-bit bitmaps). If so, then you also
> > > need to fix ext2fs_mark_generic_bmap() (and others) so that we
> > > convert blocks to clusters if needed.
> > 
> > It's possible to do this (bad thing) by not specifying EXT2_FLAG_64BITS when
> > calling ext2fs_openfs(); see patch 26 for a quick fix.
> 
> Bigalloc requires the new-style bitmaps, so patch 26 is the right fix
> for this.

Any thoughts on patch #1?  It fixes bugs in the recently-landed patch "tune2fs:
zero inode table when removing checksums".

--D
> 
> 						- 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 - Oct. 10, 2013, 12:26 a.m.
On Wed, Oct 09, 2013 at 03:10:28PM -0700, Darrick J. Wong wrote:
> 
> Any thoughts on patch #1?  It fixes bugs in the recently-landed patch "tune2fs:
> zero inode table when removing checksums".

I haven't gotten to it yet since I've been focusing on patches which
apply to the maint branch (i.e., for a 1.42.x release).

The metadata checksum patches are on the master/next branch, which
will be for an eventual 1.43 release.

What I plan to do is to get all or most of the changes which are
appropriate for the maint branch applied.  I'll then merge the maint
branch into the next branch, and then start applying the patches which
are specific for a future 1.43 release.

Cheers,

					- 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
Darrick J. Wong - Oct. 10, 2013, 10:04 p.m.
On Wed, Oct 09, 2013 at 08:26:02PM -0400, Theodore Ts'o wrote:
> On Wed, Oct 09, 2013 at 03:10:28PM -0700, Darrick J. Wong wrote:
> > 
> > Any thoughts on patch #1?  It fixes bugs in the recently-landed patch "tune2fs:
> > zero inode table when removing checksums".
> 
> I haven't gotten to it yet since I've been focusing on patches which
> apply to the maint branch (i.e., for a 1.42.x release).
> 
> The metadata checksum patches are on the master/next branch, which
> will be for an eventual 1.43 release.
> 
> What I plan to do is to get all or most of the changes which are
> appropriate for the maint branch applied.  I'll then merge the maint
> branch into the next branch, and then start applying the patches which
> are specific for a future 1.43 release.

Ok.  Do you want me to hold off on reissuing changed patches and sending out
new patches until then?  Or should I just send 'em and we can deal with the
blizzard later?

Patches 2, 14-16, and 22 have changed, and there are now 9 more patches, mostly
to fix resize2fs bugs.

Eh, I'll move the ones you took to the start of my quilt, and hope they don't
change too much when they get merged in.  That'll help me keep the in-review
patches in a consecutive pile.

--D
> 
> Cheers,
> 
> 					- 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
--
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/misc/tune2fs.c b/misc/tune2fs.c
index ddf3259..52247e0 100644
--- a/misc/tune2fs.c
+++ b/misc/tune2fs.c
@@ -820,20 +820,17 @@  static void disable_uninit_bg(ext2_filsys fs, __u32 csum_feature_flag)
 	/* The bbitmap is zeroed; we must mark group metadata blocks in use */
 	for (i = 0; i < fs->group_desc_count; i++) {
 		b = ext2fs_block_bitmap_loc(fs, i);
-		ext2fs_mark_block_bitmap2(fs->block_map, EXT2FS_B2C(fs, b));
+		ext2fs_mark_block_bitmap2(fs->block_map, b);
 		b = ext2fs_inode_bitmap_loc(fs, i);
-		ext2fs_mark_block_bitmap2(fs->block_map, EXT2FS_B2C(fs, b));
+		ext2fs_mark_block_bitmap2(fs->block_map, b);
 
 		retval = ext2fs_super_and_bgd_loc2(fs, i, &b, &c, &d, NULL);
 		if (retval == 0 && b)
-			ext2fs_mark_block_bitmap2(fs->block_map,
-						  EXT2FS_B2C(fs, b));
+			ext2fs_mark_block_bitmap2(fs->block_map, b);
 		if (retval == 0 && c)
-			ext2fs_mark_block_bitmap2(fs->block_map,
-						  EXT2FS_B2C(fs, c));
+			ext2fs_mark_block_bitmap2(fs->block_map, c);
 		if (retval == 0 && d)
-			ext2fs_mark_block_bitmap2(fs->block_map,
-						  EXT2FS_B2C(fs, d));
+			ext2fs_mark_block_bitmap2(fs->block_map, d);
 		if (retval) {
 			com_err("disable_uninit_bg", retval,
 				"while initializing block bitmaps");