All of lore.kernel.org
 help / color / mirror / Atom feed
From: Brian Foster <bfoster@redhat.com>
To: Christoph Hellwig <hch@lst.de>
Cc: xfs@oss.sgi.com
Subject: Re: [PATCH 4/4] xfs: rewrite and optimize the delalloc write path
Date: Fri, 26 Aug 2016 12:03:39 -0400	[thread overview]
Message-ID: <20160826160339.GC17728@bfoster.bfoster> (raw)
In-Reply-To: <20160826143344.GB21535@lst.de>

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

... which should probably be mentioned in the commit log description. In
any event, I think it's probably helpful enough to just separate out the
prealloc rework from everything else.

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

They don't appear to be related, for one. Second (and IMO), it makes it
notably easier to review and deal with any potential regressions down
the road. As it is, I made a more thorough pass through the code and I
didn't notice anything that looked wrong, but I'd still prefer to see
the changes separated out to be confident I didn't miss anything.

> > 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:
> 

Yeah, I suppose the indentation itself is not so much what stands out.
Looking again, I think it's more the combination of that and the
multi-line checks we're combining.

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

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

Brian

---8<---

diff --git a/fs/xfs/xfs_iomap.c b/fs/xfs/xfs_iomap.c
index 2b449f5..45e5268 100644
--- a/fs/xfs/xfs_iomap.c
+++ b/fs/xfs/xfs_iomap.c
@@ -488,6 +488,54 @@ check_writeio:
 	return alloc_blocks;
 }
 
+/*
+ * If we are doing a write at the end of the file and there are no allocations
+ * past this one, then extend the allocation out to the file system's write
+ * iosize.
+ *
+ * As an exception we don't do any preallocation at all if the file is smaller
+ * than the minimum preallocation and we are using the default dynamic
+ * preallocation scheme, as it is likely this is the only write to the file that
+ * is going to be done.
+ *
+ * We clean up any extra space left over when the file is closed in
+ * xfs_inactive().
+ */
+static int
+xfs_iomap_prealloc(
+	struct xfs_inode	*ip,
+	loff_t			offset,
+	loff_t			count,
+	xfs_extnum_t		idx,
+	struct xfs_bmbt_irec	*prev)
+{
+	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;
+
+	if (!(mp->m_flags & XFS_MOUNT_DFLT_IOSIZE) &&
+	    (XFS_ISIZE(ip) < XFS_FSB_TO_B(mp, mp->m_writeio_blocks)))
+		return 0;
+
+	/*
+	 * If an explicit allocsize is set, the file is small, or we
+	 * are writing behind a hole, then use the minimum 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)
+		alloc_blocks = mp->m_writeio_blocks;
+	else
+		alloc_blocks =
+			xfs_iomap_prealloc_size(ip, offset, prev);
+
+	return alloc_blocks;
+}
+
 static int
 xfs_file_iomap_begin_delay(
 	struct inode		*inode,
@@ -507,6 +555,7 @@ xfs_file_iomap_begin_delay(
 	struct xfs_bmbt_irec	got;
 	struct xfs_bmbt_irec	prev;
 	xfs_extnum_t		idx;
+	xfs_fsblock_t		prealloc = 0;
 
 	ASSERT(!XFS_IS_REALTIME_INODE(ip));
 	ASSERT(!xfs_get_extsz_hint(ip));
@@ -554,41 +603,14 @@ xfs_file_iomap_begin_delay(
 	end_fsb = orig_end_fsb =
 		min(XFS_B_TO_FSB(mp, offset + count), maxbytes_fsb);
 
-	/*
-	 * If we are doing a write at the end of the file and there are no
-	 * allocations past this one, then extend the allocation out to the
-	 * file system's write iosize.
-	 *
-	 * As an exception we don't do any preallocation at all if the file
-	 * is smaller than the minimum preallocation and we are using the
-	 * default dynamic preallocation scheme, as it is likely this is the
-	 * only write to the file that is going to be done.
-	 *
-	 * We clean up any extra space left over when the file is closed in
-	 * xfs_inactive().
-	 */
-	if (eof && offset + count > XFS_ISIZE(ip) &&
-	    ((mp->m_flags & XFS_MOUNT_DFLT_IOSIZE) ||
-	     XFS_ISIZE(ip) >= XFS_FSB_TO_B(mp, mp->m_writeio_blocks))) {
-		xfs_fsblock_t		alloc_blocks;
-		xfs_off_t		aligned_offset;
-		xfs_extlen_t		align;
-
-		/*
-		 * If an explicit allocsize is set, the file is small, or we
-		 * are writing behind a hole, then use the minimum 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)
-			alloc_blocks = mp->m_writeio_blocks;
-		else
-			alloc_blocks =
-				xfs_iomap_prealloc_size(ip, offset, &prev);
+	if (eof)
+		prealloc = xfs_iomap_prealloc(ip, offset, count, idx, &prev);
+	if (prealloc) {
+		xfs_off_t	aligned_offset;
+		xfs_extlen_t	align;
 
 		aligned_offset = XFS_WRITEIO_ALIGN(mp, offset + count - 1);
-		end_fsb = XFS_B_TO_FSBT(mp, aligned_offset) + alloc_blocks;
+		end_fsb = XFS_B_TO_FSBT(mp, aligned_offset) + prealloc;
 
 		align = xfs_eof_alignment(ip, 0);
 		if (align)

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

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

  reply	other threads:[~2016-08-26 16:03 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-08-21 21:51 iomap write fixes Christoph Hellwig
2016-08-21 21:51 ` [PATCH 1/4] xfs: move xfs_bmbt_to_iomap up Christoph Hellwig
2016-08-25 12:38   ` Brian Foster
2016-08-21 21:51 ` [PATCH 2/4] xfs: factor our a helper to calculate the EOF alignment Christoph Hellwig
2016-08-25 12:38   ` Brian Foster
2016-08-21 21:51 ` [PATCH 3/4] xfs: make xfs_inode_set_eofblocks_tag cheaper for the common case Christoph Hellwig
2016-08-25 12:38   ` Brian Foster
2016-08-26 14:26     ` Christoph Hellwig
2016-08-26 16:02       ` Brian Foster
2016-08-30 14:40         ` Christoph Hellwig
2016-08-30 23:03           ` Dave Chinner
2016-08-21 21:51 ` [PATCH 4/4] xfs: rewrite and optimize the delalloc write path Christoph Hellwig
2016-08-25 14:37   ` Brian Foster
2016-08-26 14:33     ` Christoph Hellwig
2016-08-26 16:03       ` Brian Foster [this message]
2016-08-26 16:07         ` Brian Foster
2016-08-30 14:44           ` Christoph Hellwig
2016-08-30 20:28           ` Brian Foster

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20160826160339.GC17728@bfoster.bfoster \
    --to=bfoster@redhat.com \
    --cc=hch@lst.de \
    --cc=xfs@oss.sgi.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.