From: Johannes Thumshirn <jthumshirn@suse.de>
To: Damien Le Moal <damien.lemoal@wdc.com>
Cc: linux-fsdevel@vger.kernel.org, linux-xfs@vger.kernel.org,
Christoph Hellwig <hch@lst.de>, Hannes Reinecke <hare@suse.de>
Subject: Re: [PATCH RFC] fs: New zonefs file system
Date: Fri, 12 Jul 2019 10:00:22 +0200 [thread overview]
Message-ID: <20190712080022.GA16276@x250.microfocus.com> (raw)
In-Reply-To: <20190712030017.14321-1-damien.lemoal@wdc.com>
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@wdc.com>
> ---
> 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@suse.de +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
next prev parent reply other threads:[~2019-07-12 8:00 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-07-12 3:00 [PATCH RFC] fs: New zonefs file system Damien Le Moal
2019-07-12 8:00 ` Johannes Thumshirn [this message]
2019-07-12 8:31 ` Damien Le Moal
2019-07-12 8:47 ` Johannes Thumshirn
2019-07-12 17:10 ` Viacheslav Dubeyko
2019-07-12 22:56 ` Damien Le Moal
2019-07-15 16:54 ` Viacheslav Dubeyko
2019-07-15 23:53 ` Damien Le Moal
2019-07-16 16:51 ` Viacheslav Dubeyko
2019-07-18 0:57 ` Damien Le Moal
2019-07-15 1:19 ` Dave Chinner
2019-07-15 6:57 ` Johannes Thumshirn
2019-07-16 11:21 ` Damien Le Moal
2019-07-18 14:11 ` Jeff Moyer
2019-07-18 23:02 ` Damien Le Moal
2019-07-19 14:25 ` Jeff Moyer
2019-07-20 1:07 ` Damien Le Moal
2019-07-22 0:12 ` Dave Chinner
2019-07-20 7:15 ` Damien Le Moal
2019-07-22 0:04 ` Dave Chinner
2019-07-22 0:09 ` Damien Le Moal
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20190712080022.GA16276@x250.microfocus.com \
--to=jthumshirn@suse.de \
--cc=damien.lemoal@wdc.com \
--cc=hare@suse.de \
--cc=hch@lst.de \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-xfs@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).