xfs
[Top] [All Lists]

Re: [PATCH 4/5] xfs: log file size updates as part of unwritten extent c

To: Christoph Hellwig <hch@xxxxxxxxxxxxx>
Subject: Re: [PATCH 4/5] xfs: log file size updates as part of unwritten extent conversion
From: Ben Myers <bpm@xxxxxxx>
Date: Thu, 10 Nov 2011 16:43:02 -0600
Cc: xfs@xxxxxxxxxxx
In-reply-to: <20111108085846.654476311@xxxxxxxxxxxxxxxxxxxxxx>
References: <20111108085614.478431403@xxxxxxxxxxxxxxxxxxxxxx> <20111108085846.654476311@xxxxxxxxxxxxxxxxxxxxxx>
User-agent: Mutt/1.5.18 (2008-05-17)
Hey Christoph,

On Tue, Nov 08, 2011 at 03:56:18AM -0500, Christoph Hellwig wrote:
> If we convert and unwritten extent past the current i_size log the size update
> as part of the extent manipulation transactions instead of doing an unlogged
> metadata update later.
> 
> Signed-off-by: Christoph Hellwig <hch@xxxxxx>
> 
> ---
>  fs/xfs/xfs_aops.c  |   11 ++++++-----
>  fs/xfs/xfs_iomap.c |   19 ++++++++++++++++++-
>  2 files changed, 24 insertions(+), 6 deletions(-)
> 
> Index: linux-2.6/fs/xfs/xfs_iomap.c
> ===================================================================
> --- linux-2.6.orig/fs/xfs/xfs_iomap.c 2011-11-08 08:02:50.234386118 +0100
> +++ linux-2.6/fs/xfs/xfs_iomap.c      2011-11-08 08:14:04.319888994 +0100
> @@ -31,6 +31,7 @@
>  #include "xfs_ialloc_btree.h"
>  #include "xfs_dinode.h"
>  #include "xfs_inode.h"
> +#include "xfs_inode_item.h"
>  #include "xfs_btree.h"
>  #include "xfs_bmap.h"
>  #include "xfs_rtalloc.h"
> @@ -645,6 +646,7 @@ xfs_iomap_write_unwritten(
>       xfs_trans_t     *tp;
>       xfs_bmbt_irec_t imap;
>       xfs_bmap_free_t free_list;
> +     xfs_fsize_t     i_size;
>       uint            resblks;
>       int             committed;
>       int             error;
> @@ -705,7 +707,22 @@ xfs_iomap_write_unwritten(
>               if (error)
>                       goto error_on_bmapi_transaction;
>  
> -             error = xfs_bmap_finish(&(tp), &(free_list), &committed);
> +             /*
> +              * Log the updated inode size as we go.  We have to be careful
> +              * to only log it up to the actual write offset if it is
> +              * halfway into a block.
> +              */
> +             i_size = XFS_FSB_TO_B(mp, offset_fsb + count_fsb);
> +             if (i_size > offset + count)
> +                     i_size = offset + count;
> +

I'm missing something here.  Why wouldn't you just 

i_size = xfs_new_eof(ip, offset + count);

It's not clear to me why offset_fsb and count_fsb should be involved in
this calculation.  Can you enlighten me?

Thanks,
        Ben

> +             i_size = xfs_new_eof(ip, i_size);
> +             if (i_size) {
> +                     ip->i_d.di_size = i_size;
> +                     xfs_trans_log_inode(tp, ip, XFS_ILOG_CORE);
> +             }
> +
> +             error = xfs_bmap_finish(&tp, &free_list, &committed);
>               if (error)
>                       goto error_on_bmapi_transaction;
>  
> Index: linux-2.6/fs/xfs/xfs_aops.c
> ===================================================================
> --- linux-2.6.orig/fs/xfs/xfs_aops.c  2011-11-08 08:12:31.586400976 +0100
> +++ linux-2.6/fs/xfs/xfs_aops.c       2011-11-08 08:14:04.319888994 +0100
> @@ -183,13 +183,14 @@ xfs_end_io(
>                       ioend->io_error = -error;
>                       goto done;
>               }
> +     } else {
> +             /*
> +              * We might have to update the on-disk file size after
> +              * extending writes.
> +              */
> +             xfs_setfilesize(ioend);
>       }
>  
> -     /*
> -      * We might have to update the on-disk file size after extending
> -      * writes.
> -      */
> -     xfs_setfilesize(ioend);
>  done:
>       xfs_destroy_ioend(ioend);
>  }
> 
> _______________________________________________
> xfs mailing list
> xfs@xxxxxxxxxxx
> http://oss.sgi.com/mailman/listinfo/xfs

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