lkml.org 
[lkml]   [2007]   [Jul]   [31]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [RFC 12/26] ext2 white-out support
    On Mon, Jul 30, Theodore Tso wrote:

    > On Mon, Jul 30, 2007 at 06:13:35PM +0200, Jan Blunck wrote:
    > > Introduce white-out support to ext2.
    > >
    > > Known Bugs:
    > > - Needs a reserved inode number for white-outs
    >
    > You picked different reserved inodes for the ext2 and ext3
    > filesystems. That's good for a NACK right there. The codepoints
    > (i.e., reserved inode numbers, feature bit masks, etc.) for ext2,
    > ext3, and ext4 MUST not overlap. After all, someone might use tune2fs
    > -j to convert an ext2 filesystem to ext3, and is it's REALLY BAD that
    > you're using a reserved inode of 7 for ext2, and 9 for ext3.

    Ouch, right.

    > Also, I note that you have created a new INCOMPAT feature flag support
    > for whiteouts. That's really unfortunate; we try to avoid introducing
    > incompatible feature flags unless absolutely necessary; note that even
    > adding a COMPAT feature flag means that you need a new version of
    > e2fsprogs if you want e2fsck to be willing to touch that filesystem.
    >
    > So --- if you're looking for a way to add whiteout support to
    > ext2/ext3 without needing a feature bit, here's how. We allocate a
    > new inode flag in struct ext3_inode.i_flags:
    >
    > #define EXT2_WHTOUT_FL 0x00040000
    >
    > We also allocate a new field in the ext2 superblock to store the
    > "whiteout inode". (Please coordinate with me so it's a superblock
    > field not in use by ext3/ext4, and so it's reserved so that no one
    > else uses it.) The superblock field, call it s_whtout_ino, stores the
    > inode number for the "white out inode".
    >
    > When you create a new whiteout file, the code checks sb->s_whtout_ino,
    > and if it is zero, it allocates a new inode, and creates it as a
    > zero-length regular file (i_mode |= S_IFREG) with the EXT2_WHTOUT_FL
    > flag set in the inode, and then store the inode number in
    > sb->s_whtout_ino. If sb->s_whtout_ino is non-zero, you must read in
    > the inode and make sure that the EXT2_WHTOUT_FL is set. If it is not,
    > then allocate a new whiteout inode as described previously. Then link
    > the inode into the directory as before.
    >
    > When reading an inode, if the EXT2_WHTOUT_FL flag is set, then set the
    > in-memory mode of the inode to be S_IFWHT.
    >
    > That's pretty much about it. For cleanliness sake, it would be good
    > if ext2_delete_inode clears sb->s_whtout_ino if the last whiteout link
    > has been deleted, but it's strictly speaking not necessary. If you do
    > it this way, the filesystem is completely backwards compatible; the
    > whiteout files will just appear to links to a normal zero-lenth file.

    Ok, this is pretty similar to the way I implemented this for tmpfs. The
    problem is that the union mount code is explicitly checking if the filesystem
    is supporting whiteout. I used to use a new filesystem flag (FS_WHITEOUT) for
    this but thought that disk filesystem like ext2/3/4 will have problem with
    that if you mount an old image. So I guess I still need a feature flag.

    > I wouldn't bother with setting the directory type field to be DT_WHT,
    > given that they will never be returned to userspace anyway.

    At the moment I still rely on this for the current readdir implementation.
    Viro already said that he doesn't want to see this (the readdir changes) in
    the kernel but in userspace.

    Thanks,
    Jan
    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2007-07-31 09:47    [W:0.031 / U:29.116 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site