| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <alpine.LFD.2.00.1008061513160.3645@localhost>
Date: Fri, 6 Aug 2010 15:23:59 +0200 (CEST)
From: Lukas Czerner <lczerner@...hat.com>
To: Dmitry Monakhov <dmonakhov@...nvz.org>
cc: Lukas Czerner <lczerner@...hat.com>, linux-ext4@...r.kernel.org,
jmoyer@...hat.com, rwheeler@...hat.com, eshishki@...hat.com,
sandeen@...hat.com, jack@...e.cz, tytso@....edu
Subject: Re: [PATCH 3/3] Add batched discard support for ext4
On Fri, 6 Aug 2010, Dmitry Monakhov wrote:
> Lukas Czerner <lczerner@...hat.com> writes:
>
> > Walk through allocation groups and trim all free extents. It can be
> > invoked through FITRIM ioctl on the file system. The main idea is to
> > provide a way to trim the whole file system if needed, since some SSD's
> > may suffer from performance loss after the whole device was filled (it
> > does not mean that fs is full!).
> >
> > It search for free extents in allocation groups specified by Byte range
> > start -> start+len. When the free extent is within this range, blocks
> > are marked as used and then trimmed. Afterwards these blocks are marked
> > as free in per-group bitmap.
> >
> > Since fstrim is a long operation it is good to have an ability to
> > interrupt it by a signal. This was added by Dmitry Monakhov.
> > Thanks Dimitry.
> >
> > Signed-off-by: Lukas Czerner <lczerner@...hat.com>
> > Signed-off-by: Dmitry Monakhov <dmonakhov@...nvz.org>
> > Reviewed-by: Jan Kara <jack@...e.cz>
> > ---
> > fs/ext4/ext4.h | 2 +
> > fs/ext4/mballoc.c | 194 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> > fs/ext4/super.c | 1 +
> > 3 files changed, 197 insertions(+), 0 deletions(-)
> >
> > diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> > index 19a4de5..2f96dc5 100644
> > --- a/fs/ext4/ext4.h
> > +++ b/fs/ext4/ext4.h
> > @@ -1558,6 +1558,8 @@ extern int ext4_mb_add_groupinfo(struct super_block *sb,
> > extern int ext4_mb_get_buddy_cache_lock(struct super_block *, ext4_group_t);
> > extern void ext4_mb_put_buddy_cache_lock(struct super_block *,
> > ext4_group_t, int);
> > +extern int ext4_trim_fs(struct super_block *, uint64_t, uint64_t, uint64_t);
> > +
> > /* inode.c */
> > struct buffer_head *ext4_getblk(handle_t *, struct inode *,
> > ext4_lblk_t, int, int *);
> > diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> > index 0e83dfd..5210676 100644
> > --- a/fs/ext4/mballoc.c
> > +++ b/fs/ext4/mballoc.c
> > @@ -4690,3 +4690,197 @@ error_return:
> > kmem_cache_free(ext4_ac_cachep, ac);
> > return;
> > }
> > +
> > +/**
> > + * ext4_trim_extent -- function to TRIM one single free extent in the group
> > + * @sb: super block for the file system
> > + * @start: starting block of the free extent in the alloc. group
> > + * @count: number of blocks to TRIM
> > + * @group: alloc. group we are working with
> > + * @e4b: ext4 buddy for the group
> > + *
> > + * Trim "count" blocks starting at "start" in the "group". To assure that no
> > + * one will allocate those blocks, mark it as used in buddy bitmap. This must
> > + * be called with under the group lock.
> > + */
> > +static int ext4_trim_extent(struct super_block *sb, int start, int count,
> > + ext4_group_t group, struct ext4_buddy *e4b)
> > +{
> > + ext4_fsblk_t discard_block;
> > + struct ext4_super_block *es = EXT4_SB(sb)->s_es;
> > + struct ext4_free_extent ex;
> > + int ret = 0;
> > +
> > + assert_spin_locked(ext4_group_lock_ptr(sb, group));
> > +
> > + ex.fe_start = start;
> > + ex.fe_group = group;
> > + ex.fe_len = count;
> > +
> > + /*
> > + * Mark blocks used, so no one can reuse them while
> > + * being trimmed.
> > + */
> > + mb_mark_used(e4b, &ex);
> > + ext4_unlock_group(sb, group);
> > +
> > + discard_block = (ext4_fsblk_t)group *
> > + EXT4_BLOCKS_PER_GROUP(sb)
> > + + start
> > + + le32_to_cpu(es->s_first_data_block);
> > + trace_ext4_discard_blocks(sb,
> > + (unsigned long long)discard_block,
> > + count);
> > + ret = sb_issue_discard(sb, discard_block, count);
> > + if (ret == -EOPNOTSUPP) {
> > + ext4_warning(sb,
> > + "discard not supported!");
> > + } else if (ret < 0) {
> > + ext4_std_error(sb, ret);
> > + }
> > + cond_resched();
> OOps seems i'm was wrong when suggested to put cond_resched() here.
> It is useless because we probably already slept inside sb_issue_discard()
> But it is reasonable to move it to ext4_trim_all_free()
> because fs size may be huge so simple bitmap traverse will take long
> enough, but almost fully populated so there are no extents longer than
> minlen so discard would not being called.
> BTW: Since Jan already noted about that in ext3's patch so ext3 version
> is good.
>
> diff --git a/fs/ext4/mballoc.c b/fs/ext4/mballoc.c
> index d3a0763..00e6147 100644
> --- a/fs/ext4/mballoc.c
> +++ b/fs/ext4/mballoc.c
> @@ -4688,7 +4688,6 @@ static int ext4_trim_extent(struct super_block *sb, int start, int count,
> } else if (ret < 0) {
> ext4_std_error(sb, ret);
> }
> - cond_resched();
>
> ext4_lock_group(sb, group);
> mb_free_blocks(NULL, e4b, start, ex.fe_len);
> @@ -4745,6 +4744,11 @@ ext4_grpblk_t ext4_trim_all_free(struct super_block *sb, struct ext4_buddy *e4b,
> }
> start = next + 1;
>
> + if (need_resched()) {
> + ext4_unlock_group(sb, group);
> + cond_resched();
> + ext4_lock_group(sb, group);
> + }
> if (signal_pending(current)) {
> count = -ERESTARTSYS;
> break;
>
>
Hmm, are you sure about this ? Wouldn't make more sense to put just
cond_resched() into the ext4_trim_fs loop, since there is no need to
check for need_resched() and lock/unlock the group. Do you expect the
group traversal to take that long ?
There are other examples of traversing whole group, but none of it is
trying to reschedule in the middle of the group.
Regards
-Lukas
--
To unsubscribe from this list: send the line "unsubscribe linux-ext4" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists