Re: [PATCH V2] fs: New zonefs file system

[Damien Le Moal <Damien.LeMoal@wdc.com>]

Darrick,

On 2019/08/21 0:26, Darrick J. Wong wrote:
[...]
>> +/*
>> + * Read super block information from the device.
>> + */
>> +static int zonefs_read_super(struct super_block *sb)
>> +{
>> +     struct zonefs_sb_info *sbi = ZONEFS_SB(sb);
>> +     struct zonefs_super *super;
>> +     struct bio bio;
>> +     struct bio_vec bio_vec;
>> +     struct page *page;
>> +     int ret;
>> +
>> +     page = alloc_page(GFP_KERNEL);
>> +     if (!page)
>> +             return -ENOMEM;
>> +
>> +     bio_init(&bio, &bio_vec, 1);
>> +     bio.bi_iter.bi_sector = 0;
>> +     bio_set_dev(&bio, sb->s_bdev);
>> +     bio_set_op_attrs(&bio, REQ_OP_READ, 0);
>> +     bio_add_page(&bio, page, PAGE_SIZE, 0);
>> +
>> +     ret = submit_bio_wait(&bio);
>> +     if (ret)
>> +             goto out;
>> +
>> +     ret = -EINVAL;
>> +     super = page_address(page);
>> +     if (le32_to_cpu(super->s_magic) != ZONEFS_MAGIC)
>> +             goto out;
>> +     sbi->s_features = le64_to_cpu(super->s_features);
>> +     if (zonefs_has_feature(sbi, ZONEFS_F_UID)) {
>> +             sbi->s_uid = make_kuid(current_user_ns(),
>> +                                    le32_to_cpu(super->s_uid));
>> +             if (!uid_valid(sbi->s_uid))
>> +                     goto out;
>> +     }
>> +     if (zonefs_has_feature(sbi, ZONEFS_F_GID)) {
>> +             sbi->s_gid = make_kgid(current_user_ns(),
>> +                                    le32_to_cpu(super->s_gid));
>> +             if (!gid_valid(sbi->s_gid))
>> +                     goto out;
>> +     }
>> +     if (zonefs_has_feature(sbi, ZONEFS_F_PERM))
>> +             sbi->s_perm = le32_to_cpu(super->s_perm);
> 
> Unknown feature bits are silently ignored.  Is that intentional?  Will
> all features be compat features?  I would find it a little annoying to
> format (for example) a F_UID filesystem only to have some old kernel
> driver ignore it.

Good point. I will add checks for unknown feature bits.

>> +/*
>> + * On-disk super block (block 0).
>> + */
>> +struct zonefs_super {
>> +
>> +     /* Magic number */
>> +     __le32          s_magic;
>> +
>> +     /* Features */
>> +     __le64          s_features;
>> +
>> +     /* 128-bit uuid */
>> +     uuid_t          s_uuid;
>> +
>> +     /* UID/GID to use for files */
>> +     __le32          s_uid;
>> +     __le32          s_gid;
>> +
>> +     /* File permissions */
>> +     __le32          s_perm;
>> +
>> +     /* Padding to 4K */
>> +     __u8            s_reserved[4056];
> 
> Hmm, I noticed that fill_super doesn't check that s_reserved is actually
> zero (or any specific value).  You might consider enforcing that so that
> future you can add fields beyond s_perm without having to burn a
> s_features bit every time you do it.

Indeed. Will fix that.

> Also a little surprised there's no checksum field here to detect bit
> flips and such. ;)

Yep. I did have that on my to-do list but forgot to actually do it... Will fix
everything and send a v3.

Thank you for the comments. One more question: should I rebase on your
iomap-for-next branch or on iomap-5.4-merge branch ? Both branches look
identical right now.

Best regards.


-- 
Damien Le Moal
Western Digital Research