xfs
[Top] [All Lists]

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

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 26/30] xfs_db: avoid libxfs buffer lookup warnings
From: Dave Chinner <david@xxxxxxxxxxxxx>
Date: Tue, 5 Nov 2013 11:52:48 +1100
Cc: xfs@xxxxxxxxxxx
Delivered-to: xfs@xxxxxxxxxxx
In-reply-to: <20131104091245.GF23564@xxxxxxxxxxxxx>
References: <1383107481-28937-1-git-send-email-david@xxxxxxxxxxxxx> <1383107481-28937-27-git-send-email-david@xxxxxxxxxxxxx> <20131104091245.GF23564@xxxxxxxxxxxxx>
User-agent: Mutt/1.5.21 (2010-09-15)
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@xxxxxxxxxxxxx

<Prev in Thread] Current Thread [Next in Thread>