On 2019/07/12 17:00, Johannes Thumshirn wrote:
> 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.
Oops. Yes, indeed. I forgot to add that. I will.
>
>>
>> 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.
Yes it is wrong. No idea how that sneaked in here. Will fix it.
>
>> 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).
Yes, indeed we do not really need the super block for now. But it is still super
useful to have so that:
1) libblkid and other such userland tools can probe the disk to see its format,
and preserve the usual "use -force option if you really want to overwrite"
behavior of all format tools.
2) Still related to previous point, the super block allows commands like:
mount /dev/sdX /mnt
and
mount -t zonefs /dev/sdX /mnt
to have the same result. That is, without the super block, if the drive was
previously formatted for btrfs or f2fs, the first command will mount that old
format, while the second will mount zonefs without necessarily erasing the old
FS super block.
3) Having the super block with a version number will allow in the future to add
more metadata (e.g. file names as decided by the application) while allowing
backward compatibility of the code.
>> + 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.
Yes, I thought of that and even had that implemented in a previous version. I
switched to the static format time definition only so that the resulting
operation of the FS is a little more like a normal file system, namely, mounting
the device does not change file attributes and so can be mounted and seen with
the same attribute no matter where it is mounted, regardless of the mount options.
> [...]
Thanks for the nits and typos pointer. I will fix that.
>> +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).
OK. Will remove.
>> +/*
>> + * 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.
This is another view. See my thinking above. Thoughts ?
Thanks !
--
Damien Le Moal
Western Digital Research
