[prev in list] [next in list] [prev in thread] [next in thread] 

List:       linux-ext4
Subject:    Re: [Ext2-devel] [RFC/RESEND][PATCH] Online resizing using meta block groups
From:       glommer () br ! ibm ! com (Glauber de Oliveira Costa)
Date:       2006-01-19 18:45:17
Message-ID: 20060119184517.GB21235 () br ! ibm ! com
[Download RAW message or body]

On Thu, Jan 19, 2006 at 04:46:44AM -0700, Andreas Dilger wrote:
> On Dec 19, 2005  14:01 -0200, Glauber de Oliveira Costa wrote:
> > This patch implements the last phase of online resizing using meta block
> > groups.
> > 
> > The old ext3_group_add is still put aside as __ext3_group_add. It's
> > done this way in order to not mess up with the resize inode
> > implementation for a while. Comments are always welcome.
> 
> The way it should be implemented is that resizing should consume all
> reserved GDT blocks from the resize inode (if any), and only then
> set the META_BG flag and continue to resize without it.  Since having
> an s_first_meta_bg value of zero is valid, it is not even possible to
> set the META_BG flag on an existing filesystem just to tell the resize
> that it is OK to use that feature.

Ok, I'll include it in next version of the patch.
 
> Instead I think the online resize code just needs to go ahead and set
> META_BG if it is being resized.  The user tools should check this and
> prompt the user for input (and allow an "-m" flag to override) if the
> META_BG flag would be enabled by the kernel.  This is because META_BG
> is incompatible and will prevent the mounting of this filesystem on
> any older kernels, so we don't want to quietly spring it on users.

Agreed. Maybe the best way should show the user which is the limit that
can be reached without it, and provide the option among canceling the
action, going till the limit, or setting the flag.

> > --- linux-2.6.14.2-orig/fs/ext3/resize.c	2005-12-12 12:25:32.000000000 +0000
> > +++ linux-2.6.14.2-metabg/fs/ext3/resize.c	2005-12-19 15:29:24.000000000 +0000
> > @@ -194,8 +194,21 @@ static int setup_new_group_blocks(struct
> > +	/* In case of the meta_bg flag is on and we're adding a group,
> > +	 * we're surelly doing it at the last meta group. Exception to this 
> 
> s/surelly/surely/
> 
> > +	 * rule happens when we're adding a new metagroup, in case the 
> 
> ... in which case...
>

Thanks for the above. As I'm not an english speaker, it may happen
sometimes (Not that they don't happen for you, it may just happen more
oftenly with us =] )
 
> > +	 * following code copies the gdt of the last one into it. This is
> > +	 * handled later by update_metagroup */
> > +	if (EXT3_HAS_INCOMPAT_FEATURE(sb,EXT3_FEATURE_INCOMPAT_META_BG) &&
> > +			gdblocks){
> 
> CodingStyle has a space before the brace "gdblocks) {".
> 
> > +		i = sbi->s_gdb_count - 1;
> > +		gdblocks = i + 1;
> > +	}
> > +	else 
> 
> CodingStyle has "} else {".
> 
> > +		i = 0;
> > +	
> >  	/* Copy all of the GDT blocks into the backup in this group */
> > -	for (i = 0, bit = 1, block = start + 1;
> > +	for (bit = 1, block = start + ext3_bg_has_super(sb,input->group);
> >  	     i < gdblocks; i++, block++, bit++) {
> >  		struct buffer_head *gdb;
Ok, I'll fix those.
 
> This seems to be dependent upon the ext3_bg_num_gdb() function working
> properly, which is actually in your second patch, so I was confused.
> Please remember to order the patch dependencies properly. :-)

Sorry for that, and thanks for the tip. I'll do it the right way when
resending.
 
> > @@ -633,7 +673,20 @@ static void update_backups(struct super_
> >  		goto exit_err;
> >  	}
> >  
> > -	while ((group = ext3_list_backups(sb, &three, &five, &seven)) < last) {
> > +	if (!EXT3_HAS_INCOMPAT_FEATURE(sb,EXT3_FEATURE_INCOMPAT_META_BG) ||
> > +		(blk_off == le32_to_cpu(sbi->s_es->s_first_data_block)))
> > +	{
> > +		func = ext3_list_backups;
> > +		arg1 = three;
> > +		arg2 = five;
> > +		arg3 = seven;
> > +	}
> > +	else{
> > +		func = ext3_list_metagroup;
> > +		arg1 = arg2 = arg3 = 0;
> > +	}
> > +	
> > +	while ((group = func(sb, &arg1, &arg2, &arg3)) < last) {
> 
> I'm not sure I understand this bit of code.  The update_backups() function
> is currently used for updating both the group backups and the superblock
> backups.  Because these two entities no longer live in the same group with
> META_BG we can't use the same function anymore.
> 
> It would seem that this distinction is being detected by the the blk_off
> parameter being s_first_data_block (implying a superblock).  This bit of
> code works, though I don't understand why you keep "three, five, seven"
> around instead of just setting arg1 = 1, arg2 = 5, arg3 = 7.

I kept because I forgot removing =] They won't be there in next one. 

> However, using ext3_list_metagroup() doesn't work here (AFAICS) because it
> assumes the whole filesystem is using metagroups, instead of only switching
> after passing s_first_meta_bg * EXT3_DESC_PER_GROUP(sb).  Instead, this
> function needs to decide if it is updating backups in either the "old" GDT
> backups, or the (up to 2 + superblock) META_BG backups.

You're right. My idea was to deal only with the last metagroup, since
it's the only one who cares about changes. It'll be a matter of
adjusting the arguments properly.
 
> I think it would just be clearer to have separate functions here - one for
> superblocks and one for group descriptors, or at least explicitly pass the
> iterator as a parameter instead of getting it implicitly from the block
> number.

I did this way because my main idea was to keep the original code as
much as possible. Two functions would be my choice beetween them. I'll
think about it a little bit better anyway.

> > +/* Adds a new entry pointed by gbh in the group descriptor array in the 
> > + * in-memory representation of the super block, and update its s_gdb_count.
> > + * 
> > + * Called with super block lock held. */
> > +static int extend_last_gdb(struct super_block *sb,
> > +		    struct buffer_head *gbh){
> > +
> > +	struct ext3_sb_info *sbi = EXT3_SB(sb);
> > +	struct buffer_head **n_group_desc;
> > +	
> > +	n_group_desc = (struct buffer_head **)kmalloc((sbi->s_gdb_count + 1)*
> > +			sizeof(struct buffer_head *),GFP_KERNEL);
> 
> Do we really need casting of kmalloc return here?  That would also allow
> proper alignment of this code:
> 
> 	n_group_desc = kmalloc((sbi->s_gdb_count + 1) *
> 			       sizeof(struct buffer_head *), GFP_KERNEL);

Ok, Will change that.


> Cheers, Andreas
> --
> Andreas Dilger
> Principal Software Engineer
> Cluster File Systems, Inc.
> 
> 

-- 
glommer
Chuck Norris does not hunt because the word hunting implies
the probability of failure. Chuck Norris goes killing.
www.chucknorrisfacts.com


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=103432&bid=230486&dat=121642
_______________________________________________
Ext2-devel mailing list
Ext2-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/ext2-devel
[prev in list] [next in list] [prev in thread] [next in thread]