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

["Darrick J. Wong" <darrick.wong@oracle.com>]

On Wed, Aug 21, 2019 at 01:05:56AM +0000, Damien Le Moal wrote:
> 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.

Generally, *-for-next is the branch you want, particularly if you decide
at some point to add your zonedfs tree to the for-next zoo.

--D

> Best regards.
> 
> 
> -- 
> Damien Le Moal
> Western Digital Research