On Fri, Jul 12, 2019 at 12:00:17PM +0900, Damien Le Moal wrote:
Hi Daminen,
Thanks for submitting zonefs.
Please find my first few comments, I'll have a second look later as well.
[...]
> Signed-off-by: Damien Le Moal <damien.lemoal@xxxxxxx>
> ---
> fs/Kconfig | 2 +
> fs/Makefile | 1 +
> fs/zonefs/Kconfig | 9 +
> fs/zonefs/Makefile | 4 +
> fs/zonefs/super.c | 1004 ++++++++++++++++++++++++++++++++++++
> fs/zonefs/zonefs.h | 190 +++++++
> include/uapi/linux/magic.h | 1 +
> 7 files changed, 1211 insertions(+)
> create mode 100644 fs/zonefs/Kconfig
> create mode 100644 fs/zonefs/Makefile
> create mode 100644 fs/zonefs/super.c
> create mode 100644 fs/zonefs/zonefs.h
It'll probably be good to add yourself as a maitainer in MAINTAINERS, so
people see there's a go-to person for patches. Also a list (fsdevel@) and a
git tree won't harm.
>
> diff --git a/fs/Kconfig b/fs/Kconfig
> index f1046cf6ad85..e48cc0e0efbb 100644
> --- a/fs/Kconfig
> +++ b/fs/Kconfig
> @@ -41,6 +41,7 @@ source "fs/ocfs2/Kconfig"
> source "fs/btrfs/Kconfig"
> source "fs/nilfs2/Kconfig"
> source "fs/f2fs/Kconfig"
> +source "fs/zonefs/Kconfig"
>
> config FS_DAX
> bool "Direct Access (DAX) support"
> @@ -262,6 +263,7 @@ source "fs/romfs/Kconfig"
> source "fs/pstore/Kconfig"
> source "fs/sysv/Kconfig"
> source "fs/ufs/Kconfig"
> +source "fs/ufs/Kconfig"
>
This hunk looks wrong.
> endif # MISC_FILESYSTEMS
>
[...]
> + /*
> + * Note: The first zone contains the super block: skip it.
> + */
I know I've been advocating for having on-disk metadata, but do we really
sacrifice a whole zone per default? I thought we'll have on-disk metadata
optional (I might be completely off the track here and need more coffee to
wake up though).
> + end = zones + sbi->s_nr_zones[ZONEFS_ZTYPE_ALL];
> + for (zone = &zones[1]; zone < end; zone = next) {
[...]
> +
> + /* Set defaults */
> + sbi->s_uid = GLOBAL_ROOT_UID;
> + sbi->s_gid = GLOBAL_ROOT_GID;
> + sbi->s_perm = S_IRUSR | S_IWUSR | S_IRGRP; /* 0640 */
> +
> +
> + ret = zonefs_read_super(sb);
> + if (ret)
> + return ret;
That would be cool to be controllable via a mount option and have it:
sbi->s_uid = opt.uid;
sbi->s_gid = opt.gid;
sbi->s_perm = opt.mode;
or pass these mount options to zonefs_read_super() and they can be set after
the feature validation.
> +
> + zones = zonefs_get_zone_info(sb);
> + if (IS_ERR(zones))
> + return PTR_ERR(zones);
> +
> + pr_info("zonefs: Mounting %s, %u zones",
> + sb->s_id, sbi->s_nr_zones[ZONEFS_ZTYPE_ALL]);
> +
> + /* Create root directory inode */
> + ret = -ENOMEM;
> + inode = new_inode(sb);
> + if (!inode)
> + goto out;
Nit: please add a blank line after the goto.
> + inode->i_ino = get_next_ino();
> + inode->i_mode = S_IFDIR | 0755;
> + inode->i_ctime = inode->i_mtime = inode->i_atime = current_time(inode);
> + inode->i_op = &simple_dir_inode_operations;
> + inode->i_fop = &simple_dir_operations;
> + inode->i_size = sizeof(struct dentry) * 2;
> + set_nlink(inode, 2);
Nit: please add a blank line here as well.
> + sb->s_root = d_make_root(inode);
> + if (!sb->s_root)
> + goto out;
[...]
> +/*
> + * Maximum length of file names: this only needs to be large enough to fit
> + * the zone group directory names and a decimal value of the start sector of
> + * the zones for file names. 16 characterse is plenty.
characters ^
[...]
> +struct zonefs_super {
> +
> + /* Magic number */
> + __le32 s_magic; /* 4 */
> +
> + /* Metadata version number */
> + __le32 s_version; /* 8 */
> +
> + /* Features */
> + __le64 s_features; /* 16 */
> +
> + /* 128-bit uuid */
> + __u8 s_uuid[16]; /* 32 */
> +
> + /* UID/GID to use for files */
> + __le32 s_uid; /* 36 */
> + __le32 s_gid; /* 40 */
> +
> + /* File permissions */
> + __le32 s_perm; /* 44 */
> +
> + /* Padding to 4K */
> + __u8 s_reserved[4052]; /* 4096 */
> +
> +} __attribute__ ((packed));
I'm not sure the (end)offset comments are of any value here, it's nothing that
can't be obtained from pahole or gdb (or even by hand).
> +
> +/*
> + * Metadata version.
> + */
> +#define ZONEFS_VERSION 1
> +
> +/*
> + * Feature flags.
> + */
> +enum zonefs_features {
> + /*
> + * Use a zone start sector value as file name.
> + */
> + ZONEFS_F_STARTSECT_NAME,
> + /*
> + * Aggregate contiguous conventional zones into a single file.
> + */
> + ZONEFS_F_AGRCNV,
> + /*
> + * Use super block specified UID for files instead of default.
> + */
> + ZONEFS_F_UID,
> + /*
> + * Use super block specified GID for files instead of default.
> + */
> + ZONEFS_F_GID,
> + /*
> + * Use super block specified file permissions instead of default 640.
> + */
> + ZONEFS_F_PERM,
> +};
I'd rather not write the uid, gid, permissions and startsect name to the
superblock but have it controllable via a mount option. Just write the feature
to the superblock so we know we _can_ control this per mount.
Byte,
Johannes
--
Johannes Thumshirn SUSE Labs Filesystems
jthumshirn@xxxxxxx +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
