Re: [PATCH v3 11/18] xfs: randomly do sparse inode allocations in DEBUG mode

[Brian Foster <bfoster@redhat.com>]

On Mon, Feb 09, 2015 at 10:02:32AM +1100, Dave Chinner wrote:
> On Fri, Feb 06, 2015 at 02:52:58PM -0500, Brian Foster wrote:
> > Sparse inode allocations generally only occur when full inode chunk
> > allocation fails. This requires some level of filesystem space usage and
> > fragmentation.
> > 
> > For filesystems formatted with sparse inode chunks enabled, do random
> > sparse inode chunk allocs when compiled in DEBUG mode to increase test
> > coverage.
> > 
> > Signed-off-by: Brian Foster <bfoster@redhat.com>
> > ---
> >  fs/xfs/libxfs/xfs_ialloc.c | 19 +++++++++++++++++--
> >  1 file changed, 17 insertions(+), 2 deletions(-)
> > 
> > diff --git a/fs/xfs/libxfs/xfs_ialloc.c b/fs/xfs/libxfs/xfs_ialloc.c
> > index 090d114..3e5d3eb 100644
> > --- a/fs/xfs/libxfs/xfs_ialloc.c
> > +++ b/fs/xfs/libxfs/xfs_ialloc.c
> > @@ -652,9 +652,18 @@ xfs_ialloc_ag_alloc(
> >  
> >  	struct xfs_perag *pag;
> >  
> > +#ifdef DEBUG
> > +	int		do_sparse = 0;
> > +
> > +	/* randomly do sparse inode allocations */
> > +	if (xfs_sb_version_hassparseinodes(&tp->t_mountp->m_sb))
> > +		do_sparse = prandom_u32() & 1;
> > +#endif
> > +
> 
> Bit ugly with all the ifdefs. If you define the do_sparse variable
> outside the ifdef, then the rest of the code other than this check
> doesn't need ifdefs.
> 

Ok.

> >  	memset(&args, 0, sizeof(args));
> >  	args.tp = tp;
> >  	args.mp = tp->t_mountp;
> > +	args.fsbno = NULLFSBLOCK;
> >  
> >  	/*
> >  	 * Locking will ensure that we don't have two callers in here
> > @@ -675,6 +684,10 @@ xfs_ialloc_ag_alloc(
> >  	agno = be32_to_cpu(agi->agi_seqno);
> >  	args.agbno = XFS_AGINO_TO_AGBNO(args.mp, newino) +
> >  		     args.mp->m_ialloc_blks;
> > +#ifdef DEBUG
> > +	if (do_sparse)
> > +		goto sparse_alloc;
> > +#endif
> >  	if (likely(newino != NULLAGINO &&
> >  		  (args.agbno < be32_to_cpu(agi->agi_length)))) {
> >  		args.fsbno = XFS_AGB_TO_FSB(args.mp, agno, args.agbno);
> > @@ -713,8 +726,7 @@ xfs_ialloc_ag_alloc(
> >  		 * subsequent requests.
> >  		 */
> >  		args.minalignslop = 0;
> > -	} else
> > -		args.fsbno = NULLFSBLOCK;
> > +	}
> >  
> >  	if (unlikely(args.fsbno == NULLFSBLOCK)) {
> >  		/*
> > @@ -769,6 +781,9 @@ xfs_ialloc_ag_alloc(
> >  	 * Finally, try a sparse allocation if the filesystem supports it and
> >  	 * the sparse allocation length is smaller than a full chunk.
> >  	 */
> > +#ifdef DEBUG
> > +sparse_alloc:
> > +#endif
> >  	if (xfs_sb_version_hassparseinodes(&args.mp->m_sb) &&
> >  	    args.mp->m_ialloc_min_blks < args.mp->m_ialloc_blks &&
> >  	    args.fsbno == NULLFSBLOCK) {
> 
> The label can go after the if() statement, right? We've already
> guaranteed all those other parameters are true, though I suspect
> there's a case where that m_ialloc_min_blks < m_ialloc_blks will
> fail: block size larger than inode chunk size (e.g. 64k block size, 512
> byte inodes) so that would result in the code above failing to
> allocate any inode chunks at all...
> 

Hmm, yeah I added that blks comparison recently simply to prevent
another allocation attempt on such systems where the block size is >=
the chunk size.

This changes behavior of such systems either way, but debug mode has
that effect regardless I suppose. We hit a codepath that we wouldn't
normally hit in that large bsize configuration, but it's debug mode and
the alignment/size of the allocation should be equivalent to the
original allocation code so I think it should be fine to move the label.

Brian

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

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