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]
Message-ID: <alpine.LFD.2.00.1106071232070.4754@dhcp-27-109.brq.redhat.com>
Date:	Tue, 7 Jun 2011 12:42:14 +0200 (CEST)
From:	Lukas Czerner <lczerner@...hat.com>
To:	"Amir G." <amir73il@...rs.sourceforge.net>
cc:	Lukas Czerner <lczerner@...hat.com>, 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, 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:)

> 
> >>
> >>  #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.

> 
> >
> >> + */
> >> +
> >> +#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.

> >
> >> +#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)

> 
> >
> >> +
> >> +#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