linux-ext4 - Re: [PATCH RFC 01/30] ext4: EXT4 snapshots (Experimental)

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
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Tue, 7 Jun 2011 16:20:53 +0300
From:	"Amir G." <amir73il@...rs.sourceforge.net>
To:	Lukas Czerner <lczerner@...hat.com>
Cc:	linux-ext4@...r.kernel.org, tytso@....edu,
	Amir Goldstein <amir73il@...rs.sf.net>,
	Yongqiang Yang <xiaoqiangnk@...il.com>
Subject: Re: [PATCH RFC 01/30] ext4: EXT4 snapshots (Experimental)

On Tue, Jun 7, 2011 at 1:42 PM, Lukas Czerner <lczerner@...hat.com> wrote:
> On Tue, 7 Jun 2011, Amir G. wrote:
>
>> >> +config EXT4_FS_SNAPSHOT
>> >> +     bool "EXT4 snapshots (Experimental)"
>> >> +     depends on EXT4_FS && EXPERIMENTAL
>> >> +     default n
>> >> +     help
>> >> +       Built-in snapshots support for ext4.
>> >> +       Requires that the filesystem has the has_snapshot and exclude_bitmap
>> >> +       features and that block size is equal to system page size.
>> >> +       Snapshots are not supported with 64bit and meta_bg features and the
>> >> +       filesystem must be mounted with ordered data mode.
>> >
>> > What exactly do you mean by not supported with 64bit feature ? Maybe I
>> > am being dense, but I do not get it.
>>
>> I mean snapshots and 64bit features are mutually exclusive.
>> Is that what you got or do I need to make it more clear?
>
> Oh, I did not notice that it belongs to the "feature" word. Thats
> probably just my English:)

Or a combination of our 'Englishes' ;-)

>
>>
>> >>
>> >>  #define outside(b, first, last)      ((b) < (first) || (b) >= (last))
>> >>  #define inside(b, first, last)       ((b) >= (first) && (b) < (last))
>> >> diff --git a/fs/ext4/snapshot.h b/fs/ext4/snapshot.h
>> >> new file mode 100644
>> >> index 0000000..a927090
>> >> --- /dev/null
>> >> +++ b/fs/ext4/snapshot.h
>> >> @@ -0,0 +1,193 @@
>> >> +/*
>> >> + * linux/fs/ext4/snapshot.h
>> >> + *
>> >> + * Written by Amir Goldstein <amir73il@...rs.sf.net>, 2008
>> >> + *
>> >> + * Copyright (C) 2008-2011 CTERA Networks
>> >> + *
>> >> + * This file is part of the Linux kernel and is made available under
>> >> + * the terms of the GNU General Public License, version 2, or at your
>> >> + * option, any later version, incorporated herein by reference.
>> >> + *
>> >> + * Ext4 snapshot extensions.
>> >
>> > This is great place to write more about snapshot design and
>> > implementation. If it is added later in a different file, then ignore it
>> > :).
>>
>> the inline documentation is scattered among several patches.
>> I should probably also add Documentation/ext4_snapshots.txt
>> with some design and overview information.
>> And perhaps insert a short short version of it here ;-)
>
> Documentation/filesystems/ext4_snapshots.txt would be the most welcome,
> thanks.

I though I'd just drop the Technical_Overview wiki as ext4_snapshots.txt:
http://sourceforge.net/apps/mediawiki/next3/index.php?title=Technical_overview
it seems like a good start, which can be completed by diving into the code.
would you agree with that statement?

>
>>
>> >
>> >> + */
>> >> +
>> >> +#ifndef _LINUX_EXT4_SNAPSHOT_H
>> >> +#define _LINUX_EXT4_SNAPSHOT_H
>> >> +
>> >> +#include <linux/version.h>
>> >> +#include <linux/delay.h>
>> >> +#include "ext4.h"
>> >> +
>> >> +
>> >> +/*
>> >> + * use signed 64bit for snapshot image addresses
>> >> + * negative addresses are used to reference snapshot meta blocks
>> >> + */
>> >> +#define ext4_snapblk_t long long
>> >
>> > typedef signed long long int ext4_snapblk_t maybe ?
>>
>> 1. checkpatch doesn't like adding new typedef to the kernel
>
> Yes, I suppose that the reason is so that people does not add new
> typedefs like crazy, but when it is well reasoned I do not think it is a
> problem.
>
>> 2. I am in th process of removing that define altogether
>
> And use what instead ? ext4 typedefs helped people to realize what type
> to use for what operation and if this type is used often enough and does
> make sense (which I do not know since I have not seen the whole series
> yet), it can help as well.
>

I found a bug last week with accessing the last 4M of a 16TB snapshot file.
it was caused by conversion from ext4_snapblk_t to ext4_lblk_t in
ext4_blk_to_path(),
so I am dropping the different offset type approach and going handle
the snapshot
file offset translation inside ext4_blk_to_path().
I won't get into it here. You will see it on the next (full) patch
series I will post.

>> >
>> >> +#define SNAPSHOT_IND_PER_BLOCK_GROUP_BITS                    \
>> >> +     (SNAPSHOT_BLOCKS_PER_GROUP_BITS-SNAPSHOT_ADDR_PER_BLOCK_BITS)
>> >> +#define SNAPSHOT_IND_PER_BLOCK_GROUP                         \
>> >> +     (1<<SNAPSHOT_IND_PER_BLOCK_GROUP_BITS) /* 32 */
>> >> +#define SNAPSHOT_DIND_BLOCK_GROUPS_BITS                              \
>> >> +     (SNAPSHOT_ADDR_PER_BLOCK_BITS-SNAPSHOT_IND_PER_BLOCK_GROUP_BITS)
>> >> +#define SNAPSHOT_DIND_BLOCK_GROUPS                           \
>> >> +     (1<<SNAPSHOT_DIND_BLOCK_GROUPS_BITS)
>> >
>> > formating
>>
>> ?
>
> #define SNAPSHOT_IND_PER_BLOCK_GROUP_BITS                                  \
>        (SNAPSHOT_BLOCKS_PER_GROUP_BITS - SNAPSHOT_ADDR_PER_BLOCK_BITS)
> #define SNAPSHOT_IND_PER_BLOCK_GROUP                                       \
>        (1 << SNAPSHOT_IND_PER_BLOCK_GROUP_BITS) /* 32 */ <- 32 what ?
> #define SNAPSHOT_DIND_BLOCK_GROUPS_BITS                                    \
>        (SNAPSHOT_ADDR_PER_BLOCK_BITS - SNAPSHOT_IND_PER_BLOCK_GROUP_BITS)
> #define SNAPSHOT_DIND_BLOCK_GROUPS                                         \
>        (1 << SNAPSHOT_DIND_BLOCK_GROUPS_BITS)
>

OK. thanks.

>>
>> >
>> >> +
>> >> +#define SNAPSHOT_BLOCK_OFFSET                                        \
>> >> +     (SNAPSHOT_DIR_BLOCKS+SNAPSHOT_IND_BLOCKS)
>> >> +#define SNAPSHOT_BLOCK(iblock)                                       \
>> >> +     ((ext4_snapblk_t)(iblock) - SNAPSHOT_BLOCK_OFFSET)
>> >> +#define SNAPSHOT_IBLOCK(block)                                       \
>> >> +     (ext4_fsblk_t)((block) + SNAPSHOT_BLOCK_OFFSET)
>> >
>> > I do not see SNAPSHOT_BLOCK() defined anywhere.
>> >
>>
>> Do you mean you don't see it used anywhere?
>> It is used by later patches, but I do need to document it's meaning here.
>
> I have missed the define before SNAPSHOT_IBLOCK sorry.
>
> Thanks!
> -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