From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from k22.active24.pl ([195.78.67.22]:41468 "EHLO k22.active24.pl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754951AbcEZRoN (ORCPT ); Thu, 26 May 2016 13:44:13 -0400 Subject: Re: r From: Krzysztof =?UTF-8?Q?B=C5=82aszkowski?= To: Christoph Hellwig Cc: Carlos Maiolino , linux-fsdevel@vger.kernel.org In-Reply-To: <20160526155348.GB28106@infradead.org> References: <1464273946.17980.15.camel@linux-q3cb.site> <20160526155348.GB28106@infradead.org> Content-Type: text/plain; charset="UTF-8" Date: Thu, 26 May 2016 19:44:09 +0200 Message-ID: <1464284649.17980.72.camel@linux-q3cb.site> Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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 > > 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@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Krzysztof Blaszkowski