xfs
[Top] [All Lists]

Re: [PATCH 06/11] xfs: kill struct xfs_iomap

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 06/11] xfs: kill struct xfs_iomap
From: Alex Elder <aelder@xxxxxxx>
Date: Thu, 29 Apr 2010 11:27:18 -0500
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20100428123015.526545338@xxxxxxxxxxxxxxxxxxxxxx>
References: <20100428122850.075189557@xxxxxxxxxxxxxxxxxxxxxx> <20100428123015.526545338@xxxxxxxxxxxxxxxxxxxxxx>
Reply-to: aelder@xxxxxxx
On Wed, 2010-04-28 at 08:28 -0400, Christoph Hellwig wrote:
> plain text document attachment (xfs-iomap-cleanup-6)
> Now that struct xfs_iomap contains exactly the same units as
> struct xfs_bmbt_irec we can just use the latter directly in the aops
> code.  Replace the missing IOMAP_NEW flag with a new boolean output
> parameter to xfs_iomap.
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>

Looks good.  My only suggestion was going to be having xfs_iomap()
support passing a null pointer for "new" if it's a don't-care in
the caller, but in the end it's called in only two spots and the
result would not be an improvement.

Reviewed-by: Alex Elder <aelder@xxxxxxx>

> Index: xfs/fs/xfs/linux-2.6/xfs_aops.c
> ===================================================================
> --- xfs.orig/fs/xfs/linux-2.6/xfs_aops.c      2010-04-27 17:39:20.699003542 
> +0200
> +++ xfs/fs/xfs/linux-2.6/xfs_aops.c   2010-04-27 18:12:14.223025612 +0200
> @@ -309,23 +309,24 @@ xfs_map_blocks(
>       struct inode            *inode,
>       loff_t                  offset,
>       ssize_t                 count,
> -     xfs_iomap_t             *mapp,
> +     struct xfs_bmbt_irec    *imap,
>       int                     flags)
>  {
>       int                     nmaps = 1;
> +     int                     new = 0;
>  
> -     return -xfs_iomap(XFS_I(inode), offset, count, flags, mapp, &nmaps);
> +     return -xfs_iomap(XFS_I(inode), offset, count, flags, imap, &nmaps, 
> &new);
>  }
>  
>  STATIC int
>  xfs_iomap_valid(
>       struct inode            *inode,
> -     xfs_iomap_t             *iomapp,
> +     struct xfs_bmbt_irec    *imap,
>       loff_t                  offset)
>  {
>       struct xfs_mount        *mp = XFS_I(inode)->i_mount;
> -     xfs_off_t               iomap_offset = XFS_FSB_TO_B(mp, 
> iomapp->iomap_offset);
> -     xfs_off_t               iomap_bsize = XFS_FSB_TO_B(mp, 
> iomapp->iomap_bsize);
> +     xfs_off_t               iomap_offset = XFS_FSB_TO_B(mp, 
> imap->br_startoff);
> +     xfs_off_t               iomap_bsize = XFS_FSB_TO_B(mp, 
> imap->br_blockcount);
>  
>       return offset >= iomap_offset &&
>               offset < iomap_offset + iomap_bsize;
> @@ -561,16 +562,16 @@ STATIC void
>  xfs_map_buffer(
>       struct inode            *inode,
>       struct buffer_head      *bh,
> -     xfs_iomap_t             *mp,
> +     struct xfs_bmbt_irec    *imap,
>       xfs_off_t               offset)
>  {
>       sector_t                bn;
>       struct xfs_mount        *m = XFS_I(inode)->i_mount;
> -     xfs_off_t               iomap_offset = XFS_FSB_TO_B(m, 
> mp->iomap_offset);
> -     xfs_daddr_t             iomap_bn = xfs_fsb_to_db(XFS_I(inode), 
> mp->iomap_bn);
> +     xfs_off_t               iomap_offset = XFS_FSB_TO_B(m, 
> imap->br_startoff);
> +     xfs_daddr_t             iomap_bn = xfs_fsb_to_db(XFS_I(inode), 
> imap->br_startblock);
>  
> -     ASSERT(mp->iomap_bn != HOLESTARTBLOCK);
> -     ASSERT(mp->iomap_bn != DELAYSTARTBLOCK);
> +     ASSERT(imap->br_startblock != HOLESTARTBLOCK);
> +     ASSERT(imap->br_startblock != DELAYSTARTBLOCK);
>  
>       bn = (iomap_bn >> (inode->i_blkbits - BBSHIFT)) +
>             ((offset - iomap_offset) >> inode->i_blkbits);
> @@ -585,14 +586,14 @@ STATIC void
>  xfs_map_at_offset(
>       struct inode            *inode,
>       struct buffer_head      *bh,
> -     xfs_iomap_t             *iomapp,
> +     struct xfs_bmbt_irec    *imap,
>       xfs_off_t               offset)
>  {
> -     ASSERT(iomapp->iomap_bn != HOLESTARTBLOCK);
> -     ASSERT(iomapp->iomap_bn != DELAYSTARTBLOCK);
> +     ASSERT(imap->br_startblock != HOLESTARTBLOCK);
> +     ASSERT(imap->br_startblock != DELAYSTARTBLOCK);
>  
>       lock_buffer(bh);
> -     xfs_map_buffer(inode, bh, iomapp, offset);
> +     xfs_map_buffer(inode, bh, imap, offset);
>       bh->b_bdev = xfs_find_bdev_for_inode(inode);
>       set_buffer_mapped(bh);
>       clear_buffer_delay(bh);
> @@ -749,7 +750,7 @@ xfs_convert_page(
>       struct inode            *inode,
>       struct page             *page,
>       loff_t                  tindex,
> -     xfs_iomap_t             *mp,
> +     struct xfs_bmbt_irec    *imap,
>       xfs_ioend_t             **ioendp,
>       struct writeback_control *wbc,
>       int                     startio,
> @@ -814,15 +815,15 @@ xfs_convert_page(
>                       else
>                               type = IOMAP_DELAY;
>  
> -                     if (!xfs_iomap_valid(inode, mp, offset)) {
> +                     if (!xfs_iomap_valid(inode, imap, offset)) {
>                               done = 1;
>                               continue;
>                       }
>  
> -                     ASSERT(mp->iomap_bn != HOLESTARTBLOCK);
> -                     ASSERT(mp->iomap_bn != DELAYSTARTBLOCK);
> +                     ASSERT(imap->br_startblock != HOLESTARTBLOCK);
> +                     ASSERT(imap->br_startblock != DELAYSTARTBLOCK);
>  
> -                     xfs_map_at_offset(inode, bh, mp, offset);
> +                     xfs_map_at_offset(inode, bh, imap, offset);
>                       if (startio) {
>                               xfs_add_to_ioend(inode, bh, offset,
>                                               type, ioendp, done);
> @@ -874,7 +875,7 @@ STATIC void
>  xfs_cluster_write(
>       struct inode            *inode,
>       pgoff_t                 tindex,
> -     xfs_iomap_t             *iomapp,
> +     struct xfs_bmbt_irec    *imap,
>       xfs_ioend_t             **ioendp,
>       struct writeback_control *wbc,
>       int                     startio,
> @@ -893,7 +894,7 @@ xfs_cluster_write(
>  
>               for (i = 0; i < pagevec_count(&pvec); i++) {
>                       done = xfs_convert_page(inode, pvec.pages[i], tindex++,
> -                                     iomapp, ioendp, wbc, startio, all_bh);
> +                                     imap, ioendp, wbc, startio, all_bh);
>                       if (done)
>                               break;
>               }
> @@ -1050,7 +1051,7 @@ xfs_page_state_convert(
>       int             unmapped) /* also implies page uptodate */
>  {
>       struct buffer_head      *bh, *head;
> -     xfs_iomap_t             iomap;
> +     struct xfs_bmbt_irec    imap;
>       xfs_ioend_t             *ioend = NULL, *iohead = NULL;
>       loff_t                  offset;
>       unsigned long           p_offset = 0;
> @@ -1124,7 +1125,7 @@ xfs_page_state_convert(
>               }
>  
>               if (iomap_valid)
> -                     iomap_valid = xfs_iomap_valid(inode, &iomap, offset);
> +                     iomap_valid = xfs_iomap_valid(inode, &imap, offset);
>  
>               /*
>                * First case, map an unwritten extent and prepare for
> @@ -1176,13 +1177,13 @@ xfs_page_state_convert(
>                               }
>  
>                               err = xfs_map_blocks(inode, offset, size,
> -                                             &iomap, flags);
> +                                             &imap, flags);
>                               if (err)
>                                       goto error;
> -                             iomap_valid = xfs_iomap_valid(inode, &iomap, 
> offset);
> +                             iomap_valid = xfs_iomap_valid(inode, &imap, 
> offset);
>                       }
>                       if (iomap_valid) {
> -                             xfs_map_at_offset(inode, bh, &iomap, offset);
> +                             xfs_map_at_offset(inode, bh, &imap, offset);
>                               if (startio) {
>                                       xfs_add_to_ioend(inode, bh, offset,
>                                                       type, &ioend,
> @@ -1206,10 +1207,10 @@ xfs_page_state_convert(
>                               size = xfs_probe_cluster(inode, page, bh,
>                                                               head, 1);
>                               err = xfs_map_blocks(inode, offset, size,
> -                                             &iomap, flags);
> +                                             &imap, flags);
>                               if (err)
>                                       goto error;
> -                             iomap_valid = xfs_iomap_valid(inode, &iomap, 
> offset);
> +                             iomap_valid = xfs_iomap_valid(inode, &imap, 
> offset);
>                       }
>  
>                       /*
> @@ -1250,13 +1251,13 @@ xfs_page_state_convert(
>  
>       if (ioend && iomap_valid) {
>               struct xfs_mount        *m = XFS_I(inode)->i_mount;
> -             xfs_off_t               iomap_offset = XFS_FSB_TO_B(m, 
> iomap.iomap_offset);
> -             xfs_off_t               iomap_bsize = XFS_FSB_TO_B(m, 
> iomap.iomap_bsize);
> +             xfs_off_t               iomap_offset = XFS_FSB_TO_B(m, 
> imap.br_startoff);
> +             xfs_off_t               iomap_bsize = XFS_FSB_TO_B(m, 
> imap.br_blockcount);
>  
>               offset = (iomap_offset + iomap_bsize - 1) >>
>                                       PAGE_CACHE_SHIFT;
>               tlast = min_t(pgoff_t, offset, last_index);
> -             xfs_cluster_write(inode, page->index + 1, &iomap, &ioend,
> +             xfs_cluster_write(inode, page->index + 1, &imap, &ioend,
>                                       wbc, startio, all_bh, tlast);
>       }
>  
> @@ -1459,10 +1460,11 @@ __xfs_get_blocks(
>       int                     direct,
>       bmapi_flags_t           flags)
>  {
> -     xfs_iomap_t             iomap;
> +     struct xfs_bmbt_irec    imap;
>       xfs_off_t               offset;
>       ssize_t                 size;
> -     int                     niomap = 1;
> +     int                     nimap = 1;
> +     int                     new = 0;
>       int                     error;
>  
>       offset = (xfs_off_t)iblock << inode->i_blkbits;
> @@ -1473,21 +1475,21 @@ __xfs_get_blocks(
>               return 0;
>  
>       error = xfs_iomap(XFS_I(inode), offset, size,
> -                          create ? flags : BMAPI_READ, &iomap, &niomap);
> +                          create ? flags : BMAPI_READ, &imap, &nimap, &new);
>       if (error)
>               return -error;
> -     if (niomap == 0)
> +     if (nimap == 0)
>               return 0;
>  
> -     if (iomap.iomap_bn != HOLESTARTBLOCK &&
> -         iomap.iomap_bn != DELAYSTARTBLOCK) {
> +     if (imap.br_startblock != HOLESTARTBLOCK &&
> +         imap.br_startblock != DELAYSTARTBLOCK) {
>               /*
>                * For unwritten extents do not report a disk address on
>                * the read case (treat as if we're reading into a hole).
>                */
> -             if (create || !(iomap.iomap_flags & IOMAP_UNWRITTEN))
> -                     xfs_map_buffer(inode, bh_result, &iomap, offset);
> -             if (create && (iomap.iomap_flags & IOMAP_UNWRITTEN)) {
> +             if (create || !ISUNWRITTEN(&imap))
> +                     xfs_map_buffer(inode, bh_result, &imap, offset);
> +             if (create && ISUNWRITTEN(&imap)) {
>                       if (direct)
>                               bh_result->b_private = inode;
>                       set_buffer_unwritten(bh_result);
> @@ -1512,10 +1514,10 @@ __xfs_get_blocks(
>       if (create &&
>           ((!buffer_mapped(bh_result) && !buffer_uptodate(bh_result)) ||
>            (offset >= i_size_read(inode)) ||
> -          (iomap.iomap_flags & (IOMAP_NEW|IOMAP_UNWRITTEN))))
> +          (new || ISUNWRITTEN(&imap))))
>               set_buffer_new(bh_result);
>  
> -     if (iomap.iomap_bn == DELAYSTARTBLOCK) {
> +     if (imap.br_startblock == DELAYSTARTBLOCK) {
>               BUG_ON(direct);
>               if (create) {
>                       set_buffer_uptodate(bh_result);
> @@ -1526,9 +1528,9 @@ __xfs_get_blocks(
>  
>       if (direct || size > (1 << inode->i_blkbits)) {
>               struct xfs_mount        *mp = XFS_I(inode)->i_mount;
> -             xfs_off_t               iomap_offset = XFS_FSB_TO_B(mp, 
> iomap.iomap_offset);
> +             xfs_off_t               iomap_offset = XFS_FSB_TO_B(mp, 
> imap.br_startoff);
>               xfs_off_t               iomap_delta = offset - iomap_offset;
> -             xfs_off_t               iomap_bsize = XFS_FSB_TO_B(mp, 
> iomap.iomap_bsize);
> +             xfs_off_t               iomap_bsize = XFS_FSB_TO_B(mp, 
> imap.br_blockcount);
>  
>               ASSERT(iomap_bsize - iomap_delta > 0);
>               offset = min_t(xfs_off_t,
> Index: xfs/fs/xfs/xfs_iomap.c
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_iomap.c       2010-04-27 17:39:20.700004101 +0200
> +++ xfs/fs/xfs/xfs_iomap.c    2010-04-27 18:12:21.135027288 +0200
> @@ -55,46 +55,25 @@
>  #define XFS_STRAT_WRITE_IMAPS        2
>  #define XFS_WRITE_IMAPS              XFS_BMAP_MAX_NMAP
>  
> -STATIC void
> -xfs_imap_to_bmap(
> -     xfs_inode_t     *ip,
> -     xfs_off_t       offset,
> -     xfs_bmbt_irec_t *imap,
> -     xfs_iomap_t     *iomapp,
> -     int             imaps,                  /* Number of imap entries */
> -     int             flags)
> -{
> -     iomapp->iomap_offset = imap->br_startoff;
> -     iomapp->iomap_bsize = imap->br_blockcount;
> -     iomapp->iomap_flags = flags;
> -     iomapp->iomap_bn = imap->br_startblock;
> -
> -     if (imap->br_startblock != HOLESTARTBLOCK &&
> -         imap->br_startblock != DELAYSTARTBLOCK &&
> -         ISUNWRITTEN(imap))
> -             iomapp->iomap_flags |= IOMAP_UNWRITTEN;
> -}
> -
>  int
>  xfs_iomap(
> -     xfs_inode_t     *ip,
> -     xfs_off_t       offset,
> -     ssize_t         count,
> -     int             flags,
> -     xfs_iomap_t     *iomapp,
> -     int             *niomaps)
> +     struct xfs_inode        *ip,
> +     xfs_off_t               offset,
> +     ssize_t                 count,
> +     int                     flags,
> +     struct xfs_bmbt_irec    *imap,
> +     int                     *nimaps,
> +     int                     *new)
>  {
> -     xfs_mount_t     *mp = ip->i_mount;
> -     xfs_fileoff_t   offset_fsb, end_fsb;
> -     int             error = 0;
> -     int             lockmode = 0;
> -     xfs_bmbt_irec_t imap;
> -     int             nimaps = 1;
> -     int             bmapi_flags = 0;
> -     int             iomap_flags = 0;
> +     struct xfs_mount        *mp = ip->i_mount;
> +     xfs_fileoff_t           offset_fsb, end_fsb;
> +     int                     error = 0;
> +     int                     lockmode = 0;
> +     int                     bmapi_flags = 0;
>  
>       ASSERT((ip->i_d.di_mode & S_IFMT) == S_IFREG);
> -     ASSERT(niomaps && *niomaps == 1);
> +
> +     *new = 0;
>  
>       if (XFS_FORCED_SHUTDOWN(mp))
>               return XFS_ERROR(EIO);
> @@ -136,8 +115,8 @@ xfs_iomap(
>  
>       error = xfs_bmapi(NULL, ip, offset_fsb,
>                       (xfs_filblks_t)(end_fsb - offset_fsb),
> -                     bmapi_flags,  NULL, 0, &imap,
> -                     &nimaps, NULL, NULL);
> +                     bmapi_flags,  NULL, 0, imap,
> +                     nimaps, NULL, NULL);
>  
>       if (error)
>               goto out;
> @@ -145,45 +124,41 @@ xfs_iomap(
>       switch (flags & (BMAPI_WRITE|BMAPI_ALLOCATE)) {
>       case BMAPI_WRITE:
>               /* If we found an extent, return it */
> -             if (nimaps &&
> -                 (imap.br_startblock != HOLESTARTBLOCK) &&
> -                 (imap.br_startblock != DELAYSTARTBLOCK)) {
> -                     trace_xfs_iomap_found(ip, offset, count, flags, &imap);
> +             if (*nimaps &&
> +                 (imap->br_startblock != HOLESTARTBLOCK) &&
> +                 (imap->br_startblock != DELAYSTARTBLOCK)) {
> +                     trace_xfs_iomap_found(ip, offset, count, flags, imap);
>                       break;
>               }
>  
>               if (flags & (BMAPI_DIRECT|BMAPI_MMAP)) {
>                       error = xfs_iomap_write_direct(ip, offset, count, flags,
> -                                                    &imap, &nimaps, nimaps);
> +                                                    imap, nimaps, *nimaps);
>               } else {
>                       error = xfs_iomap_write_delay(ip, offset, count, flags,
> -                                                   &imap, &nimaps);
> +                                                   imap, nimaps);
>               }
>               if (!error) {
> -                     trace_xfs_iomap_alloc(ip, offset, count, flags, &imap);
> +                     trace_xfs_iomap_alloc(ip, offset, count, flags, imap);
>               }
> -             iomap_flags = IOMAP_NEW;
> +             *new = 1;
>               break;
>       case BMAPI_ALLOCATE:
>               /* If we found an extent, return it */
>               xfs_iunlock(ip, lockmode);
>               lockmode = 0;
>  
> -             if (nimaps && !isnullstartblock(imap.br_startblock)) {
> -                     trace_xfs_iomap_found(ip, offset, count, flags, &imap);
> +             if (*nimaps && !isnullstartblock(imap->br_startblock)) {
> +                     trace_xfs_iomap_found(ip, offset, count, flags, imap);
>                       break;
>               }
>  
>               error = xfs_iomap_write_allocate(ip, offset, count,
> -                                              &imap, &nimaps);
> +                                              imap, nimaps);
>               break;
>       }
>  
> -     ASSERT(nimaps <= 1);
> -
> -     if (nimaps)
> -             xfs_imap_to_bmap(ip, offset, &imap, iomapp, nimaps, 
> iomap_flags);
> -     *niomaps = nimaps;
> +     ASSERT(*nimaps <= 1);
>  
>  out:
>       if (lockmode)
> @@ -191,7 +166,6 @@ out:
>       return XFS_ERROR(error);
>  }
>  
> -
>  STATIC int
>  xfs_iomap_eof_align_last_fsb(
>       xfs_mount_t     *mp,
> Index: xfs/fs/xfs/xfs_iomap.h
> ===================================================================
> --- xfs.orig/fs/xfs/xfs_iomap.h       2010-04-27 17:39:27.783067167 +0200
> +++ xfs/fs/xfs/xfs_iomap.h    2010-04-27 18:12:14.232003612 +0200
> @@ -47,35 +47,11 @@ typedef enum {
>       { BMAPI_MMAP,           "MMAP" }, \
>       { BMAPI_TRYLOCK,        "TRYLOCK" }
>  
> -/*
> - * xfs_iomap_t:  File system I/O map
> - *
> - * The iomap_bn field is expressed in 512-byte blocks, and is where the
> - * mapping starts on disk.
> - *
> - * The iomap_offset, iomap_bsize and iomap_delta fields are in bytes.
> - * iomap_offset is the offset of the mapping in the file itself.
> - * iomap_bsize is the size of the mapping,  iomap_delta is the
> - * desired data's offset into the mapping, given the offset supplied
> - * to the file I/O map routine.
> - *
> - * When a request is made to read beyond the logical end of the object,
> - * iomap_size may be set to 0, but iomap_offset and iomap_length should be 
> set
> - * to the actual amount of underlying storage that has been allocated, if 
> any.
> - */
> -
> -typedef struct xfs_iomap {
> -     xfs_daddr_t             iomap_bn;       /* first 512B blk of mapping */
> -     xfs_off_t               iomap_offset;   /* offset of mapping, bytes */
> -     xfs_off_t               iomap_bsize;    /* size of mapping, bytes */
> -     iomap_flags_t           iomap_flags;
> -} xfs_iomap_t;
> -
>  struct xfs_inode;
>  struct xfs_bmbt_irec;
>  
>  extern int xfs_iomap(struct xfs_inode *, xfs_off_t, ssize_t, int,
> -                  struct xfs_iomap *, int *);
> +                  struct xfs_bmbt_irec *, int *, int *);
>  extern int xfs_iomap_write_direct(struct xfs_inode *, xfs_off_t, size_t,
>                                 int, struct xfs_bmbt_irec *, int *, int);
>  extern int xfs_iomap_write_delay(struct xfs_inode *, xfs_off_t, size_t, int,
> 
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs



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