On Tue, Jun 7, 2011 at 1:42 PM, Lukas Czerner <lczerner@xxxxxxxxxx> 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@xxxxxxxxxxxx>, 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
