On Thu, Mar 28, 2013 at 06:40:23PM -0500, Ben Myers wrote:
> Hey Dave,
>
> On Tue, Mar 12, 2013 at 11:30:45PM +1100, Dave Chinner wrote:
> > From: Dave Chinner <dchinner@xxxxxxxxxx>
> >
> > This addition follows the same pattern as the dir2 block CRCs, but
> > with a few differences. The main difference is that the free block
> > header is different between the v2 and v3 formats, so an "in-core"
> > free block header has been added and _todisk/_from_disk functions
> > used to abstract the differences in structure format from the code.
> > This is similar to the on-disk superblock versus the in-core
> > superblock setup. The in-core strucutre is populated when the buffer
> > is read from disk, all the in memory checks and modifications are
> > done on the in-core version of the structure which is written back
> > to the buffer before the buffer is logged.
> >
> > Signed-off-by: Dave Chinner <dchinner@xxxxxxxxxx>
>
> Just a few nits below.
>
> > -static void
> > -xfs_dir2_free_verify(
> > +static bool
> > +xfs_dir3_free_verify(
> > struct xfs_buf *bp)
> > {
> > struct xfs_mount *mp = bp->b_target->bt_mount;
> > struct xfs_dir2_free_hdr *hdr = bp->b_addr;
> > - int block_ok = 0;
> >
> > - block_ok = hdr->magic == cpu_to_be32(XFS_DIR2_FREE_MAGIC);
> > - if (!block_ok) {
> > - XFS_CORRUPTION_ERROR("xfs_dir2_free_verify magic",
> > - XFS_ERRLEVEL_LOW, mp, hdr);
> > - xfs_buf_ioerror(bp, EFSCORRUPTED);
> > + if (xfs_sb_version_hascrc(&mp->m_sb)) {
> > + struct xfs_dir3_blk_hdr *hdr3 = bp->b_addr;
> > +
> > + if (hdr3->magic != be32_to_cpu(XFS_DIR3_FREE_MAGIC))
> > + return false;
> > + if (!uuid_equal(&hdr3->uuid, &mp->m_sb.sb_uuid))
> > + return false;
> > + if (be64_to_cpu(hdr3->blkno) != bp->b_bn)
> > + return false;
> > + } else {
> > + if (hdr->magic != be32_to_cpu(XFS_DIR2_FREE_MAGIC))
> > + return false;
> > }
> > +
> > + /* XXX: should bounds check the xfs_dir3_icfree_hdr here */
>
> What did you have in mind?
Checking that the header fields:
a) are within valid bounds; and
b) match what is in the bests array.
> > - if (!hdr) {
> > - /* One less used entry in the free table. */
> > - be32_add_cpu(&free->hdr.nused, -1);
> > - xfs_dir2_free_log_header(tp, fbp);
> >
> > - /*
> > - * If this was the last entry in the table, we can trim the
> > - * table size back. There might be other entries at the end
> > - * referring to non-existent data blocks, get those too.
> > - */
>
> You dropped this comment. Was there something wrong with it?
No, just an oversight when rearranging the code.
> > /* Log the free entry that changed, unless we got rid of it. */
> > if (logfree)
> > xfs_dir2_free_log_bests(tp, fbp, findex, findex);
> > @@ -1062,10 +1221,15 @@ xfs_dir2_leafn_remove(
> > if (error)
> > return error;
> > free = fbp->b_addr;
> > - ASSERT(free->hdr.magic == cpu_to_be32(XFS_DIR2_FREE_MAGIC));
> > - ASSERT(be32_to_cpu(free->hdr.firstdb) ==
> > - xfs_dir2_free_max_bests(mp) *
> > - (fdb - XFS_DIR2_FREE_FIRSTDB(mp)));
> > +#ifdef DEBUG
> > + {
> > + struct xfs_dir3_icfree_hdr freehdr;
> > + xfs_dir3_free_hdr_from_disk(&freehdr, free);
> > + ASSERT(be32_to_cpu(freehdr.firstdb) ==
>
> I think freehdr.firstdb is already in cpu endianness.
Right. Well spotted. I've already posted a fix for it - sparse
endian checking picked this up. My current series folds those fixes
back into the original patches....
Cheers,
Dave.
--
Dave Chinner
david@xxxxxxxxxxxxx
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
