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?
> +static int
> +xfs_dir3_free_get_buf(
> + struct xfs_trans *tp,
> + struct xfs_inode *dp,
> + xfs_dir2_db_t fbno,
> + struct xfs_buf **bpp)
> +{
> + struct xfs_mount *mp = dp->i_mount;
> + struct xfs_buf *bp;
> + int error;
> + struct xfs_dir3_icfree_hdr hdr;
> +
> + error = xfs_da_get_buf(tp, dp, xfs_dir2_db_to_da(mp, fbno),
> + -1, &bp, XFS_DATA_FORK);
> + if (error)
> + return error;
> +
> + bp->b_ops = &xfs_dir3_free_buf_ops;;
Extra ;
> +
> + /*
> + * Initialize the new block to be empty, and remember
> + * its first slot as our empty slot.
> + */
> + hdr.magic = XFS_DIR2_FREE_MAGIC;
> + hdr.firstdb = 0;
> + hdr.nused = 0;
> + hdr.nvalid = 0;
> + if (xfs_sb_version_hascrc(&mp->m_sb)) {
> + struct xfs_dir3_free_hdr *hdr3 = bp->b_addr;
> +
> + hdr.magic = XFS_DIR3_FREE_MAGIC;
> + hdr3->hdr.blkno = cpu_to_be64(bp->b_bn);
> + hdr3->hdr.owner = cpu_to_be64(dp->i_ino);
> + uuid_copy(&hdr3->hdr.uuid, &mp->m_sb.sb_uuid);
> +
Extra line
> + }
> + xfs_dir3_free_hdr_to_disk(bp->b_addr, &hdr);
> + *bpp = bp;
> + return 0;
> }
>
> /*
...
> @@ -909,59 +1059,68 @@ xfs_dir2_data_block_free(
> {
> struct xfs_trans *tp = args->trans;
> int logfree = 0;
> + __be16 *bests;
> + struct xfs_dir3_icfree_hdr freehdr;
Seems to be an extra line in this function here, although you don't see it added in the patch.
>
> - 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?
> - if (findex == be32_to_cpu(free->hdr.nvalid) - 1) {
> - int i; /* free entry index */
> + xfs_dir3_free_hdr_from_disk(&freehdr, free);
>
> - for (i = findex - 1; i >= 0; i--) {
> - if (free->bests[i] != cpu_to_be16(NULLDATAOFF))
> - break;
> - }
> - free->hdr.nvalid = cpu_to_be32(i + 1);
> - logfree = 0;
> - } else {
> - /* Not the last entry, just punch it out. */
> - free->bests[findex] = cpu_to_be16(NULLDATAOFF);
> - logfree = 1;
> - }
> + bests = xfs_dir3_free_bests_p(tp->t_mountp, free);
> + if (hdr) {
> /*
> - * If there are no useful entries left in the block,
> - * get rid of the block if we can.
> + * Data block is not empty, just set the free entry to the new
> + * value.
> */
> - if (!free->hdr.nused) {
> - int error;
> + bests[findex] = cpu_to_be16(longest);
> + xfs_dir2_free_log_bests(tp, fbp, findex, findex);
> + return 0;
> + }
>
> - error = xfs_dir2_shrink_inode(args, fdb, fbp);
> - if (error == 0) {
> - fbp = NULL;
> - logfree = 0;
> - } else if (error != ENOSPC || args->total != 0)
> - return error;
> - /*
> - * It's possible to get ENOSPC if there is no
> - * space reservation. In this case some one
> - * else will eventually get rid of this block.
> - */
> + /*
> + * One less used entry in the free table. Unused is not converted
> + * because we only need to know if it zero
> + */
> + freehdr.nused--;
> +
> + if (findex == freehdr.nvalid - 1) {
> + int i; /* free entry index */
> +
> + for (i = findex - 1; i >= 0; i--) {
> + if (bests[i] != cpu_to_be16(NULLDATAOFF))
> + break;
> }
> + freehdr.nvalid = i + 1;
> + logfree = 0;
> } else {
> + /* Not the last entry, just punch it out. */
> + bests[findex] = cpu_to_be16(NULLDATAOFF);
> + logfree = 1;
> + }
> +
> + xfs_dir3_free_hdr_to_disk(free, &freehdr);
> + xfs_dir2_free_log_header(tp, fbp);
> +
> + /*
> + * If there are no useful entries left in the block, get rid of the
> + * block if we can.
> + */
> + if (!freehdr.nused) {
> + int error;
> +
> + error = xfs_dir2_shrink_inode(args, fdb, fbp);
> + if (error == 0) {
> + fbp = NULL;
> + logfree = 0;
> + } else if (error != ENOSPC || args->total != 0)
> + return error;
> /*
> - * Data block is not empty, just set the free entry to the new
> - * value.
> + * It's possible to get ENOSPC if there is no
> + * space reservation. In this case some one
> + * else will eventually get rid of this block.
> */
> - free->bests[findex] = cpu_to_be16(longest);
> - logfree = 1;
> }
>
> +
Extra line.
> /* 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.
> @@ -1547,20 +1714,26 @@ xfs_dir2_node_addname_int(
> if (!fbp)
> continue;
> free = fbp->b_addr;
> - ASSERT(free->hdr.magic == cpu_to_be32(XFS_DIR2_FREE_MAGIC));
> findex = 0;
> }
> /*
> * Look at the current free entry. Is it good enough?
> + *
> + * The bests initialisation should be wher eteh bufer is read in
where the buffer
> + * the above branch. But gcc is too stupid to realise that bests
> + * iand the freehdr are actually initialised if they are placed
and
> + * there, so we have to do it here to avoid warnings. Blech.
> */
...
> @@ -1614,11 +1787,11 @@ xfs_dir2_node_addname_int(
> * If there wasn't a freespace block, the read will
> * return a NULL fbp. Allocate and initialize a new one.
> */
> - if( fbp == NULL ) {
> - if ((error = xfs_dir2_grow_inode(args, XFS_DIR2_FREE_SPACE,
> - &fbno))) {
> + if(!fbp) {
Add a space before the opening paren.
Looks good.
Regards,
Ben
_______________________________________________
xfs mailing list
xfs@xxxxxxxxxxx
http://oss.sgi.com/mailman/listinfo/xfs
