| Message ID | 1313033308-882-2-git-send-email-xiaoqiangnk@gmail.com |
|---|---|
| State | Superseded, archived |
| Headers | show |
On 2011-08-10, at 9:28 PM, Yongqiang Yang wrote: > This patch added a function named __ext4_group_extend() whose code > is copied from ext4_group_extend(). __ext4_group_extend() assumes > the parameter is valid and has been checked by caller. > > __ext4_group_extend() will be used by new resize implementation. It > can also be used by ext4_group_extend(), but this patch series does > not do this. Since this is duplicating a lot of code from ext4_group_extend(), this patch should be written in such a way that this new function is added, and the duplicate code is removed from ext4_group_extend() and calls the new function instead. It looks like all of these patches are adding a completely duplicate set of functions for doing the resizing, even though they are largely the same as the existing code, and it will mean duplicate efforts to maintain both copies of the code. > Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com> > --- > fs/ext4/resize.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 files changed, 53 insertions(+), 0 deletions(-) > > diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c > index 707d3f1..6ffbdb6 100644 > --- a/fs/ext4/resize.c > +++ b/fs/ext4/resize.c > @@ -969,6 +969,59 @@ exit_put: > } /* ext4_group_add */ > > /* > + * extend a group without checking assuming that checking has been done. > + */ > +static int __ext4_group_extend(struct super_block *sb, > + ext4_fsblk_t o_blocks_count, ext4_grpblk_t add) > +{ > + struct ext4_super_block *es = EXT4_SB(sb)->s_es; > + handle_t *handle; > + int err = 0, err2; > + > + /* We will update the superblock, one block bitmap, and > + * one group descriptor via ext4_ext4_group_add_blocks(). Typo here: "ext4_ext4" > + */ > + handle = ext4_journal_start_sb(sb, 3); > + if (IS_ERR(handle)) { > + err = PTR_ERR(handle); > + ext4_warning(sb, "error %d on journal start", err); > + goto out; > + } > + > + err = ext4_journal_get_write_access(handle, EXT4_SB(sb)->s_sbh); > + if (err) { > + ext4_warning(sb, "error %d on journal write access", err); > + ext4_journal_stop(handle); > + goto out; > + } > + > + ext4_blocks_count_set(es, o_blocks_count + add); > + ext4_debug("freeing blocks %llu through %llu\n", o_blocks_count, > + o_blocks_count + add); > + /* We add the blocks to the bitmap and set the group need init bit */ > + err = ext4_group_add_blocks(handle, sb, o_blocks_count, add); > + if (err) > + goto exit_journal; > + ext4_handle_dirty_super(handle, sb); > + ext4_debug("freed blocks %llu through %llu\n", o_blocks_count, > + o_blocks_count + add); > +exit_journal: > + err2 = ext4_journal_stop(handle); > + if (err2 && !err) > + err = err2; > + > + if (!err) { > + if (test_opt(sb, DEBUG)) > + printk(KERN_DEBUG "EXT4-fs: extended group to %llu " > + "blocks\n", ext4_blocks_count(es)); > + update_backups(sb, EXT4_SB(sb)->s_sbh->b_blocknr, (char *)es, > + sizeof(struct ext4_super_block)); > + } > +out: > + return err; > +} > + > +/* > * Extend the filesystem to the new number of blocks specified. This entry > * point is only used to extend the current filesystem to the end of the last > * existing group. It can be accessed via ioctl, or by "remount,resize=<size>" > -- > 1.7.5.1 > Cheers, Andreas -- 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
On Thu, Aug 11, 2011 at 1:47 PM, Andreas Dilger <adilger@dilger.ca> wrote: > On 2011-08-10, at 9:28 PM, Yongqiang Yang wrote: >> This patch added a function named __ext4_group_extend() whose code >> is copied from ext4_group_extend(). __ext4_group_extend() assumes >> the parameter is valid and has been checked by caller. >> >> __ext4_group_extend() will be used by new resize implementation. It >> can also be used by ext4_group_extend(), but this patch series does >> not do this. > > Since this is duplicating a lot of code from ext4_group_extend(), this > patch should be written in such a way that this new function is added, > and the duplicate code is removed from ext4_group_extend() and calls > the new function instead. > > It looks like all of these patches are adding a completely duplicate set > of functions for doing the resizing, even though they are largely the > same as the existing code, and it will mean duplicate efforts to maintain > both copies of the code. YES! This needs some feedbacks, I thought the old resize interface will be removed after the new resize interface goes into upstream. So I did not touch old interface. If we will remain the old interface, I can make some patches which let old resize use common code instead. Thank you for your review. Yongqiang. > >> Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com> >> --- >> fs/ext4/resize.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++ >> 1 files changed, 53 insertions(+), 0 deletions(-) >> >> diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c >> index 707d3f1..6ffbdb6 100644 >> --- a/fs/ext4/resize.c >> +++ b/fs/ext4/resize.c >> @@ -969,6 +969,59 @@ exit_put: >> } /* ext4_group_add */ >> >> /* >> + * extend a group without checking assuming that checking has been done. >> + */ >> +static int __ext4_group_extend(struct super_block *sb, >> + ext4_fsblk_t o_blocks_count, ext4_grpblk_t add) >> +{ >> + struct ext4_super_block *es = EXT4_SB(sb)->s_es; >> + handle_t *handle; >> + int err = 0, err2; >> + >> + /* We will update the superblock, one block bitmap, and >> + * one group descriptor via ext4_ext4_group_add_blocks(). > > Typo here: "ext4_ext4" > >> + */ >> + handle = ext4_journal_start_sb(sb, 3); >> + if (IS_ERR(handle)) { >> + err = PTR_ERR(handle); >> + ext4_warning(sb, "error %d on journal start", err); >> + goto out; >> + } >> + >> + err = ext4_journal_get_write_access(handle, EXT4_SB(sb)->s_sbh); >> + if (err) { >> + ext4_warning(sb, "error %d on journal write access", err); >> + ext4_journal_stop(handle); >> + goto out; >> + } >> + >> + ext4_blocks_count_set(es, o_blocks_count + add); >> + ext4_debug("freeing blocks %llu through %llu\n", o_blocks_count, >> + o_blocks_count + add); >> + /* We add the blocks to the bitmap and set the group need init bit */ >> + err = ext4_group_add_blocks(handle, sb, o_blocks_count, add); >> + if (err) >> + goto exit_journal; >> + ext4_handle_dirty_super(handle, sb); >> + ext4_debug("freed blocks %llu through %llu\n", o_blocks_count, >> + o_blocks_count + add); >> +exit_journal: >> + err2 = ext4_journal_stop(handle); >> + if (err2 && !err) >> + err = err2; >> + >> + if (!err) { >> + if (test_opt(sb, DEBUG)) >> + printk(KERN_DEBUG "EXT4-fs: extended group to %llu " >> + "blocks\n", ext4_blocks_count(es)); >> + update_backups(sb, EXT4_SB(sb)->s_sbh->b_blocknr, (char *)es, >> + sizeof(struct ext4_super_block)); >> + } >> +out: >> + return err; >> +} >> + >> +/* >> * Extend the filesystem to the new number of blocks specified. This entry >> * point is only used to extend the current filesystem to the end of the last >> * existing group. It can be accessed via ioctl, or by "remount,resize=<size>" >> -- >> 1.7.5.1 >> > > > Cheers, Andreas > > > > > >
On Thu, Aug 11, 2011 at 02:27:41PM +0800, Yongqiang Yang wrote: > YES! This needs some feedbacks, I thought the old resize interface > will be removed after the new resize interface goes into upstream. So > I did not touch > old interface. If we will remain the old interface, I can make some > patches which let old resize use common code instead. Yes, the backwards compatibility requirements for the kernel means that we have to keep the old ioctl's working for at least 2-3 years... - 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
On 2011-08-21, at 11:07 AM, Ted Ts'o <tytso@mit.edu> wrote: > On Thu, Aug 11, 2011 at 02:27:41PM +0800, Yongqiang Yang wrote: >> YES! This needs some feedbacks, I thought the old resize interface >> will be removed after the new resize interface goes into upstream. So >> I did not touch >> old interface. If we will remain the old interface, I can make some >> patches which let old resize use common code instead. > > Yes, the backwards compatibility requirements for the kernel means > that we have to keep the old ioctl's working for at least 2-3 years... I'd be happy with just wiring up the old ioctls to the new code, and having the kernel ignore the passed parameters and just do it's own thing to resize to the end of the group. Cheers, Andreas-- 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
diff --git a/fs/ext4/resize.c b/fs/ext4/resize.c index 707d3f1..6ffbdb6 100644 --- a/fs/ext4/resize.c +++ b/fs/ext4/resize.c @@ -969,6 +969,59 @@ exit_put: } /* ext4_group_add */ /* + * extend a group without checking assuming that checking has been done. + */ +static int __ext4_group_extend(struct super_block *sb, + ext4_fsblk_t o_blocks_count, ext4_grpblk_t add) +{ + struct ext4_super_block *es = EXT4_SB(sb)->s_es; + handle_t *handle; + int err = 0, err2; + + /* We will update the superblock, one block bitmap, and + * one group descriptor via ext4_ext4_group_add_blocks(). + */ + handle = ext4_journal_start_sb(sb, 3); + if (IS_ERR(handle)) { + err = PTR_ERR(handle); + ext4_warning(sb, "error %d on journal start", err); + goto out; + } + + err = ext4_journal_get_write_access(handle, EXT4_SB(sb)->s_sbh); + if (err) { + ext4_warning(sb, "error %d on journal write access", err); + ext4_journal_stop(handle); + goto out; + } + + ext4_blocks_count_set(es, o_blocks_count + add); + ext4_debug("freeing blocks %llu through %llu\n", o_blocks_count, + o_blocks_count + add); + /* We add the blocks to the bitmap and set the group need init bit */ + err = ext4_group_add_blocks(handle, sb, o_blocks_count, add); + if (err) + goto exit_journal; + ext4_handle_dirty_super(handle, sb); + ext4_debug("freed blocks %llu through %llu\n", o_blocks_count, + o_blocks_count + add); +exit_journal: + err2 = ext4_journal_stop(handle); + if (err2 && !err) + err = err2; + + if (!err) { + if (test_opt(sb, DEBUG)) + printk(KERN_DEBUG "EXT4-fs: extended group to %llu " + "blocks\n", ext4_blocks_count(es)); + update_backups(sb, EXT4_SB(sb)->s_sbh->b_blocknr, (char *)es, + sizeof(struct ext4_super_block)); + } +out: + return err; +} + +/* * Extend the filesystem to the new number of blocks specified. This entry * point is only used to extend the current filesystem to the end of the last * existing group. It can be accessed via ioctl, or by "remount,resize=<size>"
This patch added a function named __ext4_group_extend() whose code is copied from ext4_group_extend(). __ext4_group_extend() assumes the parameter is valid and has been checked by caller. __ext4_group_extend() will be used by new resize implementation. It can also be used by ext4_group_extend(), but this patch series does not do this. Signed-off-by: Yongqiang Yang <xiaoqiangnk@gmail.com> --- fs/ext4/resize.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++ 1 files changed, 53 insertions(+), 0 deletions(-)