xfs
[Top] [All Lists]

Re: [PATCH 08/25] xfs: introduce xfs_bmapi_delay()

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 08/25] xfs: introduce xfs_bmapi_delay()
From: Alex Elder <aelder@xxxxxxx>
Date: Fri, 2 Sep 2011 17:23:58 -0500
Cc: <xfs@xxxxxxxxxxx>
In-reply-to: <20110824060641.979845427@xxxxxxxxxxxxxxxxxxxxxx>
References: <20110824060428.789245205@xxxxxxxxxxxxxxxxxxxxxx> <20110824060641.979845427@xxxxxxxxxxxxxxxxxxxxxx>
Reply-to: <aelder@xxxxxxx>
On Wed, 2011-08-24 at 02:04 -0400, Christoph Hellwig wrote:
> plain text document attachment (xfs-bmapi-add-xfs_bmapi_delay)
> Delalloc reservations are much simpler than allocations, so give them a
> separate bmapi-level interface.  Using the previously added
> xfs_bmapi_reserve_delalloc we get a function that is only minimally
> more complicated than xfs_bmapi_read, which is far from the complexity
> in xfs_bmapi.  Also remove the XFS_BMAPI_DELAY code after switching
> over the only user to xfs_bmapi_delay.
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>

I have a minor suggestion but it's not worth
rejecting this over.  Also a question.  But
regardless, this looks good:

Reviewed-by: Alex Elder <aelder@xxxxxxx>

I'm out of time today.  I will have to continue
later.

> Index: xfs/fs/xfs/xfs_bmap.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_bmap.c        2011-08-23 21:12:56.272619118 +0200
> +++ xfs/fs/xfs/xfs_bmap.c     2011-08-23 21:12:59.355935746 +0200
> @@ -4238,7 +4238,7 @@ xfs_bmap_validate_ret(
>               ASSERT(i == 0 ||
>                      mval[i - 1].br_startoff + mval[i - 1].br_blockcount ==
>                      mval[i].br_startoff);
> -             if ((flags & XFS_BMAPI_WRITE) && !(flags & XFS_BMAPI_DELAY))
> +             if (flags & XFS_BMAPI_WRITE)
>                       ASSERT(mval[i].br_startblock != DELAYSTARTBLOCK &&
>                              mval[i].br_startblock != HOLESTARTBLOCK);
>               ASSERT(mval[i].br_state == XFS_EXT_NORM ||
> @@ -4553,6 +4553,90 @@ out_unreserve_quota:
>  }
>  
>  /*
> + * Map file blocks to filesystem blocks, adding delayed allocations as 
> needed.
> + */
> +int
> +xfs_bmapi_delay(
> +     struct xfs_inode        *ip,    /* incore inode */
> +     xfs_fileoff_t           bno,    /* starting file offs. mapped */
> +     xfs_filblks_t           len,    /* length to map in file */
> +     struct xfs_bmbt_irec    *mval,  /* output: map values */
> +     int                     *nmap,  /* i/o: mval size/count */
> +     int                     flags)  /* XFS_BMAPI_... */
> +{
> +     struct xfs_mount        *mp = ip->i_mount;
> +     struct xfs_ifork        *ifp = XFS_IFORK_PTR(ip, XFS_DATA_FORK);
> +     struct xfs_bmbt_irec    got;    /* current file extent record */
> +     struct xfs_bmbt_irec    prev;   /* previous file extent record */
> +     xfs_fileoff_t           obno;   /* old block number (offset) */
> +     xfs_fileoff_t           end;    /* end of mapped file region */
> +     xfs_extnum_t            lastx;  /* last useful extent number */
> +     int                     eof;    /* we've hit the end of extents */
> +     int                     n = 0;  /* current extent index */
> +     int                     error = 0;
> +
> +     ASSERT(*nmap >= 1);
> +     ASSERT(*nmap <= XFS_BMAP_MAX_NMAP);
> +     ASSERT(!(flags & ~XFS_BMAPI_ENTIRE));
> +

Rearrange the following test to use the pattern (assigning error)
used in xfs_bmapi_read().

> +     if (unlikely(XFS_TEST_ERROR(
> +         (XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_EXTENTS &&
> +          XFS_IFORK_FORMAT(ip, XFS_DATA_FORK) != XFS_DINODE_FMT_BTREE),

Are you certain that XFS_DINODE_FMT_LOCAL is not possible here?
I tried to trace it back but I'm still not sure.  The transaction
pointer passed is null, so it would have tripped an assertion
in the previous code.  (A simple explanation would reassure me.)

> +          mp, XFS_ERRTAG_BMAPIFORMAT, XFS_RANDOM_BMAPIFORMAT))) {
> +             XFS_ERROR_REPORT("xfs_bmapi_delay", XFS_ERRLEVEL_LOW, mp);
> +             return XFS_ERROR(EFSCORRUPTED);
> +     }
> +
> +     if (XFS_FORCED_SHUTDOWN(mp))
> +             return XFS_ERROR(EIO);
> +
> +     XFS_STATS_INC(xs_blk_mapw);
> +
> +
. . .

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