Re: [PATCH v3 07/18] xfs: handle sparse inode chunks in icreate log recovery

[Brian Foster <bfoster@redhat.com>]

On Sat, Feb 07, 2015 at 10:19:46AM +1100, Dave Chinner wrote:
> On Fri, Feb 06, 2015 at 02:52:54PM -0500, Brian Foster wrote:
> > Recovery of icreate transactions assumes hardcoded values for the inode
> > count and chunk length.
> > 
> > Sparse inode chunks are allocated in units of m_ialloc_min_blks. Update
> > the icreate validity checks to allow for appropriately sized inode
> > chunks and verify the inode count matches what is expected based on the
> > extent length rather than assuming a hardcoded count.
> > 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> >  fs/xfs/xfs_log_recover.c | 19 +++++++++++++------
> >  1 file changed, 13 insertions(+), 6 deletions(-)
> > 
> > diff --git a/fs/xfs/xfs_log_recover.c b/fs/xfs/xfs_log_recover.c
> > index ecc73d5..5a5ee20 100644
> > --- a/fs/xfs/xfs_log_recover.c
> > +++ b/fs/xfs/xfs_log_recover.c
> > @@ -3068,12 +3068,19 @@ xlog_recover_do_icreate_pass2(
> >  		return -EINVAL;
> >  	}
> >  
> > -	/* existing allocation is fixed value */
> > -	ASSERT(count == mp->m_ialloc_inos);
> > -	ASSERT(length == mp->m_ialloc_blks);
> > -	if (count != mp->m_ialloc_inos ||
> > -	     length != mp->m_ialloc_blks) {
> > -		xfs_warn(log->l_mp, "xlog_recover_do_icreate_trans: bad count 2");
> > +	/* inode chunk is either full or sparse */
> > +	if (length != mp->m_ialloc_blks &&
> > +	    length != mp->m_ialloc_min_blks) {
> > +		xfs_warn(log->l_mp,
> > +			 "%s: unsupported chunk length", __FUNCTION__);
> > +		return -EINVAL;
> > +	}
> 
> Hmmm - this would prevent recovery of sparse inode chunk allocation
> in multiples of mp->m_ialloc_min_blks, right? Surely we can allow
> any sub-chunk extent size to be allocated as long as alignment and
> size restrictions are met?
> 

Indeed it does prevent recovery of allocations in multiples of
m_ialloc_min_blks, but that is not supported at the moment so this alone
shouldn't cause any problems. The reason for the limitation is the same
general issue of dealing with allocations that may not be guaranteed to
convert directly to valid inode records. In other words, this is the
minimum allocation we can guarantee will span only a single record.

Now that I think about it after doing the whole agbno range thing,
perhaps an 'end_align' allocation argument might be a way to deal with
that one? For example, sparse allocs could request a cluster aligned
start bno and a chunk aligned end bno..? That might require multiple
sparse alloc attempts to take full advantage, probably requires more
thought either way...

Also note that for all fs' that this feature supports (i.e., v5), the
next multiple of the m_ialloc_min_blks corresponds to a full chunk (due
to the cluster size scaling). Given all of that, I'd at least prefer to
defer more advanced allocation support from this initial series as an
enhancement (provided there's value).

Brian

> Cheers,
> 
> Dave.
> -- 
> Dave Chinner
> david@fromorbit.com

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