| 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
| ||
|
Date: Thu, 18 Nov 2010 13:12:44 +0100 (CET)
From: Lukas Czerner <lczerner@...hat.com>
To: Eric Sandeen <sandeen@...hat.com>
cc: Lukas Czerner <lczerner@...hat.com>, linux-ext4@...r.kernel.org,
tytso@....edu, adilger@...ger.ca
Subject: Re: [PATCH 4/7] e2fsck: Discard free data and inode blocks.
On Tue, 16 Nov 2010, Eric Sandeen wrote:
-snip-
> >
> > +static void e2fsck_should_discard(e2fsck_t ctx, io_manager manager,
> > + blk64_t start, blk64_t count)
> > +{
> > + ext2_filsys fs = ctx->fs;
> > + int ret = 0;
> > +
> > + if (ctx->options & E2F_OPT_DISCARD)
> > + ret = manager->discard(fs->io, start, count, fs);
>
> as suggested earlier maybe you can just pass in blocksize unless there's
> a real expectation that other OSes might need other fs-> info?
>
> "e2fsck_should_discard" sounds lke it is a test that would return
> true/false, but this actually does discard and is a void.
>
> I'd probably rename it to better imply what itdoes, maybe
> just e2fsck_blocks_discard or e2fsck_discard ?
>
> > +
> > + if (ret)
>
> any point to issuing a warning here if we encounter an error and stop?
Right.
>
> > + ctx->options &= ~E2F_OPT_DISCARD;
> > +}
> > +
> > #define NO_BLK ((blk64_t) -1)
> >
> > static void print_bitmap_problem(e2fsck_t ctx, int problem,
> > @@ -108,6 +136,7 @@ static void check_block_bitmaps(e2fsck_t ctx)
> > int group = 0;
> > int blocks = 0;
> > blk64_t free_blocks = 0;
> > + blk64_t first_free = ext2fs_blocks_count(fs->super);
> > int group_free = 0;
> > int actual, bitmap;
> > struct problem_context pctx;
> > @@ -120,6 +149,7 @@ static void check_block_bitmaps(e2fsck_t ctx)
> > int cmp_block = 0;
> > int redo_flag = 0;
> > blk64_t super_blk, old_desc_blk, new_desc_blk;
> > + io_manager manager = ctx->fs->io->manager;
> >
> > clear_problem_context(&pctx);
> > free_array = (int *) e2fsck_allocate_memory(ctx,
> > @@ -280,11 +310,24 @@ redo_counts:
> > }
> > ctx->flags |= E2F_FLAG_PROG_SUPPRESS;
> > had_problem++;
> > + /*
> > + * If there a problem we should turn off the discard so we
> > + * do not compromise the filesystem.
> > + */
> > + ctx->options &= ~E2F_OPT_DISCARD;
>
> Would a warning be reasonable here?
I am not really sure this is necessary.
>
> Or, maybe rather than trying to catch all the locations where
> you want to turn off the flag, can we just test for a changed
> fs prior to any discard in e2fsck_should_discard()?
> Not sure, just thinking out loud.
Adding test for changed filesystem into e2fsck_should_discard() does
make sence, however it is not sufficient because even in case of fs
error it may hit discard before any changes to filesystem has been made.
>
> > do_counts:
> > if (!bitmap && (!skip_group || csum_flag)) {
> > group_free++;
> > free_blocks++;
> > + if (first_free > i)
> > + first_free = i;
> > + } else {
> > + if ((i > first_free) &&
> > + (ctx->options & E2F_OPT_DISCARD))
> > + e2fsck_should_discard(ctx, manager, first_free,
> > + (i - first_free));
> > + first_free = ext2fs_blocks_count(fs->super);
> > }
> > blocks ++;
> > if ((blocks == fs->super->s_blocks_per_group) ||
> > @@ -381,6 +424,7 @@ static void check_inode_bitmaps(e2fsck_t ctx)
> > int csum_flag;
> > int skip_group = 0;
> > int redo_flag = 0;
> > + io_manager manager = ctx->fs->io->manager;
> >
> > clear_problem_context(&pctx);
> > free_array = (int *) e2fsck_allocate_memory(ctx,
> > @@ -500,6 +544,11 @@ redo_counts:
> > }
> > ctx->flags |= E2F_FLAG_PROG_SUPPRESS;
> > had_problem++;
> > + /*
> > + * If there is a problem we should turn off the discard so we
> > + * do not compromise the filesystem.
> > + */
> > + ctx->options &= ~E2F_OPT_DISCARD;
> >
> > do_counts:
> > if (bitmap) {
> > @@ -509,11 +558,43 @@ do_counts:
> > group_free++;
> > free_inodes++;
> > }
> > +
> > inodes++;
> > if ((inodes == fs->super->s_inodes_per_group) ||
> > (i == fs->super->s_inodes_count)) {
> > +
> > free_array[group] = group_free;
> > dir_array[group] = dirs_count;
> > +
> > + /* Discard inode table */
> > + if (ctx->options & E2F_OPT_DISCARD) {
> > + blk64_t used_blks, blk, num;
> > +
> > + used_blks = DIV_ROUND_UP(
> > + (EXT2_INODES_PER_GROUP(fs->super) -
> > + group_free),
> > + EXT2_INODES_PER_BLOCK(fs->super));
> > +
> > + blk = ext2fs_inode_table_loc(fs, group) +
> > + used_blks;
> > + num = fs->inode_blocks_per_group -
> > + used_blks;
> > + e2fsck_should_discard(ctx, manager, blk, num);
> > + }
> > +
> > + /*
> > + * If discard zeroes data and the group inode table
> > + * was not zeroed yet, set itable as zeroed
> > + */
> > + if ((ctx->options & E2F_OPT_DISCARD) &&
> > + (manager->discard_zeroes_data) &&
> > + !(ext2fs_bg_flags_test(fs, group,
> > + EXT2_BG_INODE_ZEROED))) {
> > + ext2fs_bg_flags_set(fs, group,
> > + EXT2_BG_INODE_ZEROED);
> > + ext2fs_group_desc_csum_set(fs, group);
> > + }
>
> Maybe for another patch, but even if we're not discarding, should we
> be manually zeroing out here & setting the flag? Hm I guess not,
> that'd be slow and defeat the purpose wouldn't it...
>
> > +
> > group ++;
> > inodes = 0;
> > skip_group = 0;
> > diff --git a/e2fsck/unix.c b/e2fsck/unix.c
> > index 7eb269c..4cf55a9 100644
> > --- a/e2fsck/unix.c
> > +++ b/e2fsck/unix.c
> > @@ -594,6 +594,12 @@ static void parse_extended_opts(e2fsck_t ctx, const char *opts)
> > } else if (strcmp(token, "fragcheck") == 0) {
> > ctx->options |= E2F_OPT_FRAGCHECK;
> > continue;
> > + } else if (strcmp(token, "discard") == 0) {
> > + ctx->options |= E2F_OPT_DISCARD;
> > + continue;
> > + } else if (strcmp(token, "nodiscard") == 0) {
> > + ctx->options &= ~E2F_OPT_DISCARD;
> > + continue;
> > } else {
> > fprintf(stderr, _("Unknown extended option: %s\n"),
> > token);
> > @@ -609,6 +615,8 @@ static void parse_extended_opts(e2fsck_t ctx, const char *opts)
> > "Valid extended options are:\n"), stderr);
> > fputs(("\tea_ver=<ea_version (1 or 2)>\n"), stderr);
> > fputs(("\tfragcheck\n"), stderr);
> > + fputs(("\tdiscard\n"), stderr);
> > + fputs(("\tnodiscard\n"), stderr);
> > fputc('\n', stderr);
> > exit(1);
> > }
> > @@ -667,6 +675,7 @@ static errcode_t PRS(int argc, char *argv[], e2fsck_t *ret_ctx)
> > ctx->program_name = *argv;
> > else
> > ctx->program_name = "e2fsck";
> > +
> > while ((c = getopt (argc, argv, "panyrcC:B:dE:fvtFVM:b:I:j:P:l:L:N:SsDk")) != EOF)
> > switch (c) {
> > case 'C':
> > @@ -943,7 +952,6 @@ static errcode_t try_open_fs(e2fsck_t ctx, int flags, io_manager io_ptr,
> > return retval;
> > }
> >
> > -
> > static const char *my_ver_string = E2FSPROGS_VERSION;
> > static const char *my_ver_date = E2FSPROGS_DATE;
> >
>
>
--
--
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