Hi Christoph,
On Thu, 2016-05-26 at 08:53 -0700, Christoph Hellwig wrote:
> Hi Krzysztof,
>
> first round of reviews below:
>
> > Date: Wed, 25 May 2016 21:50:11 +0200
> > Subject: [PATCH 1/7] kconfig note
> >
> >
> > Signed-off-by: KB <kb@xxxxxxxxxxxxxxx>
>
> This part looks good, but I'd add a littlle more text and move it last
> in the series.
okay, what would you write in this section ?
Will move the patch.
> The other patches could also use a little more
> descriptive text in the changelogs.
>
> > diff --git a/fs/freevxfs/vxfs.h b/fs/freevxfs/vxfs.h
> > index c8a9265..5dc8949 100644
> > --- a/fs/freevxfs/vxfs.h
> > +++ b/fs/freevxfs/vxfs.h
> > @@ -152,7 +152,7 @@ struct vxfs_sb {
> > /*
> > * Actually much more...
> > */
> > -};
> > +} __packed;
>
> Can you explain why this is added?
>
I was thinking that on-disk memory mapped structures should be "packed"
especially if they contain 1 or 2 bytes fields in the middle.
If the structure does not have the packed attribute then its size may be
bigger than the size of similar structure (by name) used on another OS
e.g. 32bit. An access to fields of such structure aligned differently
(due to e.g. 64 bit compiler) will end up with data corruption.
(that's the basics, why i write this ?)
the vxfs_sb structure has u8 and u16's so usage of __pack is desired
over here. the __pack will not cause any harm (because if we count all
u8 and u16, they are still u32 aligned) and it will prevent from
hypothetical misalignment (just in case). It is also a tip that the
structure is on-disk and must be compiler alignment independent.
> >
> >
> > /*
> > @@ -168,9 +168,32 @@ struct vxfs_sb_info {
> > ino_t vsi_fshino; /* fileset header inode */
> > daddr_t vsi_oltext; /* OLT extent */
> > daddr_t vsi_oltsize; /* OLT size */
> > + int byte_order;
> > + int silent;
> > +};
>
> Can you align these with the rest of the fields? Also where does the
> silent flag come from here?
I reckon aligning it is not necessary, because this structure is
run-time, cpu endian and it lives in memory only (but storage device)
"silent" int comes from arg passed over to vxfs_fill_super() of the same
name. My workaround, I do not like functions which have long arguments
list. 2 args is fine, 3 - maybe too long, 4 - it must be done something,
5+ - bad day ? ;)
>
> > +#define VXFS_FS32(field1, field2) fhp->field1 = fs32_to_cpu(bo, dbh->field2)
>
> Please remove this wrapper.
>
> > +#define VXFS_FS32(field1, field2) vip->field1 = fs32_to_cpu(bo, dip->field2)
> > +#define VXFS_FS64(field1, field2) vip->field1 = fs64_to_cpu(bo, dip->field2)
> > +#define VXFS_FS16(field1, field2) vip->field1 = fs16_to_cpu(bo, dip->field2)
>
> and these.
well, this will be difficult. These macros help saving lots of typing
over and over again.
although they raise checkpatch errors I will not opt-out because scope
of these macros is local. These macros are not intended to be used
anywhere else than functions like: dip2vip_cpy() and dbh2fhp().
I can #undef them later. Will you agree ?
>
> > + sbp->s_fs_info = infp;
> > + do {
> > + if (!vxfs_try_sb_magic(sbp, 1, cpu_to_le32(VXFS_SUPER_MAGIC))) {
> > + infp->byte_order = BO_LE; /* SCO */
> > + break;
> > + }
> > +
> > + if (!vxfs_try_sb_magic(sbp, 8, cpu_to_be32(VXFS_SUPER_MAGIC))) {
> > + infp->byte_order = BO_BE; /* HP-UX pa-risc likely */
> > + break;
>
> I'd normally move this into a separate patch. But even if this
> stay in here it should be mentioned in the patch changelog.
indeed, that's quite important new feature.
changelog ?, where is it ?
>
> > vxfs_put_fake_inode(infp->vsi_ilist);
> > vxfs_put_fake_inode(infp->vsi_stilist);
> > out:
> > - brelse(bp);
> > + if (infp->vsi_bp)
> > + brelse(infp->vsi_bp);
>
> brelse handles a null argument just fine.
wasn't sure so added the "if" just in case of it didn't handle.
that "if" may stay there still.
>
> > extern void vxfs_inode_info_free(struct vxfs_inode_info *vip);
> > +extern void vxfs_destroy_inode(struct inode *ip);
> >
> > /* vxfs_lookup.c */
> > extern const struct inode_operations vxfs_dir_inode_ops;
> > diff --git a/fs/freevxfs/vxfs_inode.c b/fs/freevxfs/vxfs_inode.c
> > index 16d8d27..9b45ad7 100644
> > --- a/fs/freevxfs/vxfs_inode.c
> > +++ b/fs/freevxfs/vxfs_inode.c
> > @@ -397,7 +397,15 @@ vxfs_iget(struct super_block *sbp, ino_t ino)
> > static void vxfs_i_callback(struct rcu_head *head)
> > {
> > struct inode *inode = container_of(head, struct inode, i_rcu);
> > - kmem_cache_free(vxfs_inode_cachep, inode->i_private);
> > + void *priv = inode->i_private;
> > +
> > + inode->i_private = NULL;
> > + kmem_cache_free(vxfs_inode_cachep, priv);
> > +}
> > +
> > +void vxfs_destroy_inode(struct inode *ip)
> > +{
> > + call_rcu(&ip->i_rcu, vxfs_i_callback);
> > }
>
> I'd rather go for embedding the VFS inode in the VxFS inode like most
> modern file systems do. Take a look at alloc_inode and destroy_inode
> in ext2 for an example.
right, and then alloc callback will be necessary. this will introduce
more changes to e.g. get_fake_inode() and around this function.
Do you really want to have this "more perfect" ?
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Krzysztof Blaszkowski
--
To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
