On Thu, Nov 17, 2016 at 01:11:54PM -0500, Brian Foster wrote:
> On Mon, Nov 14, 2016 at 06:12:33PM +0100, Christoph Hellwig wrote:
> > Rewrite the function using xfs_iext_lookup_extent and xfs_iext_get_extent,
> > and massage the flow into something easily understandable.
> >
> > Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> > ---
> > fs/xfs/libxfs/xfs_bmap.c | 64 ++++++++++++++++++++++++------------------------
> > 1 file changed, 32 insertions(+), 32 deletions(-)
> >
> > diff --git a/fs/xfs/libxfs/xfs_bmap.c b/fs/xfs/libxfs/xfs_bmap.c
> > index 5c3c4dd..98f490b 100644
> > --- a/fs/xfs/libxfs/xfs_bmap.c
> > +++ b/fs/xfs/libxfs/xfs_bmap.c
> > @@ -1523,44 +1523,44 @@ xfs_bmap_first_unused(
> > */
> > int /* error */
> > xfs_bmap_last_before(
> > - xfs_trans_t *tp, /* transaction pointer */
> > - xfs_inode_t *ip, /* incore inode */
> > - xfs_fileoff_t *last_block, /* last block */
> > - int whichfork) /* data or attr fork */
> > + struct xfs_trans *tp, /* transaction pointer */
> > + struct xfs_inode *ip, /* incore inode */
> > + xfs_fileoff_t *last_block, /* last block */
> > + int whichfork) /* data or attr fork */
> > {
> > - xfs_fileoff_t bno; /* input file offset */
> > - int eof; /* hit end of file */
> > - xfs_bmbt_rec_host_t *ep; /* pointer to last extent */
> > - int error; /* error return value */
> > - xfs_bmbt_irec_t got; /* current extent value */
> > - xfs_ifork_t *ifp; /* inode fork pointer */
> > - xfs_extnum_t lastx; /* last extent used */
> > - xfs_bmbt_irec_t prev; /* previous extent value */
> > + struct xfs_ifork *ifp = XFS_IFORK_PTR(ip, whichfork);
> > + struct xfs_bmbt_irec got;
> > + xfs_extnum_t idx;
> > + int error;
> >
> > - if (XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_BTREE &&
> > - XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_EXTENTS &&
> > - XFS_IFORK_FORMAT(ip, whichfork) != XFS_DINODE_FMT_LOCAL)
> > - return -EIO;
> > - if (XFS_IFORK_FORMAT(ip, whichfork) == XFS_DINODE_FMT_LOCAL) {
> > + switch (XFS_IFORK_FORMAT(ip, whichfork)) {
> > + case XFS_DINODE_FMT_LOCAL:
> > *last_block = 0;
> > return 0;
> > + case XFS_DINODE_FMT_BTREE:
> > + case XFS_DINODE_FMT_EXTENTS:
> > + break;
> > + default:
> > + return -EIO;
> > }
> > - ifp = XFS_IFORK_PTR(ip, whichfork);
> > - if (!(ifp->if_flags & XFS_IFEXTENTS) &&
> > - (error = xfs_iread_extents(tp, ip, whichfork)))
> > - return error;
> > - bno = *last_block - 1;
> > - ep = xfs_bmap_search_extents(ip, bno, whichfork, &eof, &lastx, &got,
> > - &prev);
> > - if (eof || xfs_bmbt_get_startoff(ep) > bno) {
> > - if (prev.br_startoff == NULLFILEOFF)
> > - *last_block = 0;
> > - else
> > - *last_block = prev.br_startoff + prev.br_blockcount;
> > +
> > + if (!(ifp->if_flags & XFS_IFEXTENTS)) {
> > + error = xfs_iread_extents(tp, ip, whichfork);
> > + if (error)
> > + return error;
> > }
> > - /*
> > - * Otherwise *last_block is already the right answer.
> > - */
> > +
> > + if (xfs_iext_lookup_extent(ip, ifp, *last_block - 1, &idx, &got)) {
> > + if (got.br_startoff <= *last_block - 1)
> > + return 0;
> > + }
> > +
> > + if (xfs_iext_get_extent(ifp, idx - 1, &got)) {
>
> It may not be an issue in current usage, but I think we should check for
> idx here (i.e., 'if (idx && xfs_iext_get_extent(..))').
I've designed xfs_iext_get_extent to gracefully handle invalid indices
(including negative ones) and return NULL.
case.
--
To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
