Re: [PATCH 3/6] merge xfs_imap into xfs_dilocate

Dave Chinner
2008-11-03T01:24:44Z

On Mon, Oct 27, 2008 at 09:41:24AM -0400, Christoph Hellwig wrote:
> xfs_imap is the only caller of xfs_dilocate and doesn't add any significant
> value. Merge the two functions and document the various cases we have for
> inode cluster lookup in the new xfs_imap.
>
> Also remove the unused im_agblkno and im_ioffset fields from struct xfs_imap
> while we're at it.

A few small things....

> @@ -1262,34 +1255,65 @@ xfs_dilocate(
> #endif /* DEBUG */
> return XFS_ERROR(EINVAL);
> }
> - if ((mp->m_sb.sb_blocksize >= XFS_INODE_CLUSTER_SIZE(mp))) {
> +
> + /*
> + * If the inode cluster size is the same as the blocksize or
> + * bigger we get to the buffer by simple arithmetics.
> + */
> + if (XFS_INODE_CLUSTER_SIZE(mp) <= mp->m_sb.sb_blocksize) {

The comment doesn't match the code. This is the case where the block
size is the same or larger than the cluster size.

> offset = XFS_INO_TO_OFFSET(mp, ino);
> ASSERT(offset < mp->m_sb.sb_inopblock);
> - *bno = XFS_AGB_TO_FSB(mp, agno, agbno);
> - *off = offset;
> - *len = 1;
> +
> + imap->im_blkno = XFS_AGB_TO_DADDR(mp, agno, agbno);
> + imap->im_len = XFS_FSB_TO_BB(mp, 1);
> + imap->im_boffset = (ushort)(offset << mp->m_sb.sb_inodelog);
> return 0;
> }
> +
> blks_per_cluster = XFS_INODE_CLUSTER_SIZE(mp) >> mp->m_sb.sb_blocklog;
> - if (*bno != NULLFSBLOCK) {
> +
> + /*
> + * If we get a block number passed from bulkstat we can use it to
> + * find the buffer easily.
> + */
> + if (imap->im_blkno) {

I'm not sure I like this special case of blkno == 0 meaning
"no block set" - it is different to the rest of the code that
uses special values to indicate "no block set". At minimum it
needs documenting at the definition of struct xfs_imap, or
perhaps a new define for "NULLIMAPBLOCK"...

> +
> + /*
> + * With aligned inodes it's again quite simple and we can skip the
> + * btree lookup.
> + */
> if (mp->m_inoalign_mask) {
> offset_agbno = agbno & mp->m_inoalign_mask;
> chunk_agbno = agbno - offset_agbno;
> +
> + /*
> + * Worst case: we actually have to actually perform a lookup in the
> + * inode btree.
> + */
> } else {

I rather dislike this method of commenting if/else constructs
as it can make it hard to see the flow of the code at a glance.
Can you move the comment inside the else case, or combine the
comment with the one above the if/else. e.g.:

/*
* If the inode chunks are aligned then use simple maths to
* find the location. Otherwise we have to do a btree
* lookup to find the location.
*/

> + xfs_btree_cur_t *cur; /* inode btree cursor */
> + xfs_agino_t chunk_agino; /* first agino in inode chunk */
> + __int32_t chunk_cnt; /* count of free inodes in chunk */
> + xfs_inofree_t chunk_free; /* mask of free inodes in chunk */
> + xfs_buf_t *agbp; /* agi buffer */
> + int i; /* temp state */
> +
> down_read(&mp->m_peraglock);
> error = xfs_ialloc_read_agi(mp, tp, agno, &agbp);
> up_read(&mp->m_peraglock);
> if (error) {
> #ifdef DEBUG
> - xfs_fs_cmn_err(CE_ALERT, mp, "xfs_dilocate: "
> + xfs_fs_cmn_err(CE_ALERT, mp, "xfs_imap: "
> "xfs_ialloc_read_agi() returned "
> "error %d, agno %d",
> error, agno);

I think this should always be emitted here, not just for
debug kernels - it's indicative of a serious error, and
when we have CRC checking it will tell us exactly what
structure is corrupt...

> #ifdef DEBUG
> - xfs_fs_cmn_err(CE_ALERT, mp, "xfs_dilocate: "
> + xfs_fs_cmn_err(CE_ALERT, mp, "xfs_imap: "
> "xfs_inobt_get_rec() failed");
> #endif /* DEBUG */
> error = XFS_ERROR(EINVAL);
> }
> + error0:
> xfs_trans_brelse(tp, agbp);
> xfs_btree_del_cursor(cur, XFS_BTREE_NOERROR);

That will delete the cursor when there is an error with a "No Error"
trace. Not exactly what we want, right?

> + /*
> + * If the inode number maps to a block outside the bounds
> + * of the file system then return NULL rather than calling
> + * read_buf and panicing when we get an error from the
> + * driver.
> + */
> + if ((imap->im_blkno + imap->im_len) >
> + XFS_FSB_TO_BB(mp, mp->m_sb.sb_dblocks)) {
> + xfs_fs_cmn_err(CE_ALERT, mp, "xfs_imap: "
> + "(imap->im_blkno (0x%llx) + imap->im_len (0x%llx)) > "
> + " XFS_FSB_TO_BB(mp, mp->m_sb.sb_dblocks) (0x%llx)",
> + (unsigned long long) imap->im_blkno,
> + (unsigned long long) imap->im_len,
> + XFS_FSB_TO_BB(mp, mp->m_sb.sb_dblocks));
> + return EINVAL;

return XFS_ERROR(EINVAL);

To match the earlier out of bounds error checks.

Cheers,

Dave.
--
Dave Chinner
david@fromorbit.com