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

From: Christoph Hellwig <hch@lst.de>
To: Brian Foster <bfoster@redhat.com>
Cc: Christoph Hellwig <hch@lst.de>, xfs@oss.sgi.com
Subject: Re: [PATCH 4/4] xfs: rewrite and optimize the delalloc write path
Date: Fri, 26 Aug 2016 16:33:44 +0200	[thread overview]
Message-ID: <20160826143344.GB21535@lst.de> (raw)
In-Reply-To: <20160825143708.GD25041@bfoster.bfoster>

On Thu, Aug 25, 2016 at 10:37:09AM -0400, Brian Foster wrote:
> On just skimming over this so far, I feel like this should be at least
> two patches, possibly 3:
> 
> - Kill xfs_bmapi_delay() and pull up associated bits to iomap().

As in just a move of code to xfs_iomap.c or also merged it with a
partіal copy of xfs_file_iomap_begin?  The first is trivial, but also
rather pointless.  The second is a bit more work, still very doable
but probably also not that useful as we're going to totally rewrite it
again in the next step.

> - Possibly separate out the part that moves iteration from the (former)
>   xfs_bmapi_delay() code up to the iomap code, if we can do so cleanly.

Well, the major point is that we get rid of the iteration as there isn't
any actual need for it.

> - Refactor/rework the preallocate logic.

But I guess I could do a pass that creates xfs_file_iomap_begin_delay
as in the new version except without the prealloc changes, and then
separate them out.  I don't quite see the point, though..
> I'm not necessarily against cleaning up/reworking the prealloc bits, but
> I'm not a huge fan of open coding all of this here in the iomap
> function. If nothing else, the indentation starts to make my eyes
> cross... could we retain one level of abstraction here for this hunk of
> logic that updates end_fsb?

We're only having three tabs of indentation.  I actually looked into
a helper for that whole block, but we'd need to pass:

ip, idx, prev, offset_fsb, offset, count, maxbytes_fsb

(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.

_______________________________________________
xfs mailing list
xfs@oss.sgi.com
http://oss.sgi.com/mailman/listinfo/xfs