Re: [PATCH 4/4] xfs: rewrite and optimize the delalloc write path

On Fri, Aug 26, 2016 at 12:07:53PM -0400, Brian Foster wrote:
> On Fri, Aug 26, 2016 at 12:03:39PM -0400, Brian Foster wrote:
> > On Fri, Aug 26, 2016 at 04:33:44PM +0200, Christoph Hellwig wrote:
> > > On Thu, Aug 25, 2016 at 10:37:09AM -0400, Brian Foster wrote:
> ...
> > > (we could potentially re-derive offset_fsb from offset if we don't
> > > mind the inefficieny and recalculate maxbytes_fsb.  This already
> > > assumes mp is trivially derived from ip)
> > > 
> > > and return
> > > 
> > > alloc_blocks, end_fsb
> > > 
> > > so the function would be quite a monster in terms of its calling
> > > convention.  Additionally we'd have the related by not qute the
> > > same if blocks around XFS_MOUNT_DFLT_IOSIZE and the isize split
> > > over two functions, which doesn't exactly help understanding
> > > the flow.
> > > 
> > 
> > Not quite sure I follow the last bit, but I don't necessarily think the
> > whole thing has to be boxed into a helper to clean it up. E.g., I'd do
> > something like the appended diff (compile tested only).
> > 
> 
> ... and if the function signature is really an issue, trade off idx &
> prev for a conditional base preallocation size (applies on top of the
> previous diff):
> 
> diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
> index 45e5268..c748429 100644
> --- a/fs/xfs/xfs_iomap.c
> +++ b/fs/xfs/xfs_iomap.c
> @@ -381,7 +381,7 @@ STATIC xfs_fsblock_t
>  xfs_iomap_prealloc_size(
>  	struct xfs_inode	*ip,
>  	xfs_off_t		offset,
> -	struct xfs_bmbt_irec	*prev)
> +	xfs_extlen_t		base)
>  {
>  	struct xfs_mount	*mp = ip->i_mount;
>  	int			shift = 0;
> @@ -406,8 +406,8 @@ xfs_iomap_prealloc_size(
>  	 * always extends to MAXEXTLEN rather than falling short due to things
>  	 * like stripe unit/width alignment of real extents.
>  	 */
> -	if (prev->br_blockcount <= (MAXEXTLEN >> 1))
> -		alloc_blocks = prev->br_blockcount << 1;
> +	if (base <= (MAXEXTLEN >> 1))
> +		alloc_blocks = base << 1;
>  	else
>  		alloc_blocks = XFS_B_TO_FSB(mp, offset);
>  	if (!alloc_blocks)
> @@ -506,12 +506,10 @@ xfs_iomap_prealloc(
>  	struct xfs_inode	*ip,
>  	loff_t			offset,
>  	loff_t			count,
> -	xfs_extnum_t		idx,
> -	struct xfs_bmbt_irec	*prev)
> +	xfs_extlen_t		base)
>  {
>  	struct xfs_mount	*mp = ip->i_mount;
>  	xfs_fsblock_t		alloc_blocks;
> -	xfs_fileoff_t		offset_fsb = XFS_B_TO_FSBT(mp, offset);
>  
>  	if (offset + count <= XFS_ISIZE(ip))
>  		return 0;
> @@ -526,12 +524,11 @@ xfs_iomap_prealloc(
>  	 */
>  	if ((mp->m_flags & XFS_MOUNT_DFLT_IOSIZE) ||
>  	    XFS_ISIZE(ip) < XFS_FSB_TO_B(mp, mp->m_dalign) ||
> -	    idx == 0 ||
> -	    prev->br_startoff + prev->br_blockcount < offset_fsb)
> +	    !base)
>  		alloc_blocks = mp->m_writeio_blocks;
>  	else
>  		alloc_blocks =
> -			xfs_iomap_prealloc_size(ip, offset, prev);
> +			xfs_iomap_prealloc_size(ip, offset, base);
>  
>  	return alloc_blocks;
>  }
> @@ -603,8 +600,15 @@ xfs_file_iomap_begin_delay(
>  	end_fsb = orig_end_fsb =
>  		min(XFS_B_TO_FSB(mp, offset + count), maxbytes_fsb);
>  
> -	if (eof)
> -		prealloc = xfs_iomap_prealloc(ip, offset, count, idx, &prev);
> +	if (eof) {
> +		xfs_extlen_t	base = 0;
> +
> +		/* use prev extent as base if contiguous */
> +		if (idx > 0 &&
> +		    prev.br_startoff + prev.br_blockcount < offset_fsb)

This should probably be: startoff + blockcount == offset_fsb

Brian

> +			base = prev.br_blockcount;
> +		prealloc = xfs_iomap_prealloc(ip, offset, count, base);
> +	}
>  	if (prealloc) {
>  		xfs_off_t	aligned_offset;
>  		xfs_extlen_t	align;
> 
> _______________________________________________
> xfs mailing list
> [email protected]
> http://oss.sgi.com/mailman/listinfo/xfs

_______________________________________________
xfs mailing list
[email protected]
http://oss.sgi.com/mailman/listinfo/xfs