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

Christoph Hellwig
2013-11-04T09:12:46Z

>
> +/*
> + * 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?

> - 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.

> + 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.

> +#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?