Re: [PATCH 26/30] xfs_db: avoid libxfs buffer lookup warnings

From: Dave Chinner <david@fromorbit.com>
To: Christoph Hellwig <hch@infradead.org>
Cc: xfs@oss.sgi.com
Subject: Re: [PATCH 26/30] xfs_db: avoid libxfs buffer lookup warnings
Date: Tue, 5 Nov 2013 11:52:48 +1100	[thread overview]
Message-ID: <20131105005248.GV6188@dastard> (raw)
In-Reply-To: <20131104091245.GF23564@infradead.org>

On Mon, Nov 04, 2013 at 01:12:45AM -0800, Christoph Hellwig wrote:
> >  
> > +/*
> > + * We are now using libxfs for our IO backend, so we should always try to use
> > + * inode cluster buffers rather than filesystem block sized buffers for reading
> > + * inodes. This means that we always use the same buffers as libxfs operations
> > + * does, and that avoids buffer cache issues caused by overlapping buffers. This
> > + * can be seen clearly when trying to read the root inode. Much of this logic is
> > + * similar to libxfs_imap().
> > + */
> >  void
> >  set_cur_inode(
> >  	xfs_ino_t	ino)
> > @@ -632,6 +640,9 @@ set_cur_inode(
> >  	xfs_agnumber_t	agno;
> >  	xfs_dinode_t	*dip;
> >  	int		offset;
> > +	int		numblks = blkbb;
> > +	xfs_agblock_t	cluster_agbno;
> > +
> >  
> >  	agno = XFS_INO_TO_AGNO(mp, ino);
> >  	agino = XFS_INO_TO_AGINO(mp, ino);
> > @@ -644,6 +655,24 @@ set_cur_inode(
> >  		return;
> >  	}
> >  	cur_agno = agno;
> > +
> > +	if (mp->m_inode_cluster_size > mp->m_sb.sb_blocksize &&
> > +	    mp->m_inoalign_mask) {
> > +		xfs_agblock_t	chunk_agbno;
> > +		xfs_agblock_t	offset_agbno;
> > +		int		blks_per_cluster;
> > +
> > +		blks_per_cluster = mp->m_inode_cluster_size >>
> > +							mp->m_sb.sb_blocklog;
> > +		offset_agbno = agbno & mp->m_inoalign_mask;
> > +		chunk_agbno = agbno - offset_agbno;
> > +		cluster_agbno = chunk_agbno +
> > +			((offset_agbno / blks_per_cluster) * blks_per_cluster);
> > +		offset += ((agbno - cluster_agbno) * mp->m_sb.sb_inopblock);
> > +		numblks = XFS_FSB_TO_BB(mp, blks_per_cluster);
> > +	} else
> > +		cluster_agbno = agbno;
> > +
> 
> Seems like this should be a separate patch?

Yes, probably should be.

> > -	if (iocur_top->bp)
> > +	if (iocur_top->bp) {
> >  		libxfs_putbuf(iocur_top->bp);
> > +		iocur_top->bp = NULL;
> > +	}
> 
> This should probably be folded into the patch that started using buffers
> in xfs_db.

Will do.

> 
> > +	int		icache_flags;	/* cache init flags */
> > +	int		bcache_flags;	/* cache init flags */
> 
> The icache_flags aren't used anywhere.  Also I'd really prefer to have
> my patch to kill the inode cache istead of adding more changes to it.

Sure - should I prepend your series to kill the icache to this one?

> > +#ifdef IO_BCOMPARE_CHECK
> > +		if (!(libxfs_bcache->c_flags & CACHE_MISCOMPARE_PURGE)) {
> > +			fprintf(stderr,
> > +	"%lx: Badness in key lookup (length)\n"
> > +	"bp=(bno 0x%llx, len %u bytes) key=(bno 0x%llx, len %u bytes)\n",
> > +				pthread_self(),
> > +				(unsigned long long)bp->b_bn, (int)bp->b_bcount,
> > +				(unsigned long long)bkey->blkno,
> > +				BBTOB(bkey->bblen));
> > +		}
> >  #endif
> 
> What is the point of the IO_BCOMPARE_CHECK ifdef if we unconditionally
> define it?

I'd like to - eventually - turn it off. In fact, I'd like to end up
with a real debug xfsprogs build like we do for the kernel code so
that the version that distros ship don't have this noise enabled but
developers can easily enable all the different debug/tracing stuff
to be able to find problems in the code....

Cheers,

Dave.
-- 
Dave Chinner
david@fromorbit.com

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