Re: [PATCH 01/16] xfs: introduce directory geometry structure

[Brian Foster <bfoster@redhat.com>]

On Mon, May 26, 2014 at 02:28:07PM +1000, Dave Chinner wrote:
> On Fri, May 23, 2014 at 03:04:59PM -0400, Brian Foster wrote:
> > On Fri, May 23, 2014 at 10:03:37AM +1000, Dave Chinner wrote:
> > > From: Dave Chinner <dchinner@redhat.com>
> > > 
> > > The directory code has a dependency on the struct xfs_mount to
> > > supply the directory block geometry. Block size, block log size,
> > > and other parameters are pre-caclulated in the struct xfs_mount or
> > > access directly from the superblock embedded in the struct
> > > xfs_mount.
> > > 
> > > Extract all of this geometry information out of the struct xfs_mount
> > > and superblock and place it into a new struct xfs_da_geometry
> > > defined by the directory code. Allocate and initialise it at mount
> > > time, and attach it to the struct xfs_mount so it canbe passed back
> > > into the directory code appropriately rather than using the struct
> > > xfs_mount.
> > > 
> > > Signed-off-by: Dave Chinner <dchinner@redhat.com>
> ....
> > > @@ -99,24 +100,56 @@ xfs_dir_mount(
> > >  	mp->m_dir_inode_ops = xfs_dir_get_ops(mp, NULL);
> > >  	mp->m_nondir_inode_ops = xfs_nondir_get_ops(mp, NULL);
> > >  
> > > -	mp->m_dirblksize = 1 << (mp->m_sb.sb_blocklog + mp->m_sb.sb_dirblklog);
> > > -	mp->m_dirblkfsbs = 1 << mp->m_sb.sb_dirblklog;
> > > -	mp->m_dirdatablk = xfs_dir2_db_to_da(mp, XFS_DIR2_DATA_FIRSTDB(mp));
> > > -	mp->m_dirleafblk = xfs_dir2_db_to_da(mp, XFS_DIR2_LEAF_FIRSTDB(mp));
> > > -	mp->m_dirfreeblk = xfs_dir2_db_to_da(mp, XFS_DIR2_FREE_FIRSTDB(mp));
> > > -
> > >  	nodehdr_size = mp->m_dir_inode_ops->node_hdr_size;
> > > -	mp->m_attr_node_ents = (mp->m_sb.sb_blocksize - nodehdr_size) /
> > > +	mp->m_dir_geo = kmem_zalloc(sizeof(struct xfs_da_geometry),
> > > +				    KM_SLEEP | KM_MAYFAIL);
> > > +	mp->m_attr_geo = kmem_zalloc(sizeof(struct xfs_da_geometry),
> > > +				     KM_SLEEP | KM_MAYFAIL);
> > > +	if (!mp->m_dir_geo || !mp->m_attr_geo) {
> > > +		kmem_free(mp->m_dir_geo);
> > > +		kmem_free(mp->m_attr_geo);
> > > +		return ENOMEM;
> > > +	}
> > 
> > While it looks like everything is handled correctly here, I think this
> > would be much cleaner if we just created a set of xfs_mount_alloc/free()
> > helpers that did all of the allocations at once. Then we wouldn't have a
> > situation where the caller has non-obvious memory allocations to clean
> > up should it fail sometime after calling xfs_da_mount().
> 
> I think that each subsystem needs to have it's own init/teardown
> functions, and that the rest of the code needs to call them
> appropriately. Yes, error handling can be non-trivial, but the idea
> that we do unconditional allocation for subsystems/configs that we
> may not use (eg. due to mkfs options) seems a little wrong to me.
> 
> And aggregating all the allocations doesn't remove the need for most
> subsystems to be able to return initialisation errors, so int he
> long run it doesn't really change the fact that we have to
> initialise and then tear down on subsequent subsystem init
> failures...
> 

Part of the reason I suggested that approach was that the allocation
appeared unconditional. Now that I think about it, a related question is
why not embed the structers into the mount?

No matter, perhaps there is good reason for that. If we do want to keep
it localized to the subsystem, creating an xfs_da_unmount() (perhaps
just an inline) would be cleaner and more consistent IMO.

Brian

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com
> 
> _______________________________________________
> xfs mailing list
> xfs@oss.sgi.com
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs