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
