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. 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?
>
>
> /*
> @@ -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?
> +#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.
> + 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.
> 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.
> 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.
--
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
